[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
"Michael S. Tsirkin" <firstname.lastname@example.org> 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 <email@example.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 <firstname.lastname@example.org> 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) +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). +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. + +188.8.131.52. 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).  The actual value within this range is ignored - 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. -  ie. once you enable MSI-X on the device, the other fields move. If you turn it off again, they move back!