| Message ID | 489671ef49381437a03917a87dc143dd9fc90559.1633929457.git.jag.raman@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio-user server in QEMU | expand |
On Mon, Oct 11, 2021 at 01:31:11AM -0400, Jagannathan Raman wrote: > Setup a handler to run vfio-user context. The context is driven by > messages to the file descriptor associated with it - get the fd for > the context and hook up the handler with it > > Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com> > Signed-off-by: John G Johnson <john.g.johnson@oracle.com> > Signed-off-by: Jagannathan Raman <jag.raman@oracle.com> > --- > hw/remote/vfio-user-obj.c | 75 ++++++++++++++++++++++++++++++++++++++- > 1 file changed, 74 insertions(+), 1 deletion(-) > > diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c > index 7ce4e5b256..05f7fff19c 100644 > --- a/hw/remote/vfio-user-obj.c > +++ b/hw/remote/vfio-user-obj.c > @@ -42,6 +42,7 @@ > #include "qapi/error.h" > #include "qapi/qapi-visit-sockets.h" > #include "qemu/notify.h" > +#include "qemu/thread.h" > #include "sysemu/sysemu.h" > #include "libvfio-user.h" > #include "hw/qdev-core.h" > @@ -72,6 +73,8 @@ struct VfuObject { > vfu_ctx_t *vfu_ctx; > > PCIDevice *pci_dev; > + > + int vfu_poll_fd; > }; > > static void vfu_object_set_socket(Object *obj, Visitor *v, const char *name, > @@ -105,6 +108,58 @@ static void vfu_object_set_device(Object *obj, const char *str, Error **errp) > trace_vfu_prop("device", str); > } > > +static void vfu_object_ctx_run(void *opaque) > +{ > + VfuObject *o = opaque; > + int ret = -1; > + > + while (ret != 0) { > + ret = vfu_run_ctx(o->vfu_ctx); > + if (ret < 0) { > + if (errno == EINTR) { > + continue; > + } else if (errno == ENOTCONN) { > + qemu_set_fd_handler(o->vfu_poll_fd, NULL, NULL, NULL); > + o->vfu_poll_fd = -1; > + object_unparent(OBJECT(o)); > + break; > + } else { > + error_setg(&error_abort, "vfu: Failed to run device %s - %s", > + o->device, strerror(errno)); > + break; > + } The libvfio-user.h doc comments say this function can return -1 (EAGAIN) when LIBVFIO_USER_FLAG_ATTACH_NB was used. Is the doc comment wrong since this patch seems to rely on vfu_run_ctx() returning 0 when there are no more commands to process? > + } > + } > +} > + > +static void vfu_object_attach_ctx(void *opaque) > +{ > + VfuObject *o = opaque; > + int ret; > + > + qemu_set_fd_handler(o->vfu_poll_fd, NULL, NULL, NULL); > + o->vfu_poll_fd = -1; > + > +retry_attach: > + ret = vfu_attach_ctx(o->vfu_ctx); > + if (ret < 0 && (errno == EAGAIN || errno == EWOULDBLOCK)) { > + goto retry_attach; Can we wait for the poll fd to become readable instead of spinning? I don't know the libvfio-user APIs so I'm not sure, but it would be nice to avoid a busy loop. > + } else if (ret < 0) { > + error_setg(&error_abort, error_abort is not appropriate for hotplugged objects, where it's less likely that the user wants to terminate the process when a failure occurs. If asynchronous errors occur then QMP Events should be raised so the QMP client gets notified and can deal with them. > + "vfu: Failed to attach device %s to context - %s", > + o->device, strerror(errno)); > + return; > + } > + > + o->vfu_poll_fd = vfu_get_poll_fd(o->vfu_ctx); > + if (o->vfu_poll_fd < 0) { > + error_setg(&error_abort, "vfu: Failed to get poll fd %s", o->device); > + return; > + } > + > + qemu_set_fd_handler(o->vfu_poll_fd, vfu_object_ctx_run, NULL, o); > +} > + > /* > * vfio-user-server depends on the availability of the 'socket' and 'device' > * properties. It also depends on devices instantiated in QEMU. These > @@ -120,7 +175,8 @@ static void vfu_object_machine_done(Notifier *notifier, void *data) > vfu_pci_type_t pci_type = VFU_PCI_TYPE_CONVENTIONAL; > int ret; > > - o->vfu_ctx = vfu_create_ctx(VFU_TRANS_SOCK, o->socket->u.q_unix.path, 0, > + o->vfu_ctx = vfu_create_ctx(VFU_TRANS_SOCK, o->socket->u.q_unix.path, > + LIBVFIO_USER_FLAG_ATTACH_NB, > o, VFU_DEV_TYPE_PCI); > if (o->vfu_ctx == NULL) { > error_setg(&error_abort, "vfu: Failed to create context - %s", > @@ -152,6 +208,21 @@ static void vfu_object_machine_done(Notifier *notifier, void *data) > o->device, strerror(errno)); > return; > } > + > + ret = vfu_realize_ctx(o->vfu_ctx); > + if (ret < 0) { > + error_setg(&error_abort, "vfu: Failed to realize device %s- %s", > + o->device, strerror(errno)); > + return; > + } > + > + o->vfu_poll_fd = vfu_get_poll_fd(o->vfu_ctx); > + if (o->vfu_poll_fd < 0) { > + error_setg(&error_abort, "vfu: Failed to get poll fd %s", o->device); > + return; > + } > + > + qemu_set_fd_handler(o->vfu_poll_fd, vfu_object_attach_ctx, NULL, o); > } > > static void vfu_object_init(Object *obj) > @@ -178,6 +249,8 @@ static void vfu_object_init(Object *obj) > > o->machine_done.notify = vfu_object_machine_done; > qemu_add_machine_init_done_notifier(&o->machine_done); > + > + o->vfu_poll_fd = -1; This must be done before the qemu_add_machine_init_done_notifier() call in the hotplug case. qemu_add_machine_init_done_notifier() invokes the notifier callback immediately if machine init was already done.
On Wed, Oct 27, 2021 at 05:21:30PM +0100, Stefan Hajnoczi wrote: > > +static void vfu_object_ctx_run(void *opaque) > > +{ > > + VfuObject *o = opaque; > > + int ret = -1; > > + > > + while (ret != 0) { > > + ret = vfu_run_ctx(o->vfu_ctx); > > + if (ret < 0) { > > + if (errno == EINTR) { > > + continue; > > + } else if (errno == ENOTCONN) { > > + qemu_set_fd_handler(o->vfu_poll_fd, NULL, NULL, NULL); > > + o->vfu_poll_fd = -1; > > + object_unparent(OBJECT(o)); > > + break; > > + } else { > > + error_setg(&error_abort, "vfu: Failed to run device %s - %s", > > + o->device, strerror(errno)); > > + break; > > + } > > The libvfio-user.h doc comments say this function can return -1 (EAGAIN) > when LIBVFIO_USER_FLAG_ATTACH_NB was used. Is the doc comment wrong > since this patch seems to rely on vfu_run_ctx() returning 0 when there > are no more commands to process? * @returns the number of requests processed (0 or more); or -1 on error, * with errno set as follows: So in fact it does return 0. The EAGAIN line needs removing; I'll fix it shortly. > > +static void vfu_object_attach_ctx(void *opaque) > > +{ > > + VfuObject *o = opaque; > > + int ret; > > + > > + qemu_set_fd_handler(o->vfu_poll_fd, NULL, NULL, NULL); > > + o->vfu_poll_fd = -1; > > + > > +retry_attach: > > + ret = vfu_attach_ctx(o->vfu_ctx); > > + if (ret < 0 && (errno == EAGAIN || errno == EWOULDBLOCK)) { > > + goto retry_attach; > > Can we wait for the poll fd to become readable instead of spinning? I > don't know the libvfio-user APIs so I'm not sure, but it would be nice > to avoid a busy loop. At this point, ->vfu_poll_fd is a listening socket, no reason why qemu couldn't epoll on it. regards john
diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c index 7ce4e5b256..05f7fff19c 100644 --- a/hw/remote/vfio-user-obj.c +++ b/hw/remote/vfio-user-obj.c @@ -42,6 +42,7 @@ #include "qapi/error.h" #include "qapi/qapi-visit-sockets.h" #include "qemu/notify.h" +#include "qemu/thread.h" #include "sysemu/sysemu.h" #include "libvfio-user.h" #include "hw/qdev-core.h" @@ -72,6 +73,8 @@ struct VfuObject { vfu_ctx_t *vfu_ctx; PCIDevice *pci_dev; + + int vfu_poll_fd; }; static void vfu_object_set_socket(Object *obj, Visitor *v, const char *name, @@ -105,6 +108,58 @@ static void vfu_object_set_device(Object *obj, const char *str, Error **errp) trace_vfu_prop("device", str); } +static void vfu_object_ctx_run(void *opaque) +{ + VfuObject *o = opaque; + int ret = -1; + + while (ret != 0) { + ret = vfu_run_ctx(o->vfu_ctx); + if (ret < 0) { + if (errno == EINTR) { + continue; + } else if (errno == ENOTCONN) { + qemu_set_fd_handler(o->vfu_poll_fd, NULL, NULL, NULL); + o->vfu_poll_fd = -1; + object_unparent(OBJECT(o)); + break; + } else { + error_setg(&error_abort, "vfu: Failed to run device %s - %s", + o->device, strerror(errno)); + break; + } + } + } +} + +static void vfu_object_attach_ctx(void *opaque) +{ + VfuObject *o = opaque; + int ret; + + qemu_set_fd_handler(o->vfu_poll_fd, NULL, NULL, NULL); + o->vfu_poll_fd = -1; + +retry_attach: + ret = vfu_attach_ctx(o->vfu_ctx); + if (ret < 0 && (errno == EAGAIN || errno == EWOULDBLOCK)) { + goto retry_attach; + } else if (ret < 0) { + error_setg(&error_abort, + "vfu: Failed to attach device %s to context - %s", + o->device, strerror(errno)); + return; + } + + o->vfu_poll_fd = vfu_get_poll_fd(o->vfu_ctx); + if (o->vfu_poll_fd < 0) { + error_setg(&error_abort, "vfu: Failed to get poll fd %s", o->device); + return; + } + + qemu_set_fd_handler(o->vfu_poll_fd, vfu_object_ctx_run, NULL, o); +} + /* * vfio-user-server depends on the availability of the 'socket' and 'device' * properties. It also depends on devices instantiated in QEMU. These @@ -120,7 +175,8 @@ static void vfu_object_machine_done(Notifier *notifier, void *data) vfu_pci_type_t pci_type = VFU_PCI_TYPE_CONVENTIONAL; int ret; - o->vfu_ctx = vfu_create_ctx(VFU_TRANS_SOCK, o->socket->u.q_unix.path, 0, + o->vfu_ctx = vfu_create_ctx(VFU_TRANS_SOCK, o->socket->u.q_unix.path, + LIBVFIO_USER_FLAG_ATTACH_NB, o, VFU_DEV_TYPE_PCI); if (o->vfu_ctx == NULL) { error_setg(&error_abort, "vfu: Failed to create context - %s", @@ -152,6 +208,21 @@ static void vfu_object_machine_done(Notifier *notifier, void *data) o->device, strerror(errno)); return; } + + ret = vfu_realize_ctx(o->vfu_ctx); + if (ret < 0) { + error_setg(&error_abort, "vfu: Failed to realize device %s- %s", + o->device, strerror(errno)); + return; + } + + o->vfu_poll_fd = vfu_get_poll_fd(o->vfu_ctx); + if (o->vfu_poll_fd < 0) { + error_setg(&error_abort, "vfu: Failed to get poll fd %s", o->device); + return; + } + + qemu_set_fd_handler(o->vfu_poll_fd, vfu_object_attach_ctx, NULL, o); } static void vfu_object_init(Object *obj) @@ -178,6 +249,8 @@ static void vfu_object_init(Object *obj) o->machine_done.notify = vfu_object_machine_done; qemu_add_machine_init_done_notifier(&o->machine_done); + + o->vfu_poll_fd = -1; } static void vfu_object_finalize(Object *obj)