[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH RFC v2 04/16] vfio-user: connect vfio proxy to remote server
From: |
John Johnson |
Subject: |
Re: [PATCH RFC v2 04/16] vfio-user: connect vfio proxy to remote server |
Date: |
Mon, 30 Aug 2021 03:00:37 +0000 |
> On Aug 24, 2021, at 7:15 AM, Stefan Hajnoczi <stefanha@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
- [PATCH RFC v2 00/16] vfio-user implementation, Elena Ufimtseva, 2021/08/16
- [PATCH RFC v2 04/16] vfio-user: connect vfio proxy to remote server, Elena Ufimtseva, 2021/08/16
- [PATCH RFC v2 03/16] vfio-user: Define type vfio_user_pci_dev_info, Elena Ufimtseva, 2021/08/16
- [PATCH RFC v2 07/16] vfio-user: get device info, Elena Ufimtseva, 2021/08/16
- [PATCH RFC v2 06/16] vfio-user: negotiate version with remote server, Elena Ufimtseva, 2021/08/16
- [PATCH RFC v2 05/16] vfio-user: define VFIO Proxy and communication functions, Elena Ufimtseva, 2021/08/16