[PATCH v6 0/3] Virtio shared dma-buf

[PATCH v6 0/3] Virtio shared dma-buf

[Albert Esteve]

v1 link -> https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg00598.html
v2 link -> https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg04530.html
v3 link -> https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg06126.html
v4 link -> https://lists.gnu.org/archive/html/qemu-devel/2023-06/msg05174.html
v5 link -> https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg00255.html
v5 -> v6:
- Unified shared table API mutex usage
- Typedef VirtioSharedObject struct
- Squashed refactor commit to fix compilation issue on patch#3

This patch covers the required steps to add support for virtio cross-device resource sharing[1],
which support is already available in the kernel.

The main usecase will be sharing dma buffers from virtio-gpu devices (as the exporter
-see VIRTIO_GPU_CMD_RESOURCE_ASSIGN_UUID in [2]), to virtio-video (under discussion)
devices (as the buffer-user or importer). Therefore, even though virtio specs talk about
resources or objects[3], this patch adds the infrastructure with dma-bufs in mind.
Note that virtio specs let the devices themselves define what a vitio object is.

These are the main parts that are covered in the patch:

- Add hash function to uuid module
- Shared resources table, to hold all resources that can be shared in the host and their assigned UUID,
  or pointers to the backend holding the resource
- Internal shared table API for virtio devices to add, lookup and remove resources
- Unit test to verify the API
- New messages to the vhost-user protocol to allow backend to interact with the shared
  table API through the control socket
- New vhost-user feature bit to enable shared objects feature

Applies cleanly to 2d8fbcb1eecd8d39171f457e583428758321d69d

[1] - https://lwn.net/Articles/828988/
[2] - https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-3730006
[3] - https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-10500011

Albert Esteve (3):
  uuid: add a hash function
  virtio-dmabuf: introduce virtio-dmabuf
  vhost-user: add shared_object msg

 MAINTAINERS                               |   7 +
 docs/interop/vhost-user.rst               |  57 +++++++
 hw/display/meson.build                    |   1 +
 hw/display/virtio-dmabuf.c                | 134 +++++++++++++++++
 hw/virtio/vhost-user.c                    | 174 ++++++++++++++++++++--
 include/hw/virtio/vhost-backend.h         |   3 +
 include/hw/virtio/virtio-dmabuf.h         | 103 +++++++++++++
 include/qemu/uuid.h                       |   2 +
 subprojects/libvhost-user/libvhost-user.c | 118 +++++++++++++++
 subprojects/libvhost-user/libvhost-user.h |  55 ++++++-
 tests/unit/meson.build                    |   1 +
 tests/unit/test-uuid.c                    |  27 ++++
 tests/unit/test-virtio-dmabuf.c           | 137 +++++++++++++++++
 util/uuid.c                               |  15 ++
 14 files changed, 820 insertions(+), 14 deletions(-)
 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

-- 
2.41.0

[PATCH v6 1/3] uuid: add a hash function

[Albert Esteve]

Add hash function to uuid module using the
djb2 hash algorithm.

Add a couple simple unit tests for the hash
function, checking collisions for similar UUIDs.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
 include/qemu/uuid.h    |  2 ++
 tests/unit/test-uuid.c | 27 +++++++++++++++++++++++++++
 util/uuid.c            | 15 +++++++++++++++
 3 files changed, 44 insertions(+)

diff --git a/include/qemu/uuid.h b/include/qemu/uuid.h
index dc40ee1fc9..e24a1099e4 100644
--- a/include/qemu/uuid.h
+++ b/include/qemu/uuid.h
@@ -96,4 +96,6 @@ int qemu_uuid_parse(const char *str, QemuUUID *uuid);
 
 QemuUUID qemu_uuid_bswap(QemuUUID uuid);
 
+uint32_t qemu_uuid_hash(const void *uuid);
+
 #endif
diff --git a/tests/unit/test-uuid.c b/tests/unit/test-uuid.c
index c111de5fc1..aedc125ae9 100644
--- a/tests/unit/test-uuid.c
+++ b/tests/unit/test-uuid.c
@@ -171,6 +171,32 @@ static void test_uuid_unparse_strdup(void)
     }
 }
 
+static void test_uuid_hash(void)
+{
+    QemuUUID uuid;
+    int i;
+
+    for (i = 0; i < 100; i++) {
+        qemu_uuid_generate(&uuid);
+        /* Obtain the UUID hash */
+        uint32_t hash_a = qemu_uuid_hash(&uuid);
+        int data_idx = g_random_int_range(0, 15);
+        /* Change a single random byte of the UUID */
+        if (uuid.data[data_idx] < 0xFF) {
+            uuid.data[data_idx]++;
+        } else {
+            uuid.data[data_idx]--;
+        }
+        /* Obtain the UUID hash again */
+        uint32_t hash_b = qemu_uuid_hash(&uuid);
+        /*
+         * Both hashes shall be different (avoid collision)
+         * for any change in the UUID fields
+         */
+        g_assert_cmpint(hash_a, !=, hash_b);
+    }
+}
+
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
@@ -179,6 +205,7 @@ int main(int argc, char **argv)
     g_test_add_func("/uuid/parse", test_uuid_parse);
     g_test_add_func("/uuid/unparse", test_uuid_unparse);
     g_test_add_func("/uuid/unparse_strdup", test_uuid_unparse_strdup);
+    g_test_add_func("/uuid/hash", test_uuid_hash);
 
     return g_test_run();
 }
diff --git a/util/uuid.c b/util/uuid.c
index b1108dde78..b366961bc6 100644
--- a/util/uuid.c
+++ b/util/uuid.c
@@ -116,3 +116,18 @@ QemuUUID qemu_uuid_bswap(QemuUUID uuid)
     bswap16s(&uuid.fields.time_high_and_version);
     return uuid;
 }
+
+/* djb2 hash algorithm */
+uint32_t qemu_uuid_hash(const void *uuid)
+{
+    QemuUUID *qid = (QemuUUID *) uuid;
+    uint32_t h = 5381;
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(qid->data); i++) {
+        h = (h << 5) + h + qid->data[i];
+    }
+
+    return h;
+}
+
-- 
2.41.0

[PATCH v6 2/3] virtio-dmabuf: introduce virtio-dmabuf

[Albert Esteve]

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 <kraxel@redhat.com>
Signed-off-by: Albert Esteve <aesteve@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-s390x@nongnu.org
 
