Re: [PATCH v10 1/7] vhost-user: Add VirtIO Shared Memory map request

[Albert Esteve <aesteve@redhat.com>]

On Mon, Oct 20, 2025 at 3:50 PM David Hildenbrand <david@redhat.com> wrote:
>
> > + * Returns: 0 on success, negative errno on failure
> > + */
> > +static int
> > +vhost_user_backend_handle_shmem_map(struct vhost_dev *dev,
> > +                                    QIOChannel *ioc,
> > +                                    VhostUserHeader *hdr,
> > +                                    VhostUserPayload *payload,
> > +                                    int fd)
> > +{
> > +    VirtioSharedMemory *shmem;
> > +    VhostUserMMap *vu_mmap = &payload->mmap;
> > +    VirtioSharedMemoryMapping *existing;
> > +    Error *local_err = NULL;
> > +    int ret = 0;
> > +
> > +    if (fd < 0) {
> > +        error_report("Bad fd for map");
> > +        ret = -EBADF;
> > +        goto send_reply;
> > +    }
> > +
> > +    if (QSIMPLEQ_EMPTY(&dev->vdev->shmem_list)) {
> > +        error_report("Device has no VIRTIO Shared Memory Regions. "
> > +                     "Requested ID: %d", vu_mmap->shmid);
> > +        ret = -EFAULT;
> > +        goto send_reply;
> > +    }
> > +
> > +    shmem = virtio_find_shmem_region(dev->vdev, vu_mmap->shmid);
> > +    if (!shmem) {
> > +        error_report("VIRTIO Shared Memory Region at "
> > +                     "ID %d not found or uninitialized", vu_mmap->shmid);
> > +        ret = -EFAULT;
> > +        goto send_reply;
> > +    }
> > +
> > +    if ((vu_mmap->shm_offset + vu_mmap->len) < vu_mmap->len ||
> > +        (vu_mmap->shm_offset + vu_mmap->len) > shmem->mr.size) {
> > +        error_report("Bad offset/len for mmap %" PRIx64 "+%" PRIx64,
> > +                     vu_mmap->shm_offset, vu_mmap->len);
> > +        ret = -EFAULT;
> > +        goto send_reply;
> > +    }
> > +
> > +    QTAILQ_FOREACH(existing, &shmem->mmaps, link) {
> > +        if (ranges_overlap(existing->offset, existing->len,
> > +                           vu_mmap->shm_offset, vu_mmap->len)) {
> > +            error_report("VIRTIO Shared Memory mapping overlap");
> > +            ret = -EFAULT;
> > +            goto send_reply;
> > +        }
> > +    }
> > +
> > +    memory_region_transaction_begin();
>
> My only comment would be whether the
> memory_region_transaction_begin()/memory_region_transaction_commit()
> should be hidden behind some
> virtio_add_shmem_map_start()/virtio_add_shmem_map_end() helpers.
>
> Talking about memory regions in this function sounds odd given that it's
> more an implementation detail hidden by other helpers.
>
> Then, we can also document why these functions exists, and what the
> contract is for calling them.

I understand. I will send a follow up patch with this, and we can
discuss the solution there. Thanks for giving it another spin!

>
> --
> Cheers
>
> David / dhildenb
>