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

[Philippe Mathieu-Daudé]

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 <kra...@redhat.com>
Signed-off-by: Albert Esteve <aest...@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-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..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 <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, 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!