+virtio-dmabuf
+M: Albert Esteve <aesteve@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 <stefanha@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 <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"
+
+
+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;
+}
+
+bool virtio_add_dmabuf(QemuUUID *uuid, int udmabuf_fd)
+{
+    bool result;
+    VirtioSharedObject *vso;
+    if (udmabuf_fd < 0) {
+        return false;
+    }
+    vso = g_new(VirtioSharedObject, 1);
+    vso->type = TYPE_DMABUF;
+    vso->value = GINT_TO_POINTER(udmabuf_fd);
+    result = virtio_add_resource(uuid, vso);
+
+    return result;
+}
+
+bool virtio_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev)
+{
+    bool result;
+    VirtioSharedObject *vso;
+    if (dev == NULL) {
+        return false;
+    }
+    vso = g_new(VirtioSharedObject, 1);
+    vso->type = TYPE_VHOST_DEV;
+    vso->value = dev;
+    result = virtio_add_resource(uuid, vso);
+
+    return result;
+}
+
+bool virtio_remove_resource(const QemuUUID *uuid)
+{
+    bool result;
+    g_mutex_lock(&lock);
+    result = g_hash_table_remove(resource_uuids, uuid);
+    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;
+}
+
+int virtio_lookup_dmabuf(const QemuUUID *uuid)
+{
+    VirtioSharedObject *vso = get_shared_object(uuid);
+    if (vso == NULL) {
+        return -1;
+    }
+    assert(vso->type == TYPE_DMABUF);
+    return GPOINTER_TO_INT(vso->value);
+}
+
+struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid)
+{
+    VirtioSharedObject *vso = get_shared_object(uuid);
+    if (vso == NULL) {
+        return NULL;
+    }
+    assert(vso->type == TYPE_VHOST_DEV);
+    return (struct vhost_dev *) vso->value;
+}
+
+SharedObjectType virtio_object_type(const QemuUUID *uuid)
+{
+    VirtioSharedObject *vso = get_shared_object(uuid);
+    if (vso == NULL) {
+        return TYPE_INVALID;
+    }
+    return vso->type;
+}
+
+void virtio_free_resources(void)
+{
+    g_mutex_lock(&lock);
+    g_hash_table_destroy(resource_uuids);
+    /* Reference count shall be 0 after the implicit unref on destroy */
+    resource_uuids = NULL;
+    g_mutex_unlock(&lock);
+}
diff --git a/include/hw/virtio/virtio-dmabuf.h b/include/hw/virtio/virtio-dmabuf.h
new file mode 100644
index 0000000000..202eec5868
--- /dev/null
+++ b/include/hw/virtio/virtio-dmabuf.h
@@ -0,0 +1,103 @@
+/*
+ * 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"
+#include "vhost.h"
+
+typedef enum SharedObjectType {
+    TYPE_INVALID = 0,
+    TYPE_DMABUF,
+    TYPE_VHOST_DEV,
+} SharedObjectType;
+
+typedef struct VirtioSharedObject {
+    SharedObjectType type;
+    gpointer value;
+} VirtioSharedObject;
+
+/**
+ * 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 caller retains ownership over the
+ *             descriptor and its lifecycle.
+ *
+ * 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_add_vhost_device() - Add a new exporter vhost device that holds the
+ * resource with the associated UUID
+ * @uuid: new resource's UUID
+ * @dev: the pointer to the vhost device that holds the resource. The caller
+ *       retains ownership over the device struct and its lifecycle.
+ *
+ * Return: true if the UUID did not exist and the device has been tracked,
+ * 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_vhost_device(QemuUUID *uuid, struct vhost_dev *dev);
+
+/**
+ * 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(const 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(const QemuUUID *uuid);
+
+/**
+ * virtio_lookup_vhost_device() - Looks for an exporter vhost device in the
+ * lookup table
+ * @uuid: resource's UUID
+ *
+ * Return: pointer to the vhost_dev struct, or NULL if the key is not found.
+ */
+struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid);
+
+/**
+ * virtio_object_type() - Looks for the type of resource in the lookup table
+ * @uuid: resource's UUID
+ *
+ * Return: the type of resource associated with the UUID, or TYPE_INVALID if
+ * the key is not found.
+ */
+SharedObjectType virtio_object_type(const QemuUUID *uuid);
+
+/**
+ * virtio_free_resources() - Destroys all keys and values of the shared
+ * resources lookup table, and frees them
+ */
+void virtio_free_resources(void);
+
+#endif /* VIRTIO_DMABUF_H */
diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index 93977cc32d..425ecc30fb 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..40fe262538
--- /dev/null
+++ b/tests/unit/test-virtio-dmabuf.c
@@ -0,0 +1,137 @@
+/*
+ * 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_add_remove_dev(void)
+{
+    QemuUUID uuid;
+    struct vhost_dev *dev = g_new0(struct vhost_dev, 1);
+    int i;
+
+    for (i = 0; i < 100; ++i) {
+        qemu_uuid_generate(&uuid);
+        virtio_add_vhost_device(&uuid, dev);
+        /* vhost device is found */
+        g_assert(virtio_lookup_vhost_device(&uuid) != NULL);
+        /* Remove the vhost device */
+        g_assert(virtio_remove_resource(&uuid));
+        /* vhost device is not found anymore */
+        g_assert(virtio_lookup_vhost_device(&uuid) == NULL);
+    }
+    g_free(dev);
+}
+
+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;
+    struct vhost_dev *dev = NULL;
+    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);
+        /* Add a new vhost device with invalid (NULL) pointer */
+        g_assert_false(virtio_add_vhost_device(&uuid, dev));
+        /* vhost device is not found */
+        g_assert(virtio_lookup_vhost_device(&uuid) == NULL);
+    }
+
+    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/add_rm_dev", test_add_remove_dev);
+    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.41.0

Re: [PATCH v6 2/3] virtio-dmabuf: introduce virtio-dmabuf

[Albert Esteve]

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

