Re: [PATCH v5 2/4] virtio-dmabuf: introduce virtio-dmabuf

[Albert Esteve <aesteve@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 8797 bytes --]

On Wed, Sep 6, 2023 at 7:56 AM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> Hi Albert,
>
> On 2/8/23 11:08, Albert Esteve wrote:
> > This API manages objects (in this iteration,
> > dmabuf fds) that can be shared along different
> > virtio devices, associated to a UUID.
> >
> > The API allows the different devices to add,
> > remove and/or retrieve the objects by simply
> > invoking the public functions that reside in the
> > virtio-dmabuf file.
> >
> > For vhost backends, the API stores the pointer
> > to the backend holding the object.
> >
> > Suggested-by: Gerd Hoffmann <kraxel@redhat.com>
> > Signed-off-by: Albert Esteve <aesteve@redhat.com>
> > ---
> >   MAINTAINERS                       |   7 ++
> >   hw/display/meson.build            |   1 +
> >   hw/display/virtio-dmabuf.c        | 136 +++++++++++++++++++++++++++++
> >   include/hw/virtio/virtio-dmabuf.h | 103 ++++++++++++++++++++++
> >   tests/unit/meson.build            |   1 +
> >   tests/unit/test-virtio-dmabuf.c   | 137 ++++++++++++++++++++++++++++++
> >   6 files changed, 385 insertions(+)
> >   create mode 100644 hw/display/virtio-dmabuf.c
> >   create mode 100644 include/hw/virtio/virtio-dmabuf.h
> >   create mode 100644 tests/unit/test-virtio-dmabuf.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 12e59b6b27..cd8487785a 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2158,6 +2158,13 @@ T: git https://gitlab.com/cohuck/qemu.git
> s390-next
> >   T: git https://github.com/borntraeger/qemu.git s390-next
> >   L: qemu-s390x@nongnu.org
> >
> > +virtio-dmabuf
> > +M: Albert Esteve <aesteve@redhat.com>
> > +S: Supported
> > +F: hw/display/virtio-dmabuf.c
> > +F: include/hw/virtio/virtio-dmabuf.h
> > +F: tests/unit/test-virtio-dmabuf.c
> > +
> >   virtiofs
> >   M: Stefan Hajnoczi <stefanha@redhat.com>
> >   S: Supported
> > diff --git a/hw/display/meson.build b/hw/display/meson.build
> > index 413ba4ab24..05619c6968 100644
> > --- a/hw/display/meson.build
> > +++ b/hw/display/meson.build
> > @@ -37,6 +37,7 @@ system_ss.add(when: 'CONFIG_MACFB', if_true:
> files('macfb.c'))
> >   system_ss.add(when: 'CONFIG_NEXTCUBE', if_true: files('next-fb.c'))
> >
> >   system_ss.add(when: 'CONFIG_VGA', if_true: files('vga.c'))
> > +system_ss.add(when: 'CONFIG_VIRTIO', if_true: files('virtio-dmabuf.c'))
> >
> >   if (config_all_devices.has_key('CONFIG_VGA_CIRRUS') or
> >       config_all_devices.has_key('CONFIG_VGA_PCI') or
> > diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
> > new file mode 100644
> > index 0000000000..e852c71ba9
> > --- /dev/null
> > +++ b/hw/display/virtio-dmabuf.c
> > @@ -0,0 +1,136 @@
> > +/*
> > + * Virtio Shared dma-buf
> > + *
> > + * Copyright Red Hat, Inc. 2023
> > + *
> > + * Authors:
> > + *     Albert Esteve <aesteve@redhat.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or
> later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "hw/virtio/virtio-dmabuf.h"
> > +
> > +
> > +static GMutex lock;
> > +static GHashTable *resource_uuids;
> > +
> > +/*
> > + * uuid_equal_func: wrapper for UUID is_equal function to
> > + * satisfy g_hash_table_new expected parameters signatures.
> > + */
> > +static int uuid_equal_func(const void *lhv, const void *rhv)
> > +{
> > +    return qemu_uuid_is_equal(lhv, rhv);
> > +}
> > +
> > +static bool virtio_add_resource(QemuUUID *uuid, struct
> VirtioSharedObject *value)
>
> Per QEMU coding style we use typedefs, so "VirtioSharedObject" here.
>
> > +{
> > +    if (resource_uuids == NULL) {
> > +        resource_uuids = g_hash_table_new_full(
> > +            qemu_uuid_hash, uuid_equal_func, NULL, g_free);
> > +    }
> > +    if (g_hash_table_lookup(resource_uuids, uuid) != NULL) {
> > +        return false;
> > +    }
> > +
> > +    return g_hash_table_insert(resource_uuids, uuid, value);
>
> Hmm shouldn't this function take the lock to access resource_uuids?
>
> > +}
> > +
> > +static gpointer virtio_lookup_resource(const QemuUUID *uuid)
> > +{
> > +    if (resource_uuids == NULL) {
> > +        return NULL;
> > +    }
> > +
> > +    return g_hash_table_lookup(resource_uuids, uuid);
>
> Ditto.
>
> Here you can directly return the casted type (VirtioSharedObject *),
> since a plain gpointer isn't really used / useful.
>
> > +}
> > +
> > +bool virtio_add_dmabuf(QemuUUID *uuid, int udmabuf_fd)
> > +{
> > +    bool result;
> > +    struct VirtioSharedObject *vso;
> > +    if (udmabuf_fd < 0) {
> > +        return false;
> > +    }
> > +    vso = g_new0(struct VirtioSharedObject, 1);
>
> s/g_new0/g_new/
>
> > +    g_mutex_lock(&lock);
> > +    vso->type = TYPE_DMABUF;
> > +    vso->value = GINT_TO_POINTER(udmabuf_fd);
> > +    result = virtio_add_resource(uuid, vso);
> > +    g_mutex_unlock(&lock);
> > +
> > +    return result;
> > +}
> > +
> > +bool virtio_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev)
> > +{
> > +    bool result;
> > +    struct VirtioSharedObject *vso;
> > +    if (dev == NULL) {
> > +        return false;
> > +    }
> > +    vso = g_new0(struct VirtioSharedObject, 1);
> > +    g_mutex_lock(&lock);
> > +    vso->type = TYPE_VHOST_DEV;
> > +    vso->value = dev;
> > +    result = virtio_add_resource(uuid, vso);
>
> Ah, you lock here... I'd rather do it in the callee.
>
> > +    g_mutex_unlock(&lock);
> > +
> > +    return result;
> > +}
> > +
> > +bool virtio_remove_resource(const QemuUUID *uuid)
> > +{
> > +    bool result;
> > +    g_mutex_lock(&lock);
> > +    result = g_hash_table_remove(resource_uuids, uuid);
> > +    g_mutex_unlock(&lock);
>
> virtio_remove_resource() correctly locks. For API parity,
> virtio_add_resource() should too.
>
> > +
> > +    return result;
> > +}
> > +
> > +static struct VirtioSharedObject *get_shared_object(const QemuUUID
> *uuid)
> > +{
> > +    g_mutex_lock(&lock);
> > +    gpointer lookup_res = virtio_lookup_resource(uuid);
> > +    g_mutex_unlock(&lock);
> > +    return (struct VirtioSharedObject*) lookup_res;
>
> See earlier, this function can be merged with virtio_lookup_resource().
>
> > +}
> > +
> > +int virtio_lookup_dmabuf(const QemuUUID *uuid)
> > +{
> > +    struct VirtioSharedObject *vso = get_shared_object(uuid);
> > +    if (vso == NULL) {
> > +        return -1;
> > +    }
> > +    assert(vso->type == TYPE_DMABUF);
> > +    return GPOINTER_TO_INT(vso->value);
> > +}
> > +
> > +struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid)
> > +{
> > +    struct VirtioSharedObject *vso = get_shared_object(uuid);
> > +    if (vso == NULL) {
> > +        return NULL;
> > +    }
> > +    assert(vso->type == TYPE_VHOST_DEV);
> > +    return (struct vhost_dev *) vso->value;
> > +}
> > +
> > +enum SharedObjectType virtio_object_type(const QemuUUID *uuid)
> > +{
> > +    struct VirtioSharedObject *vso = get_shared_object(uuid);
> > +    if (vso == NULL) {
> > +        return TYPE_INVALID;
> > +    }
> > +    return vso->type;
> > +}
> > +
> > +void virtio_free_resources(void)
> > +{
> > +    g_hash_table_destroy(resource_uuids);
>
> Lock?
>
> > +    /* Reference count shall be 0 after the implicit unref on destroy */
> > +    resource_uuids = NULL;
> > +}
> > diff --git a/include/hw/virtio/virtio-dmabuf.h
> b/include/hw/virtio/virtio-dmabuf.h
> > new file mode 100644
> > index 0000000000..536e622555
> > --- /dev/null
> > +++ b/include/hw/virtio/virtio-dmabuf.h
> > @@ -0,0 +1,103 @@
> > +/*
> > + * Virtio Shared dma-buf
> > + *
> > + * Copyright Red Hat, Inc. 2023
> > + *
> > + * Authors:
> > + *     Albert Esteve <aesteve@redhat.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#ifndef VIRTIO_DMABUF_H
> > +#define VIRTIO_DMABUF_H
> > +
> > +#include "qemu/osdep.h"
> > +
> > +#include <glib.h>
> > +#include "qemu/uuid.h"
> > +#include "vhost.h"
> > +
> > +enum SharedObjectType {
> > +    TYPE_INVALID = 0,
> > +    TYPE_DMABUF,
> > +    TYPE_VHOST_DEV,
> > +};
> > +
>
> Please declare a
>
> typedef
>
> > +struct VirtioSharedObject {
> > +    enum SharedObjectType type;
> > +    gpointer value;
> > +};
>
> VirtioSharedObject;
>
> and use it instead of 'struct VirtioSharedObject'.
>
>
You mean making the struct anonymous and typedefing?
Should I do the same with the enum? In other files I see enums are typedef
too, but not anonymous (e.g., block/qcow2.h).
So I could do the same here.

For the rest... Ack!


> Regards,
>
> Phil.
>
>

[-- Attachment #2: Type: text/html, Size: 11652 bytes --]