Re: [virtio-comment] comment on pci section

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

On Tue, Feb 04, 2014 at 01:35:55PM +1030, Rusty Russell wrote:
> "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.

This last part doesn't apply anymore, does it?

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