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

[David Hildenbrand <david@redhat.com>]

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