Hi
Hi
This API manages objects (in this iteration,
dmabuf fds) that can be shared along different
virtio devices.
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.
Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
hw/display/meson.build | 1 +
hw/display/virtio-dmabuf.c | 88 +++++++++++++++++++++++
include/hw/virtio/virtio-dmabuf.h | 58 ++++++++++++++++
tests/unit/meson.build | 1 +
tests/unit/test-virtio-dmabuf.c | 112 ++++++++++++++++++++++++++++++
5 files changed, 260 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/hw/display/meson.build b/hw/display/meson.build
index 17165bd536..62a27395c0 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -37,6 +37,7 @@ softmmu_ss.add(when: 'CONFIG_MACFB', if_true: files('macfb.c'))
softmmu_ss.add(when: 'CONFIG_NEXTCUBE', if_true: files('next-fb.c'))
softmmu_ss.add(when: 'CONFIG_VGA', if_true: files('vga.c'))
+softmmu_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..3db939a2e3
--- /dev/null
+++ b/hw/display/virtio-dmabuf.c
@@ -0,0 +1,88 @@
+/*
+ * 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"
+
+
+#define UUID_TO_POINTER(i) \
+ ((gpointer) (g_intern_static_string(qemu_uuid_unparse_strdup((&i)))))
+
This will leak.
Where did you spot the issue? The reference operator at `&i`? The cast to gpointer?
I tried to mimic GINT_TO_POINTER macro here.
g_intern_static_string() takes a const char *, qemu_uuid_unparse_strdup() returns an allocated string which you don't track or free (which you are not supposed to with static_string). Anyway, you shouldn't need this API if you implement hash and equal func for UUID as suggested.
+
+static GMutex lock;
+static GHashTable *resource_uuids;
+
Rather than having global variables, shouldn't we associate virtio resources with the virtio bus instead?
I am sorry but I am not sure how you mean. Wouldn't that mean to create a virtio-pci device with
the virtio-dmabuf object? AFAIK the virtio-bus conforms the transport layer for connecting to the guest.
The goal with virtio-dmabuf is "connecting" different virtio backends, which are already connected to the
virtio-bus (and the guest). Strictly speaking not even that, just needs to be accessible from different devices
to add and retrieve descriptors (dealing with concurrent accesses).
But maybe I am not understanding your point!
All the virtio devices are attached to a virtio bus. Thus I think shared resource tracking should be implemented on the bus. Maybe Michael could comment on that first.
My idea was having the hash table available for all backends that need to interact with it, hence I was not thinking of it as a virtio device that needs to be added in the
command line (e.g., -device virtio-dmabuf-pci) to have it available. But I guess that is what you are proposing?
I am preparing a v2 of this patch addressing all other comments but this one. I hope that is ok. We can continue the discussion about how to handle this on the
next version of the patch.
The other comment that I did not address was the one from Cornelia regarding the location of the new files. Same thing, we can continue the
conversation over the next version. If there is a good point for having it in another path in the project, I do not have problems moving it.
This is nearly my first patch, so still need to get a bit more familiar with the project structure :)
+
+static bool virtio_add_resource(QemuUUID uuid, gpointer value)
+{
+ gpointer struuid = UUID_TO_POINTER(uuid);
+ if (resource_uuids == NULL) {
+ resource_uuids = g_hash_table_new_full(NULL, NULL, NULL, g_free);
You create "resource_uuids" implicitly in 2 places, in 2 different ways. Let's not do that, and have an explicit initialization step instead (it might be with the virtio bus construction, if we move the code there)
Ok!
+ } else if (g_hash_table_lookup(resource_uuids, struuid) != NULL) {
You could implement a hash_func and key_equal_func for QemuUUID instead.
Sounds like a good idea. I will add an initial, separate commit for that.
+ return false;
+ }
+
+ return g_hash_table_insert(resource_uuids, struuid, value);
+}
+
+static gpointer virtio_lookup_resource(QemuUUID uuid)
+{
+ if (resource_uuids == NULL) {
+ return NULL;
+ }
+
+ return g_hash_table_lookup(resource_uuids, UUID_TO_POINTER(uuid));
+}
+
+bool virtio_add_dmabuf(QemuUUID uuid, int udmabuf_fd)
+{
+ bool result;
+ if (udmabuf_fd < 0) {
+ return false;
+ }
+ g_mutex_lock(&lock);
+ if (resource_uuids == NULL) {
+ resource_uuids = g_hash_table_new(NULL, NULL);
+ }
+ result = virtio_add_resource(uuid, GINT_TO_POINTER(udmabuf_fd));
+ g_mutex_unlock(&lock);
+
+ return result;
+}
+
+bool virtio_remove_resource(QemuUUID uuid)
+{
+ bool result;
+ g_mutex_lock(&lock);
+ result = g_hash_table_remove(resource_uuids, UUID_TO_POINTER(uuid));
+ g_mutex_unlock(&lock);
+
+ return result;
+}
+
+int virtio_lookup_dmabuf(QemuUUID uuid)
+{
+ g_mutex_lock(&lock);
+ gpointer lookup_res = virtio_lookup_resource(uuid);
+ g_mutex_unlock(&lock);
+ if (lookup_res == NULL) {
+ return -1;
+ }
+
+ return GPOINTER_TO_INT(lookup_res);
+}
+
+void virtio_free_resources(void)
+{
+ g_hash_table_destroy(resource_uuids);
+ /* 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..1c1c713128
--- /dev/null
+++ b/include/hw/virtio/virtio-dmabuf.h
@@ -0,0 +1,58 @@
+/*
+ * 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"
+
+/**
+ * virtio_add_dmabuf() - Add a new dma-buf resource to the lookup table
+ * @uuid: new resource's UUID
+ * @dmabuf_fd: the dma-buf descriptor that will be stored and shared with
+ * other virtio devices
The comment should be clear about fd ownership. In this case, it looks like it's the caller's responsibility to properly handle its lifecycle.
Sure.
+ *
+ * Note: @dmabuf_fd must be a valid (non-negative) file descriptor.
+ *
+ * Return: true if the UUID did not exist and the resource has been added,
+ * false if another resource with the same UUID already existed.
+ * Note that if it finds a repeated UUID, the resource is not inserted in
+ * the lookup table.
+ */
+bool virtio_add_dmabuf(QemuUUID uuid, int dmabuf_fd);
+
+/**
+ * virtio_remove_resource() - Removes a resource from the lookup table
+ * @uuid: resource's UUID
+ *
+ * Return: true if the UUID has been found and removed from the lookup table.
+ */
+bool virtio_remove_resource(QemuUUID uuid);
+
+/**
+ * virtio_lookup_dmabuf() - Looks for a dma-buf resource in the lookup table
+ * @uuid: resource's UUID
+ *
+ * Return: the dma-buf file descriptor integer, or -1 if the key is not found.
+ */
+int virtio_lookup_dmabuf(QemuUUID uuid);
+
+/**
+ * virtio_free_resources() - Destroys all keys and values of the shared
+ * resources lookup table, and frees them
+ */
+void virtio_free_resources(void);
If it's part of the virtio bus, we won't need to have an API for it, it will be done as part of object destruction.
+
+#endif /* VIRTIO_DMABUF_H */
diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index 3bc78d8660..eb2a1a8872 100644
--- a/tests/unit/meson.build
+++ b/tests/unit/meson.build
@@ -50,6 +50,7 @@ tests = {
'test-qapi-util': [],
'test-interval-tree': [],
'test-xs-node': [qom],
+ 'test-virtio-dmabuf': [meson.project_source_root() / 'hw/display/virtio-dmabuf.c'],
}
if have_system or have_tools
diff --git a/tests/unit/test-virtio-dmabuf.c b/tests/unit/test-virtio-dmabuf.c
new file mode 100644
index 0000000000..063830c91c
--- /dev/null
+++ b/tests/unit/test-virtio-dmabuf.c
@@ -0,0 +1,112 @@
+/*
+ * QEMU tests for shared dma-buf API
+ *
+ * Copyright (c) 2023 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "hw/virtio/virtio-dmabuf.h"
+
+
+static void test_add_remove_resources(void)
+{
+ QemuUUID uuid;
+ int i, dmabuf_fd;
+
+ for (i = 0; i < 100; ++i) {
+ qemu_uuid_generate(&uuid);
+ dmabuf_fd = g_random_int_range(3, 500);
+ /* Add a new resource */
+ g_assert(virtio_add_dmabuf(uuid, dmabuf_fd));
+ g_assert_cmpint(virtio_lookup_dmabuf(uuid), ==, dmabuf_fd);
+ /* Remove the resource */
+ g_assert(virtio_remove_resource(uuid));
+ /* Resource is not found anymore */
+ g_assert_cmpint(virtio_lookup_dmabuf(uuid), ==, -1);
+ }
+}
+
+static void test_remove_invalid_resource(void)
+{
+ QemuUUID uuid;
+ int i;
+
+ for (i = 0; i < 20; ++i) {
+ qemu_uuid_generate(&uuid);
+ g_assert_cmpint(virtio_lookup_dmabuf(uuid), ==, -1);
+ /* Removing a resource that does not exist returns false */
+ g_assert_false(virtio_remove_resource(uuid));
+ }
+}
+
+static void test_add_invalid_resource(void)
+{
+ QemuUUID uuid;
+ int i, dmabuf_fd = -2, alt_dmabuf = 2;
+
+ for (i = 0; i < 20; ++i) {
+ qemu_uuid_generate(&uuid);
+ /* Add a new resource with invalid (negative) resource fd */
+ g_assert_false(virtio_add_dmabuf(uuid, dmabuf_fd));
+ /* Resource is not found */
+ g_assert_cmpint(virtio_lookup_dmabuf(uuid), ==, -1);
+ }
+
+ for (i = 0; i < 20; ++i) {
+ /* Add a valid resource */
+ qemu_uuid_generate(&uuid);
+ dmabuf_fd = g_random_int_range(3, 500);
+ g_assert(virtio_add_dmabuf(uuid, dmabuf_fd));
+ g_assert_cmpint(virtio_lookup_dmabuf(uuid), ==, dmabuf_fd);
+ /* Add a new resource with repeated uuid returns false */
+ g_assert_false(virtio_add_dmabuf(uuid, alt_dmabuf));
+ /* The value for the uuid key is not replaced */
+ g_assert_cmpint(virtio_lookup_dmabuf(uuid), ==, dmabuf_fd);
+ }
+}
+
+static void test_free_resources(void)
+{
+ QemuUUID uuids[20];
+ int i, dmabuf_fd;
+
+ for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
+ qemu_uuid_generate(&uuids[i]);
+ dmabuf_fd = g_random_int_range(3, 500);
+ g_assert(virtio_add_dmabuf(uuids[i], dmabuf_fd));
+ g_assert_cmpint(virtio_lookup_dmabuf(uuids[i]), ==, dmabuf_fd);
+ }
+ virtio_free_resources();
+ for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
+ /* None of the resources is found after free'd */
+ g_assert_cmpint(virtio_lookup_dmabuf(uuids[i]), ==, -1);
+ }
+
+}
+
+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/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();
+}
--
2.40.0
Marc-André Lureau
Marc-André Lureau