On Wed, Sep 6, 2023 at 1:15 PM Albert Esteve <aesteve@redhat.com> 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 <kraxel@redhat.com>
> Signed-off-by: Albert Esteve <aesteve@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-s390x@nongnu.org
>
> +virtio-dmabuf
> +M: Albert Esteve <aesteve@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 <stefanha@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 <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"
> +
> +
> +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;
> +}
> +
> +bool virtio_add_dmabuf(QemuUUID *uuid, int udmabuf_fd)
> +{
> +    bool result;
> +    VirtioSharedObject *vso;
> +    if (udmabuf_fd < 0) {
> +        return false;
> +    }
> +    vso = g_new(VirtioSharedObject, 1);
> +    vso->type = TYPE_DMABUF;
> +    vso->value = GINT_TO_POINTER(udmabuf_fd);
> +    result = virtio_add_resource(uuid, vso);
> +
> +    return result;
>

Just realized that the result variable is not required anymore
with the last change.

Not sure if it is worth sending a new review for this...
I can take it into account for a follow-up refactor for the next
update I make on the file. Or just send a separate trivial patch.


> +}
> +
> +bool virtio_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev)
> +{
> +    bool result;
> +    VirtioSharedObject *vso;
> +    if (dev == NULL) {
> +        return false;
> +    }
> +    vso = g_new(VirtioSharedObject, 1);
> +    vso->type = TYPE_VHOST_DEV;
> +    vso->value = dev;
> +    result = virtio_add_resource(uuid, vso);
> +
> +    return result;
> +}
> +
> +bool virtio_remove_resource(const QemuUUID *uuid)
> +{
> +    bool result;
> +    g_mutex_lock(&lock);
> +    result = g_hash_table_remove(resource_uuids, uuid);
> +    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;
> +}
> +
> +int virtio_lookup_dmabuf(const QemuUUID *uuid)
> +{
> +    VirtioSharedObject *vso = get_shared_object(uuid);
> +    if (vso == NULL) {
> +        return -1;
> +    }
> +    assert(vso->type == TYPE_DMABUF);
> +    return GPOINTER_TO_INT(vso->value);
> +}
> +
> +struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid)
> +{
> +    VirtioSharedObject *vso = get_shared_object(uuid);
> +    if (vso == NULL) {
> +        return NULL;
> +    }
> +    assert(vso->type == TYPE_VHOST_DEV);
> +    return (struct vhost_dev *) vso->value;
> +}
> +
> +SharedObjectType virtio_object_type(const QemuUUID *uuid)
> +{
> +    VirtioSharedObject *vso = get_shared_object(uuid);
> +    if (vso == NULL) {
> +        return TYPE_INVALID;
> +    }
> +    return vso->type;
> +}
> +
> +void virtio_free_resources(void)
> +{
> +    g_mutex_lock(&lock);
> +    g_hash_table_destroy(resource_uuids);
> +    /* Reference count shall be 0 after the implicit unref on destroy */
> +    resource_uuids = NULL;
> +    g_mutex_unlock(&lock);
> +}
> diff --git a/include/hw/virtio/virtio-dmabuf.h
> b/include/hw/virtio/virtio-dmabuf.h
> new file mode 100644
> index 0000000000..202eec5868
> --- /dev/null
> +++ b/include/hw/virtio/virtio-dmabuf.h
> @@ -0,0 +1,103 @@
> +/*
> + * 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"
> +#include "vhost.h"
> +
> +typedef enum SharedObjectType {
> +    TYPE_INVALID = 0,
> +    TYPE_DMABUF,
> +    TYPE_VHOST_DEV,
> +} SharedObjectType;
> +
> +typedef struct VirtioSharedObject {
> +    SharedObjectType type;
> +    gpointer value;
> +} VirtioSharedObject;
> +
> +/**
> + * 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 caller retains ownership over the
> + *             descriptor and its lifecycle.
> + *
> + * 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_add_vhost_device() - Add a new exporter vhost device that holds
> the
> + * resource with the associated UUID
> + * @uuid: new resource's UUID
> + * @dev: the pointer to the vhost device that holds the resource. The
> caller
> + *       retains ownership over the device struct and its lifecycle.
> + *
> + * Return: true if the UUID did not exist and the device has been tracked,
> + * 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_vhost_device(QemuUUID *uuid, struct vhost_dev *dev);
> +
> +/**
> + * 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(const 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(const QemuUUID *uuid);
> +
> +/**
> + * virtio_lookup_vhost_device() - Looks for an exporter vhost device in
> the
> + * lookup table
> + * @uuid: resource's UUID
> + *
> + * Return: pointer to the vhost_dev struct, or NULL if the key is not
> found.
> + */
> +struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid);
> +
> +/**
> + * virtio_object_type() - Looks for the type of resource in the lookup
> table
> + * @uuid: resource's UUID
> + *
> + * Return: the type of resource associated with the UUID, or TYPE_INVALID
> if
> + * the key is not found.
> + */
> +SharedObjectType virtio_object_type(const QemuUUID *uuid);
> +
> +/**
> + * virtio_free_resources() - Destroys all keys and values of the shared
> + * resources lookup table, and frees them
> + */
> +void virtio_free_resources(void);
> +
> +#endif /* VIRTIO_DMABUF_H */
> diff --git a/tests/unit/meson.build b/tests/unit/meson.build
> index 93977cc32d..425ecc30fb 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..40fe262538
> --- /dev/null
> +++ b/tests/unit/test-virtio-dmabuf.c
> @@ -0,0 +1,137 @@
> +/*
> + * 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_add_remove_dev(void)
> +{
> +    QemuUUID uuid;
> +    struct vhost_dev *dev = g_new0(struct vhost_dev, 1);
> +    int i;
> +
> +    for (i = 0; i < 100; ++i) {
> +        qemu_uuid_generate(&uuid);
> +        virtio_add_vhost_device(&uuid, dev);
> +        /* vhost device is found */
> +        g_assert(virtio_lookup_vhost_device(&uuid) != NULL);
> +        /* Remove the vhost device */
> +        g_assert(virtio_remove_resource(&uuid));
> +        /* vhost device is not found anymore */
> +        g_assert(virtio_lookup_vhost_device(&uuid) == NULL);
> +    }
> +    g_free(dev);
> +}
> +
> +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;
> +    struct vhost_dev *dev = NULL;
> +    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);
> +        /* Add a new vhost device with invalid (NULL) pointer */
> +        g_assert_false(virtio_add_vhost_device(&uuid, dev));
> +        /* vhost device is not found */
> +        g_assert(virtio_lookup_vhost_device(&uuid) == NULL);
> +    }
> +
> +    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/add_rm_dev", test_add_remove_dev);
> +    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.41.0
>
>

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

Re: [PATCH v6 2/3] virtio-dmabuf: introduce virtio-dmabuf

[Philippe Mathieu-Daudé]

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 <kraxel@redhat.com>
> Signed-off-by: Albert Esteve <aesteve@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-s390x@nongnu.org
>   
> +virtio-dmabuf
> +M: Albert Esteve <aesteve@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 <stefanha@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 <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"
> +
> +
> +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!

Re: [PATCH v6 2/3] virtio-dmabuf: introduce virtio-dmabuf

[Albert Esteve]

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

On Wed, Sep 6, 2023 at 4:21 PM Philippe Mathieu-Daudé <philmd@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 <kraxel@redhat.com>
> > Signed-off-by: Albert Esteve <aesteve@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-s390x@nongnu.org
> >
> > +virtio-dmabuf
> > +M: Albert Esteve <aesteve@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 <stefanha@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 <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"
> > +
> > +
> > +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!

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

[PATCH v6 3/3] vhost-user: add shared_object msg

[Albert Esteve]

Add three new vhost-user protocol
`VHOST_USER_BACKEND_SHARED_OBJECT_* messages`.
These new messages are sent from vhost-user
back-ends to interact with the virtio-dmabuf
table in order to add or remove themselves as
virtio exporters, or lookup for virtio dma-buf
shared objects.

The action taken in the front-end depends
on the type stored in the virtio shared
object hash table.

When the table holds a pointer to a vhost
backend for a given UUID, the front-end sends
a VHOST_USER_GET_SHARED_OBJECT to the
backend holding the shared object.

In the libvhost-user library we need to add
helper functions to allow sending messages to
interact with the virtio shared objects
hash table.

The messages can only be sent after successfully
negotiating a new VHOST_USER_PROTOCOL_F_SHARED_OBJECT
vhost-user protocol feature bit.

Finally, refactor code to send response message so
that all common parts both for the common REPLY_ACK
case, and other data responses, can call it and
avoid code repetition.

Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
 docs/interop/vhost-user.rst               |  57 +++++++
 hw/virtio/vhost-user.c                    | 174 ++++++++++++++++++++--
 include/hw/virtio/vhost-backend.h         |   3 +
 subprojects/libvhost-user/libvhost-user.c | 118 +++++++++++++++
 subprojects/libvhost-user/libvhost-user.h |  55 ++++++-
 5 files changed, 393 insertions(+), 14 deletions(-)

diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
index 5a070adbc1..415bb47a19 100644
--- a/docs/interop/vhost-user.rst
+++ b/docs/interop/vhost-user.rst
@@ -1440,6 +1440,18 @@ Front-end message types
   query the back-end for its device status as defined in the Virtio
   specification.
 
+``VHOST_USER_GET_SHARED_OBJECT``
+  :id: 41
+  :equivalent ioctl: N/A
+  :request payload: ``struct VhostUserShared``
+  :reply payload: dmabuf fd
+
+  When the ``VHOST_USER_PROTOCOL_F_SHARED_OBJECT`` protocol
+  feature has been successfully negotiated, and the UUID is found
+  in the exporters cache, this message is submitted by the front-end
+  to retrieve a given dma-buf fd from a given back-end, determined by
+  the requested UUID. Back-end will reply passing the fd when the operation
+  is successful, or no fd otherwise.
 
 Back-end message types
 ----------------------
@@ -1528,6 +1540,51 @@ is sent by the front-end.
 
   The state.num field is currently reserved and must be set to 0.
 
+``VHOST_USER_BACKEND_SHARED_OBJECT_ADD``
+  :id: 6
+  :equivalent ioctl: N/A
+  :request payload: ``struct VhostUserShared``
+  :reply payload: N/A
+
+  When the ``VHOST_USER_PROTOCOL_F_SHARED_OBJECT`` protocol
+  feature has been successfully negotiated, this message can be submitted
+  by the backends to add themselves as exporters to the virtio shared lookup
+  table. The back-end device gets associated with a UUID in the shared table.
+  The back-end is responsible of keeping its own table with exported dma-buf fds.
+  When another back-end tries to import the resource associated with the UUID,
+  it will send a message to the front-end, which will act as a proxy to the
+  exporter back-end. If ``VHOST_USER_PROTOCOL_F_REPLY_ACK`` is negotiated, and
+  the back-end sets the ``VHOST_USER_NEED_REPLY`` flag, the front-end must
+  respond with zero when operation is successfully completed, or non-zero
+  otherwise.
+
+``VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE``
+  :id: 7
+  :equivalent ioctl: N/A
+  :request payload: ``struct VhostUserShared``
+  :reply payload: N/A
+
+  When the ``VHOST_USER_PROTOCOL_F_SHARED_OBJECT`` protocol
+  feature has been successfully negotiated, this message can be submitted
+  by the backend to remove themselves from to the virtio-dmabuf shared
+  table API. The shared table will remove the back-end device associated with
+  the UUID. If ``VHOST_USER_PROTOCOL_F_REPLY_ACK`` is negotiated, and the
+  back-end sets the ``VHOST_USER_NEED_REPLY`` flag, the front-end must respond
+  with zero when operation is successfully completed, or non-zero otherwise.
+
+``VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP``
+  :id: 8
+  :equivalent ioctl: N/A
+  :request payload: ``struct VhostUserShared``
+  :reply payload: dmabuf fd and ``u64``
+
+  When the ``VHOST_USER_PROTOCOL_F_SHARED_OBJECT`` protocol
+  feature has been successfully negotiated, this message can be submitted
+  by the backends to retrieve a given dma-buf fd from the virtio-dmabuf
+  shared table given a UUID. Frontend will reply passing the fd and a zero
+  when the operation is successful, or non-zero otherwise. Note that if the
+  operation fails, no fd is sent to the backend.
+
 .. _reply_ack:
 
 VHOST_USER_PROTOCOL_F_REPLY_ACK
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 8dcf049d42..28fa0ace42 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -10,6 +10,7 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "hw/virtio/virtio-dmabuf.h"
 #include "hw/virtio/vhost.h"
 #include "hw/virtio/virtio-crypto.h"
 #include "hw/virtio/vhost-user.h"
@@ -21,6 +22,7 @@
 #include "sysemu/kvm.h"
 #include "qemu/error-report.h"
 #include "qemu/main-loop.h"
+#include "qemu/uuid.h"
 #include "qemu/sockets.h"
 #include "sysemu/runstate.h"
 #include "sysemu/cryptodev.h"
@@ -74,6 +76,7 @@ enum VhostUserProtocolFeature {
     /* Feature 14 reserved for VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS. */
     VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15,
     VHOST_USER_PROTOCOL_F_STATUS = 16,
+    VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 17,
     VHOST_USER_PROTOCOL_F_MAX
 };
 
@@ -121,6 +124,7 @@ typedef enum VhostUserRequest {
     VHOST_USER_REM_MEM_REG = 38,
     VHOST_USER_SET_STATUS = 39,
     VHOST_USER_GET_STATUS = 40,
+    VHOST_USER_GET_SHARED_OBJECT = 41,
     VHOST_USER_MAX
 } VhostUserRequest;
 
@@ -129,6 +133,9 @@ typedef enum VhostUserBackendRequest {
     VHOST_USER_BACKEND_IOTLB_MSG = 1,
     VHOST_USER_BACKEND_CONFIG_CHANGE_MSG = 2,
     VHOST_USER_BACKEND_VRING_HOST_NOTIFIER_MSG = 3,
+    VHOST_USER_BACKEND_SHARED_OBJECT_ADD = 6,
+    VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE = 7,
+    VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP = 8,
     VHOST_USER_BACKEND_MAX
 }  VhostUserBackendRequest;
 
@@ -202,6 +209,10 @@ typedef struct VhostUserInflight {
     uint16_t queue_size;
 } VhostUserInflight;
 
