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

[Greg Kurz <groug@kaod.org>]

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

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.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]