* [PATCH v3 1/5] vhost-user: Add VIRTIO Shared Memory map request
2024-09-12 14:53 [PATCH v3 0/5] vhost-user: Add SHMEM_MAP/UNMAP requests Albert Esteve
@ 2024-09-12 14:53 ` Albert Esteve
2024-09-16 17:21 ` Stefan Hajnoczi
2024-09-17 10:07 ` David Hildenbrand
2024-09-12 14:53 ` [PATCH v3 2/5] virtio: Track shared memory mappings Albert Esteve
` (5 subsequent siblings)
6 siblings, 2 replies; 29+ messages in thread
From: Albert Esteve @ 2024-09-12 14:53 UTC (permalink / raw)
To: qemu-devel
Cc: Alex Bennée, slp, hi, mst, david, jasowang, stefanha,
Stefano Garzarella, stevensd, 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`. 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 its valid.
The backend can munmap 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
mmap the regions with PROT_NONE to reserve
the virtual memory space.
The device model needs to create MemoryRegion
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 | 122 ++++++++++++++++++++++
hw/virtio/virtio.c | 13 +++
include/hw/virtio/virtio.h | 5 +
subprojects/libvhost-user/libvhost-user.c | 60 +++++++++++
subprojects/libvhost-user/libvhost-user.h | 52 +++++++++
5 files changed, 252 insertions(+)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 00561daa06..338cc942ec 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 {
@@ -1749,6 +1770,100 @@ 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,
+ VhostUserMMap *vu_mmap,
+ int fd)
+{
+ void *addr = 0;
+ MemoryRegion *mr = 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;
+ }
+
+ mr = &dev->vdev->shmem_list[vu_mmap->shmid];
+
+ if (!mr) {
+ 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) > mr->size) {
+ error_report("Bad offset/len for mmap %" PRIx64 "+%" PRIx64,
+ vu_mmap->shm_offset, vu_mmap->len);
+ return -EFAULT;
+ }
+
+ void *shmem_ptr = memory_region_get_ram_ptr(mr);
+
+ addr = mmap(shmem_ptr + vu_mmap->shm_offset, vu_mmap->len,
+ ((vu_mmap->flags & VHOST_USER_FLAG_MAP_R) ? PROT_READ : 0) |
+ ((vu_mmap->flags & VHOST_USER_FLAG_MAP_W) ? PROT_WRITE : 0),
+ MAP_SHARED | MAP_FIXED, fd, vu_mmap->fd_offset);
+
+ if (addr == MAP_FAILED) {
+ error_report("Failed to mmap mem fd");
+ return -EFAULT;
+ }
+
+ return 0;
+}
+
+static int
+vhost_user_backend_handle_shmem_unmap(struct vhost_dev *dev,
+ VhostUserMMap *vu_mmap)
+{
+ void *addr = 0;
+ MemoryRegion *mr = NULL;
+
+ 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;
+ }
+
+ mr = &dev->vdev->shmem_list[vu_mmap->shmid];
+
+ if (!mr) {
+ 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) > mr->size) {
+ error_report("Bad offset/len for mmap %" PRIx64 "+%" PRIx64,
+ vu_mmap->shm_offset, vu_mmap->len);
+ return -EFAULT;
+ }
+
+ void *shmem_ptr = memory_region_get_ram_ptr(mr);
+
+ addr = mmap(shmem_ptr + vu_mmap->shm_offset, vu_mmap->len,
+ PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
+
+ if (addr == MAP_FAILED) {
+ error_report("Failed to unmap memory");
+ return -EFAULT;
+ }
+
+ return 0;
+}
+
static void close_backend_channel(struct vhost_user *u)
{
g_source_destroy(u->backend_src);
@@ -1817,6 +1932,13 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition,
ret = vhost_user_backend_handle_shared_object_lookup(dev->opaque, ioc,
&hdr, &payload);
break;
+ case VHOST_USER_BACKEND_SHMEM_MAP:
+ ret = vhost_user_backend_handle_shmem_map(dev, &payload.mmap,
+ fd ? fd[0] : -1);
+ break;
+ case VHOST_USER_BACKEND_SHMEM_UNMAP:
+ ret = vhost_user_backend_handle_shmem_unmap(dev, &payload.mmap);
+ break;
default:
error_report("Received unexpected msg type: %d.", hdr.request);
ret = -EINVAL;
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 9e10cbc058..ccc4f2cd75 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3059,6 +3059,17 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f)
return vmstate_save_state(f, &vmstate_virtio, vdev, NULL);
}
+MemoryRegion *virtio_new_shmem_region(VirtIODevice *vdev)
+{
+ MemoryRegion *mr;
+ ++vdev->n_shmem_regions;
+ vdev->shmem_list = g_renew(MemoryRegion, vdev->shmem_list,
+ vdev->n_shmem_regions);
+ mr = &vdev->shmem_list[vdev->n_shmem_regions - 1];
+ mr = g_new0(MemoryRegion, 1);
+ return mr;
+}
+
/* 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)
@@ -3481,6 +3492,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;
}
/*
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 0fcbc5c0c6..d4a2f664d9 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -167,6 +167,9 @@ struct VirtIODevice
*/
EventNotifier config_notifier;
bool device_iotlb_enabled;
+ /* Shared memory region for vhost-user mappings. */
+ MemoryRegion *shmem_list;
+ int n_shmem_regions;
};
struct VirtioDeviceClass {
@@ -286,6 +289,8 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
int virtio_save(VirtIODevice *vdev, QEMUFile *f);
+MemoryRegion *virtio_new_shmem_region(VirtIODevice *vdev);
+
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..496268e12b 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -1592,6 +1592,66 @@ 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)
+{
+ 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,
+ },
+ };
+
+ 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_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 deb40e77b3..ea4902e876 100644
--- a/subprojects/libvhost-user/libvhost-user.h
+++ b/subprojects/libvhost-user/libvhost-user.h
@@ -127,6 +127,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 +188,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;
+
#if defined(_WIN32) && (defined(__x86_64__) || defined(__i386__))
# define VU_PACKED __attribute__((gcc_struct, packed))
#else
@@ -214,6 +234,7 @@ typedef struct VhostUserMsg {
VhostUserVringArea area;
VhostUserInflight inflight;
VhostUserShared object;
+ VhostUserMMap mmap;
} payload;
int fds[VHOST_MEMORY_BASELINE_NREGIONS];
@@ -597,6 +618,37 @@ 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
+ *
+ * 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);
+
+/**
+ * vu_shmem_map:
+ * @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.45.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 1/5] vhost-user: Add VIRTIO Shared Memory map request
2024-09-12 14:53 ` [PATCH v3 1/5] vhost-user: Add VIRTIO Shared Memory map request Albert Esteve
@ 2024-09-16 17:21 ` Stefan Hajnoczi
2024-11-25 9:55 ` Albert Esteve
2024-09-17 10:07 ` David Hildenbrand
1 sibling, 1 reply; 29+ messages in thread
From: Stefan Hajnoczi @ 2024-09-16 17:21 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, Alex Bennée, slp, hi, mst, david, jasowang,
Stefano Garzarella, stevensd
[-- Attachment #1: Type: text/plain, Size: 15369 bytes --]
On Thu, Sep 12, 2024 at 04:53:31PM +0200, 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`. 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 its valid.
>
> The backend can munmap 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
> mmap the regions with PROT_NONE to reserve
> the virtual memory space.
>
> The device model needs to create MemoryRegion
> 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 | 122 ++++++++++++++++++++++
> hw/virtio/virtio.c | 13 +++
> include/hw/virtio/virtio.h | 5 +
> subprojects/libvhost-user/libvhost-user.c | 60 +++++++++++
> subprojects/libvhost-user/libvhost-user.h | 52 +++++++++
> 5 files changed, 252 insertions(+)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 00561daa06..338cc942ec 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 {
> @@ -1749,6 +1770,100 @@ 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,
> + VhostUserMMap *vu_mmap,
> + int fd)
> +{
> + void *addr = 0;
> + MemoryRegion *mr = 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;
> + }
> +
> + mr = &dev->vdev->shmem_list[vu_mmap->shmid];
> +
> + if (!mr) {
> + 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) > mr->size) {
> + error_report("Bad offset/len for mmap %" PRIx64 "+%" PRIx64,
> + vu_mmap->shm_offset, vu_mmap->len);
> + return -EFAULT;
> + }
> +
> + void *shmem_ptr = memory_region_get_ram_ptr(mr);
> +
> + addr = mmap(shmem_ptr + vu_mmap->shm_offset, vu_mmap->len,
> + ((vu_mmap->flags & VHOST_USER_FLAG_MAP_R) ? PROT_READ : 0) |
> + ((vu_mmap->flags & VHOST_USER_FLAG_MAP_W) ? PROT_WRITE : 0),
> + MAP_SHARED | MAP_FIXED, fd, vu_mmap->fd_offset);
> +
> + if (addr == MAP_FAILED) {
> + error_report("Failed to mmap mem fd");
> + return -EFAULT;
> + }
> +
> + return 0;
> +}
> +
> +static int
> +vhost_user_backend_handle_shmem_unmap(struct vhost_dev *dev,
> + VhostUserMMap *vu_mmap)
> +{
> + void *addr = 0;
> + MemoryRegion *mr = NULL;
> +
> + 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;
> + }
> +
> + mr = &dev->vdev->shmem_list[vu_mmap->shmid];
> +
> + if (!mr) {
> + 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) > mr->size) {
> + error_report("Bad offset/len for mmap %" PRIx64 "+%" PRIx64,
> + vu_mmap->shm_offset, vu_mmap->len);
> + return -EFAULT;
> + }
> +
> + void *shmem_ptr = memory_region_get_ram_ptr(mr);
> +
> + addr = mmap(shmem_ptr + vu_mmap->shm_offset, vu_mmap->len,
> + PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
> +
> + if (addr == MAP_FAILED) {
> + error_report("Failed to unmap memory");
> + return -EFAULT;
> + }
> +
> + return 0;
> +}
> +
> static void close_backend_channel(struct vhost_user *u)
> {
> g_source_destroy(u->backend_src);
> @@ -1817,6 +1932,13 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition,
> ret = vhost_user_backend_handle_shared_object_lookup(dev->opaque, ioc,
> &hdr, &payload);
> break;
> + case VHOST_USER_BACKEND_SHMEM_MAP:
> + ret = vhost_user_backend_handle_shmem_map(dev, &payload.mmap,
> + fd ? fd[0] : -1);
> + break;
> + case VHOST_USER_BACKEND_SHMEM_UNMAP:
> + ret = vhost_user_backend_handle_shmem_unmap(dev, &payload.mmap);
> + break;
> default:
> error_report("Received unexpected msg type: %d.", hdr.request);
> ret = -EINVAL;
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 9e10cbc058..ccc4f2cd75 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3059,6 +3059,17 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f)
> return vmstate_save_state(f, &vmstate_virtio, vdev, NULL);
> }
>
> +MemoryRegion *virtio_new_shmem_region(VirtIODevice *vdev)
> +{
> + MemoryRegion *mr;
> + ++vdev->n_shmem_regions;
> + vdev->shmem_list = g_renew(MemoryRegion, vdev->shmem_list,
> + vdev->n_shmem_regions);
> + mr = &vdev->shmem_list[vdev->n_shmem_regions - 1];
> + mr = g_new0(MemoryRegion, 1);
> + return mr;
> +}
This function looks broken. shmem_list[] is reallocated so old
MemoryRegion pointers will be dangling pointers. And then the
MemoryRegion is allocated again using g_new0() but there is no way to
retrieve that address again via shmem_list[].
I expected something like this:
MemoryRegion *virtio_new_shmem_region(VirtIODevice *vdev)
{
MemoryRegion *mr;
assert(vdev->n_shmem_regions < INT_MAX);
++vdev->n_shmem_regions;
vdev->shmem_list = g_renew(MemoryRegion *, vdev->shmem_list,
vdev->n_shmem_regions);
mr = g_new0(MemoryRegion, 1);
vdev->shmem_list[vdev->n_shmem_regions - 1] = mr;
return mr;
}
> +
> /* 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)
> @@ -3481,6 +3492,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;
shmem_list[] and each MemoryRegion needs to be free somewhere.
virtio_device_instance_finalize()?
> + vdev->n_shmem_regions = 0;
> }
>
> /*
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 0fcbc5c0c6..d4a2f664d9 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -167,6 +167,9 @@ struct VirtIODevice
> */
> EventNotifier config_notifier;
> bool device_iotlb_enabled;
> + /* Shared memory region for vhost-user mappings. */
> + MemoryRegion *shmem_list;
> + int n_shmem_regions;
> };
>
> struct VirtioDeviceClass {
> @@ -286,6 +289,8 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
>
> int virtio_save(VirtIODevice *vdev, QEMUFile *f);
>
> +MemoryRegion *virtio_new_shmem_region(VirtIODevice *vdev);
> +
> 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..496268e12b 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -1592,6 +1592,66 @@ 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)
> +{
> + 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,
> + },
> + };
> +
> + 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_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 deb40e77b3..ea4902e876 100644
> --- a/subprojects/libvhost-user/libvhost-user.h
> +++ b/subprojects/libvhost-user/libvhost-user.h
> @@ -127,6 +127,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 +188,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;
> +
> #if defined(_WIN32) && (defined(__x86_64__) || defined(__i386__))
> # define VU_PACKED __attribute__((gcc_struct, packed))
> #else
> @@ -214,6 +234,7 @@ typedef struct VhostUserMsg {
> VhostUserVringArea area;
> VhostUserInflight inflight;
> VhostUserShared object;
> + VhostUserMMap mmap;
> } payload;
>
> int fds[VHOST_MEMORY_BASELINE_NREGIONS];
> @@ -597,6 +618,37 @@ 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
> + *
> + * 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);
How is the fd passed to the front-end?
> +
> +/**
> + * vu_shmem_map:
"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.45.2
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 1/5] vhost-user: Add VIRTIO Shared Memory map request
2024-09-16 17:21 ` Stefan Hajnoczi
@ 2024-11-25 9:55 ` Albert Esteve
0 siblings, 0 replies; 29+ messages in thread
From: Albert Esteve @ 2024-11-25 9:55 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, Alex Bennée, slp, hi, mst, david, jasowang,
Stefano Garzarella, stevensd
On Mon, Sep 16, 2024 at 7:21 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Thu, Sep 12, 2024 at 04:53:31PM +0200, 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`. 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 its valid.
> >
> > The backend can munmap 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
> > mmap the regions with PROT_NONE to reserve
> > the virtual memory space.
> >
> > The device model needs to create MemoryRegion
> > 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 | 122 ++++++++++++++++++++++
> > hw/virtio/virtio.c | 13 +++
> > include/hw/virtio/virtio.h | 5 +
> > subprojects/libvhost-user/libvhost-user.c | 60 +++++++++++
> > subprojects/libvhost-user/libvhost-user.h | 52 +++++++++
> > 5 files changed, 252 insertions(+)
> >
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index 00561daa06..338cc942ec 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 {
> > @@ -1749,6 +1770,100 @@ 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,
> > + VhostUserMMap *vu_mmap,
> > + int fd)
> > +{
> > + void *addr = 0;
> > + MemoryRegion *mr = 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;
> > + }
> > +
> > + mr = &dev->vdev->shmem_list[vu_mmap->shmid];
> > +
> > + if (!mr) {
> > + 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) > mr->size) {
> > + error_report("Bad offset/len for mmap %" PRIx64 "+%" PRIx64,
> > + vu_mmap->shm_offset, vu_mmap->len);
> > + return -EFAULT;
> > + }
> > +
> > + void *shmem_ptr = memory_region_get_ram_ptr(mr);
> > +
> > + addr = mmap(shmem_ptr + vu_mmap->shm_offset, vu_mmap->len,
> > + ((vu_mmap->flags & VHOST_USER_FLAG_MAP_R) ? PROT_READ : 0) |
> > + ((vu_mmap->flags & VHOST_USER_FLAG_MAP_W) ? PROT_WRITE : 0),
> > + MAP_SHARED | MAP_FIXED, fd, vu_mmap->fd_offset);
> > +
> > + if (addr == MAP_FAILED) {
> > + error_report("Failed to mmap mem fd");
> > + return -EFAULT;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +vhost_user_backend_handle_shmem_unmap(struct vhost_dev *dev,
> > + VhostUserMMap *vu_mmap)
> > +{
> > + void *addr = 0;
> > + MemoryRegion *mr = NULL;
> > +
> > + 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;
> > + }
> > +
> > + mr = &dev->vdev->shmem_list[vu_mmap->shmid];
> > +
> > + if (!mr) {
> > + 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) > mr->size) {
> > + error_report("Bad offset/len for mmap %" PRIx64 "+%" PRIx64,
> > + vu_mmap->shm_offset, vu_mmap->len);
> > + return -EFAULT;
> > + }
> > +
> > + void *shmem_ptr = memory_region_get_ram_ptr(mr);
> > +
> > + addr = mmap(shmem_ptr + vu_mmap->shm_offset, vu_mmap->len,
> > + PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
> > +
> > + if (addr == MAP_FAILED) {
> > + error_report("Failed to unmap memory");
> > + return -EFAULT;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static void close_backend_channel(struct vhost_user *u)
> > {
> > g_source_destroy(u->backend_src);
> > @@ -1817,6 +1932,13 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition,
> > ret = vhost_user_backend_handle_shared_object_lookup(dev->opaque, ioc,
> > &hdr, &payload);
> > break;
> > + case VHOST_USER_BACKEND_SHMEM_MAP:
> > + ret = vhost_user_backend_handle_shmem_map(dev, &payload.mmap,
> > + fd ? fd[0] : -1);
> > + break;
> > + case VHOST_USER_BACKEND_SHMEM_UNMAP:
> > + ret = vhost_user_backend_handle_shmem_unmap(dev, &payload.mmap);
> > + break;
> > default:
> > error_report("Received unexpected msg type: %d.", hdr.request);
> > ret = -EINVAL;
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 9e10cbc058..ccc4f2cd75 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -3059,6 +3059,17 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f)
> > return vmstate_save_state(f, &vmstate_virtio, vdev, NULL);
> > }
> >
> > +MemoryRegion *virtio_new_shmem_region(VirtIODevice *vdev)
> > +{
> > + MemoryRegion *mr;
> > + ++vdev->n_shmem_regions;
> > + vdev->shmem_list = g_renew(MemoryRegion, vdev->shmem_list,
> > + vdev->n_shmem_regions);
> > + mr = &vdev->shmem_list[vdev->n_shmem_regions - 1];
> > + mr = g_new0(MemoryRegion, 1);
> > + return mr;
> > +}
>
> This function looks broken. shmem_list[] is reallocated so old
> MemoryRegion pointers will be dangling pointers. And then the
> MemoryRegion is allocated again using g_new0() but there is no way to
> retrieve that address again via shmem_list[].
>
> I expected something like this:
>
> MemoryRegion *virtio_new_shmem_region(VirtIODevice *vdev)
> {
> MemoryRegion *mr;
>
> assert(vdev->n_shmem_regions < INT_MAX);
> ++vdev->n_shmem_regions;
> vdev->shmem_list = g_renew(MemoryRegion *, vdev->shmem_list,
> vdev->n_shmem_regions);
> mr = g_new0(MemoryRegion, 1);
> vdev->shmem_list[vdev->n_shmem_regions - 1] = mr;
> return mr;
> }
>
> > +
> > /* 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)
> > @@ -3481,6 +3492,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;
>
> shmem_list[] and each MemoryRegion needs to be free somewhere.
> virtio_device_instance_finalize()?
>
> > + vdev->n_shmem_regions = 0;
> > }
> >
> > /*
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index 0fcbc5c0c6..d4a2f664d9 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -167,6 +167,9 @@ struct VirtIODevice
> > */
> > EventNotifier config_notifier;
> > bool device_iotlb_enabled;
> > + /* Shared memory region for vhost-user mappings. */
> > + MemoryRegion *shmem_list;
> > + int n_shmem_regions;
> > };
> >
> > struct VirtioDeviceClass {
> > @@ -286,6 +289,8 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
> >
> > int virtio_save(VirtIODevice *vdev, QEMUFile *f);
> >
> > +MemoryRegion *virtio_new_shmem_region(VirtIODevice *vdev);
> > +
> > 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..496268e12b 100644
> > --- a/subprojects/libvhost-user/libvhost-user.c
> > +++ b/subprojects/libvhost-user/libvhost-user.c
> > @@ -1592,6 +1592,66 @@ 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)
> > +{
> > + 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,
> > + },
> > + };
> > +
> > + 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_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 deb40e77b3..ea4902e876 100644
> > --- a/subprojects/libvhost-user/libvhost-user.h
> > +++ b/subprojects/libvhost-user/libvhost-user.h
> > @@ -127,6 +127,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 +188,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;
> > +
> > #if defined(_WIN32) && (defined(__x86_64__) || defined(__i386__))
> > # define VU_PACKED __attribute__((gcc_struct, packed))
> > #else
> > @@ -214,6 +234,7 @@ typedef struct VhostUserMsg {
> > VhostUserVringArea area;
> > VhostUserInflight inflight;
> > VhostUserShared object;
> > + VhostUserMMap mmap;
> > } payload;
> >
> > int fds[VHOST_MEMORY_BASELINE_NREGIONS];
> > @@ -597,6 +618,37 @@ 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
> > + *
> > + * 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);
>
> How is the fd passed to the front-end?
True, I have tested the backend part directly with rust-vmm
(https://github.com/rust-vmm/vhost/pull/251), so I have not run this
code myself.
I'll add an fd argument and try to cover it with a test at least, as
we discussed in other threads.
>
> > +
> > +/**
> > + * vu_shmem_map:
>
> "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.45.2
> >
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 1/5] vhost-user: Add VIRTIO Shared Memory map request
2024-09-12 14:53 ` [PATCH v3 1/5] vhost-user: Add VIRTIO Shared Memory map request Albert Esteve
2024-09-16 17:21 ` Stefan Hajnoczi
@ 2024-09-17 10:07 ` David Hildenbrand
2024-11-25 8:28 ` Albert Esteve
1 sibling, 1 reply; 29+ messages in thread
From: David Hildenbrand @ 2024-09-17 10:07 UTC (permalink / raw)
To: Albert Esteve, qemu-devel
Cc: Alex Bennée, slp, hi, mst, jasowang, stefanha,
Stefano Garzarella, stevensd
On 12.09.24 16:53, 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`. 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 its valid.
>
> The backend can munmap 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
> mmap the regions with PROT_NONE to reserve
> the virtual memory space.
>
> The device model needs to create MemoryRegion
> 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 | 122 ++++++++++++++++++++++
> hw/virtio/virtio.c | 13 +++
> include/hw/virtio/virtio.h | 5 +
> subprojects/libvhost-user/libvhost-user.c | 60 +++++++++++
> subprojects/libvhost-user/libvhost-user.h | 52 +++++++++
> 5 files changed, 252 insertions(+)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 00561daa06..338cc942ec 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 {
> @@ -1749,6 +1770,100 @@ 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,
> + VhostUserMMap *vu_mmap,
> + int fd)
> +{
> + void *addr = 0;
> + MemoryRegion *mr = 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;
> + }
> +
> + mr = &dev->vdev->shmem_list[vu_mmap->shmid];
> +
> + if (!mr) {
> + 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) > mr->size) {
> + error_report("Bad offset/len for mmap %" PRIx64 "+%" PRIx64,
> + vu_mmap->shm_offset, vu_mmap->len);
> + return -EFAULT;
> + }
> +
> + void *shmem_ptr = memory_region_get_ram_ptr(mr);
> +
> + addr = mmap(shmem_ptr + vu_mmap->shm_offset, vu_mmap->len,
> + ((vu_mmap->flags & VHOST_USER_FLAG_MAP_R) ? PROT_READ : 0) |
> + ((vu_mmap->flags & VHOST_USER_FLAG_MAP_W) ? PROT_WRITE : 0),
> + MAP_SHARED | MAP_FIXED, fd, vu_mmap->fd_offset);
> +
I'm sorry, but that looks completely wrong. You cannot just take some
RAM memory region/ RAMBlock that has properly set flags/fd/whatssoever
and map whatever you want in there.
Likely you would need a distinct RAMBlock/RAM memory region per mmap(),
and would end up mmaping implicitly via qemu_ram_mmap().
Then, your shared region would simply be an empty container into which
you map these RAM memory regions.
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 1/5] vhost-user: Add VIRTIO Shared Memory map request
2024-09-17 10:07 ` David Hildenbrand
@ 2024-11-25 8:28 ` Albert Esteve
2024-11-27 10:50 ` David Hildenbrand
0 siblings, 1 reply; 29+ messages in thread
From: Albert Esteve @ 2024-11-25 8:28 UTC (permalink / raw)
To: David Hildenbrand
Cc: qemu-devel, Alex Bennée, slp, hi, mst, jasowang, stefanha,
Stefano Garzarella, stevensd
On Tue, Sep 17, 2024 at 12:08 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 12.09.24 16:53, 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`. 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 its valid.
> >
> > The backend can munmap 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
> > mmap the regions with PROT_NONE to reserve
> > the virtual memory space.
> >
> > The device model needs to create MemoryRegion
> > 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 | 122 ++++++++++++++++++++++
> > hw/virtio/virtio.c | 13 +++
> > include/hw/virtio/virtio.h | 5 +
> > subprojects/libvhost-user/libvhost-user.c | 60 +++++++++++
> > subprojects/libvhost-user/libvhost-user.h | 52 +++++++++
> > 5 files changed, 252 insertions(+)
> >
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index 00561daa06..338cc942ec 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 {
> > @@ -1749,6 +1770,100 @@ 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,
> > + VhostUserMMap *vu_mmap,
> > + int fd)
> > +{
> > + void *addr = 0;
> > + MemoryRegion *mr = 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;
> > + }
> > +
> > + mr = &dev->vdev->shmem_list[vu_mmap->shmid];
> > +
> > + if (!mr) {
> > + 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) > mr->size) {
> > + error_report("Bad offset/len for mmap %" PRIx64 "+%" PRIx64,
> > + vu_mmap->shm_offset, vu_mmap->len);
> > + return -EFAULT;
> > + }
> > +
> > + void *shmem_ptr = memory_region_get_ram_ptr(mr);
> > +
> > + addr = mmap(shmem_ptr + vu_mmap->shm_offset, vu_mmap->len,
> > + ((vu_mmap->flags & VHOST_USER_FLAG_MAP_R) ? PROT_READ : 0) |
> > + ((vu_mmap->flags & VHOST_USER_FLAG_MAP_W) ? PROT_WRITE : 0),
> > + MAP_SHARED | MAP_FIXED, fd, vu_mmap->fd_offset);
> > +
>
> I'm sorry, but that looks completely wrong. You cannot just take some
> RAM memory region/ RAMBlock that has properly set flags/fd/whatssoever
> and map whatever you want in there.
>
> Likely you would need a distinct RAMBlock/RAM memory region per mmap(),
> and would end up mmaping implicitly via qemu_ram_mmap().
>
> Then, your shared region would simply be an empty container into which
> you map these RAM memory regions.
Hi, sorry it took me so long to get back to this. Lately I have been
testing the patch and fixing bugs, and I am was going to add some
tests to be able to verify the patch without having to use a backend
(which is what I am doing right now).
But I wanted to address/discuss this comment. I am not sure of the
actual problem with the current approach (I am not completely aware of
the concern in your first paragraph), but I see other instances where
qemu mmaps stuff into a MemoryRegion. Take into account that the
implementation follows the definition of shared memory region here:
https://docs.oasis-open.org/virtio/virtio/v1.3/csd01/virtio-v1.3-csd01.html#x1-10200010
Which hints to a memory region per ID, not one per required map. So
the current strategy seems to fit it better.
Also, I was aware that I was not the first one attempting this, so I
based this code in previous attempts (maybe I should give credit in
the commit now that I think of):
https://gitlab.com/virtio-fs/qemu/-/blob/qemu5.0-virtiofs-dax/hw/virtio/vhost-user-fs.c?ref_type=heads#L75
As you can see, it pretty much follows the same strategy. And in my
examples I have been able to use this to video stream with multiple
queues mapped into the shared memory (used to capture video frames),
using the backend I mentioned above for testing. So the concept works.
I may be wrong with this, but for what I understood looking at the
code, crosvm uses a similar strategy. Reserve a memory block and use
for all your mappings, and use an allocator to find a free slot.
And if I were to do what you say, those distinct RAMBlocks should be
created when the device starts? What would be their size? Should I
create them when qemu receives a request to mmap? How would the driver
find the RAMBlock?
BR,
Albert.
>
> --
> Cheers,
>
> David / dhildenb
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 1/5] vhost-user: Add VIRTIO Shared Memory map request
2024-11-25 8:28 ` Albert Esteve
@ 2024-11-27 10:50 ` David Hildenbrand
2024-11-27 12:10 ` David Hildenbrand
2024-11-27 12:55 ` Albert Esteve
0 siblings, 2 replies; 29+ messages in thread
From: David Hildenbrand @ 2024-11-27 10:50 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, Alex Bennée, slp, hi, mst, jasowang, stefanha,
Stefano Garzarella, stevensd
>> RAM memory region/ RAMBlock that has properly set flags/fd/whatssoever
>> and map whatever you want in there.
>>
>> Likely you would need a distinct RAMBlock/RAM memory region per mmap(),
>> and would end up mmaping implicitly via qemu_ram_mmap().
>>
>> Then, your shared region would simply be an empty container into which
>> you map these RAM memory regions.
>
Hi,
> Hi, sorry it took me so long to get back to this. Lately I have been
> testing the patch and fixing bugs, and I am was going to add some
> tests to be able to verify the patch without having to use a backend
> (which is what I am doing right now).
>
> But I wanted to address/discuss this comment. I am not sure of the
> actual problem with the current approach (I am not completely aware of
> the concern in your first paragraph), but I see other instances where
> qemu mmaps stuff into a MemoryRegion.
I suggest you take a look at the three relevant MAP_FIXED users outside
of user emulation code.
(1) hw/vfio/helpers.c: We create a custom memory region + RAMBlock with
memory_region_init_ram_device_ptr(). This doesn't mmap(MAP_FIXED)
into any existing RAMBlock.
(2) system/physmem.c: I suggest you take a close look at
qemu_ram_remap() and how it is one example of how RAMBlock
properties describe exactly what is mmaped.
(3) util/mmap-alloc.c: Well, this is the code that performs the mmap(),
to bring a RAMBlock to life. See qemu_ram_mmap().
There is some oddity hw/xen/xen-mapcache.c; XEN mapcache seems to manage
guest RAM without RAMBlocks.
> Take into account that the
> implementation follows the definition of shared memory region here:
> https://docs.oasis-open.org/virtio/virtio/v1.3/csd01/virtio-v1.3-csd01.html#x1-10200010
> Which hints to a memory region per ID, not one per required map. So
> the current strategy seems to fit it better.
I'm confused, we are talking about an implementation detail here? How is
that related to the spec?
>
> Also, I was aware that I was not the first one attempting this, so I
> based this code in previous attempts (maybe I should give credit in
> the commit now that I think of):
> https://gitlab.com/virtio-fs/qemu/-/blob/qemu5.0-virtiofs-dax/hw/virtio/vhost-user-fs.c?ref_type=heads#L75
> As you can see, it pretty much follows the same strategy.
So, people did some hacky things in a QEMU fork 6 years ago ... :) That
cannot possibly be a good argument why we should have it like that in QEMU.
> And in my
> examples I have been able to use this to video stream with multiple
> queues mapped into the shared memory (used to capture video frames),
> using the backend I mentioned above for testing. So the concept works.
> I may be wrong with this, but for what I understood looking at the
> code, crosvm uses a similar strategy. Reserve a memory block and use
> for all your mappings, and use an allocator to find a free slot.
>
Again, I suggest you take a look at what a RAMBlock is, and how it's
properties describe the containing mmap().
> And if I were to do what you say, those distinct RAMBlocks should be
> created when the device starts? What would be their size? Should I
> create them when qemu receives a request to mmap? How would the driver
> find the RAMBlock?
You'd have an empty memory region container into which you will map
memory regions that describe the memory you want to share.
mr = g_new0(MemoryRegion, 1);
memory_region_init(mr, OBJECT(TODO), "vhost-user-shm", region_size);
Assuming you are requested to mmap an fd, you'd create a new
MemoryRegion+RAMBlock that describes the memory and performs the mmap()
for you:
map_mr = g_new0(MemoryRegion, 1);
memory_region_init_ram_from_fd(map_mr, OBJECT(TODO), "TODO", map_size,
RAM_SHARED, map_fd, map_offs, errp);
To then map it into your container:
memory_region_add_subregion(mr, offset_within_container, map_mr);
To unmap, you'd first remove the subregion, to then unref the map_mr.
The only alternative would be to do it like (1) above: you perform all
of the mmap() yourself, and create it using
memory_region_init_ram_device_ptr(). This will set RAM_PREALLOC on the
RAMBlock and tell QEMU "this is special, just disregard it". The bad
thing about RAM_PREALLOC will be that it will not be compatible with
vfio, not communicated to other vhost-user devices etc ... whereby what
I describe above would just work with them.
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 1/5] vhost-user: Add VIRTIO Shared Memory map request
2024-11-27 10:50 ` David Hildenbrand
@ 2024-11-27 12:10 ` David Hildenbrand
2024-11-27 12:17 ` David Hildenbrand
2024-11-27 12:55 ` Albert Esteve
1 sibling, 1 reply; 29+ messages in thread
From: David Hildenbrand @ 2024-11-27 12:10 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, Alex Bennée, slp, hi, mst, jasowang, stefanha,
Stefano Garzarella, stevensd
On 27.11.24 11:50, David Hildenbrand wrote:
>
>>> RAM memory region/ RAMBlock that has properly set flags/fd/whatssoever
>>> and map whatever you want in there.
>>>
>>> Likely you would need a distinct RAMBlock/RAM memory region per mmap(),
>>> and would end up mmaping implicitly via qemu_ram_mmap().
>>>
>>> Then, your shared region would simply be an empty container into which
>>> you map these RAM memory regions.
>>
>
> Hi,
>
>> Hi, sorry it took me so long to get back to this. Lately I have been
>> testing the patch and fixing bugs, and I am was going to add some
>> tests to be able to verify the patch without having to use a backend
>> (which is what I am doing right now).
>>
>> But I wanted to address/discuss this comment. I am not sure of the
>> actual problem with the current approach (I am not completely aware of
>> the concern in your first paragraph), but I see other instances where
>> qemu mmaps stuff into a MemoryRegion.
>
> I suggest you take a look at the three relevant MAP_FIXED users outside
> of user emulation code.
>
> (1) hw/vfio/helpers.c: We create a custom memory region + RAMBlock with
> memory_region_init_ram_device_ptr(). This doesn't mmap(MAP_FIXED)
> into any existing RAMBlock.
>
> (2) system/physmem.c: I suggest you take a close look at
> qemu_ram_remap() and how it is one example of how RAMBlock
> properties describe exactly what is mmaped.
>
> (3) util/mmap-alloc.c: Well, this is the code that performs the mmap(),
> to bring a RAMBlock to life. See qemu_ram_mmap().
>
> There is some oddity hw/xen/xen-mapcache.c; XEN mapcache seems to manage
> guest RAM without RAMBlocks.
>
>> Take into account that the
>> implementation follows the definition of shared memory region here:
>> https://docs.oasis-open.org/virtio/virtio/v1.3/csd01/virtio-v1.3-csd01.html#x1-10200010
>> Which hints to a memory region per ID, not one per required map. So
>> the current strategy seems to fit it better.
>
> I'm confused, we are talking about an implementation detail here? How is
> that related to the spec?
>
>>
>> Also, I was aware that I was not the first one attempting this, so I
>> based this code in previous attempts (maybe I should give credit in
>> the commit now that I think of):
>> https://gitlab.com/virtio-fs/qemu/-/blob/qemu5.0-virtiofs-dax/hw/virtio/vhost-user-fs.c?ref_type=heads#L75
>> As you can see, it pretty much follows the same strategy.
>
> So, people did some hacky things in a QEMU fork 6 years ago ... :) That
> cannot possibly be a good argument why we should have it like that in QEMU.
>
>> And in my
>> examples I have been able to use this to video stream with multiple
>> queues mapped into the shared memory (used to capture video frames),
>> using the backend I mentioned above for testing. So the concept works.
>> I may be wrong with this, but for what I understood looking at the
>> code, crosvm uses a similar strategy. Reserve a memory block and use
>> for all your mappings, and use an allocator to find a free slot.
>>
>
> Again, I suggest you take a look at what a RAMBlock is, and how it's
> properties describe the containing mmap().
>
>> And if I were to do what you say, those distinct RAMBlocks should be
>> created when the device starts? What would be their size? Should I
>> create them when qemu receives a request to mmap? How would the driver
>> find the RAMBlock?
>
> You'd have an empty memory region container into which you will map
> memory regions that describe the memory you want to share.
>
> mr = g_new0(MemoryRegion, 1);
> memory_region_init(mr, OBJECT(TODO), "vhost-user-shm", region_size);
>
>
> Assuming you are requested to mmap an fd, you'd create a new
> MemoryRegion+RAMBlock that describes the memory and performs the mmap()
> for you:
>
> map_mr = g_new0(MemoryRegion, 1);
> memory_region_init_ram_from_fd(map_mr, OBJECT(TODO), "TODO", map_size,
> RAM_SHARED, map_fd, map_offs, errp);
>
> To then map it into your container:
>
> memory_region_add_subregion(mr, offset_within_container, map_mr);
>
>
> To unmap, you'd first remove the subregion, to then unref the map_mr.
>
>
>
> The only alternative would be to do it like (1) above: you perform all
> of the mmap() yourself, and create it using
> memory_region_init_ram_device_ptr(). This will set RAM_PREALLOC on the
> RAMBlock and tell QEMU "this is special, just disregard it". The bad
> thing about RAM_PREALLOC will be that it will not be compatible with
> vfio, not communicated to other vhost-user devices etc ... whereby what
> I describe above would just work with them.
>
FWIW, I took another look at this patch and I cannot really make sense
of what you are doing.
In virtio_new_shmem_region(), you allocate a region, but I don't see how
it would ever get initialized?
In vhost_user_backend_handle_shmem_map(), you then assume that it is
suddenly a RAM memory region? For example, that
memory_region_get_ram_ptr() returns something reasonable.
Likely, you really just want to initialize that MR using
memory_region_init_ram_from_fd(), and have that just perform the proper
mmap() for you and setup the RAMBlock?
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 1/5] vhost-user: Add VIRTIO Shared Memory map request
2024-11-27 12:10 ` David Hildenbrand
@ 2024-11-27 12:17 ` David Hildenbrand
2024-11-27 12:31 ` Albert Esteve
0 siblings, 1 reply; 29+ messages in thread
From: David Hildenbrand @ 2024-11-27 12:17 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, Alex Bennée, slp, hi, mst, jasowang, stefanha,
Stefano Garzarella, stevensd
On 27.11.24 13:10, David Hildenbrand wrote:
> On 27.11.24 11:50, David Hildenbrand wrote:
>>
>>>> RAM memory region/ RAMBlock that has properly set flags/fd/whatssoever
>>>> and map whatever you want in there.
>>>>
>>>> Likely you would need a distinct RAMBlock/RAM memory region per mmap(),
>>>> and would end up mmaping implicitly via qemu_ram_mmap().
>>>>
>>>> Then, your shared region would simply be an empty container into which
>>>> you map these RAM memory regions.
>>>
>>
>> Hi,
>>
>>> Hi, sorry it took me so long to get back to this. Lately I have been
>>> testing the patch and fixing bugs, and I am was going to add some
>>> tests to be able to verify the patch without having to use a backend
>>> (which is what I am doing right now).
>>>
>>> But I wanted to address/discuss this comment. I am not sure of the
>>> actual problem with the current approach (I am not completely aware of
>>> the concern in your first paragraph), but I see other instances where
>>> qemu mmaps stuff into a MemoryRegion.
>>
>> I suggest you take a look at the three relevant MAP_FIXED users outside
>> of user emulation code.
>>
>> (1) hw/vfio/helpers.c: We create a custom memory region + RAMBlock with
>> memory_region_init_ram_device_ptr(). This doesn't mmap(MAP_FIXED)
>> into any existing RAMBlock.
>>
>> (2) system/physmem.c: I suggest you take a close look at
>> qemu_ram_remap() and how it is one example of how RAMBlock
>> properties describe exactly what is mmaped.
>>
>> (3) util/mmap-alloc.c: Well, this is the code that performs the mmap(),
>> to bring a RAMBlock to life. See qemu_ram_mmap().
>>
>> There is some oddity hw/xen/xen-mapcache.c; XEN mapcache seems to manage
>> guest RAM without RAMBlocks.
>>
>>> Take into account that the
>>> implementation follows the definition of shared memory region here:
>>> https://docs.oasis-open.org/virtio/virtio/v1.3/csd01/virtio-v1.3-csd01.html#x1-10200010
>>> Which hints to a memory region per ID, not one per required map. So
>>> the current strategy seems to fit it better.
>>
>> I'm confused, we are talking about an implementation detail here? How is
>> that related to the spec?
>>
>>>
>>> Also, I was aware that I was not the first one attempting this, so I
>>> based this code in previous attempts (maybe I should give credit in
>>> the commit now that I think of):
>>> https://gitlab.com/virtio-fs/qemu/-/blob/qemu5.0-virtiofs-dax/hw/virtio/vhost-user-fs.c?ref_type=heads#L75
>>> As you can see, it pretty much follows the same strategy.
>>
>> So, people did some hacky things in a QEMU fork 6 years ago ... :) That
>> cannot possibly be a good argument why we should have it like that in QEMU.
>>
>>> And in my
>>> examples I have been able to use this to video stream with multiple
>>> queues mapped into the shared memory (used to capture video frames),
>>> using the backend I mentioned above for testing. So the concept works.
>>> I may be wrong with this, but for what I understood looking at the
>>> code, crosvm uses a similar strategy. Reserve a memory block and use
>>> for all your mappings, and use an allocator to find a free slot.
>>>
>>
>> Again, I suggest you take a look at what a RAMBlock is, and how it's
>> properties describe the containing mmap().
>>
>>> And if I were to do what you say, those distinct RAMBlocks should be
>>> created when the device starts? What would be their size? Should I
>>> create them when qemu receives a request to mmap? How would the driver
>>> find the RAMBlock?
>>
>> You'd have an empty memory region container into which you will map
>> memory regions that describe the memory you want to share.
>>
>> mr = g_new0(MemoryRegion, 1);
>> memory_region_init(mr, OBJECT(TODO), "vhost-user-shm", region_size);
>>
>>
>> Assuming you are requested to mmap an fd, you'd create a new
>> MemoryRegion+RAMBlock that describes the memory and performs the mmap()
>> for you:
>>
>> map_mr = g_new0(MemoryRegion, 1);
>> memory_region_init_ram_from_fd(map_mr, OBJECT(TODO), "TODO", map_size,
>> RAM_SHARED, map_fd, map_offs, errp);
>>
>> To then map it into your container:
>>
>> memory_region_add_subregion(mr, offset_within_container, map_mr);
>>
>>
>> To unmap, you'd first remove the subregion, to then unref the map_mr.
>>
>>
>>
>> The only alternative would be to do it like (1) above: you perform all
>> of the mmap() yourself, and create it using
>> memory_region_init_ram_device_ptr(). This will set RAM_PREALLOC on the
>> RAMBlock and tell QEMU "this is special, just disregard it". The bad
>> thing about RAM_PREALLOC will be that it will not be compatible with
>> vfio, not communicated to other vhost-user devices etc ... whereby what
>> I describe above would just work with them.
>>
>
> FWIW, I took another look at this patch and I cannot really make sense
> of what you are doing.
>
> In virtio_new_shmem_region(), you allocate a region, but I don't see how
> it would ever get initialized?
>
> In vhost_user_backend_handle_shmem_map(), you then assume that it is
> suddenly a RAM memory region? For example, that
> memory_region_get_ram_ptr() returns something reasonable.
>
> Likely, you really just want to initialize that MR using
> memory_region_init_ram_from_fd(), and have that just perform the proper
> mmap() for you and setup the RAMBlock?
In patch #4 I find: memory_region_init_ram_ptr(vdev->shmem_list[i].mr ...
Which does what I describe as the alternative. That makes it clearer
that you are not operating on arbitrary RAMBlocks. So that should indeed
work.
The way you structured these patches really is suboptimal for review.
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 1/5] vhost-user: Add VIRTIO Shared Memory map request
2024-11-27 12:17 ` David Hildenbrand
@ 2024-11-27 12:31 ` Albert Esteve
2024-11-27 12:40 ` David Hildenbrand
0 siblings, 1 reply; 29+ messages in thread
From: Albert Esteve @ 2024-11-27 12:31 UTC (permalink / raw)
To: David Hildenbrand
Cc: qemu-devel, Alex Bennée, slp, hi, mst, jasowang, stefanha,
Stefano Garzarella, stevensd
On Wed, Nov 27, 2024 at 1:18 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 27.11.24 13:10, David Hildenbrand wrote:
> > On 27.11.24 11:50, David Hildenbrand wrote:
> >>
> >>>> RAM memory region/ RAMBlock that has properly set flags/fd/whatssoever
> >>>> and map whatever you want in there.
> >>>>
> >>>> Likely you would need a distinct RAMBlock/RAM memory region per mmap(),
> >>>> and would end up mmaping implicitly via qemu_ram_mmap().
> >>>>
> >>>> Then, your shared region would simply be an empty container into which
> >>>> you map these RAM memory regions.
> >>>
> >>
> >> Hi,
> >>
> >>> Hi, sorry it took me so long to get back to this. Lately I have been
> >>> testing the patch and fixing bugs, and I am was going to add some
> >>> tests to be able to verify the patch without having to use a backend
> >>> (which is what I am doing right now).
> >>>
> >>> But I wanted to address/discuss this comment. I am not sure of the
> >>> actual problem with the current approach (I am not completely aware of
> >>> the concern in your first paragraph), but I see other instances where
> >>> qemu mmaps stuff into a MemoryRegion.
> >>
> >> I suggest you take a look at the three relevant MAP_FIXED users outside
> >> of user emulation code.
> >>
> >> (1) hw/vfio/helpers.c: We create a custom memory region + RAMBlock with
> >> memory_region_init_ram_device_ptr(). This doesn't mmap(MAP_FIXED)
> >> into any existing RAMBlock.
> >>
> >> (2) system/physmem.c: I suggest you take a close look at
> >> qemu_ram_remap() and how it is one example of how RAMBlock
> >> properties describe exactly what is mmaped.
> >>
> >> (3) util/mmap-alloc.c: Well, this is the code that performs the mmap(),
> >> to bring a RAMBlock to life. See qemu_ram_mmap().
> >>
> >> There is some oddity hw/xen/xen-mapcache.c; XEN mapcache seems to manage
> >> guest RAM without RAMBlocks.
> >>
> >>> Take into account that the
> >>> implementation follows the definition of shared memory region here:
> >>> https://docs.oasis-open.org/virtio/virtio/v1.3/csd01/virtio-v1.3-csd01.html#x1-10200010
> >>> Which hints to a memory region per ID, not one per required map. So
> >>> the current strategy seems to fit it better.
> >>
> >> I'm confused, we are talking about an implementation detail here? How is
> >> that related to the spec?
> >>
> >>>
> >>> Also, I was aware that I was not the first one attempting this, so I
> >>> based this code in previous attempts (maybe I should give credit in
> >>> the commit now that I think of):
> >>> https://gitlab.com/virtio-fs/qemu/-/blob/qemu5.0-virtiofs-dax/hw/virtio/vhost-user-fs.c?ref_type=heads#L75
> >>> As you can see, it pretty much follows the same strategy.
> >>
> >> So, people did some hacky things in a QEMU fork 6 years ago ... :) That
> >> cannot possibly be a good argument why we should have it like that in QEMU.
> >>
> >>> And in my
> >>> examples I have been able to use this to video stream with multiple
> >>> queues mapped into the shared memory (used to capture video frames),
> >>> using the backend I mentioned above for testing. So the concept works.
> >>> I may be wrong with this, but for what I understood looking at the
> >>> code, crosvm uses a similar strategy. Reserve a memory block and use
> >>> for all your mappings, and use an allocator to find a free slot.
> >>>
> >>
> >> Again, I suggest you take a look at what a RAMBlock is, and how it's
> >> properties describe the containing mmap().
> >>
> >>> And if I were to do what you say, those distinct RAMBlocks should be
> >>> created when the device starts? What would be their size? Should I
> >>> create them when qemu receives a request to mmap? How would the driver
> >>> find the RAMBlock?
> >>
> >> You'd have an empty memory region container into which you will map
> >> memory regions that describe the memory you want to share.
> >>
> >> mr = g_new0(MemoryRegion, 1);
> >> memory_region_init(mr, OBJECT(TODO), "vhost-user-shm", region_size);
> >>
> >>
> >> Assuming you are requested to mmap an fd, you'd create a new
> >> MemoryRegion+RAMBlock that describes the memory and performs the mmap()
> >> for you:
> >>
> >> map_mr = g_new0(MemoryRegion, 1);
> >> memory_region_init_ram_from_fd(map_mr, OBJECT(TODO), "TODO", map_size,
> >> RAM_SHARED, map_fd, map_offs, errp);
> >>
> >> To then map it into your container:
> >>
> >> memory_region_add_subregion(mr, offset_within_container, map_mr);
> >>
> >>
> >> To unmap, you'd first remove the subregion, to then unref the map_mr.
> >>
> >>
> >>
> >> The only alternative would be to do it like (1) above: you perform all
> >> of the mmap() yourself, and create it using
> >> memory_region_init_ram_device_ptr(). This will set RAM_PREALLOC on the
> >> RAMBlock and tell QEMU "this is special, just disregard it". The bad
> >> thing about RAM_PREALLOC will be that it will not be compatible with
> >> vfio, not communicated to other vhost-user devices etc ... whereby what
> >> I describe above would just work with them.
> >>
> >
> > FWIW, I took another look at this patch and I cannot really make sense
> > of what you are doing.
> >
> > In virtio_new_shmem_region(), you allocate a region, but I don't see how
> > it would ever get initialized?
> >
> > In vhost_user_backend_handle_shmem_map(), you then assume that it is
> > suddenly a RAM memory region? For example, that
> > memory_region_get_ram_ptr() returns something reasonable.
> >
> > Likely, you really just want to initialize that MR using
> > memory_region_init_ram_from_fd(), and have that just perform the proper
> > mmap() for you and setup the RAMBlock?
>
> In patch #4 I find: memory_region_init_ram_ptr(vdev->shmem_list[i].mr ...
>
> Which does what I describe as the alternative. That makes it clearer
> that you are not operating on arbitrary RAMBlocks. So that should indeed
> work.
>
> The way you structured these patches really is suboptimal for review.
Firstly, thank you so much for taking another look at the patch after
so much time. I understand it may be challenging given the time it
passed since I sent this version. Also, I rushed this version trying
to get to settle the spec first while I was working on something else,
so the code was untested and has quite a few bugs. I am mentioning it
just to note that I think this patch should not get a fine grain
review in its current status (although Stefan kindly did review it). I
fixed all (at least most) of the bugs in what would be the next
version, and it is what I use for testing. When I say I proved this
code and works, I am mostly referring to my local changes. As it is
here, this won't work.
That said, I understand what you mean, I will try to refactor /
reorder the patch to make it easier to review.
BR,
Albert.
>
> --
> Cheers,
>
> David / dhildenb
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 1/5] vhost-user: Add VIRTIO Shared Memory map request
2024-11-27 12:31 ` Albert Esteve
@ 2024-11-27 12:40 ` David Hildenbrand
0 siblings, 0 replies; 29+ messages in thread
From: David Hildenbrand @ 2024-11-27 12:40 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, Alex Bennée, slp, hi, mst, jasowang, stefanha,
Stefano Garzarella, stevensd
On 27.11.24 13:31, Albert Esteve wrote:
> On Wed, Nov 27, 2024 at 1:18 PM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 27.11.24 13:10, David Hildenbrand wrote:
>>> On 27.11.24 11:50, David Hildenbrand wrote:
>>>>
>>>>>> RAM memory region/ RAMBlock that has properly set flags/fd/whatssoever
>>>>>> and map whatever you want in there.
>>>>>>
>>>>>> Likely you would need a distinct RAMBlock/RAM memory region per mmap(),
>>>>>> and would end up mmaping implicitly via qemu_ram_mmap().
>>>>>>
>>>>>> Then, your shared region would simply be an empty container into which
>>>>>> you map these RAM memory regions.
>>>>>
>>>>
>>>> Hi,
>>>>
>>>>> Hi, sorry it took me so long to get back to this. Lately I have been
>>>>> testing the patch and fixing bugs, and I am was going to add some
>>>>> tests to be able to verify the patch without having to use a backend
>>>>> (which is what I am doing right now).
>>>>>
>>>>> But I wanted to address/discuss this comment. I am not sure of the
>>>>> actual problem with the current approach (I am not completely aware of
>>>>> the concern in your first paragraph), but I see other instances where
>>>>> qemu mmaps stuff into a MemoryRegion.
>>>>
>>>> I suggest you take a look at the three relevant MAP_FIXED users outside
>>>> of user emulation code.
>>>>
>>>> (1) hw/vfio/helpers.c: We create a custom memory region + RAMBlock with
>>>> memory_region_init_ram_device_ptr(). This doesn't mmap(MAP_FIXED)
>>>> into any existing RAMBlock.
>>>>
>>>> (2) system/physmem.c: I suggest you take a close look at
>>>> qemu_ram_remap() and how it is one example of how RAMBlock
>>>> properties describe exactly what is mmaped.
>>>>
>>>> (3) util/mmap-alloc.c: Well, this is the code that performs the mmap(),
>>>> to bring a RAMBlock to life. See qemu_ram_mmap().
>>>>
>>>> There is some oddity hw/xen/xen-mapcache.c; XEN mapcache seems to manage
>>>> guest RAM without RAMBlocks.
>>>>
>>>>> Take into account that the
>>>>> implementation follows the definition of shared memory region here:
>>>>> https://docs.oasis-open.org/virtio/virtio/v1.3/csd01/virtio-v1.3-csd01.html#x1-10200010
>>>>> Which hints to a memory region per ID, not one per required map. So
>>>>> the current strategy seems to fit it better.
>>>>
>>>> I'm confused, we are talking about an implementation detail here? How is
>>>> that related to the spec?
>>>>
>>>>>
>>>>> Also, I was aware that I was not the first one attempting this, so I
>>>>> based this code in previous attempts (maybe I should give credit in
>>>>> the commit now that I think of):
>>>>> https://gitlab.com/virtio-fs/qemu/-/blob/qemu5.0-virtiofs-dax/hw/virtio/vhost-user-fs.c?ref_type=heads#L75
>>>>> As you can see, it pretty much follows the same strategy.
>>>>
>>>> So, people did some hacky things in a QEMU fork 6 years ago ... :) That
>>>> cannot possibly be a good argument why we should have it like that in QEMU.
>>>>
>>>>> And in my
>>>>> examples I have been able to use this to video stream with multiple
>>>>> queues mapped into the shared memory (used to capture video frames),
>>>>> using the backend I mentioned above for testing. So the concept works.
>>>>> I may be wrong with this, but for what I understood looking at the
>>>>> code, crosvm uses a similar strategy. Reserve a memory block and use
>>>>> for all your mappings, and use an allocator to find a free slot.
>>>>>
>>>>
>>>> Again, I suggest you take a look at what a RAMBlock is, and how it's
>>>> properties describe the containing mmap().
>>>>
>>>>> And if I were to do what you say, those distinct RAMBlocks should be
>>>>> created when the device starts? What would be their size? Should I
>>>>> create them when qemu receives a request to mmap? How would the driver
>>>>> find the RAMBlock?
>>>>
>>>> You'd have an empty memory region container into which you will map
>>>> memory regions that describe the memory you want to share.
>>>>
>>>> mr = g_new0(MemoryRegion, 1);
>>>> memory_region_init(mr, OBJECT(TODO), "vhost-user-shm", region_size);
>>>>
>>>>
>>>> Assuming you are requested to mmap an fd, you'd create a new
>>>> MemoryRegion+RAMBlock that describes the memory and performs the mmap()
>>>> for you:
>>>>
>>>> map_mr = g_new0(MemoryRegion, 1);
>>>> memory_region_init_ram_from_fd(map_mr, OBJECT(TODO), "TODO", map_size,
>>>> RAM_SHARED, map_fd, map_offs, errp);
>>>>
>>>> To then map it into your container:
>>>>
>>>> memory_region_add_subregion(mr, offset_within_container, map_mr);
>>>>
>>>>
>>>> To unmap, you'd first remove the subregion, to then unref the map_mr.
>>>>
>>>>
>>>>
>>>> The only alternative would be to do it like (1) above: you perform all
>>>> of the mmap() yourself, and create it using
>>>> memory_region_init_ram_device_ptr(). This will set RAM_PREALLOC on the
>>>> RAMBlock and tell QEMU "this is special, just disregard it". The bad
>>>> thing about RAM_PREALLOC will be that it will not be compatible with
>>>> vfio, not communicated to other vhost-user devices etc ... whereby what
>>>> I describe above would just work with them.
>>>>
>>>
>>> FWIW, I took another look at this patch and I cannot really make sense
>>> of what you are doing.
>>>
>>> In virtio_new_shmem_region(), you allocate a region, but I don't see how
>>> it would ever get initialized?
>>>
>>> In vhost_user_backend_handle_shmem_map(), you then assume that it is
>>> suddenly a RAM memory region? For example, that
>>> memory_region_get_ram_ptr() returns something reasonable.
>>>
>>> Likely, you really just want to initialize that MR using
>>> memory_region_init_ram_from_fd(), and have that just perform the proper
>>> mmap() for you and setup the RAMBlock?
>>
>> In patch #4 I find: memory_region_init_ram_ptr(vdev->shmem_list[i].mr ...
>>
>> Which does what I describe as the alternative. That makes it clearer
>> that you are not operating on arbitrary RAMBlocks. So that should indeed
>> work.
>>
>> The way you structured these patches really is suboptimal for review.
>
> Firstly, thank you so much for taking another look at the patch after
> so much time. I understand it may be challenging given the time it
> passed since I sent this version. Also, I rushed this version trying
> to get to settle the spec first while I was working on something else,
> so the code was untested and has quite a few bugs. I am mentioning it
> just to note that I think this patch should not get a fine grain
> review in its current status (although Stefan kindly did review it). I
> fixed all (at least most) of the bugs in what would be the next
> version, and it is what I use for testing. When I say I proved this
> code and works, I am mostly referring to my local changes. As it is
> here, this won't work.
>
> That said, I understand what you mean, I will try to refactor /
> reorder the patch to make it easier to review.
If we could make it clearer that we are operating on RAM_PREALLOC
MRs/RAMBlocks in when handling shmem_map/shmem_unmap requests somehow,
that would already take quite some magic out of this code. :)
Having that said, I now understand why you want RAM_PREALLOC, because
you might only activate/deactivate some parts within a RAMBlock.
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 1/5] vhost-user: Add VIRTIO Shared Memory map request
2024-11-27 10:50 ` David Hildenbrand
2024-11-27 12:10 ` David Hildenbrand
@ 2024-11-27 12:55 ` Albert Esteve
1 sibling, 0 replies; 29+ messages in thread
From: Albert Esteve @ 2024-11-27 12:55 UTC (permalink / raw)
To: David Hildenbrand
Cc: qemu-devel, Alex Bennée, slp, hi, mst, jasowang, stefanha,
Stefano Garzarella, stevensd
On Wed, Nov 27, 2024 at 11:50 AM David Hildenbrand <david@redhat.com> wrote:
>
>
> >> RAM memory region/ RAMBlock that has properly set flags/fd/whatssoever
> >> and map whatever you want in there.
> >>
> >> Likely you would need a distinct RAMBlock/RAM memory region per mmap(),
> >> and would end up mmaping implicitly via qemu_ram_mmap().
> >>
> >> Then, your shared region would simply be an empty container into which
> >> you map these RAM memory regions.
> >
>
> Hi,
>
> > Hi, sorry it took me so long to get back to this. Lately I have been
> > testing the patch and fixing bugs, and I am was going to add some
> > tests to be able to verify the patch without having to use a backend
> > (which is what I am doing right now).
> >
> > But I wanted to address/discuss this comment. I am not sure of the
> > actual problem with the current approach (I am not completely aware of
> > the concern in your first paragraph), but I see other instances where
> > qemu mmaps stuff into a MemoryRegion.
>
> I suggest you take a look at the three relevant MAP_FIXED users outside
> of user emulation code.
>
> (1) hw/vfio/helpers.c: We create a custom memory region + RAMBlock with
> memory_region_init_ram_device_ptr(). This doesn't mmap(MAP_FIXED)
> into any existing RAMBlock.
>
> (2) system/physmem.c: I suggest you take a close look at
> qemu_ram_remap() and how it is one example of how RAMBlock
> properties describe exactly what is mmaped.
>
> (3) util/mmap-alloc.c: Well, this is the code that performs the mmap(),
> to bring a RAMBlock to life. See qemu_ram_mmap().
>
> There is some oddity hw/xen/xen-mapcache.c; XEN mapcache seems to manage
> guest RAM without RAMBlocks.
>
> > Take into account that the
> > implementation follows the definition of shared memory region here:
> > https://docs.oasis-open.org/virtio/virtio/v1.3/csd01/virtio-v1.3-csd01.html#x1-10200010
> > Which hints to a memory region per ID, not one per required map. So
> > the current strategy seems to fit it better.
>
> I'm confused, we are talking about an implementation detail here? How is
> that related to the spec?
What I try to say is that for me, conceptually, sounds weird to
implement it (as per your suggestion) as a ram block per mmap(); when
the concept we are trying to implement is multiple ram blocks, with
different IDs, and each accepting multiple mmap() as long as there is
enough free space. I fail to see how having multiple IDs for different
shared memory block translates to the implementation when each mmap()
gets its own RAMBlock. Or how the offset the device requests for the
mmap has a meaning when the base memory will change for different
RAMBlocks? But I do not mean as it your suggestion is wrong, I
definitively need to review this more in depth, as my understanding of
this code in QEMU is relatively limited still.
>
> >
> > Also, I was aware that I was not the first one attempting this, so I
> > based this code in previous attempts (maybe I should give credit in
> > the commit now that I think of):
> > https://gitlab.com/virtio-fs/qemu/-/blob/qemu5.0-virtiofs-dax/hw/virtio/vhost-user-fs.c?ref_type=heads#L75
> > As you can see, it pretty much follows the same strategy.
>
> So, people did some hacky things in a QEMU fork 6 years ago ... :) That
> cannot possibly be a good argument why we should have it like that in QEMU.
Fair. I just took those patches as a good source for what I was trying
to do, as people involved had (and have, for the most part :) ) better
idea of QEMU internals that I do. But it is true that that does not
make it a source of truth.
>
> > And in my
> > examples I have been able to use this to video stream with multiple
> > queues mapped into the shared memory (used to capture video frames),
> > using the backend I mentioned above for testing. So the concept works.
> > I may be wrong with this, but for what I understood looking at the
> > code, crosvm uses a similar strategy. Reserve a memory block and use
> > for all your mappings, and use an allocator to find a free slot.
> >
>
> Again, I suggest you take a look at what a RAMBlock is, and how it's
> properties describe the containing mmap().
>
> > And if I were to do what you say, those distinct RAMBlocks should be
> > created when the device starts? What would be their size? Should I
> > create them when qemu receives a request to mmap? How would the driver
> > find the RAMBlock?
>
> You'd have an empty memory region container into which you will map
> memory regions that describe the memory you want to share.
>
> mr = g_new0(MemoryRegion, 1);
> memory_region_init(mr, OBJECT(TODO), "vhost-user-shm", region_size);
>
>
> Assuming you are requested to mmap an fd, you'd create a new
> MemoryRegion+RAMBlock that describes the memory and performs the mmap()
> for you:
>
> map_mr = g_new0(MemoryRegion, 1);
> memory_region_init_ram_from_fd(map_mr, OBJECT(TODO), "TODO", map_size,
> RAM_SHARED, map_fd, map_offs, errp);
>
> To then map it into your container:
>
> memory_region_add_subregion(mr, offset_within_container, map_mr);
>
>
> To unmap, you'd first remove the subregion, to then unref the map_mr.
>
>
>
> The only alternative would be to do it like (1) above: you perform all
> of the mmap() yourself, and create it using
> memory_region_init_ram_device_ptr(). This will set RAM_PREALLOC on the
> RAMBlock and tell QEMU "this is special, just disregard it". The bad
> thing about RAM_PREALLOC will be that it will not be compatible with
> vfio, not communicated to other vhost-user devices etc ... whereby what
> I describe above would just work with them.
OK. Given that I have a device with which to test, I think it is
definitively worth trying to implement this approach and see how it
works. I'll respond to this thread with progress/results before
sending the next version.
And thanks for the explanation!
BR,
Albert.
>
> --
> Cheers,
>
> David / dhildenb
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 2/5] virtio: Track shared memory mappings
2024-09-12 14:53 [PATCH v3 0/5] vhost-user: Add SHMEM_MAP/UNMAP requests Albert Esteve
2024-09-12 14:53 ` [PATCH v3 1/5] vhost-user: Add VIRTIO Shared Memory map request Albert Esteve
@ 2024-09-12 14:53 ` Albert Esteve
2024-09-16 17:27 ` Stefan Hajnoczi
2024-09-12 14:53 ` [PATCH v3 3/5] vhost_user: Add frontend command for shmem config Albert Esteve
` (4 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: Albert Esteve @ 2024-09-12 14:53 UTC (permalink / raw)
To: qemu-devel
Cc: Alex Bennée, slp, hi, mst, david, jasowang, stefanha,
Stefano Garzarella, stevensd, Albert Esteve
Update shmem_list to be able to track
active mappings on VIRTIO shared memory
regions. This allows to verify that new
mapping request received from backends
do not overlap. If they do, the request
shall fail in order to adhere to the specs.
Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
hw/virtio/vhost-user.c | 31 +++++++++++++-------
hw/virtio/virtio.c | 58 ++++++++++++++++++++++++++++++++++----
include/hw/virtio/virtio.h | 25 ++++++++++++++--
3 files changed, 96 insertions(+), 18 deletions(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 338cc942ec..de0bb35257 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1776,7 +1776,7 @@ vhost_user_backend_handle_shmem_map(struct vhost_dev *dev,
int fd)
{
void *addr = 0;
- MemoryRegion *mr = NULL;
+ VirtSharedMemory *shmem = NULL;
if (fd < 0) {
error_report("Bad fd for map");
@@ -1791,22 +1791,29 @@ vhost_user_backend_handle_shmem_map(struct vhost_dev *dev,
return -EFAULT;
}
- mr = &dev->vdev->shmem_list[vu_mmap->shmid];
+ shmem = &dev->vdev->shmem_list[vu_mmap->shmid];
- if (!mr) {
+ 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) > mr->size) {
+ (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;
}
- void *shmem_ptr = memory_region_get_ram_ptr(mr);
+ if (virtio_shmem_map_overlaps(shmem, vu_mmap->shm_offset, vu_mmap->len)) {
+ error_report("Requested memory (%" PRIx64 "+%" PRIx64 ") overalps "
+ "with previously mapped memory",
+ vu_mmap->shm_offset, vu_mmap->len);
+ return -EFAULT;
+ }
+
+ void *shmem_ptr = memory_region_get_ram_ptr(shmem->mr);
addr = mmap(shmem_ptr + vu_mmap->shm_offset, vu_mmap->len,
((vu_mmap->flags & VHOST_USER_FLAG_MAP_R) ? PROT_READ : 0) |
@@ -1818,6 +1825,8 @@ vhost_user_backend_handle_shmem_map(struct vhost_dev *dev,
return -EFAULT;
}
+ virtio_add_shmem_map(shmem, vu_mmap->shm_offset, vu_mmap->len);
+
return 0;
}
@@ -1826,7 +1835,7 @@ vhost_user_backend_handle_shmem_unmap(struct vhost_dev *dev,
VhostUserMMap *vu_mmap)
{
void *addr = 0;
- MemoryRegion *mr = NULL;
+ VirtSharedMemory *shmem = NULL;
if (!dev->vdev->shmem_list ||
dev->vdev->n_shmem_regions <= vu_mmap->shmid) {
@@ -1836,22 +1845,22 @@ vhost_user_backend_handle_shmem_unmap(struct vhost_dev *dev,
return -EFAULT;
}
- mr = &dev->vdev->shmem_list[vu_mmap->shmid];
+ shmem = &dev->vdev->shmem_list[vu_mmap->shmid];
- if (!mr) {
+ 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) > mr->size) {
+ (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;
}
- void *shmem_ptr = memory_region_get_ram_ptr(mr);
+ void *shmem_ptr = memory_region_get_ram_ptr(shmem->mr);
addr = mmap(shmem_ptr + vu_mmap->shm_offset, vu_mmap->len,
PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
@@ -1861,6 +1870,8 @@ vhost_user_backend_handle_shmem_unmap(struct vhost_dev *dev,
return -EFAULT;
}
+ virtio_del_shmem_map(shmem, vu_mmap->shm_offset, vu_mmap->len);
+
return 0;
}
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index ccc4f2cd75..0e2cd62a15 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3059,15 +3059,52 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f)
return vmstate_save_state(f, &vmstate_virtio, vdev, NULL);
}
-MemoryRegion *virtio_new_shmem_region(VirtIODevice *vdev)
+VirtSharedMemory *virtio_new_shmem_region(VirtIODevice *vdev)
{
- MemoryRegion *mr;
+ VirtSharedMemory *shmem = NULL;
++vdev->n_shmem_regions;
- vdev->shmem_list = g_renew(MemoryRegion, vdev->shmem_list,
+ vdev->shmem_list = g_renew(VirtSharedMemory, vdev->shmem_list,
vdev->n_shmem_regions);
- mr = &vdev->shmem_list[vdev->n_shmem_regions - 1];
- mr = g_new0(MemoryRegion, 1);
- return mr;
+ shmem = &vdev->shmem_list[vdev->n_shmem_regions - 1];
+ shmem = g_new0(VirtSharedMemory, 1);
+ QTAILQ_INIT(&shmem->mapped_regions);
+ return shmem;
+}
+
+void virtio_add_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
+ uint64_t size)
+{
+ MappedMemoryRegion *mmap = g_new0(MappedMemoryRegion, 1);
+ mmap->offset = offset;
+ mmap->size = int128_make64(size);
+ QTAILQ_REMOVE(&shmem->mapped_regions, mmap, link);
+ g_free(mmap);
+}
+
+void virtio_del_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
+ uint64_t size)
+{
+ MappedMemoryRegion *mmap = g_new0(MappedMemoryRegion, 1);
+ mmap->offset = offset;
+ mmap->size = int128_make64(size);
+ QTAILQ_INSERT_TAIL(&shmem->mapped_regions, mmap, link);
+ g_free(mmap);
+}
+
+bool virtio_shmem_map_overlaps(VirtSharedMemory *shmem, hwaddr offset,
+ uint64_t size)
+{
+ MappedMemoryRegion *map_reg;
+ hwaddr new_reg_end = offset + size;
+ QTAILQ_FOREACH(map_reg, &shmem->mapped_regions, link) {
+ hwaddr region_end = map_reg->offset + map_reg->size;
+ if ((map_reg->offset == offset) ||
+ (map_reg->offset < offset && region_end >= offset) ||
+ (offset < map_reg->offset && new_reg_end >= map_reg->offset )) {
+ return true;
+ }
+ }
+ return false;
}
/* A wrapper for use as a VMState .put function */
@@ -4007,11 +4044,20 @@ 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->mapped_regions)) {
+ MappedMemoryRegion *mmap_reg = QTAILQ_FIRST(&shmem->mapped_regions);
+ QTAILQ_REMOVE(&shmem->mapped_regions, mmap_reg, link);
+ }
+ }
}
static Property virtio_properties[] = {
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index d4a2f664d9..5b801f33f5 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 {
+ Int128 size;
+ hwaddr offset;
+ QTAILQ_ENTRY(MappedMemoryRegion) link;
+};
+
+typedef struct MappedMemoryRegion MappedMemoryRegion;
+
+struct VirtSharedMemory {
+ MemoryRegion *mr;
+ QTAILQ_HEAD(, MappedMemoryRegion) mapped_regions;
+};
+
+typedef struct VirtSharedMemory VirtSharedMemory;
+
/**
* struct VirtIODevice - common VirtIO structure
* @name: name of the device
@@ -168,7 +183,7 @@ struct VirtIODevice
EventNotifier config_notifier;
bool device_iotlb_enabled;
/* Shared memory region for vhost-user mappings. */
- MemoryRegion *shmem_list;
+ VirtSharedMemory *shmem_list;
int n_shmem_regions;
};
@@ -289,7 +304,13 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
int virtio_save(VirtIODevice *vdev, QEMUFile *f);
-MemoryRegion *virtio_new_shmem_region(VirtIODevice *vdev);
+VirtSharedMemory *virtio_new_shmem_region(VirtIODevice *vdev);
+void virtio_add_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
+ uint64_t size);
+void virtio_del_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
+ uint64_t size);
+bool virtio_shmem_map_overlaps(VirtSharedMemory *shmem, hwaddr offset,
+ uint64_t size);
extern const VMStateInfo virtio_vmstate_info;
--
2.45.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 2/5] virtio: Track shared memory mappings
2024-09-12 14:53 ` [PATCH v3 2/5] virtio: Track shared memory mappings Albert Esteve
@ 2024-09-16 17:27 ` Stefan Hajnoczi
0 siblings, 0 replies; 29+ messages in thread
From: Stefan Hajnoczi @ 2024-09-16 17:27 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, Alex Bennée, slp, hi, mst, david, jasowang,
Stefano Garzarella, stevensd
[-- Attachment #1: Type: text/plain, Size: 9801 bytes --]
On Thu, Sep 12, 2024 at 04:53:32PM +0200, Albert Esteve wrote:
> Update shmem_list to be able to track
> active mappings on VIRTIO shared memory
> regions. This allows to verify that new
> mapping request received from backends
> do not overlap. If they do, the request
> shall fail in order to adhere to the specs.
>
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> ---
> hw/virtio/vhost-user.c | 31 +++++++++++++-------
> hw/virtio/virtio.c | 58 ++++++++++++++++++++++++++++++++++----
> include/hw/virtio/virtio.h | 25 ++++++++++++++--
> 3 files changed, 96 insertions(+), 18 deletions(-)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 338cc942ec..de0bb35257 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1776,7 +1776,7 @@ vhost_user_backend_handle_shmem_map(struct vhost_dev *dev,
> int fd)
> {
> void *addr = 0;
> - MemoryRegion *mr = NULL;
> + VirtSharedMemory *shmem = NULL;
>
> if (fd < 0) {
> error_report("Bad fd for map");
> @@ -1791,22 +1791,29 @@ vhost_user_backend_handle_shmem_map(struct vhost_dev *dev,
> return -EFAULT;
> }
>
> - mr = &dev->vdev->shmem_list[vu_mmap->shmid];
> + shmem = &dev->vdev->shmem_list[vu_mmap->shmid];
>
> - if (!mr) {
> + 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) > mr->size) {
> + (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;
> }
>
> - void *shmem_ptr = memory_region_get_ram_ptr(mr);
> + if (virtio_shmem_map_overlaps(shmem, vu_mmap->shm_offset, vu_mmap->len)) {
> + error_report("Requested memory (%" PRIx64 "+%" PRIx64 ") overalps "
> + "with previously mapped memory",
> + vu_mmap->shm_offset, vu_mmap->len);
> + return -EFAULT;
> + }
> +
> + void *shmem_ptr = memory_region_get_ram_ptr(shmem->mr);
>
> addr = mmap(shmem_ptr + vu_mmap->shm_offset, vu_mmap->len,
> ((vu_mmap->flags & VHOST_USER_FLAG_MAP_R) ? PROT_READ : 0) |
> @@ -1818,6 +1825,8 @@ vhost_user_backend_handle_shmem_map(struct vhost_dev *dev,
> return -EFAULT;
> }
>
> + virtio_add_shmem_map(shmem, vu_mmap->shm_offset, vu_mmap->len);
> +
> return 0;
> }
>
> @@ -1826,7 +1835,7 @@ vhost_user_backend_handle_shmem_unmap(struct vhost_dev *dev,
> VhostUserMMap *vu_mmap)
> {
> void *addr = 0;
> - MemoryRegion *mr = NULL;
> + VirtSharedMemory *shmem = NULL;
>
> if (!dev->vdev->shmem_list ||
> dev->vdev->n_shmem_regions <= vu_mmap->shmid) {
> @@ -1836,22 +1845,22 @@ vhost_user_backend_handle_shmem_unmap(struct vhost_dev *dev,
> return -EFAULT;
> }
>
> - mr = &dev->vdev->shmem_list[vu_mmap->shmid];
> + shmem = &dev->vdev->shmem_list[vu_mmap->shmid];
>
> - if (!mr) {
> + 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) > mr->size) {
> + (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;
> }
Please add a check for an existing mapping that matches [shm_offset,
shm_offset + len). This will prevent partial unmap as required by the
spec.
>
> - void *shmem_ptr = memory_region_get_ram_ptr(mr);
> + void *shmem_ptr = memory_region_get_ram_ptr(shmem->mr);
>
> addr = mmap(shmem_ptr + vu_mmap->shm_offset, vu_mmap->len,
> PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
> @@ -1861,6 +1870,8 @@ vhost_user_backend_handle_shmem_unmap(struct vhost_dev *dev,
> return -EFAULT;
> }
>
> + virtio_del_shmem_map(shmem, vu_mmap->shm_offset, vu_mmap->len);
> +
> return 0;
> }
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index ccc4f2cd75..0e2cd62a15 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3059,15 +3059,52 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f)
> return vmstate_save_state(f, &vmstate_virtio, vdev, NULL);
> }
>
> -MemoryRegion *virtio_new_shmem_region(VirtIODevice *vdev)
> +VirtSharedMemory *virtio_new_shmem_region(VirtIODevice *vdev)
> {
> - MemoryRegion *mr;
> + VirtSharedMemory *shmem = NULL;
> ++vdev->n_shmem_regions;
> - vdev->shmem_list = g_renew(MemoryRegion, vdev->shmem_list,
> + vdev->shmem_list = g_renew(VirtSharedMemory, vdev->shmem_list,
> vdev->n_shmem_regions);
> - mr = &vdev->shmem_list[vdev->n_shmem_regions - 1];
> - mr = g_new0(MemoryRegion, 1);
> - return mr;
> + shmem = &vdev->shmem_list[vdev->n_shmem_regions - 1];
> + shmem = g_new0(VirtSharedMemory, 1);
> + QTAILQ_INIT(&shmem->mapped_regions);
> + return shmem;
> +}
> +
> +void virtio_add_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
> + uint64_t size)
> +{
> + MappedMemoryRegion *mmap = g_new0(MappedMemoryRegion, 1);
> + mmap->offset = offset;
> + mmap->size = int128_make64(size);
> + QTAILQ_REMOVE(&shmem->mapped_regions, mmap, link);
> + g_free(mmap);
mmap needs to be inserted into mapped_regions and must not be freed by
this function.
> +}
> +
> +void virtio_del_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
> + uint64_t size)
> +{
> + MappedMemoryRegion *mmap = g_new0(MappedMemoryRegion, 1);
> + mmap->offset = offset;
> + mmap->size = int128_make64(size);
> + QTAILQ_INSERT_TAIL(&shmem->mapped_regions, mmap, link);
This function needs to search the list for the matching mmap and remove
it from the list. It should not create a new mmap.
> + g_free(mmap);
> +}
> +
> +bool virtio_shmem_map_overlaps(VirtSharedMemory *shmem, hwaddr offset,
> + uint64_t size)
> +{
> + MappedMemoryRegion *map_reg;
> + hwaddr new_reg_end = offset + size;
> + QTAILQ_FOREACH(map_reg, &shmem->mapped_regions, link) {
> + hwaddr region_end = map_reg->offset + map_reg->size;
> + if ((map_reg->offset == offset) ||
> + (map_reg->offset < offset && region_end >= offset) ||
> + (offset < map_reg->offset && new_reg_end >= map_reg->offset )) {
> + return true;
> + }
> + }
> + return false;
> }
>
> /* A wrapper for use as a VMState .put function */
> @@ -4007,11 +4044,20 @@ 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->mapped_regions)) {
> + MappedMemoryRegion *mmap_reg = QTAILQ_FIRST(&shmem->mapped_regions);
> + QTAILQ_REMOVE(&shmem->mapped_regions, mmap_reg, link);
g_free(mmap_reg)
> + }
> + }
> }
>
> static Property virtio_properties[] = {
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index d4a2f664d9..5b801f33f5 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 {
> + Int128 size;
> + hwaddr offset;
> + QTAILQ_ENTRY(MappedMemoryRegion) link;
> +};
> +
> +typedef struct MappedMemoryRegion MappedMemoryRegion;
> +
> +struct VirtSharedMemory {
> + MemoryRegion *mr;
> + QTAILQ_HEAD(, MappedMemoryRegion) mapped_regions;
> +};
> +
> +typedef struct VirtSharedMemory VirtSharedMemory;
> +
> /**
> * struct VirtIODevice - common VirtIO structure
> * @name: name of the device
> @@ -168,7 +183,7 @@ struct VirtIODevice
> EventNotifier config_notifier;
> bool device_iotlb_enabled;
> /* Shared memory region for vhost-user mappings. */
> - MemoryRegion *shmem_list;
> + VirtSharedMemory *shmem_list;
> int n_shmem_regions;
> };
>
> @@ -289,7 +304,13 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
>
> int virtio_save(VirtIODevice *vdev, QEMUFile *f);
>
> -MemoryRegion *virtio_new_shmem_region(VirtIODevice *vdev);
> +VirtSharedMemory *virtio_new_shmem_region(VirtIODevice *vdev);
> +void virtio_add_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
> + uint64_t size);
> +void virtio_del_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
> + uint64_t size);
> +bool virtio_shmem_map_overlaps(VirtSharedMemory *shmem, hwaddr offset,
> + uint64_t size);
>
> extern const VMStateInfo virtio_vmstate_info;
>
> --
> 2.45.2
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 3/5] vhost_user: Add frontend command for shmem config
2024-09-12 14:53 [PATCH v3 0/5] vhost-user: Add SHMEM_MAP/UNMAP requests Albert Esteve
2024-09-12 14:53 ` [PATCH v3 1/5] vhost-user: Add VIRTIO Shared Memory map request Albert Esteve
2024-09-12 14:53 ` [PATCH v3 2/5] virtio: Track shared memory mappings Albert Esteve
@ 2024-09-12 14:53 ` Albert Esteve
2024-09-17 7:48 ` Stefan Hajnoczi
2024-09-17 7:50 ` Stefan Hajnoczi
2024-09-12 14:53 ` [PATCH v3 4/5] vhost-user-dev: Add cache BAR Albert Esteve
` (3 subsequent siblings)
6 siblings, 2 replies; 29+ messages in thread
From: Albert Esteve @ 2024-09-12 14:53 UTC (permalink / raw)
To: qemu-devel
Cc: Alex Bennée, slp, hi, mst, david, jasowang, stefanha,
Stefano Garzarella, stevensd, 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 | 45 +++++++++++++++++++++++++++++++
include/hw/virtio/vhost-backend.h | 6 +++++
include/hw/virtio/vhost-user.h | 1 +
3 files changed, 52 insertions(+)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index de0bb35257..83f5c02bea 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -57,6 +57,8 @@
*/
#define VHOST_USER_MAX_CONFIG_SIZE 256
+#define VHOST_USER_MAX_SHMEM_REGIONS 256
+
#define VHOST_USER_PROTOCOL_FEATURE_MASK ((1 << VHOST_USER_PROTOCOL_F_MAX) - 1)
typedef enum VhostUserRequest {
@@ -104,6 +106,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 +141,12 @@ typedef struct VhostUserMemRegMsg {
VhostUserMemoryRegion region;
} VhostUserMemRegMsg;
+typedef struct VhostUserShMemConfig {
+ uint32_t nregions;
+ uint32_t padding;
+ uint64_t memory_sizes[VHOST_MEMORY_BASELINE_NREGIONS];
+} VhostUserShMemConfig;
+
typedef struct VhostUserLog {
uint64_t mmap_size;
uint64_t mmap_offset;
@@ -245,6 +254,7 @@ typedef union {
VhostUserShared object;
VhostUserTransferDeviceState transfer_state;
VhostUserMMap mmap;
+ VhostUserShMemConfig shmem;
} VhostUserPayload;
typedef struct VhostUserMsg {
@@ -3134,6 +3144,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 <= VHOST_USER_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,
@@ -3172,4 +3216,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..f9c2955420 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -159,6 +159,11 @@ 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);
+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 +219,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 324cd8663a..d4bb2c3958 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
};
--
2.45.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 3/5] vhost_user: Add frontend command for shmem config
2024-09-12 14:53 ` [PATCH v3 3/5] vhost_user: Add frontend command for shmem config Albert Esteve
@ 2024-09-17 7:48 ` Stefan Hajnoczi
2024-09-17 7:50 ` Stefan Hajnoczi
1 sibling, 0 replies; 29+ messages in thread
From: Stefan Hajnoczi @ 2024-09-17 7:48 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, Alex Bennée, slp, hi, mst, david, jasowang,
Stefano Garzarella, stevensd
[-- Attachment #1: Type: text/plain, Size: 5924 bytes --]
On Thu, Sep 12, 2024 at 04:53:33PM +0200, 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 | 45 +++++++++++++++++++++++++++++++
> include/hw/virtio/vhost-backend.h | 6 +++++
> include/hw/virtio/vhost-user.h | 1 +
> 3 files changed, 52 insertions(+)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index de0bb35257..83f5c02bea 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -57,6 +57,8 @@
> */
> #define VHOST_USER_MAX_CONFIG_SIZE 256
>
> +#define VHOST_USER_MAX_SHMEM_REGIONS 256
> +
> #define VHOST_USER_PROTOCOL_FEATURE_MASK ((1 << VHOST_USER_PROTOCOL_F_MAX) - 1)
>
> typedef enum VhostUserRequest {
> @@ -104,6 +106,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 +141,12 @@ typedef struct VhostUserMemRegMsg {
> VhostUserMemoryRegion region;
> } VhostUserMemRegMsg;
>
> +typedef struct VhostUserShMemConfig {
> + uint32_t nregions;
> + uint32_t padding;
> + uint64_t memory_sizes[VHOST_MEMORY_BASELINE_NREGIONS];
> +} VhostUserShMemConfig;
> +
> typedef struct VhostUserLog {
> uint64_t mmap_size;
> uint64_t mmap_offset;
> @@ -245,6 +254,7 @@ typedef union {
> VhostUserShared object;
> VhostUserTransferDeviceState transfer_state;
> VhostUserMMap mmap;
> + VhostUserShMemConfig shmem;
> } VhostUserPayload;
>
> typedef struct VhostUserMsg {
> @@ -3134,6 +3144,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 <= VHOST_USER_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,
> @@ -3172,4 +3216,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..f9c2955420 100644
> --- a/include/hw/virtio/vhost-backend.h
> +++ b/include/hw/virtio/vhost-backend.h
> @@ -159,6 +159,11 @@ 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);
> +typedef int (*vhost_get_shmem_config_op)(struct vhost_dev *dev,
> + int *nregions,
> + uint64_t *memory_sizes,
VHOST_USER_MAX_SHMEM_REGIONS is defined inside vhost-user.c and not
visible here. However, vhost_get_shmem_config_op() assumes the caller
knows the maximum number of memory_sizes[] array elements.
I think a constant should be declared in the VIRTIO header files and a
doc comment is needed here stating that memory_sizes[] must be at least
VIRTIO_MAX_SHMEM_REGIONS.
> + Error **errp);
> +
>
> typedef struct VhostOps {
> VhostBackendType backend_type;
> @@ -214,6 +219,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 324cd8663a..d4bb2c3958 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
> };
>
> --
> 2.45.2
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 3/5] vhost_user: Add frontend command for shmem config
2024-09-12 14:53 ` [PATCH v3 3/5] vhost_user: Add frontend command for shmem config Albert Esteve
2024-09-17 7:48 ` Stefan Hajnoczi
@ 2024-09-17 7:50 ` Stefan Hajnoczi
1 sibling, 0 replies; 29+ messages in thread
From: Stefan Hajnoczi @ 2024-09-17 7:50 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, Alex Bennée, slp, hi, mst, david, jasowang,
Stefano Garzarella, stevensd
[-- Attachment #1: Type: text/plain, Size: 5604 bytes --]
On Thu, Sep 12, 2024 at 04:53:33PM +0200, 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 | 45 +++++++++++++++++++++++++++++++
> include/hw/virtio/vhost-backend.h | 6 +++++
> include/hw/virtio/vhost-user.h | 1 +
> 3 files changed, 52 insertions(+)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index de0bb35257..83f5c02bea 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -57,6 +57,8 @@
> */
> #define VHOST_USER_MAX_CONFIG_SIZE 256
>
> +#define VHOST_USER_MAX_SHMEM_REGIONS 256
> +
> #define VHOST_USER_PROTOCOL_FEATURE_MASK ((1 << VHOST_USER_PROTOCOL_F_MAX) - 1)
>
> typedef enum VhostUserRequest {
> @@ -104,6 +106,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 +141,12 @@ typedef struct VhostUserMemRegMsg {
> VhostUserMemoryRegion region;
> } VhostUserMemRegMsg;
>
> +typedef struct VhostUserShMemConfig {
> + uint32_t nregions;
> + uint32_t padding;
> + uint64_t memory_sizes[VHOST_MEMORY_BASELINE_NREGIONS];
I think this should be VHOST_USER_MAX_SHMEM_REGIONS.
> +} VhostUserShMemConfig;
> +
> typedef struct VhostUserLog {
> uint64_t mmap_size;
> uint64_t mmap_offset;
> @@ -245,6 +254,7 @@ typedef union {
> VhostUserShared object;
> VhostUserTransferDeviceState transfer_state;
> VhostUserMMap mmap;
> + VhostUserShMemConfig shmem;
> } VhostUserPayload;
>
> typedef struct VhostUserMsg {
> @@ -3134,6 +3144,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 <= VHOST_USER_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,
> @@ -3172,4 +3216,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..f9c2955420 100644
> --- a/include/hw/virtio/vhost-backend.h
> +++ b/include/hw/virtio/vhost-backend.h
> @@ -159,6 +159,11 @@ 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);
> +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 +219,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 324cd8663a..d4bb2c3958 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
> };
>
> --
> 2.45.2
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 4/5] vhost-user-dev: Add cache BAR
2024-09-12 14:53 [PATCH v3 0/5] vhost-user: Add SHMEM_MAP/UNMAP requests Albert Esteve
` (2 preceding siblings ...)
2024-09-12 14:53 ` [PATCH v3 3/5] vhost_user: Add frontend command for shmem config Albert Esteve
@ 2024-09-12 14:53 ` Albert Esteve
2024-09-17 8:27 ` Stefan Hajnoczi
2024-09-17 8:32 ` Stefan Hajnoczi
2024-09-12 14:53 ` [PATCH v3 5/5] vhost_user: Add MEM_READ/WRITE backend requests Albert Esteve
` (2 subsequent siblings)
6 siblings, 2 replies; 29+ messages in thread
From: Albert Esteve @ 2024-09-12 14:53 UTC (permalink / raw)
To: qemu-devel
Cc: Alex Bennée, slp, hi, mst, david, jasowang, stefanha,
Stefano Garzarella, stevensd, Albert Esteve
Add a cache BAR in the vhost-user-device
into which files can be directly mapped.
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 | 37 +++++++++++++++++++++++++++--
hw/virtio/vhost-user-device-pci.c | 39 ++++++++++++++++++++++++++++---
2 files changed, 71 insertions(+), 5 deletions(-)
diff --git a/hw/virtio/vhost-user-base.c b/hw/virtio/vhost-user-base.c
index 2bc3423326..f2597d021a 100644
--- a/hw/virtio/vhost-user-base.c
+++ b/hw/virtio/vhost-user-base.c
@@ -271,7 +271,9 @@ 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[8];
+ void *cache_ptr;
+ 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));
@@ -331,6 +333,37 @@ static void vub_device_realize(DeviceState *dev, Error **errp)
do_vhost_user_cleanup(vdev, vub);
}
+ ret = vub->vhost_dev.vhost_ops->vhost_get_shmem_config(&vub->vhost_dev,
+ &nregions,
+ memory_sizes,
+ errp);
+
+ if (ret < 0) {
+ do_vhost_user_cleanup(vdev, vub);
+ }
+
+ for (i = 0; i < nregions; i++) {
+ if (memory_sizes[i]) {
+ 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);
+ return;
+ }
+
+ cache_ptr = mmap(NULL, memory_sizes[i], PROT_NONE,
+ MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
+ if (cache_ptr == MAP_FAILED) {
+ error_setg_errno(errp, errno, "Unable to mmap blank cache");
+ return;
+ }
+
+ virtio_new_shmem_region(vdev);
+ memory_region_init_ram_ptr(vdev->shmem_list[i].mr,
+ OBJECT(vdev), "vub-shm-" + i,
+ memory_sizes[i], cache_ptr);
+ }
+ }
+
qemu_chr_fe_set_handlers(&vub->chardev, NULL, NULL, vub_event, NULL,
dev, NULL, true);
}
diff --git a/hw/virtio/vhost-user-device-pci.c b/hw/virtio/vhost-user-device-pci.c
index efaf55d3dd..abf4e90c21 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_CACHE_BAR 2
+
struct VHostUserDevicePCI {
VirtIOPCIProxy parent_obj;
VHostUserBase vub;
+ MemoryRegion cachebar;
};
#define TYPE_VHOST_USER_DEVICE_PCI "vhost-user-device-pci-base"
@@ -25,10 +29,39 @@ 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, cache_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 - cache_size) {
+ error_setg(errp, "Total shared memory required overflow");
+ return;
+ }
+ cache_size = cache_size + mr->size;
+ }
+ if (cache_size) {
+ memory_region_init(&dev->cachebar, OBJECT(vpci_dev),
+ "vhost-device-pci-cachebar", cache_size);
+ for (i = 0; i < vdev->n_shmem_regions; i++) {
+ mr = vdev->shmem_list[i].mr;
+ memory_region_add_subregion(&dev->cachebar, offset, mr);
+ virtio_pci_add_shm_cap(vpci_dev, VIRTIO_DEVICE_PCI_CACHE_BAR,
+ offset, mr->size, i);
+ offset = offset + mr->size;
+ }
+ pci_register_bar(&vpci_dev->pci_dev, VIRTIO_DEVICE_PCI_CACHE_BAR,
+ PCI_BASE_ADDRESS_SPACE_MEMORY |
+ PCI_BASE_ADDRESS_MEM_PREFETCH |
+ PCI_BASE_ADDRESS_MEM_TYPE_64,
+ &dev->cachebar);
+ }
}
static void vhost_user_device_pci_class_init(ObjectClass *klass, void *data)
--
2.45.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 4/5] vhost-user-dev: Add cache BAR
2024-09-12 14:53 ` [PATCH v3 4/5] vhost-user-dev: Add cache BAR Albert Esteve
@ 2024-09-17 8:27 ` Stefan Hajnoczi
2024-11-25 16:16 ` Albert Esteve
2024-11-26 12:51 ` Albert Esteve
2024-09-17 8:32 ` Stefan Hajnoczi
1 sibling, 2 replies; 29+ messages in thread
From: Stefan Hajnoczi @ 2024-09-17 8:27 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, Alex Bennée, slp, hi, mst, david, jasowang,
Stefano Garzarella, stevensd
[-- Attachment #1: Type: text/plain, Size: 7645 bytes --]
On Thu, Sep 12, 2024 at 04:53:34PM +0200, Albert Esteve wrote:
> Add a cache BAR in the vhost-user-device
> into which files can be directly mapped.
>
> 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>
> ---
Not all devices derive from vhost-user-base.c so this does not offer
full coverage. I think that's okay since few devices currently use
VIRTIO Shared Memory Regions. A note about this in the commit
description would be useful though. Which vhost-user devices gain VIRTIO
Shared Memory Region support and what should you do if your device is
not included in this list?
> hw/virtio/vhost-user-base.c | 37 +++++++++++++++++++++++++++--
> hw/virtio/vhost-user-device-pci.c | 39 ++++++++++++++++++++++++++++---
> 2 files changed, 71 insertions(+), 5 deletions(-)
>
> diff --git a/hw/virtio/vhost-user-base.c b/hw/virtio/vhost-user-base.c
> index 2bc3423326..f2597d021a 100644
> --- a/hw/virtio/vhost-user-base.c
> +++ b/hw/virtio/vhost-user-base.c
> @@ -271,7 +271,9 @@ 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[8];
> + void *cache_ptr;
> + 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));
> @@ -331,6 +333,37 @@ static void vub_device_realize(DeviceState *dev, Error **errp)
> do_vhost_user_cleanup(vdev, vub);
Missing return statement.
> }
>
> + ret = vub->vhost_dev.vhost_ops->vhost_get_shmem_config(&vub->vhost_dev,
> + &nregions,
> + memory_sizes,
Buffer overflow. vhost_get_shmem_config() copies out up to 256
memory_sizes[] elements. Please introduce a constant in the VIRTIO
header and use it instead of hardcoding uint64_t memory_sizes[8] above.
> + errp);
> +
> + if (ret < 0) {
> + do_vhost_user_cleanup(vdev, vub);
Missing return statement.
> + }
> +
> + for (i = 0; i < nregions; i++) {
> + if (memory_sizes[i]) {
> + 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);
> + return;
Missing do_vhost_user_cleanup().
> + }
> +
> + cache_ptr = mmap(NULL, memory_sizes[i], PROT_NONE,
> + MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> + if (cache_ptr == MAP_FAILED) {
> + error_setg_errno(errp, errno, "Unable to mmap blank cache");
> + return;
Missing do_vhost_user_cleanup().
> + }
> +
> + virtio_new_shmem_region(vdev);
> + memory_region_init_ram_ptr(vdev->shmem_list[i].mr,
> + OBJECT(vdev), "vub-shm-" + i,
> + memory_sizes[i], cache_ptr);
I think memory_region_init_ram_ptr() is included in live migration, so
the contents of VIRTIO Shared Memory Regions will be transferred to the
destination QEMU and written to the equivalent memory region there. I'm
not sure this works:
1. If there are PROT_NONE memory ranges, then live migration will
probably crash the source QEMU while trying to send this memory to
the destination QEMU.
2. If the destination vhost-user device has not yet loaded its state and
sent MAP messages setting up the VIRTIO Shared Memory Region, then
receiving migrated data and writing it into this memory will fail.
QEMU has a migration blocker API so that devices can refuse live
migration. For the time being a migration blocker is probably needed
here. See migrate_add_blocker()/migrate_del_blocker().
> + }
> + }
> +
> qemu_chr_fe_set_handlers(&vub->chardev, NULL, NULL, vub_event, NULL,
> dev, NULL, true);
> }
> diff --git a/hw/virtio/vhost-user-device-pci.c b/hw/virtio/vhost-user-device-pci.c
> index efaf55d3dd..abf4e90c21 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_CACHE_BAR 2
"Cache" is ambigous. Call it shmem_bar here and everywhere else?
> +
> struct VHostUserDevicePCI {
> VirtIOPCIProxy parent_obj;
>
> VHostUserBase vub;
> + MemoryRegion cachebar;
> };
>
> #define TYPE_VHOST_USER_DEVICE_PCI "vhost-user-device-pci-base"
> @@ -25,10 +29,39 @@ 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, cache_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 - cache_size) {
> + error_setg(errp, "Total shared memory required overflow");
> + return;
> + }
> + cache_size = cache_size + mr->size;
> + }
> + if (cache_size) {
> + memory_region_init(&dev->cachebar, OBJECT(vpci_dev),
> + "vhost-device-pci-cachebar", cache_size);
> + for (i = 0; i < vdev->n_shmem_regions; i++) {
> + mr = vdev->shmem_list[i].mr;
> + memory_region_add_subregion(&dev->cachebar, offset, mr);
> + virtio_pci_add_shm_cap(vpci_dev, VIRTIO_DEVICE_PCI_CACHE_BAR,
> + offset, mr->size, i);
> + offset = offset + mr->size;
> + }
> + pci_register_bar(&vpci_dev->pci_dev, VIRTIO_DEVICE_PCI_CACHE_BAR,
> + PCI_BASE_ADDRESS_SPACE_MEMORY |
> + PCI_BASE_ADDRESS_MEM_PREFETCH |
> + PCI_BASE_ADDRESS_MEM_TYPE_64,
> + &dev->cachebar);
> + }
> }
>
> static void vhost_user_device_pci_class_init(ObjectClass *klass, void *data)
> --
> 2.45.2
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 4/5] vhost-user-dev: Add cache BAR
2024-09-17 8:27 ` Stefan Hajnoczi
@ 2024-11-25 16:16 ` Albert Esteve
2024-11-26 7:55 ` Albert Esteve
2024-11-26 12:51 ` Albert Esteve
1 sibling, 1 reply; 29+ messages in thread
From: Albert Esteve @ 2024-11-25 16:16 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, Alex Bennée, slp, hi, mst, david, jasowang,
Stefano Garzarella, stevensd
On Tue, Sep 17, 2024 at 10:27 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Thu, Sep 12, 2024 at 04:53:34PM +0200, Albert Esteve wrote:
> > Add a cache BAR in the vhost-user-device
> > into which files can be directly mapped.
> >
> > 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>
> > ---
>
> Not all devices derive from vhost-user-base.c so this does not offer
> full coverage. I think that's okay since few devices currently use
> VIRTIO Shared Memory Regions. A note about this in the commit
> description would be useful though. Which vhost-user devices gain VIRTIO
> Shared Memory Region support and what should you do if your device is
> not included in this list?
>
> > hw/virtio/vhost-user-base.c | 37 +++++++++++++++++++++++++++--
> > hw/virtio/vhost-user-device-pci.c | 39 ++++++++++++++++++++++++++++---
> > 2 files changed, 71 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-user-base.c b/hw/virtio/vhost-user-base.c
> > index 2bc3423326..f2597d021a 100644
> > --- a/hw/virtio/vhost-user-base.c
> > +++ b/hw/virtio/vhost-user-base.c
> > @@ -271,7 +271,9 @@ 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[8];
> > + void *cache_ptr;
> > + 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));
> > @@ -331,6 +333,37 @@ static void vub_device_realize(DeviceState *dev, Error **errp)
> > do_vhost_user_cleanup(vdev, vub);
>
> Missing return statement.
True, but this is unrelated to this patchset. I will fix it in a
different patch, so that it can find its way in faster.
>
> > }
> >
> > + ret = vub->vhost_dev.vhost_ops->vhost_get_shmem_config(&vub->vhost_dev,
> > + &nregions,
> > + memory_sizes,
>
> Buffer overflow. vhost_get_shmem_config() copies out up to 256
> memory_sizes[] elements. Please introduce a constant in the VIRTIO
> header and use it instead of hardcoding uint64_t memory_sizes[8] above.
>
> > + errp);
> > +
> > + if (ret < 0) {
> > + do_vhost_user_cleanup(vdev, vub);
>
> Missing return statement.
Same here.
>
> > + }
> > +
> > + for (i = 0; i < nregions; i++) {
> > + if (memory_sizes[i]) {
> > + 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);
> > + return;
>
> Missing do_vhost_user_cleanup().
Maybe a goto would be preferable here? Just because the same exit
pattern occurs quite a few times now.
>
> > + }
> > +
> > + cache_ptr = mmap(NULL, memory_sizes[i], PROT_NONE,
> > + MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
>
>
>
> > + if (cache_ptr == MAP_FAILED) {
> > + error_setg_errno(errp, errno, "Unable to mmap blank cache");
> > + return;
>
> Missing do_vhost_user_cleanup().
>
> > + }
> > +
> > + virtio_new_shmem_region(vdev);
> > + memory_region_init_ram_ptr(vdev->shmem_list[i].mr,
> > + OBJECT(vdev), "vub-shm-" + i,
> > + memory_sizes[i], cache_ptr);
>
> I think memory_region_init_ram_ptr() is included in live migration, so
> the contents of VIRTIO Shared Memory Regions will be transferred to the
> destination QEMU and written to the equivalent memory region there. I'm
> not sure this works:
> 1. If there are PROT_NONE memory ranges, then live migration will
> probably crash the source QEMU while trying to send this memory to
> the destination QEMU.
> 2. If the destination vhost-user device has not yet loaded its state and
> sent MAP messages setting up the VIRTIO Shared Memory Region, then
> receiving migrated data and writing it into this memory will fail.
>
> QEMU has a migration blocker API so that devices can refuse live
> migration. For the time being a migration blocker is probably needed
> here. See migrate_add_blocker()/migrate_del_blocker().
>
> > + }
> > + }
> > +
> > qemu_chr_fe_set_handlers(&vub->chardev, NULL, NULL, vub_event, NULL,
> > dev, NULL, true);
> > }
> > diff --git a/hw/virtio/vhost-user-device-pci.c b/hw/virtio/vhost-user-device-pci.c
> > index efaf55d3dd..abf4e90c21 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_CACHE_BAR 2
>
> "Cache" is ambigous. Call it shmem_bar here and everywhere else?
>
> > +
> > struct VHostUserDevicePCI {
> > VirtIOPCIProxy parent_obj;
> >
> > VHostUserBase vub;
> > + MemoryRegion cachebar;
> > };
> >
> > #define TYPE_VHOST_USER_DEVICE_PCI "vhost-user-device-pci-base"
> > @@ -25,10 +29,39 @@ 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, cache_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 - cache_size) {
> > + error_setg(errp, "Total shared memory required overflow");
> > + return;
> > + }
> > + cache_size = cache_size + mr->size;
> > + }
> > + if (cache_size) {
> > + memory_region_init(&dev->cachebar, OBJECT(vpci_dev),
> > + "vhost-device-pci-cachebar", cache_size);
> > + for (i = 0; i < vdev->n_shmem_regions; i++) {
> > + mr = vdev->shmem_list[i].mr;
> > + memory_region_add_subregion(&dev->cachebar, offset, mr);
> > + virtio_pci_add_shm_cap(vpci_dev, VIRTIO_DEVICE_PCI_CACHE_BAR,
> > + offset, mr->size, i);
> > + offset = offset + mr->size;
> > + }
> > + pci_register_bar(&vpci_dev->pci_dev, VIRTIO_DEVICE_PCI_CACHE_BAR,
> > + PCI_BASE_ADDRESS_SPACE_MEMORY |
> > + PCI_BASE_ADDRESS_MEM_PREFETCH |
> > + PCI_BASE_ADDRESS_MEM_TYPE_64,
> > + &dev->cachebar);
> > + }
> > }
> >
> > static void vhost_user_device_pci_class_init(ObjectClass *klass, void *data)
> > --
> > 2.45.2
> >
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 4/5] vhost-user-dev: Add cache BAR
2024-11-25 16:16 ` Albert Esteve
@ 2024-11-26 7:55 ` Albert Esteve
0 siblings, 0 replies; 29+ messages in thread
From: Albert Esteve @ 2024-11-26 7:55 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, Alex Bennée, slp, hi, mst, david, jasowang,
Stefano Garzarella, stevensd
On Mon, Nov 25, 2024 at 5:16 PM Albert Esteve <aesteve@redhat.com> wrote:
>
> On Tue, Sep 17, 2024 at 10:27 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > On Thu, Sep 12, 2024 at 04:53:34PM +0200, Albert Esteve wrote:
> > > Add a cache BAR in the vhost-user-device
> > > into which files can be directly mapped.
> > >
> > > 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>
> > > ---
> >
> > Not all devices derive from vhost-user-base.c so this does not offer
> > full coverage. I think that's okay since few devices currently use
> > VIRTIO Shared Memory Regions. A note about this in the commit
> > description would be useful though. Which vhost-user devices gain VIRTIO
> > Shared Memory Region support and what should you do if your device is
> > not included in this list?
> >
> > > hw/virtio/vhost-user-base.c | 37 +++++++++++++++++++++++++++--
> > > hw/virtio/vhost-user-device-pci.c | 39 ++++++++++++++++++++++++++++---
> > > 2 files changed, 71 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/hw/virtio/vhost-user-base.c b/hw/virtio/vhost-user-base.c
> > > index 2bc3423326..f2597d021a 100644
> > > --- a/hw/virtio/vhost-user-base.c
> > > +++ b/hw/virtio/vhost-user-base.c
> > > @@ -271,7 +271,9 @@ 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[8];
> > > + void *cache_ptr;
> > > + 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));
> > > @@ -331,6 +333,37 @@ static void vub_device_realize(DeviceState *dev, Error **errp)
> > > do_vhost_user_cleanup(vdev, vub);
> >
> > Missing return statement.
>
> True, but this is unrelated to this patchset. I will fix it in a
> different patch, so that it can find its way in faster.
>
> >
> > > }
> > >
> > > + ret = vub->vhost_dev.vhost_ops->vhost_get_shmem_config(&vub->vhost_dev,
> > > + &nregions,
> > > + memory_sizes,
> >
> > Buffer overflow. vhost_get_shmem_config() copies out up to 256
> > memory_sizes[] elements. Please introduce a constant in the VIRTIO
> > header and use it instead of hardcoding uint64_t memory_sizes[8] above.
> >
> > > + errp);
> > > +
> > > + if (ret < 0) {
> > > + do_vhost_user_cleanup(vdev, vub);
> >
> > Missing return statement.
>
> Same here.
I'll correct myself here, this one was introduced in this patch, so is
not in mainline. Anyway, I think a goto may be a clearer pattern to
avoid missing the return statement.
> >
> > > + }
> > > +
> > > + for (i = 0; i < nregions; i++) {
> > > + if (memory_sizes[i]) {
> > > + 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);
> > > + return;
> >
> > Missing do_vhost_user_cleanup().
>
> Maybe a goto would be preferable here? Just because the same exit
> pattern occurs quite a few times now.
>
> >
> > > + }
> > > +
> > > + cache_ptr = mmap(NULL, memory_sizes[i], PROT_NONE,
> > > + MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> >
> >
> >
> > > + if (cache_ptr == MAP_FAILED) {
> > > + error_setg_errno(errp, errno, "Unable to mmap blank cache");
> > > + return;
> >
> > Missing do_vhost_user_cleanup().
> >
> > > + }
> > > +
> > > + virtio_new_shmem_region(vdev);
> > > + memory_region_init_ram_ptr(vdev->shmem_list[i].mr,
> > > + OBJECT(vdev), "vub-shm-" + i,
> > > + memory_sizes[i], cache_ptr);
> >
> > I think memory_region_init_ram_ptr() is included in live migration, so
> > the contents of VIRTIO Shared Memory Regions will be transferred to the
> > destination QEMU and written to the equivalent memory region there. I'm
> > not sure this works:
> > 1. If there are PROT_NONE memory ranges, then live migration will
> > probably crash the source QEMU while trying to send this memory to
> > the destination QEMU.
> > 2. If the destination vhost-user device has not yet loaded its state and
> > sent MAP messages setting up the VIRTIO Shared Memory Region, then
> > receiving migrated data and writing it into this memory will fail.
> >
> > QEMU has a migration blocker API so that devices can refuse live
> > migration. For the time being a migration blocker is probably needed
> > here. See migrate_add_blocker()/migrate_del_blocker().
> >
> > > + }
> > > + }
> > > +
> > > qemu_chr_fe_set_handlers(&vub->chardev, NULL, NULL, vub_event, NULL,
> > > dev, NULL, true);
> > > }
> > > diff --git a/hw/virtio/vhost-user-device-pci.c b/hw/virtio/vhost-user-device-pci.c
> > > index efaf55d3dd..abf4e90c21 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_CACHE_BAR 2
> >
> > "Cache" is ambigous. Call it shmem_bar here and everywhere else?
> >
> > > +
> > > struct VHostUserDevicePCI {
> > > VirtIOPCIProxy parent_obj;
> > >
> > > VHostUserBase vub;
> > > + MemoryRegion cachebar;
> > > };
> > >
> > > #define TYPE_VHOST_USER_DEVICE_PCI "vhost-user-device-pci-base"
> > > @@ -25,10 +29,39 @@ 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, cache_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 - cache_size) {
> > > + error_setg(errp, "Total shared memory required overflow");
> > > + return;
> > > + }
> > > + cache_size = cache_size + mr->size;
> > > + }
> > > + if (cache_size) {
> > > + memory_region_init(&dev->cachebar, OBJECT(vpci_dev),
> > > + "vhost-device-pci-cachebar", cache_size);
> > > + for (i = 0; i < vdev->n_shmem_regions; i++) {
> > > + mr = vdev->shmem_list[i].mr;
> > > + memory_region_add_subregion(&dev->cachebar, offset, mr);
> > > + virtio_pci_add_shm_cap(vpci_dev, VIRTIO_DEVICE_PCI_CACHE_BAR,
> > > + offset, mr->size, i);
> > > + offset = offset + mr->size;
> > > + }
> > > + pci_register_bar(&vpci_dev->pci_dev, VIRTIO_DEVICE_PCI_CACHE_BAR,
> > > + PCI_BASE_ADDRESS_SPACE_MEMORY |
> > > + PCI_BASE_ADDRESS_MEM_PREFETCH |
> > > + PCI_BASE_ADDRESS_MEM_TYPE_64,
> > > + &dev->cachebar);
> > > + }
> > > }
> > >
> > > static void vhost_user_device_pci_class_init(ObjectClass *klass, void *data)
> > > --
> > > 2.45.2
> > >
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 4/5] vhost-user-dev: Add cache BAR
2024-09-17 8:27 ` Stefan Hajnoczi
2024-11-25 16:16 ` Albert Esteve
@ 2024-11-26 12:51 ` Albert Esteve
1 sibling, 0 replies; 29+ messages in thread
From: Albert Esteve @ 2024-11-26 12:51 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, Alex Bennée, slp, hi, mst, david, jasowang,
Stefano Garzarella, stevensd
On Tue, Sep 17, 2024 at 10:27 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Thu, Sep 12, 2024 at 04:53:34PM +0200, Albert Esteve wrote:
> > Add a cache BAR in the vhost-user-device
> > into which files can be directly mapped.
> >
> > 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>
> > ---
>
> Not all devices derive from vhost-user-base.c so this does not offer
> full coverage. I think that's okay since few devices currently use
> VIRTIO Shared Memory Regions. A note about this in the commit
> description would be useful though. Which vhost-user devices gain VIRTIO
> Shared Memory Region support and what should you do if your device is
> not included in this list?
>
> > hw/virtio/vhost-user-base.c | 37 +++++++++++++++++++++++++++--
> > hw/virtio/vhost-user-device-pci.c | 39 ++++++++++++++++++++++++++++---
> > 2 files changed, 71 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-user-base.c b/hw/virtio/vhost-user-base.c
> > index 2bc3423326..f2597d021a 100644
> > --- a/hw/virtio/vhost-user-base.c
> > +++ b/hw/virtio/vhost-user-base.c
> > @@ -271,7 +271,9 @@ 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[8];
> > + void *cache_ptr;
> > + 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));
> > @@ -331,6 +333,37 @@ static void vub_device_realize(DeviceState *dev, Error **errp)
> > do_vhost_user_cleanup(vdev, vub);
>
> Missing return statement.
>
> > }
> >
> > + ret = vub->vhost_dev.vhost_ops->vhost_get_shmem_config(&vub->vhost_dev,
> > + &nregions,
> > + memory_sizes,
>
> Buffer overflow. vhost_get_shmem_config() copies out up to 256
> memory_sizes[] elements. Please introduce a constant in the VIRTIO
> header and use it instead of hardcoding uint64_t memory_sizes[8] above.
>
> > + errp);
> > +
> > + if (ret < 0) {
> > + do_vhost_user_cleanup(vdev, vub);
>
> Missing return statement.
>
> > + }
> > +
> > + for (i = 0; i < nregions; i++) {
> > + if (memory_sizes[i]) {
> > + 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);
> > + return;
>
> Missing do_vhost_user_cleanup().
>
> > + }
> > +
> > + cache_ptr = mmap(NULL, memory_sizes[i], PROT_NONE,
> > + MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
>
>
>
> > + if (cache_ptr == MAP_FAILED) {
> > + error_setg_errno(errp, errno, "Unable to mmap blank cache");
> > + return;
>
> Missing do_vhost_user_cleanup().
>
> > + }
> > +
> > + virtio_new_shmem_region(vdev);
> > + memory_region_init_ram_ptr(vdev->shmem_list[i].mr,
> > + OBJECT(vdev), "vub-shm-" + i,
> > + memory_sizes[i], cache_ptr);
>
> I think memory_region_init_ram_ptr() is included in live migration, so
> the contents of VIRTIO Shared Memory Regions will be transferred to the
> destination QEMU and written to the equivalent memory region there. I'm
> not sure this works:
> 1. If there are PROT_NONE memory ranges, then live migration will
> probably crash the source QEMU while trying to send this memory to
> the destination QEMU.
> 2. If the destination vhost-user device has not yet loaded its state and
> sent MAP messages setting up the VIRTIO Shared Memory Region, then
> receiving migrated data and writing it into this memory will fail.
>
> QEMU has a migration blocker API so that devices can refuse live
> migration. For the time being a migration blocker is probably needed
> here. See migrate_add_blocker()/migrate_del_blocker().
I did not even think about migration. Also, did not know about these
functions. Thanks, I have explicitly called migrate_add_blocker
from the realize function. The migrate_del_blocker will be done
implicitly within the vhost_dev_cleanup call.
>
> > + }
> > + }
> > +
> > qemu_chr_fe_set_handlers(&vub->chardev, NULL, NULL, vub_event, NULL,
> > dev, NULL, true);
> > }
> > diff --git a/hw/virtio/vhost-user-device-pci.c b/hw/virtio/vhost-user-device-pci.c
> > index efaf55d3dd..abf4e90c21 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_CACHE_BAR 2
>
> "Cache" is ambigous. Call it shmem_bar here and everywhere else?
>
> > +
> > struct VHostUserDevicePCI {
> > VirtIOPCIProxy parent_obj;
> >
> > VHostUserBase vub;
> > + MemoryRegion cachebar;
> > };
> >
> > #define TYPE_VHOST_USER_DEVICE_PCI "vhost-user-device-pci-base"
> > @@ -25,10 +29,39 @@ 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, cache_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 - cache_size) {
> > + error_setg(errp, "Total shared memory required overflow");
> > + return;
> > + }
> > + cache_size = cache_size + mr->size;
> > + }
> > + if (cache_size) {
> > + memory_region_init(&dev->cachebar, OBJECT(vpci_dev),
> > + "vhost-device-pci-cachebar", cache_size);
> > + for (i = 0; i < vdev->n_shmem_regions; i++) {
> > + mr = vdev->shmem_list[i].mr;
> > + memory_region_add_subregion(&dev->cachebar, offset, mr);
> > + virtio_pci_add_shm_cap(vpci_dev, VIRTIO_DEVICE_PCI_CACHE_BAR,
> > + offset, mr->size, i);
> > + offset = offset + mr->size;
> > + }
> > + pci_register_bar(&vpci_dev->pci_dev, VIRTIO_DEVICE_PCI_CACHE_BAR,
> > + PCI_BASE_ADDRESS_SPACE_MEMORY |
> > + PCI_BASE_ADDRESS_MEM_PREFETCH |
> > + PCI_BASE_ADDRESS_MEM_TYPE_64,
> > + &dev->cachebar);
> > + }
> > }
> >
> > static void vhost_user_device_pci_class_init(ObjectClass *klass, void *data)
> > --
> > 2.45.2
> >
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 4/5] vhost-user-dev: Add cache BAR
2024-09-12 14:53 ` [PATCH v3 4/5] vhost-user-dev: Add cache BAR Albert Esteve
2024-09-17 8:27 ` Stefan Hajnoczi
@ 2024-09-17 8:32 ` Stefan Hajnoczi
2024-09-17 8:36 ` Stefan Hajnoczi
1 sibling, 1 reply; 29+ messages in thread
From: Stefan Hajnoczi @ 2024-09-17 8:32 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, Alex Bennée, slp, hi, mst, david, jasowang,
Stefano Garzarella, stevensd
[-- Attachment #1: Type: text/plain, Size: 1537 bytes --]
On Thu, Sep 12, 2024 at 04:53:34PM +0200, Albert Esteve wrote:
> @@ -331,6 +333,37 @@ static void vub_device_realize(DeviceState *dev, Error **errp)
> do_vhost_user_cleanup(vdev, vub);
> }
>
> + ret = vub->vhost_dev.vhost_ops->vhost_get_shmem_config(&vub->vhost_dev,
> + &nregions,
> + memory_sizes,
> + errp);
> +
> + if (ret < 0) {
> + do_vhost_user_cleanup(vdev, vub);
> + }
> +
> + for (i = 0; i < nregions; i++) {
> + if (memory_sizes[i]) {
> + 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);
> + return;
> + }
> +
> + cache_ptr = mmap(NULL, memory_sizes[i], PROT_NONE,
> + MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> + if (cache_ptr == MAP_FAILED) {
> + error_setg_errno(errp, errno, "Unable to mmap blank cache");
> + return;
> + }
> +
> + virtio_new_shmem_region(vdev);
> + memory_region_init_ram_ptr(vdev->shmem_list[i].mr,
I don't think this works because virtio_new_shmem_region() leaves .mr =
NULL? Why allocates the MemoryRegion and assigns it to shmem_list[i].mr?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 4/5] vhost-user-dev: Add cache BAR
2024-09-17 8:32 ` Stefan Hajnoczi
@ 2024-09-17 8:36 ` Stefan Hajnoczi
0 siblings, 0 replies; 29+ messages in thread
From: Stefan Hajnoczi @ 2024-09-17 8:36 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Albert Esteve, qemu-devel, Alex Bennée, slp, hi, mst, david,
jasowang, Stefano Garzarella, stevensd
On Tue, 17 Sept 2024 at 10:33, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Thu, Sep 12, 2024 at 04:53:34PM +0200, Albert Esteve wrote:
> > @@ -331,6 +333,37 @@ static void vub_device_realize(DeviceState *dev, Error **errp)
> > do_vhost_user_cleanup(vdev, vub);
> > }
> >
> > + ret = vub->vhost_dev.vhost_ops->vhost_get_shmem_config(&vub->vhost_dev,
> > + &nregions,
> > + memory_sizes,
> > + errp);
> > +
> > + if (ret < 0) {
> > + do_vhost_user_cleanup(vdev, vub);
> > + }
> > +
> > + for (i = 0; i < nregions; i++) {
> > + if (memory_sizes[i]) {
> > + 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);
> > + return;
> > + }
> > +
> > + cache_ptr = mmap(NULL, memory_sizes[i], PROT_NONE,
> > + MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> > + if (cache_ptr == MAP_FAILED) {
> > + error_setg_errno(errp, errno, "Unable to mmap blank cache");
> > + return;
> > + }
> > +
> > + virtio_new_shmem_region(vdev);
> > + memory_region_init_ram_ptr(vdev->shmem_list[i].mr,
>
> I don't think this works because virtio_new_shmem_region() leaves .mr =
> NULL? Why allocates the MemoryRegion and assigns it to shmem_list[i].mr?
"Why" -> "Who"
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 5/5] vhost_user: Add MEM_READ/WRITE backend requests
2024-09-12 14:53 [PATCH v3 0/5] vhost-user: Add SHMEM_MAP/UNMAP requests Albert Esteve
` (3 preceding siblings ...)
2024-09-12 14:53 ` [PATCH v3 4/5] vhost-user-dev: Add cache BAR Albert Esteve
@ 2024-09-12 14:53 ` Albert Esteve
2024-09-12 14:59 ` [PATCH v3 0/5] vhost-user: Add SHMEM_MAP/UNMAP requests Albert Esteve
2024-09-16 17:57 ` Stefan Hajnoczi
6 siblings, 0 replies; 29+ messages in thread
From: Albert Esteve @ 2024-09-12 14:53 UTC (permalink / raw)
To: qemu-devel
Cc: Alex Bennée, slp, hi, mst, david, jasowang, stefanha,
Stefano Garzarella, stevensd, 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 | 84 +++++++++++++++++++
subprojects/libvhost-user/libvhost-user.h | 38 +++++++++
3 files changed, 197 insertions(+), 24 deletions(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 83f5c02bea..8a4718ae10 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -120,6 +120,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;
@@ -147,6 +149,12 @@ typedef struct VhostUserShMemConfig {
uint64_t memory_sizes[VHOST_MEMORY_BASELINE_NREGIONS];
} 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;
@@ -255,6 +263,7 @@ typedef union {
VhostUserTransferDeviceState transfer_state;
VhostUserMMap mmap;
VhostUserShMemConfig shmem;
+ VhostUserMemRWMsg mem_rw;
} VhostUserPayload;
typedef struct VhostUserMsg {
@@ -343,17 +352,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;
@@ -1885,6 +1900,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);
@@ -1900,7 +1937,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;
@@ -1919,46 +1956,59 @@ 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, &payload.mmap,
+ ret = vhost_user_backend_handle_shmem_map(dev, &payload->mmap,
fd ? fd[0] : -1);
break;
case VHOST_USER_BACKEND_SHMEM_UNMAP:
- ret = vhost_user_backend_handle_shmem_unmap(dev, &payload.mmap);
+ ret = vhost_user_backend_handle_shmem_unmap(dev, &payload->mmap);
+ 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);
@@ -1970,10 +2020,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;
}
@@ -1991,6 +2041,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 496268e12b..08de54896b 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -1652,6 +1652,90 @@ 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,
+ }
+ }
+ };
+
+ 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;
+ }
+
+ data = malloc(msg_reply.payload.mem_rw.size);
+ if (!data) {
+ DPRINT("Failed to malloc read memory data");
+ goto out_err;
+ }
+
+ 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,
+ }
+ }
+ };
+ memcpy(msg.payload.mem_rw.data, data, size);
+
+ 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 ea4902e876..8ec49dcb1b 100644
--- a/subprojects/libvhost-user/libvhost-user.h
+++ b/subprojects/libvhost-user/libvhost-user.h
@@ -129,6 +129,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;
@@ -152,6 +154,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;
@@ -235,6 +243,7 @@ typedef struct VhostUserMsg {
VhostUserInflight inflight;
VhostUserShared object;
VhostUserMMap mmap;
+ VhostUserMemRWMsg mem_rw;
} payload;
int fds[VHOST_MEMORY_BASELINE_NREGIONS];
@@ -649,6 +658,35 @@ 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:
* @dev: a VuDev context
--
2.45.2
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v3 0/5] vhost-user: Add SHMEM_MAP/UNMAP requests
2024-09-12 14:53 [PATCH v3 0/5] vhost-user: Add SHMEM_MAP/UNMAP requests Albert Esteve
` (4 preceding siblings ...)
2024-09-12 14:53 ` [PATCH v3 5/5] vhost_user: Add MEM_READ/WRITE backend requests Albert Esteve
@ 2024-09-12 14:59 ` Albert Esteve
2024-09-16 17:57 ` Stefan Hajnoczi
6 siblings, 0 replies; 29+ messages in thread
From: Albert Esteve @ 2024-09-12 14:59 UTC (permalink / raw)
To: qemu-devel
Cc: Alex Bennée, slp, hi, mst, david, jasowang, stefanha,
Stefano Garzarella, stevensd
[-- Attachment #1: Type: text/plain, Size: 2908 bytes --]
Link to the documentation:
https://lore.kernel.org/all/20240912144432.126717-1-aesteve@redhat.com/T/#t
On Thu, Sep 12, 2024 at 4:53 PM Albert Esteve <aesteve@redhat.com> wrote:
> Hi all,
>
> v2->v3:
> - Add track for mapped memory in VIRTIO
> Shared memory regions, so that boundaries
> can be verified when a request for
> new mmap is received
> - Use address_space_read/write() for
> MEM_READ/_WRITE handling methods.
> - Improve/fix support for flexible
> array members for MEM_READ/_WRITE requests.
> - Split documentation into a separate patch.
> - Various small fixes from previous review.
>
> The usecase for this patch is, e.g., to support
> vhost-user-gpu RESOURCE_BLOB operations,
> or DAX Window request for virtio-fs. In
> general, any operation where a backend
> need to request the frontend to mmap an
> fd into a VIRTIO Shared Memory Region,
> so that the guest can then access it.
>
> After receiving the SHMEM_MAP/UNMAP request,
> the frontend will perform the mmap with the
> instructed parameters (i.e., shmid, shm_offset,
> fd_offset, fd, lenght).
>
> As there are already a couple devices
> that could benefit of such a feature,
> and more could require it in the future,
> the goal is to make the implementation
> generic.
>
> To that end, the VIRTIO Shared Memory
> Region list is declared in the `VirtIODevice`
> struct.
>
> 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).
>
> Finally, MEM_READ/WRITE backend requests are
> added to deal with a potential issue when having
> any backend sharing a descriptor that references
> a mapping to another backend. The first
> backend will not be able to see these
> mappings. So these requests are a fallback
> for vhost-user memory translation fails.
>
> Albert Esteve (5):
> vhost-user: Add VIRTIO Shared Memory map request
> virtio: Track shared memory mappings
> vhost_user: Add frontend command for shmem config
> vhost-user-dev: Add cache BAR
> vhost_user: Add MEM_READ/WRITE backend requests
>
> hw/virtio/vhost-user-base.c | 37 ++-
> hw/virtio/vhost-user-device-pci.c | 39 +++-
> hw/virtio/vhost-user.c | 273 ++++++++++++++++++++--
> hw/virtio/virtio.c | 59 +++++
> include/hw/virtio/vhost-backend.h | 6 +
> include/hw/virtio/vhost-user.h | 1 +
> include/hw/virtio/virtio.h | 26 +++
> subprojects/libvhost-user/libvhost-user.c | 144 ++++++++++++
> subprojects/libvhost-user/libvhost-user.h | 90 +++++++
> 9 files changed, 648 insertions(+), 27 deletions(-)
>
> --
> 2.45.2
>
>
[-- Attachment #2: Type: text/html, Size: 3515 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 0/5] vhost-user: Add SHMEM_MAP/UNMAP requests
2024-09-12 14:53 [PATCH v3 0/5] vhost-user: Add SHMEM_MAP/UNMAP requests Albert Esteve
` (5 preceding siblings ...)
2024-09-12 14:59 ` [PATCH v3 0/5] vhost-user: Add SHMEM_MAP/UNMAP requests Albert Esteve
@ 2024-09-16 17:57 ` Stefan Hajnoczi
2024-09-17 7:05 ` Albert Esteve
6 siblings, 1 reply; 29+ messages in thread
From: Stefan Hajnoczi @ 2024-09-16 17:57 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, Alex Bennée, slp, hi, mst, david, jasowang,
Stefano Garzarella, stevensd
[-- Attachment #1: Type: text/plain, Size: 575 bytes --]
This patch series could use tests. The first two patches seem broken and
testing would have revealed that the memory allocation and pointers are
not quite right.
One testing approach is to write a test device using libvhost-user that
exposes VIRTIO Shared Memory Regions, launch QEMU in qtest mode with
--device vhost-user-device, and then use the qtest API to enumerate and
access the VIRTIO Shared Memory Regions. Unfortunately this involves
writing quite a bit of test code. I can explain it in more detail if you
want.
Does anyone have other ideas for testing?
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v3 0/5] vhost-user: Add SHMEM_MAP/UNMAP requests
2024-09-16 17:57 ` Stefan Hajnoczi
@ 2024-09-17 7:05 ` Albert Esteve
2024-09-17 7:43 ` Stefan Hajnoczi
0 siblings, 1 reply; 29+ messages in thread
From: Albert Esteve @ 2024-09-17 7:05 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, Alex Bennée, slp, hi, mst, david, jasowang,
Stefano Garzarella, stevensd
[-- Attachment #1: Type: text/plain, Size: 1140 bytes --]
On Mon, Sep 16, 2024 at 7:57 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> This patch series could use tests. The first two patches seem broken and
> testing would have revealed that the memory allocation and pointers are
> not quite right.
>
My bad. Previous version of the patch I did test with a device that I've
been working on that utilizes the map/unmap messages. But I skipped it
for this one. I will test it for any coming versions.
>
> One testing approach is to write a test device using libvhost-user that
> exposes VIRTIO Shared Memory Regions, launch QEMU in qtest mode with
> --device vhost-user-device, and then use the qtest API to enumerate and
> access the VIRTIO Shared Memory Regions. Unfortunately this involves
> writing quite a bit of test code. I can explain it in more detail if you
> want.
>
If we want to have tests covering the feature within qemu, I can try
to do this. I'm also more comfortable if there are tests in place.
As I mentioned, before this patch I was verifying with an
external device myself.
>
> Does anyone have other ideas for testing?
>
> Stefan
>
[-- Attachment #2: Type: text/html, Size: 1927 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 0/5] vhost-user: Add SHMEM_MAP/UNMAP requests
2024-09-17 7:05 ` Albert Esteve
@ 2024-09-17 7:43 ` Stefan Hajnoczi
0 siblings, 0 replies; 29+ messages in thread
From: Stefan Hajnoczi @ 2024-09-17 7:43 UTC (permalink / raw)
To: Albert Esteve
Cc: qemu-devel, Alex Bennée, slp, hi, mst, david, jasowang,
Stefano Garzarella, stevensd
[-- Attachment #1: Type: text/plain, Size: 2671 bytes --]
On Tue, Sep 17, 2024 at 09:05:34AM +0200, Albert Esteve wrote:
> On Mon, Sep 16, 2024 at 7:57 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> > This patch series could use tests. The first two patches seem broken and
> > testing would have revealed that the memory allocation and pointers are
> > not quite right.
> >
>
> My bad. Previous version of the patch I did test with a device that I've
> been working on that utilizes the map/unmap messages. But I skipped it
> for this one. I will test it for any coming versions.
>
>
> >
> > One testing approach is to write a test device using libvhost-user that
> > exposes VIRTIO Shared Memory Regions, launch QEMU in qtest mode with
> > --device vhost-user-device, and then use the qtest API to enumerate and
> > access the VIRTIO Shared Memory Regions. Unfortunately this involves
> > writing quite a bit of test code. I can explain it in more detail if you
> > want.
> >
>
> If we want to have tests covering the feature within qemu, I can try
> to do this. I'm also more comfortable if there are tests in place.
> As I mentioned, before this patch I was verifying with an
> external device myself.
Good, automated tests will continue to be protected by tests after it is
merged.
QEMU's qtest framework (tests/qtest/) launches QEMU in the special qtest
mode where the guest does not execute CPU instructions. The test case
can send commands like reading and writing guest RAM and hardware
registers so it can remote-control QEMU as if it were a running guest.
https://www.qemu.org/docs/master/devel/testing/qtest.html
qtest is low-level but there are VIRTIO qtest APIs that offer something
similar to a VIRTIO driver API. You could extend that API to support
VIRTIO Shared Memory Regions over virtio-pci
(tests/qtest/libqos/virtio-pci.c).
A vhost-user device is also required. You could implement a dummy device
with libvhost-user that has a few VIRTIO Shared Memory Regions and
nothing else (no virtqueues, etc). The dummy device would create a
shared memory file descriptor and send the SHMEM_MAP message.
Then a qtest test case can be written that launches the dummy vhost-user
device and QEMU with --device vhost-user-device. Using the qtest VIRTIO
API you can initialize the device, enumerate VIRTIO Shared Memory
Regions (using the new qtest API you added), and test that
loading/storing to the VIRTIO Shared Memory Region works.
It would also be possible to test more advanced cases like 256 VIRTIO
Shared Memory Regions, skipping regions with 0 size, MAP/UNMAP sequences
including rejecting partial UNMAP and overlapping MAP, etc.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread