Re: [virtio-comment] comment on pci section

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

On Fri, Jan 31, 2014 at 03:16:09PM +1030, Rusty Russell wrote:
> Rusty Russell <rusty@au1.ibm.com> writes:
> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> On Wed, Jan 29, 2014 at 04:38:58PM +1030, Rusty Russell wrote:
> >>> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >>> > Two comments received from a co-worker:
> >>> >
> >>> > 3.1 "The device MUST NOT consume buffers before DRIVER_OK ..." - I
> >>> > personally
> >>> > would like to see also something like:  "device must not generate
> >>> > interrupts if
> >>> > DRIVER_OK is not set” to better handle power management transitions.
> >>> 
> >>> That addition seems to make sense, but I don't understand the reference
> >>> to power management transitions?
> >>
> >> Well - ATM we do things like suspend by device reset and re-initialization,
> >> right? I'm guessing during e.g. resume it's important not to get
> >> interrupts too early. I'll check.
> >>
> >> Hmm actually we have a chapter about initialization but not cleanup.
> >> Might be a good addition.
> >
> > Maybe, but it would only need to say to reset the device and you can
> > remove buffers safely.
> 
> How's this?  Actually does two things
> 
> 1) Explicitly allow drivers to access config space during feature
>    negotiation.
> 
> 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.
> 
> diff --git a/content.tex b/content.tex
> index 48698fc..00d5fb3 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -520,7 +520,10 @@ 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 access the device-specific configuration fields, even if
> +   those fields are conditional on feature bits, as long as those
> +   feature buts are offered by the device.

I'm not sure why we want to allow this.
Which use cases need such an access?
Also, do we need to allow writes?
How about s/access/read but not write/ ?
If driver sets configuration it seems easier if device
can validate it depending on features.

>  
>  \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.
> @@ -540,9 +543,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 send interrupts 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
> @@ -765,7 +768,19 @@ 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, the device is and
> +all configured virtqueue are considered live.  A device and all
> +virtqueues are no longer live once the device has been reset.

How about simply all device virtqueues?
Device live concept does not seem to be used ...

> +
> +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 to "unexpose" buffers.

I would put a period after decrement the available index - it must not
do this  for any reason. Also add "of a live virtqueue" to
make it clear.

> +
> +Thus a driver will normally reset a device to remove exposed buffers.

How about
a driver MUST 

>  
>  \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options}
>  
> @@ -2880,7 +2895,8 @@ device MUST NOT steer new packets to virtqueues
>  receveq1..receiveqN (i.e. other than receiveq0) and MUST NOT read from
>  transmitq1..transmitqN (i.e. other than transmitq0); accordingly,
>  the driver MUST NOT transmit new packets on virtqueues other than
> -transmitq0.
> +transmitq0.  These virtqueues are no longer considered live, and thus
> +the driver may be safely remove any buffers from them.

MAY ?

This is not supported for legacy devices at least:
if VQ is not live this implies driver can do crazy things
like decrement available index.

Really worth it?

>  
>  \subparagraph{Legacy Interface: Automatic receive steering in multiqueue mode}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Automatic receive steering in multiqueue mode / Legacy Interface: Automatic receive steering in multiqueue mode}
>  For legacy devices, the virtqueue_paris field is in the