1. Patchew
  2. QEMU
  3. View series

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

Greg Kurz posted 1 patch 6 years, 12 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/149321107781.13002.9081643707477650100.stgit@bahia.lan
Test checkpatch passed
Test docker passed
Test s390x passed
hw/virtio/virtio.c |    4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[Qemu-devel] [PATCH] virtio: allow broken device to notify guest
Posted by Greg Kurz 6 years, 12 months ago 
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>
---
 hw/virtio/virtio.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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] [PATCH] virtio: allow broken device to notify guest
Posted by Cornelia Huck 6 years, 12 months ago 
On Wed, 26 Apr 2017 14:51:17 +0200
Greg Kurz <groug@kaod.org> 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>
> ---
>  hw/virtio/virtio.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> 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)
> 

Good catch.

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Should this be cc:stable, as it's a spec violation?
Re: [Qemu-devel] [PATCH] virtio: allow broken device to notify guest
Posted by Greg Kurz 6 years, 12 months ago 
On Wed, 26 Apr 2017 15:15:48 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Wed, 26 Apr 2017 14:51:17 +0200
> Greg Kurz <groug@kaod.org> 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>
> > ---
> >  hw/virtio/virtio.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > 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)
> >   
> 
> Good catch.
> 
> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> 
> Should this be cc:stable, as it's a spec violation?
> 

I don't know if this qualifies for stable, but if it does then it affects
all versions >= 2.8.0.
Re: [Qemu-devel] [PATCH] virtio: allow broken device to notify guest
Posted by Michael S. Tsirkin 6 years, 12 months ago 
On Wed, Apr 26, 2017 at 03:29:46PM +0200, Greg Kurz wrote:
> On Wed, 26 Apr 2017 15:15:48 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> 
> > On Wed, 26 Apr 2017 14:51:17 +0200
> > Greg Kurz <groug@kaod.org> 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>
> > > ---
> > >  hw/virtio/virtio.c |    4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > 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)
> > >   
> > 
> > Good catch.
> > 
> > Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > 
> > Should this be cc:stable, as it's a spec violation?
> > 
> 
> I don't know if this qualifies for stable, but if it does then it affects
> all versions >= 2.8.0.


It's a SHOULD so not a violation, just a quality of implementation
issue. Seems a bit too intrusive for stable and we are yet to
have drivers actually handling these errors, so let's wait a bit
and see.

I'll apply this to master for now.


-- 
MST
Re: [Qemu-devel] [PATCH] virtio: allow broken device to notify guest
Posted by Greg Kurz 6 years, 12 months ago 
On Thu, 27 Apr 2017 02:42:56 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Apr 26, 2017 at 03:29:46PM +0200, Greg Kurz wrote:
> > On Wed, 26 Apr 2017 15:15:48 +0200
> > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> >   
> > > On Wed, 26 Apr 2017 14:51:17 +0200
> > > Greg Kurz <groug@kaod.org> 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>
> > > > ---
> > > >  hw/virtio/virtio.c |    4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > 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)
> > > >     
> > > 
> > > Good catch.
> > > 
> > > Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > > 
> > > Should this be cc:stable, as it's a spec violation?
> > >   
> > 
> > I don't know if this qualifies for stable, but if it does then it affects
> > all versions >= 2.8.0.  
> 
> 
> It's a SHOULD so not a violation, just a quality of implementation

Setting DEVICE_NEEDS_RESET is indeed a SHOULD, but failing to send the
configuration change notification violates a MUST statement, FWIW.

> issue. Seems a bit too intrusive for stable and we are yet to
> have drivers actually handling these errors, so let's wait a bit
> and see.
> 

Fair enough.

> I'll apply this to master for now.
> 
> 

Thanks.

--
Greg
Re: [Qemu-devel] [PATCH] virtio: allow broken device to notify guest
Posted by Michael S. Tsirkin 6 years, 12 months ago 
On Thu, Apr 27, 2017 at 09:03:37AM +0200, Greg Kurz wrote:
> On Thu, 27 Apr 2017 02:42:56 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Apr 26, 2017 at 03:29:46PM +0200, Greg Kurz wrote:
> > > On Wed, 26 Apr 2017 15:15:48 +0200
> > > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > >   
> > > > On Wed, 26 Apr 2017 14:51:17 +0200
> > > > Greg Kurz <groug@kaod.org> 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>
> > > > > ---
> > > > >  hw/virtio/virtio.c |    4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > 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)
> > > > >     
> > > > 
> > > > Good catch.
> > > > 
> > > > Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > > > 
> > > > Should this be cc:stable, as it's a spec violation?
> > > >   
> > > 
> > > I don't know if this qualifies for stable, but if it does then it affects
> > > all versions >= 2.8.0.  
> > 
> > 
> > It's a SHOULD so not a violation, just a quality of implementation
> 
> Setting DEVICE_NEEDS_RESET is indeed a SHOULD, but failing to send the
> configuration change notification violates a MUST statement, FWIW.

OK I'm convinced it's a -stable material.

> > issue. Seems a bit too intrusive for stable and we are yet to
> > have drivers actually handling these errors, so let's wait a bit
> > and see.
> > 
> 
> Fair enough.
> 
> > I'll apply this to master for now.
> > 
> > 
> 
> Thanks.
> 
> --
> Greg

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.