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;
+}
+
+bool virtio_add_dmabuf(QemuUUID *uuid, int udmabuf_fd)
+{
+ bool result;
+ VirtioSharedObject *vso;
+ if (udmabuf_fd < 0) {
+ return false;
+ }
+ vso = g_new(VirtioSharedObject, 1);
+ vso->type = TYPE_DMABUF;
+ vso->value = GINT_TO_POINTER(udmabuf_fd);
+ result = virtio_add_resource(uuid, vso);
+
+ return result;
+}
+
+bool virtio_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev)
+{
+ bool result;
+ VirtioSharedObject *vso;
+ if (dev == NULL) {
+ return false;
+ }
+ vso = g_new(VirtioSharedObject, 1);
+ vso->type = TYPE_VHOST_DEV;
+ vso->value = dev;
+ result = virtio_add_resource(uuid, vso);
+
+ 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);
+
+ 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;
+}
+
+int virtio_lookup_dmabuf(const QemuUUID *uuid)
+{
+ 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)
+{
+ VirtioSharedObject *vso = get_shared_object(uuid);
+ if (vso == NULL) {
+ return NULL;
+ }
+ assert(vso->type == TYPE_VHOST_DEV);
+ return (struct vhost_dev *) vso->value;
+}
+
+SharedObjectType virtio_object_type(const QemuUUID *uuid)
+{
+ VirtioSharedObject *vso = get_shared_object(uuid);
+ if (vso == NULL) {
+ return TYPE_INVALID;
+ }
+ return vso->type;
+}
+
+void virtio_free_resources(void)
+{
+ g_mutex_lock(&lock);
+ g_hash_table_destroy(resource_uuids);
+ /* Reference count shall be 0 after the implicit unref on destroy */
+ resource_uuids = NULL;
+ g_mutex_unlock(&lock);
+}
diff --git a/include/hw/virtio/virtio-dmabuf.h b/include/hw/virtio/virtio-dmabuf.h
new file mode 100644
index 0000000000..202eec5868
--- /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"
+
+typedef enum SharedObjectType {
+ TYPE_INVALID = 0,
+ TYPE_DMABUF,
+ TYPE_VHOST_DEV,
+} SharedObjectType;
+
+typedef struct VirtioSharedObject {
+ SharedObjectType type;
+ gpointer value;
+} VirtioSharedObject;
+
+/**
+ * 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 caller retains ownership over the
+ * descriptor and its lifecycle.
+ *
+ * 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_add_vhost_device() - Add a new exporter vhost device that holds the
+ * resource with the associated UUID
+ * @uuid: new resource's UUID
+ * @dev: the pointer to the vhost device that holds the resource. The caller
+ * retains ownership over the device struct and its lifecycle.
+ *
+ * Return: true if the UUID did not exist and the device has been tracked,
+ * 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_vhost_device(QemuUUID *uuid, struct vhost_dev *dev);
+
+/**
+ * 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(const 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(const QemuUUID *uuid);
+
+/**
+ * virtio_lookup_vhost_device() - Looks for an exporter vhost device in the
+ * lookup table
+ * @uuid: resource's UUID
+ *
+ * Return: pointer to the vhost_dev struct, or NULL if the key is not found.
+ */
+struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid);
+
+/**
+ * virtio_object_type() - Looks for the type of resource in the lookup table
+ * @uuid: resource's UUID
+ *
+ * Return: the type of resource associated with the UUID, or TYPE_INVALID if
+ * the key is not found.
+ */
+SharedObjectType virtio_object_type(const QemuUUID *uuid);
+
+/**
+ * virtio_free_resources() - Destroys all keys and values of the shared
+ * resources lookup table, and frees them
+ */
+void virtio_free_resources(void);
+
+#endif /* VIRTIO_DMABUF_H */
diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index 93977cc32d..425ecc30fb 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..40fe262538
--- /dev/null
+++ b/tests/unit/test-virtio-dmabuf.c
@@ -0,0 +1,137 @@
+/*
+ * 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_add_remove_dev(void)
+{
+ QemuUUID uuid;
+ struct vhost_dev *dev = g_new0(struct vhost_dev, 1);
+ int i;
+
+ for (i = 0; i < 100; ++i) {
+ qemu_uuid_generate(&uuid);
+ virtio_add_vhost_device(&uuid, dev);
+ /* vhost device is found */
+ g_assert(virtio_lookup_vhost_device(&uuid) != NULL);
+ /* Remove the vhost device */
+ g_assert(virtio_remove_resource(&uuid));
+ /* vhost device is not found anymore */
+ g_assert(virtio_lookup_vhost_device(&uuid) == NULL);
+ }
+ g_free(dev);
+}
+
+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;
+ struct vhost_dev *dev = NULL;
+ 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);
+ /* Add a new vhost device with invalid (NULL) pointer */
+ g_assert_false(virtio_add_vhost_device(&uuid, dev));
+ /* vhost device is not found */
+ g_assert(virtio_lookup_vhost_device(&uuid) == NULL);
+ }
+
+ 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/add_rm_dev", test_add_remove_dev);
+ 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.41.0On 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!
On Wed, Sep 6, 2023 at 4:21 PM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:
> 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!
>
> Suggestions look good, and is true that will lead to more safe and
maintanable code in the future,
if we add more branches where the mutex has to be handled.
I will send another revision then, thanks!
On Wed, Sep 6, 2023 at 1:15 PM Albert Esteve <aesteve@redhat.com> 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;
> +}
> +
> +bool virtio_add_dmabuf(QemuUUID *uuid, int udmabuf_fd)
> +{
> + bool result;
> + VirtioSharedObject *vso;
> + if (udmabuf_fd < 0) {
> + return false;
> + }
> + vso = g_new(VirtioSharedObject, 1);
> + vso->type = TYPE_DMABUF;
> + vso->value = GINT_TO_POINTER(udmabuf_fd);
> + result = virtio_add_resource(uuid, vso);
> +
> + return result;
>
Just realized that the result variable is not required anymore
with the last change.
Not sure if it is worth sending a new review for this...
I can take it into account for a follow-up refactor for the next
update I make on the file. Or just send a separate trivial patch.
> +}
> +
> +bool virtio_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev)
> +{
> + bool result;
> + VirtioSharedObject *vso;
> + if (dev == NULL) {
> + return false;
> + }
> + vso = g_new(VirtioSharedObject, 1);
> + vso->type = TYPE_VHOST_DEV;
> + vso->value = dev;
> + result = virtio_add_resource(uuid, vso);
> +
> + 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);
> +
> + 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;
> +}
> +
> +int virtio_lookup_dmabuf(const QemuUUID *uuid)
> +{
> + 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)
> +{
> + VirtioSharedObject *vso = get_shared_object(uuid);
> + if (vso == NULL) {
> + return NULL;
> + }
> + assert(vso->type == TYPE_VHOST_DEV);
> + return (struct vhost_dev *) vso->value;
> +}
> +
> +SharedObjectType virtio_object_type(const QemuUUID *uuid)
> +{
> + VirtioSharedObject *vso = get_shared_object(uuid);
> + if (vso == NULL) {
> + return TYPE_INVALID;
> + }
> + return vso->type;
> +}
> +
> +void virtio_free_resources(void)
> +{
> + g_mutex_lock(&lock);
> + g_hash_table_destroy(resource_uuids);
> + /* Reference count shall be 0 after the implicit unref on destroy */
> + resource_uuids = NULL;
> + g_mutex_unlock(&lock);
> +}
> diff --git a/include/hw/virtio/virtio-dmabuf.h
> b/include/hw/virtio/virtio-dmabuf.h
> new file mode 100644
> index 0000000000..202eec5868
> --- /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"
> +
> +typedef enum SharedObjectType {
> + TYPE_INVALID = 0,
> + TYPE_DMABUF,
> + TYPE_VHOST_DEV,
> +} SharedObjectType;
> +
> +typedef struct VirtioSharedObject {
> + SharedObjectType type;
> + gpointer value;
> +} VirtioSharedObject;
> +
> +/**
> + * 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 caller retains ownership over the
> + * descriptor and its lifecycle.
> + *
> + * 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_add_vhost_device() - Add a new exporter vhost device that holds
> the
> + * resource with the associated UUID
> + * @uuid: new resource's UUID
> + * @dev: the pointer to the vhost device that holds the resource. The
> caller
> + * retains ownership over the device struct and its lifecycle.
> + *
> + * Return: true if the UUID did not exist and the device has been tracked,
> + * 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_vhost_device(QemuUUID *uuid, struct vhost_dev *dev);
> +
> +/**
> + * 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(const 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(const QemuUUID *uuid);
> +
> +/**
> + * virtio_lookup_vhost_device() - Looks for an exporter vhost device in
> the
> + * lookup table
> + * @uuid: resource's UUID
> + *
> + * Return: pointer to the vhost_dev struct, or NULL if the key is not
> found.
> + */
> +struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid);
> +
> +/**
> + * virtio_object_type() - Looks for the type of resource in the lookup
> table
> + * @uuid: resource's UUID
> + *
> + * Return: the type of resource associated with the UUID, or TYPE_INVALID
> if
> + * the key is not found.
> + */
> +SharedObjectType virtio_object_type(const QemuUUID *uuid);
> +
> +/**
> + * virtio_free_resources() - Destroys all keys and values of the shared
> + * resources lookup table, and frees them
> + */
> +void virtio_free_resources(void);
> +
> +#endif /* VIRTIO_DMABUF_H */
> diff --git a/tests/unit/meson.build b/tests/unit/meson.build
> index 93977cc32d..425ecc30fb 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..40fe262538
> --- /dev/null
> +++ b/tests/unit/test-virtio-dmabuf.c
> @@ -0,0 +1,137 @@
> +/*
> + * 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_add_remove_dev(void)
> +{
> + QemuUUID uuid;
> + struct vhost_dev *dev = g_new0(struct vhost_dev, 1);
> + int i;
> +
> + for (i = 0; i < 100; ++i) {
> + qemu_uuid_generate(&uuid);
> + virtio_add_vhost_device(&uuid, dev);
> + /* vhost device is found */
> + g_assert(virtio_lookup_vhost_device(&uuid) != NULL);
> + /* Remove the vhost device */
> + g_assert(virtio_remove_resource(&uuid));
> + /* vhost device is not found anymore */
> + g_assert(virtio_lookup_vhost_device(&uuid) == NULL);
> + }
> + g_free(dev);
> +}
> +
> +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;
> + struct vhost_dev *dev = NULL;
> + 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);
> + /* Add a new vhost device with invalid (NULL) pointer */
> + g_assert_false(virtio_add_vhost_device(&uuid, dev));
> + /* vhost device is not found */
> + g_assert(virtio_lookup_vhost_device(&uuid) == NULL);
> + }
> +
> + 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/add_rm_dev", test_add_remove_dev);
> + 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.41.0
>
>
© 2016 - 2024 Red Hat, Inc.