Re: [PATCH v5 3/4] vhost-user: add shared_object msg

[Philippe Mathieu-Daudé]

On 2/8/23 11:08, Albert Esteve wrote:
> Add three new vhost-user protocol
> `VHOST_USER_BACKEND_SHARED_OBJECT_* messages`.
> These new messages are sent from vhost-user
> back-ends to interact with the virtio-dmabuf
> table in order to add or remove themselves as
> virtio exporters, or lookup for virtio dma-buf
> shared objects.
>
> The action taken in the front-end depends
> on the type stored in the virtio shared
> object hash table.
>
> When the table holds a pointer to a vhost
> backend for a given UUID, the front-end sends
> a VHOST_USER_GET_SHARED_OBJECT to the
> backend holding the shared object.
>
> In the libvhost-user library we need to add
> helper functions to allow sending messages to
> interact with the virtio shared objects
> hash table.
>
> The messages can only be sent after successfully
> negotiating a new VHOST_USER_PROTOCOL_F_SHARED_OBJECT
> vhost-user protocol feature bit.
>
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> ---
>   docs/interop/vhost-user.rst               |  57 ++++++++
>   hw/virtio/vhost-user.c                    | 166 ++++++++++++++++++++++
>   include/hw/virtio/vhost-backend.h         |   3 +
>   subprojects/libvhost-user/libvhost-user.c | 118 +++++++++++++++
>   subprojects/libvhost-user/libvhost-user.h |  55 ++++++-
>   5 files changed, 398 insertions(+), 1 deletion(-)


> +static bool
> +vhost_user_backend_send_dmabuf_fd(QIOChannel *ioc, VhostUserHeader *hdr,
> +                                  VhostUserPayload *payload)
> +{
> +    Error *local_err = NULL;
> +    struct iovec iov[2];
> +
> +    if (hdr->flags & VHOST_USER_NEED_REPLY_MASK) {
> +        hdr->flags &= ~VHOST_USER_NEED_REPLY_MASK;
> +    }
> +    hdr->flags |= VHOST_USER_REPLY_MASK;
> +
> +    hdr->size = sizeof(payload->u64);
> +
> +    iov[0].iov_base = hdr;
> +    iov[0].iov_len = VHOST_USER_HDR_SIZE;
> +    iov[1].iov_base = payload;
> +    iov[1].iov_len = hdr->size;
> +
> +    if (qio_channel_writev_all(ioc, iov, ARRAY_SIZE(iov), &local_err)) {
> +        error_report_err(local_err);

This function could have a 'Error **errp' parameter to propagate
the error to the caller.

> +        return false;
> +    }
> +    return true;
> +}
> +
> +static bool
> +vhost_user_backend_send_dmabuf_fd(QIOChannel *ioc, VhostUserHeader *hdr,
> +                                  VhostUserPayload *payload)
> +{
> +    hdr->size = sizeof(payload->u64);
> +    return vhost_user_send_resp(ioc, hdr, payload);
> +}

I'm confused by having two vhost_user_backend_send_dmabuf_fd() functions
with different body...

> +int vhost_user_get_shared_object(struct vhost_dev *dev, unsigned char *uuid,
> +                                 int *dmabuf_fd)
> +{
> +    struct vhost_user *u = dev->opaque;
> +    CharBackend *chr = u->user->chr;
> +    int ret;
> +    VhostUserMsg msg = {
> +        .hdr.request = VHOST_USER_GET_SHARED_OBJECT,
> +        .hdr.flags = VHOST_USER_VERSION,
> +    };
> +    memcpy(msg.payload.object.uuid, uuid, sizeof(msg.payload.object.uuid));
> +
> +    ret = vhost_user_write(dev, &msg, NULL, 0);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    ret = vhost_user_read(dev, &msg);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    if (msg.hdr.request != VHOST_USER_GET_SHARED_OBJECT) {
> +        error_report("Received unexpected msg type. "
> +                     "Expected %d received %d",
> +                     VHOST_USER_GET_SHARED_OBJECT, msg.hdr.request);
> +        return -EPROTO;
> +    }
> +
> +    *dmabuf_fd = qemu_chr_fe_get_msgfd(chr);
> +    if (*dmabuf_fd < 0) {
> +        error_report("Failed to get dmabuf fd");
> +        return -EIO;
> +    }
> +
> +    return 0;
> +}
> +
> +static int
> +vhost_user_backend_handle_shared_object_lookup(struct vhost_user *u,
> +                                               QIOChannel *ioc,
> +                                               VhostUserHeader *hdr,
> +                                               VhostUserPayload *payload)

Also propagate a 'Error **errp'.

> +{
> +    QemuUUID uuid;
> +    CharBackend *chr = u->user->chr;
> +    int dmabuf_fd = -1;
> +    int fd_num = 0;
> +
> +    memcpy(uuid.data, payload->object.uuid, sizeof(payload->object.uuid));
> +
> +    payload->u64 = 0;
> +    switch (virtio_object_type(&uuid)) {
> +        case TYPE_DMABUF:
> +            dmabuf_fd = virtio_lookup_dmabuf(&uuid);
> +            break;
> +        case TYPE_VHOST_DEV:
> +        {
> +            struct vhost_dev *dev = virtio_lookup_vhost_device(&uuid);
> +            if (dev == NULL) {
> +                payload->u64 = -EINVAL;
> +                break;
> +            }
> +            int ret = vhost_user_get_shared_object(dev, uuid.data, &dmabuf_fd);
> +            if (ret < 0) {
> +                payload->u64 = ret;
> +            }
> +            break;
> +        }
> +        case TYPE_INVALID:
> +            payload->u64 = -EINVAL;
> +            break;
> +    }
> +
> +    if (dmabuf_fd != -1) {
> +        fd_num++;
> +    }
> +
> +    if (qemu_chr_fe_set_msgfds(chr, &dmabuf_fd, fd_num) < 0) {
> +        error_report("Failed to set msg fds.");
> +        payload->u64 = -EINVAL;
> +    }
> +
> +    if (!vhost_user_backend_send_dmabuf_fd(ioc, hdr, payload)) {
> +        error_report("Failed to write response msg.");
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}