Re: [PATCH v3 1/5] vhost-user: Add VIRTIO Shared Memory map request

[David Hildenbrand]

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 <aest...@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