On Mon, Feb 24, 2025 at 10:16 AM David Hildenbrand <da...@redhat.com> wrote: > > On 24.02.25 09:54, Albert Esteve wrote: > > On Mon, Feb 17, 2025 at 9:01 PM David Hildenbrand <da...@redhat.com> wrote: > >> > >> On 17.02.25 17:40, Albert Esteve wrote: > >>> Hi all, > >>> > >> > >> Hi, > >> > >> looks like our debugging session was successfu :) > >> > >> One question below. > >> > >>> v3->v4 > >>> - Change mmap strategy to use RAM blocks > >>> and subregions. > >>> - Add new bitfield to qmp feature map > >>> - Followed most review comments from > >>> last iteration. > >>> - Merged documentation patch again with > >>> this one. Makes more sense to > >>> review them together after all. > >>> - Add documentation for MEM_READ/WRITE > >>> messages. > >>> > >>> The goal of this patch is to support > >>> dynamic fd-backed memory maps initiated > >>> from vhost-user backends. > >>> There are many devices that could already > >>> benefit of this feature, e.g., > >>> virtiofs or virtio-gpu. > >>> > >>> After receiving the SHMEM_MAP/UNMAP request, > >>> the frontend creates the RAMBlock form the > >>> fd and maps it by adding it as a subregion > >>> of the shared memory region container. > >>> > >>> The VIRTIO Shared Memory Region list is > >>> declared in the `VirtIODevice` struct > >>> to make it generic. > >>> > >>> TODO: There was a conversation on the > >>> previous version around adding tests > >>> to the patch (which I have acknowledged). > >>> However, given the numerous changes > >>> that the patch already has, I have > >>> decided to send it early and collect > >>> some feedback while I work on the > >>> tests for the next iteration. > >>> Given that I have been able to > >>> test the implementation with > >>> my local setup, I am more or less > >>> confident that, at least, the code > >>> is in a relatively sane state > >>> so that no reviewing time is > >>> wasted on broken patches. > >>> > >>> This patch also includes: > >>> - SHMEM_CONFIG frontend request that is > >>> specifically meant to allow generic > >>> vhost-user-device frontend to be able to > >>> query VIRTIO Shared Memory settings from the > >>> backend (as this device is generic and agnostic > >>> of the actual backend configuration). > >>> > >>> - MEM_READ/WRITE backend requests are > >>> added to deal with a potential issue when having > >>> multiple backends sharing a file descriptor. > >>> When a backend calls SHMEM_MAP it makes > >>> accessing to the region fail for other > >>> backend as it is missing from their translation > >>> table. So these requests are a fallback > >>> for vhost-user memory translation fails. > >> > >> Can you elaborate what the issue here is? > >> > >> Why would SHMEM_MAP make accessing the region fail for other backends -- > >> what makes this missing from their translation? > > > > This issue was raised by Stefan Hajnoczi in one of the first > > iterations of this patchset, based upon previous David Gilbert's work > > on the virtiofs DAX Window. > > > > Let me paste here some of his remarks: > > > > """ > > Other backends don't see these mappings. If the guest submits a vring > > descriptor referencing a mapping to another backend, then that backend > > won't be able to access this memory. > > """ > > [...] > > """ > > A bit more detail: > > > > Device A has a VIRTIO Shared Memory Region. An application mmaps that > > memory (examples: guest userspace driver using Linux VFIO, a guest > > kernel driver that exposes the memory to userspace via mmap, or guest > > kernel DAX). The application passes that memory as an I/O buffer to > > device B (e.g. O_DIRECT disk I/O). > > > > The result is that device B's vhost-user backend receives a vring > > descriptor that points to a guest memory address in device A's VIRTIO > > Shared Memory Region. Since device B does not have this memory in its > > table, it cannot translate the address and the device breaks. > > """ > > > > I have not triggered the issue myself. So the idea is that the next > > patch will *definitively* include some testing for the commits that I > > cannot verify with my local setup. > > Hah! But isn't that exact problem which is now solved by our rework? > > Whatever is mapped in the VIRTIO Shared Memory Region will be > communicated to all other vhost-user devices. So they should have that > memory in their map and should be able to access it.
You mean the SET_MEM_TABLE message after the vhost_commit is sent to all vhost-user devices? I was not sure, as I was testing with a single device, that would be great, and simplify the patch a lot. > > The only thing vhost-user devices cannot access are IIRC ram_device_ptr > memory regions (e.g., from vfio devices). But that is independent shared > memory regions. > > -- > Cheers, > > David / dhildenb >
