[Qemu-devel] [PATCH] dataplane: support viostor virtio-pci status bit setting

[Qemu-devel] [PATCH] dataplane: support viostor virtio-pci status bit setting

[Stefan Hajnoczi]

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(-)

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

Re: [Qemu-devel] [PATCH] dataplane: support viostor virtio-pci status bit setting

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH] dataplane: support viostor virtio-pci status bit setting

[Stefan Hajnoczi]

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

Re: [Qemu-devel] [PATCH] dataplane: support viostor virtio-pci status bit setting

[Stefan Hajnoczi]

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

Re: [Qemu-devel] [PATCH] dataplane: support viostor virtio-pci status bit setting

[Vadim Rozenfeld]

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

Re: [Qemu-devel] [PATCH] dataplane: support viostor virtio-pci status bit setting

[Stefan Hajnoczi]

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

Re: [Qemu-devel] [PATCH] dataplane: support viostor virtio-pci status bit setting

[Michael S. Tsirkin]

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.