[PATCH v4 0/4] Virtio shared dma-buf

[PATCH v4 0/4] Virtio shared dma-buf

[Albert Esteve]

v1 link -> https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg00598.html
v2 link -> https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg04530.html
v3 link -> https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg06126.html
v3 -> v4:
    - Split the different message types for shared objects into different messages
    - Add a vhost-user feature bit to negotiate the shared object feature
    - Minor improvements following suggestions

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

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

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

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

Applies cleanly to b455ce4c2f300c8ba47cba7232dd03261368a4cb

[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):
  uuid: add a hash function
  virtio-dmabuf: introduce virtio-dmabuf
  vhost-user: add shared_object msg
  vhost-user: refactor send_resp code

 MAINTAINERS                               |   7 ++
 docs/interop/vhost-user.rst               |  42 ++++++++
 hw/display/meson.build                    |   1 +
 hw/display/virtio-dmabuf.c                |  90 +++++++++++++++++
 hw/virtio/vhost-user.c                    | 115 +++++++++++++++++++---
 include/hw/virtio/virtio-dmabuf.h         |  59 +++++++++++
 include/qemu/uuid.h                       |   2 +
 subprojects/libvhost-user/libvhost-user.c | 101 +++++++++++++++++++
 subprojects/libvhost-user/libvhost-user.h |  53 +++++++++-
 tests/unit/meson.build                    |   1 +
 tests/unit/test-uuid.c                    |  27 +++++
 tests/unit/test-virtio-dmabuf.c           | 112 +++++++++++++++++++++
 util/uuid.c                               |  14 +++
 13 files changed, 610 insertions(+), 14 deletions(-)
 create mode 100644 hw/display/virtio-dmabuf.c
 create mode 100644 include/hw/virtio/virtio-dmabuf.h
 create mode 100644 tests/unit/test-virtio-dmabuf.c

-- 
2.40.0

[PATCH v4 1/4] uuid: add a hash function

[Albert Esteve]

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

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

Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
 include/qemu/uuid.h    |  2 ++
 tests/unit/test-uuid.c | 27 +++++++++++++++++++++++++++
 util/uuid.c            | 14 ++++++++++++++
 3 files changed, 43 insertions(+)

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

