Re: [virtio-comment] comment on pci section

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Feb 05, 2014 at 04:25:16PM +0100, Cornelia Huck wrote:
> On Wed, 05 Feb 2014 09:49:18 +1030
> Rusty Russell <rusty@au1.ibm.com> wrote:
> 
> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> > > On Tue, Feb 04, 2014 at 01:35:55PM +1030, Rusty Russell wrote:
> > >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > >> 2) Add the concept of a "live" virtqueue, and explicitly disallow
> > >>    moving it backwards or changing descriptors.  Make it clear that
> > >>    taking the net device out of mq mode makes the extra vq's not-live.
> > >
> > > This last part doesn't apply anymore, does it?
> > 
> > Yes, indeed.
> > 
> > Here's what I'm proposing to apply, if noone objects:
> > 
> > Subject: Cleanup and setup clarifications
> > From: Rusty Russell <rusty@au1.ibm.com>
> > 
> > 1) Explicitly allow drivers to read config space during feature
> >    negotiation.
> > 
> > 2) Add the concept of a "live" virtqueue, and explicitly disallow
> >    moving it backwards or changing descriptors.
> > 
> > Signed-off-by: Rusty Russell <rusty@au1.ibm.com>
> > 
> > diff --git a/content.tex b/content.tex
> > index fa33435..413a4ad 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -524,7 +524,11 @@ The driver MUST follow this sequence to initialize a device:
> >  \item Set the DRIVER status bit: the guest OS knows how to drive the device.
> > 
> >  \item Read device feature bits, and write the subset of feature bits
> > -   understood by the OS and driver to the device.
> > +   understood by the OS and driver to the device.  During this step the
> > +   driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device before accepting it.
> > +   The device MUST allow reading of any device-specific configuration field
> > +   before FEATURES_OK is set.  This includes fields which are conditional
> > +   on feature bits, as long as those feature bits are offered by the device.
> > 
> >  \item\label{itm:General Initialization And Device Operation / Device Initialization / Set FEATURES-OK} Set the FEATURES_OK status bit.  The driver MUST not accept
> >     new feature bits after this step.
> > @@ -544,9 +548,9 @@ The driver MUST follow this sequence to initialize a device:
> >  If any of these steps go irrecoverably wrong, the driver SHOULD
> >  set the FAILED status bit to indicate that it has given up on the
> >  device (it can reset the device later to restart if desired).  The
> > -driver MUST not continue initialization in that case.
> > +driver MUST NOT continue initialization in that case.
> > 
> > -The device MUST NOT consume buffers before DRIVER_OK, and the driver
> > +The device MUST NOT consume buffers or notify the driver before DRIVER_OK, and the driver
> >  MUST NOT notify the device before it sets DRIVER_OK.
> > 
> >  Devices SHOULD support all valid combinations of features, but we know
> > @@ -769,7 +773,18 @@ suppressed by the device:
> >  For devices where the configuration information can be changed, an
> >  interrupt is delivered when a configuration change occurs.
> > 
> > +\section{Device Cleanup}\label{sec:General Initialization And Device Operation / Device Cleanup}
> > 
> > +Once the driver has set the DRIVER_OK status bit, all the configured virtqueue of the device are considered live.  All of a device's virtqueues are no longer live once the device has been reset.
> 
> What about something similar to my earlier suggestions?
> 
> "Once the driver has set the DRIVER_OK status bit, all of the
> configured virtqueues of the device are considered live.  Any of the
> virtqueues of a device are no longer live once the device has been
> reset."
> 
> ?
> > +
> > +A driver MUST NOT alter descriptor table entries which have been
> > +exposed in the available ring (and not marked consumed by the device
> > +in the used ring) of a live virtqueue.
> > +
> > +A driver MUST NOT decrement the available index on a live virtqueue (ie.
> > +do not try to "unexpose" buffers).
> 
> "(i.e. the driver MUST NOT try to "unexpose" buffers)."
> 
> ?

ACK

> > +
> > +Thus a driver MUST ensure a virtqueue isn't live (by device reset) before removing exposed buffers.
> > 
> >  \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options}