Re: [virtio] [PATCH v2] initialization: add extra device status handshake

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

On Thu, Sep 26, 2013 at 12:05:19PM +0930, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > On Wed, Sep 25, 2013 at 10:32:03AM +0200, Paolo Bonzini wrote:
> >> Il 25/09/2013 10:28, Michael S. Tsirkin ha scritto:
> >> > > Failure to start vhost (which is fatal for vhost-scsi,
> >> > > since there is no userspace fallback unlike vhost-net) could be reported
> >> > > by not acknowledging FEATURES_OK.
> >> >
> >> > I think what you are asking for really, is
> >> > DEVICE_OK flag that locks both features and rings
> >> > and enables device.
> >> 
> >> Yes, you're right.  Plus the fact that the device might not acknowledge
> >> DEVICE_OK, which renders the device unusable.
> >> 
> >> There is a difference, though.  What happens if a virtqueue is only
> >> enabled by a particular feature?  FEATURES_OK would let the device
> >> disable discovery of that virtqueue.  DEVICE_OK wouldn't.
> >> 
> >> Paolo
> >
> > Maybe we want two new steps: FEATURES_OK and DEVICE_OK?
> 
> Hmm, indeed.
> 
> We need FEATURES_OK.  Consider a radical future change to the virtqueue
> structure: we need to finish feature negotiation before virtqueues get
> touched.
> 
> You've convinced me we also need a "setup complete" step, which I wanted
> to avoid since it would be (in Linux at least) a driver responsibility.
> We might as well re-use DRIVER_OK for this (which, after all, was the
> original intent!).
> 
> eg. consider virtio_net.c.  We would ack DRIVER_OK before
> register_netdev().  But we still haven't filled the receive buffers.
> The reason we do it that order is so that the device doesn't send us an
> rx interrupt before the net device is registered.  We could set
> DRIVER_OK then set FAILED if we OOM getting receive buffers, but it'd be
> nice to do it properly.
> 
> I'd *really* like the qemu v1.0 implementation to enforce this ordering,
> so we don't get errors in future, too...
> 
> How's this then?
> 
> commit 0792f04eef4c445141a7f44dd927eeef3bfce7b7
> Author: Rusty Russell <rusty@au1.ibm.com>
> Date:   Wed Sep 25 11:56:04 2013 +0930
> 
>     2.2.1: FEATURES_OK.
>     
>     Based on MST's ideas, but a bit simpler.
>     
>     Signed-off-by: Rusty Russell <rusty@au1.ibm.com>

Looks good overall, but I have a question. See below.

> diff --git a/virtio-v1.0-wd01-part1-specification.txt b/virtio-v1.0-wd01-part1-specification.txt
> index b4298bb..14f6a22 100644
> --- a/virtio-v1.0-wd01-part1-specification.txt
> +++ b/virtio-v1.0-wd01-part1-specification.txt
> @@ -106,6 +106,9 @@ This field is 0 upon reset, otherwise at least one bit should be set:
>    may be a significant (or infinite) delay before setting this
>    bit.
>  
> +  FEATURES_OK (8) Indicates that the driver has acknowledged all the
> +  features it understands, and feature negotiation is complete.
> +
>    DRIVER_OK (4) Indicates that the driver is set up and ready to
>    drive the device.
>  
> @@ -444,23 +447,46 @@ how to communicate with the specific device.
>  
>  3. The DRIVER status bit is set: we know how to drive the device.
>  
> -4. Device-specific setup, including reading the device feature
> -  bits, discovery of virtqueues for the device, optional per-bus
> -  setup, and reading and possibly writing the device's virtio
> -  configuration space.
> +4. Device feature bits are read, and the the subset of feature bits
> +   understood by the OS and driver is written to the device.
> +
> +5. The FEATURES_OK status bit is set.
>  
> -5. The subset of device feature bits understood by the driver is
> -   written to the device.
> +6. The status byte is re-read to ensure the FEATURES_OK bit is still
> +   set: otherwise, the device does not support our subset of features
> +   and the device is unusable.
>  
> -6. The DRIVER_OK status bit is set.
> +7. Device-specific setup, including discovery of virtqueues for the
> +   device, optional per-bus setup, reading and possibly writing the
> +   device's virtio configuration space, and population of virtqueues.
>  
> -7. The device can now be used (ie. buffers added to the
> -   virtqueues)[4]
> +8. The DRIVER_OK status bit is set.  At this point the device is
> +   "live".

What exactly does this imply?
Should device consume buffers before it's live, or should
it postpone this until DRIVER_OK?


>  
>  If any of these steps go irrecoverably wrong, the guest 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).
>  
> +Devices should support all valid combinations of features, but we know
> +that implementations may well make assuptions that they will only be
> +used by fully-optimized drivers.  The resetting of the FEATURES_OK flag
> +provides a semi-graceful failure mode for this case.
> +
> +2.2.1.1. Legacy Interface: Device Initialization
> +---------------------------
> +Legacy devices do not support the FEATURES_OK status bit, and thus did
> +not have a graceful way for the device to indicate unsupported feature
> +combinations.  It also did not provide a clear mechanism to end
> +feature negotiation, which meant that devices finalized features on
> +first-use, and no features could be introduced which radically changed
> +the initial operation of the device.
> +
> +Legacy device implementations often used the device before setting the
> +DRIVER_OK bit.
> +
> +The result was the steps 5 and 6 were omitted, and steps 7 and 8
> +were conflated.
> +
>  2.2.2. Device Operation
>  ----------------------
>  
> @@ -2863,10 +2889,6 @@ the non-PCI implementations (currently lguest and S/390).
>  
>  [3] The actual value within this range is ignored
>  
> -[4] Historically, drivers have used the device before steps 5 and 6.
> -This is only allowed if the driver does not use any features
> -which would alter this early use of the device.
> -
>  [5] ie. once you enable MSI-X on the device, the other fields move.
>  If you turn it off again, they move back!
>