OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.

 


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-dev message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]


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


On Thu, Oct 03, 2013 at 02:30:46PM +0930, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > On Wed, Oct 02, 2013 at 10:22:13AM +0930, Rusty Russell wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> > On Tue, Oct 01, 2013 at 05:42:52PM +0930, Rusty Russell wrote:
> >> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> >> > On Tue, Oct 01, 2013 at 11:19:57AM +0930, Rusty Russell wrote:
> >> >> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> >> >> > On Thu, Sep 26, 2013 at 12:05:19PM +0930, Rusty Russell wrote:
> >> >> >> >>    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?
> >> >> >> 
> >> >> >> The device MUST NOT consume buffers before DRIVER_OK.
> >> >> >>     - The driver is free to rip those descriptors back out if it wants
> >> >> >>     to clean up and fail, for example.
> >> >> >>     
> >> >> >> The driver MUST NOT notify the device before DRIVER_OK.
> >> >> >>     - It's not ready yet...
> >> >> >
> >> >> > This last one will need checks on data path in driver:
> >> >> > if (vq->dev->driver_ok)
> >> >> > 	vq_kick();
> >> >> > consider that once we register a netdev it
> >> >> > can start sending packets.
> >> >> 
> >> >> That's why the *driver* will need to set DRIVER_OK manually now.
> >> >
> >> > OK, but this means driver can fail after DRIVER_OK. Is that OK?
> >> 
> >> Yes, because that was always true.  Due to bugs, or OOM, it may be
> >> necessary to set FAILED on a device during operation.
> >
> > OK, in that case DRIVER_OK looks OK to me. So you'll post
> > the full version so we can OK this change on the next meeting?
> 
> Yes, I simply added the MUST NOT language to the last patch:
> 
> commit e1879030ddda125083ca3cc59fc106062b9ae7b4
> 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.  VIRTIO-30.
>     
>     Signed-off-by: Rusty Russell <rusty@au1.ibm.com>

I really meant send it as patch by itself,
starting a new thread.
not within body in response to another mail :)


> diff --git a/virtio-v1.0-wd01-part1-specification.txt b/virtio-v1.0-wd01-part1-specification.txt
> index b4298bb..938abb8 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,49 @@ 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".
>  
>  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).
>  
> +The device must not consume buffers 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
> +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 +2892,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!
>  
> 


[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]