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

[Albert Esteve <aesteve@redhat.com>]

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

On Thu, Sep 5, 2024 at 6:39 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:

> 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.
>

I agree. I want it to be correct first. If you agree on splitting the spec
bits from this
patch I'm already happy. I suggested splitting READ_/WRITE_MEM messages
because I thought that it was a virtiofs-specific issue.

The alternative that you proposed is interesting. I'll take it into
account. But I
feel I prefer to go for the better solution, and if I get too entangled,
then switch
to the easier implementation.

I think we could do this in 2 patches:
1. Split the documentation bits for SHMEM_MAP/_UNMAP. The
    implementation for these messages will go into the second patch.
2. The implementation patch: keep going for the time being with
     READ_/WRITE_MEM support. And the documentation for that
    is kept it within this patch. This way if we switch to the frontend
    updating vhost-user memory table, we weren't set in any specific
    solution if patch 1 has been already merged.

BR,
Albert.


>
> 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: Type: text/html, Size: 10028 bytes --]