> On Aug 24, 2021, at 7:15 AM, Stefan Hajnoczi <stefa...@redhat.com> wrote: > > On Mon, Aug 16, 2021 at 09:42:37AM -0700, Elena Ufimtseva wrote: >> @@ -3361,13 +3362,35 @@ static void vfio_user_pci_realize(PCIDevice *pdev, >> Error **errp) >> VFIOUserPCIDevice *udev = VFIO_USER_PCI(pdev); >> VFIOPCIDevice *vdev = VFIO_PCI_BASE(pdev); >> VFIODevice *vbasedev = &vdev->vbasedev; >> + SocketAddress addr; >> + VFIOProxy *proxy; >> + Error *err = NULL; >> >> + /* >> + * TODO: make option parser understand SocketAddress >> + * and use that instead of having scaler options > > s/scaler/scalar/ >
OK >> +VFIOProxy *vfio_user_connect_dev(SocketAddress *addr, Error **errp) >> +{ >> + VFIOProxy *proxy; >> + QIOChannelSocket *sioc; >> + QIOChannel *ioc; >> + char *sockname; >> + >> + if (addr->type != SOCKET_ADDRESS_TYPE_UNIX) { >> + error_setg(errp, "vfio_user_connect - bad address family"); >> + return NULL; >> + } >> + sockname = addr->u.q_unix.path; >> + >> + sioc = qio_channel_socket_new(); >> + ioc = QIO_CHANNEL(sioc); >> + if (qio_channel_socket_connect_sync(sioc, addr, errp)) { >> + object_unref(OBJECT(ioc)); >> + return NULL; >> + } >> + qio_channel_set_blocking(ioc, true, NULL); >> + >> + proxy = g_malloc0(sizeof(VFIOProxy)); >> + proxy->sockname = sockname; > > sockname is addr->u.q_unix.path, so there's an assumption that the > lifetime of the addr argument is at least as long as the proxy object's > lifetime. This doesn't seem to be the case in vfio_user_pci_realize() > since the SocketAddress variable is declared on the stack. > > I suggest making SocketAddress *addr const so it's obvious that this > function just reads it (doesn't take ownership of the pointer) and > copying the UNIX domain socket path with g_strdup() to avoid the > dangling pointer. > OK >> + proxy->ioc = ioc; >> + proxy->flags = VFIO_PROXY_CLIENT; >> + proxy->state = VFIO_PROXY_CONNECTED; >> + qemu_cond_init(&proxy->close_cv); >> + >> + if (vfio_user_iothread == NULL) { >> + vfio_user_iothread = iothread_create("VFIO user", errp); >> + } > > Why is a dedicated IOThread needed for VFIO user? > It seemed the best model for inbound message processing. Most messages are replies, so the receiver will either signal a thread waiting the reply or report any errors from the server if there is no waiter. None of this requires the BQL. If the message is a request - which currently only happens if device DMA targets guest memory that wasn’t mmap()d by QEMU or if the ’secure-dma’ option is used - then the receiver can then acquire BQL so it can call the VFIO code to handle the request. >> + >> + qemu_mutex_init(&proxy->lock); >> + QTAILQ_INIT(&proxy->free); >> + QTAILQ_INIT(&proxy->pending); >> + QLIST_INSERT_HEAD(&vfio_user_sockets, proxy, next); >> + >> + return proxy; >> +} >> + > > /* Called with the BQL */ OK >> +void vfio_user_disconnect(VFIOProxy *proxy) >> +{ >> + VFIOUserReply *r1, *r2; >> + >> + qemu_mutex_lock(&proxy->lock); >> + >> + /* our side is quitting */ >> + if (proxy->state == VFIO_PROXY_CONNECTED) { >> + vfio_user_shutdown(proxy); >> + if (!QTAILQ_EMPTY(&proxy->pending)) { >> + error_printf("vfio_user_disconnect: outstanding requests\n"); >> + } >> + } >> + object_unref(OBJECT(proxy->ioc)); >> + proxy->ioc = NULL; >> + >> + proxy->state = VFIO_PROXY_CLOSING; >> + QTAILQ_FOREACH_SAFE(r1, &proxy->pending, next, r2) { >> + qemu_cond_destroy(&r1->cv); >> + QTAILQ_REMOVE(&proxy->pending, r1, next); >> + g_free(r1); >> + } >> + QTAILQ_FOREACH_SAFE(r1, &proxy->free, next, r2) { >> + qemu_cond_destroy(&r1->cv); >> + QTAILQ_REMOVE(&proxy->free, r1, next); >> + g_free(r1); >> + } >> + >> + /* >> + * Make sure the iothread isn't blocking anywhere >> + * with a ref to this proxy by waiting for a BH >> + * handler to run after the proxy fd handlers were >> + * deleted above. >> + */ >> + proxy->close_wait = 1; > > Please use true. '1' is shorter but it's less obvious to the reader (I > had to jump to the definition to check whether this field was bool or > int). > I assume this is also true for the other boolean struct members I’ve added. >> + aio_bh_schedule_oneshot(iothread_get_aio_context(vfio_user_iothread), >> + vfio_user_cb, proxy); >> + >> + /* drop locks so the iothread can make progress */ >> + qemu_mutex_unlock_iothread(); > > Why does the BQL needs to be dropped so vfio_user_iothread can make > progress? > See above. Acquiring BQL by the iothread is rare, but I have to handle the case where a disconnect is concurrent with an incoming request message that is waiting for the BQL. See the proxy state check after BQL is acquired in vfio_user_recv() >> + qemu_cond_wait(&proxy->close_cv, &proxy->lock); JJ