From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 2DD68C7EE22 for ; Tue, 9 May 2023 12:53:38 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pwMq3-0005yI-Lg; Tue, 09 May 2023 08:53:07 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pwMq1-0005y3-2B for qemu-devel@nongnu.org; Tue, 09 May 2023 08:53:06 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pwMpx-00014J-K5 for qemu-devel@nongnu.org; Tue, 09 May 2023 08:53:04 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1683636780; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=I738u/s8TIfM6D9DoG8UTKkE/yVGSviOcfpcNVGqIr8=; b=DVvPaoT4PrERKYpkdWmO5+wTrgE6pk8pdA3BV6TqLaShosKiWKttFI2urSM+zZ4v3GSJwT 99IF//2t/M/rPmE0kdsLh2B58aZaZuCq0sYZEwTFcursAov4BJvTmOjh3UZ78xSQfX6lty 9MFbHbzpMg/DwX71utB50wcICnIdAbU= Received: from mail-pf1-f200.google.com (mail-pf1-f200.google.com [209.85.210.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-576-wp2DTtVbOOuaJXFWfgrGyQ-1; Tue, 09 May 2023 08:52:58 -0400 X-MC-Unique: wp2DTtVbOOuaJXFWfgrGyQ-1 Received: by mail-pf1-f200.google.com with SMTP id d2e1a72fcca58-643a9203dc2so2850532b3a.1 for ; Tue, 09 May 2023 05:52:58 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1683636778; x=1686228778; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=I738u/s8TIfM6D9DoG8UTKkE/yVGSviOcfpcNVGqIr8=; b=BMYInygkgXU0Lq2tromIOvpX5Kc3ZokDx9SwwYCfGskPHtKRJqP1ilKn9vSJlXOqiJ LDI2dCNuXVaVZ23C2DF8cy/W7CYdl4w/C/iuw20zMHQpB5MPaWHiKdj99YJ5k4rchqUA FyeptneoKnsfr8lVbrSZybsgXv/G4TUDulCTqg0Ievz7XR2MMFa1Yn4DRTQVl1t/epsH JjBxRwIu5lNcvjatC+EbqT+qI2PHkh+q6Je/iwxSUfCBYyV4yeLEQTq0Zq64s6ZHJTtP 5rE7TWRIWb/o4vYWdoZ0Yoqe9o37rnsY5gfduNCRpeFfEZ1FMKnQAmSgQUULrMlx2KJR K3HQ== X-Gm-Message-State: AC+VfDzG8zWngbESfuvEp5rlHFkyg0PmN1Un5sItv3CaYNWHHR7ddWMT Aa2k/vdFNThrtzRAla5Mv/VlbzHt0V1YXSq4QObwhEXOBdlKPTIput860mCTWBSTK05XhZVGT2j yXZMFj520eiD2nbTyCsNwjoJHI+YiQZo= X-Received: by 2002:a17:90a:9906:b0:23f:83de:7e4a with SMTP id b6-20020a17090a990600b0023f83de7e4amr13484664pjp.7.1683636777673; Tue, 09 May 2023 05:52:57 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ4D7y9d9Tf276dPApLJ0j0x0Qtn5whe9m0JXL8YL/NAcQ65sqx9qUQT1T8ug7vPP7LcqKsHEdbmLAWE2AVYCs8= X-Received: by 2002:a17:90a:9906:b0:23f:83de:7e4a with SMTP id b6-20020a17090a990600b0023f83de7e4amr13484651pjp.7.1683636777301; Tue, 09 May 2023 05:52:57 -0700 (PDT) MIME-Version: 1.0 References: <20230503081911.119168-1-aesteve@redhat.com> <20230503081911.119168-2-aesteve@redhat.com> In-Reply-To: From: Albert Esteve Date: Tue, 9 May 2023 14:52:46 +0200 Message-ID: Subject: Re: [PATCH 1/4] virtio-dmabuf: introduce virtio-dmabuf To: =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= Cc: qemu-devel@nongnu.org, "Michael S. Tsirkin" Content-Type: multipart/alternative; boundary="0000000000000ad3c705fb423c31" Received-SPF: pass client-ip=170.10.133.124; envelope-from=aesteve@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org --0000000000000ad3c705fb423c31 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, May 9, 2023 at 12:53=E2=80=AFPM Marc-Andr=C3=A9 Lureau < marcandre.lureau@gmail.com> wrote: > Hi > > On Wed, May 3, 2023 at 12:21=E2=80=AFPM Albert Esteve 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 >> --- >> 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 >> + * >> + * 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 =3D UUID_TO_POINTER(uuid); >> + if (resource_uuids =3D=3D NULL) { >> + resource_uuids =3D g_hash_table_new_full(NULL, NULL, NULL, g_fr= ee); >> > > 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) !=3D 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 =3D=3D 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 =3D=3D NULL) { >> + resource_uuids =3D g_hash_table_new(NULL, NULL); >> + } >> + result =3D 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 =3D 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 =3D virtio_lookup_resource(uuid); >> + g_mutex_unlock(&lock); >> + if (lookup_res =3D=3D 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 =3D 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 >> + * >> + * 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 >> +#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 wi= th >> + * 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 =3D { >> '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 =3D 0; i < 100; ++i) { >> + qemu_uuid_generate(&uuid); >> + dmabuf_fd =3D 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), =3D=3D, dmabuf_fd); >> + /* Remove the resource */ >> + g_assert(virtio_remove_resource(uuid)); >> + /* Resource is not found anymore */ >> + g_assert_cmpint(virtio_lookup_dmabuf(uuid), =3D=3D, -1); >> + } >> +} >> + >> +static void test_remove_invalid_resource(void) >> +{ >> + QemuUUID uuid; >> + int i; >> + >> + for (i =3D 0; i < 20; ++i) { >> + qemu_uuid_generate(&uuid); >> + g_assert_cmpint(virtio_lookup_dmabuf(uuid), =3D=3D, -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 =3D -2, alt_dmabuf =3D 2; >> + >> + for (i =3D 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), =3D=3D, -1); >> + } >> + >> + for (i =3D 0; i < 20; ++i) { >> + /* Add a valid resource */ >> + qemu_uuid_generate(&uuid); >> + dmabuf_fd =3D g_random_int_range(3, 500); >> + g_assert(virtio_add_dmabuf(uuid, dmabuf_fd)); >> + g_assert_cmpint(virtio_lookup_dmabuf(uuid), =3D=3D, 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), =3D=3D, dmabuf_fd); >> + } >> +} >> + >> +static void test_free_resources(void) >> +{ >> + QemuUUID uuids[20]; >> + int i, dmabuf_fd; >> + >> + for (i =3D 0; i < ARRAY_SIZE(uuids); ++i) { >> + qemu_uuid_generate(&uuids[i]); >> + dmabuf_fd =3D g_random_int_range(3, 500); >> + g_assert(virtio_add_dmabuf(uuids[i], dmabuf_fd)); >> + g_assert_cmpint(virtio_lookup_dmabuf(uuids[i]), =3D=3D, dmabuf_= fd); >> + } >> + virtio_free_resources(); >> + for (i =3D 0; i < ARRAY_SIZE(uuids); ++i) { >> + /* None of the resources is found after free'd */ >> + g_assert_cmpint(virtio_lookup_dmabuf(uuids[i]), =3D=3D, -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=C3=A9 Lureau > --0000000000000ad3c705fb423c31 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Tue, May 9, 2023 at 12:53= =E2=80=AFPM Marc-Andr=C3=A9 Lureau <marcandre.lureau@gmail.com> wrote:
Hi

