| Message ID | 1358437614-14968-1-git-send-email-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jan 17, 2013 at 04:46:54PM +0100, Stefan Hajnoczi wrote: > The viostor virtio-blk driver for Windows does not use the > VIRTIO_CONFIG_S_DRIVER bit. It only sets the VIRTIO_CONFIG_S_DRIVER_OK > bit. > > The viostor driver refreshes the virtio-pci status byte sometimes while > the guest is running. We misinterpret 0x4 (VIRTIO_CONFIG_S_DRIVER_OK) > as an indication that virtio-blk-data-plane should be stopped since 0x2 > (VIRTIO_CONFIG_S_DRIVER) is missing. The result is that the device > becomes unresponsive. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> I think you actually want if (s->dataplane && !(status & VIRTIO_CONFIG_S_DRIVER_OK))) so stop on any error. This is also consistent with what vhost-net does. > --- > hw/virtio-blk.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c > index df57b35..34913ee 100644 > --- a/hw/virtio-blk.c > +++ b/hw/virtio-blk.c > @@ -571,7 +571,8 @@ static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status) > uint32_t features; > > #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE > - if (s->dataplane && !(status & VIRTIO_CONFIG_S_DRIVER)) { > + if (s->dataplane && !(status & (VIRTIO_CONFIG_S_DRIVER | > + VIRTIO_CONFIG_S_DRIVER_OK))) { > virtio_blk_data_plane_stop(s->dataplane); > } > #endif > -- > 1.8.0.2
On Thu, Jan 17, 2013 at 05:59:17PM +0200, Michael S. Tsirkin wrote: > On Thu, Jan 17, 2013 at 04:46:54PM +0100, Stefan Hajnoczi wrote: > > The viostor virtio-blk driver for Windows does not use the > > VIRTIO_CONFIG_S_DRIVER bit. It only sets the VIRTIO_CONFIG_S_DRIVER_OK > > bit. > > > > The viostor driver refreshes the virtio-pci status byte sometimes while > > the guest is running. We misinterpret 0x4 (VIRTIO_CONFIG_S_DRIVER_OK) > > as an indication that virtio-blk-data-plane should be stopped since 0x2 > > (VIRTIO_CONFIG_S_DRIVER) is missing. The result is that the device > > becomes unresponsive. > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > I think you actually want > > if (s->dataplane && !(status & VIRTIO_CONFIG_S_DRIVER_OK))) > > so stop on any error. > > This is also consistent with what vhost-net does. We can't do that because of the Linux virtio quirk where it begins using virtqueues before VIRTIO_CONFIG_S_DRIVER_OK. I original checked VIRTIO_CONFIG_S_DRIVER_OK but I'm pretty sure I hit a situation where the status would still be set ~DRIVER_OK after the first kick - this would stop data plane and the vring last indices would no longer be in sync when it started again. So I think it's most robust to check both DRIVER and DRIVER_OK. Stefan
On Thu, Jan 17, 2013 at 04:46:54PM +0100, Stefan Hajnoczi wrote: > The viostor virtio-blk driver for Windows does not use the > VIRTIO_CONFIG_S_DRIVER bit. It only sets the VIRTIO_CONFIG_S_DRIVER_OK > bit. > > The viostor driver refreshes the virtio-pci status byte sometimes while > the guest is running. We misinterpret 0x4 (VIRTIO_CONFIG_S_DRIVER_OK) > as an indication that virtio-blk-data-plane should be stopped since 0x2 > (VIRTIO_CONFIG_S_DRIVER) is missing. The result is that the device > becomes unresponsive. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > hw/virtio-blk.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan
On Friday, January 18, 2013 05:59:37 PM Stefan Hajnoczi wrote: > On Thu, Jan 17, 2013 at 04:46:54PM +0100, Stefan Hajnoczi wrote: > > The viostor virtio-blk driver for Windows does not use the > > VIRTIO_CONFIG_S_DRIVER bit. It only sets the VIRTIO_CONFIG_S_DRIVER_OK > > bit. Will be added in the next build. Thank you, Vadim. > > > > The viostor driver refreshes the virtio-pci status byte sometimes while > > the guest is running. We misinterpret 0x4 (VIRTIO_CONFIG_S_DRIVER_OK) > > as an indication that virtio-blk-data-plane should be stopped since 0x2 > > (VIRTIO_CONFIG_S_DRIVER) is missing. The result is that the device > > becomes unresponsive. > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > > > hw/virtio-blk.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > Applied to my block tree: > https://github.com/stefanha/qemu/commits/block > > Stefan
On Sat, Jan 19, 2013 at 09:59:57AM +0200, Vadim Rozenfeld wrote: > On Friday, January 18, 2013 05:59:37 PM Stefan Hajnoczi wrote: > > On Thu, Jan 17, 2013 at 04:46:54PM +0100, Stefan Hajnoczi wrote: > > > The viostor virtio-blk driver for Windows does not use the > > > VIRTIO_CONFIG_S_DRIVER bit. It only sets the VIRTIO_CONFIG_S_DRIVER_OK > > > bit. > Will be added in the next build. Nice, thanks. For compatibility with existing viostor drivers QEMU will carry this patch. Stefan
On Mon, Jan 21, 2013 at 10:36:18AM +0100, Stefan Hajnoczi wrote: > On Sat, Jan 19, 2013 at 09:59:57AM +0200, Vadim Rozenfeld wrote: > > On Friday, January 18, 2013 05:59:37 PM Stefan Hajnoczi wrote: > > > On Thu, Jan 17, 2013 at 04:46:54PM +0100, Stefan Hajnoczi wrote: > > > > The viostor virtio-blk driver for Windows does not use the > > > > VIRTIO_CONFIG_S_DRIVER bit. It only sets the VIRTIO_CONFIG_S_DRIVER_OK > > > > bit. > > Will be added in the next build. > > Nice, thanks. For compatibility with existing viostor drivers QEMU will > carry this patch. > > Stefan I still think it's wrong: when DRIVER_OK is cleared you should stop device I think even if DRIVER is set. This patch keeps dataplane running if DRIVER is set.
On Mon, Jan 21, 2013 at 12:05:16PM +0200, Michael S. Tsirkin wrote: > On Mon, Jan 21, 2013 at 10:36:18AM +0100, Stefan Hajnoczi wrote: > > On Sat, Jan 19, 2013 at 09:59:57AM +0200, Vadim Rozenfeld wrote: > > > On Friday, January 18, 2013 05:59:37 PM Stefan Hajnoczi wrote: > > > > On Thu, Jan 17, 2013 at 04:46:54PM +0100, Stefan Hajnoczi wrote: > > > > > The viostor virtio-blk driver for Windows does not use the > > > > > VIRTIO_CONFIG_S_DRIVER bit. It only sets the VIRTIO_CONFIG_S_DRIVER_OK > > > > > bit. > > > Will be added in the next build. > > > > Nice, thanks. For compatibility with existing viostor drivers QEMU will > > carry this patch. > > > > Stefan > > I still think it's wrong: when DRIVER_OK is cleared you should stop > device I think even if DRIVER is set. > This patch keeps dataplane running if DRIVER is set. When virtio-blk-data-plane is started vring indices start from 0. When the guest submits requests before DRIVER_OK the vring indices become non-zero. Now if it sets status DRIVER (but ~DRIVER_OK) we'd stop the vring. The guest driver doesn't know we've effectively reset the vring. So on the next kick guest and host are no longer in sync and we get one of the "index moved from 0 to 50" type errors. So I think the consistent solution is to treat DRIVER as the status level where vring processing starts and therefore keep the vring alive until the device is reset. Your approach allows guests to reset virtqueues by clearing the DRIVER_OK bit. I haven't seen anything that depends on this and in practice Linux guest drivers do not limit vring lifetime to DRIVER_OK, so it's riskier IMO to stop when DRIVER_OK gets cleared. Stefan
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c index df57b35..34913ee 100644 --- a/hw/virtio-blk.c +++ b/hw/virtio-blk.c @@ -571,7 +571,8 @@ static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status) uint32_t features; #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE - if (s->dataplane && !(status & VIRTIO_CONFIG_S_DRIVER)) { + if (s->dataplane && !(status & (VIRTIO_CONFIG_S_DRIVER | + VIRTIO_CONFIG_S_DRIVER_OK))) { virtio_blk_data_plane_stop(s->dataplane); } #endif
The viostor virtio-blk driver for Windows does not use the VIRTIO_CONFIG_S_DRIVER bit. It only sets the VIRTIO_CONFIG_S_DRIVER_OK bit. The viostor driver refreshes the virtio-pci status byte sometimes while the guest is running. We misinterpret 0x4 (VIRTIO_CONFIG_S_DRIVER_OK) as an indication that virtio-blk-data-plane should be stopped since 0x2 (VIRTIO_CONFIG_S_DRIVER) is missing. The result is that the device becomes unresponsive. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- hw/virtio-blk.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)