Re: [RFC PATCH v2 0/5] vhost-user: Add SHMEM_MAP/UNMAP requests

[Stefan Hajnoczi]

On Fri, Jun 28, 2024 at 04:57:05PM +0200, Albert Esteve wrote:
> Hi all,
> 
> v1->v2:
> - Corrected typos and clarifications from
>   first review
> - Added SHMEM_CONFIG frontend request to
>   query VIRTIO shared memory regions from
>   backends
> - vhost-user-device to use SHMEM_CONFIG
>   to request and initialise regions
> - Added MEM_READ/WRITE backend requests
>   in case address translation fails
>   accessing VIRTIO Shared Memory Regions
>   with MMAPs

Hi Albert,
I will be offline next week. I've posted comments.

I think the hard part will be adjusting vhost-user backend code to make
use of MEM_READ/MEM_WRITE when address translation fails. Ideally every
guest memory access (including vring accesses) should fall back to
MEM_READ/MEM_WRITE.

A good test for MEM_READ/MEM_WRITE is to completely skip setting the
memory table from the frontend and fall back for every guest memory
access. If the vhost-user backend still works without a memory table
then you know MEM_READ/MEM_WRITE is working too.

The vhost-user spec should probably contain a comment explaining that
MEM_READ/MEM_WRITE may be necessary when other device backends use
SHMEM_MAP due to the incomplete memory table that prevents translating
those memory addresses. In other words, if the guest has a device that
uses SHMEM_MAP, then all other vhost-user devices should support
MEM_READ/MEM_WRITE in order to ensure that DMA works with Shared Memory
Regions.

Stefan

> 
> This is an update of my attempt to have
> backends support dynamic fd mapping into VIRTIO
> Shared Memory Regions. After the first review
> I have added more commits and new messages
> to the vhost-user protocol.
> However, I still have some doubts as to
> how will this work, specially regarding
> the MEM_READ and MEM_WRITE commands.
> Thus, I am still looking for feedback,
> to ensure that I am going in the right
> direction with the implementation.
> 
> The usecase for this patch is, e.g., to support
> vhost-user-gpu RESOURCE_BLOB operations,
> or DAX Window request for virtio-fs. In
> general, any operation where a backend
> need to request the frontend to mmap an
> fd into a VIRTIO Shared Memory Region,
> so that the guest can then access it.
> 
> After receiving the SHMEM_MAP/UNMAP request,
> the frontend will perform the mmap with the
> instructed parameters (i.e., shmid, shm_offset,
> fd_offset, fd, lenght).
> 
> As there are already a couple devices
> that could benefit of such a feature,
> and more could require it in the future,
> the goal is to make the implementation
> generic.
> 
> To that end, the VIRTIO Shared Memory
> Region list is declared in the `VirtIODevice`
> struct.
> 
> 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).
> 
> Finally, MEM_READ/WRITE backend requests are
> added to deal with a potential issue when having
> any backend sharing a descriptor that references
> a mapping to another backend. The first
> backend will not be able to see these
> mappings. So these requests are a fallback
> for vhost-user memory translation fails.
> 
> Albert Esteve (5):
>   vhost-user: Add VIRTIO Shared Memory map request
>   vhost_user: Add frontend command for shmem config
>   vhost-user-dev: Add cache BAR
>   vhost_user: Add MEM_READ/WRITE backend requests
>   vhost_user: Implement mem_read/mem_write handlers
> 
>  docs/interop/vhost-user.rst               |  58 ++++++
>  hw/virtio/vhost-user-base.c               |  39 +++-
>  hw/virtio/vhost-user-device-pci.c         |  37 +++-
>  hw/virtio/vhost-user.c                    | 221 ++++++++++++++++++++++
>  hw/virtio/virtio.c                        |  12 ++
>  include/hw/virtio/vhost-backend.h         |   6 +
>  include/hw/virtio/vhost-user.h            |   1 +
>  include/hw/virtio/virtio.h                |   5 +
>  subprojects/libvhost-user/libvhost-user.c | 149 +++++++++++++++
>  subprojects/libvhost-user/libvhost-user.h |  91 +++++++++
>  10 files changed, 614 insertions(+), 5 deletions(-)
> 
> -- 
> 2.45.2
>

signature.asc
Description: PGP signature