[PATCH v4 2/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.

Suggested-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
 MAINTAINERS                       |   7 ++
 hw/display/meson.build            |   1 +
 hw/display/virtio-dmabuf.c        |  90 ++++++++++++++++++++++++
 include/hw/virtio/virtio-dmabuf.h |  59 ++++++++++++++++
 tests/unit/meson.build            |   1 +
 tests/unit/test-virtio-dmabuf.c   | 112 ++++++++++++++++++++++++++++++
 6 files changed, 270 insertions(+)
 create mode 100644 hw/display/virtio-dmabuf.c
 create mode 100644 include/hw/virtio/virtio-dmabuf.h
 create mode 100644 tests/unit/test-virtio-dmabuf.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7f323cd2eb..ce77a691a1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2150,6 +2150,13 @@ T: git https://gitlab.com/cohuck/qemu.git s390-next
 T: git https://github.com/borntraeger/qemu.git s390-next
 L: qemu-s390x@nongnu.org
 
+virtio-dmabuf
+M: Albert Esteve <aesteve@redhat.com>
+S: Supported
+F: hw/display/virtio-dmabuf.c
+F: include/hw/virtio/virtio-dmabuf.h
+F: tests/unit/test-virtio-dmabuf.c
+
 virtiofs
 M: Stefan Hajnoczi <stefanha@redhat.com>
 S: Supported
diff --git a/hw/display/meson.build b/hw/display/meson.build
index 413ba4ab24..05619c6968 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -37,6 +37,7 @@ system_ss.add(when: 'CONFIG_MACFB', if_true: files('macfb.c'))
 system_ss.add(when: 'CONFIG_NEXTCUBE', if_true: files('next-fb.c'))
 
 system_ss.add(when: 'CONFIG_VGA', if_true: files('vga.c'))
+system_ss.add(when: 'CONFIG_VIRTIO', if_true: files('virtio-dmabuf.c'))
 
 if (config_all_devices.has_key('CONFIG_VGA_CIRRUS') or
     config_all_devices.has_key('CONFIG_VGA_PCI') or
diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
new file mode 100644
index 0000000000..7dba0b2c71
--- /dev/null
+++ b/hw/display/virtio-dmabuf.c
@@ -0,0 +1,90 @@
+/*
+ * Virtio Shared dma-buf
+ *
+ * Copyright Red Hat, Inc. 2023
+ *
+ * Authors:
+ *     Albert Esteve <aesteve@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "hw/virtio/virtio-dmabuf.h"
+
+
+static GMutex lock;
+static GHashTable *resource_uuids;
+
+/*
+ * uuid_equal_func: wrapper for UUID is_equal function to
+ * satisfy g_hash_table_new expected parameters signatures.
+ */
+static int uuid_equal_func(const void *lhv, const void *rhv)
+{
+    return qemu_uuid_is_equal(lhv, rhv);
+}
+
+static bool virtio_add_resource(QemuUUID *uuid, gpointer value)
+{
+    assert(resource_uuids != NULL);
+    if (g_hash_table_lookup(resource_uuids, uuid) != NULL) {
+        return false;
+    }
+
+    return g_hash_table_insert(resource_uuids, uuid, value);
+}
+
+static gpointer virtio_lookup_resource(const QemuUUID *uuid)
+{
+    if (resource_uuids == NULL) {
+        return NULL;
+    }
+
+    return g_hash_table_lookup(resource_uuids, 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(qemu_uuid_hash, uuid_equal_func);
+    }
+    result = virtio_add_resource(uuid, GINT_TO_POINTER(udmabuf_fd));
+    g_mutex_unlock(&lock);
+
+    return result;
+}
+
+bool virtio_remove_resource(const QemuUUID *uuid)
+{
+    bool result;
+    g_mutex_lock(&lock);
+    result = g_hash_table_remove(resource_uuids, uuid);
+    g_mutex_unlock(&lock);
+
+    return result;
+}
+
+int virtio_lookup_dmabuf(const 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..4fdd394c4b
--- /dev/null
+++ b/include/hw/virtio/virtio-dmabuf.h
@@ -0,0 +1,59 @@
+/*
+ * 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 caller retains ownership over the
+ *             descriptor and its lifecycle.
+ *
+ * Note: @dmabuf_fd must be a valid (non-negative) file descriptor.
+ *
+ * Return: true if the UUID did not exist and the resource has been added,
+ * false if another resource with the same UUID already existed.
+ * Note that if it finds a repeated UUID, the resource is not inserted in
+ * the lookup table.
+ */
+bool virtio_add_dmabuf(QemuUUID *uuid, int dmabuf_fd);
+
+/**
+ * virtio_remove_resource() - Removes a resource from the lookup table
+ * @uuid: resource's UUID
+ *
+ * Return: true if the UUID has been found and removed from the lookup table.
+ */
+bool virtio_remove_resource(const QemuUUID *uuid);
+
+/**
+ * virtio_lookup_dmabuf() - Looks for a dma-buf resource in the lookup table
+ * @uuid: resource's UUID
+ *
+ * Return: the dma-buf file descriptor integer, or -1 if the key is not found.
+ */
+int virtio_lookup_dmabuf(const QemuUUID *uuid);
+
+/**
+ * virtio_free_resources() - Destroys all keys and values of the shared
+ * resources lookup table, and frees them
+ */
+void virtio_free_resources(void);
+
+#endif /* VIRTIO_DMABUF_H */
diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index 93977cc32d..425ecc30fb 100644
--- a/tests/unit/meson.build
+++ b/tests/unit/meson.build
@@ -50,6 +50,7 @@ tests = {
   'test-qapi-util': [],
   'test-interval-tree': [],
   'test-xs-node': [qom],
+  'test-virtio-dmabuf': [meson.project_source_root() / 'hw/display/virtio-dmabuf.c'],
 }
 
 if have_system or have_tools
diff --git a/tests/unit/test-virtio-dmabuf.c b/tests/unit/test-virtio-dmabuf.c
new file mode 100644
index 0000000000..53436aa93d
--- /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

Re: [PATCH v4 2/4] virtio-dmabuf: introduce virtio-dmabuf

[Michael S. Tsirkin]

On Mon, Jun 26, 2023 at 09:34:24AM +0200, Albert Esteve 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.
> Suggested-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> ---
>  MAINTAINERS                       |   7 ++
>  hw/display/meson.build            |   1 +
>  hw/display/virtio-dmabuf.c        |  90 ++++++++++++++++++++++++
>  include/hw/virtio/virtio-dmabuf.h |  59 ++++++++++++++++
>  tests/unit/meson.build            |   1 +
>  tests/unit/test-virtio-dmabuf.c   | 112 ++++++++++++++++++++++++++++++
>  6 files changed, 270 insertions(+)
>  create mode 100644 hw/display/virtio-dmabuf.c
>  create mode 100644 include/hw/virtio/virtio-dmabuf.h
>  create mode 100644 tests/unit/test-virtio-dmabuf.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7f323cd2eb..ce77a691a1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2150,6 +2150,13 @@ T: git https://gitlab.com/cohuck/qemu.git s390-next
>  T: git https://github.com/borntraeger/qemu.git s390-next
>  L: qemu-s390x@nongnu.org
>  
> +virtio-dmabuf
> +M: Albert Esteve <aesteve@redhat.com>
> +S: Supported
> +F: hw/display/virtio-dmabuf.c
> +F: include/hw/virtio/virtio-dmabuf.h
> +F: tests/unit/test-virtio-dmabuf.c
> +
>  virtiofs
>  M: Stefan Hajnoczi <stefanha@redhat.com>
>  S: Supported

Hmm new functionality that no one seems to care
about enough to review,

Gerd you suggested this - ready to be a maintainer?
co-maintain with Albert?


> diff --git a/hw/display/meson.build b/hw/display/meson.build
> index 413ba4ab24..05619c6968 100644
> --- a/hw/display/meson.build
> +++ b/hw/display/meson.build
> @@ -37,6 +37,7 @@ system_ss.add(when: 'CONFIG_MACFB', if_true: files('macfb.c'))
>  system_ss.add(when: 'CONFIG_NEXTCUBE', if_true: files('next-fb.c'))
>  
>  system_ss.add(when: 'CONFIG_VGA', if_true: files('vga.c'))
> +system_ss.add(when: 'CONFIG_VIRTIO', if_true: files('virtio-dmabuf.c'))
>  
>  if (config_all_devices.has_key('CONFIG_VGA_CIRRUS') or
>      config_all_devices.has_key('CONFIG_VGA_PCI') or
> diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
> new file mode 100644
> index 0000000000..7dba0b2c71
> --- /dev/null
> +++ b/hw/display/virtio-dmabuf.c
> @@ -0,0 +1,90 @@
> +/*
> + * Virtio Shared dma-buf
> + *
> + * Copyright Red Hat, Inc. 2023
> + *
> + * Authors:
> + *     Albert Esteve <aesteve@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "hw/virtio/virtio-dmabuf.h"
> +
> +
> +static GMutex lock;
> +static GHashTable *resource_uuids;
> +
> +/*
> + * uuid_equal_func: wrapper for UUID is_equal function to
> + * satisfy g_hash_table_new expected parameters signatures.
> + */
> +static int uuid_equal_func(const void *lhv, const void *rhv)
> +{
> +    return qemu_uuid_is_equal(lhv, rhv);
> +}
> +
> +static bool virtio_add_resource(QemuUUID *uuid, gpointer value)
> +{
> +    assert(resource_uuids != NULL);
> +    if (g_hash_table_lookup(resource_uuids, uuid) != NULL) {
> +        return false;
> +    }
> +
> +    return g_hash_table_insert(resource_uuids, uuid, value);
> +}
> +
> +static gpointer virtio_lookup_resource(const QemuUUID *uuid)
> +{
> +    if (resource_uuids == NULL) {
> +        return NULL;
> +    }
> +
> +    return g_hash_table_lookup(resource_uuids, 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(qemu_uuid_hash, uuid_equal_func);
> +    }
> +    result = virtio_add_resource(uuid, GINT_TO_POINTER(udmabuf_fd));
> +    g_mutex_unlock(&lock);
> +
> +    return result;
> +}
> +
> +bool virtio_remove_resource(const QemuUUID *uuid)
> +{
> +    bool result;
> +    g_mutex_lock(&lock);
> +    result = g_hash_table_remove(resource_uuids, uuid);
> +    g_mutex_unlock(&lock);
> +
> +    return result;
> +}
> +
> +int virtio_lookup_dmabuf(const 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..4fdd394c4b
> --- /dev/null
> +++ b/include/hw/virtio/virtio-dmabuf.h
> @@ -0,0 +1,59 @@
> +/*
> + * 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 caller retains ownership over the
> + *             descriptor and its lifecycle.
> + *
> + * Note: @dmabuf_fd must be a valid (non-negative) file descriptor.
> + *
> + * Return: true if the UUID did not exist and the resource has been added,
> + * false if another resource with the same UUID already existed.
> + * Note that if it finds a repeated UUID, the resource is not inserted in
> + * the lookup table.
> + */
> +bool virtio_add_dmabuf(QemuUUID *uuid, int dmabuf_fd);
> +
> +/**
> + * virtio_remove_resource() - Removes a resource from the lookup table
> + * @uuid: resource's UUID
> + *
> + * Return: true if the UUID has been found and removed from the lookup table.
> + */
> +bool virtio_remove_resource(const QemuUUID *uuid);
> +
> +/**
> + * virtio_lookup_dmabuf() - Looks for a dma-buf resource in the lookup table
> + * @uuid: resource's UUID
> + *
> + * Return: the dma-buf file descriptor integer, or -1 if the key is not found.
> + */
> +int virtio_lookup_dmabuf(const QemuUUID *uuid);
> +
> +/**
> + * virtio_free_resources() - Destroys all keys and values of the shared
> + * resources lookup table, and frees them
> + */
> +void virtio_free_resources(void);
> +
> +#endif /* VIRTIO_DMABUF_H */
> diff --git a/tests/unit/meson.build b/tests/unit/meson.build
> index 93977cc32d..425ecc30fb 100644
> --- a/tests/unit/meson.build
> +++ b/tests/unit/meson.build
> @@ -50,6 +50,7 @@ tests = {
>    'test-qapi-util': [],
>    'test-interval-tree': [],
>    'test-xs-node': [qom],
> +  'test-virtio-dmabuf': [meson.project_source_root() / 'hw/display/virtio-dmabuf.c'],
>  }
>  
>  if have_system or have_tools
> diff --git a/tests/unit/test-virtio-dmabuf.c b/tests/unit/test-virtio-dmabuf.c
> new file mode 100644
> index 0000000000..53436aa93d
> --- /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

Re: [PATCH v4 2/4] virtio-dmabuf: introduce virtio-dmabuf

[Gerd Hoffmann]

  Hi,

> >  M: Stefan Hajnoczi <stefanha@redhat.com>
> >  S: Supported
> 
> Hmm new functionality that no one seems to care
> about enough to review,
> 
> Gerd you suggested this - ready to be a maintainer?
> co-maintain with Albert?

Quite busy with firmware, I don't really have the time to take more qemu
stuff, I'm already struggling to care about the qemu bits I'm still
listed as maintainer for ...

Patch looks fine to me.  This just maintains a list of buffers,
references by uuid.  Use case is buffer sharing, i.e. guest creates a
virtio-gpu buffers, shares it with another driver (virtio-video for
example) inside the guest, the buffer uuid can be used by the device
emulation backends on the host side to establish the same sharing.

take care,
  Gerd

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

[Albert Esteve]

Add three new vhost-user protocol
`VHOST_USER_BACKEND_SHARED_OBJECT_* messages`.
These new messages are sent from vhost-user
back-ends to interact with the virtio-dmabuf
table in order to add, 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 we need to add
helper functions to allow sending messages to
interact with the virtio shared objects
hash table.

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

Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
 docs/interop/vhost-user.rst               |  42 +++++++++
 hw/virtio/vhost-user.c                    |  99 +++++++++++++++++++++
 subprojects/libvhost-user/libvhost-user.c | 101 ++++++++++++++++++++++
 subprojects/libvhost-user/libvhost-user.h |  53 +++++++++++-
 4 files changed, 294 insertions(+), 1 deletion(-)

diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
index 5a070adbc1..bca5600ff1 100644
--- a/docs/interop/vhost-user.rst
+++ b/docs/interop/vhost-user.rst
@@ -1528,6 +1528,48 @@ is sent by the front-end.
 
   The state.num field is currently reserved and must be set to 0.
 
+``VHOST_USER_BACKEND_SHARED_OBJECT_ADD``
+  :id: 6
+  :equivalent ioctl: N/A
+  :request payload: ``struct VhostUserShared``
+  :reply payload: N/A
+
+  When the ``VHOST_USER_PROTOCOL_F_SHARED_OBJECT`` protocol
+  feature has been successfully negotiated, this message can be submitted
+  by the backends to add a new dma-buf fd to the virtio-dmabuf shared
+  table API can send this message. The fd gets associated with a UUID.
+  If ``VHOST_USER_PROTOCOL_F_REPLY_ACK`` is negotiated, and the back-end sets
+  the ``VHOST_USER_NEED_REPLY`` flag, the front-end must respond with zero when
+  operation is successfully completed, or non-zero otherwise.
+
+``VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE``
+  :id: 7
+  :equivalent ioctl: N/A
+  :request payload: ``struct VhostUserShared``
+  :reply payload: N/A
+
+  When the ``VHOST_USER_PROTOCOL_F_SHARED_OBJECT`` protocol
+  feature has been successfully negotiated, this message can be submitted
+  by the backend to remove a dma-buf from to the virtio-dmabuf shared
+  table API can send this message. The shared table will remove the dma-buf
+  fd associated with the UUID. If ``VHOST_USER_PROTOCOL_F_REPLY_ACK`` is
+  negotiated, and the back-end sets the ``VHOST_USER_NEED_REPLY`` flag, the
+  front-end must respond with zero when operation is successfully completed,
+  or non-zero otherwise.
+
+``VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP``
+  :id: 8
+  :equivalent ioctl: N/A
+  :request payload: ``struct VhostUserShared``
+  :reply payload: dmabuf fd and ``u64``
+
+  When the ``VHOST_USER_PROTOCOL_F_SHARED_OBJECT`` protocol
+  feature has been successfully negotiated, this message can be submitted
+  by the backends to retrieve a given dma-buf fd from the virtio-dmabuf
+  shared table given a UUID. Frontend will reply passing the fd and a zero
+  when the operation is successful, or non-zero otherwise. Note that if the
+  operation fails, no fd is sent to the backend.
+
 .. _reply_ack:
 
 VHOST_USER_PROTOCOL_F_REPLY_ACK
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 74a2a28663..e340c39a19 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"
@@ -73,6 +75,7 @@ enum VhostUserProtocolFeature {
     /* Feature 14 reserved for VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS. */
     VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15,
     VHOST_USER_PROTOCOL_F_STATUS = 16,
+    VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 17,
     VHOST_USER_PROTOCOL_F_MAX
 };
 
@@ -128,6 +131,9 @@ 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_ADD = 6,
+    VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE = 7,
+    VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP = 8,
     VHOST_USER_BACKEND_MAX
 }  VhostUserSlaveRequest;
 
@@ -190,6 +196,10 @@ typedef struct VhostUserInflight {
     uint16_t queue_size;
 } VhostUserInflight;
 
+typedef struct VhostUserShared {
+    unsigned char uuid[16];
+} VhostUserShared;
+
 typedef struct {
     VhostUserRequest request;
 
@@ -214,6 +224,7 @@ typedef union {
         VhostUserCryptoSession session;
         VhostUserVringArea area;
         VhostUserInflight inflight;
+        VhostUserShared object;
 } VhostUserPayload;
 
 typedef struct VhostUserMsg {
@@ -1582,6 +1593,83 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
     return 0;
 }
 
+static int
+vhost_user_backend_handle_shared_object_add(VhostUserShared *object,
+                                            int dmabuf_fd)
+{
+    QemuUUID uuid;
+
+    memcpy(uuid.data, object->uuid, sizeof(object->uuid));
+    return virtio_add_dmabuf(&uuid, dmabuf_fd);
+}
+
+static int
+vhost_user_backend_handle_shared_object_remove(VhostUserShared *object)
+{
+    QemuUUID uuid;
+
+    memcpy(uuid.data, object->uuid, sizeof(object->uuid));
+    return virtio_remove_resource(&uuid);
+}
+
+static bool
+vhost_user_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->u64);
+
+    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 int
+vhost_user_backend_handle_shared_object_lookup(struct vhost_user *u,
+                                               QIOChannel *ioc,
+                                               VhostUserHeader *hdr,
+                                               VhostUserPayload *payload)
+{
+    QemuUUID uuid;
+    CharBackend *chr = u->user->chr;
+    int dmabuf_fd = -1;
+    int fd_num = 0;
+
+    memcpy(uuid.data, payload->object.uuid, sizeof(payload->object.uuid));
+
+    dmabuf_fd = virtio_lookup_dmabuf(&uuid);
+    if (dmabuf_fd != -1) {
+        fd_num++;
+    }
+
+    payload->u64 = 0;
+    if (qemu_chr_fe_set_msgfds(chr, &dmabuf_fd, fd_num) < 0) {
+        error_report("Failed to set msg fds.");
+        payload->u64 = -EINVAL;
+    }
+
+    if (!vhost_user_backend_send_dmabuf_fd(ioc, hdr, payload)) {
+        error_report("Failed to write response msg.");
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
 static void close_slave_channel(struct vhost_user *u)
 {
     g_source_destroy(u->slave_src);
@@ -1639,6 +1727,17 @@ 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_ADD:
+        ret = vhost_user_backend_handle_shared_object_add(&payload.object,
+                                                          fd ? fd[0] : -1);
+        break;
+    case VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE:
+        ret = vhost_user_backend_handle_shared_object_remove(&payload.object);
+        break;
+    case VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP:
+        ret = vhost_user_backend_handle_shared_object_lookup(dev->opaque, ioc,
+                                                             &hdr, &payload);
+        break;
     default:
         error_report("Received unexpected msg type: %d.", hdr.request);
         ret = -EINVAL;
diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index 8fb61e2df2..672d8292a0 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -1403,6 +1403,107 @@ 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)
+{
+    bool result = false;
+    VhostUserMsg msg_reply;
+    VhostUserMsg msg = {
+        .request = VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP,
+        .size = sizeof(msg.payload.object),
+        .flags = VHOST_USER_VERSION | VHOST_USER_NEED_REPLY_MASK,
+    };
+
+    memcpy(msg.payload.object.uuid, uuid, sizeof(uuid[0]) * UUID_LEN);
+
+    if (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SHARED_OBJECT)) {
+        return false;
+    }
+
+    pthread_mutex_lock(&dev->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;
+    }
+
+    if (msg_reply.fd_num != 1) {
+        DPRINT("Received unexpected number of fds. Expected 1, received %d",
+               msg_reply.fd_num);
+        goto out;
+    }
+
+    *dmabuf_fd = msg_reply.fds[0];
+    result = *dmabuf_fd > 0 && msg_reply.payload.u64 == 0;
+out:
+    pthread_mutex_unlock(&dev->slave_mutex);
+
+    return result;
+}
+
+static bool
+vu_send_message(VuDev *dev, VhostUserMsg *vmsg)
+{
+    bool result = false;
+    pthread_mutex_lock(&dev->slave_mutex);
+    if (!vu_message_write(dev, dev->slave_fd, vmsg)) {
+        goto out;
+    }
+
+    result = true;
+out:
+    pthread_mutex_unlock(&dev->slave_mutex);
+
+    return result;
+}
+
+bool
+vu_add_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN], int dmabuf_fd)
+{
+    int fd_num = 0;
+    VhostUserMsg msg = {
+        .request = VHOST_USER_BACKEND_SHARED_OBJECT_ADD,
+        .size = sizeof(msg.payload.object),
+        .flags = VHOST_USER_VERSION,
+    };
+
+    msg.fds[fd_num++] = dmabuf_fd;
+    msg.fd_num = fd_num;
+    memcpy(msg.payload.object.uuid, uuid, sizeof(uuid[0]) * UUID_LEN);
+
+    if (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SHARED_OBJECT)) {
+        return false;
+    }
+
+    return vu_send_message(dev, &msg);
+}
+
+bool
+vu_rm_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN])
+{
+    VhostUserMsg msg = {
+        .request = VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE,
+        .size = sizeof(msg.payload.object),
+        .flags = VHOST_USER_VERSION,
+    };
+
+    memcpy(msg.payload.object.uuid, uuid, sizeof(uuid[0]) * UUID_LEN);
+
+    if (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SHARED_OBJECT)) {
+        return false;
+    }
+
+    return vu_send_message(dev, &msg);
+}
+
 static bool
 vu_set_vring_call_exec(VuDev *dev, VhostUserMsg *vmsg)
 {
diff --git a/subprojects/libvhost-user/libvhost-user.h b/subprojects/libvhost-user/libvhost-user.h
index 49208cceaa..907af1bcda 100644
--- a/subprojects/libvhost-user/libvhost-user.h
+++ b/subprojects/libvhost-user/libvhost-user.h
@@ -64,7 +64,8 @@ enum VhostUserProtocolFeature {
     VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD = 12,
     VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14,
     VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15,
-
+    /* Feature 16 is reserved for VHOST_USER_PROTOCOL_F_STATUS. */
+    VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 17,
     VHOST_USER_PROTOCOL_F_MAX
 };
 
@@ -119,6 +120,9 @@ 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_ADD = 6,
+    VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE = 7,
+    VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP = 8,
     VHOST_USER_BACKEND_MAX
 }  VhostUserSlaveRequest;
 
