On Wed, Sep 6, 2023 at 7:56 AM Philippe Mathieu-Daudé <phi...@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 <kra...@redhat.com> > > Signed-off-by: Albert Esteve <aest...@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-s3...@nongnu.org > > > > +virtio-dmabuf > > +M: Albert Esteve <aest...@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 <stefa...@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 <aest...@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 <aest...@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. > >
