Re: [PATCH v6 2/3] virtio-dmabuf: introduce virtio-dmabuf

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 6/9/23 13:15, 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        | 134 +++++++++++++++++++++++++++++
>   include/hw/virtio/virtio-dmabuf.h | 103 ++++++++++++++++++++++
>   tests/unit/meson.build            |   1 +
>   tests/unit/test-virtio-dmabuf.c   | 137 ++++++++++++++++++++++++++++++
>   6 files changed, 383 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 3b29568ed4..fb0f7b823f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2150,6 +2150,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..268ffe81ec
> --- /dev/null
> +++ b/hw/display/virtio-dmabuf.c
> @@ -0,0 +1,134 @@
> +/*
> + * 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, VirtioSharedObject *value)
> +{
> +    bool result;
> +    g_mutex_lock(&lock);
> +    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) {
> +        g_mutex_unlock(&lock);
> +        return false;
> +    }
> +    result = g_hash_table_insert(resource_uuids, uuid, value);
> +    g_mutex_unlock(&lock);
> +
> +    return result;
> +}

Alternatively same logic, but simpler / safer:

static bool virtio_add_resource(...)
{
     bool result = false;

     g_mutex_lock(&lock);
     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) {
        result = g_hash_table_insert(resource_uuids, uuid, value);
     }
     g_mutex_unlock(&lock);

     return result;
}

> +static VirtioSharedObject *get_shared_object(const QemuUUID *uuid)
> +{
> +    g_mutex_lock(&lock);
> +    if (resource_uuids == NULL) {
> +        g_mutex_unlock(&lock);
> +        return NULL;
> +    }
> +    gpointer lookup_res = g_hash_table_lookup(resource_uuids, uuid);
> +    g_mutex_unlock(&lock);
> +    return (VirtioSharedObject*) lookup_res;
> +}

Similarly:

static VirtioSharedObject *get_shared_object(const QemuUUID *uuid)
{
     gpointer lookup_res = NULL;

     g_mutex_lock(&lock);
     if (resource_uuids != NULL) {
         lookup_res = g_hash_table_lookup(resource_uuids, uuid);
     }
     g_mutex_unlock(&lock);

     return (VirtioSharedObject *)lookup_res;
}

> +int main(int argc, char **argv)
> +{
> +    g_test_init(&argc, &argv, NULL);
> +    g_test_add_func("/virtio-dmabuf/add_rm_res", test_add_remove_resources);
> +        g_test_add_func("/virtio-dmabuf/add_rm_dev", test_add_remove_dev);

Mis-indent.

> +    g_test_add_func("/virtio-dmabuf/rm_invalid_res",
> +                    test_remove_invalid_resource);
> +    g_test_add_func("/virtio-dmabuf/add_invalid_res",
> +                    test_add_invalid_resource);
> +    g_test_add_func("/virtio-dmabuf/free_res", test_free_resources);
> +
> +    return g_test_run();
> +}

Thanks for updating, LGTM!