On Wed, Sep 6, 2023 at 4:21 PM Philippe Mathieu-Daudé <phi...@linaro.org> wrote:
> On 6/9/23 13:15, Albert Esteve wrote: > > This API manages objects (in this iteration, > > dmabuf fds) that can be shared along different > > virtio devices, associated to a UUID. > > > > 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. > > > > For vhost backends, the API stores the pointer > > to the backend holding the object. > > > > Suggested-by: Gerd Hoffmann <kra...@redhat.com> > > Signed-off-by: Albert Esteve <aest...@redhat.com> > > --- > > MAINTAINERS | 7 ++ > > hw/display/meson.build | 1 + > > hw/display/virtio-dmabuf.c | 134 +++++++++++++++++++++++++++++ > > include/hw/virtio/virtio-dmabuf.h | 103 ++++++++++++++++++++++ > > tests/unit/meson.build | 1 + > > tests/unit/test-virtio-dmabuf.c | 137 ++++++++++++++++++++++++++++++ > > 6 files changed, 383 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/MAINTAINERS b/MAINTAINERS > > index 3b29568ed4..fb0f7b823f 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -2150,6 +2150,13 @@ T: git https://gitlab.com/cohuck/qemu.git > s390-next > > T: git https://github.com/borntraeger/qemu.git s390-next > > L: qemu-s3...@nongnu.org > > > > +virtio-dmabuf > > +M: Albert Esteve <aest...@redhat.com> > > +S: Supported > > +F: hw/display/virtio-dmabuf.c > > +F: include/hw/virtio/virtio-dmabuf.h > > +F: tests/unit/test-virtio-dmabuf.c > > + > > virtiofs > > M: Stefan Hajnoczi <stefa...@redhat.com> > > S: Supported > > diff --git a/hw/display/meson.build b/hw/display/meson.build > > index 413ba4ab24..05619c6968 100644 > > --- a/hw/display/meson.build > > +++ b/hw/display/meson.build > > @@ -37,6 +37,7 @@ system_ss.add(when: 'CONFIG_MACFB', if_true: > files('macfb.c')) > > system_ss.add(when: 'CONFIG_NEXTCUBE', if_true: files('next-fb.c')) > > > > system_ss.add(when: 'CONFIG_VGA', if_true: files('vga.c')) > > +system_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..268ffe81ec > > --- /dev/null > > +++ b/hw/display/virtio-dmabuf.c > > @@ -0,0 +1,134 @@ > > +/* > > + * 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" > > + > > + > > +static GMutex lock; > > +static GHashTable *resource_uuids; > > + > > +/* > > + * uuid_equal_func: wrapper for UUID is_equal function to > > + * satisfy g_hash_table_new expected parameters signatures. > > + */ > > +static int uuid_equal_func(const void *lhv, const void *rhv) > > +{ > > + return qemu_uuid_is_equal(lhv, rhv); > > +} > > + > > +static bool virtio_add_resource(QemuUUID *uuid, VirtioSharedObject > *value) > > +{ > > + bool result; > > + g_mutex_lock(&lock); > > + if (resource_uuids == NULL) { > > + resource_uuids = g_hash_table_new_full( > > + qemu_uuid_hash, uuid_equal_func, NULL, g_free); > > + } > > + if (g_hash_table_lookup(resource_uuids, uuid) != NULL) { > > + g_mutex_unlock(&lock); > > + return false; > > + } > > + result = g_hash_table_insert(resource_uuids, uuid, value); > > + g_mutex_unlock(&lock); > > + > > + return result; > > +} > > Alternatively same logic, but simpler / safer: > > static bool virtio_add_resource(...) > { > bool result = false; > > g_mutex_lock(&lock); > if (resource_uuids == NULL) { > resource_uuids = g_hash_table_new_full(qemu_uuid_hash, > uuid_equal_func, > NULL, > g_free); > } > if (g_hash_table_lookup(resource_uuids, uuid) == NULL) { > result = g_hash_table_insert(resource_uuids, uuid, value); > } > g_mutex_unlock(&lock); > > return result; > } > > > +static VirtioSharedObject *get_shared_object(const QemuUUID *uuid) > > +{ > > + g_mutex_lock(&lock); > > + if (resource_uuids == NULL) { > > + g_mutex_unlock(&lock); > > + return NULL; > > + } > > + gpointer lookup_res = g_hash_table_lookup(resource_uuids, uuid); > > + g_mutex_unlock(&lock); > > + return (VirtioSharedObject*) lookup_res; > > +} > > Similarly: > > static VirtioSharedObject *get_shared_object(const QemuUUID *uuid) > { > gpointer lookup_res = NULL; > > g_mutex_lock(&lock); > if (resource_uuids != NULL) { > lookup_res = g_hash_table_lookup(resource_uuids, uuid); > } > g_mutex_unlock(&lock); > > return (VirtioSharedObject *)lookup_res; > } > > > +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/add_rm_dev", > test_add_remove_dev); > > Mis-indent. > > > + 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(); > > +} > > Thanks for updating, LGTM! > > Suggestions look good, and is true that will lead to more safe and maintanable code in the future, if we add more branches where the mutex has to be handled. I will send another revision then, thanks!
