qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] virtio: guard vring access when setting notific


From: Cornelia Huck
Subject: Re: [Qemu-devel] [PATCH] virtio: guard vring access when setting notification
Date: Wed, 1 Mar 2017 18:14:52 +0100

On Wed, 1 Mar 2017 19:04:56 +0200
"Michael S. Tsirkin" <address@hidden> 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" <address@hidden> 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.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]