Hi On Tue, May 9, 2023 at 4:53 PM Albert Esteve <aest...@redhat.com> wrote:
> > > On Tue, May 9, 2023 at 12:53 PM Marc-André Lureau < > marcandre.lur...@gmail.com> wrote: > >> Hi >> >> On Wed, May 3, 2023 at 12:21 PM Albert Esteve <aest...@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 <aest...@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 <aest...@redhat.com> >>> + * >>> + * This work is licensed under the terms of the GNU GPL, version 2 or >>> later. >>> + * See the COPYING file in the top-level directory. >>> + */ >>> + >>> +#include "hw/virtio/virtio-dmabuf.h" >>> + >>> + >>> +#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 <aest...@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