Re: [virtio-comment] comment on pci section

[Rusty Russell <rusty@au1.ibm.com>]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Mon, Feb 03, 2014 at 12:00:03PM +1030, Rusty Russell wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> > On Fri, Jan 31, 2014 at 03:16:09PM +1030, Rusty Russell wrote:
>> >> Rusty Russell <rusty@au1.ibm.com> writes:
>> >>  \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.
>> 
>> It came up as I was designing the new balloon.  In that case, I didn't
>> want a feature bit for every possible page size, so I put the offered
>> page size in the config space.  In that case, the driver will want to
>> check that it can meet the given page size before accepting the device.
>
> Right. So how about
> 	 driver MAY read (but not write) the device-specific
> 	 configuration fields to check that it can
> 	 support it before accepting the device.

Done:

\item Read device feature bits, and write the subset of feature bits
   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.

>> > How about
>> > a driver MUST 
>> 
>> I reworked this a bit more:
>> 
>>    Thus a driver MUST ensure a virtqueue isn't live before removing exposed buffers (by device reset, or some manner specified for that specific device type).
>
> okay but need to re-order:
>
>  Thus a driver MUST ensure a virtqueue isn't live (by device reset, or
> in some manner specified for that specific device type) before removing
> exposed buffers.

Much nicer.   Done.

>> MQ net can make virtqueues non-live as noted below.
>> 
>> >
>> >>  
>> >>  \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?
>> 
>> Oh, so you can't remove buffers from an MQ virtqueue on leaving MQ mode?
>> That's pretty poor.
>> 
>> Cheers,
>> Rusty.
>
> Well, removing buffers could be implemented by consuming all buffers
> by the device. Seems somewhat cleaner.

So the device would mark them all used (and zero length?).

That's a little icky.  Let's leave it as-is for now.

> I think it's more a question of adding/removing VQs themselves.
> Device Initialization at the moment specifies that VQ setup
> must be complete before DRIVER_OK.
> Would be nice to change that but maybe build a standard way
> to do this then?

Hmm, we specify that all possible virtqueues should be initialized,
and that's what net does at the moment.

> I also wonder whether this can work for all transports.
> Also, e.g.  MSIX allows changing # of vectors, e.g. MSI doesn't.
> Also, a bunch of internal APIs in Linux would have to be extended
> if we want to allow allocating arbitrary vectors.

Indeed, let's leave initialization as-is, at least for now.

Cheers,
Rusty.

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.  Make it clear that
   taking the net device out of mq mode makes the extra vq's not-live.

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.
+
+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).
+
+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}