Re: [virtio-comment] comment on pci section

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

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

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.

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


ACK

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

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.

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

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?

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.

-- 
MST