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?
So after re-reading your comment and looking for more examples in the codebase, I see
it is not uncommon to have a named struct also typedef in the same declaration.
So I will typedef, but not make it anonymous, same for the enum.
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.