* [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests
@ 2025-02-17 16:40 Albert Esteve
2025-02-17 16:40 ` [PATCH v4 1/9] vhost-user: Add VirtIO Shared Memory map request Albert Esteve
` (9 more replies)
0 siblings, 10 replies; 36+ messages in thread
From: Albert Esteve @ 2025-02-17 16:40 UTC (permalink / raw)
To: qemu-devel
Cc: slp, stevensd, Alex Bennée, Stefano Garzarella, stefanha,
david, hi, mst, jasowang, Albert Esteve
Hi all,
v3->v4
- Change mmap strategy to use RAM blocks
and subregions.
- Add new bitfield to qmp feature map
- Followed most review comments from
last iteration.
- Merged documentation patch again with
this one. Makes more sense to
review them together after all.
- Add documentation for MEM_READ/WRITE
messages.
The goal of this patch is to support
dynamic fd-backed memory maps initiated
from vhost-user backends.
There are many devices that could already
benefit of this feature, e.g.,
virtiofs or virtio-gpu.
After receiving the SHMEM_MAP/UNMAP request,
the frontend creates the RAMBlock form the
fd and maps it by adding it as a subregion
of the shared memory region container.
The VIRTIO Shared Memory Region list is
declared in the `VirtIODevice` struct
to make it generic.
TODO: There was a conversation on the
previous version around adding tests
to the patch (which I have acknowledged).
However, given the numerous changes
that the patch already has, I have
decided to send it early and collect
some feedback while I work on the
tests for the next iteration.
Given that I have been able to
test the implementation with
my local setup, I am more or less
confident that, at least, the code
is in a relatively sane state
so that no reviewing time is
wasted on broken patches.
This patch also includes:
- SHMEM_CONFIG frontend request that is
specifically meant to allow generic
vhost-user-device frontend to be able to
query VIRTIO Shared Memory settings from the
backend (as this device is generic and agnostic
of the actual backend configuration).
- MEM_READ/WRITE backend requests are
added to deal with a potential issue when having
multiple backends sharing a file descriptor.
When a backend calls SHMEM_MAP it makes
accessing to the region fail for other
backend as it is missing from their translation
table. So these requests are a fallback
for vhost-user memory translation fails.
Albert Esteve (9):
vhost-user: Add VirtIO Shared Memory map request
vhost_user.rst: Align VhostUserMsg excerpt members
vhost_user.rst: Add SHMEM_MAP/_UNMAP to spec
vhost_user: Add frontend get_shmem_config command
vhost_user.rst: Add GET_SHMEM_CONFIG message
qmp: add shmem feature map
vhost-user-devive: Add shmem BAR
vhost_user: Add mem_read/write backend requests
vhost_user.rst: Add MEM_READ/WRITE messages
docs/interop/vhost-user.rst | 110 +++++++++
hw/virtio/vhost-user-base.c | 47 +++-
hw/virtio/vhost-user-device-pci.c | 36 ++-
hw/virtio/vhost-user.c | 272 ++++++++++++++++++++--
hw/virtio/virtio-qmp.c | 3 +
hw/virtio/virtio.c | 81 +++++++
include/hw/virtio/vhost-backend.h | 9 +
include/hw/virtio/vhost-user.h | 1 +
include/hw/virtio/virtio.h | 29 +++
subprojects/libvhost-user/libvhost-user.c | 160 +++++++++++++
subprojects/libvhost-user/libvhost-user.h | 92 ++++++++
11 files changed, 813 insertions(+), 27 deletions(-)
--
2.48.1
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v4 1/9] vhost-user: Add VirtIO Shared Memory map request
2025-02-17 16:40 [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests Albert Esteve
@ 2025-02-17 16:40 ` Albert Esteve
2025-02-18 6:43 ` Stefan Hajnoczi
` (2 more replies)
2025-02-17 16:40 ` [PATCH v4 2/9] vhost_user.rst: Align VhostUserMsg excerpt members Albert Esteve
` (8 subsequent siblings)
9 siblings, 3 replies; 36+ messages in thread
From: Albert Esteve @ 2025-02-17 16:40 UTC (permalink / raw)
To: qemu-devel
Cc: slp, stevensd, Alex Bennée, Stefano Garzarella, stefanha,
david, hi, mst, jasowang, Albert Esteve
Add SHMEM_MAP/UNMAP requests to vhost-user to
handle VIRTIO Shared Memory mappings.
This request allows backends to dynamically map
fds into a VIRTIO Shared Memory Region indentified
by its `shmid`. The map is performed by calling
`memory_region_init_ram_from_fd` and adding the
new region as a subregion of the shmem container
MR. Then, the fd memory is advertised to the
driver as a base addres + offset, so it can be
read/written (depending on the mmap flags
requested) while it is valid.
The backend can unmap the memory range
in a given VIRTIO Shared Memory Region (again,
identified by its `shmid`), to free it.
Upon receiving this message, the front-end
must delete the MR as a subregion of
the shmem container region and free its
resources.
Note that commit all these operations need
to be delayed to after we respond the request
to the backend to avoid deadlocks.
The device model needs to create VirtSharedMemory
instances for the VirtIO Shared Memory Regions
and add them to the `VirtIODevice` instance.
Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
hw/virtio/vhost-user.c | 134 ++++++++++++++++++++++
hw/virtio/virtio.c | 81 +++++++++++++
include/hw/virtio/virtio.h | 27 +++++
subprojects/libvhost-user/libvhost-user.c | 70 +++++++++++
subprojects/libvhost-user/libvhost-user.h | 55 +++++++++
5 files changed, 367 insertions(+)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 267b612587..d88e6f8c3c 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -115,6 +115,8 @@ typedef enum VhostUserBackendRequest {
VHOST_USER_BACKEND_SHARED_OBJECT_ADD = 6,
VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE = 7,
VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP = 8,
+ VHOST_USER_BACKEND_SHMEM_MAP = 9,
+ VHOST_USER_BACKEND_SHMEM_UNMAP = 10,
VHOST_USER_BACKEND_MAX
} VhostUserBackendRequest;
@@ -192,6 +194,24 @@ typedef struct VhostUserShared {
unsigned char uuid[16];
} VhostUserShared;
+/* For the flags field of VhostUserMMap */
+#define VHOST_USER_FLAG_MAP_R (1u << 0)
+#define VHOST_USER_FLAG_MAP_W (1u << 1)
+
+typedef struct {
+ /* VIRTIO Shared Memory Region ID */
+ uint8_t shmid;
+ uint8_t padding[7];
+ /* File offset */
+ uint64_t fd_offset;
+ /* Offset within the VIRTIO Shared Memory Region */
+ uint64_t shm_offset;
+ /* Size of the mapping */
+ uint64_t len;
+ /* Flags for the mmap operation, from VHOST_USER_FLAG_* */
+ uint64_t flags;
+} VhostUserMMap;
+
typedef struct {
VhostUserRequest request;
@@ -224,6 +244,7 @@ typedef union {
VhostUserInflight inflight;
VhostUserShared object;
VhostUserTransferDeviceState transfer_state;
+ VhostUserMMap mmap;
} VhostUserPayload;
typedef struct VhostUserMsg {
@@ -1770,6 +1791,111 @@ vhost_user_backend_handle_shared_object_lookup(struct vhost_user *u,
return 0;
}
+static int
+vhost_user_backend_handle_shmem_map(struct vhost_dev *dev,
+ QIOChannel *ioc,
+ VhostUserHeader *hdr,
+ VhostUserPayload *payload,
+ int fd, Error **errp)
+{
+ VirtSharedMemory *shmem = NULL;
+ VhostUserMMap *vu_mmap = &payload->mmap;
+ g_autoptr(GString) shm_name = g_string_new(NULL);
+
+ if (fd < 0) {
+ error_report("Bad fd for map");
+ return -EBADF;
+ }
+
+ if (!dev->vdev->shmem_list ||
+ dev->vdev->n_shmem_regions <= vu_mmap->shmid) {
+ error_report("Device only has %d VIRTIO Shared Memory Regions. "
+ "Requested ID: %d",
+ dev->vdev->n_shmem_regions, vu_mmap->shmid);
+ return -EFAULT;
+ }
+
+ shmem = &dev->vdev->shmem_list[vu_mmap->shmid];
+
+ if (!shmem) {
+ error_report("VIRTIO Shared Memory Region at "
+ "ID %d unitialized", vu_mmap->shmid);
+ return -EFAULT;
+ }
+
+ if ((vu_mmap->shm_offset + vu_mmap->len) < vu_mmap->len ||
+ (vu_mmap->shm_offset + vu_mmap->len) > shmem->mr->size) {
+ error_report("Bad offset/len for mmap %" PRIx64 "+%" PRIx64,
+ vu_mmap->shm_offset, vu_mmap->len);
+ return -EFAULT;
+ }
+
+ g_string_printf(shm_name, "virtio-shm%i-%lu",
+ vu_mmap->shmid, vu_mmap->shm_offset);
+
+ memory_region_transaction_begin();
+ virtio_add_shmem_map(shmem, shm_name->str, vu_mmap->shm_offset,
+ vu_mmap->fd_offset, vu_mmap->len, fd, errp);
+
+ hdr->size = sizeof(payload->u64);
+ vhost_user_send_resp(ioc, hdr, payload, errp);
+ memory_region_transaction_commit();
+
+ return 0;
+}
+
+static int
+vhost_user_backend_handle_shmem_unmap(struct vhost_dev *dev,
+ QIOChannel *ioc,
+ VhostUserHeader *hdr,
+ VhostUserPayload *payload,
+ Error **errp)
+{
+ VirtSharedMemory *shmem = NULL;
+ MappedMemoryRegion *mmap = NULL;
+ VhostUserMMap *vu_mmap = &payload->mmap;
+
+ if (!dev->vdev->shmem_list ||
+ dev->vdev->n_shmem_regions <= vu_mmap->shmid) {
+ error_report("Device only has %d VIRTIO Shared Memory Regions. "
+ "Requested ID: %d",
+ dev->vdev->n_shmem_regions, vu_mmap->shmid);
+ return -EFAULT;
+ }
+
+ shmem = &dev->vdev->shmem_list[vu_mmap->shmid];
+
+ if (!shmem) {
+ error_report("VIRTIO Shared Memory Region at "
+ "ID %d unitialized", vu_mmap->shmid);
+ return -EFAULT;
+ }
+
+ if ((vu_mmap->shm_offset + vu_mmap->len) < vu_mmap->len ||
+ (vu_mmap->shm_offset + vu_mmap->len) > shmem->mr->size) {
+ error_report("Bad offset/len for unmmap %" PRIx64 "+%" PRIx64,
+ vu_mmap->shm_offset, vu_mmap->len);
+ return -EFAULT;
+ }
+
+ mmap = virtio_find_shmem_map(shmem, vu_mmap->shm_offset, vu_mmap->len);
+ if (!mmap) {
+ return -EFAULT;
+ }
+
+ memory_region_transaction_begin();
+ memory_region_del_subregion(shmem->mr, mmap->mem);
+
+ hdr->size = sizeof(payload->u64);
+ vhost_user_send_resp(ioc, hdr, payload, errp);
+ memory_region_transaction_commit();
+
+ /* Free the MemoryRegion only after vhost_commit */
+ virtio_del_shmem_map(shmem, vu_mmap->shm_offset, vu_mmap->len);
+
+ return 0;
+}
+
static void close_backend_channel(struct vhost_user *u)
{
g_source_destroy(u->backend_src);
@@ -1837,6 +1963,14 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition,
case VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP:
ret = vhost_user_backend_handle_shared_object_lookup(dev->opaque, ioc,
&hdr, &payload);
+ case VHOST_USER_BACKEND_SHMEM_MAP:
+ ret = vhost_user_backend_handle_shmem_map(dev, ioc, &hdr, &payload,
+ fd ? fd[0] : -1, &local_err);
+ break;
+ case VHOST_USER_BACKEND_SHMEM_UNMAP:
+ ret = vhost_user_backend_handle_shmem_unmap(dev, ioc, &hdr, &payload,
+ &local_err);
+ break;
break;
default:
error_report("Received unexpected msg type: %d.", hdr.request);
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 85110bce37..47d0ddb820 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3063,6 +3063,75 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f)
return vmstate_save_state(f, &vmstate_virtio, vdev, NULL);
}
+VirtSharedMemory *virtio_new_shmem_region(VirtIODevice *vdev)
+{
+ ++vdev->n_shmem_regions;
+ vdev->shmem_list = g_renew(VirtSharedMemory, vdev->shmem_list,
+ vdev->n_shmem_regions);
+ vdev->shmem_list[vdev->n_shmem_regions - 1].mr = g_new0(MemoryRegion, 1);
+ QTAILQ_INIT(&vdev->shmem_list[vdev->n_shmem_regions - 1].mmaps);
+ return &vdev->shmem_list[vdev->n_shmem_regions - 1];
+}
+
+void virtio_add_shmem_map(VirtSharedMemory *shmem, const char *shm_name,
+ hwaddr shm_offset, hwaddr fd_offset, uint64_t size,
+ int fd, Error **errp)
+{
+ MappedMemoryRegion *mmap;
+ fd = dup(fd);
+ if (fd < 0) {
+ error_setg_errno(errp, errno, "Failed to duplicate fd");
+ return;
+ }
+
+ if (shm_offset + size > shmem->mr->size) {
+ error_setg(errp, "Memory exceeds the shared memory boundaries");
+ return;
+ }
+
+ mmap = g_new0(MappedMemoryRegion, 1);
+ mmap->mem = g_new0(MemoryRegion, 1);
+ mmap->offset = shm_offset;
+ if (!memory_region_init_ram_from_fd(mmap->mem,
+ OBJECT(shmem->mr),
+ shm_name, size, RAM_SHARED,
+ fd, fd_offset, errp)) {
+ error_setg(errp, "Failed to mmap region %s", shm_name);
+ close(fd);
+ g_free(mmap->mem);
+ g_free(mmap);
+ return;
+ }
+ memory_region_add_subregion(shmem->mr, shm_offset, mmap->mem);
+
+ QTAILQ_INSERT_TAIL(&shmem->mmaps, mmap, link);
+}
+
+MappedMemoryRegion *virtio_find_shmem_map(VirtSharedMemory *shmem,
+ hwaddr offset, uint64_t size)
+{
+ MappedMemoryRegion *mmap;
+ QTAILQ_FOREACH(mmap, &shmem->mmaps, link) {
+ if (mmap->offset == offset && mmap->mem->size == size) {
+ return mmap;
+ }
+ }
+ return NULL;
+}
+
+void virtio_del_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
+ uint64_t size)
+{
+ MappedMemoryRegion *mmap = virtio_find_shmem_map(shmem, offset, size);
+ if (mmap == NULL) {
+ return;
+ }
+
+ object_unparent(OBJECT(mmap->mem));
+ QTAILQ_REMOVE(&shmem->mmaps, mmap, link);
+ g_free(mmap);
+}
+
/* A wrapper for use as a VMState .put function */
static int virtio_device_put(QEMUFile *f, void *opaque, size_t size,
const VMStateField *field, JSONWriter *vmdesc)
@@ -3492,6 +3561,8 @@ void virtio_init(VirtIODevice *vdev, uint16_t device_id, size_t config_size)
virtio_vmstate_change, vdev);
vdev->device_endian = virtio_default_endian();
vdev->use_guest_notifier_mask = true;
+ vdev->shmem_list = NULL;
+ vdev->n_shmem_regions = 0;
}
/*
@@ -4005,11 +4076,21 @@ static void virtio_device_free_virtqueues(VirtIODevice *vdev)
static void virtio_device_instance_finalize(Object *obj)
{
VirtIODevice *vdev = VIRTIO_DEVICE(obj);
+ VirtSharedMemory *shmem = NULL;
+ int i;
virtio_device_free_virtqueues(vdev);
g_free(vdev->config);
g_free(vdev->vector_queues);
+ for (i = 0; i< vdev->n_shmem_regions; i++) {
+ shmem = &vdev->shmem_list[i];
+ while (!QTAILQ_EMPTY(&shmem->mmaps)) {
+ MappedMemoryRegion *mmap_reg = QTAILQ_FIRST(&shmem->mmaps);
+ QTAILQ_REMOVE(&shmem->mmaps, mmap_reg, link);
+ g_free(mmap_reg);
+ }
+ }
}
static const Property virtio_properties[] = {
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 6386910280..a778547c79 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -98,6 +98,21 @@ enum virtio_device_endian {
VIRTIO_DEVICE_ENDIAN_BIG,
};
+struct MappedMemoryRegion {
+ MemoryRegion *mem;
+ hwaddr offset;
+ QTAILQ_ENTRY(MappedMemoryRegion) link;
+};
+
+typedef struct MappedMemoryRegion MappedMemoryRegion;
+
+struct VirtSharedMemory {
+ MemoryRegion *mr;
+ QTAILQ_HEAD(, MappedMemoryRegion) mmaps;
+};
+
+typedef struct VirtSharedMemory VirtSharedMemory;
+
/**
* struct VirtIODevice - common VirtIO structure
* @name: name of the device
@@ -167,6 +182,9 @@ struct VirtIODevice
*/
EventNotifier config_notifier;
bool device_iotlb_enabled;
+ /* Shared memory region for vhost-user mappings. */
+ VirtSharedMemory *shmem_list;
+ int n_shmem_regions;
};
struct VirtioDeviceClass {
@@ -289,6 +307,15 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
int virtio_save(VirtIODevice *vdev, QEMUFile *f);
+VirtSharedMemory *virtio_new_shmem_region(VirtIODevice *vdev);
+void virtio_add_shmem_map(VirtSharedMemory *shmem, const char *shm_name,
+ hwaddr shm_offset, hwaddr fd_offset, uint64_t size,
+ int fd, Error **errp);
+MappedMemoryRegion *virtio_find_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
+ uint64_t size);
+void virtio_del_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
+ uint64_t size);
+
extern const VMStateInfo virtio_vmstate_info;
#define VMSTATE_VIRTIO_DEVICE \
diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index 9c630c2170..034cbfdc3c 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -1592,6 +1592,76 @@ vu_rm_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN])
return vu_send_message(dev, &msg);
}
+bool
+vu_shmem_map(VuDev *dev, uint8_t shmid, uint64_t fd_offset,
+ uint64_t shm_offset, uint64_t len, uint64_t flags, int fd)
+{
+ VhostUserMsg vmsg = {
+ .request = VHOST_USER_BACKEND_SHMEM_MAP,
+ .size = sizeof(vmsg.payload.mmap),
+ .flags = VHOST_USER_VERSION,
+ .payload.mmap = {
+ .shmid = shmid,
+ .fd_offset = fd_offset,
+ .shm_offset = shm_offset,
+ .len = len,
+ .flags = flags,
+ },
+ .fd_num = 1,
+ .fds[0] = fd,
+ };
+
+ if (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SHMEM)) {
+ return false;
+ }
+
+ if (vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_REPLY_ACK)) {
+ vmsg.flags |= VHOST_USER_NEED_REPLY_MASK;
+ }
+
+ pthread_mutex_lock(&dev->backend_mutex);
+ if (!vu_message_write(dev, dev->backend_fd, &vmsg)) {
+ pthread_mutex_unlock(&dev->backend_mutex);
+ return false;
+ }
+
+ /* Also unlocks the backend_mutex */
+ return vu_process_message_reply(dev, &vmsg);
+}
+
+bool
+vu_shmem_unmap(VuDev *dev, uint8_t shmid, uint64_t shm_offset, uint64_t len)
+{
+ VhostUserMsg vmsg = {
+ .request = VHOST_USER_BACKEND_SHMEM_UNMAP,
+ .size = sizeof(vmsg.payload.mmap),
+ .flags = VHOST_USER_VERSION,
+ .payload.mmap = {
+ .shmid = shmid,
+ .fd_offset = 0,
+ .shm_offset = shm_offset,
+ .len = len,
+ },
+ };
+
+ if (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SHMEM)) {
+ return false;
+ }
+
+ if (vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_REPLY_ACK)) {
+ vmsg.flags |= VHOST_USER_NEED_REPLY_MASK;
+ }
+
+ pthread_mutex_lock(&dev->backend_mutex);
+ if (!vu_message_write(dev, dev->backend_fd, &vmsg)) {
+ pthread_mutex_unlock(&dev->backend_mutex);
+ return false;
+ }
+
+ /* Also unlocks the backend_mutex */
+ return vu_process_message_reply(dev, &vmsg);
+}
+
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 2ffc58c11b..e9adb836f0 100644
--- a/subprojects/libvhost-user/libvhost-user.h
+++ b/subprojects/libvhost-user/libvhost-user.h
@@ -69,6 +69,8 @@ enum VhostUserProtocolFeature {
/* Feature 16 is reserved for VHOST_USER_PROTOCOL_F_STATUS. */
/* Feature 17 reserved for VHOST_USER_PROTOCOL_F_XEN_MMAP. */
VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 18,
+ /* Feature 19 is reserved for VHOST_USER_PROTOCOL_F_DEVICE_STATE */
+ VHOST_USER_PROTOCOL_F_SHMEM = 20,
VHOST_USER_PROTOCOL_F_MAX
};
@@ -127,6 +129,8 @@ typedef enum VhostUserBackendRequest {
VHOST_USER_BACKEND_SHARED_OBJECT_ADD = 6,
VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE = 7,
VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP = 8,
+ VHOST_USER_BACKEND_SHMEM_MAP = 9,
+ VHOST_USER_BACKEND_SHMEM_UNMAP = 10,
VHOST_USER_BACKEND_MAX
} VhostUserBackendRequest;
@@ -186,6 +190,24 @@ typedef struct VhostUserShared {
unsigned char uuid[UUID_LEN];
} VhostUserShared;
+/* For the flags field of VhostUserMMap */
+#define VHOST_USER_FLAG_MAP_R (1u << 0)
+#define VHOST_USER_FLAG_MAP_W (1u << 1)
+
+typedef struct {
+ /* VIRTIO Shared Memory Region ID */
+ uint8_t shmid;
+ uint8_t padding[7];
+ /* File offset */
+ uint64_t fd_offset;
+ /* Offset within the VIRTIO Shared Memory Region */
+ uint64_t shm_offset;
+ /* Size of the mapping */
+ uint64_t len;
+ /* Flags for the mmap operation, from VHOST_USER_FLAG_* */
+ uint64_t flags;
+} VhostUserMMap;
+
#define VU_PACKED __attribute__((packed))
typedef struct VhostUserMsg {
@@ -210,6 +232,7 @@ typedef struct VhostUserMsg {
VhostUserVringArea area;
VhostUserInflight inflight;
VhostUserShared object;
+ VhostUserMMap mmap;
} payload;
int fds[VHOST_MEMORY_BASELINE_NREGIONS];
@@ -593,6 +616,38 @@ bool vu_add_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN]);
*/
bool vu_rm_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN]);
+/**
+ * vu_shmem_map:
+ * @dev: a VuDev context
+ * @shmid: VIRTIO Shared Memory Region ID
+ * @fd_offset: File offset
+ * @shm_offset: Offset within the VIRTIO Shared Memory Region
+ * @len: Size of the mapping
+ * @flags: Flags for the mmap operation
+ * @fd: A file descriptor
+ *
+ * Advertises a new mapping to be made in a given VIRTIO Shared Memory Region.
+ *
+ * Returns: TRUE on success, FALSE on failure.
+ */
+bool vu_shmem_map(VuDev *dev, uint8_t shmid, uint64_t fd_offset,
+ uint64_t shm_offset, uint64_t len, uint64_t flags, int fd);
+
+/**
+ * vu_shmem_unmap:
+ * @dev: a VuDev context
+ * @shmid: VIRTIO Shared Memory Region ID
+ * @fd_offset: File offset
+ * @len: Size of the mapping
+ *
+ * The front-end un-mmaps a given range in the VIRTIO Shared Memory Region
+ * with the requested `shmid`.
+ *
+ * Returns: TRUE on success, FALSE on failure.
+ */
+bool vu_shmem_unmap(VuDev *dev, uint8_t shmid, uint64_t shm_offset,
+ uint64_t len);
+
/**
* vu_queue_set_notification:
* @dev: a VuDev context
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v4 2/9] vhost_user.rst: Align VhostUserMsg excerpt members
2025-02-17 16:40 [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests Albert Esteve
2025-02-17 16:40 ` [PATCH v4 1/9] vhost-user: Add VirtIO Shared Memory map request Albert Esteve
@ 2025-02-17 16:40 ` Albert Esteve
2025-02-18 6:44 ` Stefan Hajnoczi
2025-02-17 16:40 ` [PATCH v4 3/9] vhost_user.rst: Add SHMEM_MAP/_UNMAP to spec Albert Esteve
` (7 subsequent siblings)
9 siblings, 1 reply; 36+ messages in thread
From: Albert Esteve @ 2025-02-17 16:40 UTC (permalink / raw)
To: qemu-devel
Cc: slp, stevensd, Alex Bennée, Stefano Garzarella, stefanha,
david, hi, mst, jasowang, Albert Esteve
Add missing members to the VhostUserMsg excerpt in
the vhost-user spec documentation.
Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
docs/interop/vhost-user.rst | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
index 2e50f2ddfa..436a94c0ee 100644
--- a/docs/interop/vhost-user.rst
+++ b/docs/interop/vhost-user.rst
@@ -366,11 +366,15 @@ In QEMU the vhost-user message is implemented with the following struct:
struct vhost_vring_state state;
struct vhost_vring_addr addr;
VhostUserMemory memory;
+ VhostUserMemRegMsg mem_reg;
VhostUserLog log;
struct vhost_iotlb_msg iotlb;
VhostUserConfig config;
+ VhostUserCryptoSession session;
VhostUserVringArea area;
VhostUserInflight inflight;
+ VhostUserShared object;
+ VhostUserTransferDeviceState transfer_state;
};
} QEMU_PACKED VhostUserMsg;
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v4 3/9] vhost_user.rst: Add SHMEM_MAP/_UNMAP to spec
2025-02-17 16:40 [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests Albert Esteve
2025-02-17 16:40 ` [PATCH v4 1/9] vhost-user: Add VirtIO Shared Memory map request Albert Esteve
2025-02-17 16:40 ` [PATCH v4 2/9] vhost_user.rst: Align VhostUserMsg excerpt members Albert Esteve
@ 2025-02-17 16:40 ` Albert Esteve
2025-02-17 16:40 ` [PATCH v4 4/9] vhost_user: Add frontend get_shmem_config command Albert Esteve
` (6 subsequent siblings)
9 siblings, 0 replies; 36+ messages in thread
From: Albert Esteve @ 2025-02-17 16:40 UTC (permalink / raw)
To: qemu-devel
Cc: slp, stevensd, Alex Bennée, Stefano Garzarella, stefanha,
david, hi, mst, jasowang, Albert Esteve
Add SHMEM_MAP/_UNMAP request to the vhost-user
spec documentation.
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
docs/interop/vhost-user.rst | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
index 436a94c0ee..c15d6ac467 100644
--- a/docs/interop/vhost-user.rst
+++ b/docs/interop/vhost-user.rst
@@ -375,6 +375,7 @@ In QEMU the vhost-user message is implemented with the following struct:
VhostUserInflight inflight;
VhostUserShared object;
VhostUserTransferDeviceState transfer_state;
+ VhostUserMMap mmap;
};
} QEMU_PACKED VhostUserMsg;
@@ -1057,6 +1058,7 @@ Protocol features
#define VHOST_USER_PROTOCOL_F_XEN_MMAP 17
#define VHOST_USER_PROTOCOL_F_SHARED_OBJECT 18
#define VHOST_USER_PROTOCOL_F_DEVICE_STATE 19
+ #define VHOST_USER_PROTOCOL_F_SHMEM 20
Front-end message types
-----------------------
@@ -1865,6 +1867,38 @@ is sent by the front-end.
when the operation is successful, or non-zero otherwise. Note that if the
operation fails, no fd is sent to the backend.
+``VHOST_USER_BACKEND_SHMEM_MAP``
+ :id: 9
+ :equivalent ioctl: N/A
+ :request payload: fd and ``struct VhostUserMMap``
+ :reply payload: N/A
+
+ When the ``VHOST_USER_PROTOCOL_F_SHMEM`` protocol feature has been
+ successfully negotiated, this message can be submitted by the backends to
+ advertise a new mapping to be made in a given VIRTIO Shared Memory Region.
+ Upon receiving the message, the front-end will mmap the given fd into the
+ VIRTIO Shared Memory Region with the requested ``shmid``. A reply is
+ generated indicating whether mapping succeeded.
+
+ Mapping over an already existing map is not allowed and request shall fail.
+ Therefore, the memory range in the request must correspond with a valid,
+ free region of the VIRTIO Shared Memory Region. Also, note that mappings
+ consume resources and that the request can fail when there are no resources
+ available.
+
+``VHOST_USER_BACKEND_SHMEM_UNMAP``
+ :id: 10
+ :equivalent ioctl: N/A
+ :request payload: ``struct VhostUserMMap``
+ :reply payload: N/A
+
+ When the ``VHOST_USER_PROTOCOL_F_SHMEM`` protocol feature has been
+ successfully negotiated, this message can be submitted by the backends so
+ that the front-end un-mmap a given range (``shm_offset``, ``len``) in the
+ VIRTIO Shared Memory Region with the requested ``shmid``. Note that the
+ given range shall correspond to the entirety of a valid mapped region.
+ A reply is generated indicating whether unmapping succeeded.
+
.. _reply_ack:
VHOST_USER_PROTOCOL_F_REPLY_ACK
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v4 4/9] vhost_user: Add frontend get_shmem_config command
2025-02-17 16:40 [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests Albert Esteve
` (2 preceding siblings ...)
2025-02-17 16:40 ` [PATCH v4 3/9] vhost_user.rst: Add SHMEM_MAP/_UNMAP to spec Albert Esteve
@ 2025-02-17 16:40 ` Albert Esteve
2025-02-18 10:27 ` Stefan Hajnoczi
2025-02-17 16:40 ` [PATCH v4 5/9] vhost_user.rst: Add GET_SHMEM_CONFIG message Albert Esteve
` (5 subsequent siblings)
9 siblings, 1 reply; 36+ messages in thread
From: Albert Esteve @ 2025-02-17 16:40 UTC (permalink / raw)
To: qemu-devel
Cc: slp, stevensd, Alex Bennée, Stefano Garzarella, stefanha,
david, hi, mst, jasowang, Albert Esteve
The frontend can use this command to retrieve
VirtIO Shared Memory Regions configuration from
the backend. The response contains the number of
shared memory regions, their size, and shmid.
This is useful when the frontend is unaware of
specific backend type and configuration,
for example, in the `vhost-user-device` case.
Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
hw/virtio/vhost-user.c | 43 +++++++++++++++++++++++++++++++
include/hw/virtio/vhost-backend.h | 9 +++++++
include/hw/virtio/vhost-user.h | 1 +
include/hw/virtio/virtio.h | 2 ++
4 files changed, 55 insertions(+)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index d88e6f8c3c..9cc148f726 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -104,6 +104,7 @@ typedef enum VhostUserRequest {
VHOST_USER_GET_SHARED_OBJECT = 41,
VHOST_USER_SET_DEVICE_STATE_FD = 42,
VHOST_USER_CHECK_DEVICE_STATE = 43,
+ VHOST_USER_GET_SHMEM_CONFIG = 44,
VHOST_USER_MAX
} VhostUserRequest;
@@ -138,6 +139,12 @@ typedef struct VhostUserMemRegMsg {
VhostUserMemoryRegion region;
} VhostUserMemRegMsg;
+typedef struct VhostUserShMemConfig {
+ uint32_t nregions;
+ uint32_t padding;
+ uint64_t memory_sizes[VIRTIO_MAX_SHMEM_REGIONS];
+} VhostUserShMemConfig;
+
typedef struct VhostUserLog {
uint64_t mmap_size;
uint64_t mmap_offset;
@@ -245,6 +252,7 @@ typedef union {
VhostUserShared object;
VhostUserTransferDeviceState transfer_state;
VhostUserMMap mmap;
+ VhostUserShMemConfig shmem;
} VhostUserPayload;
typedef struct VhostUserMsg {
@@ -3146,6 +3154,40 @@ static int vhost_user_check_device_state(struct vhost_dev *dev, Error **errp)
return 0;
}
+static int vhost_user_get_shmem_config(struct vhost_dev *dev,
+ int *nregions,
+ uint64_t *memory_sizes,
+ Error **errp)
+{
+ int ret;
+ VhostUserMsg msg = {
+ .hdr.request = VHOST_USER_GET_SHMEM_CONFIG,
+ .hdr.flags = VHOST_USER_VERSION,
+ };
+
+ if (!virtio_has_feature(dev->protocol_features,
+ VHOST_USER_PROTOCOL_F_SHMEM)) {
+ return 0;
+ }
+
+ ret = vhost_user_write(dev, &msg, NULL, 0);
+ if (ret < 0) {
+ return ret;
+ }
+
+ ret = vhost_user_read(dev, &msg);
+ if (ret < 0) {
+ return ret;
+ }
+
+ assert(msg.payload.shmem.nregions <= VIRTIO_MAX_SHMEM_REGIONS);
+ *nregions = msg.payload.shmem.nregions;
+ memcpy(memory_sizes,
+ &msg.payload.shmem.memory_sizes,
+ sizeof(uint64_t) * VHOST_MEMORY_BASELINE_NREGIONS);
+ return 0;
+}
+
const VhostOps user_ops = {
.backend_type = VHOST_BACKEND_TYPE_USER,
.vhost_backend_init = vhost_user_backend_init,
@@ -3184,4 +3226,5 @@ const VhostOps user_ops = {
.vhost_supports_device_state = vhost_user_supports_device_state,
.vhost_set_device_state_fd = vhost_user_set_device_state_fd,
.vhost_check_device_state = vhost_user_check_device_state,
+ .vhost_get_shmem_config = vhost_user_get_shmem_config,
};
diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
index 70c2e8ffee..b40d82a111 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -159,6 +159,14 @@ typedef int (*vhost_set_device_state_fd_op)(struct vhost_dev *dev,
int *reply_fd,
Error **errp);
typedef int (*vhost_check_device_state_op)(struct vhost_dev *dev, Error **errp);
+/*
+ * Max regions is VIRTIO_MAX_SHMEM_REGIONS, so that is the maximum
+ * number of memory_sizes that will be accepted. */
+typedef int (*vhost_get_shmem_config_op)(struct vhost_dev *dev,
+ int *nregions,
+ uint64_t *memory_sizes,
+ Error **errp);
+
typedef struct VhostOps {
VhostBackendType backend_type;
@@ -214,6 +222,7 @@ typedef struct VhostOps {
vhost_supports_device_state_op vhost_supports_device_state;
vhost_set_device_state_fd_op vhost_set_device_state_fd;
vhost_check_device_state_op vhost_check_device_state;
+ vhost_get_shmem_config_op vhost_get_shmem_config;
} VhostOps;
int vhost_backend_update_device_iotlb(struct vhost_dev *dev,
diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
index 9a3f238b43..bacc7d184c 100644
--- a/include/hw/virtio/vhost-user.h
+++ b/include/hw/virtio/vhost-user.h
@@ -32,6 +32,7 @@ enum VhostUserProtocolFeature {
/* Feature 17 reserved for VHOST_USER_PROTOCOL_F_XEN_MMAP. */
VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 18,
VHOST_USER_PROTOCOL_F_DEVICE_STATE = 19,
+ VHOST_USER_PROTOCOL_F_SHMEM = 20,
VHOST_USER_PROTOCOL_F_MAX
};
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index a778547c79..319e2f5b06 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -81,6 +81,8 @@ typedef struct VirtQueueElement
#define VIRTIO_NO_VECTOR 0xffff
+#define VIRTIO_MAX_SHMEM_REGIONS 256
+
/* special index value used internally for config irqs */
#define VIRTIO_CONFIG_IRQ_IDX -1
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v4 5/9] vhost_user.rst: Add GET_SHMEM_CONFIG message
2025-02-17 16:40 [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests Albert Esteve
` (3 preceding siblings ...)
2025-02-17 16:40 ` [PATCH v4 4/9] vhost_user: Add frontend get_shmem_config command Albert Esteve
@ 2025-02-17 16:40 ` Albert Esteve
2025-02-18 10:33 ` Stefan Hajnoczi
2025-02-17 16:40 ` [PATCH v4 6/9] qmp: add shmem feature map Albert Esteve
` (4 subsequent siblings)
9 siblings, 1 reply; 36+ messages in thread
From: Albert Esteve @ 2025-02-17 16:40 UTC (permalink / raw)
To: qemu-devel
Cc: slp, stevensd, Alex Bennée, Stefano Garzarella, stefanha,
david, hi, mst, jasowang, Albert Esteve
Add GET_SHMEM_CONFIG vhost-user frontend
message to the spec documentation.
Reviewed-by: Alyssa Ross <hi@alyssa.is>
Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
docs/interop/vhost-user.rst | 39 +++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
index c15d6ac467..96156f1900 100644
--- a/docs/interop/vhost-user.rst
+++ b/docs/interop/vhost-user.rst
@@ -350,6 +350,20 @@ Device state transfer parameters
In the future, additional phases might be added e.g. to allow
iterative migration while the device is running.
+VIRTIO Shared Memory Region configuration
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
++-------------+---------+------------+----+--------------+
+| num regions | padding | mem size 0 | .. | mem size 255 |
++-------------+---------+------------+----+--------------+
+
+:num regions: a 32-bit number of regions
+
+:padding: 32-bit
+
+:mem size: contains `num regions` 64-bit fields representing the size of each
+ VIRTIO Shared Memory Region
+
C structure
-----------
@@ -376,6 +390,7 @@ In QEMU the vhost-user message is implemented with the following struct:
VhostUserShared object;
VhostUserTransferDeviceState transfer_state;
VhostUserMMap mmap;
+ VhostUserShMemConfig shmem;
};
} QEMU_PACKED VhostUserMsg;
@@ -1733,6 +1748,30 @@ Front-end message types
Using this function requires prior negotiation of the
``VHOST_USER_PROTOCOL_F_DEVICE_STATE`` feature.
+``VHOST_USER_GET_SHMEM_CONFIG``
+ :id: 44
+ :equivalent ioctl: N/A
+ :request payload: N/A
+ :reply payload: ``struct VhostUserShMemConfig``
+
+ When the ``VHOST_USER_PROTOCOL_F_SHMEM`` protocol feature has been
+ successfully negotiated, this message can be submitted by the front-end
+ to gather the VIRTIO Shared Memory Region configuration. The back-end will
+ respond with the number of VIRTIO Shared Memory Regions it requires, and
+ each shared memory region size in an array. The shared memory IDs are
+ represented by the array index. The information returned shall comply
+ with the following rules:
+
+ * The shared information will remain valid and unchanged for the entire
+ lifetime of the connection.
+
+ * The Shared Memory Region size must be a multiple of the page size
+ supported by mmap(2).
+
+ * The size may be 0 if the region is unused. This can happen when the
+ device does not support an optional feature but does support a feature
+ that uses a higher shmid.
+
Back-end message types
----------------------
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v4 6/9] qmp: add shmem feature map
2025-02-17 16:40 [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests Albert Esteve
` (4 preceding siblings ...)
2025-02-17 16:40 ` [PATCH v4 5/9] vhost_user.rst: Add GET_SHMEM_CONFIG message Albert Esteve
@ 2025-02-17 16:40 ` Albert Esteve
2025-02-18 10:34 ` Stefan Hajnoczi
2025-02-17 16:40 ` [PATCH v4 7/9] vhost-user-devive: Add shmem BAR Albert Esteve
` (3 subsequent siblings)
9 siblings, 1 reply; 36+ messages in thread
From: Albert Esteve @ 2025-02-17 16:40 UTC (permalink / raw)
To: qemu-devel
Cc: slp, stevensd, Alex Bennée, Stefano Garzarella, stefanha,
david, hi, mst, jasowang, Albert Esteve
Add new vhost-user protocol
VHOST_USER_PROTOCOL_F_SHMEM feature to
feature map.
Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
hw/virtio/virtio-qmp.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
index 3b6377cf0d..8c2cfd0916 100644
--- a/hw/virtio/virtio-qmp.c
+++ b/hw/virtio/virtio-qmp.c
@@ -127,6 +127,9 @@ static const qmp_virtio_feature_map_t vhost_user_protocol_map[] = {
FEATURE_ENTRY(VHOST_USER_PROTOCOL_F_DEVICE_STATE, \
"VHOST_USER_PROTOCOL_F_DEVICE_STATE: Backend device state transfer "
"supported"),
+ FEATURE_ENTRY(VHOST_USER_PROTOCOL_F_SHMEM, \
+ "VHOST_USER_PROTOCOL_F_SHMEM: Backend shared memory mapping "
+ "supported"),
{ -1, "" }
};
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v4 7/9] vhost-user-devive: Add shmem BAR
2025-02-17 16:40 [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests Albert Esteve
` (5 preceding siblings ...)
2025-02-17 16:40 ` [PATCH v4 6/9] qmp: add shmem feature map Albert Esteve
@ 2025-02-17 16:40 ` Albert Esteve
2025-02-18 10:41 ` Stefan Hajnoczi
2025-02-17 16:40 ` [PATCH v4 8/9] vhost_user: Add mem_read/write backend requests Albert Esteve
` (2 subsequent siblings)
9 siblings, 1 reply; 36+ messages in thread
From: Albert Esteve @ 2025-02-17 16:40 UTC (permalink / raw)
To: qemu-devel
Cc: slp, stevensd, Alex Bennée, Stefano Garzarella, stefanha,
david, hi, mst, jasowang, Albert Esteve
Add a shmem BAR block in the vhost-user-device,
which files can be directly mapped into.
The number, shmid, and size of the VIRTIO Shared
Memory subregions is retrieved through a
get_shmem_config message sent by the
vhost-user-base module on the realize step,
after virtio_init().
By default, if VHOST_USER_PROTOCOL_F_SHMEM
feature is not supported by the backend,
there is no cache.
Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
hw/virtio/vhost-user-base.c | 47 +++++++++++++++++++++++++++++--
hw/virtio/vhost-user-device-pci.c | 36 +++++++++++++++++++++--
2 files changed, 78 insertions(+), 5 deletions(-)
diff --git a/hw/virtio/vhost-user-base.c b/hw/virtio/vhost-user-base.c
index 2bc3423326..8d4bca98a8 100644
--- a/hw/virtio/vhost-user-base.c
+++ b/hw/virtio/vhost-user-base.c
@@ -16,6 +16,7 @@
#include "hw/virtio/virtio-bus.h"
#include "hw/virtio/vhost-user-base.h"
#include "qemu/error-report.h"
+#include "migration/blocker.h"
static void vub_start(VirtIODevice *vdev)
{
@@ -271,7 +272,8 @@ static void vub_device_realize(DeviceState *dev, Error **errp)
{
VirtIODevice *vdev = VIRTIO_DEVICE(dev);
VHostUserBase *vub = VHOST_USER_BASE(dev);
- int ret;
+ uint64_t memory_sizes[VIRTIO_MAX_SHMEM_REGIONS];
+ int i, ret, nregions;
if (!vub->chardev.chr) {
error_setg(errp, "vhost-user-base: missing chardev");
@@ -314,7 +316,7 @@ static void vub_device_realize(DeviceState *dev, Error **errp)
/* Allocate queues */
vub->vqs = g_ptr_array_sized_new(vub->num_vqs);
- for (int i = 0; i < vub->num_vqs; i++) {
+ for (i = 0; i < vub->num_vqs; i++) {
g_ptr_array_add(vub->vqs,
virtio_add_queue(vdev, vub->vq_size,
vub_handle_output));
@@ -328,11 +330,50 @@ static void vub_device_realize(DeviceState *dev, Error **errp)
VHOST_BACKEND_TYPE_USER, 0, errp);
if (ret < 0) {
- do_vhost_user_cleanup(vdev, vub);
+ goto err;
+ }
+
+ ret = vub->vhost_dev.vhost_ops->vhost_get_shmem_config(&vub->vhost_dev,
+ &nregions,
+ memory_sizes,
+ errp);
+
+ if (ret < 0) {
+ goto err;
+ }
+
+ for (i = 0; i < nregions; i++) {
+ if (memory_sizes[i]) {
+ if (vub->vhost_dev.migration_blocker == NULL) {
+ error_setg(&vub->vhost_dev.migration_blocker,
+ "Migration disabled: devices with VIRTIO Shared Memory "
+ "Regions do not support migration yet.");
+ ret = migrate_add_blocker_normal(
+ &vub->vhost_dev.migration_blocker,
+ errp);
+
+ if (ret < 0) {
+ goto err;
+ }
+ }
+
+ if (memory_sizes[i] % qemu_real_host_page_size() != 0) {
+ error_setg(errp, "Shared memory %d size must be a power of 2 "
+ "no smaller than the page size", i);
+ goto err;
+ }
+
+ memory_region_init(virtio_new_shmem_region(vdev)->mr,
+ OBJECT(vdev), "vub-shm-" + i,
+ memory_sizes[i]);
+ }
}
qemu_chr_fe_set_handlers(&vub->chardev, NULL, NULL, vub_event, NULL,
dev, NULL, true);
+ return;
+err:
+ do_vhost_user_cleanup(vdev, vub);
}
static void vub_device_unrealize(DeviceState *dev)
diff --git a/hw/virtio/vhost-user-device-pci.c b/hw/virtio/vhost-user-device-pci.c
index efaf55d3dd..f215cae925 100644
--- a/hw/virtio/vhost-user-device-pci.c
+++ b/hw/virtio/vhost-user-device-pci.c
@@ -8,14 +8,18 @@
*/
#include "qemu/osdep.h"
+#include "qapi/error.h"
#include "hw/qdev-properties.h"
#include "hw/virtio/vhost-user-base.h"
#include "hw/virtio/virtio-pci.h"
+#define VIRTIO_DEVICE_PCI_SHMEM_BAR 2
+
struct VHostUserDevicePCI {
VirtIOPCIProxy parent_obj;
VHostUserBase vub;
+ MemoryRegion shmembar;
};
#define TYPE_VHOST_USER_DEVICE_PCI "vhost-user-device-pci-base"
@@ -25,10 +29,38 @@ OBJECT_DECLARE_SIMPLE_TYPE(VHostUserDevicePCI, VHOST_USER_DEVICE_PCI)
static void vhost_user_device_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
{
VHostUserDevicePCI *dev = VHOST_USER_DEVICE_PCI(vpci_dev);
- DeviceState *vdev = DEVICE(&dev->vub);
+ DeviceState *dev_state = DEVICE(&dev->vub);
+ VirtIODevice *vdev = VIRTIO_DEVICE(dev_state);
+ MemoryRegion *mr;
+ uint64_t offset = 0, shmem_size = 0;
+ int i;
vpci_dev->nvectors = 1;
- qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
+ qdev_realize(dev_state, BUS(&vpci_dev->bus), errp);
+
+ for (i = 0; i < vdev->n_shmem_regions; i++) {
+ mr = vdev->shmem_list[i].mr;
+ if (mr->size > UINT64_MAX - shmem_size) {
+ error_setg(errp, "Total shared memory required overflow");
+ return;
+ }
+ shmem_size = shmem_size + mr->size;
+ }
+ if (shmem_size) {
+ memory_region_init(&dev->shmembar, OBJECT(vpci_dev),
+ "vhost-device-pci-shmembar", shmem_size);
+ for (i = 0; i < vdev->n_shmem_regions; i++) {
+ memory_region_add_subregion(&dev->shmembar, offset, mr);
+ virtio_pci_add_shm_cap(vpci_dev, VIRTIO_DEVICE_PCI_SHMEM_BAR,
+ offset, mr->size, i);
+ offset = offset + mr->size;
+ }
+ pci_register_bar(&vpci_dev->pci_dev, VIRTIO_DEVICE_PCI_SHMEM_BAR,
+ PCI_BASE_ADDRESS_SPACE_MEMORY |
+ PCI_BASE_ADDRESS_MEM_PREFETCH |
+ PCI_BASE_ADDRESS_MEM_TYPE_64,
+ &dev->shmembar);
+ }
}
static void vhost_user_device_pci_class_init(ObjectClass *klass, void *data)
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v4 8/9] vhost_user: Add mem_read/write backend requests
2025-02-17 16:40 [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests Albert Esteve
` (6 preceding siblings ...)
2025-02-17 16:40 ` [PATCH v4 7/9] vhost-user-devive: Add shmem BAR Albert Esteve
@ 2025-02-17 16:40 ` Albert Esteve
2025-02-18 10:57 ` Stefan Hajnoczi
2025-02-17 16:40 ` [PATCH v4 9/9] vhost_user.rst: Add MEM_READ/WRITE messages Albert Esteve
2025-02-17 20:01 ` [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests David Hildenbrand
9 siblings, 1 reply; 36+ messages in thread
From: Albert Esteve @ 2025-02-17 16:40 UTC (permalink / raw)
To: qemu-devel
Cc: slp, stevensd, Alex Bennée, Stefano Garzarella, stefanha,
david, hi, mst, jasowang, Albert Esteve
With SHMEM_MAP messages, sharing descriptors between
devices will cause that these devices do not see the
mappings, and fail to access these memory regions.
To solve this, introduce MEM_READ/WRITE requests
that will get triggered as a fallback when
vhost-user memory translation fails.
MEM_READ/WRITE requests have flexible array members,
since we do not know in advance the number of bytes
in the mapped region. Therefore, we need to allow
bigger message sizes for these types, and ensure
we allocate sufficient memory for them.
Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
hw/virtio/vhost-user.c | 99 +++++++++++++++++------
subprojects/libvhost-user/libvhost-user.c | 90 +++++++++++++++++++++
subprojects/libvhost-user/libvhost-user.h | 37 +++++++++
3 files changed, 202 insertions(+), 24 deletions(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 9cc148f726..ab92905a36 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -118,6 +118,8 @@ typedef enum VhostUserBackendRequest {
VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP = 8,
VHOST_USER_BACKEND_SHMEM_MAP = 9,
VHOST_USER_BACKEND_SHMEM_UNMAP = 10,
+ VHOST_USER_BACKEND_MEM_READ = 11,
+ VHOST_USER_BACKEND_MEM_WRITE = 12,
VHOST_USER_BACKEND_MAX
} VhostUserBackendRequest;
@@ -145,6 +147,12 @@ typedef struct VhostUserShMemConfig {
uint64_t memory_sizes[VIRTIO_MAX_SHMEM_REGIONS];
} VhostUserShMemConfig;
+typedef struct VhostUserMemRWMsg {
+ uint64_t guest_address;
+ uint32_t size;
+ uint8_t data[];
+} VhostUserMemRWMsg;
+
typedef struct VhostUserLog {
uint64_t mmap_size;
uint64_t mmap_offset;
@@ -253,6 +261,7 @@ typedef union {
VhostUserTransferDeviceState transfer_state;
VhostUserMMap mmap;
VhostUserShMemConfig shmem;
+ VhostUserMemRWMsg mem_rw;
} VhostUserPayload;
typedef struct VhostUserMsg {
@@ -341,17 +350,23 @@ static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg)
return r;
}
- /* validate message size is sane */
- if (msg->hdr.size > VHOST_USER_PAYLOAD_SIZE) {
- error_report("Failed to read msg header."
- " Size %d exceeds the maximum %zu.", msg->hdr.size,
- VHOST_USER_PAYLOAD_SIZE);
- return -EPROTO;
- }
-
if (msg->hdr.size) {
p += VHOST_USER_HDR_SIZE;
size = msg->hdr.size;
+ /* validate message size is sane */
+ if (msg->hdr.size > VHOST_USER_PAYLOAD_SIZE) {
+ switch(msg->hdr.request) {
+ case VHOST_USER_BACKEND_MEM_READ:
+ case VHOST_USER_BACKEND_MEM_WRITE:
+ p = g_malloc0(size);
+ break;
+ default:
+ error_report("Failed to read msg header."
+ " Size %d exceeds the maximum %zu.",
+ size, VHOST_USER_PAYLOAD_SIZE);
+ return -EPROTO;
+ }
+ }
r = qemu_chr_fe_read_all(chr, p, size);
if (r != size) {
int saved_errno = errno;
@@ -1904,6 +1919,28 @@ vhost_user_backend_handle_shmem_unmap(struct vhost_dev *dev,
return 0;
}
+static int
+vhost_user_backend_handle_mem_read(struct vhost_dev *dev,
+ VhostUserMemRWMsg *mem_rw)
+{
+ MemTxResult result;
+ result = address_space_read(dev->vdev->dma_as, mem_rw->guest_address,
+ MEMTXATTRS_UNSPECIFIED, &mem_rw->data,
+ mem_rw->size);
+ return result;
+}
+
+static int
+vhost_user_backend_handle_mem_write(struct vhost_dev *dev,
+ VhostUserMemRWMsg *mem_rw)
+{
+ MemTxResult result;
+ result = address_space_write(dev->vdev->dma_as, mem_rw->guest_address,
+ MEMTXATTRS_UNSPECIFIED, &mem_rw->data,
+ mem_rw->size);
+ return result;
+}
+
static void close_backend_channel(struct vhost_user *u)
{
g_source_destroy(u->backend_src);
@@ -1919,7 +1956,7 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition,
struct vhost_dev *dev = opaque;
struct vhost_user *u = dev->opaque;
VhostUserHeader hdr = { 0, };
- VhostUserPayload payload = { 0, };
+ VhostUserPayload *payload = g_new0(VhostUserPayload, 1);
Error *local_err = NULL;
gboolean rc = G_SOURCE_CONTINUE;
int ret = 0;
@@ -1938,47 +1975,60 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition,
}
if (hdr.size > VHOST_USER_PAYLOAD_SIZE) {
- error_report("Failed to read msg header."
- " Size %d exceeds the maximum %zu.", hdr.size,
- VHOST_USER_PAYLOAD_SIZE);
- goto err;
+ switch (hdr.request) {
+ case VHOST_USER_BACKEND_MEM_READ:
+ case VHOST_USER_BACKEND_MEM_WRITE:
+ payload = g_malloc0(hdr.size);
+ break;
+ default:
+ error_report("Failed to read msg header."
+ " Size %d exceeds the maximum %zu.", hdr.size,
+ VHOST_USER_PAYLOAD_SIZE);
+ goto err;
+ }
}
/* Read payload */
- if (qio_channel_read_all(ioc, (char *) &payload, hdr.size, &local_err)) {
+ if (qio_channel_read_all(ioc, (char *) payload, hdr.size, &local_err)) {
error_report_err(local_err);
goto err;
}
switch (hdr.request) {
case VHOST_USER_BACKEND_IOTLB_MSG:
- ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb);
+ ret = vhost_backend_handle_iotlb_msg(dev, &payload->iotlb);
break;
case VHOST_USER_BACKEND_CONFIG_CHANGE_MSG:
ret = vhost_user_backend_handle_config_change(dev);
break;
case VHOST_USER_BACKEND_VRING_HOST_NOTIFIER_MSG:
- ret = vhost_user_backend_handle_vring_host_notifier(dev, &payload.area,
+ ret = vhost_user_backend_handle_vring_host_notifier(dev, &payload->area,
fd ? fd[0] : -1);
break;
case VHOST_USER_BACKEND_SHARED_OBJECT_ADD:
- ret = vhost_user_backend_handle_shared_object_add(dev, &payload.object);
+ 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(dev,
- &payload.object);
+ &payload->object);
break;
case VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP:
ret = vhost_user_backend_handle_shared_object_lookup(dev->opaque, ioc,
- &hdr, &payload);
+ &hdr, payload);
+ break;
case VHOST_USER_BACKEND_SHMEM_MAP:
- ret = vhost_user_backend_handle_shmem_map(dev, ioc, &hdr, &payload,
+ ret = vhost_user_backend_handle_shmem_map(dev, ioc, &hdr, payload,
fd ? fd[0] : -1, &local_err);
break;
case VHOST_USER_BACKEND_SHMEM_UNMAP:
- ret = vhost_user_backend_handle_shmem_unmap(dev, ioc, &hdr, &payload,
+ ret = vhost_user_backend_handle_shmem_unmap(dev, ioc, &hdr, payload,
&local_err);
break;
+ case VHOST_USER_BACKEND_MEM_READ:
+ ret = vhost_user_backend_handle_mem_read(dev, &payload->mem_rw);
+ break;
+ case VHOST_USER_BACKEND_MEM_WRITE:
+ ret = vhost_user_backend_handle_mem_write(dev, &payload->mem_rw);
break;
default:
error_report("Received unexpected msg type: %d.", hdr.request);
@@ -1990,10 +2040,10 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition,
* directly in their request handlers.
*/
if (hdr.flags & VHOST_USER_NEED_REPLY_MASK) {
- payload.u64 = !!ret;
- hdr.size = sizeof(payload.u64);
+ payload->u64 = !!ret;
+ hdr.size = sizeof(payload->u64);
- if (!vhost_user_send_resp(ioc, &hdr, &payload, &local_err)) {
+ if (!vhost_user_send_resp(ioc, &hdr, payload, &local_err)) {
error_report_err(local_err);
goto err;
}
@@ -2011,6 +2061,7 @@ fdcleanup:
close(fd[i]);
}
}
+ g_free(payload);
return rc;
}
diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index 034cbfdc3c..575a0af556 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -1662,6 +1662,96 @@ vu_shmem_unmap(VuDev *dev, uint8_t shmid, uint64_t shm_offset, uint64_t len)
return vu_process_message_reply(dev, &vmsg);
}
+bool
+vu_send_mem_read(VuDev *dev, uint64_t guest_addr, uint32_t size,
+ uint8_t *data)
+{
+ VhostUserMsg msg_reply;
+ VhostUserMsg msg = {
+ .request = VHOST_USER_BACKEND_MEM_READ,
+ .size = sizeof(msg.payload.mem_rw),
+ .flags = VHOST_USER_VERSION | VHOST_USER_NEED_REPLY_MASK,
+ .payload = {
+ .mem_rw = {
+ .guest_address = guest_addr,
+ .size = size,
+ }
+ }
+ };
+
+ if (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SHMEM)) {
+ return false;
+ }
+
+ pthread_mutex_lock(&dev->backend_mutex);
+ if (!vu_message_write(dev, dev->backend_fd, &msg)) {
+ goto out_err;
+ }
+
+ if (!vu_message_read_default(dev, dev->backend_fd, &msg_reply)) {
+ goto out_err;
+ }
+
+ if (msg_reply.request != msg.request) {
+ DPRINT("Received unexpected msg type. Expected %d, received %d",
+ msg.request, msg_reply.request);
+ goto out_err;
+ }
+
+ if (msg_reply.payload.mem_rw.size != size) {
+ DPRINT("Received unexpected number of bytes in the response. "
+ "Expected %d, received %d",
+ size, msg_reply.payload.mem_rw.size);
+ goto out_err;
+ }
+
+ /* TODO: It should be possible to avoid memcpy() here by receiving
+ * directly into the caller's buffer. */
+ memcpy(data, msg_reply.payload.mem_rw.data, size);
+ pthread_mutex_unlock(&dev->backend_mutex);
+ return true;
+
+out_err:
+ pthread_mutex_unlock(&dev->backend_mutex);
+ return false;
+}
+
+bool
+vu_send_mem_write(VuDev *dev, uint64_t guest_addr, uint32_t size,
+ uint8_t *data)
+{
+ VhostUserMsg msg = {
+ .request = VHOST_USER_BACKEND_MEM_WRITE,
+ .size = sizeof(msg.payload.mem_rw),
+ .flags = VHOST_USER_VERSION,
+ .payload = {
+ .mem_rw = {
+ .guest_address = guest_addr,
+ .size = size,
+ }
+ }
+ };
+ /* TODO: It should be possible to avoid memcpy() here by receiving
+ * directly into the caller's buffer. */
+ memcpy(msg.payload.mem_rw.data, data, size);
+
+ if (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SHMEM)) {
+ return false;
+ }
+
+ if (vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_REPLY_ACK)) {
+ msg.flags |= VHOST_USER_NEED_REPLY_MASK;
+ }
+
+ if (!vu_message_write(dev, dev->backend_fd, &msg)) {
+ pthread_mutex_unlock(&dev->backend_mutex);
+ return false;
+ }
+
+ /* Also unlocks the backend_mutex */
+ return vu_process_message_reply(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 e9adb836f0..57e2fb9c98 100644
--- a/subprojects/libvhost-user/libvhost-user.h
+++ b/subprojects/libvhost-user/libvhost-user.h
@@ -131,6 +131,8 @@ typedef enum VhostUserBackendRequest {
VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP = 8,
VHOST_USER_BACKEND_SHMEM_MAP = 9,
VHOST_USER_BACKEND_SHMEM_UNMAP = 10,
+ VHOST_USER_BACKEND_MEM_READ = 11,
+ VHOST_USER_BACKEND_MEM_WRITE = 12,
VHOST_USER_BACKEND_MAX
} VhostUserBackendRequest;
@@ -154,6 +156,12 @@ typedef struct VhostUserMemRegMsg {
VhostUserMemoryRegion region;
} VhostUserMemRegMsg;
+typedef struct VhostUserMemRWMsg {
+ uint64_t guest_address;
+ uint32_t size;
+ uint8_t data[];
+} VhostUserMemRWMsg;
+
typedef struct VhostUserLog {
uint64_t mmap_size;
uint64_t mmap_offset;
@@ -233,6 +241,7 @@ typedef struct VhostUserMsg {
VhostUserInflight inflight;
VhostUserShared object;
VhostUserMMap mmap;
+ VhostUserMemRWMsg mem_rw;
} payload;
int fds[VHOST_MEMORY_BASELINE_NREGIONS];
@@ -647,6 +656,34 @@ bool vu_shmem_map(VuDev *dev, uint8_t shmid, uint64_t fd_offset,
*/
bool vu_shmem_unmap(VuDev *dev, uint8_t shmid, uint64_t shm_offset,
uint64_t len);
+/**
+ * vu_send_mem_read:
+ * @dev: a VuDev context
+ * @guest_addr: guest physical address to read
+ * @size: number of bytes to read
+ * @data: head of an unitialized bytes array
+ *
+ * Reads `size` bytes of `guest_addr` in the frontend and stores
+ * them in `data`.
+ *
+ * Returns: TRUE on success, FALSE on failure.
+ */
+bool vu_send_mem_read(VuDev *dev, uint64_t guest_addr, uint32_t size,
+ uint8_t *data);
+
+/**
+ * vu_send_mem_write:
+ * @dev: a VuDev context
+ * @guest_addr: guest physical address to write
+ * @size: number of bytes to write
+ * @data: head of an array with `size` bytes to write
+ *
+ * Writes `size` bytes from `data` into `guest_addr` in the frontend.
+ *
+ * Returns: TRUE on success, FALSE on failure.
+ */
+bool vu_send_mem_write(VuDev *dev, uint64_t guest_addr, uint32_t size,
+ uint8_t *data);
/**
* vu_queue_set_notification:
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v4 9/9] vhost_user.rst: Add MEM_READ/WRITE messages
2025-02-17 16:40 [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests Albert Esteve
` (7 preceding siblings ...)
2025-02-17 16:40 ` [PATCH v4 8/9] vhost_user: Add mem_read/write backend requests Albert Esteve
@ 2025-02-17 16:40 ` Albert Esteve
2025-02-18 11:00 ` Stefan Hajnoczi
2025-02-17 20:01 ` [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests David Hildenbrand
9 siblings, 1 reply; 36+ messages in thread
From: Albert Esteve @ 2025-02-17 16:40 UTC (permalink / raw)
To: qemu-devel
Cc: slp, stevensd, Alex Bennée, Stefano Garzarella, stefanha,
david, hi, mst, jasowang, Albert Esteve
Add MEM_READ/WRITE request to the vhost-user
spec documentation.
Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
docs/interop/vhost-user.rst | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
index 96156f1900..9f7a2c4cf7 100644
--- a/docs/interop/vhost-user.rst
+++ b/docs/interop/vhost-user.rst
@@ -391,6 +391,7 @@ In QEMU the vhost-user message is implemented with the following struct:
VhostUserTransferDeviceState transfer_state;
VhostUserMMap mmap;
VhostUserShMemConfig shmem;
+ VhostUserMemRWMsg mem_rw;
};
} QEMU_PACKED VhostUserMsg;
@@ -1938,6 +1939,38 @@ is sent by the front-end.
given range shall correspond to the entirety of a valid mapped region.
A reply is generated indicating whether unmapping succeeded.
+``VHOST_USER_BACKEND_MEM_READ``
+ :id: 11
+ :equivalent ioctl: N/A
+ :request payload: ``struct VhostUserMemRWMsg``
+ :reply payload: N/A
+
+ When the ``VHOST_USER_PROTOCOL_F_SHMEM`` protocol feature has been
+ successfully negotiated, this message can be submitted by the backends to
+ read a memory region that has failed to resolve a translation due to an
+ incomplete memory table, after another device called
+ ``VHOST_USER_BACKEND_SHMEM_MAP`` for the same region on a shared
+ descriptor file.
+
+ This mechanism works as a fallback for resolving those memory
+ accesses and ensure that DMA works with Shared Memory Regions.
+
+``VHOST_USER_BACKEND_MEM_WRITE``
+ :id: 12
+ :equivalent ioctl: N/A
+ :request payload: ``struct VhostUserMemRWMsg``
+ :reply payload: N/A
+
+ When the ``VHOST_USER_PROTOCOL_F_SHMEM`` protocol feature has been
+ successfully negotiated, this message can be submitted by the backends to
+ write a memory region that has failed due to resolve a translation an
+ incomplete memory table after another device called
+ ``VHOST_USER_BACKEND_SHMEM_MAP`` for the same region on a shared
+ descriptor file.
+
+ This mechanism works as a fallback for resolving those memory
+ accesses and ensure that DMA works with Shared Memory Regions.
+
.. _reply_ack:
VHOST_USER_PROTOCOL_F_REPLY_ACK
--
2.48.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests
2025-02-17 16:40 [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests Albert Esteve
` (8 preceding siblings ...)
2025-02-17 16:40 ` [PATCH v4 9/9] vhost_user.rst: Add MEM_READ/WRITE messages Albert Esteve
@ 2025-02-17 20:01 ` David Hildenbrand
2025-02-24 8:54 ` Albert Esteve
9 siblings, 1 reply; 36+ messages in thread
From: David Hildenbrand @ 2025-02-17 20:01 UTC (permalink / raw)
To: Albert Esteve, qemu-devel
Cc: slp, stevensd, Alex Bennée, Stefano Garzarella, stefanha, hi,
mst, jasowang
On 17.02.25 17:40, Albert Esteve wrote:
> Hi all,
>
Hi,
looks like our debugging session was successfu :)
One question below.
> v3->v4
> - Change mmap strategy to use RAM blocks
> and subregions.
> - Add new bitfield to qmp feature map
> - Followed most review comments from
> last iteration.
> - Merged documentation patch again with
> this one. Makes more sense to
> review them together after all.
> - Add documentation for MEM_READ/WRITE
> messages.
>
> The goal of this patch is to support
> dynamic fd-backed memory maps initiated
> from vhost-user backends.
> There are many devices that could already
> benefit of this feature, e.g.,
> virtiofs or virtio-gpu.
>
> After receiving the SHMEM_MAP/UNMAP request,
> the frontend creates the RAMBlock form the
> fd and maps it by adding it as a subregion
> of the shared memory region container.
>
> The VIRTIO Shared Memory Region list is
> declared in the `VirtIODevice` struct
> to make it generic.
>
> TODO: There was a conversation on the
> previous version around adding tests
> to the patch (which I have acknowledged).
> However, given the numerous changes
> that the patch already has, I have
> decided to send it early and collect
> some feedback while I work on the
> tests for the next iteration.
> Given that I have been able to
> test the implementation with
> my local setup, I am more or less
> confident that, at least, the code
> is in a relatively sane state
> so that no reviewing time is
> wasted on broken patches.
>
> This patch also includes:
> - SHMEM_CONFIG frontend request that is
> specifically meant to allow generic
> vhost-user-device frontend to be able to
> query VIRTIO Shared Memory settings from the
> backend (as this device is generic and agnostic
> of the actual backend configuration).
>
> - MEM_READ/WRITE backend requests are
> added to deal with a potential issue when having
> multiple backends sharing a file descriptor.
> When a backend calls SHMEM_MAP it makes
> accessing to the region fail for other
> backend as it is missing from their translation
> table. So these requests are a fallback
> for vhost-user memory translation fails.
Can you elaborate what the issue here is?
Why would SHMEM_MAP make accessing the region fail for other backends --
what makes this missing from their translation?
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 1/9] vhost-user: Add VirtIO Shared Memory map request
2025-02-17 16:40 ` [PATCH v4 1/9] vhost-user: Add VirtIO Shared Memory map request Albert Esteve
@ 2025-02-18 6:43 ` Stefan Hajnoczi
2025-02-18 10:33 ` Albert Esteve
2025-03-06 14:48 ` Albert Esteve
2025-02-18 10:19 ` Stefan Hajnoczi
2025-02-20 10:59 ` Alyssa Ross
2 siblings, 2 replies; 36+ messages in thread
From: Stefan Hajnoczi @ 2025-02-18 6:43 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, slp, stevensd, Alex Bennée, Stefano Garzarella,
david, hi, mst, jasowang
[-- Attachment #1: Type: text/plain, Size: 22543 bytes --]
On Mon, Feb 17, 2025 at 05:40:04PM +0100, Albert Esteve wrote:
> Add SHMEM_MAP/UNMAP requests to vhost-user to
> handle VIRTIO Shared Memory mappings.
>
> This request allows backends to dynamically map
> fds into a VIRTIO Shared Memory Region indentified
> by its `shmid`. The map is performed by calling
> `memory_region_init_ram_from_fd` and adding the
> new region as a subregion of the shmem container
> MR. Then, the fd memory is advertised to the
> driver as a base addres + offset, so it can be
> read/written (depending on the mmap flags
> requested) while it is valid.
>
> The backend can unmap the memory range
> in a given VIRTIO Shared Memory Region (again,
> identified by its `shmid`), to free it.
> Upon receiving this message, the front-end
> must delete the MR as a subregion of
> the shmem container region and free its
> resources.
>
> Note that commit all these operations need
> to be delayed to after we respond the request
> to the backend to avoid deadlocks.
>
> The device model needs to create VirtSharedMemory
> instances for the VirtIO Shared Memory Regions
> and add them to the `VirtIODevice` instance.
>
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> ---
> hw/virtio/vhost-user.c | 134 ++++++++++++++++++++++
> hw/virtio/virtio.c | 81 +++++++++++++
> include/hw/virtio/virtio.h | 27 +++++
> subprojects/libvhost-user/libvhost-user.c | 70 +++++++++++
> subprojects/libvhost-user/libvhost-user.h | 55 +++++++++
> 5 files changed, 367 insertions(+)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 267b612587..d88e6f8c3c 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -115,6 +115,8 @@ typedef enum VhostUserBackendRequest {
> VHOST_USER_BACKEND_SHARED_OBJECT_ADD = 6,
> VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE = 7,
> VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP = 8,
> + VHOST_USER_BACKEND_SHMEM_MAP = 9,
> + VHOST_USER_BACKEND_SHMEM_UNMAP = 10,
> VHOST_USER_BACKEND_MAX
> } VhostUserBackendRequest;
>
> @@ -192,6 +194,24 @@ typedef struct VhostUserShared {
> unsigned char uuid[16];
> } VhostUserShared;
>
> +/* For the flags field of VhostUserMMap */
> +#define VHOST_USER_FLAG_MAP_R (1u << 0)
> +#define VHOST_USER_FLAG_MAP_W (1u << 1)
> +
> +typedef struct {
> + /* VIRTIO Shared Memory Region ID */
> + uint8_t shmid;
> + uint8_t padding[7];
> + /* File offset */
> + uint64_t fd_offset;
> + /* Offset within the VIRTIO Shared Memory Region */
> + uint64_t shm_offset;
> + /* Size of the mapping */
> + uint64_t len;
> + /* Flags for the mmap operation, from VHOST_USER_FLAG_* */
> + uint64_t flags;
> +} VhostUserMMap;
> +
> typedef struct {
> VhostUserRequest request;
>
> @@ -224,6 +244,7 @@ typedef union {
> VhostUserInflight inflight;
> VhostUserShared object;
> VhostUserTransferDeviceState transfer_state;
> + VhostUserMMap mmap;
> } VhostUserPayload;
>
> typedef struct VhostUserMsg {
> @@ -1770,6 +1791,111 @@ vhost_user_backend_handle_shared_object_lookup(struct vhost_user *u,
> return 0;
> }
>
> +static int
> +vhost_user_backend_handle_shmem_map(struct vhost_dev *dev,
> + QIOChannel *ioc,
> + VhostUserHeader *hdr,
> + VhostUserPayload *payload,
> + int fd, Error **errp)
> +{
> + VirtSharedMemory *shmem = NULL;
> + VhostUserMMap *vu_mmap = &payload->mmap;
> + g_autoptr(GString) shm_name = g_string_new(NULL);
> +
> + if (fd < 0) {
> + error_report("Bad fd for map");
> + return -EBADF;
> + }
> +
> + if (!dev->vdev->shmem_list ||
> + dev->vdev->n_shmem_regions <= vu_mmap->shmid) {
> + error_report("Device only has %d VIRTIO Shared Memory Regions. "
> + "Requested ID: %d",
> + dev->vdev->n_shmem_regions, vu_mmap->shmid);
> + return -EFAULT;
> + }
> +
> + shmem = &dev->vdev->shmem_list[vu_mmap->shmid];
> +
> + if (!shmem) {
> + error_report("VIRTIO Shared Memory Region at "
> + "ID %d unitialized", vu_mmap->shmid);
> + return -EFAULT;
> + }
> +
> + if ((vu_mmap->shm_offset + vu_mmap->len) < vu_mmap->len ||
> + (vu_mmap->shm_offset + vu_mmap->len) > shmem->mr->size) {
> + error_report("Bad offset/len for mmap %" PRIx64 "+%" PRIx64,
> + vu_mmap->shm_offset, vu_mmap->len);
> + return -EFAULT;
> + }
> +
> + g_string_printf(shm_name, "virtio-shm%i-%lu",
> + vu_mmap->shmid, vu_mmap->shm_offset);
> +
> + memory_region_transaction_begin();
> + virtio_add_shmem_map(shmem, shm_name->str, vu_mmap->shm_offset,
> + vu_mmap->fd_offset, vu_mmap->len, fd, errp);
Error handling is missing. In particular, if virtio_add_shmem_map() sets
errp due to an error, then vhost_user_send_resp() will crash if it also
needs to set the error. An Error object can only be filled in once.
> +
> + hdr->size = sizeof(payload->u64);
> + vhost_user_send_resp(ioc, hdr, payload, errp);
payload->u64 has not been initialized. It contains the contents of
payload->mmap, which is not the u64 return value that must be sent here.
I think something like the following is necessary:
payload->u64 = 0;
> + memory_region_transaction_commit();
> +
> + return 0;
> +}
> +
> +static int
> +vhost_user_backend_handle_shmem_unmap(struct vhost_dev *dev,
> + QIOChannel *ioc,
> + VhostUserHeader *hdr,
> + VhostUserPayload *payload,
> + Error **errp)
> +{
> + VirtSharedMemory *shmem = NULL;
> + MappedMemoryRegion *mmap = NULL;
> + VhostUserMMap *vu_mmap = &payload->mmap;
> +
> + if (!dev->vdev->shmem_list ||
> + dev->vdev->n_shmem_regions <= vu_mmap->shmid) {
> + error_report("Device only has %d VIRTIO Shared Memory Regions. "
> + "Requested ID: %d",
> + dev->vdev->n_shmem_regions, vu_mmap->shmid);
> + return -EFAULT;
> + }
> +
> + shmem = &dev->vdev->shmem_list[vu_mmap->shmid];
> +
> + if (!shmem) {
> + error_report("VIRTIO Shared Memory Region at "
> + "ID %d unitialized", vu_mmap->shmid);
> + return -EFAULT;
> + }
> +
> + if ((vu_mmap->shm_offset + vu_mmap->len) < vu_mmap->len ||
> + (vu_mmap->shm_offset + vu_mmap->len) > shmem->mr->size) {
> + error_report("Bad offset/len for unmmap %" PRIx64 "+%" PRIx64,
> + vu_mmap->shm_offset, vu_mmap->len);
> + return -EFAULT;
> + }
> +
> + mmap = virtio_find_shmem_map(shmem, vu_mmap->shm_offset, vu_mmap->len);
> + if (!mmap) {
> + return -EFAULT;
> + }
> +
> + memory_region_transaction_begin();
> + memory_region_del_subregion(shmem->mr, mmap->mem);
> +
> + hdr->size = sizeof(payload->u64);
> + vhost_user_send_resp(ioc, hdr, payload, errp);
Same uninitialized payload->u64 issue here.
> + memory_region_transaction_commit();
> +
> + /* Free the MemoryRegion only after vhost_commit */
> + virtio_del_shmem_map(shmem, vu_mmap->shm_offset, vu_mmap->len);
> +
> + return 0;
> +}
> +
> static void close_backend_channel(struct vhost_user *u)
> {
> g_source_destroy(u->backend_src);
> @@ -1837,6 +1963,14 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition,
> case VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP:
> ret = vhost_user_backend_handle_shared_object_lookup(dev->opaque, ioc,
> &hdr, &payload);
> + case VHOST_USER_BACKEND_SHMEM_MAP:
> + ret = vhost_user_backend_handle_shmem_map(dev, ioc, &hdr, &payload,
> + fd ? fd[0] : -1, &local_err);
> + break;
> + case VHOST_USER_BACKEND_SHMEM_UNMAP:
> + ret = vhost_user_backend_handle_shmem_unmap(dev, ioc, &hdr, &payload,
> + &local_err);
> + break;
> break;
> default:
> error_report("Received unexpected msg type: %d.", hdr.request);
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 85110bce37..47d0ddb820 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3063,6 +3063,75 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f)
> return vmstate_save_state(f, &vmstate_virtio, vdev, NULL);
> }
>
> +VirtSharedMemory *virtio_new_shmem_region(VirtIODevice *vdev)
> +{
> + ++vdev->n_shmem_regions;
> + vdev->shmem_list = g_renew(VirtSharedMemory, vdev->shmem_list,
> + vdev->n_shmem_regions);
> + vdev->shmem_list[vdev->n_shmem_regions - 1].mr = g_new0(MemoryRegion, 1);
> + QTAILQ_INIT(&vdev->shmem_list[vdev->n_shmem_regions - 1].mmaps);
QTAILQ cannot be used inside a struct that is reallocated using
g_renew() because a dangling tql_prev pointer will be left after
reallocation. From QTAILQ_INIT():
(head)->tqh_circ.tql_prev = &(head)->tqh_circ; <--- not realloc-safe
Instead of using g_renew() on an array, consider using a list from
"qemu/queue.h". Lookup by shmid becomes O(n) instead of O(1), but it
doesn't matter in practice since existing devices have a small number of
VIRTIO Shared Memory Regions.
> + return &vdev->shmem_list[vdev->n_shmem_regions - 1];
> +}
> +
> +void virtio_add_shmem_map(VirtSharedMemory *shmem, const char *shm_name,
> + hwaddr shm_offset, hwaddr fd_offset, uint64_t size,
> + int fd, Error **errp)
> +{
> + MappedMemoryRegion *mmap;
> + fd = dup(fd);
> + if (fd < 0) {
> + error_setg_errno(errp, errno, "Failed to duplicate fd");
> + return;
> + }
> +
> + if (shm_offset + size > shmem->mr->size) {
> + error_setg(errp, "Memory exceeds the shared memory boundaries");
> + return;
fd is leaked here.
> + }
> +
> + mmap = g_new0(MappedMemoryRegion, 1);
> + mmap->mem = g_new0(MemoryRegion, 1);
> + mmap->offset = shm_offset;
> + if (!memory_region_init_ram_from_fd(mmap->mem,
> + OBJECT(shmem->mr),
> + shm_name, size, RAM_SHARED,
> + fd, fd_offset, errp)) {
> + error_setg(errp, "Failed to mmap region %s", shm_name);
> + close(fd);
> + g_free(mmap->mem);
> + g_free(mmap);
> + return;
> + }
> + memory_region_add_subregion(shmem->mr, shm_offset, mmap->mem);
> +
> + QTAILQ_INSERT_TAIL(&shmem->mmaps, mmap, link);
> +}
> +
> +MappedMemoryRegion *virtio_find_shmem_map(VirtSharedMemory *shmem,
> + hwaddr offset, uint64_t size)
> +{
> + MappedMemoryRegion *mmap;
> + QTAILQ_FOREACH(mmap, &shmem->mmaps, link) {
> + if (mmap->offset == offset && mmap->mem->size == size) {
> + return mmap;
> + }
> + }
> + return NULL;
> +}
> +
> +void virtio_del_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
> + uint64_t size)
> +{
> + MappedMemoryRegion *mmap = virtio_find_shmem_map(shmem, offset, size);
> + if (mmap == NULL) {
> + return;
> + }
> +
> + object_unparent(OBJECT(mmap->mem));
> + QTAILQ_REMOVE(&shmem->mmaps, mmap, link);
> + g_free(mmap);
> +}
> +
> /* A wrapper for use as a VMState .put function */
> static int virtio_device_put(QEMUFile *f, void *opaque, size_t size,
> const VMStateField *field, JSONWriter *vmdesc)
> @@ -3492,6 +3561,8 @@ void virtio_init(VirtIODevice *vdev, uint16_t device_id, size_t config_size)
> virtio_vmstate_change, vdev);
> vdev->device_endian = virtio_default_endian();
> vdev->use_guest_notifier_mask = true;
> + vdev->shmem_list = NULL;
> + vdev->n_shmem_regions = 0;
> }
>
> /*
> @@ -4005,11 +4076,21 @@ static void virtio_device_free_virtqueues(VirtIODevice *vdev)
> static void virtio_device_instance_finalize(Object *obj)
> {
> VirtIODevice *vdev = VIRTIO_DEVICE(obj);
> + VirtSharedMemory *shmem = NULL;
> + int i;
>
> virtio_device_free_virtqueues(vdev);
>
> g_free(vdev->config);
> g_free(vdev->vector_queues);
> + for (i = 0; i< vdev->n_shmem_regions; i++) {
> + shmem = &vdev->shmem_list[i];
> + while (!QTAILQ_EMPTY(&shmem->mmaps)) {
> + MappedMemoryRegion *mmap_reg = QTAILQ_FIRST(&shmem->mmaps);
> + QTAILQ_REMOVE(&shmem->mmaps, mmap_reg, link);
> + g_free(mmap_reg);
Is it possible to reuse virtio_del_shmem_map() to avoid code duplication
and inconsistencies?
while (!QTAILQ_EMPTY(&shmem->mmaps)) {
MappedMemoryRegion *mmap_reg = QTAILQ_FIRST(&shmem->mmaps);
virtio_del_shmem_map(shmem, mmap_reg->offset, mmap_reg->mem->size);
}
> + }
> + }
Missing g_free(vdev->shmem_list).
> }
>
> static const Property virtio_properties[] = {
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 6386910280..a778547c79 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -98,6 +98,21 @@ enum virtio_device_endian {
> VIRTIO_DEVICE_ENDIAN_BIG,
> };
>
> +struct MappedMemoryRegion {
> + MemoryRegion *mem;
> + hwaddr offset;
> + QTAILQ_ENTRY(MappedMemoryRegion) link;
> +};
> +
> +typedef struct MappedMemoryRegion MappedMemoryRegion;
> +
> +struct VirtSharedMemory {
> + MemoryRegion *mr;
> + QTAILQ_HEAD(, MappedMemoryRegion) mmaps;
> +};
> +
> +typedef struct VirtSharedMemory VirtSharedMemory;
> +
> /**
> * struct VirtIODevice - common VirtIO structure
> * @name: name of the device
> @@ -167,6 +182,9 @@ struct VirtIODevice
> */
> EventNotifier config_notifier;
> bool device_iotlb_enabled;
> + /* Shared memory region for vhost-user mappings. */
This is core VIRTIO code and Shared Memory Regions are a VIRTIO concept.
It is possible that built-in (non-vhost) devices might also add their
shared memory regions with virtio_new_shmem_region(), so let's not talk
about vhost-user specifically.
> + VirtSharedMemory *shmem_list;
> + int n_shmem_regions;
> };
>
> struct VirtioDeviceClass {
> @@ -289,6 +307,15 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
>
> int virtio_save(VirtIODevice *vdev, QEMUFile *f);
>
> +VirtSharedMemory *virtio_new_shmem_region(VirtIODevice *vdev);
> +void virtio_add_shmem_map(VirtSharedMemory *shmem, const char *shm_name,
> + hwaddr shm_offset, hwaddr fd_offset, uint64_t size,
> + int fd, Error **errp);
> +MappedMemoryRegion *virtio_find_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
> + uint64_t size);
> +void virtio_del_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
> + uint64_t size);
> +
> extern const VMStateInfo virtio_vmstate_info;
>
> #define VMSTATE_VIRTIO_DEVICE \
> diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> index 9c630c2170..034cbfdc3c 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -1592,6 +1592,76 @@ vu_rm_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN])
> return vu_send_message(dev, &msg);
> }
>
> +bool
> +vu_shmem_map(VuDev *dev, uint8_t shmid, uint64_t fd_offset,
> + uint64_t shm_offset, uint64_t len, uint64_t flags, int fd)
> +{
> + VhostUserMsg vmsg = {
> + .request = VHOST_USER_BACKEND_SHMEM_MAP,
> + .size = sizeof(vmsg.payload.mmap),
> + .flags = VHOST_USER_VERSION,
> + .payload.mmap = {
> + .shmid = shmid,
> + .fd_offset = fd_offset,
> + .shm_offset = shm_offset,
> + .len = len,
> + .flags = flags,
> + },
> + .fd_num = 1,
> + .fds[0] = fd,
> + };
> +
> + if (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SHMEM)) {
> + return false;
> + }
> +
> + if (vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_REPLY_ACK)) {
> + vmsg.flags |= VHOST_USER_NEED_REPLY_MASK;
> + }
Setting this flag is inconsistent with
vhost_user_backend_handle_shmem_map() and
vhost_user_backend_handle_shmem_unmap(). They call
vhost_user_send_resp() explicitly instead of relying on backend_read()
to send the return value when VHOST_USER_NEED_REPLY_MASK is set. This
inconsistency means that a successful return message will be sent twice
when VHOST_USER_NEED_REPLY_MASK is set.
Either these new vhost-user messages could be specified with a mandatory
reply payload or the patch could be updated to avoid the explicit
vhost_user_send_resp() calls. I think the latter is more commonly used
by other vhost-user messages, so it's probably best to do it the same
way.
> +
> + pthread_mutex_lock(&dev->backend_mutex);
> + if (!vu_message_write(dev, dev->backend_fd, &vmsg)) {
> + pthread_mutex_unlock(&dev->backend_mutex);
> + return false;
> + }
> +
> + /* Also unlocks the backend_mutex */
> + return vu_process_message_reply(dev, &vmsg);
> +}
> +
> +bool
> +vu_shmem_unmap(VuDev *dev, uint8_t shmid, uint64_t shm_offset, uint64_t len)
> +{
> + VhostUserMsg vmsg = {
> + .request = VHOST_USER_BACKEND_SHMEM_UNMAP,
> + .size = sizeof(vmsg.payload.mmap),
> + .flags = VHOST_USER_VERSION,
> + .payload.mmap = {
> + .shmid = shmid,
> + .fd_offset = 0,
> + .shm_offset = shm_offset,
> + .len = len,
> + },
> + };
> +
> + if (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SHMEM)) {
> + return false;
> + }
> +
> + if (vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_REPLY_ACK)) {
> + vmsg.flags |= VHOST_USER_NEED_REPLY_MASK;
> + }
> +
> + pthread_mutex_lock(&dev->backend_mutex);
> + if (!vu_message_write(dev, dev->backend_fd, &vmsg)) {
> + pthread_mutex_unlock(&dev->backend_mutex);
> + return false;
> + }
> +
> + /* Also unlocks the backend_mutex */
> + return vu_process_message_reply(dev, &vmsg);
> +}
> +
> 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 2ffc58c11b..e9adb836f0 100644
> --- a/subprojects/libvhost-user/libvhost-user.h
> +++ b/subprojects/libvhost-user/libvhost-user.h
> @@ -69,6 +69,8 @@ enum VhostUserProtocolFeature {
> /* Feature 16 is reserved for VHOST_USER_PROTOCOL_F_STATUS. */
> /* Feature 17 reserved for VHOST_USER_PROTOCOL_F_XEN_MMAP. */
> VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 18,
> + /* Feature 19 is reserved for VHOST_USER_PROTOCOL_F_DEVICE_STATE */
> + VHOST_USER_PROTOCOL_F_SHMEM = 20,
> VHOST_USER_PROTOCOL_F_MAX
> };
>
> @@ -127,6 +129,8 @@ typedef enum VhostUserBackendRequest {
> VHOST_USER_BACKEND_SHARED_OBJECT_ADD = 6,
> VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE = 7,
> VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP = 8,
> + VHOST_USER_BACKEND_SHMEM_MAP = 9,
> + VHOST_USER_BACKEND_SHMEM_UNMAP = 10,
> VHOST_USER_BACKEND_MAX
> } VhostUserBackendRequest;
>
> @@ -186,6 +190,24 @@ typedef struct VhostUserShared {
> unsigned char uuid[UUID_LEN];
> } VhostUserShared;
>
> +/* For the flags field of VhostUserMMap */
> +#define VHOST_USER_FLAG_MAP_R (1u << 0)
> +#define VHOST_USER_FLAG_MAP_W (1u << 1)
> +
> +typedef struct {
> + /* VIRTIO Shared Memory Region ID */
> + uint8_t shmid;
> + uint8_t padding[7];
> + /* File offset */
> + uint64_t fd_offset;
> + /* Offset within the VIRTIO Shared Memory Region */
> + uint64_t shm_offset;
> + /* Size of the mapping */
> + uint64_t len;
> + /* Flags for the mmap operation, from VHOST_USER_FLAG_* */
> + uint64_t flags;
> +} VhostUserMMap;
> +
> #define VU_PACKED __attribute__((packed))
>
> typedef struct VhostUserMsg {
> @@ -210,6 +232,7 @@ typedef struct VhostUserMsg {
> VhostUserVringArea area;
> VhostUserInflight inflight;
> VhostUserShared object;
> + VhostUserMMap mmap;
> } payload;
>
> int fds[VHOST_MEMORY_BASELINE_NREGIONS];
> @@ -593,6 +616,38 @@ bool vu_add_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN]);
> */
> bool vu_rm_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN]);
>
> +/**
> + * vu_shmem_map:
> + * @dev: a VuDev context
> + * @shmid: VIRTIO Shared Memory Region ID
> + * @fd_offset: File offset
> + * @shm_offset: Offset within the VIRTIO Shared Memory Region
> + * @len: Size of the mapping
> + * @flags: Flags for the mmap operation
> + * @fd: A file descriptor
> + *
> + * Advertises a new mapping to be made in a given VIRTIO Shared Memory Region.
> + *
> + * Returns: TRUE on success, FALSE on failure.
> + */
> +bool vu_shmem_map(VuDev *dev, uint8_t shmid, uint64_t fd_offset,
> + uint64_t shm_offset, uint64_t len, uint64_t flags, int fd);
> +
> +/**
> + * vu_shmem_unmap:
> + * @dev: a VuDev context
> + * @shmid: VIRTIO Shared Memory Region ID
> + * @fd_offset: File offset
> + * @len: Size of the mapping
> + *
> + * The front-end un-mmaps a given range in the VIRTIO Shared Memory Region
> + * with the requested `shmid`.
> + *
> + * Returns: TRUE on success, FALSE on failure.
> + */
> +bool vu_shmem_unmap(VuDev *dev, uint8_t shmid, uint64_t shm_offset,
> + uint64_t len);
> +
> /**
> * vu_queue_set_notification:
> * @dev: a VuDev context
> --
> 2.48.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 2/9] vhost_user.rst: Align VhostUserMsg excerpt members
2025-02-17 16:40 ` [PATCH v4 2/9] vhost_user.rst: Align VhostUserMsg excerpt members Albert Esteve
@ 2025-02-18 6:44 ` Stefan Hajnoczi
0 siblings, 0 replies; 36+ messages in thread
From: Stefan Hajnoczi @ 2025-02-18 6:44 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, slp, stevensd, Alex Bennée, Stefano Garzarella,
david, hi, mst, jasowang
[-- Attachment #1: Type: text/plain, Size: 351 bytes --]
On Mon, Feb 17, 2025 at 05:40:05PM +0100, Albert Esteve wrote:
> Add missing members to the VhostUserMsg excerpt in
> the vhost-user spec documentation.
>
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> ---
> docs/interop/vhost-user.rst | 4 ++++
> 1 file changed, 4 insertions(+)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 1/9] vhost-user: Add VirtIO Shared Memory map request
2025-02-17 16:40 ` [PATCH v4 1/9] vhost-user: Add VirtIO Shared Memory map request Albert Esteve
2025-02-18 6:43 ` Stefan Hajnoczi
@ 2025-02-18 10:19 ` Stefan Hajnoczi
2025-02-20 10:59 ` Alyssa Ross
2 siblings, 0 replies; 36+ messages in thread
From: Stefan Hajnoczi @ 2025-02-18 10:19 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, slp, stevensd, Alex Bennée, Stefano Garzarella,
david, hi, mst, jasowang
[-- Attachment #1: Type: text/plain, Size: 1127 bytes --]
On Mon, Feb 17, 2025 at 05:40:04PM +0100, Albert Esteve wrote:
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 85110bce37..47d0ddb820 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3063,6 +3063,75 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f)
> return vmstate_save_state(f, &vmstate_virtio, vdev, NULL);
> }
>
> +VirtSharedMemory *virtio_new_shmem_region(VirtIODevice *vdev)
> +{
> + ++vdev->n_shmem_regions;
> + vdev->shmem_list = g_renew(VirtSharedMemory, vdev->shmem_list,
> + vdev->n_shmem_regions);
> + vdev->shmem_list[vdev->n_shmem_regions - 1].mr = g_new0(MemoryRegion, 1);
> + QTAILQ_INIT(&vdev->shmem_list[vdev->n_shmem_regions - 1].mmaps);
> + return &vdev->shmem_list[vdev->n_shmem_regions - 1];
> +}
On second thought, it would be easier to change shmem_list's type to
VirtSharedMemory** and g_renew(VirtSharedMemory*, ...). That way the
array approach can be kept without worrying about reallocating the
VirtSharedMemory structs themselves. Only the array of pointers is
reallocated.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 4/9] vhost_user: Add frontend get_shmem_config command
2025-02-17 16:40 ` [PATCH v4 4/9] vhost_user: Add frontend get_shmem_config command Albert Esteve
@ 2025-02-18 10:27 ` Stefan Hajnoczi
0 siblings, 0 replies; 36+ messages in thread
From: Stefan Hajnoczi @ 2025-02-18 10:27 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, slp, stevensd, Alex Bennée, Stefano Garzarella,
david, hi, mst, jasowang
[-- Attachment #1: Type: text/plain, Size: 5987 bytes --]
On Mon, Feb 17, 2025 at 05:40:07PM +0100, Albert Esteve wrote:
> The frontend can use this command to retrieve
> VirtIO Shared Memory Regions configuration from
> the backend. The response contains the number of
> shared memory regions, their size, and shmid.
>
> This is useful when the frontend is unaware of
> specific backend type and configuration,
> for example, in the `vhost-user-device` case.
>
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> ---
> hw/virtio/vhost-user.c | 43 +++++++++++++++++++++++++++++++
> include/hw/virtio/vhost-backend.h | 9 +++++++
> include/hw/virtio/vhost-user.h | 1 +
> include/hw/virtio/virtio.h | 2 ++
> 4 files changed, 55 insertions(+)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index d88e6f8c3c..9cc148f726 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -104,6 +104,7 @@ typedef enum VhostUserRequest {
> VHOST_USER_GET_SHARED_OBJECT = 41,
> VHOST_USER_SET_DEVICE_STATE_FD = 42,
> VHOST_USER_CHECK_DEVICE_STATE = 43,
> + VHOST_USER_GET_SHMEM_CONFIG = 44,
> VHOST_USER_MAX
> } VhostUserRequest;
>
> @@ -138,6 +139,12 @@ typedef struct VhostUserMemRegMsg {
> VhostUserMemoryRegion region;
> } VhostUserMemRegMsg;
>
> +typedef struct VhostUserShMemConfig {
> + uint32_t nregions;
> + uint32_t padding;
> + uint64_t memory_sizes[VIRTIO_MAX_SHMEM_REGIONS];
> +} VhostUserShMemConfig;
> +
> typedef struct VhostUserLog {
> uint64_t mmap_size;
> uint64_t mmap_offset;
> @@ -245,6 +252,7 @@ typedef union {
> VhostUserShared object;
> VhostUserTransferDeviceState transfer_state;
> VhostUserMMap mmap;
> + VhostUserShMemConfig shmem;
> } VhostUserPayload;
>
> typedef struct VhostUserMsg {
> @@ -3146,6 +3154,40 @@ static int vhost_user_check_device_state(struct vhost_dev *dev, Error **errp)
> return 0;
> }
>
> +static int vhost_user_get_shmem_config(struct vhost_dev *dev,
> + int *nregions,
> + uint64_t *memory_sizes,
> + Error **errp)
> +{
> + int ret;
> + VhostUserMsg msg = {
> + .hdr.request = VHOST_USER_GET_SHMEM_CONFIG,
> + .hdr.flags = VHOST_USER_VERSION,
> + };
> +
> + if (!virtio_has_feature(dev->protocol_features,
> + VHOST_USER_PROTOCOL_F_SHMEM)) {
> + return 0;
> + }
> +
> + ret = vhost_user_write(dev, &msg, NULL, 0);
> + if (ret < 0) {
> + return ret;
> + }
> +
> + ret = vhost_user_read(dev, &msg);
> + if (ret < 0) {
> + return ret;
> + }
> +
> + assert(msg.payload.shmem.nregions <= VIRTIO_MAX_SHMEM_REGIONS);
> + *nregions = msg.payload.shmem.nregions;
> + memcpy(memory_sizes,
> + &msg.payload.shmem.memory_sizes,
> + sizeof(uint64_t) * VHOST_MEMORY_BASELINE_NREGIONS);
Should this be VIRTIO_MAX_SHMEM_REGIONS instead of
VHOST_MEMORY_BASELINE_NREGIONS?
> + return 0;
> +}
> +
> const VhostOps user_ops = {
> .backend_type = VHOST_BACKEND_TYPE_USER,
> .vhost_backend_init = vhost_user_backend_init,
> @@ -3184,4 +3226,5 @@ const VhostOps user_ops = {
> .vhost_supports_device_state = vhost_user_supports_device_state,
> .vhost_set_device_state_fd = vhost_user_set_device_state_fd,
> .vhost_check_device_state = vhost_user_check_device_state,
> + .vhost_get_shmem_config = vhost_user_get_shmem_config,
> };
> diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
> index 70c2e8ffee..b40d82a111 100644
> --- a/include/hw/virtio/vhost-backend.h
> +++ b/include/hw/virtio/vhost-backend.h
> @@ -159,6 +159,14 @@ typedef int (*vhost_set_device_state_fd_op)(struct vhost_dev *dev,
> int *reply_fd,
> Error **errp);
> typedef int (*vhost_check_device_state_op)(struct vhost_dev *dev, Error **errp);
> +/*
> + * Max regions is VIRTIO_MAX_SHMEM_REGIONS, so that is the maximum
> + * number of memory_sizes that will be accepted. */
> +typedef int (*vhost_get_shmem_config_op)(struct vhost_dev *dev,
> + int *nregions,
> + uint64_t *memory_sizes,
> + Error **errp);
> +
>
> typedef struct VhostOps {
> VhostBackendType backend_type;
> @@ -214,6 +222,7 @@ typedef struct VhostOps {
> vhost_supports_device_state_op vhost_supports_device_state;
> vhost_set_device_state_fd_op vhost_set_device_state_fd;
> vhost_check_device_state_op vhost_check_device_state;
> + vhost_get_shmem_config_op vhost_get_shmem_config;
> } VhostOps;
>
> int vhost_backend_update_device_iotlb(struct vhost_dev *dev,
> diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
> index 9a3f238b43..bacc7d184c 100644
> --- a/include/hw/virtio/vhost-user.h
> +++ b/include/hw/virtio/vhost-user.h
> @@ -32,6 +32,7 @@ enum VhostUserProtocolFeature {
> /* Feature 17 reserved for VHOST_USER_PROTOCOL_F_XEN_MMAP. */
> VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 18,
> VHOST_USER_PROTOCOL_F_DEVICE_STATE = 19,
> + VHOST_USER_PROTOCOL_F_SHMEM = 20,
> VHOST_USER_PROTOCOL_F_MAX
> };
>
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index a778547c79..319e2f5b06 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -81,6 +81,8 @@ typedef struct VirtQueueElement
>
> #define VIRTIO_NO_VECTOR 0xffff
>
> +#define VIRTIO_MAX_SHMEM_REGIONS 256
> +
> /* special index value used internally for config irqs */
> #define VIRTIO_CONFIG_IRQ_IDX -1
>
> --
> 2.48.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 5/9] vhost_user.rst: Add GET_SHMEM_CONFIG message
2025-02-17 16:40 ` [PATCH v4 5/9] vhost_user.rst: Add GET_SHMEM_CONFIG message Albert Esteve
@ 2025-02-18 10:33 ` Stefan Hajnoczi
0 siblings, 0 replies; 36+ messages in thread
From: Stefan Hajnoczi @ 2025-02-18 10:33 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, slp, stevensd, Alex Bennée, Stefano Garzarella,
david, hi, mst, jasowang
[-- Attachment #1: Type: text/plain, Size: 419 bytes --]
On Mon, Feb 17, 2025 at 05:40:08PM +0100, Albert Esteve wrote:
> Add GET_SHMEM_CONFIG vhost-user frontend
> message to the spec documentation.
>
> Reviewed-by: Alyssa Ross <hi@alyssa.is>
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> ---
> docs/interop/vhost-user.rst | 39 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 39 insertions(+)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 1/9] vhost-user: Add VirtIO Shared Memory map request
2025-02-18 6:43 ` Stefan Hajnoczi
@ 2025-02-18 10:33 ` Albert Esteve
2025-03-06 14:48 ` Albert Esteve
1 sibling, 0 replies; 36+ messages in thread
From: Albert Esteve @ 2025-02-18 10:33 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, slp, stevensd, Alex Bennée, Stefano Garzarella,
david, hi, mst, jasowang
On Tue, Feb 18, 2025 at 7:43 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Mon, Feb 17, 2025 at 05:40:04PM +0100, Albert Esteve wrote:
> > Add SHMEM_MAP/UNMAP requests to vhost-user to
> > handle VIRTIO Shared Memory mappings.
> >
> > This request allows backends to dynamically map
> > fds into a VIRTIO Shared Memory Region indentified
> > by its `shmid`. The map is performed by calling
> > `memory_region_init_ram_from_fd` and adding the
> > new region as a subregion of the shmem container
> > MR. Then, the fd memory is advertised to the
> > driver as a base addres + offset, so it can be
> > read/written (depending on the mmap flags
> > requested) while it is valid.
> >
> > The backend can unmap the memory range
> > in a given VIRTIO Shared Memory Region (again,
> > identified by its `shmid`), to free it.
> > Upon receiving this message, the front-end
> > must delete the MR as a subregion of
> > the shmem container region and free its
> > resources.
> >
> > Note that commit all these operations need
> > to be delayed to after we respond the request
> > to the backend to avoid deadlocks.
> >
> > The device model needs to create VirtSharedMemory
> > instances for the VirtIO Shared Memory Regions
> > and add them to the `VirtIODevice` instance.
> >
> > Signed-off-by: Albert Esteve <aesteve@redhat.com>
> > ---
> > hw/virtio/vhost-user.c | 134 ++++++++++++++++++++++
> > hw/virtio/virtio.c | 81 +++++++++++++
> > include/hw/virtio/virtio.h | 27 +++++
> > subprojects/libvhost-user/libvhost-user.c | 70 +++++++++++
> > subprojects/libvhost-user/libvhost-user.h | 55 +++++++++
> > 5 files changed, 367 insertions(+)
> >
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index 267b612587..d88e6f8c3c 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -115,6 +115,8 @@ typedef enum VhostUserBackendRequest {
> > VHOST_USER_BACKEND_SHARED_OBJECT_ADD = 6,
> > VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE = 7,
> > VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP = 8,
> > + VHOST_USER_BACKEND_SHMEM_MAP = 9,
> > + VHOST_USER_BACKEND_SHMEM_UNMAP = 10,
> > VHOST_USER_BACKEND_MAX
> > } VhostUserBackendRequest;
> >
> > @@ -192,6 +194,24 @@ typedef struct VhostUserShared {
> > unsigned char uuid[16];
> > } VhostUserShared;
> >
> > +/* For the flags field of VhostUserMMap */
> > +#define VHOST_USER_FLAG_MAP_R (1u << 0)
> > +#define VHOST_USER_FLAG_MAP_W (1u << 1)
> > +
> > +typedef struct {
> > + /* VIRTIO Shared Memory Region ID */
> > + uint8_t shmid;
> > + uint8_t padding[7];
> > + /* File offset */
> > + uint64_t fd_offset;
> > + /* Offset within the VIRTIO Shared Memory Region */
> > + uint64_t shm_offset;
> > + /* Size of the mapping */
> > + uint64_t len;
> > + /* Flags for the mmap operation, from VHOST_USER_FLAG_* */
> > + uint64_t flags;
> > +} VhostUserMMap;
> > +
> > typedef struct {
> > VhostUserRequest request;
> >
> > @@ -224,6 +244,7 @@ typedef union {
> > VhostUserInflight inflight;
> > VhostUserShared object;
> > VhostUserTransferDeviceState transfer_state;
> > + VhostUserMMap mmap;
> > } VhostUserPayload;
> >
> > typedef struct VhostUserMsg {
> > @@ -1770,6 +1791,111 @@ vhost_user_backend_handle_shared_object_lookup(struct vhost_user *u,
> > return 0;
> > }
> >
> > +static int
> > +vhost_user_backend_handle_shmem_map(struct vhost_dev *dev,
> > + QIOChannel *ioc,
> > + VhostUserHeader *hdr,
> > + VhostUserPayload *payload,
> > + int fd, Error **errp)
> > +{
> > + VirtSharedMemory *shmem = NULL;
> > + VhostUserMMap *vu_mmap = &payload->mmap;
> > + g_autoptr(GString) shm_name = g_string_new(NULL);
> > +
> > + if (fd < 0) {
> > + error_report("Bad fd for map");
> > + return -EBADF;
> > + }
> > +
> > + if (!dev->vdev->shmem_list ||
> > + dev->vdev->n_shmem_regions <= vu_mmap->shmid) {
> > + error_report("Device only has %d VIRTIO Shared Memory Regions. "
> > + "Requested ID: %d",
> > + dev->vdev->n_shmem_regions, vu_mmap->shmid);
> > + return -EFAULT;
> > + }
> > +
> > + shmem = &dev->vdev->shmem_list[vu_mmap->shmid];
> > +
> > + if (!shmem) {
> > + error_report("VIRTIO Shared Memory Region at "
> > + "ID %d unitialized", vu_mmap->shmid);
> > + return -EFAULT;
> > + }
> > +
> > + if ((vu_mmap->shm_offset + vu_mmap->len) < vu_mmap->len ||
> > + (vu_mmap->shm_offset + vu_mmap->len) > shmem->mr->size) {
> > + error_report("Bad offset/len for mmap %" PRIx64 "+%" PRIx64,
> > + vu_mmap->shm_offset, vu_mmap->len);
> > + return -EFAULT;
> > + }
> > +
> > + g_string_printf(shm_name, "virtio-shm%i-%lu",
> > + vu_mmap->shmid, vu_mmap->shm_offset);
> > +
> > + memory_region_transaction_begin();
> > + virtio_add_shmem_map(shmem, shm_name->str, vu_mmap->shm_offset,
> > + vu_mmap->fd_offset, vu_mmap->len, fd, errp);
>
> Error handling is missing. In particular, if virtio_add_shmem_map() sets
> errp due to an error, then vhost_user_send_resp() will crash if it also
> needs to set the error. An Error object can only be filled in once.
>
> > +
> > + hdr->size = sizeof(payload->u64);
> > + vhost_user_send_resp(ioc, hdr, payload, errp);
>
> payload->u64 has not been initialized. It contains the contents of
> payload->mmap, which is not the u64 return value that must be sent here.
> I think something like the following is necessary:
> payload->u64 = 0;
>
> > + memory_region_transaction_commit();
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +vhost_user_backend_handle_shmem_unmap(struct vhost_dev *dev,
> > + QIOChannel *ioc,
> > + VhostUserHeader *hdr,
> > + VhostUserPayload *payload,
> > + Error **errp)
> > +{
> > + VirtSharedMemory *shmem = NULL;
> > + MappedMemoryRegion *mmap = NULL;
> > + VhostUserMMap *vu_mmap = &payload->mmap;
> > +
> > + if (!dev->vdev->shmem_list ||
> > + dev->vdev->n_shmem_regions <= vu_mmap->shmid) {
> > + error_report("Device only has %d VIRTIO Shared Memory Regions. "
> > + "Requested ID: %d",
> > + dev->vdev->n_shmem_regions, vu_mmap->shmid);
> > + return -EFAULT;
> > + }
> > +
> > + shmem = &dev->vdev->shmem_list[vu_mmap->shmid];
> > +
> > + if (!shmem) {
> > + error_report("VIRTIO Shared Memory Region at "
> > + "ID %d unitialized", vu_mmap->shmid);
> > + return -EFAULT;
> > + }
> > +
> > + if ((vu_mmap->shm_offset + vu_mmap->len) < vu_mmap->len ||
> > + (vu_mmap->shm_offset + vu_mmap->len) > shmem->mr->size) {
> > + error_report("Bad offset/len for unmmap %" PRIx64 "+%" PRIx64,
> > + vu_mmap->shm_offset, vu_mmap->len);
> > + return -EFAULT;
> > + }
> > +
> > + mmap = virtio_find_shmem_map(shmem, vu_mmap->shm_offset, vu_mmap->len);
> > + if (!mmap) {
> > + return -EFAULT;
> > + }
> > +
> > + memory_region_transaction_begin();
> > + memory_region_del_subregion(shmem->mr, mmap->mem);
> > +
> > + hdr->size = sizeof(payload->u64);
> > + vhost_user_send_resp(ioc, hdr, payload, errp);
>
> Same uninitialized payload->u64 issue here.
>
> > + memory_region_transaction_commit();
> > +
> > + /* Free the MemoryRegion only after vhost_commit */
> > + virtio_del_shmem_map(shmem, vu_mmap->shm_offset, vu_mmap->len);
> > +
> > + return 0;
> > +}
> > +
> > static void close_backend_channel(struct vhost_user *u)
> > {
> > g_source_destroy(u->backend_src);
> > @@ -1837,6 +1963,14 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition,
> > case VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP:
> > ret = vhost_user_backend_handle_shared_object_lookup(dev->opaque, ioc,
> > &hdr, &payload);
> > + case VHOST_USER_BACKEND_SHMEM_MAP:
> > + ret = vhost_user_backend_handle_shmem_map(dev, ioc, &hdr, &payload,
> > + fd ? fd[0] : -1, &local_err);
> > + break;
> > + case VHOST_USER_BACKEND_SHMEM_UNMAP:
> > + ret = vhost_user_backend_handle_shmem_unmap(dev, ioc, &hdr, &payload,
> > + &local_err);
> > + break;
> > break;
> > default:
> > error_report("Received unexpected msg type: %d.", hdr.request);
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 85110bce37..47d0ddb820 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -3063,6 +3063,75 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f)
> > return vmstate_save_state(f, &vmstate_virtio, vdev, NULL);
> > }
> >
> > +VirtSharedMemory *virtio_new_shmem_region(VirtIODevice *vdev)
> > +{
> > + ++vdev->n_shmem_regions;
> > + vdev->shmem_list = g_renew(VirtSharedMemory, vdev->shmem_list,
> > + vdev->n_shmem_regions);
> > + vdev->shmem_list[vdev->n_shmem_regions - 1].mr = g_new0(MemoryRegion, 1);
> > + QTAILQ_INIT(&vdev->shmem_list[vdev->n_shmem_regions - 1].mmaps);
>
> QTAILQ cannot be used inside a struct that is reallocated using
> g_renew() because a dangling tql_prev pointer will be left after
> reallocation. From QTAILQ_INIT():
> (head)->tqh_circ.tql_prev = &(head)->tqh_circ; <--- not realloc-safe
>
> Instead of using g_renew() on an array, consider using a list from
> "qemu/queue.h". Lookup by shmid becomes O(n) instead of O(1), but it
> doesn't matter in practice since existing devices have a small number of
> VIRTIO Shared Memory Regions.
>
> > + return &vdev->shmem_list[vdev->n_shmem_regions - 1];
> > +}
> > +
> > +void virtio_add_shmem_map(VirtSharedMemory *shmem, const char *shm_name,
> > + hwaddr shm_offset, hwaddr fd_offset, uint64_t size,
> > + int fd, Error **errp)
> > +{
> > + MappedMemoryRegion *mmap;
> > + fd = dup(fd);
> > + if (fd < 0) {
> > + error_setg_errno(errp, errno, "Failed to duplicate fd");
> > + return;
> > + }
> > +
> > + if (shm_offset + size > shmem->mr->size) {
> > + error_setg(errp, "Memory exceeds the shared memory boundaries");
> > + return;
>
> fd is leaked here.
>
> > + }
> > +
> > + mmap = g_new0(MappedMemoryRegion, 1);
> > + mmap->mem = g_new0(MemoryRegion, 1);
> > + mmap->offset = shm_offset;
> > + if (!memory_region_init_ram_from_fd(mmap->mem,
> > + OBJECT(shmem->mr),
> > + shm_name, size, RAM_SHARED,
> > + fd, fd_offset, errp)) {
> > + error_setg(errp, "Failed to mmap region %s", shm_name);
> > + close(fd);
> > + g_free(mmap->mem);
> > + g_free(mmap);
> > + return;
> > + }
> > + memory_region_add_subregion(shmem->mr, shm_offset, mmap->mem);
> > +
> > + QTAILQ_INSERT_TAIL(&shmem->mmaps, mmap, link);
> > +}
> > +
> > +MappedMemoryRegion *virtio_find_shmem_map(VirtSharedMemory *shmem,
> > + hwaddr offset, uint64_t size)
> > +{
> > + MappedMemoryRegion *mmap;
> > + QTAILQ_FOREACH(mmap, &shmem->mmaps, link) {
> > + if (mmap->offset == offset && mmap->mem->size == size) {
> > + return mmap;
> > + }
> > + }
> > + return NULL;
> > +}
> > +
> > +void virtio_del_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
> > + uint64_t size)
> > +{
> > + MappedMemoryRegion *mmap = virtio_find_shmem_map(shmem, offset, size);
> > + if (mmap == NULL) {
> > + return;
> > + }
> > +
> > + object_unparent(OBJECT(mmap->mem));
> > + QTAILQ_REMOVE(&shmem->mmaps, mmap, link);
> > + g_free(mmap);
> > +}
> > +
> > /* A wrapper for use as a VMState .put function */
> > static int virtio_device_put(QEMUFile *f, void *opaque, size_t size,
> > const VMStateField *field, JSONWriter *vmdesc)
> > @@ -3492,6 +3561,8 @@ void virtio_init(VirtIODevice *vdev, uint16_t device_id, size_t config_size)
> > virtio_vmstate_change, vdev);
> > vdev->device_endian = virtio_default_endian();
> > vdev->use_guest_notifier_mask = true;
> > + vdev->shmem_list = NULL;
> > + vdev->n_shmem_regions = 0;
> > }
> >
> > /*
> > @@ -4005,11 +4076,21 @@ static void virtio_device_free_virtqueues(VirtIODevice *vdev)
> > static void virtio_device_instance_finalize(Object *obj)
> > {
> > VirtIODevice *vdev = VIRTIO_DEVICE(obj);
> > + VirtSharedMemory *shmem = NULL;
> > + int i;
> >
> > virtio_device_free_virtqueues(vdev);
> >
> > g_free(vdev->config);
> > g_free(vdev->vector_queues);
> > + for (i = 0; i< vdev->n_shmem_regions; i++) {
> > + shmem = &vdev->shmem_list[i];
> > + while (!QTAILQ_EMPTY(&shmem->mmaps)) {
> > + MappedMemoryRegion *mmap_reg = QTAILQ_FIRST(&shmem->mmaps);
> > + QTAILQ_REMOVE(&shmem->mmaps, mmap_reg, link);
> > + g_free(mmap_reg);
>
> Is it possible to reuse virtio_del_shmem_map() to avoid code duplication
> and inconsistencies?
>
> while (!QTAILQ_EMPTY(&shmem->mmaps)) {
> MappedMemoryRegion *mmap_reg = QTAILQ_FIRST(&shmem->mmaps);
> virtio_del_shmem_map(shmem, mmap_reg->offset, mmap_reg->mem->size);
> }
Yes that is a good idea, thanks. Maybe I could change the function
signature to receive the mmap_reg as parameter instead of the offset
and size combo.
>
> > + }
> > + }
>
> Missing g_free(vdev->shmem_list).
>
> > }
> >
> > static const Property virtio_properties[] = {
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index 6386910280..a778547c79 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -98,6 +98,21 @@ enum virtio_device_endian {
> > VIRTIO_DEVICE_ENDIAN_BIG,
> > };
> >
> > +struct MappedMemoryRegion {
> > + MemoryRegion *mem;
> > + hwaddr offset;
> > + QTAILQ_ENTRY(MappedMemoryRegion) link;
> > +};
> > +
> > +typedef struct MappedMemoryRegion MappedMemoryRegion;
> > +
> > +struct VirtSharedMemory {
> > + MemoryRegion *mr;
> > + QTAILQ_HEAD(, MappedMemoryRegion) mmaps;
> > +};
> > +
> > +typedef struct VirtSharedMemory VirtSharedMemory;
> > +
> > /**
> > * struct VirtIODevice - common VirtIO structure
> > * @name: name of the device
> > @@ -167,6 +182,9 @@ struct VirtIODevice
> > */
> > EventNotifier config_notifier;
> > bool device_iotlb_enabled;
> > + /* Shared memory region for vhost-user mappings. */
>
> This is core VIRTIO code and Shared Memory Regions are a VIRTIO concept.
> It is possible that built-in (non-vhost) devices might also add their
> shared memory regions with virtio_new_shmem_region(), so let's not talk
> about vhost-user specifically.
>
> > + VirtSharedMemory *shmem_list;
> > + int n_shmem_regions;
> > };
> >
> > struct VirtioDeviceClass {
> > @@ -289,6 +307,15 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
> >
> > int virtio_save(VirtIODevice *vdev, QEMUFile *f);
> >
> > +VirtSharedMemory *virtio_new_shmem_region(VirtIODevice *vdev);
> > +void virtio_add_shmem_map(VirtSharedMemory *shmem, const char *shm_name,
> > + hwaddr shm_offset, hwaddr fd_offset, uint64_t size,
> > + int fd, Error **errp);
> > +MappedMemoryRegion *virtio_find_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
> > + uint64_t size);
> > +void virtio_del_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
> > + uint64_t size);
> > +
> > extern const VMStateInfo virtio_vmstate_info;
> >
> > #define VMSTATE_VIRTIO_DEVICE \
> > diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> > index 9c630c2170..034cbfdc3c 100644
> > --- a/subprojects/libvhost-user/libvhost-user.c
> > +++ b/subprojects/libvhost-user/libvhost-user.c
> > @@ -1592,6 +1592,76 @@ vu_rm_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN])
> > return vu_send_message(dev, &msg);
> > }
> >
> > +bool
> > +vu_shmem_map(VuDev *dev, uint8_t shmid, uint64_t fd_offset,
> > + uint64_t shm_offset, uint64_t len, uint64_t flags, int fd)
> > +{
> > + VhostUserMsg vmsg = {
> > + .request = VHOST_USER_BACKEND_SHMEM_MAP,
> > + .size = sizeof(vmsg.payload.mmap),
> > + .flags = VHOST_USER_VERSION,
> > + .payload.mmap = {
> > + .shmid = shmid,
> > + .fd_offset = fd_offset,
> > + .shm_offset = shm_offset,
> > + .len = len,
> > + .flags = flags,
> > + },
> > + .fd_num = 1,
> > + .fds[0] = fd,
> > + };
> > +
> > + if (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SHMEM)) {
> > + return false;
> > + }
> > +
> > + if (vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_REPLY_ACK)) {
> > + vmsg.flags |= VHOST_USER_NEED_REPLY_MASK;
> > + }
>
> Setting this flag is inconsistent with
> vhost_user_backend_handle_shmem_map() and
> vhost_user_backend_handle_shmem_unmap(). They call
> vhost_user_send_resp() explicitly instead of relying on backend_read()
> to send the return value when VHOST_USER_NEED_REPLY_MASK is set. This
> inconsistency means that a successful return message will be sent twice
> when VHOST_USER_NEED_REPLY_MASK is set.
It is true that is inconsistent, I need to check for the field before
sending the reponse in the handlers. But at least it will not respond
twice as `vhost_user_send_resp()` unset the VHOST_USER_NEED_REPLY_MASK
bit.
>
> Either these new vhost-user messages could be specified with a mandatory
> reply payload or the patch could be updated to avoid the explicit
> vhost_user_send_resp() calls. I think the latter is more commonly used
> by other vhost-user messages, so it's probably best to do it the same
> way.
The explicit calls are necessary to be able to control that
vhost_commit is called after the response is sent, or else it will
cause a deadlock. What I need to do is to check for the
VHOST_USER_NEED_REPLY_MASK bit before calling it. Then
`vhost_user_send_resp()` will unset it to avoid double reponse.
>
> > +
> > + pthread_mutex_lock(&dev->backend_mutex);
> > + if (!vu_message_write(dev, dev->backend_fd, &vmsg)) {
> > + pthread_mutex_unlock(&dev->backend_mutex);
> > + return false;
> > + }
> > +
> > + /* Also unlocks the backend_mutex */
> > + return vu_process_message_reply(dev, &vmsg);
> > +}
> > +
> > +bool
> > +vu_shmem_unmap(VuDev *dev, uint8_t shmid, uint64_t shm_offset, uint64_t len)
> > +{
> > + VhostUserMsg vmsg = {
> > + .request = VHOST_USER_BACKEND_SHMEM_UNMAP,
> > + .size = sizeof(vmsg.payload.mmap),
> > + .flags = VHOST_USER_VERSION,
> > + .payload.mmap = {
> > + .shmid = shmid,
> > + .fd_offset = 0,
> > + .shm_offset = shm_offset,
> > + .len = len,
> > + },
> > + };
> > +
> > + if (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SHMEM)) {
> > + return false;
> > + }
> > +
> > + if (vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_REPLY_ACK)) {
> > + vmsg.flags |= VHOST_USER_NEED_REPLY_MASK;
> > + }
> > +
> > + pthread_mutex_lock(&dev->backend_mutex);
> > + if (!vu_message_write(dev, dev->backend_fd, &vmsg)) {
> > + pthread_mutex_unlock(&dev->backend_mutex);
> > + return false;
> > + }
> > +
> > + /* Also unlocks the backend_mutex */
> > + return vu_process_message_reply(dev, &vmsg);
> > +}
> > +
> > 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 2ffc58c11b..e9adb836f0 100644
> > --- a/subprojects/libvhost-user/libvhost-user.h
> > +++ b/subprojects/libvhost-user/libvhost-user.h
> > @@ -69,6 +69,8 @@ enum VhostUserProtocolFeature {
> > /* Feature 16 is reserved for VHOST_USER_PROTOCOL_F_STATUS. */
> > /* Feature 17 reserved for VHOST_USER_PROTOCOL_F_XEN_MMAP. */
> > VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 18,
> > + /* Feature 19 is reserved for VHOST_USER_PROTOCOL_F_DEVICE_STATE */
> > + VHOST_USER_PROTOCOL_F_SHMEM = 20,
> > VHOST_USER_PROTOCOL_F_MAX
> > };
> >
> > @@ -127,6 +129,8 @@ typedef enum VhostUserBackendRequest {
> > VHOST_USER_BACKEND_SHARED_OBJECT_ADD = 6,
> > VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE = 7,
> > VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP = 8,
> > + VHOST_USER_BACKEND_SHMEM_MAP = 9,
> > + VHOST_USER_BACKEND_SHMEM_UNMAP = 10,
> > VHOST_USER_BACKEND_MAX
> > } VhostUserBackendRequest;
> >
> > @@ -186,6 +190,24 @@ typedef struct VhostUserShared {
> > unsigned char uuid[UUID_LEN];
> > } VhostUserShared;
> >
> > +/* For the flags field of VhostUserMMap */
> > +#define VHOST_USER_FLAG_MAP_R (1u << 0)
> > +#define VHOST_USER_FLAG_MAP_W (1u << 1)
> > +
> > +typedef struct {
> > + /* VIRTIO Shared Memory Region ID */
> > + uint8_t shmid;
> > + uint8_t padding[7];
> > + /* File offset */
> > + uint64_t fd_offset;
> > + /* Offset within the VIRTIO Shared Memory Region */
> > + uint64_t shm_offset;
> > + /* Size of the mapping */
> > + uint64_t len;
> > + /* Flags for the mmap operation, from VHOST_USER_FLAG_* */
> > + uint64_t flags;
> > +} VhostUserMMap;
> > +
> > #define VU_PACKED __attribute__((packed))
> >
> > typedef struct VhostUserMsg {
> > @@ -210,6 +232,7 @@ typedef struct VhostUserMsg {
> > VhostUserVringArea area;
> > VhostUserInflight inflight;
> > VhostUserShared object;
> > + VhostUserMMap mmap;
> > } payload;
> >
> > int fds[VHOST_MEMORY_BASELINE_NREGIONS];
> > @@ -593,6 +616,38 @@ bool vu_add_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN]);
> > */
> > bool vu_rm_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN]);
> >
> > +/**
> > + * vu_shmem_map:
> > + * @dev: a VuDev context
> > + * @shmid: VIRTIO Shared Memory Region ID
> > + * @fd_offset: File offset
> > + * @shm_offset: Offset within the VIRTIO Shared Memory Region
> > + * @len: Size of the mapping
> > + * @flags: Flags for the mmap operation
> > + * @fd: A file descriptor
> > + *
> > + * Advertises a new mapping to be made in a given VIRTIO Shared Memory Region.
> > + *
> > + * Returns: TRUE on success, FALSE on failure.
> > + */
> > +bool vu_shmem_map(VuDev *dev, uint8_t shmid, uint64_t fd_offset,
> > + uint64_t shm_offset, uint64_t len, uint64_t flags, int fd);
> > +
> > +/**
> > + * vu_shmem_unmap:
> > + * @dev: a VuDev context
> > + * @shmid: VIRTIO Shared Memory Region ID
> > + * @fd_offset: File offset
> > + * @len: Size of the mapping
> > + *
> > + * The front-end un-mmaps a given range in the VIRTIO Shared Memory Region
> > + * with the requested `shmid`.
> > + *
> > + * Returns: TRUE on success, FALSE on failure.
> > + */
> > +bool vu_shmem_unmap(VuDev *dev, uint8_t shmid, uint64_t shm_offset,
> > + uint64_t len);
> > +
> > /**
> > * vu_queue_set_notification:
> > * @dev: a VuDev context
> > --
> > 2.48.1
> >
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 6/9] qmp: add shmem feature map
2025-02-17 16:40 ` [PATCH v4 6/9] qmp: add shmem feature map Albert Esteve
@ 2025-02-18 10:34 ` Stefan Hajnoczi
0 siblings, 0 replies; 36+ messages in thread
From: Stefan Hajnoczi @ 2025-02-18 10:34 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, slp, stevensd, Alex Bennée, Stefano Garzarella,
david, hi, mst, jasowang
[-- Attachment #1: Type: text/plain, Size: 342 bytes --]
On Mon, Feb 17, 2025 at 05:40:09PM +0100, Albert Esteve wrote:
> Add new vhost-user protocol
> VHOST_USER_PROTOCOL_F_SHMEM feature to
> feature map.
>
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> ---
> hw/virtio/virtio-qmp.c | 3 +++
> 1 file changed, 3 insertions(+)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 7/9] vhost-user-devive: Add shmem BAR
2025-02-17 16:40 ` [PATCH v4 7/9] vhost-user-devive: Add shmem BAR Albert Esteve
@ 2025-02-18 10:41 ` Stefan Hajnoczi
2025-02-18 10:55 ` Albert Esteve
0 siblings, 1 reply; 36+ messages in thread
From: Stefan Hajnoczi @ 2025-02-18 10:41 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, slp, stevensd, Alex Bennée, Stefano Garzarella,
david, hi, mst, jasowang
[-- Attachment #1: Type: text/plain, Size: 6890 bytes --]
On Mon, Feb 17, 2025 at 05:40:10PM +0100, Albert Esteve wrote:
> Add a shmem BAR block in the vhost-user-device,
> which files can be directly mapped into.
>
> The number, shmid, and size of the VIRTIO Shared
> Memory subregions is retrieved through a
> get_shmem_config message sent by the
> vhost-user-base module on the realize step,
> after virtio_init().
>
> By default, if VHOST_USER_PROTOCOL_F_SHMEM
> feature is not supported by the backend,
> there is no cache.
>
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> ---
> hw/virtio/vhost-user-base.c | 47 +++++++++++++++++++++++++++++--
> hw/virtio/vhost-user-device-pci.c | 36 +++++++++++++++++++++--
> 2 files changed, 78 insertions(+), 5 deletions(-)
>
> diff --git a/hw/virtio/vhost-user-base.c b/hw/virtio/vhost-user-base.c
> index 2bc3423326..8d4bca98a8 100644
> --- a/hw/virtio/vhost-user-base.c
> +++ b/hw/virtio/vhost-user-base.c
> @@ -16,6 +16,7 @@
> #include "hw/virtio/virtio-bus.h"
> #include "hw/virtio/vhost-user-base.h"
> #include "qemu/error-report.h"
> +#include "migration/blocker.h"
>
> static void vub_start(VirtIODevice *vdev)
> {
> @@ -271,7 +272,8 @@ static void vub_device_realize(DeviceState *dev, Error **errp)
> {
> VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> VHostUserBase *vub = VHOST_USER_BASE(dev);
> - int ret;
> + uint64_t memory_sizes[VIRTIO_MAX_SHMEM_REGIONS];
> + int i, ret, nregions;
>
> if (!vub->chardev.chr) {
> error_setg(errp, "vhost-user-base: missing chardev");
> @@ -314,7 +316,7 @@ static void vub_device_realize(DeviceState *dev, Error **errp)
>
> /* Allocate queues */
> vub->vqs = g_ptr_array_sized_new(vub->num_vqs);
> - for (int i = 0; i < vub->num_vqs; i++) {
> + for (i = 0; i < vub->num_vqs; i++) {
> g_ptr_array_add(vub->vqs,
> virtio_add_queue(vdev, vub->vq_size,
> vub_handle_output));
> @@ -328,11 +330,50 @@ static void vub_device_realize(DeviceState *dev, Error **errp)
> VHOST_BACKEND_TYPE_USER, 0, errp);
>
> if (ret < 0) {
> - do_vhost_user_cleanup(vdev, vub);
> + goto err;
> + }
> +
> + ret = vub->vhost_dev.vhost_ops->vhost_get_shmem_config(&vub->vhost_dev,
> + &nregions,
> + memory_sizes,
> + errp);
> +
> + if (ret < 0) {
> + goto err;
> + }
> +
> + for (i = 0; i < nregions; i++) {
> + if (memory_sizes[i]) {
> + if (vub->vhost_dev.migration_blocker == NULL) {
> + error_setg(&vub->vhost_dev.migration_blocker,
> + "Migration disabled: devices with VIRTIO Shared Memory "
> + "Regions do not support migration yet.");
> + ret = migrate_add_blocker_normal(
> + &vub->vhost_dev.migration_blocker,
> + errp);
> +
> + if (ret < 0) {
> + goto err;
> + }
> + }
> +
> + if (memory_sizes[i] % qemu_real_host_page_size() != 0) {
> + error_setg(errp, "Shared memory %d size must be a power of 2 "
> + "no smaller than the page size", i);
> + goto err;
> + }
> +
> + memory_region_init(virtio_new_shmem_region(vdev)->mr,
Does this code support non-contiguous shmids? For example, if a device
has two Shared Memory Regions defined in its spec but the first one is
optional, then the device might have memory_sizes[0] == 0 and
memory_sizes[1] > 0. In that case the Shared Memory Region must have
shmid 1 and not shmid 0.
> + OBJECT(vdev), "vub-shm-" + i,
> + memory_sizes[i]);
> + }
> }
>
> qemu_chr_fe_set_handlers(&vub->chardev, NULL, NULL, vub_event, NULL,
> dev, NULL, true);
> + return;
> +err:
> + do_vhost_user_cleanup(vdev, vub);
> }
>
> static void vub_device_unrealize(DeviceState *dev)
> diff --git a/hw/virtio/vhost-user-device-pci.c b/hw/virtio/vhost-user-device-pci.c
> index efaf55d3dd..f215cae925 100644
> --- a/hw/virtio/vhost-user-device-pci.c
> +++ b/hw/virtio/vhost-user-device-pci.c
> @@ -8,14 +8,18 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qapi/error.h"
> #include "hw/qdev-properties.h"
> #include "hw/virtio/vhost-user-base.h"
> #include "hw/virtio/virtio-pci.h"
>
> +#define VIRTIO_DEVICE_PCI_SHMEM_BAR 2
> +
> struct VHostUserDevicePCI {
> VirtIOPCIProxy parent_obj;
>
> VHostUserBase vub;
> + MemoryRegion shmembar;
> };
>
> #define TYPE_VHOST_USER_DEVICE_PCI "vhost-user-device-pci-base"
> @@ -25,10 +29,38 @@ OBJECT_DECLARE_SIMPLE_TYPE(VHostUserDevicePCI, VHOST_USER_DEVICE_PCI)
> static void vhost_user_device_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> {
> VHostUserDevicePCI *dev = VHOST_USER_DEVICE_PCI(vpci_dev);
> - DeviceState *vdev = DEVICE(&dev->vub);
> + DeviceState *dev_state = DEVICE(&dev->vub);
> + VirtIODevice *vdev = VIRTIO_DEVICE(dev_state);
> + MemoryRegion *mr;
> + uint64_t offset = 0, shmem_size = 0;
> + int i;
>
> vpci_dev->nvectors = 1;
> - qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
> + qdev_realize(dev_state, BUS(&vpci_dev->bus), errp);
> +
> + for (i = 0; i < vdev->n_shmem_regions; i++) {
> + mr = vdev->shmem_list[i].mr;
> + if (mr->size > UINT64_MAX - shmem_size) {
> + error_setg(errp, "Total shared memory required overflow");
> + return;
> + }
> + shmem_size = shmem_size + mr->size;
> + }
> + if (shmem_size) {
> + memory_region_init(&dev->shmembar, OBJECT(vpci_dev),
> + "vhost-device-pci-shmembar", shmem_size);
> + for (i = 0; i < vdev->n_shmem_regions; i++) {
> + memory_region_add_subregion(&dev->shmembar, offset, mr);
> + virtio_pci_add_shm_cap(vpci_dev, VIRTIO_DEVICE_PCI_SHMEM_BAR,
> + offset, mr->size, i);
> + offset = offset + mr->size;
> + }
> + pci_register_bar(&vpci_dev->pci_dev, VIRTIO_DEVICE_PCI_SHMEM_BAR,
> + PCI_BASE_ADDRESS_SPACE_MEMORY |
> + PCI_BASE_ADDRESS_MEM_PREFETCH |
> + PCI_BASE_ADDRESS_MEM_TYPE_64,
> + &dev->shmembar);
> + }
> }
>
> static void vhost_user_device_pci_class_init(ObjectClass *klass, void *data)
> --
> 2.48.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 7/9] vhost-user-devive: Add shmem BAR
2025-02-18 10:41 ` Stefan Hajnoczi
@ 2025-02-18 10:55 ` Albert Esteve
2025-02-18 13:25 ` Stefan Hajnoczi
0 siblings, 1 reply; 36+ messages in thread
From: Albert Esteve @ 2025-02-18 10:55 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, slp, stevensd, Alex Bennée, Stefano Garzarella,
david, hi, mst, jasowang
On Tue, Feb 18, 2025 at 11:41 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Mon, Feb 17, 2025 at 05:40:10PM +0100, Albert Esteve wrote:
> > Add a shmem BAR block in the vhost-user-device,
> > which files can be directly mapped into.
> >
> > The number, shmid, and size of the VIRTIO Shared
> > Memory subregions is retrieved through a
> > get_shmem_config message sent by the
> > vhost-user-base module on the realize step,
> > after virtio_init().
> >
> > By default, if VHOST_USER_PROTOCOL_F_SHMEM
> > feature is not supported by the backend,
> > there is no cache.
> >
> > Signed-off-by: Albert Esteve <aesteve@redhat.com>
> > ---
> > hw/virtio/vhost-user-base.c | 47 +++++++++++++++++++++++++++++--
> > hw/virtio/vhost-user-device-pci.c | 36 +++++++++++++++++++++--
> > 2 files changed, 78 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-user-base.c b/hw/virtio/vhost-user-base.c
> > index 2bc3423326..8d4bca98a8 100644
> > --- a/hw/virtio/vhost-user-base.c
> > +++ b/hw/virtio/vhost-user-base.c
> > @@ -16,6 +16,7 @@
> > #include "hw/virtio/virtio-bus.h"
> > #include "hw/virtio/vhost-user-base.h"
> > #include "qemu/error-report.h"
> > +#include "migration/blocker.h"
> >
> > static void vub_start(VirtIODevice *vdev)
> > {
> > @@ -271,7 +272,8 @@ static void vub_device_realize(DeviceState *dev, Error **errp)
> > {
> > VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > VHostUserBase *vub = VHOST_USER_BASE(dev);
> > - int ret;
> > + uint64_t memory_sizes[VIRTIO_MAX_SHMEM_REGIONS];
> > + int i, ret, nregions;
> >
> > if (!vub->chardev.chr) {
> > error_setg(errp, "vhost-user-base: missing chardev");
> > @@ -314,7 +316,7 @@ static void vub_device_realize(DeviceState *dev, Error **errp)
> >
> > /* Allocate queues */
> > vub->vqs = g_ptr_array_sized_new(vub->num_vqs);
> > - for (int i = 0; i < vub->num_vqs; i++) {
> > + for (i = 0; i < vub->num_vqs; i++) {
> > g_ptr_array_add(vub->vqs,
> > virtio_add_queue(vdev, vub->vq_size,
> > vub_handle_output));
> > @@ -328,11 +330,50 @@ static void vub_device_realize(DeviceState *dev, Error **errp)
> > VHOST_BACKEND_TYPE_USER, 0, errp);
> >
> > if (ret < 0) {
> > - do_vhost_user_cleanup(vdev, vub);
> > + goto err;
> > + }
> > +
> > + ret = vub->vhost_dev.vhost_ops->vhost_get_shmem_config(&vub->vhost_dev,
> > + &nregions,
> > + memory_sizes,
> > + errp);
> > +
> > + if (ret < 0) {
> > + goto err;
> > + }
> > +
> > + for (i = 0; i < nregions; i++) {
> > + if (memory_sizes[i]) {
> > + if (vub->vhost_dev.migration_blocker == NULL) {
> > + error_setg(&vub->vhost_dev.migration_blocker,
> > + "Migration disabled: devices with VIRTIO Shared Memory "
> > + "Regions do not support migration yet.");
> > + ret = migrate_add_blocker_normal(
> > + &vub->vhost_dev.migration_blocker,
> > + errp);
> > +
> > + if (ret < 0) {
> > + goto err;
> > + }
> > + }
> > +
> > + if (memory_sizes[i] % qemu_real_host_page_size() != 0) {
> > + error_setg(errp, "Shared memory %d size must be a power of 2 "
> > + "no smaller than the page size", i);
> > + goto err;
> > + }
> > +
> > + memory_region_init(virtio_new_shmem_region(vdev)->mr,
>
> Does this code support non-contiguous shmids? For example, if a device
> has two Shared Memory Regions defined in its spec but the first one is
> optional, then the device might have memory_sizes[0] == 0 and
> memory_sizes[1] > 0. In that case the Shared Memory Region must have
> shmid 1 and not shmid 0.
Yes, it does. That is guarded by ` if (memory_sizes[i]) {`, which only
initializes the region if memory_sizes[i] > 0. The main downsize of
that, is that it requires to send as many `memory_sizes` elements as
the highest shmid for the device. But as it is, it is supported by
this code.
>
> > + OBJECT(vdev), "vub-shm-" + i,
> > + memory_sizes[i]);
> > + }
> > }
> >
> > qemu_chr_fe_set_handlers(&vub->chardev, NULL, NULL, vub_event, NULL,
> > dev, NULL, true);
> > + return;
> > +err:
> > + do_vhost_user_cleanup(vdev, vub);
> > }
> >
> > static void vub_device_unrealize(DeviceState *dev)
> > diff --git a/hw/virtio/vhost-user-device-pci.c b/hw/virtio/vhost-user-device-pci.c
> > index efaf55d3dd..f215cae925 100644
> > --- a/hw/virtio/vhost-user-device-pci.c
> > +++ b/hw/virtio/vhost-user-device-pci.c
> > @@ -8,14 +8,18 @@
> > */
> >
> > #include "qemu/osdep.h"
> > +#include "qapi/error.h"
> > #include "hw/qdev-properties.h"
> > #include "hw/virtio/vhost-user-base.h"
> > #include "hw/virtio/virtio-pci.h"
> >
> > +#define VIRTIO_DEVICE_PCI_SHMEM_BAR 2
> > +
> > struct VHostUserDevicePCI {
> > VirtIOPCIProxy parent_obj;
> >
> > VHostUserBase vub;
> > + MemoryRegion shmembar;
> > };
> >
> > #define TYPE_VHOST_USER_DEVICE_PCI "vhost-user-device-pci-base"
> > @@ -25,10 +29,38 @@ OBJECT_DECLARE_SIMPLE_TYPE(VHostUserDevicePCI, VHOST_USER_DEVICE_PCI)
> > static void vhost_user_device_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> > {
> > VHostUserDevicePCI *dev = VHOST_USER_DEVICE_PCI(vpci_dev);
> > - DeviceState *vdev = DEVICE(&dev->vub);
> > + DeviceState *dev_state = DEVICE(&dev->vub);
> > + VirtIODevice *vdev = VIRTIO_DEVICE(dev_state);
> > + MemoryRegion *mr;
> > + uint64_t offset = 0, shmem_size = 0;
> > + int i;
> >
> > vpci_dev->nvectors = 1;
> > - qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
> > + qdev_realize(dev_state, BUS(&vpci_dev->bus), errp);
> > +
> > + for (i = 0; i < vdev->n_shmem_regions; i++) {
> > + mr = vdev->shmem_list[i].mr;
> > + if (mr->size > UINT64_MAX - shmem_size) {
> > + error_setg(errp, "Total shared memory required overflow");
> > + return;
> > + }
> > + shmem_size = shmem_size + mr->size;
> > + }
> > + if (shmem_size) {
> > + memory_region_init(&dev->shmembar, OBJECT(vpci_dev),
> > + "vhost-device-pci-shmembar", shmem_size);
> > + for (i = 0; i < vdev->n_shmem_regions; i++) {
> > + memory_region_add_subregion(&dev->shmembar, offset, mr);
> > + virtio_pci_add_shm_cap(vpci_dev, VIRTIO_DEVICE_PCI_SHMEM_BAR,
> > + offset, mr->size, i);
> > + offset = offset + mr->size;
> > + }
> > + pci_register_bar(&vpci_dev->pci_dev, VIRTIO_DEVICE_PCI_SHMEM_BAR,
> > + PCI_BASE_ADDRESS_SPACE_MEMORY |
> > + PCI_BASE_ADDRESS_MEM_PREFETCH |
> > + PCI_BASE_ADDRESS_MEM_TYPE_64,
> > + &dev->shmembar);
> > + }
> > }
> >
> > static void vhost_user_device_pci_class_init(ObjectClass *klass, void *data)
> > --
> > 2.48.1
> >
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 8/9] vhost_user: Add mem_read/write backend requests
2025-02-17 16:40 ` [PATCH v4 8/9] vhost_user: Add mem_read/write backend requests Albert Esteve
@ 2025-02-18 10:57 ` Stefan Hajnoczi
0 siblings, 0 replies; 36+ messages in thread
From: Stefan Hajnoczi @ 2025-02-18 10:57 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, slp, stevensd, Alex Bennée, Stefano Garzarella,
david, hi, mst, jasowang
[-- Attachment #1: Type: text/plain, Size: 15732 bytes --]
On Mon, Feb 17, 2025 at 05:40:11PM +0100, Albert Esteve wrote:
> With SHMEM_MAP messages, sharing descriptors between
> devices will cause that these devices do not see the
> mappings, and fail to access these memory regions.
>
> To solve this, introduce MEM_READ/WRITE requests
> that will get triggered as a fallback when
> vhost-user memory translation fails.
>
> MEM_READ/WRITE requests have flexible array members,
> since we do not know in advance the number of bytes
> in the mapped region. Therefore, we need to allow
> bigger message sizes for these types, and ensure
> we allocate sufficient memory for them.
>
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> ---
> hw/virtio/vhost-user.c | 99 +++++++++++++++++------
> subprojects/libvhost-user/libvhost-user.c | 90 +++++++++++++++++++++
> subprojects/libvhost-user/libvhost-user.h | 37 +++++++++
> 3 files changed, 202 insertions(+), 24 deletions(-)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 9cc148f726..ab92905a36 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -118,6 +118,8 @@ typedef enum VhostUserBackendRequest {
> VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP = 8,
> VHOST_USER_BACKEND_SHMEM_MAP = 9,
> VHOST_USER_BACKEND_SHMEM_UNMAP = 10,
> + VHOST_USER_BACKEND_MEM_READ = 11,
> + VHOST_USER_BACKEND_MEM_WRITE = 12,
> VHOST_USER_BACKEND_MAX
> } VhostUserBackendRequest;
>
> @@ -145,6 +147,12 @@ typedef struct VhostUserShMemConfig {
> uint64_t memory_sizes[VIRTIO_MAX_SHMEM_REGIONS];
> } VhostUserShMemConfig;
>
> +typedef struct VhostUserMemRWMsg {
> + uint64_t guest_address;
> + uint32_t size;
> + uint8_t data[];
> +} VhostUserMemRWMsg;
> +
> typedef struct VhostUserLog {
> uint64_t mmap_size;
> uint64_t mmap_offset;
> @@ -253,6 +261,7 @@ typedef union {
> VhostUserTransferDeviceState transfer_state;
> VhostUserMMap mmap;
> VhostUserShMemConfig shmem;
> + VhostUserMemRWMsg mem_rw;
> } VhostUserPayload;
>
> typedef struct VhostUserMsg {
> @@ -341,17 +350,23 @@ static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg)
> return r;
> }
>
> - /* validate message size is sane */
> - if (msg->hdr.size > VHOST_USER_PAYLOAD_SIZE) {
> - error_report("Failed to read msg header."
> - " Size %d exceeds the maximum %zu.", msg->hdr.size,
> - VHOST_USER_PAYLOAD_SIZE);
> - return -EPROTO;
> - }
> -
> if (msg->hdr.size) {
> p += VHOST_USER_HDR_SIZE;
> size = msg->hdr.size;
> + /* validate message size is sane */
> + if (msg->hdr.size > VHOST_USER_PAYLOAD_SIZE) {
> + switch(msg->hdr.request) {
> + case VHOST_USER_BACKEND_MEM_READ:
> + case VHOST_USER_BACKEND_MEM_WRITE:
> + p = g_malloc0(size);
This doesn't work because the function signature is:
static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg)
The caller expects msg to be filled in. Setting p to newly allocated
heap memory leaks this memory and the caller will still be looking at
the old msg.
Perhaps variable-length commands should be treated differently:
vhost_user_read() only reads msg->hdr and leaves it up to the caller to
read the remaining msg->hdr.size bytes later.
> + break;
> + default:
> + error_report("Failed to read msg header."
> + " Size %d exceeds the maximum %zu.",
> + size, VHOST_USER_PAYLOAD_SIZE);
> + return -EPROTO;
> + }
> + }
> r = qemu_chr_fe_read_all(chr, p, size);
> if (r != size) {
> int saved_errno = errno;
> @@ -1904,6 +1919,28 @@ vhost_user_backend_handle_shmem_unmap(struct vhost_dev *dev,
> return 0;
> }
>
> +static int
> +vhost_user_backend_handle_mem_read(struct vhost_dev *dev,
> + VhostUserMemRWMsg *mem_rw)
> +{
> + MemTxResult result;
> + result = address_space_read(dev->vdev->dma_as, mem_rw->guest_address,
> + MEMTXATTRS_UNSPECIFIED, &mem_rw->data,
> + mem_rw->size);
> + return result;
> +}
> +
> +static int
> +vhost_user_backend_handle_mem_write(struct vhost_dev *dev,
> + VhostUserMemRWMsg *mem_rw)
> +{
> + MemTxResult result;
> + result = address_space_write(dev->vdev->dma_as, mem_rw->guest_address,
> + MEMTXATTRS_UNSPECIFIED, &mem_rw->data,
> + mem_rw->size);
> + return result;
> +}
> +
> static void close_backend_channel(struct vhost_user *u)
> {
> g_source_destroy(u->backend_src);
> @@ -1919,7 +1956,7 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition,
> struct vhost_dev *dev = opaque;
> struct vhost_user *u = dev->opaque;
> VhostUserHeader hdr = { 0, };
> - VhostUserPayload payload = { 0, };
> + VhostUserPayload *payload = g_new0(VhostUserPayload, 1);
> Error *local_err = NULL;
> gboolean rc = G_SOURCE_CONTINUE;
> int ret = 0;
> @@ -1938,47 +1975,60 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition,
> }
>
> if (hdr.size > VHOST_USER_PAYLOAD_SIZE) {
> - error_report("Failed to read msg header."
> - " Size %d exceeds the maximum %zu.", hdr.size,
> - VHOST_USER_PAYLOAD_SIZE);
> - goto err;
> + switch (hdr.request) {
> + case VHOST_USER_BACKEND_MEM_READ:
> + case VHOST_USER_BACKEND_MEM_WRITE:
> + payload = g_malloc0(hdr.size);
> + break;
> + default:
> + error_report("Failed to read msg header."
> + " Size %d exceeds the maximum %zu.", hdr.size,
> + VHOST_USER_PAYLOAD_SIZE);
> + goto err;
> + }
> }
>
> /* Read payload */
> - if (qio_channel_read_all(ioc, (char *) &payload, hdr.size, &local_err)) {
> + if (qio_channel_read_all(ioc, (char *) payload, hdr.size, &local_err)) {
> error_report_err(local_err);
> goto err;
> }
>
> switch (hdr.request) {
> case VHOST_USER_BACKEND_IOTLB_MSG:
> - ret = vhost_backend_handle_iotlb_msg(dev, &payload.iotlb);
> + ret = vhost_backend_handle_iotlb_msg(dev, &payload->iotlb);
> break;
> case VHOST_USER_BACKEND_CONFIG_CHANGE_MSG:
> ret = vhost_user_backend_handle_config_change(dev);
> break;
> case VHOST_USER_BACKEND_VRING_HOST_NOTIFIER_MSG:
> - ret = vhost_user_backend_handle_vring_host_notifier(dev, &payload.area,
> + ret = vhost_user_backend_handle_vring_host_notifier(dev, &payload->area,
> fd ? fd[0] : -1);
> break;
> case VHOST_USER_BACKEND_SHARED_OBJECT_ADD:
> - ret = vhost_user_backend_handle_shared_object_add(dev, &payload.object);
> + 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(dev,
> - &payload.object);
> + &payload->object);
> break;
> case VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP:
> ret = vhost_user_backend_handle_shared_object_lookup(dev->opaque, ioc,
> - &hdr, &payload);
> + &hdr, payload);
> + break;
> case VHOST_USER_BACKEND_SHMEM_MAP:
> - ret = vhost_user_backend_handle_shmem_map(dev, ioc, &hdr, &payload,
> + ret = vhost_user_backend_handle_shmem_map(dev, ioc, &hdr, payload,
> fd ? fd[0] : -1, &local_err);
> break;
> case VHOST_USER_BACKEND_SHMEM_UNMAP:
> - ret = vhost_user_backend_handle_shmem_unmap(dev, ioc, &hdr, &payload,
> + ret = vhost_user_backend_handle_shmem_unmap(dev, ioc, &hdr, payload,
> &local_err);
> break;
> + case VHOST_USER_BACKEND_MEM_READ:
> + ret = vhost_user_backend_handle_mem_read(dev, &payload->mem_rw);
> + break;
> + case VHOST_USER_BACKEND_MEM_WRITE:
> + ret = vhost_user_backend_handle_mem_write(dev, &payload->mem_rw);
> break;
> default:
> error_report("Received unexpected msg type: %d.", hdr.request);
> @@ -1990,10 +2040,10 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition,
> * directly in their request handlers.
> */
> if (hdr.flags & VHOST_USER_NEED_REPLY_MASK) {
> - payload.u64 = !!ret;
> - hdr.size = sizeof(payload.u64);
> + payload->u64 = !!ret;
> + hdr.size = sizeof(payload->u64);
>
> - if (!vhost_user_send_resp(ioc, &hdr, &payload, &local_err)) {
> + if (!vhost_user_send_resp(ioc, &hdr, payload, &local_err)) {
> error_report_err(local_err);
> goto err;
> }
> @@ -2011,6 +2061,7 @@ fdcleanup:
> close(fd[i]);
> }
> }
> + g_free(payload);
> return rc;
> }
>
> diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> index 034cbfdc3c..575a0af556 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -1662,6 +1662,96 @@ vu_shmem_unmap(VuDev *dev, uint8_t shmid, uint64_t shm_offset, uint64_t len)
> return vu_process_message_reply(dev, &vmsg);
> }
>
> +bool
> +vu_send_mem_read(VuDev *dev, uint64_t guest_addr, uint32_t size,
> + uint8_t *data)
> +{
> + VhostUserMsg msg_reply;
> + VhostUserMsg msg = {
> + .request = VHOST_USER_BACKEND_MEM_READ,
> + .size = sizeof(msg.payload.mem_rw),
> + .flags = VHOST_USER_VERSION | VHOST_USER_NEED_REPLY_MASK,
> + .payload = {
> + .mem_rw = {
> + .guest_address = guest_addr,
> + .size = size,
> + }
> + }
> + };
> +
> + if (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SHMEM)) {
> + return false;
> + }
> +
> + pthread_mutex_lock(&dev->backend_mutex);
> + if (!vu_message_write(dev, dev->backend_fd, &msg)) {
> + goto out_err;
> + }
> +
> + if (!vu_message_read_default(dev, dev->backend_fd, &msg_reply)) {
> + goto out_err;
> + }
> +
> + if (msg_reply.request != msg.request) {
> + DPRINT("Received unexpected msg type. Expected %d, received %d",
> + msg.request, msg_reply.request);
> + goto out_err;
> + }
> +
> + if (msg_reply.payload.mem_rw.size != size) {
> + DPRINT("Received unexpected number of bytes in the response. "
> + "Expected %d, received %d",
> + size, msg_reply.payload.mem_rw.size);
> + goto out_err;
> + }
> +
> + /* TODO: It should be possible to avoid memcpy() here by receiving
> + * directly into the caller's buffer. */
> + memcpy(data, msg_reply.payload.mem_rw.data, size);
> + pthread_mutex_unlock(&dev->backend_mutex);
> + return true;
> +
> +out_err:
> + pthread_mutex_unlock(&dev->backend_mutex);
> + return false;
> +}
> +
> +bool
> +vu_send_mem_write(VuDev *dev, uint64_t guest_addr, uint32_t size,
> + uint8_t *data)
> +{
> + VhostUserMsg msg = {
> + .request = VHOST_USER_BACKEND_MEM_WRITE,
> + .size = sizeof(msg.payload.mem_rw),
> + .flags = VHOST_USER_VERSION,
> + .payload = {
> + .mem_rw = {
> + .guest_address = guest_addr,
> + .size = size,
> + }
> + }
> + };
> + /* TODO: It should be possible to avoid memcpy() here by receiving
> + * directly into the caller's buffer. */
> + memcpy(msg.payload.mem_rw.data, data, size);
> +
> + if (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SHMEM)) {
> + return false;
> + }
> +
> + if (vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_REPLY_ACK)) {
> + msg.flags |= VHOST_USER_NEED_REPLY_MASK;
> + }
> +
> + if (!vu_message_write(dev, dev->backend_fd, &msg)) {
> + pthread_mutex_unlock(&dev->backend_mutex);
> + return false;
> + }
> +
> + /* Also unlocks the backend_mutex */
> + return vu_process_message_reply(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 e9adb836f0..57e2fb9c98 100644
> --- a/subprojects/libvhost-user/libvhost-user.h
> +++ b/subprojects/libvhost-user/libvhost-user.h
> @@ -131,6 +131,8 @@ typedef enum VhostUserBackendRequest {
> VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP = 8,
> VHOST_USER_BACKEND_SHMEM_MAP = 9,
> VHOST_USER_BACKEND_SHMEM_UNMAP = 10,
> + VHOST_USER_BACKEND_MEM_READ = 11,
> + VHOST_USER_BACKEND_MEM_WRITE = 12,
> VHOST_USER_BACKEND_MAX
> } VhostUserBackendRequest;
>
> @@ -154,6 +156,12 @@ typedef struct VhostUserMemRegMsg {
> VhostUserMemoryRegion region;
> } VhostUserMemRegMsg;
>
> +typedef struct VhostUserMemRWMsg {
> + uint64_t guest_address;
> + uint32_t size;
> + uint8_t data[];
> +} VhostUserMemRWMsg;
> +
> typedef struct VhostUserLog {
> uint64_t mmap_size;
> uint64_t mmap_offset;
> @@ -233,6 +241,7 @@ typedef struct VhostUserMsg {
> VhostUserInflight inflight;
> VhostUserShared object;
> VhostUserMMap mmap;
> + VhostUserMemRWMsg mem_rw;
> } payload;
>
> int fds[VHOST_MEMORY_BASELINE_NREGIONS];
> @@ -647,6 +656,34 @@ bool vu_shmem_map(VuDev *dev, uint8_t shmid, uint64_t fd_offset,
> */
> bool vu_shmem_unmap(VuDev *dev, uint8_t shmid, uint64_t shm_offset,
> uint64_t len);
> +/**
> + * vu_send_mem_read:
> + * @dev: a VuDev context
> + * @guest_addr: guest physical address to read
> + * @size: number of bytes to read
> + * @data: head of an unitialized bytes array
> + *
> + * Reads `size` bytes of `guest_addr` in the frontend and stores
> + * them in `data`.
> + *
> + * Returns: TRUE on success, FALSE on failure.
> + */
> +bool vu_send_mem_read(VuDev *dev, uint64_t guest_addr, uint32_t size,
> + uint8_t *data);
> +
> +/**
> + * vu_send_mem_write:
> + * @dev: a VuDev context
> + * @guest_addr: guest physical address to write
> + * @size: number of bytes to write
> + * @data: head of an array with `size` bytes to write
> + *
> + * Writes `size` bytes from `data` into `guest_addr` in the frontend.
> + *
> + * Returns: TRUE on success, FALSE on failure.
> + */
> +bool vu_send_mem_write(VuDev *dev, uint64_t guest_addr, uint32_t size,
> + uint8_t *data);
>
> /**
> * vu_queue_set_notification:
> --
> 2.48.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 9/9] vhost_user.rst: Add MEM_READ/WRITE messages
2025-02-17 16:40 ` [PATCH v4 9/9] vhost_user.rst: Add MEM_READ/WRITE messages Albert Esteve
@ 2025-02-18 11:00 ` Stefan Hajnoczi
2025-02-18 12:50 ` Albert Esteve
0 siblings, 1 reply; 36+ messages in thread
From: Stefan Hajnoczi @ 2025-02-18 11:00 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, slp, stevensd, Alex Bennée, Stefano Garzarella,
david, hi, mst, jasowang
[-- Attachment #1: Type: text/plain, Size: 2540 bytes --]
On Mon, Feb 17, 2025 at 05:40:12PM +0100, Albert Esteve wrote:
> Add MEM_READ/WRITE request to the vhost-user
> spec documentation.
>
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> ---
> docs/interop/vhost-user.rst | 33 +++++++++++++++++++++++++++++++++
> 1 file changed, 33 insertions(+)
>
> diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
> index 96156f1900..9f7a2c4cf7 100644
> --- a/docs/interop/vhost-user.rst
> +++ b/docs/interop/vhost-user.rst
> @@ -391,6 +391,7 @@ In QEMU the vhost-user message is implemented with the following struct:
> VhostUserTransferDeviceState transfer_state;
> VhostUserMMap mmap;
> VhostUserShMemConfig shmem;
> + VhostUserMemRWMsg mem_rw;
Is this struct defined anywhere in the spec?
> };
> } QEMU_PACKED VhostUserMsg;
>
> @@ -1938,6 +1939,38 @@ is sent by the front-end.
> given range shall correspond to the entirety of a valid mapped region.
> A reply is generated indicating whether unmapping succeeded.
>
> +``VHOST_USER_BACKEND_MEM_READ``
> + :id: 11
> + :equivalent ioctl: N/A
> + :request payload: ``struct VhostUserMemRWMsg``
> + :reply payload: N/A
> +
> + When the ``VHOST_USER_PROTOCOL_F_SHMEM`` protocol feature has been
> + successfully negotiated, this message can be submitted by the backends to
> + read a memory region that has failed to resolve a translation due to an
> + incomplete memory table, after another device called
> + ``VHOST_USER_BACKEND_SHMEM_MAP`` for the same region on a shared
> + descriptor file.
> +
> + This mechanism works as a fallback for resolving those memory
> + accesses and ensure that DMA works with Shared Memory Regions.
> +
> +``VHOST_USER_BACKEND_MEM_WRITE``
> + :id: 12
> + :equivalent ioctl: N/A
> + :request payload: ``struct VhostUserMemRWMsg``
> + :reply payload: N/A
> +
> + When the ``VHOST_USER_PROTOCOL_F_SHMEM`` protocol feature has been
> + successfully negotiated, this message can be submitted by the backends to
> + write a memory region that has failed due to resolve a translation an
> + incomplete memory table after another device called
> + ``VHOST_USER_BACKEND_SHMEM_MAP`` for the same region on a shared
> + descriptor file.
> +
> + This mechanism works as a fallback for resolving those memory
> + accesses and ensure that DMA works with Shared Memory Regions.
> +
> .. _reply_ack:
>
> VHOST_USER_PROTOCOL_F_REPLY_ACK
> --
> 2.48.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 9/9] vhost_user.rst: Add MEM_READ/WRITE messages
2025-02-18 11:00 ` Stefan Hajnoczi
@ 2025-02-18 12:50 ` Albert Esteve
0 siblings, 0 replies; 36+ messages in thread
From: Albert Esteve @ 2025-02-18 12:50 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, slp, stevensd, Alex Bennée, Stefano Garzarella,
david, hi, mst, jasowang
On Tue, Feb 18, 2025 at 12:00 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Mon, Feb 17, 2025 at 05:40:12PM +0100, Albert Esteve wrote:
> > Add MEM_READ/WRITE request to the vhost-user
> > spec documentation.
> >
> > Signed-off-by: Albert Esteve <aesteve@redhat.com>
> > ---
> > docs/interop/vhost-user.rst | 33 +++++++++++++++++++++++++++++++++
> > 1 file changed, 33 insertions(+)
> >
> > diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
> > index 96156f1900..9f7a2c4cf7 100644
> > --- a/docs/interop/vhost-user.rst
> > +++ b/docs/interop/vhost-user.rst
> > @@ -391,6 +391,7 @@ In QEMU the vhost-user message is implemented with the following struct:
> > VhostUserTransferDeviceState transfer_state;
> > VhostUserMMap mmap;
> > VhostUserShMemConfig shmem;
> > + VhostUserMemRWMsg mem_rw;
>
> Is this struct defined anywhere in the spec?
Ah, no, right. I did not see all the other structs defined, so I
assumed some were just to be found in the code. But then it wouldn't
serve as a specification... I realized I was not looking at the right
section.
VhostUserMMap is also missing, then. I will add them in the next
iteration in their respective commits.
>
> > };
> > } QEMU_PACKED VhostUserMsg;
> >
> > @@ -1938,6 +1939,38 @@ is sent by the front-end.
> > given range shall correspond to the entirety of a valid mapped region.
> > A reply is generated indicating whether unmapping succeeded.
> >
> > +``VHOST_USER_BACKEND_MEM_READ``
> > + :id: 11
> > + :equivalent ioctl: N/A
> > + :request payload: ``struct VhostUserMemRWMsg``
> > + :reply payload: N/A
> > +
> > + When the ``VHOST_USER_PROTOCOL_F_SHMEM`` protocol feature has been
> > + successfully negotiated, this message can be submitted by the backends to
> > + read a memory region that has failed to resolve a translation due to an
> > + incomplete memory table, after another device called
> > + ``VHOST_USER_BACKEND_SHMEM_MAP`` for the same region on a shared
> > + descriptor file.
> > +
> > + This mechanism works as a fallback for resolving those memory
> > + accesses and ensure that DMA works with Shared Memory Regions.
> > +
> > +``VHOST_USER_BACKEND_MEM_WRITE``
> > + :id: 12
> > + :equivalent ioctl: N/A
> > + :request payload: ``struct VhostUserMemRWMsg``
> > + :reply payload: N/A
> > +
> > + When the ``VHOST_USER_PROTOCOL_F_SHMEM`` protocol feature has been
> > + successfully negotiated, this message can be submitted by the backends to
> > + write a memory region that has failed due to resolve a translation an
> > + incomplete memory table after another device called
> > + ``VHOST_USER_BACKEND_SHMEM_MAP`` for the same region on a shared
> > + descriptor file.
> > +
> > + This mechanism works as a fallback for resolving those memory
> > + accesses and ensure that DMA works with Shared Memory Regions.
> > +
> > .. _reply_ack:
> >
> > VHOST_USER_PROTOCOL_F_REPLY_ACK
> > --
> > 2.48.1
> >
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 7/9] vhost-user-devive: Add shmem BAR
2025-02-18 10:55 ` Albert Esteve
@ 2025-02-18 13:25 ` Stefan Hajnoczi
2025-02-18 15:04 ` Albert Esteve
0 siblings, 1 reply; 36+ messages in thread
From: Stefan Hajnoczi @ 2025-02-18 13:25 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, slp, stevensd, Alex Bennée, Stefano Garzarella,
david, hi, mst, jasowang
[-- Attachment #1: Type: text/plain, Size: 8686 bytes --]
On Tue, Feb 18, 2025 at 11:55:33AM +0100, Albert Esteve wrote:
> On Tue, Feb 18, 2025 at 11:41 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > On Mon, Feb 17, 2025 at 05:40:10PM +0100, Albert Esteve wrote:
> > > Add a shmem BAR block in the vhost-user-device,
> > > which files can be directly mapped into.
> > >
> > > The number, shmid, and size of the VIRTIO Shared
> > > Memory subregions is retrieved through a
> > > get_shmem_config message sent by the
> > > vhost-user-base module on the realize step,
> > > after virtio_init().
> > >
> > > By default, if VHOST_USER_PROTOCOL_F_SHMEM
> > > feature is not supported by the backend,
> > > there is no cache.
> > >
> > > Signed-off-by: Albert Esteve <aesteve@redhat.com>
> > > ---
> > > hw/virtio/vhost-user-base.c | 47 +++++++++++++++++++++++++++++--
> > > hw/virtio/vhost-user-device-pci.c | 36 +++++++++++++++++++++--
> > > 2 files changed, 78 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/hw/virtio/vhost-user-base.c b/hw/virtio/vhost-user-base.c
> > > index 2bc3423326..8d4bca98a8 100644
> > > --- a/hw/virtio/vhost-user-base.c
> > > +++ b/hw/virtio/vhost-user-base.c
> > > @@ -16,6 +16,7 @@
> > > #include "hw/virtio/virtio-bus.h"
> > > #include "hw/virtio/vhost-user-base.h"
> > > #include "qemu/error-report.h"
> > > +#include "migration/blocker.h"
> > >
> > > static void vub_start(VirtIODevice *vdev)
> > > {
> > > @@ -271,7 +272,8 @@ static void vub_device_realize(DeviceState *dev, Error **errp)
> > > {
> > > VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > > VHostUserBase *vub = VHOST_USER_BASE(dev);
> > > - int ret;
> > > + uint64_t memory_sizes[VIRTIO_MAX_SHMEM_REGIONS];
> > > + int i, ret, nregions;
> > >
> > > if (!vub->chardev.chr) {
> > > error_setg(errp, "vhost-user-base: missing chardev");
> > > @@ -314,7 +316,7 @@ static void vub_device_realize(DeviceState *dev, Error **errp)
> > >
> > > /* Allocate queues */
> > > vub->vqs = g_ptr_array_sized_new(vub->num_vqs);
> > > - for (int i = 0; i < vub->num_vqs; i++) {
> > > + for (i = 0; i < vub->num_vqs; i++) {
> > > g_ptr_array_add(vub->vqs,
> > > virtio_add_queue(vdev, vub->vq_size,
> > > vub_handle_output));
> > > @@ -328,11 +330,50 @@ static void vub_device_realize(DeviceState *dev, Error **errp)
> > > VHOST_BACKEND_TYPE_USER, 0, errp);
> > >
> > > if (ret < 0) {
> > > - do_vhost_user_cleanup(vdev, vub);
> > > + goto err;
> > > + }
> > > +
> > > + ret = vub->vhost_dev.vhost_ops->vhost_get_shmem_config(&vub->vhost_dev,
> > > + &nregions,
> > > + memory_sizes,
> > > + errp);
> > > +
> > > + if (ret < 0) {
> > > + goto err;
> > > + }
> > > +
> > > + for (i = 0; i < nregions; i++) {
> > > + if (memory_sizes[i]) {
> > > + if (vub->vhost_dev.migration_blocker == NULL) {
> > > + error_setg(&vub->vhost_dev.migration_blocker,
> > > + "Migration disabled: devices with VIRTIO Shared Memory "
> > > + "Regions do not support migration yet.");
> > > + ret = migrate_add_blocker_normal(
> > > + &vub->vhost_dev.migration_blocker,
> > > + errp);
> > > +
> > > + if (ret < 0) {
> > > + goto err;
> > > + }
> > > + }
> > > +
> > > + if (memory_sizes[i] % qemu_real_host_page_size() != 0) {
> > > + error_setg(errp, "Shared memory %d size must be a power of 2 "
> > > + "no smaller than the page size", i);
> > > + goto err;
> > > + }
> > > +
> > > + memory_region_init(virtio_new_shmem_region(vdev)->mr,
> >
> > Does this code support non-contiguous shmids? For example, if a device
> > has two Shared Memory Regions defined in its spec but the first one is
> > optional, then the device might have memory_sizes[0] == 0 and
> > memory_sizes[1] > 0. In that case the Shared Memory Region must have
> > shmid 1 and not shmid 0.
>
> Yes, it does. That is guarded by ` if (memory_sizes[i]) {`, which only
> initializes the region if memory_sizes[i] > 0. The main downsize of
> that, is that it requires to send as many `memory_sizes` elements as
> the highest shmid for the device. But as it is, it is supported by
> this code.
shmids are not preserved when there are gaps:
for (i = 0; i < vdev->n_shmem_regions; i++) {
memory_region_add_subregion(&dev->shmembar, offset, mr);
virtio_pci_add_shm_cap(vpci_dev, VIRTIO_DEVICE_PCI_SHMEM_BAR,
offset, mr->size, i);
^
vdev->n_shmem_regions is incremented by virtio_new_shmem_region().
virtio_new_shmem_region() is only called on non-empty Shared Memory
Regions.
In the example I gave with empty shmid 0 and non-empty shmid 1 I think
we end up with vdev->n_shmem_regions == 1. shmdid 1 is exposed to the
guest with shmid 0.
Have I missed something?
> >
> > > + OBJECT(vdev), "vub-shm-" + i,
> > > + memory_sizes[i]);
> > > + }
> > > }
> > >
> > > qemu_chr_fe_set_handlers(&vub->chardev, NULL, NULL, vub_event, NULL,
> > > dev, NULL, true);
> > > + return;
> > > +err:
> > > + do_vhost_user_cleanup(vdev, vub);
> > > }
> > >
> > > static void vub_device_unrealize(DeviceState *dev)
> > > diff --git a/hw/virtio/vhost-user-device-pci.c b/hw/virtio/vhost-user-device-pci.c
> > > index efaf55d3dd..f215cae925 100644
> > > --- a/hw/virtio/vhost-user-device-pci.c
> > > +++ b/hw/virtio/vhost-user-device-pci.c
> > > @@ -8,14 +8,18 @@
> > > */
> > >
> > > #include "qemu/osdep.h"
> > > +#include "qapi/error.h"
> > > #include "hw/qdev-properties.h"
> > > #include "hw/virtio/vhost-user-base.h"
> > > #include "hw/virtio/virtio-pci.h"
> > >
> > > +#define VIRTIO_DEVICE_PCI_SHMEM_BAR 2
> > > +
> > > struct VHostUserDevicePCI {
> > > VirtIOPCIProxy parent_obj;
> > >
> > > VHostUserBase vub;
> > > + MemoryRegion shmembar;
> > > };
> > >
> > > #define TYPE_VHOST_USER_DEVICE_PCI "vhost-user-device-pci-base"
> > > @@ -25,10 +29,38 @@ OBJECT_DECLARE_SIMPLE_TYPE(VHostUserDevicePCI, VHOST_USER_DEVICE_PCI)
> > > static void vhost_user_device_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> > > {
> > > VHostUserDevicePCI *dev = VHOST_USER_DEVICE_PCI(vpci_dev);
> > > - DeviceState *vdev = DEVICE(&dev->vub);
> > > + DeviceState *dev_state = DEVICE(&dev->vub);
> > > + VirtIODevice *vdev = VIRTIO_DEVICE(dev_state);
> > > + MemoryRegion *mr;
> > > + uint64_t offset = 0, shmem_size = 0;
> > > + int i;
> > >
> > > vpci_dev->nvectors = 1;
> > > - qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
> > > + qdev_realize(dev_state, BUS(&vpci_dev->bus), errp);
> > > +
> > > + for (i = 0; i < vdev->n_shmem_regions; i++) {
> > > + mr = vdev->shmem_list[i].mr;
> > > + if (mr->size > UINT64_MAX - shmem_size) {
> > > + error_setg(errp, "Total shared memory required overflow");
> > > + return;
> > > + }
> > > + shmem_size = shmem_size + mr->size;
> > > + }
> > > + if (shmem_size) {
> > > + memory_region_init(&dev->shmembar, OBJECT(vpci_dev),
> > > + "vhost-device-pci-shmembar", shmem_size);
> > > + for (i = 0; i < vdev->n_shmem_regions; i++) {
> > > + memory_region_add_subregion(&dev->shmembar, offset, mr);
> > > + virtio_pci_add_shm_cap(vpci_dev, VIRTIO_DEVICE_PCI_SHMEM_BAR,
> > > + offset, mr->size, i);
> > > + offset = offset + mr->size;
> > > + }
> > > + pci_register_bar(&vpci_dev->pci_dev, VIRTIO_DEVICE_PCI_SHMEM_BAR,
> > > + PCI_BASE_ADDRESS_SPACE_MEMORY |
> > > + PCI_BASE_ADDRESS_MEM_PREFETCH |
> > > + PCI_BASE_ADDRESS_MEM_TYPE_64,
> > > + &dev->shmembar);
> > > + }
> > > }
> > >
> > > static void vhost_user_device_pci_class_init(ObjectClass *klass, void *data)
> > > --
> > > 2.48.1
> > >
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 7/9] vhost-user-devive: Add shmem BAR
2025-02-18 13:25 ` Stefan Hajnoczi
@ 2025-02-18 15:04 ` Albert Esteve
0 siblings, 0 replies; 36+ messages in thread
From: Albert Esteve @ 2025-02-18 15:04 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, slp, stevensd, Alex Bennée, Stefano Garzarella,
david, hi, mst, jasowang
On Tue, Feb 18, 2025 at 2:29 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Tue, Feb 18, 2025 at 11:55:33AM +0100, Albert Esteve wrote:
> > On Tue, Feb 18, 2025 at 11:41 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > >
> > > On Mon, Feb 17, 2025 at 05:40:10PM +0100, Albert Esteve wrote:
> > > > Add a shmem BAR block in the vhost-user-device,
> > > > which files can be directly mapped into.
> > > >
> > > > The number, shmid, and size of the VIRTIO Shared
> > > > Memory subregions is retrieved through a
> > > > get_shmem_config message sent by the
> > > > vhost-user-base module on the realize step,
> > > > after virtio_init().
> > > >
> > > > By default, if VHOST_USER_PROTOCOL_F_SHMEM
> > > > feature is not supported by the backend,
> > > > there is no cache.
> > > >
> > > > Signed-off-by: Albert Esteve <aesteve@redhat.com>
> > > > ---
> > > > hw/virtio/vhost-user-base.c | 47 +++++++++++++++++++++++++++++--
> > > > hw/virtio/vhost-user-device-pci.c | 36 +++++++++++++++++++++--
> > > > 2 files changed, 78 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/hw/virtio/vhost-user-base.c b/hw/virtio/vhost-user-base.c
> > > > index 2bc3423326..8d4bca98a8 100644
> > > > --- a/hw/virtio/vhost-user-base.c
> > > > +++ b/hw/virtio/vhost-user-base.c
> > > > @@ -16,6 +16,7 @@
> > > > #include "hw/virtio/virtio-bus.h"
> > > > #include "hw/virtio/vhost-user-base.h"
> > > > #include "qemu/error-report.h"
> > > > +#include "migration/blocker.h"
> > > >
> > > > static void vub_start(VirtIODevice *vdev)
> > > > {
> > > > @@ -271,7 +272,8 @@ static void vub_device_realize(DeviceState *dev, Error **errp)
> > > > {
> > > > VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > > > VHostUserBase *vub = VHOST_USER_BASE(dev);
> > > > - int ret;
> > > > + uint64_t memory_sizes[VIRTIO_MAX_SHMEM_REGIONS];
> > > > + int i, ret, nregions;
> > > >
> > > > if (!vub->chardev.chr) {
> > > > error_setg(errp, "vhost-user-base: missing chardev");
> > > > @@ -314,7 +316,7 @@ static void vub_device_realize(DeviceState *dev, Error **errp)
> > > >
> > > > /* Allocate queues */
> > > > vub->vqs = g_ptr_array_sized_new(vub->num_vqs);
> > > > - for (int i = 0; i < vub->num_vqs; i++) {
> > > > + for (i = 0; i < vub->num_vqs; i++) {
> > > > g_ptr_array_add(vub->vqs,
> > > > virtio_add_queue(vdev, vub->vq_size,
> > > > vub_handle_output));
> > > > @@ -328,11 +330,50 @@ static void vub_device_realize(DeviceState *dev, Error **errp)
> > > > VHOST_BACKEND_TYPE_USER, 0, errp);
> > > >
> > > > if (ret < 0) {
> > > > - do_vhost_user_cleanup(vdev, vub);
> > > > + goto err;
> > > > + }
> > > > +
> > > > + ret = vub->vhost_dev.vhost_ops->vhost_get_shmem_config(&vub->vhost_dev,
> > > > + &nregions,
> > > > + memory_sizes,
> > > > + errp);
> > > > +
> > > > + if (ret < 0) {
> > > > + goto err;
> > > > + }
> > > > +
> > > > + for (i = 0; i < nregions; i++) {
> > > > + if (memory_sizes[i]) {
> > > > + if (vub->vhost_dev.migration_blocker == NULL) {
> > > > + error_setg(&vub->vhost_dev.migration_blocker,
> > > > + "Migration disabled: devices with VIRTIO Shared Memory "
> > > > + "Regions do not support migration yet.");
> > > > + ret = migrate_add_blocker_normal(
> > > > + &vub->vhost_dev.migration_blocker,
> > > > + errp);
> > > > +
> > > > + if (ret < 0) {
> > > > + goto err;
> > > > + }
> > > > + }
> > > > +
> > > > + if (memory_sizes[i] % qemu_real_host_page_size() != 0) {
> > > > + error_setg(errp, "Shared memory %d size must be a power of 2 "
> > > > + "no smaller than the page size", i);
> > > > + goto err;
> > > > + }
> > > > +
> > > > + memory_region_init(virtio_new_shmem_region(vdev)->mr,
> > >
> > > Does this code support non-contiguous shmids? For example, if a device
> > > has two Shared Memory Regions defined in its spec but the first one is
> > > optional, then the device might have memory_sizes[0] == 0 and
> > > memory_sizes[1] > 0. In that case the Shared Memory Region must have
> > > shmid 1 and not shmid 0.
> >
> > Yes, it does. That is guarded by ` if (memory_sizes[i]) {`, which only
> > initializes the region if memory_sizes[i] > 0. The main downsize of
> > that, is that it requires to send as many `memory_sizes` elements as
> > the highest shmid for the device. But as it is, it is supported by
> > this code.
>
> shmids are not preserved when there are gaps:
>
> for (i = 0; i < vdev->n_shmem_regions; i++) {
> memory_region_add_subregion(&dev->shmembar, offset, mr);
> virtio_pci_add_shm_cap(vpci_dev, VIRTIO_DEVICE_PCI_SHMEM_BAR,
> offset, mr->size, i);
> ^
>
> vdev->n_shmem_regions is incremented by virtio_new_shmem_region().
> virtio_new_shmem_region() is only called on non-empty Shared Memory
> Regions.
>
> In the example I gave with empty shmid 0 and non-empty shmid 1 I think
> we end up with vdev->n_shmem_regions == 1. shmdid 1 is exposed to the
> guest with shmid 0.
Ah, right. I considered your example when I was doing it, but the code
is buggy indeed. The code that I tested is mostly on the shm map API
part, with a custom pci device.
As mentioned in the initial message, I will add tests for the next
iteration. Thanks for finding this one!
>
> Have I missed something?
>
> > >
> > > > + OBJECT(vdev), "vub-shm-" + i,
> > > > + memory_sizes[i]);
> > > > + }
> > > > }
> > > >
> > > > qemu_chr_fe_set_handlers(&vub->chardev, NULL, NULL, vub_event, NULL,
> > > > dev, NULL, true);
> > > > + return;
> > > > +err:
> > > > + do_vhost_user_cleanup(vdev, vub);
> > > > }
> > > >
> > > > static void vub_device_unrealize(DeviceState *dev)
> > > > diff --git a/hw/virtio/vhost-user-device-pci.c b/hw/virtio/vhost-user-device-pci.c
> > > > index efaf55d3dd..f215cae925 100644
> > > > --- a/hw/virtio/vhost-user-device-pci.c
> > > > +++ b/hw/virtio/vhost-user-device-pci.c
> > > > @@ -8,14 +8,18 @@
> > > > */
> > > >
> > > > #include "qemu/osdep.h"
> > > > +#include "qapi/error.h"
> > > > #include "hw/qdev-properties.h"
> > > > #include "hw/virtio/vhost-user-base.h"
> > > > #include "hw/virtio/virtio-pci.h"
> > > >
> > > > +#define VIRTIO_DEVICE_PCI_SHMEM_BAR 2
> > > > +
> > > > struct VHostUserDevicePCI {
> > > > VirtIOPCIProxy parent_obj;
> > > >
> > > > VHostUserBase vub;
> > > > + MemoryRegion shmembar;
> > > > };
> > > >
> > > > #define TYPE_VHOST_USER_DEVICE_PCI "vhost-user-device-pci-base"
> > > > @@ -25,10 +29,38 @@ OBJECT_DECLARE_SIMPLE_TYPE(VHostUserDevicePCI, VHOST_USER_DEVICE_PCI)
> > > > static void vhost_user_device_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> > > > {
> > > > VHostUserDevicePCI *dev = VHOST_USER_DEVICE_PCI(vpci_dev);
> > > > - DeviceState *vdev = DEVICE(&dev->vub);
> > > > + DeviceState *dev_state = DEVICE(&dev->vub);
> > > > + VirtIODevice *vdev = VIRTIO_DEVICE(dev_state);
> > > > + MemoryRegion *mr;
> > > > + uint64_t offset = 0, shmem_size = 0;
> > > > + int i;
> > > >
> > > > vpci_dev->nvectors = 1;
> > > > - qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
> > > > + qdev_realize(dev_state, BUS(&vpci_dev->bus), errp);
> > > > +
> > > > + for (i = 0; i < vdev->n_shmem_regions; i++) {
> > > > + mr = vdev->shmem_list[i].mr;
> > > > + if (mr->size > UINT64_MAX - shmem_size) {
> > > > + error_setg(errp, "Total shared memory required overflow");
> > > > + return;
> > > > + }
> > > > + shmem_size = shmem_size + mr->size;
> > > > + }
> > > > + if (shmem_size) {
> > > > + memory_region_init(&dev->shmembar, OBJECT(vpci_dev),
> > > > + "vhost-device-pci-shmembar", shmem_size);
> > > > + for (i = 0; i < vdev->n_shmem_regions; i++) {
> > > > + memory_region_add_subregion(&dev->shmembar, offset, mr);
> > > > + virtio_pci_add_shm_cap(vpci_dev, VIRTIO_DEVICE_PCI_SHMEM_BAR,
> > > > + offset, mr->size, i);
> > > > + offset = offset + mr->size;
> > > > + }
> > > > + pci_register_bar(&vpci_dev->pci_dev, VIRTIO_DEVICE_PCI_SHMEM_BAR,
> > > > + PCI_BASE_ADDRESS_SPACE_MEMORY |
> > > > + PCI_BASE_ADDRESS_MEM_PREFETCH |
> > > > + PCI_BASE_ADDRESS_MEM_TYPE_64,
> > > > + &dev->shmembar);
> > > > + }
> > > > }
> > > >
> > > > static void vhost_user_device_pci_class_init(ObjectClass *klass, void *data)
> > > > --
> > > > 2.48.1
> > > >
> >
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 1/9] vhost-user: Add VirtIO Shared Memory map request
2025-02-17 16:40 ` [PATCH v4 1/9] vhost-user: Add VirtIO Shared Memory map request Albert Esteve
2025-02-18 6:43 ` Stefan Hajnoczi
2025-02-18 10:19 ` Stefan Hajnoczi
@ 2025-02-20 10:59 ` Alyssa Ross
2 siblings, 0 replies; 36+ messages in thread
From: Alyssa Ross @ 2025-02-20 10:59 UTC (permalink / raw)
To: Albert Esteve, qemu-devel
Cc: slp, stevensd, Alex Bennée, Stefano Garzarella, stefanha,
david, mst, jasowang, Albert Esteve
[-- Attachment #1: Type: text/plain, Size: 736 bytes --]
Albert Esteve <aesteve@redhat.com> writes:
> @@ -192,6 +194,24 @@ typedef struct VhostUserShared {
> unsigned char uuid[16];
> } VhostUserShared;
>
> +/* For the flags field of VhostUserMMap */
> +#define VHOST_USER_FLAG_MAP_R (1u << 0)
> +#define VHOST_USER_FLAG_MAP_W (1u << 1)
> +
> +typedef struct {
> + /* VIRTIO Shared Memory Region ID */
> + uint8_t shmid;
> + uint8_t padding[7];
> + /* File offset */
> + uint64_t fd_offset;
> + /* Offset within the VIRTIO Shared Memory Region */
> + uint64_t shm_offset;
> + /* Size of the mapping */
> + uint64_t len;
> + /* Flags for the mmap operation, from VHOST_USER_FLAG_* */
Perhaps this should say VHOST_USER_FLAG_MAP_*?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests
2025-02-17 20:01 ` [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests David Hildenbrand
@ 2025-02-24 8:54 ` Albert Esteve
2025-02-24 9:16 ` David Hildenbrand
0 siblings, 1 reply; 36+ messages in thread
From: Albert Esteve @ 2025-02-24 8:54 UTC (permalink / raw)
To: David Hildenbrand
Cc: qemu-devel, slp, stevensd, Alex Bennée, Stefano Garzarella,
stefanha, hi, mst, jasowang
On Mon, Feb 17, 2025 at 9:01 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 17.02.25 17:40, Albert Esteve wrote:
> > Hi all,
> >
>
> Hi,
>
> looks like our debugging session was successfu :)
>
> One question below.
>
> > v3->v4
> > - Change mmap strategy to use RAM blocks
> > and subregions.
> > - Add new bitfield to qmp feature map
> > - Followed most review comments from
> > last iteration.
> > - Merged documentation patch again with
> > this one. Makes more sense to
> > review them together after all.
> > - Add documentation for MEM_READ/WRITE
> > messages.
> >
> > The goal of this patch is to support
> > dynamic fd-backed memory maps initiated
> > from vhost-user backends.
> > There are many devices that could already
> > benefit of this feature, e.g.,
> > virtiofs or virtio-gpu.
> >
> > After receiving the SHMEM_MAP/UNMAP request,
> > the frontend creates the RAMBlock form the
> > fd and maps it by adding it as a subregion
> > of the shared memory region container.
> >
> > The VIRTIO Shared Memory Region list is
> > declared in the `VirtIODevice` struct
> > to make it generic.
> >
> > TODO: There was a conversation on the
> > previous version around adding tests
> > to the patch (which I have acknowledged).
> > However, given the numerous changes
> > that the patch already has, I have
> > decided to send it early and collect
> > some feedback while I work on the
> > tests for the next iteration.
> > Given that I have been able to
> > test the implementation with
> > my local setup, I am more or less
> > confident that, at least, the code
> > is in a relatively sane state
> > so that no reviewing time is
> > wasted on broken patches.
> >
> > This patch also includes:
> > - SHMEM_CONFIG frontend request that is
> > specifically meant to allow generic
> > vhost-user-device frontend to be able to
> > query VIRTIO Shared Memory settings from the
> > backend (as this device is generic and agnostic
> > of the actual backend configuration).
> >
> > - MEM_READ/WRITE backend requests are
> > added to deal with a potential issue when having
> > multiple backends sharing a file descriptor.
> > When a backend calls SHMEM_MAP it makes
> > accessing to the region fail for other
> > backend as it is missing from their translation
> > table. So these requests are a fallback
> > for vhost-user memory translation fails.
>
> Can you elaborate what the issue here is?
>
> Why would SHMEM_MAP make accessing the region fail for other backends --
> what makes this missing from their translation?
This issue was raised by Stefan Hajnoczi in one of the first
iterations of this patchset, based upon previous David Gilbert's work
on the virtiofs DAX Window.
Let me paste here some of his remarks:
"""
Other backends don't see these mappings. If the guest submits a vring
descriptor referencing a mapping to another backend, then that backend
won't be able to access this memory.
"""
[...]
"""
A bit more detail:
Device A has a VIRTIO Shared Memory Region. An application mmaps that
memory (examples: guest userspace driver using Linux VFIO, a guest
kernel driver that exposes the memory to userspace via mmap, or guest
kernel DAX). The application passes that memory as an I/O buffer to
device B (e.g. O_DIRECT disk I/O).
The result is that device B's vhost-user backend receives a vring
descriptor that points to a guest memory address in device A's VIRTIO
Shared Memory Region. Since device B does not have this memory in its
table, it cannot translate the address and the device breaks.
"""
I have not triggered the issue myself. So the idea is that the next
patch will *definitively* include some testing for the commits that I
cannot verify with my local setup.
BR,
Albert.
>
> --
> Cheers,
>
> David / dhildenb
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests
2025-02-24 8:54 ` Albert Esteve
@ 2025-02-24 9:16 ` David Hildenbrand
2025-02-24 9:35 ` Albert Esteve
0 siblings, 1 reply; 36+ messages in thread
From: David Hildenbrand @ 2025-02-24 9:16 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, slp, stevensd, Alex Bennée, Stefano Garzarella,
stefanha, hi, mst, jasowang
On 24.02.25 09:54, Albert Esteve wrote:
> On Mon, Feb 17, 2025 at 9:01 PM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 17.02.25 17:40, Albert Esteve wrote:
>>> Hi all,
>>>
>>
>> Hi,
>>
>> looks like our debugging session was successfu :)
>>
>> One question below.
>>
>>> v3->v4
>>> - Change mmap strategy to use RAM blocks
>>> and subregions.
>>> - Add new bitfield to qmp feature map
>>> - Followed most review comments from
>>> last iteration.
>>> - Merged documentation patch again with
>>> this one. Makes more sense to
>>> review them together after all.
>>> - Add documentation for MEM_READ/WRITE
>>> messages.
>>>
>>> The goal of this patch is to support
>>> dynamic fd-backed memory maps initiated
>>> from vhost-user backends.
>>> There are many devices that could already
>>> benefit of this feature, e.g.,
>>> virtiofs or virtio-gpu.
>>>
>>> After receiving the SHMEM_MAP/UNMAP request,
>>> the frontend creates the RAMBlock form the
>>> fd and maps it by adding it as a subregion
>>> of the shared memory region container.
>>>
>>> The VIRTIO Shared Memory Region list is
>>> declared in the `VirtIODevice` struct
>>> to make it generic.
>>>
>>> TODO: There was a conversation on the
>>> previous version around adding tests
>>> to the patch (which I have acknowledged).
>>> However, given the numerous changes
>>> that the patch already has, I have
>>> decided to send it early and collect
>>> some feedback while I work on the
>>> tests for the next iteration.
>>> Given that I have been able to
>>> test the implementation with
>>> my local setup, I am more or less
>>> confident that, at least, the code
>>> is in a relatively sane state
>>> so that no reviewing time is
>>> wasted on broken patches.
>>>
>>> This patch also includes:
>>> - SHMEM_CONFIG frontend request that is
>>> specifically meant to allow generic
>>> vhost-user-device frontend to be able to
>>> query VIRTIO Shared Memory settings from the
>>> backend (as this device is generic and agnostic
>>> of the actual backend configuration).
>>>
>>> - MEM_READ/WRITE backend requests are
>>> added to deal with a potential issue when having
>>> multiple backends sharing a file descriptor.
>>> When a backend calls SHMEM_MAP it makes
>>> accessing to the region fail for other
>>> backend as it is missing from their translation
>>> table. So these requests are a fallback
>>> for vhost-user memory translation fails.
>>
>> Can you elaborate what the issue here is?
>>
>> Why would SHMEM_MAP make accessing the region fail for other backends --
>> what makes this missing from their translation?
>
> This issue was raised by Stefan Hajnoczi in one of the first
> iterations of this patchset, based upon previous David Gilbert's work
> on the virtiofs DAX Window.
>
> Let me paste here some of his remarks:
>
> """
> Other backends don't see these mappings. If the guest submits a vring
> descriptor referencing a mapping to another backend, then that backend
> won't be able to access this memory.
> """
> [...]
> """
> A bit more detail:
>
> Device A has a VIRTIO Shared Memory Region. An application mmaps that
> memory (examples: guest userspace driver using Linux VFIO, a guest
> kernel driver that exposes the memory to userspace via mmap, or guest
> kernel DAX). The application passes that memory as an I/O buffer to
> device B (e.g. O_DIRECT disk I/O).
>
> The result is that device B's vhost-user backend receives a vring
> descriptor that points to a guest memory address in device A's VIRTIO
> Shared Memory Region. Since device B does not have this memory in its
> table, it cannot translate the address and the device breaks.
> """
>
> I have not triggered the issue myself. So the idea is that the next
> patch will *definitively* include some testing for the commits that I
> cannot verify with my local setup.
Hah! But isn't that exact problem which is now solved by our rework?
Whatever is mapped in the VIRTIO Shared Memory Region will be
communicated to all other vhost-user devices. So they should have that
memory in their map and should be able to access it.
The only thing vhost-user devices cannot access are IIRC ram_device_ptr
memory regions (e.g., from vfio devices). But that is independent shared
memory regions.
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests
2025-02-24 9:16 ` David Hildenbrand
@ 2025-02-24 9:35 ` Albert Esteve
2025-02-24 9:49 ` David Hildenbrand
0 siblings, 1 reply; 36+ messages in thread
From: Albert Esteve @ 2025-02-24 9:35 UTC (permalink / raw)
To: David Hildenbrand
Cc: qemu-devel, slp, stevensd, Alex Bennée, Stefano Garzarella,
stefanha, hi, mst, jasowang
On Mon, Feb 24, 2025 at 10:16 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 24.02.25 09:54, Albert Esteve wrote:
> > On Mon, Feb 17, 2025 at 9:01 PM David Hildenbrand <david@redhat.com> wrote:
> >>
> >> On 17.02.25 17:40, Albert Esteve wrote:
> >>> Hi all,
> >>>
> >>
> >> Hi,
> >>
> >> looks like our debugging session was successfu :)
> >>
> >> One question below.
> >>
> >>> v3->v4
> >>> - Change mmap strategy to use RAM blocks
> >>> and subregions.
> >>> - Add new bitfield to qmp feature map
> >>> - Followed most review comments from
> >>> last iteration.
> >>> - Merged documentation patch again with
> >>> this one. Makes more sense to
> >>> review them together after all.
> >>> - Add documentation for MEM_READ/WRITE
> >>> messages.
> >>>
> >>> The goal of this patch is to support
> >>> dynamic fd-backed memory maps initiated
> >>> from vhost-user backends.
> >>> There are many devices that could already
> >>> benefit of this feature, e.g.,
> >>> virtiofs or virtio-gpu.
> >>>
> >>> After receiving the SHMEM_MAP/UNMAP request,
> >>> the frontend creates the RAMBlock form the
> >>> fd and maps it by adding it as a subregion
> >>> of the shared memory region container.
> >>>
> >>> The VIRTIO Shared Memory Region list is
> >>> declared in the `VirtIODevice` struct
> >>> to make it generic.
> >>>
> >>> TODO: There was a conversation on the
> >>> previous version around adding tests
> >>> to the patch (which I have acknowledged).
> >>> However, given the numerous changes
> >>> that the patch already has, I have
> >>> decided to send it early and collect
> >>> some feedback while I work on the
> >>> tests for the next iteration.
> >>> Given that I have been able to
> >>> test the implementation with
> >>> my local setup, I am more or less
> >>> confident that, at least, the code
> >>> is in a relatively sane state
> >>> so that no reviewing time is
> >>> wasted on broken patches.
> >>>
> >>> This patch also includes:
> >>> - SHMEM_CONFIG frontend request that is
> >>> specifically meant to allow generic
> >>> vhost-user-device frontend to be able to
> >>> query VIRTIO Shared Memory settings from the
> >>> backend (as this device is generic and agnostic
> >>> of the actual backend configuration).
> >>>
> >>> - MEM_READ/WRITE backend requests are
> >>> added to deal with a potential issue when having
> >>> multiple backends sharing a file descriptor.
> >>> When a backend calls SHMEM_MAP it makes
> >>> accessing to the region fail for other
> >>> backend as it is missing from their translation
> >>> table. So these requests are a fallback
> >>> for vhost-user memory translation fails.
> >>
> >> Can you elaborate what the issue here is?
> >>
> >> Why would SHMEM_MAP make accessing the region fail for other backends --
> >> what makes this missing from their translation?
> >
> > This issue was raised by Stefan Hajnoczi in one of the first
> > iterations of this patchset, based upon previous David Gilbert's work
> > on the virtiofs DAX Window.
> >
> > Let me paste here some of his remarks:
> >
> > """
> > Other backends don't see these mappings. If the guest submits a vring
> > descriptor referencing a mapping to another backend, then that backend
> > won't be able to access this memory.
> > """
> > [...]
> > """
> > A bit more detail:
> >
> > Device A has a VIRTIO Shared Memory Region. An application mmaps that
> > memory (examples: guest userspace driver using Linux VFIO, a guest
> > kernel driver that exposes the memory to userspace via mmap, or guest
> > kernel DAX). The application passes that memory as an I/O buffer to
> > device B (e.g. O_DIRECT disk I/O).
> >
> > The result is that device B's vhost-user backend receives a vring
> > descriptor that points to a guest memory address in device A's VIRTIO
> > Shared Memory Region. Since device B does not have this memory in its
> > table, it cannot translate the address and the device breaks.
> > """
> >
> > I have not triggered the issue myself. So the idea is that the next
> > patch will *definitively* include some testing for the commits that I
> > cannot verify with my local setup.
>
> Hah! But isn't that exact problem which is now solved by our rework?
>
> Whatever is mapped in the VIRTIO Shared Memory Region will be
> communicated to all other vhost-user devices. So they should have that
> memory in their map and should be able to access it.
You mean the SET_MEM_TABLE message after the vhost_commit is sent to
all vhost-user devices? I was not sure, as I was testing with a single
device, that would be great, and simplify the patch a lot.
>
> The only thing vhost-user devices cannot access are IIRC ram_device_ptr
> memory regions (e.g., from vfio devices). But that is independent shared
> memory regions.
>
> --
> Cheers,
>
> David / dhildenb
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests
2025-02-24 9:35 ` Albert Esteve
@ 2025-02-24 9:49 ` David Hildenbrand
2025-02-24 13:41 ` Albert Esteve
0 siblings, 1 reply; 36+ messages in thread
From: David Hildenbrand @ 2025-02-24 9:49 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, slp, stevensd, Alex Bennée, Stefano Garzarella,
stefanha, hi, mst, jasowang
On 24.02.25 10:35, Albert Esteve wrote:
> On Mon, Feb 24, 2025 at 10:16 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 24.02.25 09:54, Albert Esteve wrote:
>>> On Mon, Feb 17, 2025 at 9:01 PM David Hildenbrand <david@redhat.com> wrote:
>>>>
>>>> On 17.02.25 17:40, Albert Esteve wrote:
>>>>> Hi all,
>>>>>
>>>>
>>>> Hi,
>>>>
>>>> looks like our debugging session was successfu :)
>>>>
>>>> One question below.
>>>>
>>>>> v3->v4
>>>>> - Change mmap strategy to use RAM blocks
>>>>> and subregions.
>>>>> - Add new bitfield to qmp feature map
>>>>> - Followed most review comments from
>>>>> last iteration.
>>>>> - Merged documentation patch again with
>>>>> this one. Makes more sense to
>>>>> review them together after all.
>>>>> - Add documentation for MEM_READ/WRITE
>>>>> messages.
>>>>>
>>>>> The goal of this patch is to support
>>>>> dynamic fd-backed memory maps initiated
>>>>> from vhost-user backends.
>>>>> There are many devices that could already
>>>>> benefit of this feature, e.g.,
>>>>> virtiofs or virtio-gpu.
>>>>>
>>>>> After receiving the SHMEM_MAP/UNMAP request,
>>>>> the frontend creates the RAMBlock form the
>>>>> fd and maps it by adding it as a subregion
>>>>> of the shared memory region container.
>>>>>
>>>>> The VIRTIO Shared Memory Region list is
>>>>> declared in the `VirtIODevice` struct
>>>>> to make it generic.
>>>>>
>>>>> TODO: There was a conversation on the
>>>>> previous version around adding tests
>>>>> to the patch (which I have acknowledged).
>>>>> However, given the numerous changes
>>>>> that the patch already has, I have
>>>>> decided to send it early and collect
>>>>> some feedback while I work on the
>>>>> tests for the next iteration.
>>>>> Given that I have been able to
>>>>> test the implementation with
>>>>> my local setup, I am more or less
>>>>> confident that, at least, the code
>>>>> is in a relatively sane state
>>>>> so that no reviewing time is
>>>>> wasted on broken patches.
>>>>>
>>>>> This patch also includes:
>>>>> - SHMEM_CONFIG frontend request that is
>>>>> specifically meant to allow generic
>>>>> vhost-user-device frontend to be able to
>>>>> query VIRTIO Shared Memory settings from the
>>>>> backend (as this device is generic and agnostic
>>>>> of the actual backend configuration).
>>>>>
>>>>> - MEM_READ/WRITE backend requests are
>>>>> added to deal with a potential issue when having
>>>>> multiple backends sharing a file descriptor.
>>>>> When a backend calls SHMEM_MAP it makes
>>>>> accessing to the region fail for other
>>>>> backend as it is missing from their translation
>>>>> table. So these requests are a fallback
>>>>> for vhost-user memory translation fails.
>>>>
>>>> Can you elaborate what the issue here is?
>>>>
>>>> Why would SHMEM_MAP make accessing the region fail for other backends --
>>>> what makes this missing from their translation?
>>>
>>> This issue was raised by Stefan Hajnoczi in one of the first
>>> iterations of this patchset, based upon previous David Gilbert's work
>>> on the virtiofs DAX Window.
>>>
>>> Let me paste here some of his remarks:
>>>
>>> """
>>> Other backends don't see these mappings. If the guest submits a vring
>>> descriptor referencing a mapping to another backend, then that backend
>>> won't be able to access this memory.
>>> """
>>> [...]
>>> """
>>> A bit more detail:
>>>
>>> Device A has a VIRTIO Shared Memory Region. An application mmaps that
>>> memory (examples: guest userspace driver using Linux VFIO, a guest
>>> kernel driver that exposes the memory to userspace via mmap, or guest
>>> kernel DAX). The application passes that memory as an I/O buffer to
>>> device B (e.g. O_DIRECT disk I/O).
>>>
>>> The result is that device B's vhost-user backend receives a vring
>>> descriptor that points to a guest memory address in device A's VIRTIO
>>> Shared Memory Region. Since device B does not have this memory in its
>>> table, it cannot translate the address and the device breaks.
>>> """
>>>
>>> I have not triggered the issue myself. So the idea is that the next
>>> patch will *definitively* include some testing for the commits that I
>>> cannot verify with my local setup.
>>
>> Hah! But isn't that exact problem which is now solved by our rework?
>>
>> Whatever is mapped in the VIRTIO Shared Memory Region will be
>> communicated to all other vhost-user devices. So they should have that
>> memory in their map and should be able to access it.
>
> You mean the SET_MEM_TABLE message after the vhost_commit is sent to
> all vhost-user devices? I was not sure, as I was testing with a single
> device, that would be great, and simplify the patch a lot.
Yes, all vhost-user devices should be updated.
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests
2025-02-24 9:49 ` David Hildenbrand
@ 2025-02-24 13:41 ` Albert Esteve
2025-02-24 13:57 ` David Hildenbrand
0 siblings, 1 reply; 36+ messages in thread
From: Albert Esteve @ 2025-02-24 13:41 UTC (permalink / raw)
To: David Hildenbrand
Cc: qemu-devel, slp, stevensd, Alex Bennée, Stefano Garzarella,
stefanha, hi, mst, jasowang
On Mon, Feb 24, 2025 at 10:49 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 24.02.25 10:35, Albert Esteve wrote:
> > On Mon, Feb 24, 2025 at 10:16 AM David Hildenbrand <david@redhat.com> wrote:
> >>
> >> On 24.02.25 09:54, Albert Esteve wrote:
> >>> On Mon, Feb 17, 2025 at 9:01 PM David Hildenbrand <david@redhat.com> wrote:
> >>>>
> >>>> On 17.02.25 17:40, Albert Esteve wrote:
> >>>>> Hi all,
> >>>>>
> >>>>
> >>>> Hi,
> >>>>
> >>>> looks like our debugging session was successfu :)
> >>>>
> >>>> One question below.
> >>>>
> >>>>> v3->v4
> >>>>> - Change mmap strategy to use RAM blocks
> >>>>> and subregions.
> >>>>> - Add new bitfield to qmp feature map
> >>>>> - Followed most review comments from
> >>>>> last iteration.
> >>>>> - Merged documentation patch again with
> >>>>> this one. Makes more sense to
> >>>>> review them together after all.
> >>>>> - Add documentation for MEM_READ/WRITE
> >>>>> messages.
> >>>>>
> >>>>> The goal of this patch is to support
> >>>>> dynamic fd-backed memory maps initiated
> >>>>> from vhost-user backends.
> >>>>> There are many devices that could already
> >>>>> benefit of this feature, e.g.,
> >>>>> virtiofs or virtio-gpu.
> >>>>>
> >>>>> After receiving the SHMEM_MAP/UNMAP request,
> >>>>> the frontend creates the RAMBlock form the
> >>>>> fd and maps it by adding it as a subregion
> >>>>> of the shared memory region container.
> >>>>>
> >>>>> The VIRTIO Shared Memory Region list is
> >>>>> declared in the `VirtIODevice` struct
> >>>>> to make it generic.
> >>>>>
> >>>>> TODO: There was a conversation on the
> >>>>> previous version around adding tests
> >>>>> to the patch (which I have acknowledged).
> >>>>> However, given the numerous changes
> >>>>> that the patch already has, I have
> >>>>> decided to send it early and collect
> >>>>> some feedback while I work on the
> >>>>> tests for the next iteration.
> >>>>> Given that I have been able to
> >>>>> test the implementation with
> >>>>> my local setup, I am more or less
> >>>>> confident that, at least, the code
> >>>>> is in a relatively sane state
> >>>>> so that no reviewing time is
> >>>>> wasted on broken patches.
> >>>>>
> >>>>> This patch also includes:
> >>>>> - SHMEM_CONFIG frontend request that is
> >>>>> specifically meant to allow generic
> >>>>> vhost-user-device frontend to be able to
> >>>>> query VIRTIO Shared Memory settings from the
> >>>>> backend (as this device is generic and agnostic
> >>>>> of the actual backend configuration).
> >>>>>
> >>>>> - MEM_READ/WRITE backend requests are
> >>>>> added to deal with a potential issue when having
> >>>>> multiple backends sharing a file descriptor.
> >>>>> When a backend calls SHMEM_MAP it makes
> >>>>> accessing to the region fail for other
> >>>>> backend as it is missing from their translation
> >>>>> table. So these requests are a fallback
> >>>>> for vhost-user memory translation fails.
> >>>>
> >>>> Can you elaborate what the issue here is?
> >>>>
> >>>> Why would SHMEM_MAP make accessing the region fail for other backends --
> >>>> what makes this missing from their translation?
> >>>
> >>> This issue was raised by Stefan Hajnoczi in one of the first
> >>> iterations of this patchset, based upon previous David Gilbert's work
> >>> on the virtiofs DAX Window.
> >>>
> >>> Let me paste here some of his remarks:
> >>>
> >>> """
> >>> Other backends don't see these mappings. If the guest submits a vring
> >>> descriptor referencing a mapping to another backend, then that backend
> >>> won't be able to access this memory.
> >>> """
> >>> [...]
> >>> """
> >>> A bit more detail:
> >>>
> >>> Device A has a VIRTIO Shared Memory Region. An application mmaps that
> >>> memory (examples: guest userspace driver using Linux VFIO, a guest
> >>> kernel driver that exposes the memory to userspace via mmap, or guest
> >>> kernel DAX). The application passes that memory as an I/O buffer to
> >>> device B (e.g. O_DIRECT disk I/O).
> >>>
> >>> The result is that device B's vhost-user backend receives a vring
> >>> descriptor that points to a guest memory address in device A's VIRTIO
> >>> Shared Memory Region. Since device B does not have this memory in its
> >>> table, it cannot translate the address and the device breaks.
> >>> """
> >>>
> >>> I have not triggered the issue myself. So the idea is that the next
> >>> patch will *definitively* include some testing for the commits that I
> >>> cannot verify with my local setup.
> >>
> >> Hah! But isn't that exact problem which is now solved by our rework?
> >>
> >> Whatever is mapped in the VIRTIO Shared Memory Region will be
> >> communicated to all other vhost-user devices. So they should have that
> >> memory in their map and should be able to access it.
> >
> > You mean the SET_MEM_TABLE message after the vhost_commit is sent to
> > all vhost-user devices? I was not sure, as I was testing with a single
> > device, that would be great, and simplify the patch a lot.
>
> Yes, all vhost-user devices should be updated.
Then, I think I agree with you, it would seem that this approach
naturally solved the issue with address translation among different
devices, as they all get the most up-to-date memory table after each
mmap.
WDYT, @Stefan Hajnoczi ?
If we are unsure, maybe we can leave the MEM_READ/WRITE support as a
later extension, and try to integrate the rest of this patch first.
>
> --
> Cheers,
>
> David / dhildenb
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests
2025-02-24 13:41 ` Albert Esteve
@ 2025-02-24 13:57 ` David Hildenbrand
2025-02-24 15:15 ` Albert Esteve
0 siblings, 1 reply; 36+ messages in thread
From: David Hildenbrand @ 2025-02-24 13:57 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, slp, stevensd, Alex Bennée, Stefano Garzarella,
stefanha, hi, mst, jasowang
On 24.02.25 14:41, Albert Esteve wrote:
> On Mon, Feb 24, 2025 at 10:49 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 24.02.25 10:35, Albert Esteve wrote:
>>> On Mon, Feb 24, 2025 at 10:16 AM David Hildenbrand <david@redhat.com> wrote:
>>>>
>>>> On 24.02.25 09:54, Albert Esteve wrote:
>>>>> On Mon, Feb 17, 2025 at 9:01 PM David Hildenbrand <david@redhat.com> wrote:
>>>>>>
>>>>>> On 17.02.25 17:40, Albert Esteve wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> looks like our debugging session was successfu :)
>>>>>>
>>>>>> One question below.
>>>>>>
>>>>>>> v3->v4
>>>>>>> - Change mmap strategy to use RAM blocks
>>>>>>> and subregions.
>>>>>>> - Add new bitfield to qmp feature map
>>>>>>> - Followed most review comments from
>>>>>>> last iteration.
>>>>>>> - Merged documentation patch again with
>>>>>>> this one. Makes more sense to
>>>>>>> review them together after all.
>>>>>>> - Add documentation for MEM_READ/WRITE
>>>>>>> messages.
>>>>>>>
>>>>>>> The goal of this patch is to support
>>>>>>> dynamic fd-backed memory maps initiated
>>>>>>> from vhost-user backends.
>>>>>>> There are many devices that could already
>>>>>>> benefit of this feature, e.g.,
>>>>>>> virtiofs or virtio-gpu.
>>>>>>>
>>>>>>> After receiving the SHMEM_MAP/UNMAP request,
>>>>>>> the frontend creates the RAMBlock form the
>>>>>>> fd and maps it by adding it as a subregion
>>>>>>> of the shared memory region container.
>>>>>>>
>>>>>>> The VIRTIO Shared Memory Region list is
>>>>>>> declared in the `VirtIODevice` struct
>>>>>>> to make it generic.
>>>>>>>
>>>>>>> TODO: There was a conversation on the
>>>>>>> previous version around adding tests
>>>>>>> to the patch (which I have acknowledged).
>>>>>>> However, given the numerous changes
>>>>>>> that the patch already has, I have
>>>>>>> decided to send it early and collect
>>>>>>> some feedback while I work on the
>>>>>>> tests for the next iteration.
>>>>>>> Given that I have been able to
>>>>>>> test the implementation with
>>>>>>> my local setup, I am more or less
>>>>>>> confident that, at least, the code
>>>>>>> is in a relatively sane state
>>>>>>> so that no reviewing time is
>>>>>>> wasted on broken patches.
>>>>>>>
>>>>>>> This patch also includes:
>>>>>>> - SHMEM_CONFIG frontend request that is
>>>>>>> specifically meant to allow generic
>>>>>>> vhost-user-device frontend to be able to
>>>>>>> query VIRTIO Shared Memory settings from the
>>>>>>> backend (as this device is generic and agnostic
>>>>>>> of the actual backend configuration).
>>>>>>>
>>>>>>> - MEM_READ/WRITE backend requests are
>>>>>>> added to deal with a potential issue when having
>>>>>>> multiple backends sharing a file descriptor.
>>>>>>> When a backend calls SHMEM_MAP it makes
>>>>>>> accessing to the region fail for other
>>>>>>> backend as it is missing from their translation
>>>>>>> table. So these requests are a fallback
>>>>>>> for vhost-user memory translation fails.
>>>>>>
>>>>>> Can you elaborate what the issue here is?
>>>>>>
>>>>>> Why would SHMEM_MAP make accessing the region fail for other backends --
>>>>>> what makes this missing from their translation?
>>>>>
>>>>> This issue was raised by Stefan Hajnoczi in one of the first
>>>>> iterations of this patchset, based upon previous David Gilbert's work
>>>>> on the virtiofs DAX Window.
>>>>>
>>>>> Let me paste here some of his remarks:
>>>>>
>>>>> """
>>>>> Other backends don't see these mappings. If the guest submits a vring
>>>>> descriptor referencing a mapping to another backend, then that backend
>>>>> won't be able to access this memory.
>>>>> """
>>>>> [...]
>>>>> """
>>>>> A bit more detail:
>>>>>
>>>>> Device A has a VIRTIO Shared Memory Region. An application mmaps that
>>>>> memory (examples: guest userspace driver using Linux VFIO, a guest
>>>>> kernel driver that exposes the memory to userspace via mmap, or guest
>>>>> kernel DAX). The application passes that memory as an I/O buffer to
>>>>> device B (e.g. O_DIRECT disk I/O).
>>>>>
>>>>> The result is that device B's vhost-user backend receives a vring
>>>>> descriptor that points to a guest memory address in device A's VIRTIO
>>>>> Shared Memory Region. Since device B does not have this memory in its
>>>>> table, it cannot translate the address and the device breaks.
>>>>> """
>>>>>
>>>>> I have not triggered the issue myself. So the idea is that the next
>>>>> patch will *definitively* include some testing for the commits that I
>>>>> cannot verify with my local setup.
>>>>
>>>> Hah! But isn't that exact problem which is now solved by our rework?
>>>>
>>>> Whatever is mapped in the VIRTIO Shared Memory Region will be
>>>> communicated to all other vhost-user devices. So they should have that
>>>> memory in their map and should be able to access it.
>>>
>>> You mean the SET_MEM_TABLE message after the vhost_commit is sent to
>>> all vhost-user devices? I was not sure, as I was testing with a single
>>> device, that would be great, and simplify the patch a lot.
>>
>> Yes, all vhost-user devices should be updated.
>
> Then, I think I agree with you, it would seem that this approach
> naturally solved the issue with address translation among different
> devices, as they all get the most up-to-date memory table after each
> mmap.
>
> WDYT, @Stefan Hajnoczi ?
> If we are unsure, maybe we can leave the MEM_READ/WRITE support as a
> later extension, and try to integrate the rest of this patch first.
As commented offline, maybe one would want the option to enable the
alternative mode, where such updates (in the SHM region) are not sent to
vhost-user devices. In such a configuration, the MEM_READ / MEM_WRITE
would be unavoidable.
What comes to mind are vhost-user devices with limited number of
supported memslots.
No idea how relevant that really is, and how many SHM regions we will
see in practice.
I recently increased the number of supported memslots for rust-vmm and
libvhost-user, but not sure about other devices (in particular, dpdk,
spdk, and whether we really care about that here).
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests
2025-02-24 13:57 ` David Hildenbrand
@ 2025-02-24 15:15 ` Albert Esteve
2025-02-26 9:53 ` David Hildenbrand
0 siblings, 1 reply; 36+ messages in thread
From: Albert Esteve @ 2025-02-24 15:15 UTC (permalink / raw)
To: David Hildenbrand
Cc: qemu-devel, slp, stevensd, Alex Bennée, Stefano Garzarella,
stefanha, hi, mst, jasowang
On Mon, Feb 24, 2025 at 2:57 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 24.02.25 14:41, Albert Esteve wrote:
> > On Mon, Feb 24, 2025 at 10:49 AM David Hildenbrand <david@redhat.com> wrote:
> >>
> >> On 24.02.25 10:35, Albert Esteve wrote:
> >>> On Mon, Feb 24, 2025 at 10:16 AM David Hildenbrand <david@redhat.com> wrote:
> >>>>
> >>>> On 24.02.25 09:54, Albert Esteve wrote:
> >>>>> On Mon, Feb 17, 2025 at 9:01 PM David Hildenbrand <david@redhat.com> wrote:
> >>>>>>
> >>>>>> On 17.02.25 17:40, Albert Esteve wrote:
> >>>>>>> Hi all,
> >>>>>>>
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> looks like our debugging session was successfu :)
> >>>>>>
> >>>>>> One question below.
> >>>>>>
> >>>>>>> v3->v4
> >>>>>>> - Change mmap strategy to use RAM blocks
> >>>>>>> and subregions.
> >>>>>>> - Add new bitfield to qmp feature map
> >>>>>>> - Followed most review comments from
> >>>>>>> last iteration.
> >>>>>>> - Merged documentation patch again with
> >>>>>>> this one. Makes more sense to
> >>>>>>> review them together after all.
> >>>>>>> - Add documentation for MEM_READ/WRITE
> >>>>>>> messages.
> >>>>>>>
> >>>>>>> The goal of this patch is to support
> >>>>>>> dynamic fd-backed memory maps initiated
> >>>>>>> from vhost-user backends.
> >>>>>>> There are many devices that could already
> >>>>>>> benefit of this feature, e.g.,
> >>>>>>> virtiofs or virtio-gpu.
> >>>>>>>
> >>>>>>> After receiving the SHMEM_MAP/UNMAP request,
> >>>>>>> the frontend creates the RAMBlock form the
> >>>>>>> fd and maps it by adding it as a subregion
> >>>>>>> of the shared memory region container.
> >>>>>>>
> >>>>>>> The VIRTIO Shared Memory Region list is
> >>>>>>> declared in the `VirtIODevice` struct
> >>>>>>> to make it generic.
> >>>>>>>
> >>>>>>> TODO: There was a conversation on the
> >>>>>>> previous version around adding tests
> >>>>>>> to the patch (which I have acknowledged).
> >>>>>>> However, given the numerous changes
> >>>>>>> that the patch already has, I have
> >>>>>>> decided to send it early and collect
> >>>>>>> some feedback while I work on the
> >>>>>>> tests for the next iteration.
> >>>>>>> Given that I have been able to
> >>>>>>> test the implementation with
> >>>>>>> my local setup, I am more or less
> >>>>>>> confident that, at least, the code
> >>>>>>> is in a relatively sane state
> >>>>>>> so that no reviewing time is
> >>>>>>> wasted on broken patches.
> >>>>>>>
> >>>>>>> This patch also includes:
> >>>>>>> - SHMEM_CONFIG frontend request that is
> >>>>>>> specifically meant to allow generic
> >>>>>>> vhost-user-device frontend to be able to
> >>>>>>> query VIRTIO Shared Memory settings from the
> >>>>>>> backend (as this device is generic and agnostic
> >>>>>>> of the actual backend configuration).
> >>>>>>>
> >>>>>>> - MEM_READ/WRITE backend requests are
> >>>>>>> added to deal with a potential issue when having
> >>>>>>> multiple backends sharing a file descriptor.
> >>>>>>> When a backend calls SHMEM_MAP it makes
> >>>>>>> accessing to the region fail for other
> >>>>>>> backend as it is missing from their translation
> >>>>>>> table. So these requests are a fallback
> >>>>>>> for vhost-user memory translation fails.
> >>>>>>
> >>>>>> Can you elaborate what the issue here is?
> >>>>>>
> >>>>>> Why would SHMEM_MAP make accessing the region fail for other backends --
> >>>>>> what makes this missing from their translation?
> >>>>>
> >>>>> This issue was raised by Stefan Hajnoczi in one of the first
> >>>>> iterations of this patchset, based upon previous David Gilbert's work
> >>>>> on the virtiofs DAX Window.
> >>>>>
> >>>>> Let me paste here some of his remarks:
> >>>>>
> >>>>> """
> >>>>> Other backends don't see these mappings. If the guest submits a vring
> >>>>> descriptor referencing a mapping to another backend, then that backend
> >>>>> won't be able to access this memory.
> >>>>> """
> >>>>> [...]
> >>>>> """
> >>>>> A bit more detail:
> >>>>>
> >>>>> Device A has a VIRTIO Shared Memory Region. An application mmaps that
> >>>>> memory (examples: guest userspace driver using Linux VFIO, a guest
> >>>>> kernel driver that exposes the memory to userspace via mmap, or guest
> >>>>> kernel DAX). The application passes that memory as an I/O buffer to
> >>>>> device B (e.g. O_DIRECT disk I/O).
> >>>>>
> >>>>> The result is that device B's vhost-user backend receives a vring
> >>>>> descriptor that points to a guest memory address in device A's VIRTIO
> >>>>> Shared Memory Region. Since device B does not have this memory in its
> >>>>> table, it cannot translate the address and the device breaks.
> >>>>> """
> >>>>>
> >>>>> I have not triggered the issue myself. So the idea is that the next
> >>>>> patch will *definitively* include some testing for the commits that I
> >>>>> cannot verify with my local setup.
> >>>>
> >>>> Hah! But isn't that exact problem which is now solved by our rework?
> >>>>
> >>>> Whatever is mapped in the VIRTIO Shared Memory Region will be
> >>>> communicated to all other vhost-user devices. So they should have that
> >>>> memory in their map and should be able to access it.
> >>>
> >>> You mean the SET_MEM_TABLE message after the vhost_commit is sent to
> >>> all vhost-user devices? I was not sure, as I was testing with a single
> >>> device, that would be great, and simplify the patch a lot.
> >>
> >> Yes, all vhost-user devices should be updated.
> >
> > Then, I think I agree with you, it would seem that this approach
> > naturally solved the issue with address translation among different
> > devices, as they all get the most up-to-date memory table after each
> > mmap.
> >
> > WDYT, @Stefan Hajnoczi ?
> > If we are unsure, maybe we can leave the MEM_READ/WRITE support as a
> > later extension, and try to integrate the rest of this patch first.
>
> As commented offline, maybe one would want the option to enable the
> alternative mode, where such updates (in the SHM region) are not sent to
> vhost-user devices. In such a configuration, the MEM_READ / MEM_WRITE
> would be unavoidable.
At first, I remember we discussed two options, having update messages
sent to all devices (which was deemed as potentially racy), or using
MEM_READ / MEM _WRITE messages. With this version of the patch there
is no option to avoid the mem_table update messages, which brings me
to my point in the previous message: it may make sense to continue
with this patch without MEM_READ/WRITE support, and leave that and the
option to make mem_table updates optional for a followup patch?
>
> What comes to mind are vhost-user devices with limited number of
> supported memslots.
>
> No idea how relevant that really is, and how many SHM regions we will
> see in practice.
In general, from what I see they usually require 1 or 2 regions,
except for virtio-scmi which requires >256.
>
> I recently increased the number of supported memslots for rust-vmm and
> libvhost-user, but not sure about other devices (in particular, dpdk,
> spdk, and whether we really care about that here).
>
> --
> Cheers,
>
> David / dhildenb
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests
2025-02-24 15:15 ` Albert Esteve
@ 2025-02-26 9:53 ` David Hildenbrand
2025-02-27 7:10 ` Stefan Hajnoczi
0 siblings, 1 reply; 36+ messages in thread
From: David Hildenbrand @ 2025-02-26 9:53 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, slp, stevensd, Alex Bennée, Stefano Garzarella,
stefanha, hi, mst, jasowang
>> As commented offline, maybe one would want the option to enable the
>> alternative mode, where such updates (in the SHM region) are not sent to
>> vhost-user devices. In such a configuration, the MEM_READ / MEM_WRITE
>> would be unavoidable.
>
> At first, I remember we discussed two options, having update messages
> sent to all devices (which was deemed as potentially racy), or using
> MEM_READ / MEM _WRITE messages. With this version of the patch there
> is no option to avoid the mem_table update messages, which brings me
> to my point in the previous message: it may make sense to continue
> with this patch without MEM_READ/WRITE support, and leave that and the
> option to make mem_table updates optional for a followup patch?
IMHO that would work for me.
>
>>
>> What comes to mind are vhost-user devices with limited number of
>> supported memslots.
>>
>> No idea how relevant that really is, and how many SHM regions we will
>> see in practice.
>
> In general, from what I see they usually require 1 or 2 regions,
> except for virtio-scmi which requires >256.
1/2 regions are not a problem. Once we're in the hundreds for a single
device, it will likely start being a problem, especially when you have more
such devices.
BUT, it would likely be a problem even with the alternative approach where
we don't communicate these regions to vhost-user: IIRC, vhost-net in
the kernel is usually limited to a maximum of 509 memslots as well as
default. Similarly, older KVM only supports a total of 509 memslots.
See https://virtio-mem.gitlab.io/user-guide/user-guide-qemu.html
"Compatibility with vhost-net and vhost-user".
In libvhost-user, and rust-vmm, we have a similar limit of ~509.
Note that for memory devices (DIMMs, virtio-mem), we'll use up to 256
memslots in case all devices support 509 memslots.
See MEMORY_DEVICES_SOFT_MEMSLOT_LIMIT:
/*
* Traditionally, KVM/vhost in many setups supported 509 memslots, whereby
* 253 memslots were "reserved" for boot memory and other devices (such
* as PCI BARs, which can get mapped dynamically) and 256 memslots were
* dedicated for DIMMs. These magic numbers worked reliably in the past.
*
* Further, using many memslots can negatively affect performance, so setting
* the soft-limit of memslots used by memory devices to the traditional
* DIMM limit of 256 sounds reasonable.
*
* If we have less than 509 memslots, we will instruct memory devices that
* support automatically deciding how many memslots to use to only use a single
* one.
*
* Hotplugging vhost devices with at least 509 memslots is not expected to
* cause problems, not even when devices automatically decided how many memslots
* to use.
*/
#define MEMORY_DEVICES_SOFT_MEMSLOT_LIMIT 256
#define MEMORY_DEVICES_SAFE_MAX_MEMSLOTS 509
That changes once you have some vhost-user devices consume combined with boot
memory more than 253 memslots.
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests
2025-02-26 9:53 ` David Hildenbrand
@ 2025-02-27 7:10 ` Stefan Hajnoczi
0 siblings, 0 replies; 36+ messages in thread
From: Stefan Hajnoczi @ 2025-02-27 7:10 UTC (permalink / raw)
To: David Hildenbrand
Cc: Albert Esteve, qemu-devel, slp, stevensd, Alex Bennée,
Stefano Garzarella, hi, mst, jasowang
[-- Attachment #1: Type: text/plain, Size: 3540 bytes --]
On Wed, Feb 26, 2025 at 10:53:01AM +0100, David Hildenbrand wrote:
> > > As commented offline, maybe one would want the option to enable the
> > > alternative mode, where such updates (in the SHM region) are not sent to
> > > vhost-user devices. In such a configuration, the MEM_READ / MEM_WRITE
> > > would be unavoidable.
> >
> > At first, I remember we discussed two options, having update messages
> > sent to all devices (which was deemed as potentially racy), or using
> > MEM_READ / MEM _WRITE messages. With this version of the patch there
> > is no option to avoid the mem_table update messages, which brings me
> > to my point in the previous message: it may make sense to continue
> > with this patch without MEM_READ/WRITE support, and leave that and the
> > option to make mem_table updates optional for a followup patch?
>
> IMHO that would work for me.
I'm happy with dropping MEM_READ/WRITE. If the memslots limit becomes a
problem then it will be necessary to think about handling things
differently, but there are many possible uses of VIRTIO Shared Memory
Regions that will not hit the limit and I don't see a need to hold them
back.
Stefan
>
> >
> > >
> > > What comes to mind are vhost-user devices with limited number of
> > > supported memslots.
> > >
> > > No idea how relevant that really is, and how many SHM regions we will
> > > see in practice.
> >
> > In general, from what I see they usually require 1 or 2 regions,
> > except for virtio-scmi which requires >256.
>
> 1/2 regions are not a problem. Once we're in the hundreds for a single
> device, it will likely start being a problem, especially when you have more
> such devices.
>
> BUT, it would likely be a problem even with the alternative approach where
> we don't communicate these regions to vhost-user: IIRC, vhost-net in
> the kernel is usually limited to a maximum of 509 memslots as well as
> default. Similarly, older KVM only supports a total of 509 memslots.
>
> See https://virtio-mem.gitlab.io/user-guide/user-guide-qemu.html
> "Compatibility with vhost-net and vhost-user".
>
> In libvhost-user, and rust-vmm, we have a similar limit of ~509.
>
>
> Note that for memory devices (DIMMs, virtio-mem), we'll use up to 256
> memslots in case all devices support 509 memslots.
> See MEMORY_DEVICES_SOFT_MEMSLOT_LIMIT:
>
> /*
> * Traditionally, KVM/vhost in many setups supported 509 memslots, whereby
> * 253 memslots were "reserved" for boot memory and other devices (such
> * as PCI BARs, which can get mapped dynamically) and 256 memslots were
> * dedicated for DIMMs. These magic numbers worked reliably in the past.
> *
> * Further, using many memslots can negatively affect performance, so setting
> * the soft-limit of memslots used by memory devices to the traditional
> * DIMM limit of 256 sounds reasonable.
> *
> * If we have less than 509 memslots, we will instruct memory devices that
> * support automatically deciding how many memslots to use to only use a single
> * one.
> *
> * Hotplugging vhost devices with at least 509 memslots is not expected to
> * cause problems, not even when devices automatically decided how many memslots
> * to use.
> */
> #define MEMORY_DEVICES_SOFT_MEMSLOT_LIMIT 256
> #define MEMORY_DEVICES_SAFE_MAX_MEMSLOTS 509
>
>
> That changes once you have some vhost-user devices consume combined with boot
> memory more than 253 memslots.
>
> --
> Cheers,
>
> David / dhildenb
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v4 1/9] vhost-user: Add VirtIO Shared Memory map request
2025-02-18 6:43 ` Stefan Hajnoczi
2025-02-18 10:33 ` Albert Esteve
@ 2025-03-06 14:48 ` Albert Esteve
1 sibling, 0 replies; 36+ messages in thread
From: Albert Esteve @ 2025-03-06 14:48 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, slp, stevensd, Alex Bennée, Stefano Garzarella,
david, hi, mst, jasowang
On Tue, Feb 18, 2025 at 7:43 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Mon, Feb 17, 2025 at 05:40:04PM +0100, Albert Esteve wrote:
> > Add SHMEM_MAP/UNMAP requests to vhost-user to
> > handle VIRTIO Shared Memory mappings.
> >
> > This request allows backends to dynamically map
> > fds into a VIRTIO Shared Memory Region indentified
> > by its `shmid`. The map is performed by calling
> > `memory_region_init_ram_from_fd` and adding the
> > new region as a subregion of the shmem container
> > MR. Then, the fd memory is advertised to the
> > driver as a base addres + offset, so it can be
> > read/written (depending on the mmap flags
> > requested) while it is valid.
> >
> > The backend can unmap the memory range
> > in a given VIRTIO Shared Memory Region (again,
> > identified by its `shmid`), to free it.
> > Upon receiving this message, the front-end
> > must delete the MR as a subregion of
> > the shmem container region and free its
> > resources.
> >
> > Note that commit all these operations need
> > to be delayed to after we respond the request
> > to the backend to avoid deadlocks.
> >
> > The device model needs to create VirtSharedMemory
> > instances for the VirtIO Shared Memory Regions
> > and add them to the `VirtIODevice` instance.
> >
> > Signed-off-by: Albert Esteve <aesteve@redhat.com>
> > ---
> > hw/virtio/vhost-user.c | 134 ++++++++++++++++++++++
> > hw/virtio/virtio.c | 81 +++++++++++++
> > include/hw/virtio/virtio.h | 27 +++++
> > subprojects/libvhost-user/libvhost-user.c | 70 +++++++++++
> > subprojects/libvhost-user/libvhost-user.h | 55 +++++++++
> > 5 files changed, 367 insertions(+)
> >
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index 267b612587..d88e6f8c3c 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -115,6 +115,8 @@ typedef enum VhostUserBackendRequest {
> > VHOST_USER_BACKEND_SHARED_OBJECT_ADD = 6,
> > VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE = 7,
> > VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP = 8,
> > + VHOST_USER_BACKEND_SHMEM_MAP = 9,
> > + VHOST_USER_BACKEND_SHMEM_UNMAP = 10,
> > VHOST_USER_BACKEND_MAX
> > } VhostUserBackendRequest;
> >
> > @@ -192,6 +194,24 @@ typedef struct VhostUserShared {
> > unsigned char uuid[16];
> > } VhostUserShared;
> >
> > +/* For the flags field of VhostUserMMap */
> > +#define VHOST_USER_FLAG_MAP_R (1u << 0)
> > +#define VHOST_USER_FLAG_MAP_W (1u << 1)
> > +
> > +typedef struct {
> > + /* VIRTIO Shared Memory Region ID */
> > + uint8_t shmid;
> > + uint8_t padding[7];
> > + /* File offset */
> > + uint64_t fd_offset;
> > + /* Offset within the VIRTIO Shared Memory Region */
> > + uint64_t shm_offset;
> > + /* Size of the mapping */
> > + uint64_t len;
> > + /* Flags for the mmap operation, from VHOST_USER_FLAG_* */
> > + uint64_t flags;
> > +} VhostUserMMap;
> > +
> > typedef struct {
> > VhostUserRequest request;
> >
> > @@ -224,6 +244,7 @@ typedef union {
> > VhostUserInflight inflight;
> > VhostUserShared object;
> > VhostUserTransferDeviceState transfer_state;
> > + VhostUserMMap mmap;
> > } VhostUserPayload;
> >
> > typedef struct VhostUserMsg {
> > @@ -1770,6 +1791,111 @@ vhost_user_backend_handle_shared_object_lookup(struct vhost_user *u,
> > return 0;
> > }
> >
> > +static int
> > +vhost_user_backend_handle_shmem_map(struct vhost_dev *dev,
> > + QIOChannel *ioc,
> > + VhostUserHeader *hdr,
> > + VhostUserPayload *payload,
> > + int fd, Error **errp)
> > +{
> > + VirtSharedMemory *shmem = NULL;
> > + VhostUserMMap *vu_mmap = &payload->mmap;
> > + g_autoptr(GString) shm_name = g_string_new(NULL);
> > +
> > + if (fd < 0) {
> > + error_report("Bad fd for map");
> > + return -EBADF;
> > + }
> > +
> > + if (!dev->vdev->shmem_list ||
> > + dev->vdev->n_shmem_regions <= vu_mmap->shmid) {
> > + error_report("Device only has %d VIRTIO Shared Memory Regions. "
> > + "Requested ID: %d",
> > + dev->vdev->n_shmem_regions, vu_mmap->shmid);
> > + return -EFAULT;
> > + }
> > +
> > + shmem = &dev->vdev->shmem_list[vu_mmap->shmid];
> > +
> > + if (!shmem) {
> > + error_report("VIRTIO Shared Memory Region at "
> > + "ID %d unitialized", vu_mmap->shmid);
> > + return -EFAULT;
> > + }
> > +
> > + if ((vu_mmap->shm_offset + vu_mmap->len) < vu_mmap->len ||
> > + (vu_mmap->shm_offset + vu_mmap->len) > shmem->mr->size) {
> > + error_report("Bad offset/len for mmap %" PRIx64 "+%" PRIx64,
> > + vu_mmap->shm_offset, vu_mmap->len);
> > + return -EFAULT;
> > + }
> > +
> > + g_string_printf(shm_name, "virtio-shm%i-%lu",
> > + vu_mmap->shmid, vu_mmap->shm_offset);
> > +
> > + memory_region_transaction_begin();
> > + virtio_add_shmem_map(shmem, shm_name->str, vu_mmap->shm_offset,
> > + vu_mmap->fd_offset, vu_mmap->len, fd, errp);
>
> Error handling is missing. In particular, if virtio_add_shmem_map() sets
> errp due to an error, then vhost_user_send_resp() will crash if it also
> needs to set the error. An Error object can only be filled in once.
Right, I think I am making my life more difficult with this. I will
omit the errp parameter in this function and just `error_print` and
return error.
>
> > +
> > + hdr->size = sizeof(payload->u64);
> > + vhost_user_send_resp(ioc, hdr, payload, errp);
>
> payload->u64 has not been initialized. It contains the contents of
> payload->mmap, which is not the u64 return value that must be sent here.
> I think something like the following is necessary:
> payload->u64 = 0;
Fair, in my tests the backend was not taking care of the return value,
so I did not care about it either. After, when I cleaned up the code
to send the patch, forgot this piece.
But I was looking at this and found a more problematic issue. Calling
`vhost_user_send_resp()` from the handler like this is fine if it does
not fail, because the function deactivates the
VHOST_USER_NEED_REPLY_MASK bit and thus it is skipped from
`backend_read()`. However, if it does fail in the handler, the goto
err path in `backend_read()` is not executed.
Currently it is not even considering the return boolean, but that's
easy to fix. However sending the response from the handler makes it
impossible to discern whether it failed in a previous check or in the
`vhost_user_send_resp()`. And if VHOST_USER_NEED_REPLY_MASK is unset
the return value is basically ignored.
The reason for doing it within the handler in the first place was to
hold the memory_transaction commit to after the response, and still
call the commit within the handler function. Otherwise, we need to run
`memory_region_transaction_commit();` from `backend_read()` for
`VHOST_USER_BACKEND_SHMEM_MAP` and `VHOST_USER_BACKEND_SHMEM_UNMAP`
message types, which makes the code arguably less nice. But that will
go through the error path in `backend_read()` and do
`close_backend_channel()`, which is what we want.
I hope I explained it clear enough. Any comments would be appreciated!
BR,
Albert.
>
> > + memory_region_transaction_commit();
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +vhost_user_backend_handle_shmem_unmap(struct vhost_dev *dev,
> > + QIOChannel *ioc,
> > + VhostUserHeader *hdr,
> > + VhostUserPayload *payload,
> > + Error **errp)
> > +{
> > + VirtSharedMemory *shmem = NULL;
> > + MappedMemoryRegion *mmap = NULL;
> > + VhostUserMMap *vu_mmap = &payload->mmap;
> > +
> > + if (!dev->vdev->shmem_list ||
> > + dev->vdev->n_shmem_regions <= vu_mmap->shmid) {
> > + error_report("Device only has %d VIRTIO Shared Memory Regions. "
> > + "Requested ID: %d",
> > + dev->vdev->n_shmem_regions, vu_mmap->shmid);
> > + return -EFAULT;
> > + }
> > +
> > + shmem = &dev->vdev->shmem_list[vu_mmap->shmid];
> > +
> > + if (!shmem) {
> > + error_report("VIRTIO Shared Memory Region at "
> > + "ID %d unitialized", vu_mmap->shmid);
> > + return -EFAULT;
> > + }
> > +
> > + if ((vu_mmap->shm_offset + vu_mmap->len) < vu_mmap->len ||
> > + (vu_mmap->shm_offset + vu_mmap->len) > shmem->mr->size) {
> > + error_report("Bad offset/len for unmmap %" PRIx64 "+%" PRIx64,
> > + vu_mmap->shm_offset, vu_mmap->len);
> > + return -EFAULT;
> > + }
> > +
> > + mmap = virtio_find_shmem_map(shmem, vu_mmap->shm_offset, vu_mmap->len);
> > + if (!mmap) {
> > + return -EFAULT;
> > + }
> > +
> > + memory_region_transaction_begin();
> > + memory_region_del_subregion(shmem->mr, mmap->mem);
> > +
> > + hdr->size = sizeof(payload->u64);
> > + vhost_user_send_resp(ioc, hdr, payload, errp);
>
> Same uninitialized payload->u64 issue here.
>
> > + memory_region_transaction_commit();
> > +
> > + /* Free the MemoryRegion only after vhost_commit */
> > + virtio_del_shmem_map(shmem, vu_mmap->shm_offset, vu_mmap->len);
> > +
> > + return 0;
> > +}
> > +
> > static void close_backend_channel(struct vhost_user *u)
> > {
> > g_source_destroy(u->backend_src);
> > @@ -1837,6 +1963,14 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition,
> > case VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP:
> > ret = vhost_user_backend_handle_shared_object_lookup(dev->opaque, ioc,
> > &hdr, &payload);
> > + case VHOST_USER_BACKEND_SHMEM_MAP:
> > + ret = vhost_user_backend_handle_shmem_map(dev, ioc, &hdr, &payload,
> > + fd ? fd[0] : -1, &local_err);
> > + break;
> > + case VHOST_USER_BACKEND_SHMEM_UNMAP:
> > + ret = vhost_user_backend_handle_shmem_unmap(dev, ioc, &hdr, &payload,
> > + &local_err);
> > + break;
> > break;
> > default:
> > error_report("Received unexpected msg type: %d.", hdr.request);
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 85110bce37..47d0ddb820 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -3063,6 +3063,75 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f)
> > return vmstate_save_state(f, &vmstate_virtio, vdev, NULL);
> > }
> >
> > +VirtSharedMemory *virtio_new_shmem_region(VirtIODevice *vdev)
> > +{
> > + ++vdev->n_shmem_regions;
> > + vdev->shmem_list = g_renew(VirtSharedMemory, vdev->shmem_list,
> > + vdev->n_shmem_regions);
> > + vdev->shmem_list[vdev->n_shmem_regions - 1].mr = g_new0(MemoryRegion, 1);
> > + QTAILQ_INIT(&vdev->shmem_list[vdev->n_shmem_regions - 1].mmaps);
>
> QTAILQ cannot be used inside a struct that is reallocated using
> g_renew() because a dangling tql_prev pointer will be left after
> reallocation. From QTAILQ_INIT():
> (head)->tqh_circ.tql_prev = &(head)->tqh_circ; <--- not realloc-safe
>
> Instead of using g_renew() on an array, consider using a list from
> "qemu/queue.h". Lookup by shmid becomes O(n) instead of O(1), but it
> doesn't matter in practice since existing devices have a small number of
> VIRTIO Shared Memory Regions.
>
> > + return &vdev->shmem_list[vdev->n_shmem_regions - 1];
> > +}
> > +
> > +void virtio_add_shmem_map(VirtSharedMemory *shmem, const char *shm_name,
> > + hwaddr shm_offset, hwaddr fd_offset, uint64_t size,
> > + int fd, Error **errp)
> > +{
> > + MappedMemoryRegion *mmap;
> > + fd = dup(fd);
> > + if (fd < 0) {
> > + error_setg_errno(errp, errno, "Failed to duplicate fd");
> > + return;
> > + }
> > +
> > + if (shm_offset + size > shmem->mr->size) {
> > + error_setg(errp, "Memory exceeds the shared memory boundaries");
> > + return;
>
> fd is leaked here.
>
> > + }
> > +
> > + mmap = g_new0(MappedMemoryRegion, 1);
> > + mmap->mem = g_new0(MemoryRegion, 1);
> > + mmap->offset = shm_offset;
> > + if (!memory_region_init_ram_from_fd(mmap->mem,
> > + OBJECT(shmem->mr),
> > + shm_name, size, RAM_SHARED,
> > + fd, fd_offset, errp)) {
> > + error_setg(errp, "Failed to mmap region %s", shm_name);
> > + close(fd);
> > + g_free(mmap->mem);
> > + g_free(mmap);
> > + return;
> > + }
> > + memory_region_add_subregion(shmem->mr, shm_offset, mmap->mem);
> > +
> > + QTAILQ_INSERT_TAIL(&shmem->mmaps, mmap, link);
> > +}
> > +
> > +MappedMemoryRegion *virtio_find_shmem_map(VirtSharedMemory *shmem,
> > + hwaddr offset, uint64_t size)
> > +{
> > + MappedMemoryRegion *mmap;
> > + QTAILQ_FOREACH(mmap, &shmem->mmaps, link) {
> > + if (mmap->offset == offset && mmap->mem->size == size) {
> > + return mmap;
> > + }
> > + }
> > + return NULL;
> > +}
> > +
> > +void virtio_del_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
> > + uint64_t size)
> > +{
> > + MappedMemoryRegion *mmap = virtio_find_shmem_map(shmem, offset, size);
> > + if (mmap == NULL) {
> > + return;
> > + }
> > +
> > + object_unparent(OBJECT(mmap->mem));
> > + QTAILQ_REMOVE(&shmem->mmaps, mmap, link);
> > + g_free(mmap);
> > +}
> > +
> > /* A wrapper for use as a VMState .put function */
> > static int virtio_device_put(QEMUFile *f, void *opaque, size_t size,
> > const VMStateField *field, JSONWriter *vmdesc)
> > @@ -3492,6 +3561,8 @@ void virtio_init(VirtIODevice *vdev, uint16_t device_id, size_t config_size)
> > virtio_vmstate_change, vdev);
> > vdev->device_endian = virtio_default_endian();
> > vdev->use_guest_notifier_mask = true;
> > + vdev->shmem_list = NULL;
> > + vdev->n_shmem_regions = 0;
> > }
> >
> > /*
> > @@ -4005,11 +4076,21 @@ static void virtio_device_free_virtqueues(VirtIODevice *vdev)
> > static void virtio_device_instance_finalize(Object *obj)
> > {
> > VirtIODevice *vdev = VIRTIO_DEVICE(obj);
> > + VirtSharedMemory *shmem = NULL;
> > + int i;
> >
> > virtio_device_free_virtqueues(vdev);
> >
> > g_free(vdev->config);
> > g_free(vdev->vector_queues);
> > + for (i = 0; i< vdev->n_shmem_regions; i++) {
> > + shmem = &vdev->shmem_list[i];
> > + while (!QTAILQ_EMPTY(&shmem->mmaps)) {
> > + MappedMemoryRegion *mmap_reg = QTAILQ_FIRST(&shmem->mmaps);
> > + QTAILQ_REMOVE(&shmem->mmaps, mmap_reg, link);
> > + g_free(mmap_reg);
>
> Is it possible to reuse virtio_del_shmem_map() to avoid code duplication
> and inconsistencies?
>
> while (!QTAILQ_EMPTY(&shmem->mmaps)) {
> MappedMemoryRegion *mmap_reg = QTAILQ_FIRST(&shmem->mmaps);
> virtio_del_shmem_map(shmem, mmap_reg->offset, mmap_reg->mem->size);
> }
>
> > + }
> > + }
>
> Missing g_free(vdev->shmem_list).
>
> > }
> >
> > static const Property virtio_properties[] = {
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index 6386910280..a778547c79 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -98,6 +98,21 @@ enum virtio_device_endian {
> > VIRTIO_DEVICE_ENDIAN_BIG,
> > };
> >
> > +struct MappedMemoryRegion {
> > + MemoryRegion *mem;
> > + hwaddr offset;
> > + QTAILQ_ENTRY(MappedMemoryRegion) link;
> > +};
> > +
> > +typedef struct MappedMemoryRegion MappedMemoryRegion;
> > +
> > +struct VirtSharedMemory {
> > + MemoryRegion *mr;
> > + QTAILQ_HEAD(, MappedMemoryRegion) mmaps;
> > +};
> > +
> > +typedef struct VirtSharedMemory VirtSharedMemory;
> > +
> > /**
> > * struct VirtIODevice - common VirtIO structure
> > * @name: name of the device
> > @@ -167,6 +182,9 @@ struct VirtIODevice
> > */
> > EventNotifier config_notifier;
> > bool device_iotlb_enabled;
> > + /* Shared memory region for vhost-user mappings. */
>
> This is core VIRTIO code and Shared Memory Regions are a VIRTIO concept.
> It is possible that built-in (non-vhost) devices might also add their
> shared memory regions with virtio_new_shmem_region(), so let's not talk
> about vhost-user specifically.
>
> > + VirtSharedMemory *shmem_list;
> > + int n_shmem_regions;
> > };
> >
> > struct VirtioDeviceClass {
> > @@ -289,6 +307,15 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
> >
> > int virtio_save(VirtIODevice *vdev, QEMUFile *f);
> >
> > +VirtSharedMemory *virtio_new_shmem_region(VirtIODevice *vdev);
> > +void virtio_add_shmem_map(VirtSharedMemory *shmem, const char *shm_name,
> > + hwaddr shm_offset, hwaddr fd_offset, uint64_t size,
> > + int fd, Error **errp);
> > +MappedMemoryRegion *virtio_find_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
> > + uint64_t size);
> > +void virtio_del_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
> > + uint64_t size);
> > +
> > extern const VMStateInfo virtio_vmstate_info;
> >
> > #define VMSTATE_VIRTIO_DEVICE \
> > diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> > index 9c630c2170..034cbfdc3c 100644
> > --- a/subprojects/libvhost-user/libvhost-user.c
> > +++ b/subprojects/libvhost-user/libvhost-user.c
> > @@ -1592,6 +1592,76 @@ vu_rm_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN])
> > return vu_send_message(dev, &msg);
> > }
> >
> > +bool
> > +vu_shmem_map(VuDev *dev, uint8_t shmid, uint64_t fd_offset,
> > + uint64_t shm_offset, uint64_t len, uint64_t flags, int fd)
> > +{
> > + VhostUserMsg vmsg = {
> > + .request = VHOST_USER_BACKEND_SHMEM_MAP,
> > + .size = sizeof(vmsg.payload.mmap),
> > + .flags = VHOST_USER_VERSION,
> > + .payload.mmap = {
> > + .shmid = shmid,
> > + .fd_offset = fd_offset,
> > + .shm_offset = shm_offset,
> > + .len = len,
> > + .flags = flags,
> > + },
> > + .fd_num = 1,
> > + .fds[0] = fd,
> > + };
> > +
> > + if (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SHMEM)) {
> > + return false;
> > + }
> > +
> > + if (vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_REPLY_ACK)) {
> > + vmsg.flags |= VHOST_USER_NEED_REPLY_MASK;
> > + }
>
> Setting this flag is inconsistent with
> vhost_user_backend_handle_shmem_map() and
> vhost_user_backend_handle_shmem_unmap(). They call
> vhost_user_send_resp() explicitly instead of relying on backend_read()
> to send the return value when VHOST_USER_NEED_REPLY_MASK is set. This
> inconsistency means that a successful return message will be sent twice
> when VHOST_USER_NEED_REPLY_MASK is set.
>
> Either these new vhost-user messages could be specified with a mandatory
> reply payload or the patch could be updated to avoid the explicit
> vhost_user_send_resp() calls. I think the latter is more commonly used
> by other vhost-user messages, so it's probably best to do it the same
> way.
>
> > +
> > + pthread_mutex_lock(&dev->backend_mutex);
> > + if (!vu_message_write(dev, dev->backend_fd, &vmsg)) {
> > + pthread_mutex_unlock(&dev->backend_mutex);
> > + return false;
> > + }
> > +
> > + /* Also unlocks the backend_mutex */
> > + return vu_process_message_reply(dev, &vmsg);
> > +}
> > +
> > +bool
> > +vu_shmem_unmap(VuDev *dev, uint8_t shmid, uint64_t shm_offset, uint64_t len)
> > +{
> > + VhostUserMsg vmsg = {
> > + .request = VHOST_USER_BACKEND_SHMEM_UNMAP,
> > + .size = sizeof(vmsg.payload.mmap),
> > + .flags = VHOST_USER_VERSION,
> > + .payload.mmap = {
> > + .shmid = shmid,
> > + .fd_offset = 0,
> > + .shm_offset = shm_offset,
> > + .len = len,
> > + },
> > + };
> > +
> > + if (!vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_SHMEM)) {
> > + return false;
> > + }
> > +
> > + if (vu_has_protocol_feature(dev, VHOST_USER_PROTOCOL_F_REPLY_ACK)) {
> > + vmsg.flags |= VHOST_USER_NEED_REPLY_MASK;
> > + }
> > +
> > + pthread_mutex_lock(&dev->backend_mutex);
> > + if (!vu_message_write(dev, dev->backend_fd, &vmsg)) {
> > + pthread_mutex_unlock(&dev->backend_mutex);
> > + return false;
> > + }
> > +
> > + /* Also unlocks the backend_mutex */
> > + return vu_process_message_reply(dev, &vmsg);
> > +}
> > +
> > 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 2ffc58c11b..e9adb836f0 100644
> > --- a/subprojects/libvhost-user/libvhost-user.h
> > +++ b/subprojects/libvhost-user/libvhost-user.h
> > @@ -69,6 +69,8 @@ enum VhostUserProtocolFeature {
> > /* Feature 16 is reserved for VHOST_USER_PROTOCOL_F_STATUS. */
> > /* Feature 17 reserved for VHOST_USER_PROTOCOL_F_XEN_MMAP. */
> > VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 18,
> > + /* Feature 19 is reserved for VHOST_USER_PROTOCOL_F_DEVICE_STATE */
> > + VHOST_USER_PROTOCOL_F_SHMEM = 20,
> > VHOST_USER_PROTOCOL_F_MAX
> > };
> >
> > @@ -127,6 +129,8 @@ typedef enum VhostUserBackendRequest {
> > VHOST_USER_BACKEND_SHARED_OBJECT_ADD = 6,
> > VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE = 7,
> > VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP = 8,
> > + VHOST_USER_BACKEND_SHMEM_MAP = 9,
> > + VHOST_USER_BACKEND_SHMEM_UNMAP = 10,
> > VHOST_USER_BACKEND_MAX
> > } VhostUserBackendRequest;
> >
> > @@ -186,6 +190,24 @@ typedef struct VhostUserShared {
> > unsigned char uuid[UUID_LEN];
> > } VhostUserShared;
> >
> > +/* For the flags field of VhostUserMMap */
> > +#define VHOST_USER_FLAG_MAP_R (1u << 0)
> > +#define VHOST_USER_FLAG_MAP_W (1u << 1)
> > +
> > +typedef struct {
> > + /* VIRTIO Shared Memory Region ID */
> > + uint8_t shmid;
> > + uint8_t padding[7];
> > + /* File offset */
> > + uint64_t fd_offset;
> > + /* Offset within the VIRTIO Shared Memory Region */
> > + uint64_t shm_offset;
> > + /* Size of the mapping */
> > + uint64_t len;
> > + /* Flags for the mmap operation, from VHOST_USER_FLAG_* */
> > + uint64_t flags;
> > +} VhostUserMMap;
> > +
> > #define VU_PACKED __attribute__((packed))
> >
> > typedef struct VhostUserMsg {
> > @@ -210,6 +232,7 @@ typedef struct VhostUserMsg {
> > VhostUserVringArea area;
> > VhostUserInflight inflight;
> > VhostUserShared object;
> > + VhostUserMMap mmap;
> > } payload;
> >
> > int fds[VHOST_MEMORY_BASELINE_NREGIONS];
> > @@ -593,6 +616,38 @@ bool vu_add_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN]);
> > */
> > bool vu_rm_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN]);
> >
> > +/**
> > + * vu_shmem_map:
> > + * @dev: a VuDev context
> > + * @shmid: VIRTIO Shared Memory Region ID
> > + * @fd_offset: File offset
> > + * @shm_offset: Offset within the VIRTIO Shared Memory Region
> > + * @len: Size of the mapping
> > + * @flags: Flags for the mmap operation
> > + * @fd: A file descriptor
> > + *
> > + * Advertises a new mapping to be made in a given VIRTIO Shared Memory Region.
> > + *
> > + * Returns: TRUE on success, FALSE on failure.
> > + */
> > +bool vu_shmem_map(VuDev *dev, uint8_t shmid, uint64_t fd_offset,
> > + uint64_t shm_offset, uint64_t len, uint64_t flags, int fd);
> > +
> > +/**
> > + * vu_shmem_unmap:
> > + * @dev: a VuDev context
> > + * @shmid: VIRTIO Shared Memory Region ID
> > + * @fd_offset: File offset
> > + * @len: Size of the mapping
> > + *
> > + * The front-end un-mmaps a given range in the VIRTIO Shared Memory Region
> > + * with the requested `shmid`.
> > + *
> > + * Returns: TRUE on success, FALSE on failure.
> > + */
> > +bool vu_shmem_unmap(VuDev *dev, uint8_t shmid, uint64_t shm_offset,
> > + uint64_t len);
> > +
> > /**
> > * vu_queue_set_notification:
> > * @dev: a VuDev context
> > --
> > 2.48.1
> >
^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2025-03-06 14:49 UTC | newest]
Thread overview: 36+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-17 16:40 [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests Albert Esteve
2025-02-17 16:40 ` [PATCH v4 1/9] vhost-user: Add VirtIO Shared Memory map request Albert Esteve
2025-02-18 6:43 ` Stefan Hajnoczi
2025-02-18 10:33 ` Albert Esteve
2025-03-06 14:48 ` Albert Esteve
2025-02-18 10:19 ` Stefan Hajnoczi
2025-02-20 10:59 ` Alyssa Ross
2025-02-17 16:40 ` [PATCH v4 2/9] vhost_user.rst: Align VhostUserMsg excerpt members Albert Esteve
2025-02-18 6:44 ` Stefan Hajnoczi
2025-02-17 16:40 ` [PATCH v4 3/9] vhost_user.rst: Add SHMEM_MAP/_UNMAP to spec Albert Esteve
2025-02-17 16:40 ` [PATCH v4 4/9] vhost_user: Add frontend get_shmem_config command Albert Esteve
2025-02-18 10:27 ` Stefan Hajnoczi
2025-02-17 16:40 ` [PATCH v4 5/9] vhost_user.rst: Add GET_SHMEM_CONFIG message Albert Esteve
2025-02-18 10:33 ` Stefan Hajnoczi
2025-02-17 16:40 ` [PATCH v4 6/9] qmp: add shmem feature map Albert Esteve
2025-02-18 10:34 ` Stefan Hajnoczi
2025-02-17 16:40 ` [PATCH v4 7/9] vhost-user-devive: Add shmem BAR Albert Esteve
2025-02-18 10:41 ` Stefan Hajnoczi
2025-02-18 10:55 ` Albert Esteve
2025-02-18 13:25 ` Stefan Hajnoczi
2025-02-18 15:04 ` Albert Esteve
2025-02-17 16:40 ` [PATCH v4 8/9] vhost_user: Add mem_read/write backend requests Albert Esteve
2025-02-18 10:57 ` Stefan Hajnoczi
2025-02-17 16:40 ` [PATCH v4 9/9] vhost_user.rst: Add MEM_READ/WRITE messages Albert Esteve
2025-02-18 11:00 ` Stefan Hajnoczi
2025-02-18 12:50 ` Albert Esteve
2025-02-17 20:01 ` [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests David Hildenbrand
2025-02-24 8:54 ` Albert Esteve
2025-02-24 9:16 ` David Hildenbrand
2025-02-24 9:35 ` Albert Esteve
2025-02-24 9:49 ` David Hildenbrand
2025-02-24 13:41 ` Albert Esteve
2025-02-24 13:57 ` David Hildenbrand
2025-02-24 15:15 ` Albert Esteve
2025-02-26 9:53 ` David Hildenbrand
2025-02-27 7:10 ` Stefan Hajnoczi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).