Re: [PATCH 1/4] virtio-dmabuf: introduce virtio-dmabuf

[Albert Esteve <aesteve@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 15522 bytes --]

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
>

[-- Attachment #2: Type: text/html, Size: 20557 bytes --]