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.
> +
>> +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 <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
>