@@ -172,6 +176,12 @@ typedef struct VhostUserInflight {
     uint16_t queue_size;
 } VhostUserInflight;
 
+#define UUID_LEN 16
+
+typedef struct VhostUserShared {
+    unsigned char uuid[UUID_LEN];
+} VhostUserShared;
+
 #if defined(_WIN32) && (defined(__x86_64__) || defined(__i386__))
 # define VU_PACKED __attribute__((gcc_struct, packed))
 #else
@@ -199,6 +209,7 @@ typedef struct VhostUserMsg {
         VhostUserConfig config;
         VhostUserVringArea area;
         VhostUserInflight inflight;
+        VhostUserShared object;
     } payload;
 
     int fds[VHOST_MEMORY_BASELINE_NREGIONS];
@@ -539,6 +550,46 @@ 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_add_shared_object:
+ * @dev: a VuDev context
+ * @uuid: UUID of the shared object
+ * @dmabuf_fd: output dma-buf file descriptor
+ *
+ * Stores a new shared object (i.e., dma-buf fd) in the hash table, and
+ * associates it with the received UUID.
+ *
+ * Returns: TRUE on success, FALSE on failure.
+ */
+bool vu_add_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN],
+                          int dmabuf_fd);
+
+/**
+ * vu_rm_shared_object:
+ * @dev: a VuDev context
+ * @uuid: UUID of the shared object
+ *
+ * Removes a shared object (i.e., dma-buf fd) associated with the
+ * received UUID from the hash table.
+ *
+ * Returns: TRUE on success, FALSE on failure.
+ */
+bool vu_rm_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN]);
+
 /**
  * vu_queue_set_notification:
  * @dev: a VuDev context
-- 
2.40.0

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

[Michael S. Tsirkin]

On Mon, Jun 26, 2023 at 09:34:25AM +0200, Albert Esteve wrote:
> Add three new vhost-user protocol
> `VHOST_USER_BACKEND_SHARED_OBJECT_* messages`.
> These new messages are sent from vhost-user
> back-ends to interact with the virtio-dmabuf
> table in order to add, 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 we need to add
> helper functions to allow sending messages to
> interact with the virtio shared objects
> hash table.
> 
> The messages can only be sent after successfully
> negotiating a new VHOST_USER_PROTOCOL_F_SHARED_OBJECT
> vhost-user protocol feature bit.
> 
> Signed-off-by: Albert Esteve <aesteve@redhat.com>

It bothers me that apparently, any backend can now
make qemu allocate any amount of memory by sending
lots of add messages.

Any way to limit this? If not - at least let's make this
a property that's opt-in?


> ---
>  docs/interop/vhost-user.rst               |  42 +++++++++
>  hw/virtio/vhost-user.c                    |  99 +++++++++++++++++++++
>  subprojects/libvhost-user/libvhost-user.c | 101 ++++++++++++++++++++++
>  subprojects/libvhost-user/libvhost-user.h |  53 +++++++++++-
>  4 files changed, 294 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
> index 5a070adbc1..bca5600ff1 100644
> --- a/docs/interop/vhost-user.rst
> +++ b/docs/interop/vhost-user.rst
> @@ -1528,6 +1528,48 @@ is sent by the front-end.
>  
>    The state.num field is currently reserved and must be set to 0.
>  
> +``VHOST_USER_BACKEND_SHARED_OBJECT_ADD``
> +  :id: 6
> +  :equivalent ioctl: N/A
> +  :request payload: ``struct VhostUserShared``
> +  :reply payload: N/A
> +
> +  When the ``VHOST_USER_PROTOCOL_F_SHARED_OBJECT`` protocol
> +  feature has been successfully negotiated, this message can be submitted
> +  by the backends to add a new dma-buf fd to the virtio-dmabuf shared
> +  table API can send this message. The fd gets associated with a UUID.
> +  If ``VHOST_USER_PROTOCOL_F_REPLY_ACK`` is negotiated, and the back-end sets
> +  the ``VHOST_USER_NEED_REPLY`` flag, the front-end must respond with zero when
> +  operation is successfully completed, or non-zero otherwise.
> +
> +``VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE``
> +  :id: 7
> +  :equivalent ioctl: N/A
> +  :request payload: ``struct VhostUserShared``
> +  :reply payload: N/A
> +
> +  When the ``VHOST_USER_PROTOCOL_F_SHARED_OBJECT`` protocol
> +  feature has been successfully negotiated, this message can be submitted
> +  by the backend to remove a dma-buf from to the virtio-dmabuf shared
> +  table API can send this message. The shared table will remove the dma-buf
> +  fd associated with the UUID. If ``VHOST_USER_PROTOCOL_F_REPLY_ACK`` is
> +  negotiated, and the back-end sets the ``VHOST_USER_NEED_REPLY`` flag, the
> +  front-end must respond with zero when operation is successfully completed,
> +  or non-zero otherwise.
> +
> +``VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP``
> +  :id: 8
> +  :equivalent ioctl: N/A
> +  :request payload: ``struct VhostUserShared``
> +  :reply payload: dmabuf fd and ``u64``
> +
> +  When the ``VHOST_USER_PROTOCOL_F_SHARED_OBJECT`` protocol
> +  feature has been successfully negotiated, this message can be submitted
> +  by the backends to retrieve a given dma-buf fd from the virtio-dmabuf
> +  shared table given a UUID. Frontend will reply passing the fd and a zero
> +  when the operation is successful, or non-zero otherwise. Note that if the
> +  operation fails, no fd is sent to the backend.
> +
>  .. _reply_ack:
>  
>  VHOST_USER_PROTOCOL_F_REPLY_ACK
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 74a2a28663..e340c39a19 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"
> @@ -73,6 +75,7 @@ enum VhostUserProtocolFeature {
>      /* Feature 14 reserved for VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS. */
>      VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15,
>      VHOST_USER_PROTOCOL_F_STATUS = 16,
> +    VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 17,
>      VHOST_USER_PROTOCOL_F_MAX
>  };
>  
> @@ -128,6 +131,9 @@ 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_ADD = 6,
> +    VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE = 7,
> +    VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP = 8,
>      VHOST_USER_BACKEND_MAX
>  }  VhostUserSlaveRequest;
>  
> @@ -190,6 +196,10 @@ typedef struct VhostUserInflight {
>      uint16_t queue_size;
>  } VhostUserInflight;
>  
> +typedef struct VhostUserShared {
> +    unsigned char uuid[16];
> +} VhostUserShared;
> +
>  typedef struct {
>      VhostUserRequest request;
>  
> @@ -214,6 +224,7 @@ typedef union {
>          VhostUserCryptoSession session;
>          VhostUserVringArea area;
>          VhostUserInflight inflight;
> +        VhostUserShared object;
>  } VhostUserPayload;
>  
>  typedef struct VhostUserMsg {
> @@ -1582,6 +1593,83 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
>      return 0;
>  }
>  
> +static int
> +vhost_user_backend_handle_shared_object_add(VhostUserShared *object,
> +                                            int dmabuf_fd)
> +{
> +    QemuUUID uuid;
> +
> +    memcpy(uuid.data, object->uuid, sizeof(object->uuid));
> +    return virtio_add_dmabuf(&uuid, dmabuf_fd);
> +}
> +
> +static int
> +vhost_user_backend_handle_shared_object_remove(VhostUserShared *object)
> +{
> +    QemuUUID uuid;
> +
> +    memcpy(uuid.data, object->uuid, sizeof(object->uuid));
> +    return virtio_remove_resource(&uuid);
> +}
> +
> +static bool
> +vhost_user_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->u64);
> +
> +    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 int
> +vhost_user_backend_handle_shared_object_lookup(struct vhost_user *u,
> +                                               QIOChannel *ioc,
> +                                               VhostUserHeader *hdr,
> +                                               VhostUserPayload *payload)
> +{
> +    QemuUUID uuid;
> +    CharBackend *chr = u->user->chr;
> +    int dmabuf_fd = -1;
> +    int fd_num = 0;
> +
> +    memcpy(uuid.data, payload->object.uuid, sizeof(payload->object.uuid));
> +
> +    dmabuf_fd = virtio_lookup_dmabuf(&uuid);
> +    if (dmabuf_fd != -1) {
> +        fd_num++;
> +    }
> +
> +    payload->u64 = 0;
> +    if (qemu_chr_fe_set_msgfds(chr, &dmabuf_fd, fd_num) < 0) {
> +        error_report("Failed to set msg fds.");
> +        payload->u64 = -EINVAL;
> +    }
> +
> +    if (!vhost_user_backend_send_dmabuf_fd(ioc, hdr, payload)) {
> +        error_report("Failed to write response msg.");
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
>  static void close_slave_channel(struct vhost_user *u)
>  {
>      g_source_destroy(u->slave_src);
> @@ -1639,6 +1727,17 @@ 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_ADD:
> +        ret = vhost_user_backend_handle_shared_object_add(&payload.object,
> +                                                          fd ? fd[0] : -1);
> +        break;
> +    case VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE:
> +        ret = vhost_user_backend_handle_shared_object_remove(&payload.object);
> +        break;
> +    case VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP:
> +        ret = vhost_user_backend_handle_shared_object_lookup(dev->opaque, ioc,
> +                                                             &hdr, &payload);
> +        break;
>      default:
>          error_report("Received unexpected msg type: %d.", hdr.request);
>          ret = -EINVAL;
> diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> index 8fb61e2df2..672d8292a0 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -1403,6 +1403,107 @@ 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)
> +{
> +    bool result = false;
> +    VhostUserMsg msg_reply;
> +    VhostUserMsg msg = {
> +        .request = VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP,
> +        .size = sizeof(msg.payload.object),
> +        .flags = VHOST_USER_VERSION | VHOST_USER_NEED_REPLY_MASK,
> +    };
> +
> +    memcpy(msg.payload.object.uuid, uuid, sizeof(uuid[0]) * UUID_LEN);
> +
> +    if (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SHARED_OBJECT)) {
> +        return false;
> +    }
> +
> +    pthread_mutex_lock(&dev->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;
> +    }
> +
> +    if (msg_reply.fd_num != 1) {
> +        DPRINT("Received unexpected number of fds. Expected 1, received %d",
> +               msg_reply.fd_num);
> +        goto out;
> +    }
> +
> +    *dmabuf_fd = msg_reply.fds[0];
> +    result = *dmabuf_fd > 0 && msg_reply.payload.u64 == 0;
> +out:
> +    pthread_mutex_unlock(&dev->slave_mutex);
> +
> +    return result;
> +}
> +
> +static bool
> +vu_send_message(VuDev *dev, VhostUserMsg *vmsg)
> +{
> +    bool result = false;
> +    pthread_mutex_lock(&dev->slave_mutex);
> +    if (!vu_message_write(dev, dev->slave_fd, vmsg)) {
> +        goto out;
> +    }
> +
> +    result = true;
> +out:
> +    pthread_mutex_unlock(&dev->slave_mutex);
> +
> +    return result;
> +}
> +
> +bool
> +vu_add_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN], int dmabuf_fd)
> +{
> +    int fd_num = 0;
> +    VhostUserMsg msg = {
> +        .request = VHOST_USER_BACKEND_SHARED_OBJECT_ADD,
> +        .size = sizeof(msg.payload.object),
> +        .flags = VHOST_USER_VERSION,
> +    };
> +
> +    msg.fds[fd_num++] = dmabuf_fd;
> +    msg.fd_num = fd_num;
> +    memcpy(msg.payload.object.uuid, uuid, sizeof(uuid[0]) * UUID_LEN);
> +
> +    if (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SHARED_OBJECT)) {
> +        return false;
> +    }
> +
> +    return vu_send_message(dev, &msg);
> +}
> +
> +bool
> +vu_rm_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN])
> +{
> +    VhostUserMsg msg = {
> +        .request = VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE,
> +        .size = sizeof(msg.payload.object),
> +        .flags = VHOST_USER_VERSION,
> +    };
> +
> +    memcpy(msg.payload.object.uuid, uuid, sizeof(uuid[0]) * UUID_LEN);
> +
> +    if (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SHARED_OBJECT)) {
> +        return false;
> +    }
> +
> +    return vu_send_message(dev, &msg);
> +}
> +
>  static bool
>  vu_set_vring_call_exec(VuDev *dev, VhostUserMsg *vmsg)
>  {
> diff --git a/subprojects/libvhost-user/libvhost-user.h b/subprojects/libvhost-user/libvhost-user.h
> index 49208cceaa..907af1bcda 100644
> --- a/subprojects/libvhost-user/libvhost-user.h
> +++ b/subprojects/libvhost-user/libvhost-user.h
> @@ -64,7 +64,8 @@ enum VhostUserProtocolFeature {
>      VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD = 12,
>      VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14,
>      VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15,
> -
> +    /* Feature 16 is reserved for VHOST_USER_PROTOCOL_F_STATUS. */
> +    VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 17,
>      VHOST_USER_PROTOCOL_F_MAX
>  };
>  
> @@ -119,6 +120,9 @@ 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_ADD = 6,
> +    VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE = 7,
> +    VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP = 8,
>      VHOST_USER_BACKEND_MAX
>  }  VhostUserSlaveRequest;
>  
> @@ -172,6 +176,12 @@ typedef struct VhostUserInflight {
>      uint16_t queue_size;
>  } VhostUserInflight;
>  
> +#define UUID_LEN 16
> +
> +typedef struct VhostUserShared {
> +    unsigned char uuid[UUID_LEN];
> +} VhostUserShared;
> +
>  #if defined(_WIN32) && (defined(__x86_64__) || defined(__i386__))
>  # define VU_PACKED __attribute__((gcc_struct, packed))
>  #else
> @@ -199,6 +209,7 @@ typedef struct VhostUserMsg {
>          VhostUserConfig config;
>          VhostUserVringArea area;
>          VhostUserInflight inflight;
> +        VhostUserShared object;
>      } payload;
>  
>      int fds[VHOST_MEMORY_BASELINE_NREGIONS];
> @@ -539,6 +550,46 @@ 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_add_shared_object:
> + * @dev: a VuDev context
> + * @uuid: UUID of the shared object
> + * @dmabuf_fd: output dma-buf file descriptor
> + *
> + * Stores a new shared object (i.e., dma-buf fd) in the hash table, and
> + * associates it with the received UUID.
> + *
> + * Returns: TRUE on success, FALSE on failure.
> + */
> +bool vu_add_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN],
> +                          int dmabuf_fd);
> +
> +/**
> + * vu_rm_shared_object:
> + * @dev: a VuDev context
> + * @uuid: UUID of the shared object
> + *
> + * Removes a shared object (i.e., dma-buf fd) associated with the
> + * received UUID from the hash table.
> + *
> + * Returns: TRUE on success, FALSE on failure.
> + */
> +bool vu_rm_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN]);
> +
>  /**
>   * vu_queue_set_notification:
>   * @dev: a VuDev context
> -- 
> 2.40.0

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

[Albert Esteve]

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

Hi Michael,

True. It may be a good idea to impose a limit in the number of entries that
can be added to the table.
And fail to add new entries once it reaches the limit.

Not sure what would be a good limit though. For example,
https://www.kernel.org/doc/html/v4.9/media/uapi/v4l/vidioc-reqbufs.html#c.v4l2_requestbuffers
does not limit the number of buffers that can be allocated simultaneously,
it is an unsigned 32-bits value.
However, I guess 16-bits (65535) would suffice to cover the vast majority
of usecases. Or even lower, and
can be adjusted later, as this API gets (more) used.

Does that make sense?

Thanks.
BR,
Albert

On Mon, Jul 10, 2023 at 9:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:

> On Mon, Jun 26, 2023 at 09:34:25AM +0200, Albert Esteve wrote:
> > Add three new vhost-user protocol
> > `VHOST_USER_BACKEND_SHARED_OBJECT_* messages`.
> > These new messages are sent from vhost-user
> > back-ends to interact with the virtio-dmabuf
> > table in order to add, 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 we need to add
> > helper functions to allow sending messages to
> > interact with the virtio shared objects
> > hash table.
> >
> > The messages can only be sent after successfully
> > negotiating a new VHOST_USER_PROTOCOL_F_SHARED_OBJECT
> > vhost-user protocol feature bit.
> >
> > Signed-off-by: Albert Esteve <aesteve@redhat.com>
>
> It bothers me that apparently, any backend can now
> make qemu allocate any amount of memory by sending
> lots of add messages.
>
> Any way to limit this? If not - at least let's make this
> a property that's opt-in?
>
>
> > ---
> >  docs/interop/vhost-user.rst               |  42 +++++++++
> >  hw/virtio/vhost-user.c                    |  99 +++++++++++++++++++++
> >  subprojects/libvhost-user/libvhost-user.c | 101 ++++++++++++++++++++++
> >  subprojects/libvhost-user/libvhost-user.h |  53 +++++++++++-
> >  4 files changed, 294 insertions(+), 1 deletion(-)
> >
> > diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
> > index 5a070adbc1..bca5600ff1 100644
> > --- a/docs/interop/vhost-user.rst
> > +++ b/docs/interop/vhost-user.rst
> > @@ -1528,6 +1528,48 @@ is sent by the front-end.
> >
> >    The state.num field is currently reserved and must be set to 0.
> >
> > +``VHOST_USER_BACKEND_SHARED_OBJECT_ADD``
> > +  :id: 6
> > +  :equivalent ioctl: N/A
> > +  :request payload: ``struct VhostUserShared``
> > +  :reply payload: N/A
> > +
> > +  When the ``VHOST_USER_PROTOCOL_F_SHARED_OBJECT`` protocol
> > +  feature has been successfully negotiated, this message can be
> submitted
> > +  by the backends to add a new dma-buf fd to the virtio-dmabuf shared
> > +  table API can send this message. The fd gets associated with a UUID.
> > +  If ``VHOST_USER_PROTOCOL_F_REPLY_ACK`` is negotiated, and the
> back-end sets
> > +  the ``VHOST_USER_NEED_REPLY`` flag, the front-end must respond with
> zero when
> > +  operation is successfully completed, or non-zero otherwise.
> > +
> > +``VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE``
> > +  :id: 7
> > +  :equivalent ioctl: N/A
> > +  :request payload: ``struct VhostUserShared``
> > +  :reply payload: N/A
> > +
> > +  When the ``VHOST_USER_PROTOCOL_F_SHARED_OBJECT`` protocol
> > +  feature has been successfully negotiated, this message can be
> submitted
> > +  by the backend to remove a dma-buf from to the virtio-dmabuf shared
> > +  table API can send this message. The shared table will remove the
> dma-buf
> > +  fd associated with the UUID. If ``VHOST_USER_PROTOCOL_F_REPLY_ACK`` is
> > +  negotiated, and the back-end sets the ``VHOST_USER_NEED_REPLY`` flag,
> the
> > +  front-end must respond with zero when operation is successfully
> completed,
> > +  or non-zero otherwise.
> > +
> > +``VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP``
> > +  :id: 8
> > +  :equivalent ioctl: N/A
> > +  :request payload: ``struct VhostUserShared``
> > +  :reply payload: dmabuf fd and ``u64``
> > +
> > +  When the ``VHOST_USER_PROTOCOL_F_SHARED_OBJECT`` protocol
> > +  feature has been successfully negotiated, this message can be
> submitted
> > +  by the backends to retrieve a given dma-buf fd from the virtio-dmabuf
> > +  shared table given a UUID. Frontend will reply passing the fd and a
> zero
> > +  when the operation is successful, or non-zero otherwise. Note that if
> the
> > +  operation fails, no fd is sent to the backend.
> > +
> >  .. _reply_ack:
> >
> >  VHOST_USER_PROTOCOL_F_REPLY_ACK
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index 74a2a28663..e340c39a19 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"
> > @@ -73,6 +75,7 @@ enum VhostUserProtocolFeature {
> >      /* Feature 14 reserved for
> VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS. */
> >      VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15,
> >      VHOST_USER_PROTOCOL_F_STATUS = 16,
> > +    VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 17,
> >      VHOST_USER_PROTOCOL_F_MAX
> >  };
> >
> > @@ -128,6 +131,9 @@ 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_ADD = 6,
> > +    VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE = 7,
> > +    VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP = 8,
> >      VHOST_USER_BACKEND_MAX
> >  }  VhostUserSlaveRequest;
> >
> > @@ -190,6 +196,10 @@ typedef struct VhostUserInflight {
> >      uint16_t queue_size;
> >  } VhostUserInflight;
> >
> > +typedef struct VhostUserShared {
> > +    unsigned char uuid[16];
> > +} VhostUserShared;
> > +
> >  typedef struct {
> >      VhostUserRequest request;
> >
> > @@ -214,6 +224,7 @@ typedef union {
> >          VhostUserCryptoSession session;
> >          VhostUserVringArea area;
> >          VhostUserInflight inflight;
> > +        VhostUserShared object;
> >  } VhostUserPayload;
> >
> >  typedef struct VhostUserMsg {
> > @@ -1582,6 +1593,83 @@ static int
> vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
> >      return 0;
> >  }
> >
> > +static int
> > +vhost_user_backend_handle_shared_object_add(VhostUserShared *object,
> > +                                            int dmabuf_fd)
> > +{
> > +    QemuUUID uuid;
> > +
> > +    memcpy(uuid.data, object->uuid, sizeof(object->uuid));
> > +    return virtio_add_dmabuf(&uuid, dmabuf_fd);
> > +}
> > +
> > +static int
> > +vhost_user_backend_handle_shared_object_remove(VhostUserShared *object)
> > +{
> > +    QemuUUID uuid;
> > +
> > +    memcpy(uuid.data, object->uuid, sizeof(object->uuid));
> > +    return virtio_remove_resource(&uuid);
> > +}
> > +
> > +static bool
> > +vhost_user_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->u64);
> > +
> > +    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 int
> > +vhost_user_backend_handle_shared_object_lookup(struct vhost_user *u,
> > +                                               QIOChannel *ioc,
> > +                                               VhostUserHeader *hdr,
> > +                                               VhostUserPayload
> *payload)
> > +{
> > +    QemuUUID uuid;
> > +    CharBackend *chr = u->user->chr;
> > +    int dmabuf_fd = -1;
> > +    int fd_num = 0;
> > +
> > +    memcpy(uuid.data, payload->object.uuid,
> sizeof(payload->object.uuid));
> > +
> > +    dmabuf_fd = virtio_lookup_dmabuf(&uuid);
> > +    if (dmabuf_fd != -1) {
> > +        fd_num++;
> > +    }
> > +
> > +    payload->u64 = 0;
> > +    if (qemu_chr_fe_set_msgfds(chr, &dmabuf_fd, fd_num) < 0) {
> > +        error_report("Failed to set msg fds.");
> > +        payload->u64 = -EINVAL;
> > +    }
> > +
> > +    if (!vhost_user_backend_send_dmabuf_fd(ioc, hdr, payload)) {
> > +        error_report("Failed to write response msg.");
> > +        return -EINVAL;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  static void close_slave_channel(struct vhost_user *u)
> >  {
> >      g_source_destroy(u->slave_src);
> > @@ -1639,6 +1727,17 @@ 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_ADD:
> > +        ret =
> vhost_user_backend_handle_shared_object_add(&payload.object,
> > +                                                          fd ? fd[0] :
> -1);
> > +        break;
> > +    case VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE:
> > +        ret =
> vhost_user_backend_handle_shared_object_remove(&payload.object);
> > +        break;
> > +    case VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP:
> > +        ret =
> vhost_user_backend_handle_shared_object_lookup(dev->opaque, ioc,
> > +                                                             &hdr,
> &payload);
> > +        break;
> >      default:
> >          error_report("Received unexpected msg type: %d.", hdr.request);
> >          ret = -EINVAL;
> > diff --git a/subprojects/libvhost-user/libvhost-user.c
> b/subprojects/libvhost-user/libvhost-user.c
> > index 8fb61e2df2..672d8292a0 100644
> > --- a/subprojects/libvhost-user/libvhost-user.c
> > +++ b/subprojects/libvhost-user/libvhost-user.c
> > @@ -1403,6 +1403,107 @@ 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)
> > +{
> > +    bool result = false;
> > +    VhostUserMsg msg_reply;
> > +    VhostUserMsg msg = {
> > +        .request = VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP,
> > +        .size = sizeof(msg.payload.object),
> > +        .flags = VHOST_USER_VERSION | VHOST_USER_NEED_REPLY_MASK,
> > +    };
> > +
> > +    memcpy(msg.payload.object.uuid, uuid, sizeof(uuid[0]) * UUID_LEN);
> > +
> > +    if (!vu_has_protocol_feature(dev,
> VHOST_USER_PROTOCOL_F_SHARED_OBJECT)) {
> > +        return false;
> > +    }
> > +
> > +    pthread_mutex_lock(&dev->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;
> > +    }
> > +
> > +    if (msg_reply.fd_num != 1) {
> > +        DPRINT("Received unexpected number of fds. Expected 1, received
> %d",
> > +               msg_reply.fd_num);
> > +        goto out;
> > +    }
> > +
> > +    *dmabuf_fd = msg_reply.fds[0];
> > +    result = *dmabuf_fd > 0 && msg_reply.payload.u64 == 0;
> > +out:
> > +    pthread_mutex_unlock(&dev->slave_mutex);
> > +
> > +    return result;
> > +}
> > +
> > +static bool
> > +vu_send_message(VuDev *dev, VhostUserMsg *vmsg)
> > +{
> > +    bool result = false;
> > +    pthread_mutex_lock(&dev->slave_mutex);
> > +    if (!vu_message_write(dev, dev->slave_fd, vmsg)) {
> > +        goto out;
> > +    }
> > +
> > +    result = true;
> > +out:
> > +    pthread_mutex_unlock(&dev->slave_mutex);
> > +
> > +    return result;
> > +}
> > +
> > +bool
> > +vu_add_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN], int
> dmabuf_fd)
> > +{
> > +    int fd_num = 0;
> > +    VhostUserMsg msg = {
> > +        .request = VHOST_USER_BACKEND_SHARED_OBJECT_ADD,
> > +        .size = sizeof(msg.payload.object),
> > +        .flags = VHOST_USER_VERSION,
> > +    };
> > +
> > +    msg.fds[fd_num++] = dmabuf_fd;
> > +    msg.fd_num = fd_num;
> > +    memcpy(msg.payload.object.uuid, uuid, sizeof(uuid[0]) * UUID_LEN);
> > +
> > +    if (!vu_has_protocol_feature(dev,
> VHOST_USER_PROTOCOL_F_SHARED_OBJECT)) {
> > +        return false;
> > +    }
> > +
> > +    return vu_send_message(dev, &msg);
> > +}
> > +
> > +bool
> > +vu_rm_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN])
> > +{
> > +    VhostUserMsg msg = {
> > +        .request = VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE,
> > +        .size = sizeof(msg.payload.object),
> > +        .flags = VHOST_USER_VERSION,
> > +    };
> > +
> > +    memcpy(msg.payload.object.uuid, uuid, sizeof(uuid[0]) * UUID_LEN);
> > +
> > +    if (!vu_has_protocol_feature(dev,
> VHOST_USER_PROTOCOL_F_SHARED_OBJECT)) {
> > +        return false;
> > +    }
> > +
> > +    return vu_send_message(dev, &msg);
> > +}
> > +
> >  static bool
> >  vu_set_vring_call_exec(VuDev *dev, VhostUserMsg *vmsg)
> >  {
> > diff --git a/subprojects/libvhost-user/libvhost-user.h
> b/subprojects/libvhost-user/libvhost-user.h
> > index 49208cceaa..907af1bcda 100644
> > --- a/subprojects/libvhost-user/libvhost-user.h
> > +++ b/subprojects/libvhost-user/libvhost-user.h
> > @@ -64,7 +64,8 @@ enum VhostUserProtocolFeature {
> >      VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD = 12,
> >      VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14,
> >      VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15,
> > -
> > +    /* Feature 16 is reserved for VHOST_USER_PROTOCOL_F_STATUS. */
> > +    VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 17,
> >      VHOST_USER_PROTOCOL_F_MAX
> >  };
> >
> > @@ -119,6 +120,9 @@ 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_ADD = 6,
> > +    VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE = 7,
> > +    VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP = 8,
> >      VHOST_USER_BACKEND_MAX
> >  }  VhostUserSlaveRequest;
> >
> > @@ -172,6 +176,12 @@ typedef struct VhostUserInflight {
> >      uint16_t queue_size;
> >  } VhostUserInflight;
> >
> > +#define UUID_LEN 16
> > +
> > +typedef struct VhostUserShared {
> > +    unsigned char uuid[UUID_LEN];
> > +} VhostUserShared;
> > +
> >  #if defined(_WIN32) && (defined(__x86_64__) || defined(__i386__))
> >  # define VU_PACKED __attribute__((gcc_struct, packed))
> >  #else
> > @@ -199,6 +209,7 @@ typedef struct VhostUserMsg {
> >          VhostUserConfig config;
> >          VhostUserVringArea area;
> >          VhostUserInflight inflight;
> > +        VhostUserShared object;
> >      } payload;
> >
> >      int fds[VHOST_MEMORY_BASELINE_NREGIONS];
> > @@ -539,6 +550,46 @@ 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_add_shared_object:
> > + * @dev: a VuDev context
> > + * @uuid: UUID of the shared object
> > + * @dmabuf_fd: output dma-buf file descriptor
> > + *
> > + * Stores a new shared object (i.e., dma-buf fd) in the hash table, and
> > + * associates it with the received UUID.
> > + *
> > + * Returns: TRUE on success, FALSE on failure.
> > + */
> > +bool vu_add_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN],
> > +                          int dmabuf_fd);
> > +
> > +/**
> > + * vu_rm_shared_object:
> > + * @dev: a VuDev context
> > + * @uuid: UUID of the shared object
> > + *
> > + * Removes a shared object (i.e., dma-buf fd) associated with the
> > + * received UUID from the hash table.
> > + *
> > + * Returns: TRUE on success, FALSE on failure.
> > + */
> > +bool vu_rm_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN]);
> > +
> >  /**
> >   * vu_queue_set_notification:
> >   * @dev: a VuDev context
> > --
> > 2.40.0
>
>

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

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

[Gerd Hoffmann]

On Mon, Jul 17, 2023 at 01:42:02PM +0200, Albert Esteve wrote:
> Hi Michael,
> 
> True. It may be a good idea to impose a limit in the number of entries that
> can be added to the table.
> And fail to add new entries once it reaches the limit.
> 
> Not sure what would be a good limit though. For example,
> https://www.kernel.org/doc/html/v4.9/media/uapi/v4l/vidioc-reqbufs.html#c.v4l2_requestbuffers
> does not limit the number of buffers that can be allocated simultaneously,
> it is an unsigned 32-bits value.
> However, I guess 16-bits (65535) would suffice to cover the vast majority
> of usecases. Or even lower, and
> can be adjusted later, as this API gets (more) used.

virtio-gpu does accounting on the total amount of memory (look for
'hostmem').  That is only used in case virgl is *not* used, with virgl
it is much harder to figure how much host memory is actually used.
Probably the virglrenderer library would have to implement that.

If we want apply limits to the memory used by buffers it probably makes
sense to do the same, i.e. account the total amount of memory used.
dma-bufs have a fixed size, so that should be doable without too much
trouble.  Might need some changes to the API because that'll give us a
few new possible failure modes.

take care,
  Gerd

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

[Michael S. Tsirkin]

On Mon, Jul 17, 2023 at 01:42:02PM +0200, Albert Esteve wrote:
> Hi Michael,
> 
> True. It may be a good idea to impose a limit in the number of entries that can
> be added to the table.
> And fail to add new entries once it reaches the limit.
> 
> Not sure what would be a good limit though. For example, https://www.kernel.org
> /doc/html/v4.9/media/uapi/v4l/vidioc-reqbufs.html#c.v4l2_requestbuffers
> does not limit the number of buffers that can be allocated simultaneously, it
> is an unsigned 32-bits value.
> However, I guess 16-bits (65535) would suffice to cover the vast majority of
> usecases. Or even lower, and
> can be adjusted later, as this API gets (more) used.
> 
> Does that make sense?
> 
> Thanks.
> BR,
> Albert

let's not top-post please.

Maybe. Another concern is qemu running out of FDs with a bad backend.

Question: why does qemu have to maintain these UUIDs in its memory?

Can't it query the backend with UUID and get the fd back?

And then, the hash table in QEMU becomes just a cache
to speed up lookups.

-- 
MST

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

[Albert Esteve]

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

On Mon, Jul 17, 2023 at 4:11 PM Michael S. Tsirkin <mst@redhat.com> wrote:

>
>
>
>
> On Mon, Jul 17, 2023 at 01:42:02PM +0200, Albert Esteve wrote:
> > Hi Michael,
> >
> > True. It may be a good idea to impose a limit in the number of entries
> that can
> > be added to the table.
> > And fail to add new entries once it reaches the limit.
> >
> > Not sure what would be a good limit though. For example,
> https://www.kernel.org
> > /doc/html/v4.9/media/uapi/v4l/vidioc-reqbufs.html#c.v4l2_requestbuffers
> > does not limit the number of buffers that can be allocated
> simultaneously, it
> > is an unsigned 32-bits value.
> > However, I guess 16-bits (65535) would suffice to cover the vast
> majority of
> > usecases. Or even lower, and
> > can be adjusted later, as this API gets (more) used.
> >
> > Does that make sense?
> >
> > Thanks.
> > BR,
> > Albert
>
> let's not top-post please.
>
> Maybe. Another concern is qemu running out of FDs with a bad backend.
>
> Question: why does qemu have to maintain these UUIDs in its memory?
>
> Can't it query the backend with UUID and get the fd back?
>

In the end, we have one backend sharing an object with other backends.
From the importer POV, it does not know who the exporter is, so it cannot
go pocking other backends until it finds the one that is holding a resource
with
the same UUID, it relies on qemu providing this information.

If we do not want qemu to hold the fds, we could, for instance, store
references to
backends that act as exporters. And then, once an importer requests for a
specific
object with its UUID, we ask for the fd to the exporter(s), hoping to find
it.

But the current solution sounds better fit to the shared objects virtio
feature.
I would be more keen to look into something like what Gerd suggested,
limiting
the memory that we use.

Nonetheless, in qemu we are storing fds, and not mmaping the dmabufs.
So I think limiting the number of entries should suffice, to ensure
that we do not run out of FDs, and memory.


>
> And then, the hash table in QEMU becomes just a cache
> to speed up lookups.
>
> --
> MST
>
>

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

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

[Michael S. Tsirkin]

On Thu, Jul 27, 2023 at 04:48:30PM +0200, Albert Esteve wrote:
> 
> 
> On Mon, Jul 17, 2023 at 4:11 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> 
> 
> 
> 
>     On Mon, Jul 17, 2023 at 01:42:02PM +0200, Albert Esteve wrote:
>     > Hi Michael,
>     >
>     > True. It may be a good idea to impose a limit in the number of entries
>     that can
>     > be added to the table.
>     > And fail to add new entries once it reaches the limit.
>     >
>     > Not sure what would be a good limit though. For example, https://
>     www.kernel.org
>     > /doc/html/v4.9/media/uapi/v4l/vidioc-reqbufs.html#c.v4l2_requestbuffers
>     > does not limit the number of buffers that can be allocated
>     simultaneously, it
>     > is an unsigned 32-bits value.
>     > However, I guess 16-bits (65535) would suffice to cover the vast majority
>     of
>     > usecases. Or even lower, and
>     > can be adjusted later, as this API gets (more) used.
>     >
>     > Does that make sense?
>     >
>     > Thanks.
>     > BR,
>     > Albert
> 
>     let's not top-post please.
> 
>     Maybe. Another concern is qemu running out of FDs with a bad backend.
> 
>     Question: why does qemu have to maintain these UUIDs in its memory?
> 
>     Can't it query the backend with UUID and get the fd back?
> 
> 
> In the end, we have one backend sharing an object with other backends.
> From the importer POV, it does not know who the exporter is, so it cannot
> go pocking other backends until it finds the one that is holding a resource
> with
> the same UUID, it relies on qemu providing this information.
> 
> If we do not want qemu to hold the fds, we could, for instance, store
> references to
> backends that act as exporters. And then, once an importer requests for a
> specific
> object with its UUID, we ask for the fd to the exporter(s), hoping to find it.


right. I'd do this. and then the existing table can be regarded
as a cache.

> But the current solution sounds better fit to the shared objects virtio
> feature.
> I would be more keen to look into something like what Gerd suggested, limiting
> the memory that we use. 
> 
> Nonetheless, in qemu we are storing fds, and not mmaping the dmabufs.
> So I think limiting the number of entries should suffice, to ensure
> that we do not run out of FDs, and memory.

my point is you really don't know how much to limit it.
if there's ability to drop the entries then you
can do this, and cache things in memory.


> 
> 
>     And then, the hash table in QEMU becomes just a cache
>     to speed up lookups.
> 
>     --
>     MST
> 
> 

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

[Albert Esteve]

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

On Thu, Jul 27, 2023 at 4:57 PM Michael S. Tsirkin <mst@redhat.com> wrote:

> On Thu, Jul 27, 2023 at 04:48:30PM +0200, Albert Esteve wrote:
> >
> >
> > On Mon, Jul 17, 2023 at 4:11 PM Michael S. Tsirkin <mst@redhat.com>
> wrote:
> >
> >
> >
> >
> >
> >     On Mon, Jul 17, 2023 at 01:42:02PM +0200, Albert Esteve wrote:
> >     > Hi Michael,
> >     >
> >     > True. It may be a good idea to impose a limit in the number of
> entries
> >     that can
> >     > be added to the table.
> >     > And fail to add new entries once it reaches the limit.
> >     >
> >     > Not sure what would be a good limit though. For example, https://
> >     www.kernel.org
> >     >
> /doc/html/v4.9/media/uapi/v4l/vidioc-reqbufs.html#c.v4l2_requestbuffers
> >     > does not limit the number of buffers that can be allocated
> >     simultaneously, it
> >     > is an unsigned 32-bits value.
> >     > However, I guess 16-bits (65535) would suffice to cover the vast
> majority
> >     of
> >     > usecases. Or even lower, and
> >     > can be adjusted later, as this API gets (more) used.
> >     >
> >     > Does that make sense?
> >     >
> >     > Thanks.
> >     > BR,
> >     > Albert
> >
> >     let's not top-post please.
> >
> >     Maybe. Another concern is qemu running out of FDs with a bad backend.
> >
> >     Question: why does qemu have to maintain these UUIDs in its memory?
> >
> >     Can't it query the backend with UUID and get the fd back?
> >
> >
> > In the end, we have one backend sharing an object with other backends.
> > From the importer POV, it does not know who the exporter is, so it cannot
> > go pocking other backends until it finds the one that is holding a
> resource
> > with
> > the same UUID, it relies on qemu providing this information.
> >
> > If we do not want qemu to hold the fds, we could, for instance, store
> > references to
> > backends that act as exporters. And then, once an importer requests for a
> > specific
> > object with its UUID, we ask for the fd to the exporter(s), hoping to
> find it.
>
>
> right. I'd do this. and then the existing table can be regarded
> as a cache.
>

It is true that it is not easy to find a limit that fits all usecases,
and the cache proposal could result in a more maintanable
solution in the long term.

I'll explore this and post a proposal for the next version
of the patch. It will mean having a bigger changeset, so
I'll try to push something as clean as possible.

BR,
Albert


>
> > But the current solution sounds better fit to the shared objects virtio
> > feature.
> > I would be more keen to look into something like what Gerd suggested,
> limiting
> > the memory that we use.
> >
> > Nonetheless, in qemu we are storing fds, and not mmaping the dmabufs.
> > So I think limiting the number of entries should suffice, to ensure
> > that we do not run out of FDs, and memory.
>
> my point is you really don't know how much to limit it.
> if there's ability to drop the entries then you
> can do this, and cache things in memory.
>
>
> >
> >
> >     And then, the hash table in QEMU becomes just a cache
> >     to speed up lookups.
> >
> >     --
> >     MST
> >
> >
>
>

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

[PATCH v4 4/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 | 44 ++++++++++++++++--------------------------
 1 file changed, 17 insertions(+), 27 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index e340c39a19..f2b224b5a3 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1612,32 +1612,34 @@ vhost_user_backend_handle_shared_object_remove(VhostUserShared *object)
     return virtio_remove_resource(&uuid);
 }
 
-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];
+    struct iovec iov[] = {
+        { .iov_base = hdr,      .iov_len = VHOST_USER_HDR_SIZE },
+        { .iov_base = payload,  .iov_len = hdr->size },
+    };
 
-    if (hdr->flags & VHOST_USER_NEED_REPLY_MASK) {
-        hdr->flags &= ~VHOST_USER_NEED_REPLY_MASK;
-    }
+    hdr->flags &= ~VHOST_USER_NEED_REPLY_MASK;
     hdr->flags |= VHOST_USER_REPLY_MASK;
 
-    hdr->size = sizeof(payload->u64);
-
-    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 bool
+vhost_user_backend_send_dmabuf_fd(QIOChannel *ioc, VhostUserHeader *hdr,
+                                  VhostUserPayload *payload)
+{
+    hdr->size = sizeof(payload->u64);
+    return vhost_user_send_resp(ioc, hdr, payload);
+}
+
 static int
 vhost_user_backend_handle_shared_object_lookup(struct vhost_user *u,
                                                QIOChannel *ioc,
@@ -1748,22 +1750,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