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.2On 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
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
>
>> 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
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 >
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
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
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 >
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
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
>
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
> >
© 2016 - 2026 Red Hat, Inc.