[PATCH 0/4] Virtio shared dma-buf

[PATCH 0/4] Virtio shared dma-buf

[Albert Esteve]

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:

Shared resources table, to hold all resources that can be shared in the host and their assigned UUID
Internal shared table API for virtio devices to add, lookup and remove resources
Unit test to verify the API.
New message to the vhost-user protocol to allow backend to interact with the shared
table API through the control socket

Applies cleanly to 4ebc33f

[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 (4):
  virtio-dmabuf: introduce virtio-dmabuf
  vhost-user: add shared_object msg
  vhost-user: refactor send_resp code
  libvhost-user: add write_msg cb to dev struct

 docs/interop/vhost-user.rst               |  15 +++
 hw/display/meson.build                    |   1 +
 hw/display/virtio-dmabuf.c                |  88 +++++++++++++++++
 hw/virtio/vhost-user.c                    |  90 ++++++++++++++---
 include/hw/virtio/virtio-dmabuf.h         |  58 +++++++++++
 subprojects/libvhost-user/libvhost-user.c |  40 ++++++++
 subprojects/libvhost-user/libvhost-user.h |  46 +++++++++
 tests/unit/meson.build                    |   1 +
 tests/unit/test-virtio-dmabuf.c           | 112 ++++++++++++++++++++++
 9 files changed, 438 insertions(+), 13 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.40.0

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

[Albert Esteve]

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)))))
+
+
+static GMutex lock;
+static GHashTable *resource_uuids;
+
+
+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);
+    } else if (g_hash_table_lookup(resource_uuids, struuid) != NULL) {
+        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
+ *
+ * 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);
+
+#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

[PATCH 2/4] vhost-user: add shared_object msg

[Albert Esteve]

Add new vhost-user protocol message
`VHOST_USER_BACKEND_SHARED_OBJECT`. This new
message is sent from vhost-user back-ends
to interact with the virtio-dmabuf table
in order to add, remove, or lookup for
virtio dma-buf shared objects.

The action taken in the front-end depends
on the type stored in the payload struct.

In the libvhost-user library add a helper
function (`vu_get_shared_object`) to send a
message to lookup for the dma-buf given the
UUID. Other operations, i.e., add and remove
entries in the shared objects table, will have
to be handled directly in the back-end.

Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
 docs/interop/vhost-user.rst               | 15 +++++
 hw/virtio/vhost-user.c                    | 68 +++++++++++++++++++++++
 subprojects/libvhost-user/libvhost-user.c | 39 +++++++++++++
 subprojects/libvhost-user/libvhost-user.h | 30 ++++++++++
 4 files changed, 152 insertions(+)

diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
index 5a070adbc1..d3d8db41e5 100644
--- a/docs/interop/vhost-user.rst
+++ b/docs/interop/vhost-user.rst
@@ -1528,6 +1528,21 @@ is sent by the front-end.
 
   The state.num field is currently reserved and must be set to 0.
 
+``VHOST_USER_BACKEND_SHARED_OBJECT``
+  :id: 6
+  :equivalent ioctl: N/A
+  :request payload: ``struct VhostUserShared``
+  :reply payload: ``struct VhostUserShared`` (only for ``LOOKUP`` requests)
+
+  Backends that need to interact with the virtio-dmabuf shared table API
+  can send this message. The operation is determined by the ``type`` member
+  of the payload struct. The valid values for the operation type are
+  ``VHOST_SHARED_OBJECT_*`` members, i.e., ``ADD``, ``LOOKUP``, and ``REMOVE``.
+  ``LOOKUP`` operations require the ``VHOST_USER_NEED_REPLY_MASK`` flag to be
+  set by the back-end, and the front-end will then send the dma-buf fd as
+  a response if the UUID matches an object in the table, or a negative value
+  otherwise.
+
 .. _reply_ack:
 
 VHOST_USER_PROTOCOL_F_REPLY_ACK
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index e5285df4ba..d0b889282a 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/vhost-user.h"
 #include "hw/virtio/vhost-backend.h"
@@ -20,6 +21,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"
@@ -138,6 +140,7 @@ typedef enum VhostUserSlaveRequest {
     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 = 6,
     VHOST_USER_BACKEND_MAX
 }  VhostUserSlaveRequest;
 
@@ -200,6 +203,18 @@ typedef struct VhostUserInflight {
     uint16_t queue_size;
 } VhostUserInflight;
 
