Re: [virtio-comment] comment on pci section

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Fri, 31 Jan 2014 15:16:09 +1030
Rusty Russell <rusty@au1.ibm.com> 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.

s/buts/bits/

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

I'd rather call that 'host->guest notifications' or something like
that. Else, it would be confusing to somebody trying to implement a
virtio-ccw device (the channel commands always conclude with an I/O
interrupt; the host->guest notifiction is 'interrupt + indicator bit').

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

"the device and all of its configured virtqueues are..." ?

> A device and all
> +virtqueues are no longer live once the device has been reset.

"A device and any of its virtqueues..." ?

> +
> +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.
> +
> +Thus a driver will normally reset a device to remove exposed buffers.

"If a driver wants to remove exposed buffers, it SHOULD reset the
device." ?

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