Re: [PATCH RFC v2 04/16] vfio-user: connect vfio proxy to remote server

[John Johnson <john.g.johnson@oracle.com>]

> 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