Re: [PATCH v4 0/9] vhost-user: Add SHMEM_MAP/UNMAP requests

[David Hildenbrand <david@redhat.com>]

On 24.02.25 10:35, Albert Esteve wrote:
> On Mon, Feb 24, 2025 at 10:16 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 24.02.25 09:54, Albert Esteve wrote:
>>> On Mon, Feb 17, 2025 at 9:01 PM David Hildenbrand <david@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.

-- 
Cheers,

David / dhildenb