Michael S. Tsirkin
2012-Dec-18 15:22 UTC
[Qemu-devel] [PATCH v6 12/12] virtio-blk: add x-data-plane=on|off performance feature
On Tue, Dec 18, 2012 at 03:57:17PM +0100, Stefan Hajnoczi wrote:> > > @@ -407,6 +409,14 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq) > > > .num_writes = 0, > > > }; > > > > > > + /* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start > > > + * dataplane here instead of waiting for .set_status(). > > > + */ > > > > By the way which guests are these? > > I ran a Windows 8 guest today with build 48 virtio-win drivers. It > notifies before the device gets its .set_status() callback invoked. > But I could swear I've seen Linux guests do this too.That's very broken. But looking at linux drivers it also seems linux guests do this even today. We have: err = drv->probe(dev); if (err) add_status(dev, VIRTIO_CONFIG_S_FAILED); else { add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK); if (drv->scan) drv->scan(dev); } this means that unless drivers implement scan() they will make device active before DRIVER_OK is written as the result linux can access it and kick. And almost no drivers implement scan. Nasty. Rusty, what do you think? Worth fixing? It does mean that for now we are stuck with a work around, but I think we need it in virtio core in qemu, it's not dataplane specific. -- MST
Rusty Russell
2012-Dec-20 04:04 UTC
[Qemu-devel] [PATCH v6 12/12] virtio-blk: add x-data-plane=on|off performance feature
"Michael S. Tsirkin" <mst at redhat.com> writes:> On Tue, Dec 18, 2012 at 03:57:17PM +0100, Stefan Hajnoczi wrote: >> > > @@ -407,6 +409,14 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq) >> > > .num_writes = 0, >> > > }; >> > > >> > > + /* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start >> > > + * dataplane here instead of waiting for .set_status(). >> > > + */ >> > >> > By the way which guests are these? >> >> I ran a Windows 8 guest today with build 48 virtio-win drivers. It >> notifies before the device gets its .set_status() callback invoked. >> But I could swear I've seen Linux guests do this too. > > > That's very broken. But looking at linux drivers it also > seems linux guests do this even today. > We have: > > err = drv->probe(dev); > if (err) > add_status(dev, VIRTIO_CONFIG_S_FAILED); > else { > add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK); > if (drv->scan) > drv->scan(dev); > } > > this means that unless drivers implement scan() they > will make device active before DRIVER_OK is written > as the result linux can access it and kick. > And almost no drivers implement scan. > Nasty.Yes, that's true. But as long as they have completed feature negotiation, we allow this (that's why we tool feature negotiation out of the drivers). For example, filling an input virtqueue may well mean we kick the vq, and almost every device does this. virtio_block is the worst: add_disk() does partition scanning.> Rusty, what do you think? Worth fixing?If we tried, I'm fairly sure things would slip through. My feeling has been that we should not rely on the status to indicate readiness.> It does mean that for now we are stuck > with a work around, but I think we need it > in virtio core in qemu, it's not dataplane > specific.Yes, it's a general problem. Cheers, Rusty.
Reasonably Related Threads
- [Qemu-devel] [PATCH v6 12/12] virtio-blk: add x-data-plane=on|off performance feature
- [PATCH v4 09/42] virtio: set FEATURES_OK
- [PATCH v4 09/42] virtio: set FEATURES_OK
- [PATCH] virtio-scsi: Add vdrv->scan for post VIRTIO_CONFIG_S_DRIVER_OK LUN scanning
- [PATCH] virtio-scsi: Add vdrv->scan for post VIRTIO_CONFIG_S_DRIVER_OK LUN scanning