[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, 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! >
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]