On Wed, May 3, 2023 at 12:21=E2=80=AFPM 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>
---
=C2=A0hw/display/meson.build=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2= =A0 =C2=A01 +
=C2=A0hw/display/virtio-dmabuf.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2=A0 88 ++++= +++++++++++++++++++
=C2=A0include/hw/virtio/virtio-dmabuf.h |=C2=A0 58 ++++++++++++++++
=C2=A0tests/unit/meson.build=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2= =A0 =C2=A01 +
=C2=A0tests/unit/test-virtio-dmabuf.c=C2=A0 =C2=A0| 112 +++++++++++++++++++= +++++++++++
=C2=A05 files changed, 260 insertions(+)
=C2=A0create mode 100644 hw/display/virtio-dmabuf.c
=C2=A0create mode 100644 include/hw/virtio/virtio-dmabuf.h
=C2=A0create 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: fil= es('macfb.c'))
=C2=A0softmmu_ss.add(when: 'CONFIG_NEXTCUBE', if_true: files('n= ext-fb.c'))

=C2=A0softmmu_ss.add(when: 'CONFIG_VGA', if_true: files('vga.c&= #39;))
+softmmu_ss.add(when: 'CONFIG_VIRTIO', if_true: files('virtio-d= mabuf.c'))

=C2=A0if (config_all_devices.has_key('CONFIG_VGA_CIRRUS') or
=C2=A0 =C2=A0 =C2=A0config_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:
+ *=C2=A0 =C2=A0 =C2=A0Albert Esteve <aesteve@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or late= r.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "hw/virtio/virtio-dmabuf.h"
+
+
+#define UUID_TO_POINTER(i) \
+=C2=A0 =C2=A0 ((gpointer) (g_intern_static_string(qemu_uuid_unparse_strdup= ((&i)))))
+

This will leak.
=C2=A0
<= /div>

Where did you spot the is= sue? The reference operator at `&i`? The cast to gpointer?
I = tried to mimic GINT_TO_POINTER macro here.
=C2=A0
+
+static GMutex lock;
+static GHashTable *resource_uuids;
+

Rather than having global variables, = shouldn't we associate virtio resources with the virtio bus instead?
=C2=A0

I am s= orry but I am not sure how you mean. Wouldn't that mean to create a vir= tio-pci device with
the virtio-dmabuf object? AFAIK the virtio-bu= s conforms the transport layer for connecting to the guest.
The g= oal with virtio-dmabuf is "connecting" different virtio backends,= =C2=A0which are already connected to the
virtio-bus (and the gues= t). Strictly speaking not even that, just needs to be accessible from diffe= rent devices
to add and retrieve descriptors (dealing with concur= rent accesses).

But maybe I am not understanding y= our point!

=C2=A0
+
+static bool virtio_add_resource(QemuUUID uuid, gpointer value)
+{
+=C2=A0 =C2=A0 gpointer struuid =3D UUID_TO_POINTER(uuid);
+=C2=A0 =C2=A0 if (resource_uuids =3D=3D NULL) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 resource_uuids =3D 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!
=C2= =A0
+=C2=A0 =C2=A0 } else if (g_hash_table_lookup(resource_uuids, struuid) !=3D= NULL) {

You could implement a hash_fun= c and key_equal_func for QemuUUID instead.
=C2=A0
=

Sounds like a good idea. I will add = an initial, separate commit for that.
=C2=A0
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 return false;
+=C2=A0 =C2=A0 }
+
+=C2=A0 =C2=A0 return g_hash_table_insert(resource_uuids, struuid, value);<= br> +}
+
+static gpointer virtio_lookup_resource(QemuUUID uuid)
+{
+=C2=A0 =C2=A0 if (resource_uuids =3D=3D NULL) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 return NULL;
+=C2=A0 =C2=A0 }
+
+=C2=A0 =C2=A0 return g_hash_table_lookup(resource_uuids, UUID_TO_POINTER(u= uid));
+}
+
+bool virtio_add_dmabuf(QemuUUID uuid, int udmabuf_fd)
+{
+=C2=A0 =C2=A0 bool result;
+=C2=A0 =C2=A0 if (udmabuf_fd < 0) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 return false;
+=C2=A0 =C2=A0 }
+=C2=A0 =C2=A0 g_mutex_lock(&lock);
+=C2=A0 =C2=A0 if (resource_uuids =3D=3D NULL) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 resource_uuids =3D g_hash_table_new(NULL, NULL= );
+=C2=A0 =C2=A0 }
+=C2=A0 =C2=A0 result =3D virtio_add_resource(uuid, GINT_TO_POINTER(udmabuf= _fd));
+=C2=A0 =C2=A0 g_mutex_unlock(&lock);
+
+=C2=A0 =C2=A0 return result;
+}
+
+bool virtio_remove_resource(QemuUUID uuid)
+{
+=C2=A0 =C2=A0 bool result;
+=C2=A0 =C2=A0 g_mutex_lock(&lock);
+=C2=A0 =C2=A0 result =3D g_hash_table_remove(resource_uuids, UUID_TO_POINT= ER(uuid));
+=C2=A0 =C2=A0 g_mutex_unlock(&lock);
+
+=C2=A0 =C2=A0 return result;
+}
+
+int virtio_lookup_dmabuf(QemuUUID uuid)
+{
+=C2=A0 =C2=A0 g_mutex_lock(&lock);
+=C2=A0 =C2=A0 gpointer lookup_res =3D virtio_lookup_resource(uuid);
+=C2=A0 =C2=A0 g_mutex_unlock(&lock);
+=C2=A0 =C2=A0 if (lookup_res =3D=3D NULL) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 return -1;
+=C2=A0 =C2=A0 }
+
+=C2=A0 =C2=A0 return GPOINTER_TO_INT(lookup_res);
+}
+
+void virtio_free_resources(void)
+{
+=C2=A0 =C2=A0 g_hash_table_destroy(resource_uuids);
+=C2=A0 =C2=A0 /* Reference count shall be 0 after the implicit unref on de= stroy */
+=C2=A0 =C2=A0 resource_uuids =3D NULL;
+}
diff --git a/include/hw/virtio/virtio-dmabuf.h b/include/hw/virtio/virtio-d= mabuf.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:
+ *=C2=A0 =C2=A0 =C2=A0Albert 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<= br> + *=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0other virtio devices
=

