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

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

"Michael S. Tsirkin" <mst@redhat.com> writes:
> 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 :)

Oh, OK.  Let's clean that up...

Cheers,
Rusty.