Patchwork dataplane: support viostor virtio-pci status bit setting

login
register
mail settings
Submitter Stefan Hajnoczi
Date Jan. 17, 2013, 3:46 p.m.
Message ID <1358437614-14968-1-git-send-email-stefanha@redhat.com>
Download mbox | patch
Permalink /patch/213313/
State New
Headers show

Comments

Stefan Hajnoczi - Jan. 17, 2013, 3:46 p.m.
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(-)
Michael S. Tsirkin - Jan. 17, 2013, 3:59 p.m.
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
Stefan Hajnoczi - Jan. 17, 2013, 4:59 p.m.
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
Stefan Hajnoczi - Jan. 18, 2013, 3:59 p.m.
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
Vadim Rozenfeld - Jan. 19, 2013, 7:59 a.m.
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
Stefan Hajnoczi - Jan. 21, 2013, 9:36 a.m.
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
Michael S. Tsirkin - Jan. 21, 2013, 10:05 a.m.
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.
Stefan Hajnoczi - Jan. 21, 2013, 3:42 p.m.
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

Patch

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