The comment should be clear about fd owner= ship. In this case, it looks like it's the caller's responsibility = to properly handle its lifecycle.
=C2=A0

Sure.
=C2=A0
+ *
+ * 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<= br> + * 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 tab= le.
+ */
+bool virtio_remove_resource(QemuUUID uuid);
+
+/**
+ * virtio_lookup_dmabuf() - Looks for a dma-buf resource in the lookup tab= le
+ * @uuid: resource's UUID
+ *
+ * Return: the dma-buf file descriptor integer, or -1 if the key is not fo= und.
+ */
+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 i= t'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.
=
=C2=A0
+
+#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 =3D {
=C2=A0 =C2=A0'test-qapi-util': [],
=C2=A0 =C2=A0'test-interval-tree': [],
=C2=A0 =C2=A0'test-xs-node': [qom],
+=C2=A0 'test-virtio-dmabuf': [meson.project_source_root() / 'h= w/display/virtio-dmabuf.c'],
=C2=A0}

=C2=A0if have_system or have_tools
diff --git a/tests/unit/test-virtio-dmabuf.c b/tests/unit/test-virtio-dmabu= f.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.=C2=A0 See the GNU<= br> + * 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)
+{
+=C2=A0 =C2=A0 QemuUUID uuid;
+=C2=A0 =C2=A0 int i, dmabuf_fd;
+
+=C2=A0 =C2=A0 for (i =3D 0; i < 100; ++i) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 qemu_uuid_generate(&uuid);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 dmabuf_fd =3D g_random_int_range(3, 500);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Add a new resource */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 g_assert(virtio_add_dmabuf(uuid, dmabuf_fd));<= br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 g_assert_cmpint(virtio_lookup_dmabuf(uuid), = =3D=3D, dmabuf_fd);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Remove the resource */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 g_assert(virtio_remove_resource(uuid));
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Resource is not found anymore */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 g_assert_cmpint(virtio_lookup_dmabuf(uuid), = =3D=3D, -1);
+=C2=A0 =C2=A0 }
+}
+
+static void test_remove_invalid_resource(void)
+{
+=C2=A0 =C2=A0 QemuUUID uuid;
+=C2=A0 =C2=A0 int i;
+
+=C2=A0 =C2=A0 for (i =3D 0; i < 20; ++i) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 qemu_uuid_generate(&uuid);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 g_assert_cmpint(virtio_lookup_dmabuf(uuid), = =3D=3D, -1);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Removing a resource that does not exist ret= urns false */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 g_assert_false(virtio_remove_resource(uuid));<= br> +=C2=A0 =C2=A0 }
+}
+
+static void test_add_invalid_resource(void)
+{
+=C2=A0 =C2=A0 QemuUUID uuid;
+=C2=A0 =C2=A0 int i, dmabuf_fd =3D -2, alt_dmabuf =3D 2;
+
+=C2=A0 =C2=A0 for (i =3D 0; i < 20; ++i) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 qemu_uuid_generate(&uuid);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Add a new resource with invalid (negative) = resource fd */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 g_assert_false(virtio_add_dmabuf(uuid, dmabuf_= fd));
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Resource is not found */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 g_assert_cmpint(virtio_lookup_dmabuf(uuid), = =3D=3D, -1);
+=C2=A0 =C2=A0 }
+
+=C2=A0 =C2=A0 for (i =3D 0; i < 20; ++i) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Add a valid resource */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 qemu_uuid_generate(&uuid);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 dmabuf_fd =3D g_random_int_range(3, 500);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 g_assert(virtio_add_dmabuf(uuid, dmabuf_fd));<= br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 g_assert_cmpint(virtio_lookup_dmabuf(uuid), = =3D=3D, dmabuf_fd);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Add a new resource with repeated uuid retur= ns false */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 g_assert_false(virtio_add_dmabuf(uuid, alt_dma= buf));
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* The value for the uuid key is not replaced = */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 g_assert_cmpint(virtio_lookup_dmabuf(uuid), = =3D=3D, dmabuf_fd);
+=C2=A0 =C2=A0 }
+}
+
+static void test_free_resources(void)
+{
+=C2=A0 =C2=A0 QemuUUID uuids[20];
+=C2=A0 =C2=A0 int i, dmabuf_fd;
+
+=C2=A0 =C2=A0 for (i =3D 0; i < ARRAY_SIZE(uuids); ++i) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 qemu_uuid_generate(&uuids[i]);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 dmabuf_fd =3D g_random_int_range(3, 500);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 g_assert(virtio_add_dmabuf(uuids[i], dmabuf_fd= ));
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 g_assert_cmpint(virtio_lookup_dmabuf(uuids[i])= , =3D=3D, dmabuf_fd);
+=C2=A0 =C2=A0 }
+=C2=A0 =C2=A0 virtio_free_resources();
+=C2=A0 =C2=A0 for (i =3D 0; i < ARRAY_SIZE(uuids); ++i) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* None of the resources is found after free&#= 39;d */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 g_assert_cmpint(virtio_lookup_dmabuf(uuids[i])= , =3D=3D, -1);
+=C2=A0 =C2=A0 }
+
+}
+
+int main(int argc, char **argv)
+{
+=C2=A0 =C2=A0 g_test_init(&argc, &argv, NULL);
+=C2=A0 =C2=A0 g_test_add_func("/virtio-dmabuf/add_rm_res", test_= add_remove_resources);
+=C2=A0 =C2=A0 g_test_add_func("/virtio-dmabuf/rm_invalid_res", +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 test= _remove_invalid_resource);
+=C2=A0 =C2=A0 g_test_add_func("/virtio-dmabuf/add_invalid_res",<= br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 test= _add_invalid_resource);
+=C2=A0 =C2=A0 g_test_add_func("/virtio-dmabuf/free_res", test_fr= ee_resources);
+
+=C2=A0 =C2=A0 return g_test_run();
+}
--
2.40.0




--
Marc-Andr=C3=A9 Lureau
--0000000000000ad3c705fb423c31--