* [PATCH v4 1/5] hw/virtio: check owner for removing objects
2024-02-19 14:34 [PATCH v4 0/5] Virtio dmabuf improvements Albert Esteve
@ 2024-02-19 14:34 ` Albert Esteve
2024-02-19 14:34 ` [PATCH v4 2/5] hw/virtio: document SharedObject structures Albert Esteve
` (4 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: Albert Esteve @ 2024-02-19 14:34 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, stefanha, alex.bennee, philmd, kraxel,
marcandre.lureau, Albert Esteve, Stefan Hajnoczi
Shared objects lack spoofing protection.
For VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE messages
received by the vhost-user interface, any backend was
allowed to remove entries from the shared table just
by knowing the UUID. Only the owner of the entry
shall be allowed to removed their resources
from the table.
To fix that, add a check for all
*SHARED_OBJECT_REMOVE messages received.
A vhost device can only remove TYPE_VHOST_DEV
entries that are owned by them, otherwise skip
the removal, and inform the device that the entry
has not been removed in the answer.
Signed-off-by: Albert Esteve <aesteve@redhat.com>
Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
---
docs/interop/vhost-user.rst | 4 +++-
hw/virtio/vhost-user.c | 21 +++++++++++++++++++--
2 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
index ad6e142f23..32496b5aa9 100644
--- a/docs/interop/vhost-user.rst
+++ b/docs/interop/vhost-user.rst
@@ -1839,7 +1839,9 @@ is sent by the front-end.
When the ``VHOST_USER_PROTOCOL_F_SHARED_OBJECT`` protocol
feature has been successfully negotiated, this message can be submitted
by the backend to remove themselves from to the virtio-dmabuf shared
- table API. The shared table will remove the back-end device associated with
+ table API. Only the back-end owning the entry (i.e., the one that first added
+ it) will have permission to remove it. Otherwise, the message is ignored.
+ The shared table will remove the back-end device associated with
the UUID. If ``VHOST_USER_PROTOCOL_F_REPLY_ACK`` is negotiated, and the
back-end sets the ``VHOST_USER_NEED_REPLY`` flag, the front-end must respond
with zero when operation is successfully completed, or non-zero otherwise.
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index f214df804b..152710d30d 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1611,11 +1611,27 @@ vhost_user_backend_handle_shared_object_add(struct vhost_dev *dev,
}
static int
-vhost_user_backend_handle_shared_object_remove(VhostUserShared *object)
+vhost_user_backend_handle_shared_object_remove(struct vhost_dev *dev,
+ VhostUserShared *object)
{
QemuUUID uuid;
memcpy(uuid.data, object->uuid, sizeof(object->uuid));
+ switch (virtio_object_type(&uuid)) {
+ case TYPE_VHOST_DEV:
+ {
+ struct vhost_dev *owner = virtio_lookup_vhost_device(&uuid);
+ if (dev != owner) {
+ /* Not allowed to remove non-owned entries */
+ return 0;
+ }
+ break;
+ }
+ default:
+ /* Not allowed to remove non-owned entries */
+ return 0;
+ }
+
return virtio_remove_resource(&uuid);
}
@@ -1794,7 +1810,8 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition,
ret = vhost_user_backend_handle_shared_object_add(dev, &payload.object);
break;
case VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE:
- ret = vhost_user_backend_handle_shared_object_remove(&payload.object);
+ ret = vhost_user_backend_handle_shared_object_remove(dev,
+ &payload.object);
break;
case VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP:
ret = vhost_user_backend_handle_shared_object_lookup(dev->opaque, ioc,
--
2.43.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v4 2/5] hw/virtio: document SharedObject structures
2024-02-19 14:34 [PATCH v4 0/5] Virtio dmabuf improvements Albert Esteve
2024-02-19 14:34 ` [PATCH v4 1/5] hw/virtio: check owner for removing objects Albert Esteve
@ 2024-02-19 14:34 ` Albert Esteve
2024-02-20 10:28 ` Manos Pitsidianakis
2024-02-19 14:34 ` [PATCH v4 3/5] hw/virtio: change dmabuf mutex to QemuMutex Albert Esteve
` (3 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Albert Esteve @ 2024-02-19 14:34 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, stefanha, alex.bennee, philmd, kraxel,
marcandre.lureau, Albert Esteve
Change VirtioSharedObject value type from
a generic pointer to a union storing the different
supported underlying types, which makes naming
less confusing.
With the update, use the chance to add kdoc
to both the SharedObjectType enum and
VirtioSharedObject struct.
Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
hw/display/virtio-dmabuf.c | 8 ++++----
include/hw/virtio/virtio-dmabuf.h | 25 ++++++++++++++++++++++++-
2 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
index 3dba4577ca..497cb6fa7c 100644
--- a/hw/display/virtio-dmabuf.c
+++ b/hw/display/virtio-dmabuf.c
@@ -57,7 +57,7 @@ bool virtio_add_dmabuf(QemuUUID *uuid, int udmabuf_fd)
}
vso = g_new(VirtioSharedObject, 1);
vso->type = TYPE_DMABUF;
- vso->value = GINT_TO_POINTER(udmabuf_fd);
+ vso->value.udma_buf = udmabuf_fd;
result = virtio_add_resource(uuid, vso);
if (!result) {
g_free(vso);
@@ -75,7 +75,7 @@ bool virtio_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev)
}
vso = g_new(VirtioSharedObject, 1);
vso->type = TYPE_VHOST_DEV;
- vso->value = dev;
+ vso->value.dev = dev;
result = virtio_add_resource(uuid, vso);
if (!result) {
g_free(vso);
@@ -114,7 +114,7 @@ int virtio_lookup_dmabuf(const QemuUUID *uuid)
return -1;
}
assert(vso->type == TYPE_DMABUF);
- return GPOINTER_TO_INT(vso->value);
+ return vso->value.udma_buf;
}
struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid)
@@ -124,7 +124,7 @@ struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid)
return NULL;
}
assert(vso->type == TYPE_VHOST_DEV);
- return (struct vhost_dev *) vso->value;
+ return (struct vhost_dev *) vso->value.dev;
}
SharedObjectType virtio_object_type(const QemuUUID *uuid)
diff --git a/include/hw/virtio/virtio-dmabuf.h b/include/hw/virtio/virtio-dmabuf.h
index 627c3b6db7..891a43162d 100644
--- a/include/hw/virtio/virtio-dmabuf.h
+++ b/include/hw/virtio/virtio-dmabuf.h
@@ -16,15 +16,38 @@
#include "qemu/uuid.h"
#include "vhost.h"
+/**
+ * SharedObjectType:
+ *
+ * Identifies the type of the underlying type that the current lookup
+ * table entry is holding.
+ *
+ * TYPE_INVALID: Invalid entry
+ * TYPE_DMABUF: Entry is a dmabuf file descriptor that can be directly
+ * shared with the requestor
+ * TYPE_VHOST_DEV: Entry is a pointer to a vhost device that is holding
+ * the shared object.
+ */
typedef enum SharedObjectType {
TYPE_INVALID = 0,
TYPE_DMABUF,
TYPE_VHOST_DEV,
} SharedObjectType;
+/**
+ * VirtioSharedObject:
+ * @type: Shared object type identifier
+ * @value: Union containing to the underlying type
+ *
+ * The VirtioSharedObject object provides a way to distinguish,
+ * store, and handle the different types supported by the lookup table.
+ */
typedef struct VirtioSharedObject {
SharedObjectType type;
- gpointer value;
+ union {
+ struct vhost_dev *dev; /* TYPE_VHOST_DEV */
+ int udma_buf; /* TYPE_DMABUF */
+ } value;
} VirtioSharedObject;
/**
--
2.43.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v4 2/5] hw/virtio: document SharedObject structures
2024-02-19 14:34 ` [PATCH v4 2/5] hw/virtio: document SharedObject structures Albert Esteve
@ 2024-02-20 10:28 ` Manos Pitsidianakis
2024-03-06 13:13 ` Albert Esteve
0 siblings, 1 reply; 15+ messages in thread
From: Manos Pitsidianakis @ 2024-02-20 10:28 UTC (permalink / raw)
To: qemu-devel, Albert Esteve
Cc: Michael S. Tsirkin, stefanha, alex.bennee, philmd, kraxel,
marcandre.lureau, Albert Esteve
On Mon, 19 Feb 2024 16:34, Albert Esteve <aesteve@redhat.com> wrote:
>Change VirtioSharedObject value type from
>a generic pointer to a union storing the different
>supported underlying types, which makes naming
>less confusing.
>
>With the update, use the chance to add kdoc
>to both the SharedObjectType enum and
>VirtioSharedObject struct.
>
>Signed-off-by: Albert Esteve <aesteve@redhat.com>
>---
> hw/display/virtio-dmabuf.c | 8 ++++----
> include/hw/virtio/virtio-dmabuf.h | 25 ++++++++++++++++++++++++-
> 2 files changed, 28 insertions(+), 5 deletions(-)
>
>diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
>index 3dba4577ca..497cb6fa7c 100644
>--- a/hw/display/virtio-dmabuf.c
>+++ b/hw/display/virtio-dmabuf.c
>@@ -57,7 +57,7 @@ bool virtio_add_dmabuf(QemuUUID *uuid, int udmabuf_fd)
> }
> vso = g_new(VirtioSharedObject, 1);
> vso->type = TYPE_DMABUF;
>- vso->value = GINT_TO_POINTER(udmabuf_fd);
>+ vso->value.udma_buf = udmabuf_fd;
> result = virtio_add_resource(uuid, vso);
> if (!result) {
> g_free(vso);
>@@ -75,7 +75,7 @@ bool virtio_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev)
> }
> vso = g_new(VirtioSharedObject, 1);
> vso->type = TYPE_VHOST_DEV;
>- vso->value = dev;
>+ vso->value.dev = dev;
> result = virtio_add_resource(uuid, vso);
> if (!result) {
> g_free(vso);
>@@ -114,7 +114,7 @@ int virtio_lookup_dmabuf(const QemuUUID *uuid)
> return -1;
> }
> assert(vso->type == TYPE_DMABUF);
>- return GPOINTER_TO_INT(vso->value);
>+ return vso->value.udma_buf;
> }
>
> struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid)
>@@ -124,7 +124,7 @@ struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid)
> return NULL;
> }
> assert(vso->type == TYPE_VHOST_DEV);
>- return (struct vhost_dev *) vso->value;
>+ return (struct vhost_dev *) vso->value.dev;
Is the casting still required?
> }
>
> SharedObjectType virtio_object_type(const QemuUUID *uuid)
>diff --git a/include/hw/virtio/virtio-dmabuf.h b/include/hw/virtio/virtio-dmabuf.h
>index 627c3b6db7..891a43162d 100644
>--- a/include/hw/virtio/virtio-dmabuf.h
>+++ b/include/hw/virtio/virtio-dmabuf.h
>@@ -16,15 +16,38 @@
> #include "qemu/uuid.h"
> #include "vhost.h"
>
>+/**
>+ * SharedObjectType:
>+ *
>+ * Identifies the type of the underlying type that the current lookup
>+ * table entry is holding.
>+ *
>+ * TYPE_INVALID: Invalid entry
>+ * TYPE_DMABUF: Entry is a dmabuf file descriptor that can be directly
>+ * shared with the requestor
>+ * TYPE_VHOST_DEV: Entry is a pointer to a vhost device that is holding
>+ * the shared object.
nit:
+ * TYPE_INVALID: Invalid entry.
+ * TYPE_DMABUF: Entry is a dmabuf file descriptor that can be
+ * directly shared with the requestor.
+ * TYPE_VHOST_DEV: Entry is a pointer to a vhost device that is holding
+ * the shared object.
>+ */
> typedef enum SharedObjectType {
> TYPE_INVALID = 0,
> TYPE_DMABUF,
> TYPE_VHOST_DEV,
> } SharedObjectType;
>
>+/**
>+ * VirtioSharedObject:
>+ * @type: Shared object type identifier
>+ * @value: Union containing to the underlying type
>+ *
>+ * The VirtioSharedObject object provides a way to distinguish,
>+ * store, and handle the different types supported by the lookup table.
>+ */
> typedef struct VirtioSharedObject {
> SharedObjectType type;
>- gpointer value;
>+ union {
>+ struct vhost_dev *dev; /* TYPE_VHOST_DEV */
>+ int udma_buf; /* TYPE_DMABUF */
>+ } value;
> } VirtioSharedObject;
>
> /**
>--
>2.43.1
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 2/5] hw/virtio: document SharedObject structures
2024-02-20 10:28 ` Manos Pitsidianakis
@ 2024-03-06 13:13 ` Albert Esteve
0 siblings, 0 replies; 15+ messages in thread
From: Albert Esteve @ 2024-03-06 13:13 UTC (permalink / raw)
To: Manos Pitsidianakis
Cc: qemu-devel, Michael S. Tsirkin, stefanha, alex.bennee, philmd,
kraxel, marcandre.lureau
[-- Attachment #1: Type: text/plain, Size: 4198 bytes --]
On Tue, Feb 20, 2024 at 11:34 AM Manos Pitsidianakis <
manos.pitsidianakis@linaro.org> wrote:
> On Mon, 19 Feb 2024 16:34, Albert Esteve <aesteve@redhat.com> wrote:
> >Change VirtioSharedObject value type from
> >a generic pointer to a union storing the different
> >supported underlying types, which makes naming
> >less confusing.
> >
> >With the update, use the chance to add kdoc
> >to both the SharedObjectType enum and
> >VirtioSharedObject struct.
> >
> >Signed-off-by: Albert Esteve <aesteve@redhat.com>
> >---
> > hw/display/virtio-dmabuf.c | 8 ++++----
> > include/hw/virtio/virtio-dmabuf.h | 25 ++++++++++++++++++++++++-
> > 2 files changed, 28 insertions(+), 5 deletions(-)
> >
> >diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
> >index 3dba4577ca..497cb6fa7c 100644
> >--- a/hw/display/virtio-dmabuf.c
> >+++ b/hw/display/virtio-dmabuf.c
> >@@ -57,7 +57,7 @@ bool virtio_add_dmabuf(QemuUUID *uuid, int udmabuf_fd)
> > }
> > vso = g_new(VirtioSharedObject, 1);
> > vso->type = TYPE_DMABUF;
> >- vso->value = GINT_TO_POINTER(udmabuf_fd);
> >+ vso->value.udma_buf = udmabuf_fd;
> > result = virtio_add_resource(uuid, vso);
> > if (!result) {
> > g_free(vso);
> >@@ -75,7 +75,7 @@ bool virtio_add_vhost_device(QemuUUID *uuid, struct
> vhost_dev *dev)
> > }
> > vso = g_new(VirtioSharedObject, 1);
> > vso->type = TYPE_VHOST_DEV;
> >- vso->value = dev;
> >+ vso->value.dev = dev;
> > result = virtio_add_resource(uuid, vso);
> > if (!result) {
> > g_free(vso);
> >@@ -114,7 +114,7 @@ int virtio_lookup_dmabuf(const QemuUUID *uuid)
> > return -1;
> > }
> > assert(vso->type == TYPE_DMABUF);
> >- return GPOINTER_TO_INT(vso->value);
> >+ return vso->value.udma_buf;
> > }
> >
> > struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid)
> >@@ -124,7 +124,7 @@ struct vhost_dev *virtio_lookup_vhost_device(const
> QemuUUID *uuid)
> > return NULL;
> > }
> > assert(vso->type == TYPE_VHOST_DEV);
> >- return (struct vhost_dev *) vso->value;
> >+ return (struct vhost_dev *) vso->value.dev;
>
> Is the casting still required?
>
> You're right, probably not anymore. I'll remove it.
>
> > }
> >
> > SharedObjectType virtio_object_type(const QemuUUID *uuid)
> >diff --git a/include/hw/virtio/virtio-dmabuf.h
> b/include/hw/virtio/virtio-dmabuf.h
> >index 627c3b6db7..891a43162d 100644
> >--- a/include/hw/virtio/virtio-dmabuf.h
> >+++ b/include/hw/virtio/virtio-dmabuf.h
> >@@ -16,15 +16,38 @@
> > #include "qemu/uuid.h"
> > #include "vhost.h"
> >
> >+/**
> >+ * SharedObjectType:
> >+ *
> >+ * Identifies the type of the underlying type that the current lookup
> >+ * table entry is holding.
> >+ *
> >+ * TYPE_INVALID: Invalid entry
> >+ * TYPE_DMABUF: Entry is a dmabuf file descriptor that can be directly
> >+ * shared with the requestor
> >+ * TYPE_VHOST_DEV: Entry is a pointer to a vhost device that is holding
> >+ * the shared object.
>
>
> nit:
>
> + * TYPE_INVALID: Invalid entry.
> + * TYPE_DMABUF: Entry is a dmabuf file descriptor that can be
> + * directly shared with the requestor.
> + * TYPE_VHOST_DEV: Entry is a pointer to a vhost device that is holding
> + * the shared object.
>
>
> >+ */
> > typedef enum SharedObjectType {
> > TYPE_INVALID = 0,
> > TYPE_DMABUF,
> > TYPE_VHOST_DEV,
> > } SharedObjectType;
> >
> >+/**
> >+ * VirtioSharedObject:
> >+ * @type: Shared object type identifier
> >+ * @value: Union containing to the underlying type
> >+ *
> >+ * The VirtioSharedObject object provides a way to distinguish,
> >+ * store, and handle the different types supported by the lookup table.
> >+ */
> > typedef struct VirtioSharedObject {
> > SharedObjectType type;
> >- gpointer value;
> >+ union {
> >+ struct vhost_dev *dev; /* TYPE_VHOST_DEV */
> >+ int udma_buf; /* TYPE_DMABUF */
> >+ } value;
> > } VirtioSharedObject;
> >
> > /**
> >--
> >2.43.1
> >
> >
>
>
[-- Attachment #2: Type: text/html, Size: 5762 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 3/5] hw/virtio: change dmabuf mutex to QemuMutex
2024-02-19 14:34 [PATCH v4 0/5] Virtio dmabuf improvements Albert Esteve
2024-02-19 14:34 ` [PATCH v4 1/5] hw/virtio: check owner for removing objects Albert Esteve
2024-02-19 14:34 ` [PATCH v4 2/5] hw/virtio: document SharedObject structures Albert Esteve
@ 2024-02-19 14:34 ` Albert Esteve
2024-02-20 10:34 ` Manos Pitsidianakis
2024-02-19 14:34 ` [PATCH v4 4/5] hw/virtio: cleanup shared resources Albert Esteve
` (2 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Albert Esteve @ 2024-02-19 14:34 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, stefanha, alex.bennee, philmd, kraxel,
marcandre.lureau, Albert Esteve
Change GMutex by QemuMutex to be able to use
lock contexts with `WITH_QEMU_LOCK_GUARD`.
As the lock needs to be initialised and there
is no central point for initialisation, add
an init public function and call it from
virtio.c, each time a new backend structure
is initialised.
Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
hw/display/virtio-dmabuf.c | 55 +++++++++++++++++--------------
hw/virtio/virtio.c | 3 ++
include/hw/virtio/virtio-dmabuf.h | 5 +++
tests/unit/test-virtio-dmabuf.c | 5 +++
4 files changed, 43 insertions(+), 25 deletions(-)
diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
index 497cb6fa7c..961094a561 100644
--- a/hw/display/virtio-dmabuf.c
+++ b/hw/display/virtio-dmabuf.c
@@ -11,11 +11,12 @@
*/
#include "qemu/osdep.h"
+#include "include/qemu/lockable.h"
#include "hw/virtio/virtio-dmabuf.h"
-static GMutex lock;
+static QemuMutex lock;
static GHashTable *resource_uuids;
/*
@@ -27,23 +28,27 @@ static int uuid_equal_func(const void *lhv, const void *rhv)
return qemu_uuid_is_equal(lhv, rhv);
}
+void virtio_dmabuf_init(void) {
+ qemu_mutex_init(&lock);
+}
+
static bool virtio_add_resource(QemuUUID *uuid, VirtioSharedObject *value)
{
bool result = true;
- g_mutex_lock(&lock);
- if (resource_uuids == NULL) {
- resource_uuids = g_hash_table_new_full(qemu_uuid_hash,
- uuid_equal_func,
- NULL,
- g_free);
- }
- if (g_hash_table_lookup(resource_uuids, uuid) == NULL) {
- g_hash_table_insert(resource_uuids, uuid, value);
- } else {
- result = false;
+ WITH_QEMU_LOCK_GUARD(&lock) {
+ if (resource_uuids == NULL) {
+ resource_uuids = g_hash_table_new_full(qemu_uuid_hash,
+ uuid_equal_func,
+ NULL,
+ g_free);
+ }
+ if (g_hash_table_lookup(resource_uuids, uuid) == NULL) {
+ g_hash_table_insert(resource_uuids, uuid, value);
+ } else {
+ result = false;
+ }
}
- g_mutex_unlock(&lock);
return result;
}
@@ -87,9 +92,9 @@ bool virtio_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev)
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);
+ WITH_QEMU_LOCK_GUARD(&lock) {
+ result = g_hash_table_remove(resource_uuids, uuid);
+ }
return result;
}
@@ -98,11 +103,11 @@ static VirtioSharedObject *get_shared_object(const QemuUUID *uuid)
{
gpointer lookup_res = NULL;
- g_mutex_lock(&lock);
- if (resource_uuids != NULL) {
- lookup_res = g_hash_table_lookup(resource_uuids, uuid);
+ WITH_QEMU_LOCK_GUARD(&lock) {
+ if (resource_uuids != NULL) {
+ lookup_res = g_hash_table_lookup(resource_uuids, uuid);
+ }
}
- g_mutex_unlock(&lock);
return (VirtioSharedObject *) lookup_res;
}
@@ -138,9 +143,9 @@ SharedObjectType virtio_object_type(const QemuUUID *uuid)
void virtio_free_resources(void)
{
- g_mutex_lock(&lock);
- g_hash_table_destroy(resource_uuids);
- /* Reference count shall be 0 after the implicit unref on destroy */
- resource_uuids = NULL;
- g_mutex_unlock(&lock);
+ WITH_QEMU_LOCK_GUARD(&lock) {
+ g_hash_table_destroy(resource_uuids);
+ /* Reference count shall be 0 after the implicit unref on destroy */
+ resource_uuids = NULL;
+ }
}
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index d229755eae..88189e7178 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -29,6 +29,7 @@
#include "hw/virtio/virtio-bus.h"
#include "hw/qdev-properties.h"
#include "hw/virtio/virtio-access.h"
+#include "hw/virtio/virtio-dmabuf.h"
#include "sysemu/dma.h"
#include "sysemu/runstate.h"
#include "virtio-qmp.h"
@@ -3221,6 +3222,8 @@ void virtio_init(VirtIODevice *vdev, uint16_t device_id, size_t config_size)
int i;
int nvectors = k->query_nvectors ? k->query_nvectors(qbus->parent) : 0;
+ // Ensure virtio dmabuf table is initialised.
+ virtio_dmabuf_init();
if (nvectors) {
vdev->vector_queues =
g_malloc0(sizeof(*vdev->vector_queues) * nvectors);
diff --git a/include/hw/virtio/virtio-dmabuf.h b/include/hw/virtio/virtio-dmabuf.h
index 891a43162d..627d84dce9 100644
--- a/include/hw/virtio/virtio-dmabuf.h
+++ b/include/hw/virtio/virtio-dmabuf.h
@@ -50,6 +50,11 @@ typedef struct VirtioSharedObject {
} value;
} VirtioSharedObject;
+/**
+ * virtio_dmabuf_init() - Initialise virtio dmabuf internal structures.
+ */
+void virtio_dmabuf_init(void);
+
/**
* virtio_add_dmabuf() - Add a new dma-buf resource to the lookup table
* @uuid: new resource's UUID
diff --git a/tests/unit/test-virtio-dmabuf.c b/tests/unit/test-virtio-dmabuf.c
index a45ec52f42..20213455ee 100644
--- a/tests/unit/test-virtio-dmabuf.c
+++ b/tests/unit/test-virtio-dmabuf.c
@@ -27,6 +27,7 @@ static void test_add_remove_resources(void)
QemuUUID uuid;
int i, dmabuf_fd;
+ virtio_dmabuf_init();
for (i = 0; i < 100; ++i) {
qemu_uuid_generate(&uuid);
dmabuf_fd = g_random_int_range(3, 500);
@@ -46,6 +47,7 @@ static void test_add_remove_dev(void)
struct vhost_dev *dev = g_new0(struct vhost_dev, 1);
int i;
+ virtio_dmabuf_init();
for (i = 0; i < 100; ++i) {
qemu_uuid_generate(&uuid);
virtio_add_vhost_device(&uuid, dev);
@@ -64,6 +66,7 @@ static void test_remove_invalid_resource(void)
QemuUUID uuid;
int i;
+ virtio_dmabuf_init();
for (i = 0; i < 20; ++i) {
qemu_uuid_generate(&uuid);
g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, -1);
@@ -78,6 +81,7 @@ static void test_add_invalid_resource(void)
struct vhost_dev *dev = NULL;
int i, dmabuf_fd = -2, alt_dmabuf = 2;
+ virtio_dmabuf_init();
for (i = 0; i < 20; ++i) {
qemu_uuid_generate(&uuid);
/* Add a new resource with invalid (negative) resource fd */
@@ -108,6 +112,7 @@ static void test_free_resources(void)
QemuUUID uuids[20];
int i, dmabuf_fd;
+ virtio_dmabuf_init();
for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
qemu_uuid_generate(&uuids[i]);
dmabuf_fd = g_random_int_range(3, 500);
--
2.43.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v4 3/5] hw/virtio: change dmabuf mutex to QemuMutex
2024-02-19 14:34 ` [PATCH v4 3/5] hw/virtio: change dmabuf mutex to QemuMutex Albert Esteve
@ 2024-02-20 10:34 ` Manos Pitsidianakis
2024-03-11 13:31 ` Albert Esteve
0 siblings, 1 reply; 15+ messages in thread
From: Manos Pitsidianakis @ 2024-02-20 10:34 UTC (permalink / raw)
To: qemu-devel, Albert Esteve
Cc: Michael S. Tsirkin, stefanha, alex.bennee, philmd, kraxel,
marcandre.lureau, Albert Esteve
Hello Albert,
This is a point of confusion for me; Volker recently pointed out in a
patch for virtio-snd that all its code runs under the BQL. Is this code
ever called without BQL, for example do the backend read/write functions
from vhost-user.c run without the BQL?
On Mon, 19 Feb 2024 16:34, Albert Esteve <aesteve@redhat.com> wrote:
>Change GMutex by QemuMutex to be able to use
>lock contexts with `WITH_QEMU_LOCK_GUARD`.
>
>As the lock needs to be initialised and there
>is no central point for initialisation, add
>an init public function and call it from
>virtio.c, each time a new backend structure
>is initialised.
>
>Signed-off-by: Albert Esteve <aesteve@redhat.com>
>---
> hw/display/virtio-dmabuf.c | 55 +++++++++++++++++--------------
> hw/virtio/virtio.c | 3 ++
> include/hw/virtio/virtio-dmabuf.h | 5 +++
> tests/unit/test-virtio-dmabuf.c | 5 +++
> 4 files changed, 43 insertions(+), 25 deletions(-)
>
>diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
>index 497cb6fa7c..961094a561 100644
>--- a/hw/display/virtio-dmabuf.c
>+++ b/hw/display/virtio-dmabuf.c
>@@ -11,11 +11,12 @@
> */
>
> #include "qemu/osdep.h"
>+#include "include/qemu/lockable.h"
>
> #include "hw/virtio/virtio-dmabuf.h"
>
>
>-static GMutex lock;
>+static QemuMutex lock;
> static GHashTable *resource_uuids;
>
> /*
>@@ -27,23 +28,27 @@ static int uuid_equal_func(const void *lhv, const void *rhv)
> return qemu_uuid_is_equal(lhv, rhv);
> }
>
>+void virtio_dmabuf_init(void) {
>+ qemu_mutex_init(&lock);
>+}
>+
> static bool virtio_add_resource(QemuUUID *uuid, VirtioSharedObject *value)
> {
> bool result = true;
>
>- g_mutex_lock(&lock);
>- if (resource_uuids == NULL) {
>- resource_uuids = g_hash_table_new_full(qemu_uuid_hash,
>- uuid_equal_func,
>- NULL,
>- g_free);
>- }
>- if (g_hash_table_lookup(resource_uuids, uuid) == NULL) {
>- g_hash_table_insert(resource_uuids, uuid, value);
>- } else {
>- result = false;
>+ WITH_QEMU_LOCK_GUARD(&lock) {
>+ if (resource_uuids == NULL) {
>+ resource_uuids = g_hash_table_new_full(qemu_uuid_hash,
>+ uuid_equal_func,
>+ NULL,
>+ g_free);
>+ }
>+ if (g_hash_table_lookup(resource_uuids, uuid) == NULL) {
>+ g_hash_table_insert(resource_uuids, uuid, value);
>+ } else {
>+ result = false;
>+ }
> }
>- g_mutex_unlock(&lock);
>
> return result;
> }
>@@ -87,9 +92,9 @@ bool virtio_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev)
> 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);
>+ WITH_QEMU_LOCK_GUARD(&lock) {
>+ result = g_hash_table_remove(resource_uuids, uuid);
>+ }
>
> return result;
> }
>@@ -98,11 +103,11 @@ static VirtioSharedObject *get_shared_object(const QemuUUID *uuid)
> {
> gpointer lookup_res = NULL;
>
>- g_mutex_lock(&lock);
>- if (resource_uuids != NULL) {
>- lookup_res = g_hash_table_lookup(resource_uuids, uuid);
>+ WITH_QEMU_LOCK_GUARD(&lock) {
>+ if (resource_uuids != NULL) {
>+ lookup_res = g_hash_table_lookup(resource_uuids, uuid);
>+ }
> }
>- g_mutex_unlock(&lock);
>
> return (VirtioSharedObject *) lookup_res;
> }
>@@ -138,9 +143,9 @@ SharedObjectType virtio_object_type(const QemuUUID *uuid)
>
> void virtio_free_resources(void)
> {
>- g_mutex_lock(&lock);
>- g_hash_table_destroy(resource_uuids);
>- /* Reference count shall be 0 after the implicit unref on destroy */
>- resource_uuids = NULL;
>- g_mutex_unlock(&lock);
>+ WITH_QEMU_LOCK_GUARD(&lock) {
>+ g_hash_table_destroy(resource_uuids);
>+ /* Reference count shall be 0 after the implicit unref on destroy */
>+ resource_uuids = NULL;
>+ }
> }
>diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>index d229755eae..88189e7178 100644
>--- a/hw/virtio/virtio.c
>+++ b/hw/virtio/virtio.c
>@@ -29,6 +29,7 @@
> #include "hw/virtio/virtio-bus.h"
> #include "hw/qdev-properties.h"
> #include "hw/virtio/virtio-access.h"
>+#include "hw/virtio/virtio-dmabuf.h"
> #include "sysemu/dma.h"
> #include "sysemu/runstate.h"
> #include "virtio-qmp.h"
>@@ -3221,6 +3222,8 @@ void virtio_init(VirtIODevice *vdev, uint16_t device_id, size_t config_size)
> int i;
> int nvectors = k->query_nvectors ? k->query_nvectors(qbus->parent) : 0;
>
>+ // Ensure virtio dmabuf table is initialised.
>+ virtio_dmabuf_init();
> if (nvectors) {
> vdev->vector_queues =
> g_malloc0(sizeof(*vdev->vector_queues) * nvectors);
>diff --git a/include/hw/virtio/virtio-dmabuf.h b/include/hw/virtio/virtio-dmabuf.h
>index 891a43162d..627d84dce9 100644
>--- a/include/hw/virtio/virtio-dmabuf.h
>+++ b/include/hw/virtio/virtio-dmabuf.h
>@@ -50,6 +50,11 @@ typedef struct VirtioSharedObject {
> } value;
> } VirtioSharedObject;
>
>+/**
>+ * virtio_dmabuf_init() - Initialise virtio dmabuf internal structures.
>+ */
>+void virtio_dmabuf_init(void);
>+
> /**
> * virtio_add_dmabuf() - Add a new dma-buf resource to the lookup table
> * @uuid: new resource's UUID
>diff --git a/tests/unit/test-virtio-dmabuf.c b/tests/unit/test-virtio-dmabuf.c
>index a45ec52f42..20213455ee 100644
>--- a/tests/unit/test-virtio-dmabuf.c
>+++ b/tests/unit/test-virtio-dmabuf.c
>@@ -27,6 +27,7 @@ static void test_add_remove_resources(void)
> QemuUUID uuid;
> int i, dmabuf_fd;
>
>+ virtio_dmabuf_init();
> for (i = 0; i < 100; ++i) {
> qemu_uuid_generate(&uuid);
> dmabuf_fd = g_random_int_range(3, 500);
>@@ -46,6 +47,7 @@ static void test_add_remove_dev(void)
> struct vhost_dev *dev = g_new0(struct vhost_dev, 1);
> int i;
>
>+ virtio_dmabuf_init();
> for (i = 0; i < 100; ++i) {
> qemu_uuid_generate(&uuid);
> virtio_add_vhost_device(&uuid, dev);
>@@ -64,6 +66,7 @@ static void test_remove_invalid_resource(void)
> QemuUUID uuid;
> int i;
>
>+ virtio_dmabuf_init();
> for (i = 0; i < 20; ++i) {
> qemu_uuid_generate(&uuid);
> g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, -1);
>@@ -78,6 +81,7 @@ static void test_add_invalid_resource(void)
> struct vhost_dev *dev = NULL;
> int i, dmabuf_fd = -2, alt_dmabuf = 2;
>
>+ virtio_dmabuf_init();
> for (i = 0; i < 20; ++i) {
> qemu_uuid_generate(&uuid);
> /* Add a new resource with invalid (negative) resource fd */
>@@ -108,6 +112,7 @@ static void test_free_resources(void)
> QemuUUID uuids[20];
> int i, dmabuf_fd;
>
>+ virtio_dmabuf_init();
> for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
> qemu_uuid_generate(&uuids[i]);
> dmabuf_fd = g_random_int_range(3, 500);
>--
>2.43.1
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 3/5] hw/virtio: change dmabuf mutex to QemuMutex
2024-02-20 10:34 ` Manos Pitsidianakis
@ 2024-03-11 13:31 ` Albert Esteve
2024-03-12 20:47 ` Alex Bennée
0 siblings, 1 reply; 15+ messages in thread
From: Albert Esteve @ 2024-03-11 13:31 UTC (permalink / raw)
To: Manos Pitsidianakis
Cc: qemu-devel, Michael S. Tsirkin, stefanha, alex.bennee, philmd,
kraxel, marcandre.lureau
[-- Attachment #1: Type: text/plain, Size: 8685 bytes --]
On Tue, Feb 20, 2024 at 11:39 AM Manos Pitsidianakis <
manos.pitsidianakis@linaro.org> wrote:
> Hello Albert,
>
> This is a point of confusion for me; Volker recently pointed out in a
> patch for virtio-snd that all its code runs under the BQL.
Hello Manos,
I updated it to QemuMutex after a suggestion from Alex Benee, but I was not
really aware it existed before his comment. So for your question I had to
check what
exactly BQL stands for in this context (big QEMU lock). Therefore, as you
can see,
I am probably not the right person to answer it.
> Is this code
> ever called without BQL, for example do the backend read/write functions
> from vhost-user.c run without the BQL?
>
To my understanding, they should as every access to the shared table may
incur
in a race. But I'd need to read the code better to verify if that is
indeed the case.
The only thing I can say is that, if this change is confusing or may lead
to issues
related to the scope of the lock, it may be better to dismiss the change and
split it to its own specific patch, so I have the chance to verify the
chance in
a better way without delaying the other commits here.
>
> On Mon, 19 Feb 2024 16:34, Albert Esteve <aesteve@redhat.com> wrote:
> >Change GMutex by QemuMutex to be able to use
> >lock contexts with `WITH_QEMU_LOCK_GUARD`.
> >
> >As the lock needs to be initialised and there
> >is no central point for initialisation, add
> >an init public function and call it from
> >virtio.c, each time a new backend structure
> >is initialised.
> >
> >Signed-off-by: Albert Esteve <aesteve@redhat.com>
> >---
> > hw/display/virtio-dmabuf.c | 55 +++++++++++++++++--------------
> > hw/virtio/virtio.c | 3 ++
> > include/hw/virtio/virtio-dmabuf.h | 5 +++
> > tests/unit/test-virtio-dmabuf.c | 5 +++
> > 4 files changed, 43 insertions(+), 25 deletions(-)
> >
> >diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
> >index 497cb6fa7c..961094a561 100644
> >--- a/hw/display/virtio-dmabuf.c
> >+++ b/hw/display/virtio-dmabuf.c
> >@@ -11,11 +11,12 @@
> > */
> >
> > #include "qemu/osdep.h"
> >+#include "include/qemu/lockable.h"
> >
> > #include "hw/virtio/virtio-dmabuf.h"
> >
> >
> >-static GMutex lock;
> >+static QemuMutex lock;
> > static GHashTable *resource_uuids;
> >
> > /*
> >@@ -27,23 +28,27 @@ static int uuid_equal_func(const void *lhv, const
> void *rhv)
> > return qemu_uuid_is_equal(lhv, rhv);
> > }
> >
> >+void virtio_dmabuf_init(void) {
> >+ qemu_mutex_init(&lock);
> >+}
> >+
> > static bool virtio_add_resource(QemuUUID *uuid, VirtioSharedObject
> *value)
> > {
> > bool result = true;
> >
> >- g_mutex_lock(&lock);
> >- if (resource_uuids == NULL) {
> >- resource_uuids = g_hash_table_new_full(qemu_uuid_hash,
> >- uuid_equal_func,
> >- NULL,
> >- g_free);
> >- }
> >- if (g_hash_table_lookup(resource_uuids, uuid) == NULL) {
> >- g_hash_table_insert(resource_uuids, uuid, value);
> >- } else {
> >- result = false;
> >+ WITH_QEMU_LOCK_GUARD(&lock) {
> >+ if (resource_uuids == NULL) {
> >+ resource_uuids = g_hash_table_new_full(qemu_uuid_hash,
> >+ uuid_equal_func,
> >+ NULL,
> >+ g_free);
> >+ }
> >+ if (g_hash_table_lookup(resource_uuids, uuid) == NULL) {
> >+ g_hash_table_insert(resource_uuids, uuid, value);
> >+ } else {
> >+ result = false;
> >+ }
> > }
> >- g_mutex_unlock(&lock);
> >
> > return result;
> > }
> >@@ -87,9 +92,9 @@ bool virtio_add_vhost_device(QemuUUID *uuid, struct
> vhost_dev *dev)
> > 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);
> >+ WITH_QEMU_LOCK_GUARD(&lock) {
> >+ result = g_hash_table_remove(resource_uuids, uuid);
> >+ }
> >
> > return result;
> > }
> >@@ -98,11 +103,11 @@ static VirtioSharedObject *get_shared_object(const
> QemuUUID *uuid)
> > {
> > gpointer lookup_res = NULL;
> >
> >- g_mutex_lock(&lock);
> >- if (resource_uuids != NULL) {
> >- lookup_res = g_hash_table_lookup(resource_uuids, uuid);
> >+ WITH_QEMU_LOCK_GUARD(&lock) {
> >+ if (resource_uuids != NULL) {
> >+ lookup_res = g_hash_table_lookup(resource_uuids, uuid);
> >+ }
> > }
> >- g_mutex_unlock(&lock);
> >
> > return (VirtioSharedObject *) lookup_res;
> > }
> >@@ -138,9 +143,9 @@ SharedObjectType virtio_object_type(const QemuUUID
> *uuid)
> >
> > void virtio_free_resources(void)
> > {
> >- g_mutex_lock(&lock);
> >- g_hash_table_destroy(resource_uuids);
> >- /* Reference count shall be 0 after the implicit unref on destroy */
> >- resource_uuids = NULL;
> >- g_mutex_unlock(&lock);
> >+ WITH_QEMU_LOCK_GUARD(&lock) {
> >+ g_hash_table_destroy(resource_uuids);
> >+ /* Reference count shall be 0 after the implicit unref on
> destroy */
> >+ resource_uuids = NULL;
> >+ }
> > }
> >diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >index d229755eae..88189e7178 100644
> >--- a/hw/virtio/virtio.c
> >+++ b/hw/virtio/virtio.c
> >@@ -29,6 +29,7 @@
> > #include "hw/virtio/virtio-bus.h"
> > #include "hw/qdev-properties.h"
> > #include "hw/virtio/virtio-access.h"
> >+#include "hw/virtio/virtio-dmabuf.h"
> > #include "sysemu/dma.h"
> > #include "sysemu/runstate.h"
> > #include "virtio-qmp.h"
> >@@ -3221,6 +3222,8 @@ void virtio_init(VirtIODevice *vdev, uint16_t
> device_id, size_t config_size)
> > int i;
> > int nvectors = k->query_nvectors ? k->query_nvectors(qbus->parent) :
> 0;
> >
> >+ // Ensure virtio dmabuf table is initialised.
> >+ virtio_dmabuf_init();
> > if (nvectors) {
> > vdev->vector_queues =
> > g_malloc0(sizeof(*vdev->vector_queues) * nvectors);
> >diff --git a/include/hw/virtio/virtio-dmabuf.h
> b/include/hw/virtio/virtio-dmabuf.h
> >index 891a43162d..627d84dce9 100644
> >--- a/include/hw/virtio/virtio-dmabuf.h
> >+++ b/include/hw/virtio/virtio-dmabuf.h
> >@@ -50,6 +50,11 @@ typedef struct VirtioSharedObject {
> > } value;
> > } VirtioSharedObject;
> >
> >+/**
> >+ * virtio_dmabuf_init() - Initialise virtio dmabuf internal structures.
> >+ */
> >+void virtio_dmabuf_init(void);
> >+
> > /**
> > * virtio_add_dmabuf() - Add a new dma-buf resource to the lookup table
> > * @uuid: new resource's UUID
> >diff --git a/tests/unit/test-virtio-dmabuf.c
> b/tests/unit/test-virtio-dmabuf.c
> >index a45ec52f42..20213455ee 100644
> >--- a/tests/unit/test-virtio-dmabuf.c
> >+++ b/tests/unit/test-virtio-dmabuf.c
> >@@ -27,6 +27,7 @@ static void test_add_remove_resources(void)
> > QemuUUID uuid;
> > int i, dmabuf_fd;
> >
> >+ virtio_dmabuf_init();
> > for (i = 0; i < 100; ++i) {
> > qemu_uuid_generate(&uuid);
> > dmabuf_fd = g_random_int_range(3, 500);
> >@@ -46,6 +47,7 @@ static void test_add_remove_dev(void)
> > struct vhost_dev *dev = g_new0(struct vhost_dev, 1);
> > int i;
> >
> >+ virtio_dmabuf_init();
> > for (i = 0; i < 100; ++i) {
> > qemu_uuid_generate(&uuid);
> > virtio_add_vhost_device(&uuid, dev);
> >@@ -64,6 +66,7 @@ static void test_remove_invalid_resource(void)
> > QemuUUID uuid;
> > int i;
> >
> >+ virtio_dmabuf_init();
> > for (i = 0; i < 20; ++i) {
> > qemu_uuid_generate(&uuid);
> > g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, -1);
> >@@ -78,6 +81,7 @@ static void test_add_invalid_resource(void)
> > struct vhost_dev *dev = NULL;
> > int i, dmabuf_fd = -2, alt_dmabuf = 2;
> >
> >+ virtio_dmabuf_init();
> > for (i = 0; i < 20; ++i) {
> > qemu_uuid_generate(&uuid);
> > /* Add a new resource with invalid (negative) resource fd */
> >@@ -108,6 +112,7 @@ static void test_free_resources(void)
> > QemuUUID uuids[20];
> > int i, dmabuf_fd;
> >
> >+ virtio_dmabuf_init();
> > for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
> > qemu_uuid_generate(&uuids[i]);
> > dmabuf_fd = g_random_int_range(3, 500);
> >--
> >2.43.1
> >
> >
>
>
[-- Attachment #2: Type: text/html, Size: 11341 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 3/5] hw/virtio: change dmabuf mutex to QemuMutex
2024-03-11 13:31 ` Albert Esteve
@ 2024-03-12 20:47 ` Alex Bennée
0 siblings, 0 replies; 15+ messages in thread
From: Alex Bennée @ 2024-03-12 20:47 UTC (permalink / raw)
To: Albert Esteve
Cc: Manos Pitsidianakis, qemu-devel, Michael S. Tsirkin, stefanha,
philmd, kraxel, marcandre.lureau
Albert Esteve <aesteve@redhat.com> writes:
> On Tue, Feb 20, 2024 at 11:39 AM Manos Pitsidianakis <manos.pitsidianakis@linaro.org> wrote:
>
> Hello Albert,
>
> This is a point of confusion for me; Volker recently pointed out in a
> patch for virtio-snd that all its code runs under the BQL.
>
> Hello Manos,
>
> I updated it to QemuMutex after a suggestion from Alex Benee, but I was not
> really aware it existed before his comment. So for your question I had to check what
> exactly BQL stands for in this context (big QEMU lock). Therefore, as you can see,
> I am probably not the right person to answer it.
>
> Is this code
> ever called without BQL, for example do the backend read/write functions
> from vhost-user.c run without the BQL?
>
> To my understanding, they should as every access to the shared table may incur
> in a race. But I'd need to read the code better to verify if that is
> indeed the case.
The BQL will be held for any MMIO access but that is not all the
accesses that happen with VirtIO. Access to the shared buffers is
controlled by protocol and certain accesses need to be atomic.
The question is are all these functions triggered by MMIO operations or
by other events?
>
> The only thing I can say is that, if this change is confusing or may lead to issues
> related to the scope of the lock, it may be better to dismiss the change and
> split it to its own specific patch, so I have the chance to verify the chance in
> a better way without delaying the other commits here.
>
>
> On Mon, 19 Feb 2024 16:34, Albert Esteve <aesteve@redhat.com> wrote:
> >Change GMutex by QemuMutex to be able to use
> >lock contexts with `WITH_QEMU_LOCK_GUARD`.
> >
> >As the lock needs to be initialised and there
> >is no central point for initialisation, add
> >an init public function and call it from
> >virtio.c, each time a new backend structure
> >is initialised.
> >
> >Signed-off-by: Albert Esteve <aesteve@redhat.com>
> >---
> > hw/display/virtio-dmabuf.c | 55 +++++++++++++++++--------------
> > hw/virtio/virtio.c | 3 ++
> > include/hw/virtio/virtio-dmabuf.h | 5 +++
> > tests/unit/test-virtio-dmabuf.c | 5 +++
> > 4 files changed, 43 insertions(+), 25 deletions(-)
> >
> >diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
> >index 497cb6fa7c..961094a561 100644
> >--- a/hw/display/virtio-dmabuf.c
> >+++ b/hw/display/virtio-dmabuf.c
> >@@ -11,11 +11,12 @@
> > */
> >
> > #include "qemu/osdep.h"
> >+#include "include/qemu/lockable.h"
> >
> > #include "hw/virtio/virtio-dmabuf.h"
> >
> >
> >-static GMutex lock;
> >+static QemuMutex lock;
> > static GHashTable *resource_uuids;
> >
> > /*
> >@@ -27,23 +28,27 @@ static int uuid_equal_func(const void *lhv, const void *rhv)
> > return qemu_uuid_is_equal(lhv, rhv);
> > }
> >
> >+void virtio_dmabuf_init(void) {
> >+ qemu_mutex_init(&lock);
> >+}
> >+
> > static bool virtio_add_resource(QemuUUID *uuid, VirtioSharedObject *value)
> > {
> > bool result = true;
> >
> >- g_mutex_lock(&lock);
> >- if (resource_uuids == NULL) {
> >- resource_uuids = g_hash_table_new_full(qemu_uuid_hash,
> >- uuid_equal_func,
> >- NULL,
> >- g_free);
> >- }
> >- if (g_hash_table_lookup(resource_uuids, uuid) == NULL) {
> >- g_hash_table_insert(resource_uuids, uuid, value);
> >- } else {
> >- result = false;
> >+ WITH_QEMU_LOCK_GUARD(&lock) {
> >+ if (resource_uuids == NULL) {
> >+ resource_uuids = g_hash_table_new_full(qemu_uuid_hash,
> >+ uuid_equal_func,
> >+ NULL,
> >+ g_free);
> >+ }
> >+ if (g_hash_table_lookup(resource_uuids, uuid) == NULL) {
> >+ g_hash_table_insert(resource_uuids, uuid, value);
> >+ } else {
> >+ result = false;
> >+ }
> > }
> >- g_mutex_unlock(&lock);
> >
> > return result;
> > }
> >@@ -87,9 +92,9 @@ bool virtio_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev)
> > 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);
> >+ WITH_QEMU_LOCK_GUARD(&lock) {
> >+ result = g_hash_table_remove(resource_uuids, uuid);
> >+ }
> >
> > return result;
> > }
> >@@ -98,11 +103,11 @@ static VirtioSharedObject *get_shared_object(const QemuUUID *uuid)
> > {
> > gpointer lookup_res = NULL;
> >
> >- g_mutex_lock(&lock);
> >- if (resource_uuids != NULL) {
> >- lookup_res = g_hash_table_lookup(resource_uuids, uuid);
> >+ WITH_QEMU_LOCK_GUARD(&lock) {
> >+ if (resource_uuids != NULL) {
> >+ lookup_res = g_hash_table_lookup(resource_uuids, uuid);
> >+ }
> > }
> >- g_mutex_unlock(&lock);
> >
> > return (VirtioSharedObject *) lookup_res;
> > }
> >@@ -138,9 +143,9 @@ SharedObjectType virtio_object_type(const QemuUUID *uuid)
> >
> > void virtio_free_resources(void)
> > {
> >- g_mutex_lock(&lock);
> >- g_hash_table_destroy(resource_uuids);
> >- /* Reference count shall be 0 after the implicit unref on destroy */
> >- resource_uuids = NULL;
> >- g_mutex_unlock(&lock);
> >+ WITH_QEMU_LOCK_GUARD(&lock) {
> >+ g_hash_table_destroy(resource_uuids);
> >+ /* Reference count shall be 0 after the implicit unref on destroy */
> >+ resource_uuids = NULL;
> >+ }
> > }
> >diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >index d229755eae..88189e7178 100644
> >--- a/hw/virtio/virtio.c
> >+++ b/hw/virtio/virtio.c
> >@@ -29,6 +29,7 @@
> > #include "hw/virtio/virtio-bus.h"
> > #include "hw/qdev-properties.h"
> > #include "hw/virtio/virtio-access.h"
> >+#include "hw/virtio/virtio-dmabuf.h"
> > #include "sysemu/dma.h"
> > #include "sysemu/runstate.h"
> > #include "virtio-qmp.h"
> >@@ -3221,6 +3222,8 @@ void virtio_init(VirtIODevice *vdev, uint16_t device_id, size_t config_size)
> > int i;
> > int nvectors = k->query_nvectors ? k->query_nvectors(qbus->parent) : 0;
> >
> >+ // Ensure virtio dmabuf table is initialised.
> >+ virtio_dmabuf_init();
> > if (nvectors) {
> > vdev->vector_queues =
> > g_malloc0(sizeof(*vdev->vector_queues) * nvectors);
> >diff --git a/include/hw/virtio/virtio-dmabuf.h b/include/hw/virtio/virtio-dmabuf.h
> >index 891a43162d..627d84dce9 100644
> >--- a/include/hw/virtio/virtio-dmabuf.h
> >+++ b/include/hw/virtio/virtio-dmabuf.h
> >@@ -50,6 +50,11 @@ typedef struct VirtioSharedObject {
> > } value;
> > } VirtioSharedObject;
> >
> >+/**
> >+ * virtio_dmabuf_init() - Initialise virtio dmabuf internal structures.
> >+ */
> >+void virtio_dmabuf_init(void);
> >+
> > /**
> > * virtio_add_dmabuf() - Add a new dma-buf resource to the lookup table
> > * @uuid: new resource's UUID
> >diff --git a/tests/unit/test-virtio-dmabuf.c b/tests/unit/test-virtio-dmabuf.c
> >index a45ec52f42..20213455ee 100644
> >--- a/tests/unit/test-virtio-dmabuf.c
> >+++ b/tests/unit/test-virtio-dmabuf.c
> >@@ -27,6 +27,7 @@ static void test_add_remove_resources(void)
> > QemuUUID uuid;
> > int i, dmabuf_fd;
> >
> >+ virtio_dmabuf_init();
> > for (i = 0; i < 100; ++i) {
> > qemu_uuid_generate(&uuid);
> > dmabuf_fd = g_random_int_range(3, 500);
> >@@ -46,6 +47,7 @@ static void test_add_remove_dev(void)
> > struct vhost_dev *dev = g_new0(struct vhost_dev, 1);
> > int i;
> >
> >+ virtio_dmabuf_init();
> > for (i = 0; i < 100; ++i) {
> > qemu_uuid_generate(&uuid);
> > virtio_add_vhost_device(&uuid, dev);
> >@@ -64,6 +66,7 @@ static void test_remove_invalid_resource(void)
> > QemuUUID uuid;
> > int i;
> >
> >+ virtio_dmabuf_init();
> > for (i = 0; i < 20; ++i) {
> > qemu_uuid_generate(&uuid);
> > g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, -1);
> >@@ -78,6 +81,7 @@ static void test_add_invalid_resource(void)
> > struct vhost_dev *dev = NULL;
> > int i, dmabuf_fd = -2, alt_dmabuf = 2;
> >
> >+ virtio_dmabuf_init();
> > for (i = 0; i < 20; ++i) {
> > qemu_uuid_generate(&uuid);
> > /* Add a new resource with invalid (negative) resource fd */
> >@@ -108,6 +112,7 @@ static void test_free_resources(void)
> > QemuUUID uuids[20];
> > int i, dmabuf_fd;
> >
> >+ virtio_dmabuf_init();
> > for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
> > qemu_uuid_generate(&uuids[i]);
> > dmabuf_fd = g_random_int_range(3, 500);
> >--
> >2.43.1
> >
> >
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 4/5] hw/virtio: cleanup shared resources
2024-02-19 14:34 [PATCH v4 0/5] Virtio dmabuf improvements Albert Esteve
` (2 preceding siblings ...)
2024-02-19 14:34 ` [PATCH v4 3/5] hw/virtio: change dmabuf mutex to QemuMutex Albert Esteve
@ 2024-02-19 14:34 ` Albert Esteve
2024-02-20 10:39 ` Manos Pitsidianakis
2024-03-12 18:13 ` Michael S. Tsirkin
2024-02-19 14:34 ` [PATCH v4 5/5] hw/virtio: rename virtio dmabuf API Albert Esteve
2024-03-12 18:23 ` [PATCH v4 0/5] Virtio dmabuf improvements Michael S. Tsirkin
5 siblings, 2 replies; 15+ messages in thread
From: Albert Esteve @ 2024-02-19 14:34 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, stefanha, alex.bennee, philmd, kraxel,
marcandre.lureau, Albert Esteve, Stefan Hajnoczi
Ensure that we cleanup all virtio shared
resources when the vhost devices is cleaned
up (after a hot unplug, or a crash).
To do so, we add a new function to the virtio_dmabuf
API called `virtio_dmabuf_vhost_cleanup`, which
loop through the table and removes all
resources owned by the vhost device parameter.
Also, add a test to verify that the new
function in the API behaves as expected.
Signed-off-by: Albert Esteve <aesteve@redhat.com>
Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
---
hw/display/virtio-dmabuf.c | 21 ++++++++++++++++++++
hw/virtio/vhost.c | 3 +++
include/hw/virtio/virtio-dmabuf.h | 10 ++++++++++
tests/unit/test-virtio-dmabuf.c | 33 +++++++++++++++++++++++++++++++
4 files changed, 67 insertions(+)
diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
index 961094a561..703b5bd979 100644
--- a/hw/display/virtio-dmabuf.c
+++ b/hw/display/virtio-dmabuf.c
@@ -141,6 +141,27 @@ SharedObjectType virtio_object_type(const QemuUUID *uuid)
return vso->type;
}
+static bool virtio_dmabuf_resource_is_owned(gpointer key,
+ gpointer value,
+ gpointer dev)
+{
+ VirtioSharedObject *vso;
+
+ vso = (VirtioSharedObject *) value;
+ return vso->type == TYPE_VHOST_DEV && vso->value.dev == dev;
+}
+
+int virtio_dmabuf_vhost_cleanup(struct vhost_dev *dev)
+{
+ int num_removed;
+
+ WITH_QEMU_LOCK_GUARD(&lock) {
+ num_removed = g_hash_table_foreach_remove(
+ resource_uuids, (GHRFunc) virtio_dmabuf_resource_is_owned, dev);
+ }
+ return num_removed;
+}
+
void virtio_free_resources(void)
{
WITH_QEMU_LOCK_GUARD(&lock) {
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 2c9ac79468..c5622eac14 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -16,6 +16,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
#include "hw/virtio/vhost.h"
+#include "hw/virtio/virtio-dmabuf.h"
#include "qemu/atomic.h"
#include "qemu/range.h"
#include "qemu/error-report.h"
@@ -1599,6 +1600,8 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
migrate_del_blocker(&hdev->migration_blocker);
g_free(hdev->mem);
g_free(hdev->mem_sections);
+ /* free virtio shared objects */
+ virtio_dmabuf_vhost_cleanup(hdev);
if (hdev->vhost_ops) {
hdev->vhost_ops->vhost_backend_cleanup(hdev);
}
diff --git a/include/hw/virtio/virtio-dmabuf.h b/include/hw/virtio/virtio-dmabuf.h
index 627d84dce9..950cd24967 100644
--- a/include/hw/virtio/virtio-dmabuf.h
+++ b/include/hw/virtio/virtio-dmabuf.h
@@ -119,6 +119,16 @@ struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid);
*/
SharedObjectType virtio_object_type(const QemuUUID *uuid);
+/**
+ * virtio_dmabuf_vhost_cleanup() - Destroys all entries of the shared
+ * resources lookup table that are owned by the vhost backend
+ * @dev: the pointer to the vhost device that owns the entries. Data is owned
+ * by the called of the function.
+ *
+ * Return: the number of resource entries removed.
+ */
+int virtio_dmabuf_vhost_cleanup(struct vhost_dev *dev);
+
/**
* virtio_free_resources() - Destroys all keys and values of the shared
* resources lookup table, and frees them
diff --git a/tests/unit/test-virtio-dmabuf.c b/tests/unit/test-virtio-dmabuf.c
index 20213455ee..e5cf7ee19f 100644
--- a/tests/unit/test-virtio-dmabuf.c
+++ b/tests/unit/test-virtio-dmabuf.c
@@ -107,6 +107,38 @@ static void test_add_invalid_resource(void)
}
}
+static void test_cleanup_res(void)
+{
+ QemuUUID uuids[20], uuid_alt;
+ struct vhost_dev *dev = g_new0(struct vhost_dev, 1);
+ struct vhost_dev *dev_alt = g_new0(struct vhost_dev, 1);
+ int i, num_removed;
+
+ for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
+ qemu_uuid_generate(&uuids[i]);
+ virtio_add_vhost_device(&uuids[i], dev);
+ /* vhost device is found */
+ g_assert(virtio_lookup_vhost_device(&uuids[i]) != NULL);
+ }
+ qemu_uuid_generate(&uuid_alt);
+ virtio_add_vhost_device(&uuid_alt, dev_alt);
+ /* vhost device is found */
+ g_assert(virtio_lookup_vhost_device(&uuid_alt) != NULL);
+ /* cleanup all dev resources */
+ num_removed = virtio_dmabuf_vhost_cleanup(dev);
+ g_assert_cmpint(num_removed, ==, ARRAY_SIZE(uuids));
+ for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
+ /* None of the dev resources is found after free'd */
+ g_assert_cmpint(virtio_lookup_dmabuf(&uuids[i]), ==, -1);
+ }
+ /* uuid_alt is still in the hash table */
+ g_assert(virtio_lookup_vhost_device(&uuid_alt) != NULL);
+
+ virtio_free_resources();
+ g_free(dev);
+ g_free(dev_alt);
+}
+
static void test_free_resources(void)
{
QemuUUID uuids[20];
@@ -136,6 +168,7 @@ int main(int argc, char **argv)
test_remove_invalid_resource);
g_test_add_func("/virtio-dmabuf/add_invalid_res",
test_add_invalid_resource);
+ g_test_add_func("/virtio-dmabuf/cleanup_dev", test_cleanup_res);
g_test_add_func("/virtio-dmabuf/free_res", test_free_resources);
return g_test_run();
--
2.43.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v4 4/5] hw/virtio: cleanup shared resources
2024-02-19 14:34 ` [PATCH v4 4/5] hw/virtio: cleanup shared resources Albert Esteve
@ 2024-02-20 10:39 ` Manos Pitsidianakis
2024-03-12 18:13 ` Michael S. Tsirkin
1 sibling, 0 replies; 15+ messages in thread
From: Manos Pitsidianakis @ 2024-02-20 10:39 UTC (permalink / raw)
To: qemu-devel, Albert Esteve
Cc: Michael S. Tsirkin, stefanha, alex.bennee, philmd, kraxel,
marcandre.lureau, Albert Esteve, Stefan Hajnoczi
On Mon, 19 Feb 2024 16:34, Albert Esteve <aesteve@redhat.com> wrote:
>Ensure that we cleanup all virtio shared
>resources when the vhost devices is cleaned
>up (after a hot unplug, or a crash).
>
>To do so, we add a new function to the virtio_dmabuf
>API called `virtio_dmabuf_vhost_cleanup`, which
>loop through the table and removes all
>resources owned by the vhost device parameter.
>
>Also, add a test to verify that the new
>function in the API behaves as expected.
>
>Signed-off-by: Albert Esteve <aesteve@redhat.com>
>Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
>---
Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> hw/display/virtio-dmabuf.c | 21 ++++++++++++++++++++
> hw/virtio/vhost.c | 3 +++
> include/hw/virtio/virtio-dmabuf.h | 10 ++++++++++
> tests/unit/test-virtio-dmabuf.c | 33 +++++++++++++++++++++++++++++++
> 4 files changed, 67 insertions(+)
>
>diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
>index 961094a561..703b5bd979 100644
>--- a/hw/display/virtio-dmabuf.c
>+++ b/hw/display/virtio-dmabuf.c
>@@ -141,6 +141,27 @@ SharedObjectType virtio_object_type(const QemuUUID *uuid)
> return vso->type;
> }
>
>+static bool virtio_dmabuf_resource_is_owned(gpointer key,
>+ gpointer value,
>+ gpointer dev)
>+{
>+ VirtioSharedObject *vso;
>+
>+ vso = (VirtioSharedObject *) value;
>+ return vso->type == TYPE_VHOST_DEV && vso->value.dev == dev;
>+}
>+
>+int virtio_dmabuf_vhost_cleanup(struct vhost_dev *dev)
>+{
>+ int num_removed;
>+
>+ WITH_QEMU_LOCK_GUARD(&lock) {
>+ num_removed = g_hash_table_foreach_remove(
>+ resource_uuids, (GHRFunc) virtio_dmabuf_resource_is_owned, dev);
>+ }
>+ return num_removed;
>+}
>+
> void virtio_free_resources(void)
> {
> WITH_QEMU_LOCK_GUARD(&lock) {
>diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>index 2c9ac79468..c5622eac14 100644
>--- a/hw/virtio/vhost.c
>+++ b/hw/virtio/vhost.c
>@@ -16,6 +16,7 @@
> #include "qemu/osdep.h"
> #include "qapi/error.h"
> #include "hw/virtio/vhost.h"
>+#include "hw/virtio/virtio-dmabuf.h"
> #include "qemu/atomic.h"
> #include "qemu/range.h"
> #include "qemu/error-report.h"
>@@ -1599,6 +1600,8 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
> migrate_del_blocker(&hdev->migration_blocker);
> g_free(hdev->mem);
> g_free(hdev->mem_sections);
>+ /* free virtio shared objects */
>+ virtio_dmabuf_vhost_cleanup(hdev);
> if (hdev->vhost_ops) {
> hdev->vhost_ops->vhost_backend_cleanup(hdev);
> }
>diff --git a/include/hw/virtio/virtio-dmabuf.h b/include/hw/virtio/virtio-dmabuf.h
>index 627d84dce9..950cd24967 100644
>--- a/include/hw/virtio/virtio-dmabuf.h
>+++ b/include/hw/virtio/virtio-dmabuf.h
>@@ -119,6 +119,16 @@ struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid);
> */
> SharedObjectType virtio_object_type(const QemuUUID *uuid);
>
>+/**
>+ * virtio_dmabuf_vhost_cleanup() - Destroys all entries of the shared
>+ * resources lookup table that are owned by the vhost backend
>+ * @dev: the pointer to the vhost device that owns the entries. Data is owned
>+ * by the called of the function.
>+ *
>+ * Return: the number of resource entries removed.
>+ */
>+int virtio_dmabuf_vhost_cleanup(struct vhost_dev *dev);
>+
> /**
> * virtio_free_resources() - Destroys all keys and values of the shared
> * resources lookup table, and frees them
>diff --git a/tests/unit/test-virtio-dmabuf.c b/tests/unit/test-virtio-dmabuf.c
>index 20213455ee..e5cf7ee19f 100644
>--- a/tests/unit/test-virtio-dmabuf.c
>+++ b/tests/unit/test-virtio-dmabuf.c
>@@ -107,6 +107,38 @@ static void test_add_invalid_resource(void)
> }
> }
>
>+static void test_cleanup_res(void)
>+{
>+ QemuUUID uuids[20], uuid_alt;
>+ struct vhost_dev *dev = g_new0(struct vhost_dev, 1);
>+ struct vhost_dev *dev_alt = g_new0(struct vhost_dev, 1);
>+ int i, num_removed;
>+
>+ for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
>+ qemu_uuid_generate(&uuids[i]);
>+ virtio_add_vhost_device(&uuids[i], dev);
>+ /* vhost device is found */
>+ g_assert(virtio_lookup_vhost_device(&uuids[i]) != NULL);
>+ }
>+ qemu_uuid_generate(&uuid_alt);
>+ virtio_add_vhost_device(&uuid_alt, dev_alt);
>+ /* vhost device is found */
>+ g_assert(virtio_lookup_vhost_device(&uuid_alt) != NULL);
>+ /* cleanup all dev resources */
>+ num_removed = virtio_dmabuf_vhost_cleanup(dev);
>+ g_assert_cmpint(num_removed, ==, ARRAY_SIZE(uuids));
>+ for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
>+ /* None of the dev resources is found after free'd */
>+ g_assert_cmpint(virtio_lookup_dmabuf(&uuids[i]), ==, -1);
>+ }
>+ /* uuid_alt is still in the hash table */
>+ g_assert(virtio_lookup_vhost_device(&uuid_alt) != NULL);
>+
>+ virtio_free_resources();
>+ g_free(dev);
>+ g_free(dev_alt);
>+}
>+
> static void test_free_resources(void)
> {
> QemuUUID uuids[20];
>@@ -136,6 +168,7 @@ int main(int argc, char **argv)
> test_remove_invalid_resource);
> g_test_add_func("/virtio-dmabuf/add_invalid_res",
> test_add_invalid_resource);
>+ g_test_add_func("/virtio-dmabuf/cleanup_dev", test_cleanup_res);
> g_test_add_func("/virtio-dmabuf/free_res", test_free_resources);
>
> return g_test_run();
>--
>2.43.1
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 4/5] hw/virtio: cleanup shared resources
2024-02-19 14:34 ` [PATCH v4 4/5] hw/virtio: cleanup shared resources Albert Esteve
2024-02-20 10:39 ` Manos Pitsidianakis
@ 2024-03-12 18:13 ` Michael S. Tsirkin
1 sibling, 0 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2024-03-12 18:13 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, stefanha, alex.bennee, philmd, kraxel,
marcandre.lureau, Stefan Hajnoczi
On Mon, Feb 19, 2024 at 03:34:22PM +0100, Albert Esteve wrote:
> Ensure that we cleanup all virtio shared
> resources when the vhost devices is cleaned
> up (after a hot unplug, or a crash).
>
> To do so, we add a new function to the virtio_dmabuf
> API called `virtio_dmabuf_vhost_cleanup`, which
> loop through the table and removes all
> resources owned by the vhost device parameter.
>
> Also, add a test to verify that the new
> function in the API behaves as expected.
>
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> hw/display/virtio-dmabuf.c | 21 ++++++++++++++++++++
> hw/virtio/vhost.c | 3 +++
> include/hw/virtio/virtio-dmabuf.h | 10 ++++++++++
> tests/unit/test-virtio-dmabuf.c | 33 +++++++++++++++++++++++++++++++
> 4 files changed, 67 insertions(+)
>
> diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
> index 961094a561..703b5bd979 100644
> --- a/hw/display/virtio-dmabuf.c
> +++ b/hw/display/virtio-dmabuf.c
> @@ -141,6 +141,27 @@ SharedObjectType virtio_object_type(const QemuUUID *uuid)
> return vso->type;
> }
>
> +static bool virtio_dmabuf_resource_is_owned(gpointer key,
> + gpointer value,
> + gpointer dev)
> +{
> + VirtioSharedObject *vso;
> +
> + vso = (VirtioSharedObject *) value;
> + return vso->type == TYPE_VHOST_DEV && vso->value.dev == dev;
> +}
> +
> +int virtio_dmabuf_vhost_cleanup(struct vhost_dev *dev)
> +{
> + int num_removed;
> +
> + WITH_QEMU_LOCK_GUARD(&lock) {
If I don't apply previous patch I can not apply this one either.
> + num_removed = g_hash_table_foreach_remove(
> + resource_uuids, (GHRFunc) virtio_dmabuf_resource_is_owned, dev);
> + }
> + return num_removed;
> +}
> +
> void virtio_free_resources(void)
> {
> WITH_QEMU_LOCK_GUARD(&lock) {
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 2c9ac79468..c5622eac14 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -16,6 +16,7 @@
> #include "qemu/osdep.h"
> #include "qapi/error.h"
> #include "hw/virtio/vhost.h"
> +#include "hw/virtio/virtio-dmabuf.h"
> #include "qemu/atomic.h"
> #include "qemu/range.h"
> #include "qemu/error-report.h"
> @@ -1599,6 +1600,8 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
> migrate_del_blocker(&hdev->migration_blocker);
> g_free(hdev->mem);
> g_free(hdev->mem_sections);
> + /* free virtio shared objects */
> + virtio_dmabuf_vhost_cleanup(hdev);
> if (hdev->vhost_ops) {
> hdev->vhost_ops->vhost_backend_cleanup(hdev);
> }
> diff --git a/include/hw/virtio/virtio-dmabuf.h b/include/hw/virtio/virtio-dmabuf.h
> index 627d84dce9..950cd24967 100644
> --- a/include/hw/virtio/virtio-dmabuf.h
> +++ b/include/hw/virtio/virtio-dmabuf.h
> @@ -119,6 +119,16 @@ struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid);
> */
> SharedObjectType virtio_object_type(const QemuUUID *uuid);
>
> +/**
> + * virtio_dmabuf_vhost_cleanup() - Destroys all entries of the shared
> + * resources lookup table that are owned by the vhost backend
> + * @dev: the pointer to the vhost device that owns the entries. Data is owned
> + * by the called of the function.
> + *
trailing space here
> + * Return: the number of resource entries removed.
> + */
> +int virtio_dmabuf_vhost_cleanup(struct vhost_dev *dev);
> +
> /**
> * virtio_free_resources() - Destroys all keys and values of the shared
> * resources lookup table, and frees them
> diff --git a/tests/unit/test-virtio-dmabuf.c b/tests/unit/test-virtio-dmabuf.c
> index 20213455ee..e5cf7ee19f 100644
> --- a/tests/unit/test-virtio-dmabuf.c
> +++ b/tests/unit/test-virtio-dmabuf.c
> @@ -107,6 +107,38 @@ static void test_add_invalid_resource(void)
> }
> }
>
> +static void test_cleanup_res(void)
> +{
> + QemuUUID uuids[20], uuid_alt;
> + struct vhost_dev *dev = g_new0(struct vhost_dev, 1);
> + struct vhost_dev *dev_alt = g_new0(struct vhost_dev, 1);
> + int i, num_removed;
> +
> + for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
> + qemu_uuid_generate(&uuids[i]);
> + virtio_add_vhost_device(&uuids[i], dev);
> + /* vhost device is found */
> + g_assert(virtio_lookup_vhost_device(&uuids[i]) != NULL);
> + }
> + qemu_uuid_generate(&uuid_alt);
> + virtio_add_vhost_device(&uuid_alt, dev_alt);
> + /* vhost device is found */
> + g_assert(virtio_lookup_vhost_device(&uuid_alt) != NULL);
> + /* cleanup all dev resources */
> + num_removed = virtio_dmabuf_vhost_cleanup(dev);
> + g_assert_cmpint(num_removed, ==, ARRAY_SIZE(uuids));
> + for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
> + /* None of the dev resources is found after free'd */
> + g_assert_cmpint(virtio_lookup_dmabuf(&uuids[i]), ==, -1);
> + }
> + /* uuid_alt is still in the hash table */
> + g_assert(virtio_lookup_vhost_device(&uuid_alt) != NULL);
> +
> + virtio_free_resources();
> + g_free(dev);
> + g_free(dev_alt);
> +}
> +
> static void test_free_resources(void)
> {
> QemuUUID uuids[20];
> @@ -136,6 +168,7 @@ int main(int argc, char **argv)
> test_remove_invalid_resource);
> g_test_add_func("/virtio-dmabuf/add_invalid_res",
> test_add_invalid_resource);
> + g_test_add_func("/virtio-dmabuf/cleanup_dev", test_cleanup_res);
> g_test_add_func("/virtio-dmabuf/free_res", test_free_resources);
>
> return g_test_run();
> --
> 2.43.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 5/5] hw/virtio: rename virtio dmabuf API
2024-02-19 14:34 [PATCH v4 0/5] Virtio dmabuf improvements Albert Esteve
` (3 preceding siblings ...)
2024-02-19 14:34 ` [PATCH v4 4/5] hw/virtio: cleanup shared resources Albert Esteve
@ 2024-02-19 14:34 ` Albert Esteve
2024-03-12 18:23 ` [PATCH v4 0/5] Virtio dmabuf improvements Michael S. Tsirkin
5 siblings, 0 replies; 15+ messages in thread
From: Albert Esteve @ 2024-02-19 14:34 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, stefanha, alex.bennee, philmd, kraxel,
marcandre.lureau, Albert Esteve, Stefan Hajnoczi
Functions in the virtio-dmabuf module
start with 'virtio_*', which is too
generic and may not correctly identify
them as part of the virtio dmabuf API.
Rename all functions to 'virtio_dmabuf_*'
instead to avoid confusion.
Signed-off-by: Albert Esteve <aesteve@redhat.com>
Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
hw/display/virtio-dmabuf.c | 14 ++++----
hw/virtio/vhost-user.c | 14 ++++----
include/hw/virtio/virtio-dmabuf.h | 33 +++++++++---------
tests/unit/test-virtio-dmabuf.c | 58 +++++++++++++++----------------
4 files changed, 60 insertions(+), 59 deletions(-)
diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
index 703b5bd979..4e32af2ad0 100644
--- a/hw/display/virtio-dmabuf.c
+++ b/hw/display/virtio-dmabuf.c
@@ -53,7 +53,7 @@ static bool virtio_add_resource(QemuUUID *uuid, VirtioSharedObject *value)
return result;
}
-bool virtio_add_dmabuf(QemuUUID *uuid, int udmabuf_fd)
+bool virtio_dmabuf_add(QemuUUID *uuid, int udmabuf_fd)
{
bool result;
VirtioSharedObject *vso;
@@ -71,7 +71,7 @@ bool virtio_add_dmabuf(QemuUUID *uuid, int udmabuf_fd)
return result;
}
-bool virtio_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev)
+bool virtio_dmabuf_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev)
{
bool result;
VirtioSharedObject *vso;
@@ -89,7 +89,7 @@ bool virtio_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev)
return result;
}
-bool virtio_remove_resource(const QemuUUID *uuid)
+bool virtio_dmabuf_remove_resource(const QemuUUID *uuid)
{
bool result;
WITH_QEMU_LOCK_GUARD(&lock) {
@@ -112,7 +112,7 @@ static VirtioSharedObject *get_shared_object(const QemuUUID *uuid)
return (VirtioSharedObject *) lookup_res;
}
-int virtio_lookup_dmabuf(const QemuUUID *uuid)
+int virtio_dmabuf_lookup(const QemuUUID *uuid)
{
VirtioSharedObject *vso = get_shared_object(uuid);
if (vso == NULL) {
@@ -122,7 +122,7 @@ int virtio_lookup_dmabuf(const QemuUUID *uuid)
return vso->value.udma_buf;
}
-struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid)
+struct vhost_dev *virtio_dmabuf_lookup_vhost_device(const QemuUUID *uuid)
{
VirtioSharedObject *vso = get_shared_object(uuid);
if (vso == NULL) {
@@ -132,7 +132,7 @@ struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid)
return (struct vhost_dev *) vso->value.dev;
}
-SharedObjectType virtio_object_type(const QemuUUID *uuid)
+SharedObjectType virtio_dmabuf_object_type(const QemuUUID *uuid)
{
VirtioSharedObject *vso = get_shared_object(uuid);
if (vso == NULL) {
@@ -162,7 +162,7 @@ int virtio_dmabuf_vhost_cleanup(struct vhost_dev *dev)
return num_removed;
}
-void virtio_free_resources(void)
+void virtio_dmabuf_free_resources(void)
{
WITH_QEMU_LOCK_GUARD(&lock) {
g_hash_table_destroy(resource_uuids);
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 152710d30d..104d7d4e62 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1607,7 +1607,7 @@ vhost_user_backend_handle_shared_object_add(struct vhost_dev *dev,
QemuUUID uuid;
memcpy(uuid.data, object->uuid, sizeof(object->uuid));
- return virtio_add_vhost_device(&uuid, dev);
+ return virtio_dmabuf_add_vhost_device(&uuid, dev);
}
static int
@@ -1617,10 +1617,10 @@ vhost_user_backend_handle_shared_object_remove(struct vhost_dev *dev,
QemuUUID uuid;
memcpy(uuid.data, object->uuid, sizeof(object->uuid));
- switch (virtio_object_type(&uuid)) {
+ switch (virtio_dmabuf_object_type(&uuid)) {
case TYPE_VHOST_DEV:
{
- struct vhost_dev *owner = virtio_lookup_vhost_device(&uuid);
+ struct vhost_dev *owner = virtio_dmabuf_lookup_vhost_device(&uuid);
if (dev != owner) {
/* Not allowed to remove non-owned entries */
return 0;
@@ -1632,7 +1632,7 @@ vhost_user_backend_handle_shared_object_remove(struct vhost_dev *dev,
return 0;
}
- return virtio_remove_resource(&uuid);
+ return virtio_dmabuf_remove_resource(&uuid);
}
static bool vhost_user_send_resp(QIOChannel *ioc, VhostUserHeader *hdr,
@@ -1710,13 +1710,13 @@ vhost_user_backend_handle_shared_object_lookup(struct vhost_user *u,
memcpy(uuid.data, payload->object.uuid, sizeof(payload->object.uuid));
payload->u64 = 0;
- switch (virtio_object_type(&uuid)) {
+ switch (virtio_dmabuf_object_type(&uuid)) {
case TYPE_DMABUF:
- dmabuf_fd = virtio_lookup_dmabuf(&uuid);
+ dmabuf_fd = virtio_dmabuf_lookup(&uuid);
break;
case TYPE_VHOST_DEV:
{
- struct vhost_dev *dev = virtio_lookup_vhost_device(&uuid);
+ struct vhost_dev *dev = virtio_dmabuf_lookup_vhost_device(&uuid);
if (dev == NULL) {
payload->u64 = -EINVAL;
break;
diff --git a/include/hw/virtio/virtio-dmabuf.h b/include/hw/virtio/virtio-dmabuf.h
index 950cd24967..b9ee761a0c 100644
--- a/include/hw/virtio/virtio-dmabuf.h
+++ b/include/hw/virtio/virtio-dmabuf.h
@@ -56,7 +56,7 @@ typedef struct VirtioSharedObject {
void virtio_dmabuf_init(void);
/**
- * virtio_add_dmabuf() - Add a new dma-buf resource to the lookup table
+ * virtio_dmabuf_add() - 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
@@ -69,11 +69,11 @@ void virtio_dmabuf_init(void);
* 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);
+bool virtio_dmabuf_add(QemuUUID *uuid, int dmabuf_fd);
/**
- * virtio_add_vhost_device() - Add a new exporter vhost device that holds the
- * resource with the associated UUID
+ * virtio_dmabuf_add_vhost_device() - Add a new exporter vhost device that
+ * holds the resource with the associated UUID
* @uuid: new resource's UUID
* @dev: the pointer to the vhost device that holds the resource. The caller
* retains ownership over the device struct and its lifecycle.
@@ -83,41 +83,42 @@ bool virtio_add_dmabuf(QemuUUID *uuid, int dmabuf_fd);
* Note that if it finds a repeated UUID, the resource is not inserted in
* the lookup table.
*/
-bool virtio_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev);
+bool virtio_dmabuf_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev);
/**
- * virtio_remove_resource() - Removes a resource from the lookup table
+ * virtio_dmabuf_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);
+bool virtio_dmabuf_remove_resource(const QemuUUID *uuid);
/**
- * virtio_lookup_dmabuf() - Looks for a dma-buf resource in the lookup table
+ * virtio_dmabuf_lookup() - 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);
+int virtio_dmabuf_lookup(const QemuUUID *uuid);
/**
- * virtio_lookup_vhost_device() - Looks for an exporter vhost device in the
- * lookup table
+ * virtio_dmabuf_lookup_vhost_device() - Looks for an exporter vhost device
+ * in the lookup table
* @uuid: resource's UUID
*
* Return: pointer to the vhost_dev struct, or NULL if the key is not found.
*/
-struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid);
+struct vhost_dev *virtio_dmabuf_lookup_vhost_device(const QemuUUID *uuid);
/**
- * virtio_object_type() - Looks for the type of resource in the lookup table
+ * virtio_dmabuf_object_type() - Looks for the type of resource in the
+ * lookup table
* @uuid: resource's UUID
*
* Return: the type of resource associated with the UUID, or TYPE_INVALID if
* the key is not found.
*/
-SharedObjectType virtio_object_type(const QemuUUID *uuid);
+SharedObjectType virtio_dmabuf_object_type(const QemuUUID *uuid);
/**
* virtio_dmabuf_vhost_cleanup() - Destroys all entries of the shared
@@ -130,9 +131,9 @@ SharedObjectType virtio_object_type(const QemuUUID *uuid);
int virtio_dmabuf_vhost_cleanup(struct vhost_dev *dev);
/**
- * virtio_free_resources() - Destroys all keys and values of the shared
+ * virtio_dmabuf_free_resources() - Destroys all keys and values of the shared
* resources lookup table, and frees them
*/
-void virtio_free_resources(void);
+void virtio_dmabuf_free_resources(void);
#endif /* VIRTIO_DMABUF_H */
diff --git a/tests/unit/test-virtio-dmabuf.c b/tests/unit/test-virtio-dmabuf.c
index e5cf7ee19f..a0ff89cd3f 100644
--- a/tests/unit/test-virtio-dmabuf.c
+++ b/tests/unit/test-virtio-dmabuf.c
@@ -32,12 +32,12 @@ static void test_add_remove_resources(void)
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);
+ g_assert(virtio_dmabuf_add(&uuid, dmabuf_fd));
+ g_assert_cmpint(virtio_dmabuf_lookup(&uuid), ==, dmabuf_fd);
/* Remove the resource */
- g_assert(virtio_remove_resource(&uuid));
+ g_assert(virtio_dmabuf_remove_resource(&uuid));
/* Resource is not found anymore */
- g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, -1);
+ g_assert_cmpint(virtio_dmabuf_lookup(&uuid), ==, -1);
}
}
@@ -50,13 +50,13 @@ static void test_add_remove_dev(void)
virtio_dmabuf_init();
for (i = 0; i < 100; ++i) {
qemu_uuid_generate(&uuid);
- virtio_add_vhost_device(&uuid, dev);
+ virtio_dmabuf_add_vhost_device(&uuid, dev);
/* vhost device is found */
- g_assert(virtio_lookup_vhost_device(&uuid) != NULL);
+ g_assert(virtio_dmabuf_lookup_vhost_device(&uuid) != NULL);
/* Remove the vhost device */
- g_assert(virtio_remove_resource(&uuid));
+ g_assert(virtio_dmabuf_remove_resource(&uuid));
/* vhost device is not found anymore */
- g_assert(virtio_lookup_vhost_device(&uuid) == NULL);
+ g_assert(virtio_dmabuf_lookup_vhost_device(&uuid) == NULL);
}
g_free(dev);
}
@@ -69,9 +69,9 @@ static void test_remove_invalid_resource(void)
virtio_dmabuf_init();
for (i = 0; i < 20; ++i) {
qemu_uuid_generate(&uuid);
- g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, -1);
+ g_assert_cmpint(virtio_dmabuf_lookup(&uuid), ==, -1);
/* Removing a resource that does not exist returns false */
- g_assert_false(virtio_remove_resource(&uuid));
+ g_assert_false(virtio_dmabuf_remove_resource(&uuid));
}
}
@@ -85,25 +85,25 @@ static void test_add_invalid_resource(void)
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));
+ g_assert_false(virtio_dmabuf_add(&uuid, dmabuf_fd));
/* Resource is not found */
- g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, -1);
+ g_assert_cmpint(virtio_dmabuf_lookup(&uuid), ==, -1);
/* Add a new vhost device with invalid (NULL) pointer */
- g_assert_false(virtio_add_vhost_device(&uuid, dev));
+ g_assert_false(virtio_dmabuf_add_vhost_device(&uuid, dev));
/* vhost device is not found */
- g_assert(virtio_lookup_vhost_device(&uuid) == NULL);
+ g_assert(virtio_dmabuf_lookup_vhost_device(&uuid) == NULL);
}
for (i = 0; i < 20; ++i) {
/* Add a valid resource */
qemu_uuid_generate(&uuid);
dmabuf_fd = g_random_int_range(3, 500);
- g_assert(virtio_add_dmabuf(&uuid, dmabuf_fd));
- g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, dmabuf_fd);
+ g_assert(virtio_dmabuf_add(&uuid, dmabuf_fd));
+ g_assert_cmpint(virtio_dmabuf_lookup(&uuid), ==, dmabuf_fd);
/* Add a new resource with repeated uuid returns false */
- g_assert_false(virtio_add_dmabuf(&uuid, alt_dmabuf));
+ g_assert_false(virtio_dmabuf_add(&uuid, alt_dmabuf));
/* The value for the uuid key is not replaced */
- g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, dmabuf_fd);
+ g_assert_cmpint(virtio_dmabuf_lookup(&uuid), ==, dmabuf_fd);
}
}
@@ -116,25 +116,25 @@ static void test_cleanup_res(void)
for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
qemu_uuid_generate(&uuids[i]);
- virtio_add_vhost_device(&uuids[i], dev);
+ virtio_dmabuf_add_vhost_device(&uuids[i], dev);
/* vhost device is found */
- g_assert(virtio_lookup_vhost_device(&uuids[i]) != NULL);
+ g_assert(virtio_dmabuf_lookup_vhost_device(&uuids[i]) != NULL);
}
qemu_uuid_generate(&uuid_alt);
- virtio_add_vhost_device(&uuid_alt, dev_alt);
+ virtio_dmabuf_add_vhost_device(&uuid_alt, dev_alt);
/* vhost device is found */
- g_assert(virtio_lookup_vhost_device(&uuid_alt) != NULL);
+ g_assert(virtio_dmabuf_lookup_vhost_device(&uuid_alt) != NULL);
/* cleanup all dev resources */
num_removed = virtio_dmabuf_vhost_cleanup(dev);
g_assert_cmpint(num_removed, ==, ARRAY_SIZE(uuids));
for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
/* None of the dev resources is found after free'd */
- g_assert_cmpint(virtio_lookup_dmabuf(&uuids[i]), ==, -1);
+ g_assert_cmpint(virtio_dmabuf_lookup(&uuids[i]), ==, -1);
}
/* uuid_alt is still in the hash table */
- g_assert(virtio_lookup_vhost_device(&uuid_alt) != NULL);
+ g_assert(virtio_dmabuf_lookup_vhost_device(&uuid_alt) != NULL);
- virtio_free_resources();
+ virtio_dmabuf_free_resources();
g_free(dev);
g_free(dev_alt);
}
@@ -148,13 +148,13 @@ static void test_free_resources(void)
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);
+ g_assert(virtio_dmabuf_add(&uuids[i], dmabuf_fd));
+ g_assert_cmpint(virtio_dmabuf_lookup(&uuids[i]), ==, dmabuf_fd);
}
- virtio_free_resources();
+ virtio_dmabuf_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);
+ g_assert_cmpint(virtio_dmabuf_lookup(&uuids[i]), ==, -1);
}
}
--
2.43.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v4 0/5] Virtio dmabuf improvements
2024-02-19 14:34 [PATCH v4 0/5] Virtio dmabuf improvements Albert Esteve
` (4 preceding siblings ...)
2024-02-19 14:34 ` [PATCH v4 5/5] hw/virtio: rename virtio dmabuf API Albert Esteve
@ 2024-03-12 18:23 ` Michael S. Tsirkin
2024-03-13 8:00 ` Albert Esteve
5 siblings, 1 reply; 15+ messages in thread
From: Michael S. Tsirkin @ 2024-03-12 18:23 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, stefanha, alex.bennee, philmd, kraxel,
marcandre.lureau
On Mon, Feb 19, 2024 at 03:34:18PM +0100, Albert Esteve wrote:
> v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg1005257.html
> v2: https://www.mail-archive.com/qemu-devel@nongnu.org/msg1014615.html
> v3: Virtio dmabuf improvements
> v3 -> v4
> - Changed GMutex by QemuMutex in virtio-dmabuf
> - Made the value at VirtioSharedObject an union
> to make naming more clear
> - Added some documentation
Dropped everything except patch 1 for now.
> Various improvements for the virtio-dmabuf module.
> This patch includes:
>
> - Check for ownership before allowing a vhost device
> to remove an object from the table.
> - Properly cleanup shared resources if a vhost device
> object gets cleaned up.
> - Rename virtio dmabuf functions to `virtio_dmabuf_*`
>
> Albert Esteve (5):
> hw/virtio: check owner for removing objects
> hw/virtio: document SharedObject structures
> hw/virtio: change dmabuf mutex to QemuMutex
> hw/virtio: cleanup shared resources
> hw/virtio: rename virtio dmabuf API
>
> docs/interop/vhost-user.rst | 4 +-
> hw/display/virtio-dmabuf.c | 98 +++++++++++++++++++------------
> hw/virtio/vhost-user.c | 31 +++++++---
> hw/virtio/vhost.c | 3 +
> hw/virtio/virtio.c | 3 +
> include/hw/virtio/virtio-dmabuf.h | 73 +++++++++++++++++------
> tests/unit/test-virtio-dmabuf.c | 82 +++++++++++++++++++-------
> 7 files changed, 211 insertions(+), 83 deletions(-)
>
> --
> 2.43.1
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 0/5] Virtio dmabuf improvements
2024-03-12 18:23 ` [PATCH v4 0/5] Virtio dmabuf improvements Michael S. Tsirkin
@ 2024-03-13 8:00 ` Albert Esteve
0 siblings, 0 replies; 15+ messages in thread
From: Albert Esteve @ 2024-03-13 8:00 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, stefanha, alex.bennee, philmd, kraxel,
marcandre.lureau
[-- Attachment #1: Type: text/plain, Size: 1719 bytes --]
On Tue, Mar 12, 2024 at 7:23 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Feb 19, 2024 at 03:34:18PM +0100, Albert Esteve wrote:
> > v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg1005257.html
> > v2: https://www.mail-archive.com/qemu-devel@nongnu.org/msg1014615.html
> > v3: Virtio dmabuf improvements
> > v3 -> v4
> > - Changed GMutex by QemuMutex in virtio-dmabuf
> > - Made the value at VirtioSharedObject an union
> > to make naming more clear
> > - Added some documentation
>
> Dropped everything except patch 1 for now.
>
Got it. Thanks!
>
> > Various improvements for the virtio-dmabuf module.
> > This patch includes:
> >
> > - Check for ownership before allowing a vhost device
> > to remove an object from the table.
> > - Properly cleanup shared resources if a vhost device
> > object gets cleaned up.
> > - Rename virtio dmabuf functions to `virtio_dmabuf_*`
> >
> > Albert Esteve (5):
> > hw/virtio: check owner for removing objects
> > hw/virtio: document SharedObject structures
> > hw/virtio: change dmabuf mutex to QemuMutex
> > hw/virtio: cleanup shared resources
> > hw/virtio: rename virtio dmabuf API
> >
> > docs/interop/vhost-user.rst | 4 +-
> > hw/display/virtio-dmabuf.c | 98 +++++++++++++++++++------------
> > hw/virtio/vhost-user.c | 31 +++++++---
> > hw/virtio/vhost.c | 3 +
> > hw/virtio/virtio.c | 3 +
> > include/hw/virtio/virtio-dmabuf.h | 73 +++++++++++++++++------
> > tests/unit/test-virtio-dmabuf.c | 82 +++++++++++++++++++-------
> > 7 files changed, 211 insertions(+), 83 deletions(-)
> >
> > --
> > 2.43.1
>
>
[-- Attachment #2: Type: text/html, Size: 2882 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread