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

[Philippe Mathieu-Daudé]

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'.

Regards,

Phil.