| Message ID | 20230503081911.119168-2-aesteve@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Virtio shared dma-buf | expand |
On Wed, May 03 2023, Albert Esteve <aesteve@redhat.com> wrote: > 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 General comment: new files should be covered in MAINTAINERS; not sure if there is any generic section that could match it, or if this should go into a new section. > index 0000000000..3db939a2e3 > --- /dev/null > +++ b/hw/display/virtio-dmabuf.c Is virtio-dmabuf only useful for stuff under display/, or could it go into a more generic section?
On Mon, May 8, 2023 at 3:12 PM Cornelia Huck <cohuck@redhat.com> wrote: > On Wed, May 03 2023, Albert Esteve <aesteve@redhat.com> wrote: > > > 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 > > General comment: new files should be covered in MAINTAINERS; not sure if > there is any generic section that could match it, or if this should go > into a new section. > You are right. I thought the entire folder would have an owner already, but I see it is split by features. I guess this would make sense under a new section then. Thanks! > > > index 0000000000..3db939a2e3 > > --- /dev/null > > +++ b/hw/display/virtio-dmabuf.c > > Is virtio-dmabuf only useful for stuff under display/, or could it go > into a more generic section? > > I hesitated myself and I wouldn't be against changing the location. In this first version of the infrastructure, it is introduced with dma-buf sharing in mind, and virtio-gpu -> virtio-video as the main usecase. Both these devices are/will be at display/, hence I ended up adding the infrastructure in the same folder, close from where it is going to be used. However, in the future other devices may want to use the shared table for other object types, the virtio specs seem to leave that door open. In that case, it may be more interesting in another folder. I had ui/ or hw/virtio/ as candidates myself. Depends on whether we want to plan ahead for future uses or keep it closer to where it is being to be used now.
Hi On Wed, May 3, 2023 at 12:21 PM Albert Esteve <aesteve@redhat.com> wrote: > 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. > + > +static GMutex lock; > +static GHashTable *resource_uuids; > + > Rather than having global variables, shouldn't we associate virtio resources with the virtio bus instead? > + > +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) + } else if (g_hash_table_lookup(resource_uuids, struuid) != NULL) { > You could implement a hash_func and key_equal_func for QemuUUID instead. > + 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. > + * > + * 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 > > >
On Tue, May 9, 2023 at 12:53 PM Marc-André Lureau < marcandre.lureau@gmail.com> wrote: > Hi > > On Wed, May 3, 2023 at 12:21 PM Albert Esteve <aesteve@redhat.com> wrote: > >> 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. > + >> +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! > + >> +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 >
Hi On Tue, May 9, 2023 at 4:53 PM Albert Esteve <aesteve@redhat.com> wrote: > > > On Tue, May 9, 2023 at 12:53 PM Marc-André Lureau < > marcandre.lureau@gmail.com> wrote: > >> Hi >> >> On Wed, May 3, 2023 at 12:21 PM Albert Esteve <aesteve@redhat.com> wrote: >> >>> 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. > > > >> + >>> +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 >> >
On Tue, May 9, 2023 at 2:57 PM Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > Hi > > On Tue, May 9, 2023 at 4:53 PM Albert Esteve <aesteve@redhat.com> wrote: > >> >> >> On Tue, May 9, 2023 at 12:53 PM Marc-André Lureau < >> marcandre.lureau@gmail.com> wrote: >> >>> Hi >>> >>> On Wed, May 3, 2023 at 12:21 PM Albert Esteve <aesteve@redhat.com> >>> wrote: >>> >>>> 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 >
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))))) + + +static GMutex lock; +static GHashTable *resource_uuids; + + +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); + } else if (g_hash_table_lookup(resource_uuids, struuid) != NULL) { + 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 + * + * 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); + +#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(); +}
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