On Tue, Sep 17, 2024 at 12:08 PM David Hildenbrand <da...@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 <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.
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
>