+typedef enum VhostUserSharedType {
+    VHOST_SHARED_OBJECT_ADD = 0,
+    VHOST_SHARED_OBJECT_LOOKUP,
+    VHOST_SHARED_OBJECT_REMOVE,
+} VhostUserSharedType;
+
+typedef struct VhostUserShared {
+    unsigned char uuid[16];
+    VhostUserSharedType type;
+    int dmabuf_fd;
+} VhostUserShared;
+
 typedef struct {
     VhostUserRequest request;
 
@@ -224,6 +239,7 @@ typedef union {
         VhostUserCryptoSession session;
         VhostUserVringArea area;
         VhostUserInflight inflight;
+        VhostUserShared object;
 } VhostUserPayload;
 
 typedef struct VhostUserMsg {
@@ -1591,6 +1607,52 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
     return 0;
 }
 
+static int vhost_user_backend_handle_shared_object(VhostUserShared *object)
+{
+    QemuUUID uuid;
+    memcpy(uuid.data, object->uuid, sizeof(object->uuid));
+
+    switch (object->type) {
+    case VHOST_SHARED_OBJECT_ADD:
+        return virtio_add_dmabuf(uuid, object->dmabuf_fd);
+    case VHOST_SHARED_OBJECT_LOOKUP:
+        object->dmabuf_fd = virtio_lookup_dmabuf(uuid);
+        if (object->dmabuf_fd < 0) {
+            return object->dmabuf_fd;
+        }
+        break;
+    case VHOST_SHARED_OBJECT_REMOVE:
+        return virtio_remove_resource(uuid);
+    }
+
+    return 0;
+}
+
+static bool
+vhost_user_backend_send_dmabuf_fd(QIOChannel *ioc, VhostUserHeader *hdr,
+                                  VhostUserPayload *payload)
+{
+    Error *local_err = NULL;
+    struct iovec iov[2];
+    if (hdr->flags & VHOST_USER_NEED_REPLY_MASK) {
+        hdr->flags &= ~VHOST_USER_NEED_REPLY_MASK;
+        hdr->flags |= VHOST_USER_REPLY_MASK;
+
+        hdr->size = sizeof(payload->object);
+
+        iov[0].iov_base = hdr;
+        iov[0].iov_len = VHOST_USER_HDR_SIZE;
+        iov[1].iov_base = payload;
+        iov[1].iov_len = hdr->size;
+
+        if (qio_channel_writev_all(ioc, iov, ARRAY_SIZE(iov), &local_err)) {
+            error_report_err(local_err);
+            return false;
+        }
+    }
+    return true;
+}
+
 static void close_slave_channel(struct vhost_user *u)
 {
     g_source_destroy(u->slave_src);
@@ -1648,6 +1710,12 @@ static gboolean slave_read(QIOChannel *ioc, GIOCondition condition,
         ret = vhost_user_slave_handle_vring_host_notifier(dev, &payload.area,
                                                           fd ? fd[0] : -1);
         break;
+    case VHOST_USER_BACKEND_SHARED_OBJECT:
+        ret = vhost_user_backend_handle_shared_object(&payload.object);
+        if (!vhost_user_backend_send_dmabuf_fd(ioc, &hdr, &payload)) {
+            goto err;
+        }
+        break;
     default:
         error_report("Received unexpected msg type: %d.", hdr.request);
         ret = -EINVAL;
diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index 8fb61e2df2..6b4b721225 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -1403,6 +1403,45 @@ bool vu_set_queue_host_notifier(VuDev *dev, VuVirtq *vq, int fd,
     return vu_process_message_reply(dev, &vmsg);
 }
 
+bool
+vu_get_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN], int *dmabuf_fd)
+{
+    int result = false;
+    VhostUserMsg msg_reply;
+    VhostUserMsg msg = {
+        .request = VHOST_USER_BACKEND_SHARED_OBJECT,
+        .size = sizeof(msg.payload.object),
+        .flags = VHOST_USER_VERSION | VHOST_USER_NEED_REPLY_MASK,
+        .payload.object = {
+            .type = VHOST_SHARED_OBJECT_LOOKUP,
+        },
+    };
+
+    memcpy(msg.payload.object.uuid, uuid, sizeof(uuid[0]) * UUID_LEN);
+
+    pthread_mutex_lock(&dev->slave_mutex);
+    if (!vu_message_write(dev, dev->slave_fd, &msg)) {
+        goto out;
+    }
+
+    if (!vu_message_read_default(dev, dev->slave_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;
+    }
+
+    *dmabuf_fd = msg_reply.payload.object.dmabuf_fd;
+    result = *dmabuf_fd > 0;
+out:
+    pthread_mutex_unlock(&dev->slave_mutex);
+
+    return result;
+}
+
 static bool
 vu_set_vring_call_exec(VuDev *dev, VhostUserMsg *vmsg)
 {
diff --git a/subprojects/libvhost-user/libvhost-user.h b/subprojects/libvhost-user/libvhost-user.h
index 49208cceaa..784db65f7c 100644
--- a/subprojects/libvhost-user/libvhost-user.h
+++ b/subprojects/libvhost-user/libvhost-user.h
@@ -119,6 +119,7 @@ typedef enum VhostUserSlaveRequest {
     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 = 6,
     VHOST_USER_BACKEND_MAX
 }  VhostUserSlaveRequest;
 
@@ -172,6 +173,20 @@ typedef struct VhostUserInflight {
     uint16_t queue_size;
 } VhostUserInflight;
 
+typedef enum VhostUserSharedType {
+    VHOST_SHARED_OBJECT_ADD = 0,
+    VHOST_SHARED_OBJECT_LOOKUP,
+    VHOST_SHARED_OBJECT_REMOVE,
+} VhostUserSharedType;
+
+#define UUID_LEN 16
+
+typedef struct VhostUserShared {
+    unsigned char uuid[UUID_LEN];
+    VhostUserSharedType type;
+    int dmabuf_fd;
+} VhostUserShared;
+
 #if defined(_WIN32) && (defined(__x86_64__) || defined(__i386__))
 # define VU_PACKED __attribute__((gcc_struct, packed))
 #else
@@ -199,6 +214,7 @@ typedef struct VhostUserMsg {
         VhostUserConfig config;
         VhostUserVringArea area;
         VhostUserInflight inflight;
+        VhostUserShared object;
     } payload;
 
     int fds[VHOST_MEMORY_BASELINE_NREGIONS];
@@ -539,6 +555,20 @@ 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_get_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_get_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN],
+                          int *dmabuf_fd);
+
 /**
  * vu_queue_set_notification:
  * @dev: a VuDev context
-- 
2.40.0

[PATCH 3/4] vhost-user: refactor send_resp code

[Albert Esteve]

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>
---
 hw/virtio/vhost-user.c | 52 +++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 28 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index d0b889282a..3e5affa67e 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1628,28 +1628,36 @@ static int vhost_user_backend_handle_shared_object(VhostUserShared *object)
     return 0;
 }
 
-static bool
-vhost_user_backend_send_dmabuf_fd(QIOChannel *ioc, VhostUserHeader *hdr,
-                                  VhostUserPayload *payload)
+static bool vhost_user_send_resp(QIOChannel *ioc, VhostUserHeader *hdr,
+                                 VhostUserPayload *payload)
 {
     Error *local_err = NULL;
     struct iovec iov[2];
-    if (hdr->flags & VHOST_USER_NEED_REPLY_MASK) {
-        hdr->flags &= ~VHOST_USER_NEED_REPLY_MASK;
-        hdr->flags |= VHOST_USER_REPLY_MASK;
+    hdr->flags &= ~VHOST_USER_NEED_REPLY_MASK;
+    hdr->flags |= VHOST_USER_REPLY_MASK;
 
-        hdr->size = sizeof(payload->object);
+    iov[0].iov_base = hdr;
+    iov[0].iov_len = VHOST_USER_HDR_SIZE;
+    iov[1].iov_base = payload;
+    iov[1].iov_len = hdr->size;
+
+    if (qio_channel_writev_all(ioc, iov, ARRAY_SIZE(iov), &local_err)) {
+        error_report_err(local_err);
+        return false;
+    }
 
-        iov[0].iov_base = hdr;
-        iov[0].iov_len = VHOST_USER_HDR_SIZE;
-        iov[1].iov_base = payload;
-        iov[1].iov_len = hdr->size;
+    return true;
+}
 
-        if (qio_channel_writev_all(ioc, iov, ARRAY_SIZE(iov), &local_err)) {
-            error_report_err(local_err);
-            return false;
-        }
+static bool
+vhost_user_backend_send_dmabuf_fd(QIOChannel *ioc, VhostUserHeader *hdr,
+                                  VhostUserPayload *payload)
+{
+    if (hdr->flags & VHOST_USER_NEED_REPLY_MASK) {
+        hdr->size = sizeof(payload->object);
+        return vhost_user_send_resp(ioc, hdr, payload);
     }
+
     return true;
 }
 
@@ -1726,22 +1734,10 @@ static gboolean slave_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;
         }
     }
-- 
2.40.0

[PATCH 4/4] libvhost-user: add write_msg cb to dev struct

[Albert Esteve]

Add vu_write_msg_cb type as a member of the VuDev
struct.

In order to interact with the virtio-dmabuf
API, vhost-user backends have available a special
message type that can be sent to the frontend
in Qemu, in order to add, lookup, or remove
entries.

To send these messages and avoid code replication,
backends will need the write_msg method to be exposed
to them, similarly to how the read_msg is for
receiving messages.

Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
 subprojects/libvhost-user/libvhost-user.c |  1 +
 subprojects/libvhost-user/libvhost-user.h | 16 ++++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index 6b4b721225..c50b353915 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -2115,6 +2115,7 @@ vu_init(VuDev *dev,
     dev->sock = socket;
     dev->panic = panic;
     dev->read_msg = read_msg ? read_msg : vu_message_read_default;
+    dev->write_msg = vu_message_write;
     dev->set_watch = set_watch;
     dev->remove_watch = remove_watch;
     dev->iface = iface;
diff --git a/subprojects/libvhost-user/libvhost-user.h b/subprojects/libvhost-user/libvhost-user.h
index 784db65f7c..f5d7162886 100644
--- a/subprojects/libvhost-user/libvhost-user.h
+++ b/subprojects/libvhost-user/libvhost-user.h
@@ -242,6 +242,7 @@ typedef void (*vu_set_features_cb) (VuDev *dev, uint64_t features);
 typedef int (*vu_process_msg_cb) (VuDev *dev, VhostUserMsg *vmsg,
                                   int *do_reply);
 typedef bool (*vu_read_msg_cb) (VuDev *dev, int sock, VhostUserMsg *vmsg);
+typedef bool (*vu_write_msg_cb) (VuDev *dev, int sock, VhostUserMsg *vmsg);
 typedef void (*vu_queue_set_started_cb) (VuDev *dev, int qidx, bool started);
 typedef bool (*vu_queue_is_processed_in_order_cb) (VuDev *dev, int qidx);
 typedef int (*vu_get_config_cb) (VuDev *dev, uint8_t *config, uint32_t len);
@@ -429,6 +430,21 @@ struct VuDev {
      */
     vu_read_msg_cb read_msg;
 
+    /*
+     * @write_msg: custom method to write vhost-user message
+     *
+     * Write data to vhost_user socket fd from the passed
+     * VhostUserMsg *vmsg struct.
+     *
+     * For the details, please refer to vu_message_write in libvhost-user.c
+     * which will be used by default when calling vu_unit.
+     * No custom method is allowed.
+     *
+     * Returns: true if vhost-user message successfully sent, false otherwise.
+     *
+     */
+    vu_write_msg_cb write_msg;
+
     /*
      * @set_watch: add or update the given fd to the watch set,
      * call cb when condition is met.
-- 
2.40.0

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

[Cornelia Huck]

On Wed, May 03 2023, 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

General comment: new files should be covered in MAINTAINERS; not sure if
there is any generic section that could match it, or if this should go
into a new section.

> index 0000000000..3db939a2e3
> --- /dev/null
> +++ b/hw/display/virtio-dmabuf.c

Is virtio-dmabuf only useful for stuff under display/, or could it go
into a more generic section?

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

[Albert Esteve]

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

On Mon, May 8, 2023 at 3:12 PM Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, May 03 2023, 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
>
> General comment: new files should be covered in MAINTAINERS; not sure if
> there is any generic section that could match it, or if this should go
> into a new section.
>

You are right. I thought the entire folder would have an owner already, but
I see
it is split by features. I guess this would make sense under a new section
then.
Thanks!


>
> > index 0000000000..3db939a2e3
> > --- /dev/null
> > +++ b/hw/display/virtio-dmabuf.c
>
> Is virtio-dmabuf only useful for stuff under display/, or could it go
> into a more generic section?
>
>
I hesitated myself and I wouldn't be against changing the location.
In this first version of the infrastructure, it is introduced with dma-buf
sharing in mind, and virtio-gpu -> virtio-video as the main usecase.
Both these devices are/will be at display/, hence I ended up adding
the infrastructure in the same folder, close from where it is going to be
used.

However, in the future other devices may want to use the shared table
for other object types, the virtio specs seem to leave that door open.
In that case, it may be more interesting in another folder.
I had ui/ or hw/virtio/ as candidates myself. Depends on whether
we want to plan ahead for future uses or keep it closer to where it
is being to be used now.

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

Re: [PATCH 4/4] libvhost-user: add write_msg cb to dev struct

[Marc-André Lureau]

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

Hi

On Wed, May 3, 2023 at 12:21 PM Albert Esteve <aesteve@redhat.com> wrote:

> Add vu_write_msg_cb type as a member of the VuDev
> struct.
>
> In order to interact with the virtio-dmabuf
> API, vhost-user backends have available a special
> message type that can be sent to the frontend
> in Qemu, in order to add, lookup, or remove
> entries.
>
> To send these messages and avoid code replication,
> backends will need the write_msg method to be exposed
> to them, similarly to how the read_msg is for
> receiving messages.
>

I think read_msg was introduced to blend libvhost-user IO to qemu mainloop
& coroutine. Is that what you have in mind for write_msg?


> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> ---
>  subprojects/libvhost-user/libvhost-user.c |  1 +
>  subprojects/libvhost-user/libvhost-user.h | 16 ++++++++++++++++
>  2 files changed, 17 insertions(+)
>
> diff --git a/subprojects/libvhost-user/libvhost-user.c
> b/subprojects/libvhost-user/libvhost-user.c
> index 6b4b721225..c50b353915 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -2115,6 +2115,7 @@ vu_init(VuDev *dev,
>      dev->sock = socket;
>      dev->panic = panic;
>      dev->read_msg = read_msg ? read_msg : vu_message_read_default;
> +    dev->write_msg = vu_message_write;
>

You are not making it customizable? And the callback is not used.


>      dev->set_watch = set_watch;
>      dev->remove_watch = remove_watch;
>      dev->iface = iface;
> diff --git a/subprojects/libvhost-user/libvhost-user.h
> b/subprojects/libvhost-user/libvhost-user.h
> index 784db65f7c..f5d7162886 100644
> --- a/subprojects/libvhost-user/libvhost-user.h
> +++ b/subprojects/libvhost-user/libvhost-user.h
> @@ -242,6 +242,7 @@ typedef void (*vu_set_features_cb) (VuDev *dev,
> uint64_t features);
>  typedef int (*vu_process_msg_cb) (VuDev *dev, VhostUserMsg *vmsg,
>                                    int *do_reply);
>  typedef bool (*vu_read_msg_cb) (VuDev *dev, int sock, VhostUserMsg *vmsg);
> +typedef bool (*vu_write_msg_cb) (VuDev *dev, int sock, VhostUserMsg
> *vmsg);
>  typedef void (*vu_queue_set_started_cb) (VuDev *dev, int qidx, bool
> started);
>  typedef bool (*vu_queue_is_processed_in_order_cb) (VuDev *dev, int qidx);
>  typedef int (*vu_get_config_cb) (VuDev *dev, uint8_t *config, uint32_t
> len);
> @@ -429,6 +430,21 @@ struct VuDev {
>       */
>      vu_read_msg_cb read_msg;
>
> +    /*
> +     * @write_msg: custom method to write vhost-user message
> +     *
> +     * Write data to vhost_user socket fd from the passed
> +     * VhostUserMsg *vmsg struct.
> +     *
> +     * For the details, please refer to vu_message_write in
> libvhost-user.c
> +     * which will be used by default when calling vu_unit.
> +     * No custom method is allowed.
>

"No custom method is allowed"?


> +     *
> +     * Returns: true if vhost-user message successfully sent, false
> otherwise.
> +     *
> +     */
> +    vu_write_msg_cb write_msg;
> +
>


-- 
Marc-André Lureau

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

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

[Marc-André Lureau]

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

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.


> +
> +static GMutex lock;
> +static GHashTable *resource_uuids;
> +
>

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


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

+    } else if (g_hash_table_lookup(resource_uuids, struuid) != NULL) {
>

You could implement a hash_func and key_equal_func for QemuUUID instead.


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


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

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

Re: [PATCH 4/4] libvhost-user: add write_msg cb to dev struct

[Albert Esteve]

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

Hi!

On Tue, May 9, 2023 at 12:12 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:
>
>> Add vu_write_msg_cb type as a member of the VuDev
>> struct.
>>
>> In order to interact with the virtio-dmabuf
>> API, vhost-user backends have available a special
>> message type that can be sent to the frontend
>> in Qemu, in order to add, lookup, or remove
>> entries.
>>
>> To send these messages and avoid code replication,
>> backends will need the write_msg method to be exposed
>> to them, similarly to how the read_msg is for
>> receiving messages.
>>
>
> I think read_msg was introduced to blend libvhost-user IO to qemu mainloop
> & coroutine. Is that what you have in mind for write_msg?
>

Uhm, after grep'ing, it seems that read_msg is only used within
libvhost-user source, so I guess it is mainly used to
allow backends to provide custom methods? Maybe I am misunderstanding.

But my idea for adding `write_msg` is exposing the write method (i.e.,
vu_message_write) to the backends,
without having the function signature in the header. This way, vhost-user
backends that want to write a message,
can just use `dev->write_msg(args...)`. Which would be equivalent to
`vu_message_write(args...)` if this
was visible for others.

Another option could be to have a specific public method sending the
requests to the frontend, that
internally, would use `vu_message_write`. But since we introduce three new
message types that
backends can send, I thought adding different methods would be a bit too
verbose.


>
>> Signed-off-by: Albert Esteve <aesteve@redhat.com>
>> ---
>>  subprojects/libvhost-user/libvhost-user.c |  1 +
>>  subprojects/libvhost-user/libvhost-user.h | 16 ++++++++++++++++
>>  2 files changed, 17 insertions(+)
>>
>> diff --git a/subprojects/libvhost-user/libvhost-user.c
>> b/subprojects/libvhost-user/libvhost-user.c
>> index 6b4b721225..c50b353915 100644
>> --- a/subprojects/libvhost-user/libvhost-user.c
>> +++ b/subprojects/libvhost-user/libvhost-user.c
>> @@ -2115,6 +2115,7 @@ vu_init(VuDev *dev,
>>      dev->sock = socket;
>>      dev->panic = panic;
>>      dev->read_msg = read_msg ? read_msg : vu_message_read_default;
>> +    dev->write_msg = vu_message_write;
>>
>
> You are not making it customizable? And the callback is not used.
>

Making it customizable would require changing the signature of `vu_init`,
and I did not see
the need for this usecase. I just want to expose the static method to the
backends.

The callback is not used because there is still no virtio device to use it.
But this whole
infrastructure will be nice to have for the next device that would require
it (e.g., virtio-video).

In that regard, this commit could be skipped from the PATCH and just change
it once there
is a virtio device that needs to send a `VHOST_USER_BACKEND_SHARED_OBJECT`
message. Basically, I needed it for testing (just had a dummy vhost-user
backend that I used for
sending messages), and then decided to keep it. But maybe having a simpler
patch is better.


>
>
>>      dev->set_watch = set_watch;
>>      dev->remove_watch = remove_watch;
>>      dev->iface = iface;
>> diff --git a/subprojects/libvhost-user/libvhost-user.h
>> b/subprojects/libvhost-user/libvhost-user.h
>> index 784db65f7c..f5d7162886 100644
>> --- a/subprojects/libvhost-user/libvhost-user.h
>> +++ b/subprojects/libvhost-user/libvhost-user.h
>> @@ -242,6 +242,7 @@ typedef void (*vu_set_features_cb) (VuDev *dev,
>> uint64_t features);
>>  typedef int (*vu_process_msg_cb) (VuDev *dev, VhostUserMsg *vmsg,
>>                                    int *do_reply);
>>  typedef bool (*vu_read_msg_cb) (VuDev *dev, int sock, VhostUserMsg
>> *vmsg);
>> +typedef bool (*vu_write_msg_cb) (VuDev *dev, int sock, VhostUserMsg
>> *vmsg);
>>  typedef void (*vu_queue_set_started_cb) (VuDev *dev, int qidx, bool
>> started);
>>  typedef bool (*vu_queue_is_processed_in_order_cb) (VuDev *dev, int qidx);
>>  typedef int (*vu_get_config_cb) (VuDev *dev, uint8_t *config, uint32_t
>> len);
>> @@ -429,6 +430,21 @@ struct VuDev {
>>       */
>>      vu_read_msg_cb read_msg;
>>
>> +    /*
>> +     * @write_msg: custom method to write vhost-user message
>> +     *
>> +     * Write data to vhost_user socket fd from the passed
>> +     * VhostUserMsg *vmsg struct.
>> +     *
>> +     * For the details, please refer to vu_message_write in
>> libvhost-user.c
>> +     * which will be used by default when calling vu_unit.
>> +     * No custom method is allowed.
>>
>
> "No custom method is allowed"?
>

I meant that I am not making it customizable (from your previous point),
as opposed to the `read_msg` method.


>
>
>> +     *
>> +     * Returns: true if vhost-user message successfully sent, false
>> otherwise.
>> +     *
>> +     */
>> +    vu_write_msg_cb write_msg;
>> +
>>
>
>
> --
> Marc-André Lureau
>

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

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

[Albert Esteve]

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

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.


> +
>> +static GMutex lock;
>> +static GHashTable *resource_uuids;
>> +
>>
>
> Rather than having global variables, shouldn't we associate virtio
> resources with the virtio bus instead?
>
>

I am sorry but I am not sure how you mean. Wouldn't that mean to create a
virtio-pci device with
the virtio-dmabuf object? AFAIK the virtio-bus conforms the transport layer
for connecting to the guest.
The goal with virtio-dmabuf is "connecting" different virtio
backends, which are already connected to the
virtio-bus (and the guest). Strictly speaking not even that, just needs to
be accessible from different devices
to add and retrieve descriptors (dealing with concurrent accesses).

But maybe I am not understanding your point!



> +
>> +static bool virtio_add_resource(QemuUUID uuid, gpointer value)
>> +{
>> +    gpointer struuid = UUID_TO_POINTER(uuid);
>> +    if (resource_uuids == NULL) {
>> +        resource_uuids = g_hash_table_new_full(NULL, NULL, NULL, g_free);
>>
>
> You create "resource_uuids" implicitly in 2 places, in 2 different ways.
> Let's not do that, and have an explicit initialization step instead (it
> might be with the virtio bus construction, if we move the code there)
>
>
Ok!


> +    } else if (g_hash_table_lookup(resource_uuids, struuid) != NULL) {
>>
>
> You could implement a hash_func and key_equal_func for QemuUUID instead.
>
>

Sounds like a good idea. I will add an initial, separate commit for that.


> +        return false;
>> +    }
>> +
>> +    return g_hash_table_insert(resource_uuids, struuid, value);
>> +}
>> +
>> +static gpointer virtio_lookup_resource(QemuUUID uuid)
>> +{
>> +    if (resource_uuids == NULL) {
>> +        return NULL;
>> +    }
>> +
>> +    return g_hash_table_lookup(resource_uuids, UUID_TO_POINTER(uuid));
>> +}
>> +
>> +bool virtio_add_dmabuf(QemuUUID uuid, int udmabuf_fd)
>> +{
>> +    bool result;
>> +    if (udmabuf_fd < 0) {
>> +        return false;
>> +    }
>> +    g_mutex_lock(&lock);
>> +    if (resource_uuids == NULL) {
>> +        resource_uuids = g_hash_table_new(NULL, NULL);
>> +    }
>> +    result = virtio_add_resource(uuid, GINT_TO_POINTER(udmabuf_fd));
>> +    g_mutex_unlock(&lock);
>> +
>> +    return result;
>> +}
>> +
>> +bool virtio_remove_resource(QemuUUID uuid)
>> +{
>> +    bool result;
>> +    g_mutex_lock(&lock);
>> +    result = g_hash_table_remove(resource_uuids, UUID_TO_POINTER(uuid));
>> +    g_mutex_unlock(&lock);
>> +
>> +    return result;
>> +}
>> +
>> +int virtio_lookup_dmabuf(QemuUUID uuid)
>> +{
>> +    g_mutex_lock(&lock);
>> +    gpointer lookup_res = virtio_lookup_resource(uuid);
>> +    g_mutex_unlock(&lock);
>> +    if (lookup_res == NULL) {
>> +        return -1;
>> +    }
>> +
>> +    return GPOINTER_TO_INT(lookup_res);
>> +}
>> +
>> +void virtio_free_resources(void)
>> +{
>> +    g_hash_table_destroy(resource_uuids);
>> +    /* Reference count shall be 0 after the implicit unref on destroy */
>> +    resource_uuids = NULL;
>> +}
>> diff --git a/include/hw/virtio/virtio-dmabuf.h
>> b/include/hw/virtio/virtio-dmabuf.h
>> new file mode 100644
>> index 0000000000..1c1c713128
>> --- /dev/null
>> +++ b/include/hw/virtio/virtio-dmabuf.h
>> @@ -0,0 +1,58 @@
>> +/*
>> + * Virtio Shared dma-buf
>> + *
>> + * Copyright Red Hat, Inc. 2023
>> + *
>> + * Authors:
>> + *     Albert Esteve <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
>

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

Re: [PATCH 4/4] libvhost-user: add write_msg cb to dev struct

[Marc-André Lureau]

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

Hi

On Tue, May 9, 2023 at 3:17 PM Albert Esteve <aesteve@redhat.com> wrote:

>
> Hi!
>
> On Tue, May 9, 2023 at 12:12 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:
>>
>>> Add vu_write_msg_cb type as a member of the VuDev
>>> struct.
>>>
>>> In order to interact with the virtio-dmabuf
>>> API, vhost-user backends have available a special
>>> message type that can be sent to the frontend
>>> in Qemu, in order to add, lookup, or remove
>>> entries.
>>>
>>> To send these messages and avoid code replication,
>>> backends will need the write_msg method to be exposed
>>> to them, similarly to how the read_msg is for
>>> receiving messages.
>>>
>>
>> I think read_msg was introduced to blend libvhost-user IO to qemu
>> mainloop & coroutine. Is that what you have in mind for write_msg?
>>
>
> Uhm, after grep'ing, it seems that read_msg is only used within
> libvhost-user source, so I guess it is mainly used to
> allow backends to provide custom methods? Maybe I am misunderstanding.
>
> But my idea for adding `write_msg` is exposing the write method (i.e.,
> vu_message_write) to the backends,
> without having the function signature in the header. This way, vhost-user
> backends that want to write a message,
> can just use `dev->write_msg(args...)`. Which would be equivalent to
> `vu_message_write(args...)` if this
> was visible for others.
>


Imho it's better to introduce a normal function in that case, that is
simply export vu_message_write_default().


> Another option could be to have a specific public method sending the
> requests to the frontend, that
> internally, would use `vu_message_write`. But since we introduce three new
> message types that
> backends can send, I thought adding different methods would be a bit too
> verbose.
>

Actually, exposing higher-level methods to send specific messages is more
correct imho.


>>
>>> Signed-off-by: Albert Esteve <aesteve@redhat.com>
>>> ---
>>>  subprojects/libvhost-user/libvhost-user.c |  1 +
>>>  subprojects/libvhost-user/libvhost-user.h | 16 ++++++++++++++++
>>>  2 files changed, 17 insertions(+)
>>>
>>> diff --git a/subprojects/libvhost-user/libvhost-user.c
>>> b/subprojects/libvhost-user/libvhost-user.c
>>> index 6b4b721225..c50b353915 100644
>>> --- a/subprojects/libvhost-user/libvhost-user.c
>>> +++ b/subprojects/libvhost-user/libvhost-user.c
>>> @@ -2115,6 +2115,7 @@ vu_init(VuDev *dev,
>>>      dev->sock = socket;
>>>      dev->panic = panic;
>>>      dev->read_msg = read_msg ? read_msg : vu_message_read_default;
>>> +    dev->write_msg = vu_message_write;
>>>
>>
>> You are not making it customizable? And the callback is not used.
>>
>
> Making it customizable would require changing the signature of `vu_init`,
> and I did not see
> the need for this usecase. I just want to expose the static method to the
> backends.
>
>
ok


> The callback is not used because there is still no virtio device to use
> it. But this whole
> infrastructure will be nice to have for the next device that would require
> it (e.g., virtio-video).
>
> In that regard, this commit could be skipped from the PATCH and just
> change it once there
> is a virtio device that needs to send a `VHOST_USER_BACKEND_SHARED_OBJECT`
> message. Basically, I needed it for testing (just had a dummy vhost-user
> backend that I used for
> sending messages), and then decided to keep it. But maybe having a simpler
> patch is better.
>
>
>>
>>
>>>      dev->set_watch = set_watch;
>>>      dev->remove_watch = remove_watch;
>>>      dev->iface = iface;
>>> diff --git a/subprojects/libvhost-user/libvhost-user.h
>>> b/subprojects/libvhost-user/libvhost-user.h
>>> index 784db65f7c..f5d7162886 100644
>>> --- a/subprojects/libvhost-user/libvhost-user.h
>>> +++ b/subprojects/libvhost-user/libvhost-user.h
>>> @@ -242,6 +242,7 @@ typedef void (*vu_set_features_cb) (VuDev *dev,
>>> uint64_t features);
>>>  typedef int (*vu_process_msg_cb) (VuDev *dev, VhostUserMsg *vmsg,
>>>                                    int *do_reply);
>>>  typedef bool (*vu_read_msg_cb) (VuDev *dev, int sock, VhostUserMsg
>>> *vmsg);
>>> +typedef bool (*vu_write_msg_cb) (VuDev *dev, int sock, VhostUserMsg
>>> *vmsg);
>>>  typedef void (*vu_queue_set_started_cb) (VuDev *dev, int qidx, bool
>>> started);
>>>  typedef bool (*vu_queue_is_processed_in_order_cb) (VuDev *dev, int
>>> qidx);
>>>  typedef int (*vu_get_config_cb) (VuDev *dev, uint8_t *config, uint32_t
>>> len);
>>> @@ -429,6 +430,21 @@ struct VuDev {
>>>       */
>>>      vu_read_msg_cb read_msg;
>>>
>>> +    /*
>>> +     * @write_msg: custom method to write vhost-user message
>>> +     *
>>> +     * Write data to vhost_user socket fd from the passed
>>> +     * VhostUserMsg *vmsg struct.
>>> +     *
>>> +     * For the details, please refer to vu_message_write in
>>> libvhost-user.c
>>> +     * which will be used by default when calling vu_unit.
>>> +     * No custom method is allowed.
>>>
>>
>> "No custom method is allowed"?
>>
>
> I meant that I am not making it customizable (from your previous point),
> as opposed to the `read_msg` method.
>
>
>>
>>
>>> +     *
>>> +     * Returns: true if vhost-user message successfully sent, false
>>> otherwise.
>>> +     *
>>> +     */
>>> +    vu_write_msg_cb write_msg;
>>> +
>>>
>>
>>
>> --
>> Marc-André Lureau
>>
>

-- 
Marc-André Lureau

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

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

[Marc-André Lureau]

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

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.


>
>
>
>> +
>>> +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: 18829 bytes --]

Re: [PATCH 4/4] libvhost-user: add write_msg cb to dev struct

[Albert Esteve]

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

On Tue, May 9, 2023 at 2:53 PM Marc-André Lureau <marcandre.lureau@gmail.com>
wrote:

> Hi
>
> On Tue, May 9, 2023 at 3:17 PM Albert Esteve <aesteve@redhat.com> wrote:
>
>>
>> Hi!
>>
>> On Tue, May 9, 2023 at 12:12 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:
>>>
>>>> Add vu_write_msg_cb type as a member of the VuDev
>>>> struct.
>>>>
>>>> In order to interact with the virtio-dmabuf
>>>> API, vhost-user backends have available a special
>>>> message type that can be sent to the frontend
>>>> in Qemu, in order to add, lookup, or remove
>>>> entries.
>>>>
>>>> To send these messages and avoid code replication,
>>>> backends will need the write_msg method to be exposed
>>>> to them, similarly to how the read_msg is for
>>>> receiving messages.
>>>>
>>>
>>> I think read_msg was introduced to blend libvhost-user IO to qemu
>>> mainloop & coroutine. Is that what you have in mind for write_msg?
>>>
>>
>> Uhm, after grep'ing, it seems that read_msg is only used within
>> libvhost-user source, so I guess it is mainly used to
>> allow backends to provide custom methods? Maybe I am misunderstanding.
>>
>> But my idea for adding `write_msg` is exposing the write method (i.e.,
>> vu_message_write) to the backends,
>> without having the function signature in the header. This way, vhost-user
>> backends that want to write a message,
>> can just use `dev->write_msg(args...)`. Which would be equivalent to
>> `vu_message_write(args...)` if this
>> was visible for others.
>>
>
>
> Imho it's better to introduce a normal function in that case, that is
> simply export vu_message_write_default().
>
>
>> Another option could be to have a specific public method sending the
>> requests to the frontend, that
>> internally, would use `vu_message_write`. But since we introduce three
>> new message types that
>> backends can send, I thought adding different methods would be a bit too
>> verbose.
>>
>
> Actually, exposing higher-level methods to send specific messages is more
> correct imho.
>

Then I will do that, thanks!


>
>
>>>
>>>> Signed-off-by: Albert Esteve <aesteve@redhat.com>
>>>> ---
>>>>  subprojects/libvhost-user/libvhost-user.c |  1 +
>>>>  subprojects/libvhost-user/libvhost-user.h | 16 ++++++++++++++++
>>>>  2 files changed, 17 insertions(+)
>>>>
>>>> diff --git a/subprojects/libvhost-user/libvhost-user.c
>>>> b/subprojects/libvhost-user/libvhost-user.c
>>>> index 6b4b721225..c50b353915 100644
>>>> --- a/subprojects/libvhost-user/libvhost-user.c
>>>> +++ b/subprojects/libvhost-user/libvhost-user.c
>>>> @@ -2115,6 +2115,7 @@ vu_init(VuDev *dev,
>>>>      dev->sock = socket;
>>>>      dev->panic = panic;
>>>>      dev->read_msg = read_msg ? read_msg : vu_message_read_default;
>>>> +    dev->write_msg = vu_message_write;
>>>>
>>>
>>> You are not making it customizable? And the callback is not used.
>>>
>>
>> Making it customizable would require changing the signature of `vu_init`,
>> and I did not see
>> the need for this usecase. I just want to expose the static method to the
>> backends.
>>
>>
> ok
>
>
>> The callback is not used because there is still no virtio device to use
>> it. But this whole
>> infrastructure will be nice to have for the next device that would
>> require it (e.g., virtio-video).
>>
>> In that regard, this commit could be skipped from the PATCH and just
>> change it once there
>> is a virtio device that needs to send a `VHOST_USER_BACKEND_SHARED_OBJECT`
>> message. Basically, I needed it for testing (just had a dummy vhost-user
>> backend that I used for
>> sending messages), and then decided to keep it. But maybe having a
>> simpler patch is better.
>>
>>
>>>
>>>
>>>>      dev->set_watch = set_watch;
>>>>      dev->remove_watch = remove_watch;
>>>>      dev->iface = iface;
>>>> diff --git a/subprojects/libvhost-user/libvhost-user.h
>>>> b/subprojects/libvhost-user/libvhost-user.h
>>>> index 784db65f7c..f5d7162886 100644
>>>> --- a/subprojects/libvhost-user/libvhost-user.h
>>>> +++ b/subprojects/libvhost-user/libvhost-user.h
>>>> @@ -242,6 +242,7 @@ typedef void (*vu_set_features_cb) (VuDev *dev,
>>>> uint64_t features);
>>>>  typedef int (*vu_process_msg_cb) (VuDev *dev, VhostUserMsg *vmsg,
>>>>                                    int *do_reply);
>>>>  typedef bool (*vu_read_msg_cb) (VuDev *dev, int sock, VhostUserMsg
>>>> *vmsg);
>>>> +typedef bool (*vu_write_msg_cb) (VuDev *dev, int sock, VhostUserMsg
>>>> *vmsg);
>>>>  typedef void (*vu_queue_set_started_cb) (VuDev *dev, int qidx, bool
>>>> started);
>>>>  typedef bool (*vu_queue_is_processed_in_order_cb) (VuDev *dev, int
>>>> qidx);
>>>>  typedef int (*vu_get_config_cb) (VuDev *dev, uint8_t *config, uint32_t
>>>> len);
>>>> @@ -429,6 +430,21 @@ struct VuDev {
>>>>       */
>>>>      vu_read_msg_cb read_msg;
>>>>
>>>> +    /*
>>>> +     * @write_msg: custom method to write vhost-user message
>>>> +     *
>>>> +     * Write data to vhost_user socket fd from the passed
>>>> +     * VhostUserMsg *vmsg struct.
>>>> +     *
>>>> +     * For the details, please refer to vu_message_write in
>>>> libvhost-user.c
>>>> +     * which will be used by default when calling vu_unit.
>>>> +     * No custom method is allowed.
>>>>
>>>
>>> "No custom method is allowed"?
>>>
>>
>> I meant that I am not making it customizable (from your previous point),
>> as opposed to the `read_msg` method.
>>
>>
>>>
>>>
>>>> +     *
>>>> +     * Returns: true if vhost-user message successfully sent, false
>>>> otherwise.
>>>> +     *
>>>> +     */
>>>> +    vu_write_msg_cb write_msg;
>>>> +
>>>>
>>>
>>>
>>> --
>>> Marc-André Lureau
>>>
>>
>
> --
> Marc-André Lureau
>

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

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

[Albert Esteve]

[-- 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 --]