On Wed, 1 Mar 2017 19:19:40 +0200 "Michael S. Tsirkin" <[email protected]> wrote:
> On Wed, Mar 01, 2017 at 06:14:52PM +0100, Cornelia Huck wrote: > > On Wed, 1 Mar 2017 19:04:56 +0200 > > "Michael S. Tsirkin" <[email protected]> wrote: > > > > > On Wed, Mar 01, 2017 at 06:00:44PM +0100, Cornelia Huck wrote: > > > > On Wed, 1 Mar 2017 18:50:34 +0200 > > > > "Michael S. Tsirkin" <[email protected]> wrote: > > > > > > > Yes all these callbacks complicate code to the point it's barely > > > > > readable. I really don't see why are we poking at device beforehand at > > > > > all. IMHO this is closer to the root of the problem. Don't do it. One > > > > > way to look at it is to say that we start aio too early. Do it at > > > > > driver_ok for virtio 1 and on kick for virtio 0 and problems will go > > > > > away. > > > > > > > > The problem exists for the case where the guest sets up only the first > > > > queue, but the host has more than one queue. The guest setting up other > > > > queues later is probably patholotical, but not really forbidden by the > > > > spec (I think). > > > > > > I think it's forbidden. spec explains how queues are set up: > > > > > > 1. Reset the device. > > > 2. Set the ACKNOWLEDGE status bit: the guest OS has notice the device. > > > 3. Set the DRIVER status bit: the guest OS knows how to drive the device. > > > 4. Read device feature bits, and write the subset of feature bits > > > understood by the OS and driver to the > > > device. During this step the driver MAY read (but MUST NOT write) the > > > device-specific configuration > > > fields to check that it can support the device before accepting it. > > > 5. Set the FEATURES_OK status bit. The driver MUST NOT accept new feature > > > bits after this step. > > > 6. Re-read device status to ensure the FEATURES_OK bit is still set: > > > otherwise, the device does not > > > support our subset of features and the device is unusable. > > > 7. Perform 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. > > > 8. Set the DRIVER_OK status bit. At this point the device is “live”. > > > > It seems I managed to read over this statement in step 7 several > > times... So "make sure that we only start aio after DRIVER_OK, and > > forbid any setup for !desc permanently" will take care of virtio-1 > > devices. > > > > > > > > And it goes on to mention a bug in legacy drivers: > > > Legacy driver implementations often used the device before setting the > > > DRIVER_OK bit, and sometimes > > > even before writing the feature bits to the device. > > > > I wonder whether we need to accommodate legacy drivers doing both that > > _and_ setting up virtqueues between using the device for the first time > > and setting DRIVER_OK? Just using the logic of "if there were no rings > > setup when we first try to start aio, ignore that queue until reset" > > would cover all but those very odd drivers, and I highly doubt anybody > > cares about such broken setups. > > > kick on a vq before this vq is setup would be a broken thing to do. > > They did this though > > for each ring > add buffer > kick > driver ok > > so it's a per-ring thing. Hm... If we just check for !desc, we should be fine after the first aio poll, I think...