+typedef struct VhostUserShared {
+    unsigned char uuid[16];
+} VhostUserShared;
+
 typedef struct {
     VhostUserRequest request;
 
@@ -226,6 +237,7 @@ typedef union {
         VhostUserCryptoSession session;
         VhostUserVringArea area;
         VhostUserInflight inflight;
+        VhostUserShared object;
 } VhostUserPayload;
 
 typedef struct VhostUserMsg {
@@ -1601,6 +1613,144 @@ static int vhost_user_backend_handle_vring_host_notifier(struct vhost_dev *dev,
     return 0;
 }
 
+static int
+vhost_user_backend_handle_shared_object_add(struct vhost_dev *dev,
+                                            VhostUserShared *object)
+{
+    QemuUUID uuid;
+
+    memcpy(uuid.data, object->uuid, sizeof(object->uuid));
+    return virtio_add_vhost_device(&uuid, dev);
+}
+
+static int
+vhost_user_backend_handle_shared_object_remove(VhostUserShared *object)
+{
+    QemuUUID uuid;
+
+    memcpy(uuid.data, object->uuid, sizeof(object->uuid));
+    return virtio_remove_resource(&uuid);
+}
+
+static bool vhost_user_send_resp(QIOChannel *ioc, VhostUserHeader *hdr,
+                                 VhostUserPayload *payload)
+{
+    Error *local_err = NULL;
+    struct iovec iov[] = {
+        { .iov_base = hdr,      .iov_len = VHOST_USER_HDR_SIZE },
+        { .iov_base = payload,  .iov_len = hdr->size },
+    };
+
+    hdr->flags &= ~VHOST_USER_NEED_REPLY_MASK;
+    hdr->flags |= VHOST_USER_REPLY_MASK;
+
+    if (qio_channel_writev_all(ioc, iov, ARRAY_SIZE(iov), &local_err)) {
+        error_report_err(local_err);
+        return false;
+    }
+
+    return true;
+}
+
+static bool
+vhost_user_backend_send_dmabuf_fd(QIOChannel *ioc, VhostUserHeader *hdr,
+                                  VhostUserPayload *payload)
+{
+    hdr->size = sizeof(payload->u64);
+    return vhost_user_send_resp(ioc, hdr, payload);
+}
+
+int vhost_user_get_shared_object(struct vhost_dev *dev, unsigned char *uuid,
+                                 int *dmabuf_fd)
+{
+    struct vhost_user *u = dev->opaque;
+    CharBackend *chr = u->user->chr;
+    int ret;
+    VhostUserMsg msg = {
+        .hdr.request = VHOST_USER_GET_SHARED_OBJECT,
+        .hdr.flags = VHOST_USER_VERSION,
+    };
+    memcpy(msg.payload.object.uuid, uuid, sizeof(msg.payload.object.uuid));
+
+    ret = vhost_user_write(dev, &msg, NULL, 0);
+    if (ret < 0) {
+        return ret;
+    }
+
+    ret = vhost_user_read(dev, &msg);
+    if (ret < 0) {
+        return ret;
+    }
+
+    if (msg.hdr.request != VHOST_USER_GET_SHARED_OBJECT) {
+        error_report("Received unexpected msg type. "
+                     "Expected %d received %d",
+                     VHOST_USER_GET_SHARED_OBJECT, msg.hdr.request);
+        return -EPROTO;
+    }
+
+    *dmabuf_fd = qemu_chr_fe_get_msgfd(chr);
+    if (*dmabuf_fd < 0) {
+        error_report("Failed to get dmabuf fd");
+        return -EIO;
+    }
+
+    return 0;
+}
+
+static int
+vhost_user_backend_handle_shared_object_lookup(struct vhost_user *u,
+                                               QIOChannel *ioc,
+                                               VhostUserHeader *hdr,
+                                               VhostUserPayload *payload)
+{
+    QemuUUID uuid;
+    CharBackend *chr = u->user->chr;
+    int dmabuf_fd = -1;
+    int fd_num = 0;
+
+    memcpy(uuid.data, payload->object.uuid, sizeof(payload->object.uuid));
+
+    payload->u64 = 0;
+    switch (virtio_object_type(&uuid)) {
+        case TYPE_DMABUF:
+            dmabuf_fd = virtio_lookup_dmabuf(&uuid);
+            break;
+        case TYPE_VHOST_DEV:
+        {
+            struct vhost_dev *dev = virtio_lookup_vhost_device(&uuid);
+            if (dev == NULL) {
+                payload->u64 = -EINVAL;
+                break;
+            }
+            int ret = vhost_user_get_shared_object(dev, uuid.data, &dmabuf_fd);
+            if (ret < 0) {
+                payload->u64 = ret;
+            }
+            break;
+        }
+        case TYPE_INVALID:
+            payload->u64 = -EINVAL;
+            break;
+    }
+
+    if (dmabuf_fd != -1) {
+        fd_num++;
+    }
+
+    if (qemu_chr_fe_set_msgfds(chr, &dmabuf_fd, fd_num) < 0) {
+        error_report("Failed to set msg fds.");
+        payload->u64 = -EINVAL;
+    }
+
+    if (!vhost_user_backend_send_dmabuf_fd(ioc, hdr, payload)) {
+        error_report("Failed to write response msg.");
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
 static void close_backend_channel(struct vhost_user *u)
 {
     g_source_destroy(u->backend_src);
@@ -1658,6 +1808,16 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition,
         ret = vhost_user_backend_handle_vring_host_notifier(dev, &payload.area,
                                                           fd ? fd[0] : -1);
         break;
+    case VHOST_USER_BACKEND_SHARED_OBJECT_ADD:
+        ret = vhost_user_backend_handle_shared_object_add(dev, &payload.object);
+        break;
+    case VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE:
+        ret = vhost_user_backend_handle_shared_object_remove(&payload.object);
+        break;
+    case VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP:
+        ret = vhost_user_backend_handle_shared_object_lookup(dev->opaque, ioc,
+                                                             &hdr, &payload);
+        break;
     default:
         error_report("Received unexpected msg type: %d.", hdr.request);
         ret = -EINVAL;
@@ -1668,22 +1828,10 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition,
      * directly in their request handlers.
      */
     if (hdr.flags & VHOST_USER_NEED_REPLY_MASK) {
-        struct iovec iovec[2];
-
-
-        hdr.flags &= ~VHOST_USER_NEED_REPLY_MASK;
-        hdr.flags |= VHOST_USER_REPLY_MASK;
-
         payload.u64 = !!ret;
         hdr.size = sizeof(payload.u64);
 
-        iovec[0].iov_base = &hdr;
-        iovec[0].iov_len = VHOST_USER_HDR_SIZE;
-        iovec[1].iov_base = &payload;
-        iovec[1].iov_len = hdr.size;
-
-        if (qio_channel_writev_all(ioc, iovec, ARRAY_SIZE(iovec), &local_err)) {
-            error_report_err(local_err);
+        if (!vhost_user_send_resp(ioc, &hdr, &payload)) {
             goto err;
         }
     }
diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
index 31a251a9f5..1860b541d8 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -196,4 +196,7 @@ int vhost_backend_handle_iotlb_msg(struct vhost_dev *dev,
 
 int vhost_user_gpu_set_socket(struct vhost_dev *dev, int fd);
 
+int vhost_user_get_shared_object(struct vhost_dev *dev, unsigned char *uuid,
+                                        int *dmabuf_fd);
+
 #endif /* VHOST_BACKEND_H */
diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index 0469a50101..432bb9fc0b 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -161,6 +161,7 @@ vu_request_to_string(unsigned int req)
         REQ(VHOST_USER_GET_MAX_MEM_SLOTS),
         REQ(VHOST_USER_ADD_MEM_REG),
         REQ(VHOST_USER_REM_MEM_REG),
+        REQ(VHOST_USER_GET_SHARED_OBJECT),
         REQ(VHOST_USER_MAX),
     };
 #undef REQ
@@ -900,6 +901,23 @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
     return false;
 }
 
+static bool
+vu_get_shared_object(VuDev *dev, VhostUserMsg *vmsg)
+{
+    int fd_num = 0;
+    int dmabuf_fd = -1;
+    if (dev->iface->get_shared_object) {
+        dmabuf_fd = dev->iface->get_shared_object(dev, &vmsg->payload.object.uuid[0]);
+    }
+    if (dmabuf_fd != -1) {
+        DPRINT("dmabuf_fd found for requested UUID\n");
+        vmsg->fds[fd_num++] = dmabuf_fd;
+    }
+    vmsg->fd_num = fd_num;
+
+    return true;
+}
+
 static bool
 vu_set_mem_table_exec_postcopy(VuDev *dev, VhostUserMsg *vmsg)
 {
@@ -1403,6 +1421,104 @@ bool vu_set_queue_host_notifier(VuDev *dev, VuVirtq *vq, int fd,
     return vu_process_message_reply(dev, &vmsg);
 }
 
+bool
+vu_lookup_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN], int *dmabuf_fd)
+{
+    bool result = false;
+    VhostUserMsg msg_reply;
+    VhostUserMsg msg = {
+        .request = VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP,
+        .size = sizeof(msg.payload.object),
+        .flags = VHOST_USER_VERSION | VHOST_USER_NEED_REPLY_MASK,
+    };
+
+    memcpy(msg.payload.object.uuid, uuid, sizeof(uuid[0]) * UUID_LEN);
+
+    if (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SHARED_OBJECT)) {
+        return false;
+    }
+
+    pthread_mutex_lock(&dev->backend_mutex);
+    if (!vu_message_write(dev, dev->backend_fd, &msg)) {
+        goto out;
+    }
+
+    if (!vu_message_read_default(dev, dev->backend_fd, &msg_reply)) {
+        goto out;
+    }
+
+    if (msg_reply.request != msg.request) {
+        DPRINT("Received unexpected msg type. Expected %d, received %d",
+               msg.request, msg_reply.request);
+        goto out;
+    }
+
+    if (msg_reply.fd_num != 1) {
+        DPRINT("Received unexpected number of fds. Expected 1, received %d",
+               msg_reply.fd_num);
+        goto out;
+    }
+
+    *dmabuf_fd = msg_reply.fds[0];
+    result = *dmabuf_fd > 0 && msg_reply.payload.u64 == 0;
+out:
+    pthread_mutex_unlock(&dev->backend_mutex);
+
+    return result;
+}
+
+static bool
+vu_send_message(VuDev *dev, VhostUserMsg *vmsg)
+{
+    bool result = false;
+    pthread_mutex_lock(&dev->backend_mutex);
+    if (!vu_message_write(dev, dev->backend_fd, vmsg)) {
+        goto out;
+    }
+
+    result = true;
+out:
+    pthread_mutex_unlock(&dev->backend_mutex);
+
+    return result;
+}
+
+bool
+vu_add_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN])
+{
+    VhostUserMsg msg = {
+        .request = VHOST_USER_BACKEND_SHARED_OBJECT_ADD,
+        .size = sizeof(msg.payload.object),
+        .flags = VHOST_USER_VERSION,
+    };
+
+    memcpy(msg.payload.object.uuid, uuid, sizeof(uuid[0]) * UUID_LEN);
+
+    if (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SHARED_OBJECT)) {
+        return false;
+    }
+
+    return vu_send_message(dev, &msg);
+}
+
+bool
+vu_rm_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN])
+{
+    VhostUserMsg msg = {
+        .request = VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE,
+        .size = sizeof(msg.payload.object),
+        .flags = VHOST_USER_VERSION,
+    };
+
+    memcpy(msg.payload.object.uuid, uuid, sizeof(uuid[0]) * UUID_LEN);
+
+    if (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SHARED_OBJECT)) {
+        return false;
+    }
+
+    return vu_send_message(dev, &msg);
+}
+
 static bool
 vu_set_vring_call_exec(VuDev *dev, VhostUserMsg *vmsg)
 {
@@ -1943,6 +2059,8 @@ vu_process_message(VuDev *dev, VhostUserMsg *vmsg)
         return vu_add_mem_reg(dev, vmsg);
     case VHOST_USER_REM_MEM_REG:
         return vu_rem_mem_reg(dev, vmsg);
+    case VHOST_USER_GET_SHARED_OBJECT:
+        return vu_get_shared_object(dev, vmsg);
     default:
         vmsg_close_fds(vmsg);
         vu_panic(dev, "Unhandled request: %d", vmsg->request);
diff --git a/subprojects/libvhost-user/libvhost-user.h b/subprojects/libvhost-user/libvhost-user.h
index 708370c5f5..b36a42a7ca 100644
--- a/subprojects/libvhost-user/libvhost-user.h
+++ b/subprojects/libvhost-user/libvhost-user.h
@@ -64,7 +64,8 @@ enum VhostUserProtocolFeature {
     VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD = 12,
     VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14,
     VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15,
-
+    /* Feature 16 is reserved for VHOST_USER_PROTOCOL_F_STATUS. */
+    VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 17,
     VHOST_USER_PROTOCOL_F_MAX
 };
 
@@ -109,6 +110,7 @@ typedef enum VhostUserRequest {
     VHOST_USER_GET_MAX_MEM_SLOTS = 36,
     VHOST_USER_ADD_MEM_REG = 37,
     VHOST_USER_REM_MEM_REG = 38,
+    VHOST_USER_GET_SHARED_OBJECT = 41,
     VHOST_USER_MAX
 } VhostUserRequest;
 
@@ -119,6 +121,9 @@ typedef enum VhostUserBackendRequest {
     VHOST_USER_BACKEND_VRING_HOST_NOTIFIER_MSG = 3,
     VHOST_USER_BACKEND_VRING_CALL = 4,
     VHOST_USER_BACKEND_VRING_ERR = 5,
+    VHOST_USER_BACKEND_SHARED_OBJECT_ADD = 6,
+    VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE = 7,
+    VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP = 8,
     VHOST_USER_BACKEND_MAX
 }  VhostUserBackendRequest;
 
@@ -172,6 +177,12 @@ typedef struct VhostUserInflight {
     uint16_t queue_size;
 } VhostUserInflight;
 
+#define UUID_LEN 16
+
+typedef struct VhostUserShared {
+    unsigned char uuid[UUID_LEN];
+} VhostUserShared;
+
 #if defined(_WIN32) && (defined(__x86_64__) || defined(__i386__))
 # define VU_PACKED __attribute__((gcc_struct, packed))
 #else
@@ -199,6 +210,7 @@ typedef struct VhostUserMsg {
         VhostUserConfig config;
         VhostUserVringArea area;
         VhostUserInflight inflight;
+        VhostUserShared object;
     } payload;
 
     int fds[VHOST_MEMORY_BASELINE_NREGIONS];
@@ -232,6 +244,7 @@ typedef int (*vu_get_config_cb) (VuDev *dev, uint8_t *config, uint32_t len);
 typedef int (*vu_set_config_cb) (VuDev *dev, const uint8_t *data,
                                  uint32_t offset, uint32_t size,
                                  uint32_t flags);
+typedef int (*vu_get_shared_object_cb) (VuDev *dev, const unsigned char *uuid);
 
 typedef struct VuDevIface {
     /* called by VHOST_USER_GET_FEATURES to get the features bitmask */
@@ -258,6 +271,8 @@ typedef struct VuDevIface {
     vu_get_config_cb get_config;
     /* set the config space of the device */
     vu_set_config_cb set_config;
+    /* get virtio shared object from the underlying vhost implementation. */
+    vu_get_shared_object_cb get_shared_object;
 } VuDevIface;
 
 typedef void (*vu_queue_handler_cb) (VuDev *dev, int qidx);
@@ -541,6 +556,44 @@ void vu_set_queue_handler(VuDev *dev, VuVirtq *vq,
 bool vu_set_queue_host_notifier(VuDev *dev, VuVirtq *vq, int fd,
                                 int size, int offset);
 
+/**
+ * vu_lookup_shared_object:
+ * @dev: a VuDev context
+ * @uuid: UUID of the shared object
+ * @dmabuf_fd: output dma-buf file descriptor
+ *
+ * Lookup for a virtio shared object (i.e., dma-buf fd) associated with the
+ * received UUID. Result, if found, is stored in the dmabuf_fd argument.
+ *
+ * Returns: whether the virtio object was found.
+ */
+bool vu_lookup_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN],
+                             int *dmabuf_fd);
+
+/**
+ * vu_add_shared_object:
+ * @dev: a VuDev context
+ * @uuid: UUID of the shared object
+ *
+ * Registers this back-end as the exporter for the object associated with
+ * the received UUID.
+ *
+ * Returns: TRUE on success, FALSE on failure.
+ */
+bool vu_add_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN]);
+
+/**
+ * vu_rm_shared_object:
+ * @dev: a VuDev context
+ * @uuid: UUID of the shared object
+ *
+ * Removes a shared object entry (i.e., back-end entry) associated with the
+ * received UUID key from the hash table.
+ *
+ * Returns: TRUE on success, FALSE on failure.
+ */
+bool vu_rm_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN]);
+
 /**
  * vu_queue_set_notification:
  * @dev: a VuDev context
-- 
2.41.0

Re: [PATCH v6 3/3] vhost-user: add shared_object msg

[Philippe Mathieu-Daudé]

Hi Albert,

On 6/9/23 13:15, Albert Esteve wrote:
> Add three new vhost-user protocol
> `VHOST_USER_BACKEND_SHARED_OBJECT_* messages`.
> These new messages are sent from vhost-user
> back-ends to interact with the virtio-dmabuf
> table in order to add or remove themselves as
> virtio exporters, or lookup for virtio dma-buf
> shared objects.
> 
> The action taken in the front-end depends
> on the type stored in the virtio shared
> object hash table.
> 
> When the table holds a pointer to a vhost
> backend for a given UUID, the front-end sends
> a VHOST_USER_GET_SHARED_OBJECT to the
> backend holding the shared object.
> 
> In the libvhost-user library we need to add
> helper functions to allow sending messages to
> interact with the virtio shared objects
> hash table.
> 
> The messages can only be sent after successfully
> negotiating a new VHOST_USER_PROTOCOL_F_SHARED_OBJECT
> vhost-user protocol feature bit.
> 
> Finally, refactor code to send response message so
> that all common parts both for the common REPLY_ACK
> case, and other data responses, can call it and
> avoid code repetition.
> 
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> ---
>   docs/interop/vhost-user.rst               |  57 +++++++
>   hw/virtio/vhost-user.c                    | 174 ++++++++++++++++++++--
>   include/hw/virtio/vhost-backend.h         |   3 +
>   subprojects/libvhost-user/libvhost-user.c | 118 +++++++++++++++
>   subprojects/libvhost-user/libvhost-user.h |  55 ++++++-
>   5 files changed, 393 insertions(+), 14 deletions(-)

Almost 400 lines of changes is too much for me to review in a
single patch. Looking at the names, can't we split virtio VS
libvhost-user?

> +static bool vhost_user_send_resp(QIOChannel *ioc, VhostUserHeader *hdr,
> +                                 VhostUserPayload *payload)
> +{
> +    Error *local_err = NULL;
> +    struct iovec iov[] = {
> +        { .iov_base = hdr,      .iov_len = VHOST_USER_HDR_SIZE },
> +        { .iov_base = payload,  .iov_len = hdr->size },
> +    };
> +
> +    hdr->flags &= ~VHOST_USER_NEED_REPLY_MASK;
> +    hdr->flags |= VHOST_USER_REPLY_MASK;
> +
> +    if (qio_channel_writev_all(ioc, iov, ARRAY_SIZE(iov), &local_err)) {
> +        error_report_err(local_err);
> +        return false;
> +    }
> +
> +    return true;
> +}

I note you  ignored my comment regarding adding a 'Error **' argument in
the previous version:
https://lore.kernel.org/qemu-devel/911eef0c-d04f-2fcf-e78b-2475cd7af8f0@linaro.org/

Re: [PATCH v6 3/3] vhost-user: add shared_object msg

[Albert Esteve]

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

On Wed, Sep 6, 2023 at 4:27 PM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> Hi Albert,
>
> On 6/9/23 13:15, Albert Esteve wrote:
> > Add three new vhost-user protocol
> > `VHOST_USER_BACKEND_SHARED_OBJECT_* messages`.
> > These new messages are sent from vhost-user
> > back-ends to interact with the virtio-dmabuf
> > table in order to add or remove themselves as
> > virtio exporters, or lookup for virtio dma-buf
> > shared objects.
> >
> > The action taken in the front-end depends
> > on the type stored in the virtio shared
> > object hash table.
> >
> > When the table holds a pointer to a vhost
> > backend for a given UUID, the front-end sends
> > a VHOST_USER_GET_SHARED_OBJECT to the
> > backend holding the shared object.
> >
> > In the libvhost-user library we need to add
> > helper functions to allow sending messages to
> > interact with the virtio shared objects
> > hash table.
> >
> > The messages can only be sent after successfully
> > negotiating a new VHOST_USER_PROTOCOL_F_SHARED_OBJECT
> > vhost-user protocol feature bit.
> >
> > Finally, refactor code to send response message so
> > that all common parts both for the common REPLY_ACK
> > case, and other data responses, can call it and
> > avoid code repetition.
> >
> > Signed-off-by: Albert Esteve <aesteve@redhat.com>
> > ---
> >   docs/interop/vhost-user.rst               |  57 +++++++
> >   hw/virtio/vhost-user.c                    | 174 ++++++++++++++++++++--
> >   include/hw/virtio/vhost-backend.h         |   3 +
> >   subprojects/libvhost-user/libvhost-user.c | 118 +++++++++++++++
> >   subprojects/libvhost-user/libvhost-user.h |  55 ++++++-
> >   5 files changed, 393 insertions(+), 14 deletions(-)
>
> Almost 400 lines of changes is too much for me to review in a
> single patch. Looking at the names, can't we split virtio VS
> libvhost-user?
>

Ack.


>
> > +static bool vhost_user_send_resp(QIOChannel *ioc, VhostUserHeader *hdr,
> > +                                 VhostUserPayload *payload)
> > +{
> > +    Error *local_err = NULL;
> > +    struct iovec iov[] = {
> > +        { .iov_base = hdr,      .iov_len = VHOST_USER_HDR_SIZE },
> > +        { .iov_base = payload,  .iov_len = hdr->size },
> > +    };
> > +
> > +    hdr->flags &= ~VHOST_USER_NEED_REPLY_MASK;
> > +    hdr->flags |= VHOST_USER_REPLY_MASK;
> > +
> > +    if (qio_channel_writev_all(ioc, iov, ARRAY_SIZE(iov), &local_err)) {
> > +        error_report_err(local_err);
> > +        return false;
> > +    }
> > +
> > +    return true;
> > +}
>
> I note you  ignored my comment regarding adding a 'Error **' argument in
> the previous version:
>
> https://lore.kernel.org/qemu-devel/911eef0c-d04f-2fcf-e78b-2475cd7af8f0@linaro.org/
>
> Sorry I missed those comments somehow :/
I'll check them and resend.

BR,
Albert

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

Re: [PATCH v6 3/3] vhost-user: add shared_object msg

[Philippe Mathieu-Daudé]

On 6/9/23 16:33, Albert Esteve wrote:

>     I note you  ignored my comment regarding adding a 'Error **' argument in
>     the previous version:
>     https://lore.kernel.org/qemu-devel/911eef0c-d04f-2fcf-e78b-2475cd7af8f0@linaro.org/ <https://lore.kernel.org/qemu-devel/911eef0c-d04f-2fcf-e78b-2475cd7af8f0@linaro.org/>
> 
> Sorry I missed those comments somehow :/

Ah, I see.

> I'll check them and resend.

You can also object to them, explaining why this isn't really
useful, if you think so. But first read the big comment in
include/qapi/error.h.

Thanks,

Phil.

Re: [PATCH v6 3/3] vhost-user: add shared_object msg

[Albert Esteve]

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

On Wed, Sep 6, 2023 at 4:43 PM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> On 6/9/23 16:33, Albert Esteve wrote:
>
> >     I note you  ignored my comment regarding adding a 'Error **'
> argument in
> >     the previous version:
> >
> https://lore.kernel.org/qemu-devel/911eef0c-d04f-2fcf-e78b-2475cd7af8f0@linaro.org/
> <
> https://lore.kernel.org/qemu-devel/911eef0c-d04f-2fcf-e78b-2475cd7af8f0@linaro.org/
> >
> >
> > Sorry I missed those comments somehow :/
>
> Ah, I see.
>
> > I'll check them and resend.
>
> You can also object to them, explaining why this isn't really
> useful, if you think so. But first read the big comment in
> include/qapi/error.h.
>
>
Sure, I understand. So far I tend to trust the judgement of the more
experienced
Qemu developers over my own, but if I wouldn't agree with what is suggested
I would object :)
So:
- Regarding the two functions with the same, seems to be solved with the
squash before,
  and it was probably causing the compile error to begin with, so one less
thing to worry about!
- Regarding splitting the commit, sure, no problem. I'll ensure they do
compile separately.
- Regarding the error, I read the long comment in the error file and
checked surrounding code. I think
  you are right and will be better propagating the error.

And I think I would address all your comments with that! Thanks for the
feedback!


> Thanks,
>
> Phil.
>
>

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

Re: [PATCH v6 3/3] vhost-user: add shared_object msg

[Albert Esteve]

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

On Wed, Sep 6, 2023 at 6:01 PM Albert Esteve <aesteve@redhat.com> wrote:

>
>
> On Wed, Sep 6, 2023 at 4:43 PM Philippe Mathieu-Daudé <philmd@linaro.org>
> wrote:
>
>> On 6/9/23 16:33, Albert Esteve wrote:
>>
>> >     I note you  ignored my comment regarding adding a 'Error **'
>> argument in
>> >     the previous version:
>> >
>> https://lore.kernel.org/qemu-devel/911eef0c-d04f-2fcf-e78b-2475cd7af8f0@linaro.org/
>> <
>> https://lore.kernel.org/qemu-devel/911eef0c-d04f-2fcf-e78b-2475cd7af8f0@linaro.org/
>> >
>> >
>> > Sorry I missed those comments somehow :/
>>
>> Ah, I see.
>>
>> > I'll check them and resend.
>>
>> You can also object to them, explaining why this isn't really
>> useful, if you think so. But first read the big comment in
>> include/qapi/error.h.
>>
>>
> Sure, I understand. So far I tend to trust the judgement of the more
> experienced
> Qemu developers over my own, but if I wouldn't agree with what is
> suggested I would object :)
> So:
> - Regarding the two functions with the same, seems to be solved with the
> squash before,
>   and it was probably causing the compile error to begin with, so one less
> thing to worry about!
> - Regarding splitting the commit, sure, no problem. I'll ensure they do
> compile separately.
> - Regarding the error, I read the long comment in the error file and
> checked surrounding code. I think
>   you are right and will be better propagating the error.
>

But I think I will omit the Error propagation for
`vhost_user_backend_handle_shared_object_lookup`.
In this function returning an error code does not necessarily mean that we
should log an error.
So if we pass the local_err from the backend_read function to the handler,
we cannot be sure
when we should actually print the log.
`vhost_backend_handle_iotlb_msg` has the same issue and does not propagate
the error either,
relies solely on `error_report` calls.

Therefore, I will propagate it for `vhost_user_send_resp` and
`vhost_user_backend_send_dmabuf_fd` only.


>
> And I think I would address all your comments with that! Thanks for the
> feedback!
>
>
>> Thanks,
>>
>> Phil.
>>
>>

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

Re: [PATCH v6 3/3] vhost-user: add shared_object msg

[Philippe Mathieu-Daudé]

On 6/9/23 18:18, Albert Esteve wrote:

>     So:
>     - Regarding the two functions with the same, seems to be solved with
>     the squash before,
>        and it was probably causing the compile error to begin with, so
>     one less thing to worry about!
>     - Regarding splitting the commit, sure, no problem. I'll ensure they
>     do compile separately.
>     - Regarding the error, I read the long comment in the error file and
>     checked surrounding code. I think
>        you are right and will be better propagating the error.
> 
> 
> But I think I will omit the Error propagation for 
> `vhost_user_backend_handle_shared_object_lookup`.
> In this function returning an error code does not necessarily mean that 
> we should log an error.
> So if we pass the local_err from the backend_read function to the 
> handler, we cannot be sure
> when we should actually print the log.
> `vhost_backend_handle_iotlb_msg` has the same issue and does not 
> propagate the error either,
> relies solely on `error_report` calls.
> 
> Therefore, I will propagate it for `vhost_user_send_resp` and 
> `vhost_user_backend_send_dmabuf_fd` only.

Sounds good!