Re: [virtio-comment] comment on pci section

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

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

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.

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

Yes, you're right.  How's this?

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

How's this:

  A driver MUST NOT decrement the available index on a live virtqueue (ie.
  do not try to "unexpose" buffers).

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

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.