On Thu, 27 Apr 2017 02:42:56 +0300 "Michael S. Tsirkin" <m...@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.h...@de.ibm.com> wrote:
> >
> > > On Wed, 26 Apr 2017 14:51:17 +0200
> > > Greg Kurz <gr...@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 <gr...@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.h...@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
pgplTfrYpAIA5.pgp
Description: OpenPGP digital signature
