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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6280 bytes --]

On Tue, Sep 03, 2024 at 10:42:34AM +0200, Albert Esteve wrote:
> Hello all,
> 
> Sorry, I have been a bit disconnected from this thread as I was on
> vacations and then had to switch tasks for a while.
> 
> I will try to go through all comments and address them for the first
> non-RFC drop of this patch series.
> 
> But I was discussing with some colleagues on this. So turns out rust-vmm's
> vhost-user-gpu will potentially use
> this soon, and a rust-vmm/vhost patch have been already posted:
> https://github.com/rust-vmm/vhost/pull/251.
> So I think it may make sense to:
> 1. Split the vhost-user documentation patch once settled. Since it is taken
> as the official spec,
>     having it upstreamed independently of the implementation will benefit
> other projects to
>     work/integrate their own code.
> 2. Split READ_/WRITE_MEM messages from SHMEM_MAP/_UNMAP patches.
>     If I remember correctly, this addresses a virtio-fs specific issue,
> that will not
>     impact either virtio-gpu nor virtio-media, or any other.

This is an architectural issue that arises from exposing VIRTIO Shared
Memory Regions in vhost-user. It was first seen with Linux virtiofs but
it could happen with other devices and/or guest operating systems.

Any VIRTIO Shared Memory Region that can be mmapped into Linux userspace
may trigger this issue. Userspace may write(2) to an O_DIRECT file with
the mmap as the source. The vhost-user-blk device will not be able to
access the source device's VIRTIO Shared Memory Region and will fail.

> So it may make
> sense
>     to separate them so that one does not stall the other. I will try to
> have both
>     integrated in the mid term.

If READ_/WRITE_MEM is a pain to implement (I think it is in the
vhost-user back-end, even though I've been a proponent of it), then
another way to deal with this issue is to specify that upon receiving
MAP/UNMAP messages, the vhost-user front-end must update the vhost-user
memory tables of all other vhost-user devices. That way vhost-user
devices will be able to access VIRTIO Shared Memory Regions mapped by
other devices.

Implementing this in QEMU should be much easier than implementing
READ_/WRITE_MEM support in device back-ends.

This will be slow and scale poorly but performance is only a problem for
devices that frequently MAP/UNMAP like virtiofs. Will virtio-gpu and
virtio-media use MAP/UNMAP often at runtime? They might be able to get
away with this simple solution.

I'd be happy with that. If someone wants to make virtiofs DAX faster,
they can implement READ/WRITE_MEM or another solution later, but let's
at least make things correct from the start.

Stefan

> 
> WDYT?
> 
> BR,
> Albert.
> 
> On Tue, Jul 16, 2024 at 3:21 AM David Stevens <stevensd@chromium.org> wrote:
> 
> > On Fri, Jul 12, 2024 at 2:47 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Fri, Jul 12, 2024 at 11:06:49AM +0900, David Stevens wrote:
> > > > On Thu, Jul 11, 2024 at 7:56 PM Alyssa Ross <hi@alyssa.is> wrote:
> > > > >
> > > > > Adding David Stevens, who implemented SHMEM_MAP and SHMEM_UNMAP in
> > > > > crosvm a couple of years ago.
> > > > >
> > > > > David, I'd be particularly interested for your thoughts on the
> > MEM_READ
> > > > > and MEM_WRITE commands, since as far as I know crosvm doesn't
> > implement
> > > > > anything like that.  The discussion leading to those being added
> > starts
> > > > > here:
> > > > >
> > > > >
> > https://lore.kernel.org/qemu-devel/20240604185416.GB90471@fedora.redhat.com/
> > > > >
> > > > > It would be great if this could be standardised between QEMU and
> > crosvm
> > > > > (and therefore have a clearer path toward being implemented in other
> > VMMs)!
> > > >
> > > > Setting aside vhost-user for a moment, the DAX example given by Stefan
> > > > won't work in crosvm today.
> > > >
> > > > Is universal access to virtio shared memory regions actually mandated
> > > > by the virtio spec? Copying from virtiofs DAX to virtiofs sharing
> > > > seems reasonable enough, but what about virtio-pmem to virtio-blk?
> > > > What about screenshotting a framebuffer in virtio-gpu shared memory to
> > > > virtio-scsi? I guess with some plumbing in the VMM, it's solvable in a
> > > > virtualized environment. But what about when you have real hardware
> > > > that speaks virtio involved? That's outside my wheelhouse, but it
> > > > doesn't seem like that would be easy to solve.
> > >
> > > Yes, it can work for physical devices if allowed by host configuration.
> > > E.g. VFIO supports that I think. Don't think VDPA does.
> >
> > I'm sure it can work, but that sounds more like a SHOULD (MAY?),
> > rather than a MUST.
> >
> > > > For what it's worth, my interpretation of the target scenario:
> > > >
> > > > > 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
> > > >
> > > > is that it's omitting how the implementation is reconciled with
> > > > section 2.10.1 of v1.3 of the virtio spec, which states that:
> > > >
> > > > > References into shared memory regions are represented as offsets from
> > > > > the beginning of the region instead of absolute memory addresses.
> > Offsets
> > > > > are used both for references between structures stored within shared
> > > > > memory and for requests placed in virtqueues that refer to shared
> > memory.
> > > >
> > > > My interpretation of that statement is that putting raw guest physical
> > > > addresses corresponding to virtio shared memory regions into a vring
> > > > is a driver spec violation.
> > > >
> > > > -David
> > >
> > > This really applies within device I think. Should be clarified ...
> >
> > You mean that a virtio device can use absolute memory addresses for
> > other devices' shared memory regions, but it can't use absolute memory
> > addresses for its own shared memory regions? That's a rather strange
> > requirement. Or is the statement simply giving an addressing strategy
> > that device type specifications are free to ignore?
> >
> > -David
> >
> >

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]