On Mon, Feb 24, 2025 at 10:49 AM David Hildenbrand <da...@redhat.com> wrote: > > On 24.02.25 10:35, Albert Esteve wrote: > > 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. > > Yes, all vhost-user devices should be updated.
Then, I think I agree with you, it would seem that this approach naturally solved the issue with address translation among different devices, as they all get the most up-to-date memory table after each mmap. WDYT, @Stefan Hajnoczi ? If we are unsure, maybe we can leave the MEM_READ/WRITE support as a later extension, and try to integrate the rest of this patch first. > > -- > Cheers, > > David / dhildenb >
