[Qemu-devel] [RESEND PATCH] virtio: allow broken device to notify guest

[Qemu-devel] [RESEND PATCH] virtio: allow broken device to notify guest

[Greg Kurz]

According to section 2.1.2 of the virtio-1 specification:

"The device SHOULD set DEVICE_NEEDS_RESET when it enters an error state that
a reset is needed. If DRIVER_OK is set, after it sets DEVICE_NEEDS_RESET,
the device MUST send a device configuration change notification to the
driver."

Commit "f5ed36635d8f virtio: stop virtqueue processing if device is broken"
introduced a virtio_error() call that just does that:

- internally mark the device as broken
- set the DEVICE_NEEDS_RESET bit in the status
- send a configuration change notification

Unfortunately, virtio_notify_vector(), called by virtio_notify_config(),
returns right away when the device is marked as broken and the notification
isn't sent in this case.

The spec doesn't say whether a broken device can send notifications
in other situations or not. But since the driver isn't supposed to do
anything but to reset the device, it makes sense to keep the check in
virtio_notify_config().

Marking the device as broken AFTER the configuration change notification was
sent is enough to fix the issue.

Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/virtio/virtio.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Re-sending this patch with Cornelia's Reviewed-by and Cc'ing qemu-stable
since this is a spec violation, as suggested in:

Message-ID: <20170427183237-mutt-send-email-mst@kernel.org>

Cheers,

--
Greg

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 03592c542a55..890b4d7eb751 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2451,12 +2451,12 @@ void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
     error_vreport(fmt, ap);
     va_end(ap);
 
-    vdev->broken = true;
-
     if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
         virtio_set_status(vdev, vdev->status | VIRTIO_CONFIG_S_NEEDS_RESET);
         virtio_notify_config(vdev);
     }
+
+    vdev->broken = true;
 }
 
 static void virtio_memory_listener_commit(MemoryListener *listener)

Re: [Qemu-devel] [RESEND PATCH] virtio: allow broken device to notify guest

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1691 bytes --]

On Wed, May 17, 2017 at 10:17:51AM +0200, Greg Kurz wrote:
> According to section 2.1.2 of the virtio-1 specification:
> 
> "The device SHOULD set DEVICE_NEEDS_RESET when it enters an error state that
> a reset is needed. If DRIVER_OK is set, after it sets DEVICE_NEEDS_RESET,
> the device MUST send a device configuration change notification to the
> driver."
> 
> Commit "f5ed36635d8f virtio: stop virtqueue processing if device is broken"
> introduced a virtio_error() call that just does that:
> 
> - internally mark the device as broken
> - set the DEVICE_NEEDS_RESET bit in the status
> - send a configuration change notification
> 
> Unfortunately, virtio_notify_vector(), called by virtio_notify_config(),
> returns right away when the device is marked as broken and the notification
> isn't sent in this case.
> 
> The spec doesn't say whether a broken device can send notifications
> in other situations or not. But since the driver isn't supposed to do
> anything but to reset the device, it makes sense to keep the check in
> virtio_notify_config().
> 
> Marking the device as broken AFTER the configuration change notification was
> sent is enough to fix the issue.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
>  hw/virtio/virtio.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Re-sending this patch with Cornelia's Reviewed-by and Cc'ing qemu-stable
> since this is a spec violation, as suggested in:
> 
> Message-ID: <20170427183237-mutt-send-email-mst@kernel.org>
> 
> Cheers,
> 
> --
> Greg

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [Qemu-stable] [RESEND PATCH] virtio: allow broken device to notify guest

[Fam Zheng]

On Wed, 05/17 10:17, Greg Kurz wrote:
> According to section 2.1.2 of the virtio-1 specification:
> 
> "The device SHOULD set DEVICE_NEEDS_RESET when it enters an error state that
> a reset is needed. If DRIVER_OK is set, after it sets DEVICE_NEEDS_RESET,
> the device MUST send a device configuration change notification to the
> driver."
> 
> Commit "f5ed36635d8f virtio: stop virtqueue processing if device is broken"
> introduced a virtio_error() call that just does that:
> 
> - internally mark the device as broken
> - set the DEVICE_NEEDS_RESET bit in the status
> - send a configuration change notification
> 
> Unfortunately, virtio_notify_vector(), called by virtio_notify_config(),
> returns right away when the device is marked as broken and the notification
> isn't sent in this case.
> 
> The spec doesn't say whether a broken device can send notifications
> in other situations or not. But since the driver isn't supposed to do
> anything but to reset the device, it makes sense to keep the check in
> virtio_notify_config().
> 
> Marking the device as broken AFTER the configuration change notification was
> sent is enough to fix the issue.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
>  hw/virtio/virtio.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Re-sending this patch with Cornelia's Reviewed-by and Cc'ing qemu-stable
> since this is a spec violation, as suggested in:
> 
> Message-ID: <20170427183237-mutt-send-email-mst@kernel.org>
> 
> Cheers,
> 
> --
> Greg
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 03592c542a55..890b4d7eb751 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2451,12 +2451,12 @@ void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
>      error_vreport(fmt, ap);
>      va_end(ap);
>  
> -    vdev->broken = true;
> -
>      if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
>          virtio_set_status(vdev, vdev->status | VIRTIO_CONFIG_S_NEEDS_RESET);
>          virtio_notify_config(vdev);
>      }
> +
> +    vdev->broken = true;
>  }
>  
>  static void virtio_memory_listener_commit(MemoryListener *listener)
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>