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

[Cornelia Huck <cohuck@redhat.com>]

On Tue, 15 Jan 2019 11:23:03 +0000
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * Cornelia Huck (cohuck@redhat.com) wrote:
> > 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?  
> 
> It's device-type specific; and potentially different for different
> shared memory regions associated with that device.

Ok; it might make sense to put a sentence like that into the generic
section.

> In the virtio-fs usecase we've really got two separate regions; one is
> a direct mapping of files on the host, the other is a structure
> containing flags/version numbers for data structures; the later
> probably has much more strict ordering semantics.

Another question: Do we expect the set of regions to be unchanged
during the lifetime of the device, or we need a mechanism to trigger
rediscovery?

> 
> > > > 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.  
> 
> Right, and it's the first option we need.

That makes things easier. Thanks!