Re: [virtio-comment] [PATCH 1/3] shared memory: Define shared memory regions

[Cornelia Huck <cohuck@redhat.com>]

On Fri, 11 Jan 2019 12:26:54 +0000
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * Cornelia Huck (cohuck@redhat.com) wrote:
> > On Fri, 11 Jan 2019 11:41:58 +0000
> > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:

> > > +\section{Shared Memory Regions}\label{sec:Basic Facilities of a Virtio Device / Shared Memory Regions}
> > > +
> > > +Shared memory regions are an additional facility
> > > +available to devices that need a region of memory that's
> > > +continuously shared between the host and the guest, rather
> > > +than passed between them in the way virtqueue elements are.

I think we probably need to clarify the expectations (consistency etc.)
a bit more, see my remarks below.

> > > +
> > > +Example uses include shared caches and version pools for versioned
> > > +data structures.
> > > +
> > > +Shared memory regions MUST NOT be used to control the operation
> > > +of the device, nor to stream data; those should still be performed
> > > +using virtqueues.
> > > +
> > > +A device may have multiple shared memory regions associated with
> > > +it.  Each region has a \field{shmid} to identify it, the meaning
> > > +of which is device specific.
> > > +
> > > +Enumeration and location of shared memory regions is performed
> > > +using a transport-specific data structure.  
> > 
> > "data structure and mechanism"?  
> 
> Changed; thanks.
> 
> > > +
> > > +The guest physical address and the host virtual address MUST NOT
> > > +be used to identify structures within the memory regions; all
> > > +addressing MUST be relative to the start of a particular region.
> > > +  
> > 
> > Is the intended implementation that the device provides a certain
> > memory region (in host memory) and exposes it to the driver? Are there
> > supposed to be any notifications of writes? Or do both simply write to
> > the region and get whatever updates the other side has made when they
> > read from the region again?  
> 
> There's no notification;  in our case we have two main uses:
>   a) Direct mapping of host files into the guests memory
> 
>   b) Mapping of a version table with quickly updated version numbers for
>      data structures to do quick invalidation

This sounds a lot like "we have a memory area, and both device and
driver may write to or read from it at any time". Are there any
expectations regarding consistency when reading data, or is there
supposed to be a device-type specific mechanism to get certain
consistent values?

> > I'm a bit unsure how to implement this for the ccw transport. Maybe a
> > new pair of ccws to read/write shared memory regions?  
> 
> Without knowing anything about CCW itself, I don't think you'd want
> to do calls to perform the reads/writes - remember these are entirely
> emulated devices, and the shared memory regions just correspond to
> memory regions in the hypervisor; so in most ways they just behave
> like a region of RAM.  If the drivers can't treat them like RAM there's
> probably no point in using this feature in that environment.

The main issue here is that s390 does not have memory mapped I/O --
even PCI uses some specialized instructions. This means we need to
figure out how to model some stuff that Just Works on other platforms.

So, basically there are two options:
- Have the device set aside a memory area; the host maps this into the
  guest and the driver can access it. No notifications, only discovery
  is needed.
- Have the device set aside a memory area; the driver can only access
  this via special operations, which the host can trap. This needs two
  more commands to be set aside, and any driver accesses need to be
  forced through these commands (that's a bit like config space).

If I've understood the intended usage correctly, we can use the simpler
first option. The drawback is that we can't add interception
possibilities (that we get via the second option) should we need them
later on.

> 
> > But we'd also
> > need a mechanism to discover the ids of those shared memory regions, I
> > think.  
> 
> Yes, I'm assuming you'll need a call to enumerate them.

Agreed.