On Wed, Nov 16, 2016 at 07:05:51PM +0100, Paolo Bonzini wrote:
> Dataplane has been omitting forever the step of setting ISR when
> an interrupt is raised. This caused little breakage, because the
> specification actually says that ISR may not be updated in MSI mode.
>
> Some versions of the Windows drivers however didn't clear MSI mode
> correctly, and proceeded using polling mode (using ISR, not the used
> ring index!) for crashdump and hibernation. If it were just crashdump
> and hibernation it would not be a big deal, but recent releases of
> Windows do not really shut down, but rather log out and hibernate to
> make the next startup faster. Hence, this manifested as a more serious
> hang during shutdown with e.g. Windows 8.1 and virtio-win 1.8.0 RPMs.
> Newer versions fixed this, while older versions do not use MSI at all.
>
> The failure has always been there for virtio dataplane, but it became
> visible after commits 9ffe337 ("virtio-blk: always use dataplane path
> if ioeventfd is active", 2016-10-30) and ad07cd6 ("virtio-scsi: always
> use dataplane path if ioeventfd is active", 2016-10-30) made virtio-blk
> and virtio-scsi always use the dataplane code under KVM. The good news
> therefore is that it was not a bug in the patches---they were doing
> exactly what they were meant for, i.e. shake out remaining dataplane bugs.
>
> The fix is not hard, so it's worth arranging for the broken drivers.
> The virtio_should_notify+event_notifier_set pair that is common to
> virtio-blk and virtio-scsi dataplane is replaced with a new public
> function virtio_notify_irqfd that also sets ISR. The irqfd emulation
> code now need not set ISR anymore, so virtio_irq is removed.
>
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> ---
> hw/block/dataplane/virtio-blk.c | 4 +---
> hw/scsi/virtio-scsi-dataplane.c | 7 -------
> hw/scsi/virtio-scsi.c | 2 +-
> hw/virtio/trace-events | 2 +-
> hw/virtio/virtio.c | 20 ++++++++++++--------
> include/hw/virtio/virtio-scsi.h | 1 -
> include/hw/virtio/virtio.h | 2 +-
> 7 files changed, 16 insertions(+), 22 deletions(-)
>
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 90ef557..d1f9f63 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -68,9 +68,7 @@ static void notify_guest_bh(void *opaque)
> unsigned i = j + ctzl(bits);
> VirtQueue *vq = virtio_get_queue(s->vdev, i);
>
> - if (virtio_should_notify(s->vdev, vq)) {
> - event_notifier_set(virtio_queue_get_guest_notifier(vq));
> - }
> + virtio_notify_irqfd(s->vdev, vq);
>
> bits &= bits - 1; /* clear right-most bit */
> }
> diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
> index f2ea29d..6b8d0f0 100644
> --- a/hw/scsi/virtio-scsi-dataplane.c
> +++ b/hw/scsi/virtio-scsi-dataplane.c
> @@ -95,13 +95,6 @@ static int virtio_scsi_vring_init(VirtIOSCSI *s, VirtQueue
> *vq, int n,
> return 0;
> }
>
> -void virtio_scsi_dataplane_notify(VirtIODevice *vdev, VirtIOSCSIReq *req)
> -{
> - if (virtio_should_notify(vdev, req->vq)) {
> - event_notifier_set(virtio_queue_get_guest_notifier(req->vq));
> - }
> -}
> -
> /* assumes s->ctx held */
> static void virtio_scsi_clear_aio(VirtIOSCSI *s)
> {
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 3e5ae6a..10fd687 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -69,7 +69,7 @@ static void virtio_scsi_complete_req(VirtIOSCSIReq *req)
> qemu_iovec_from_buf(&req->resp_iov, 0, &req->resp, req->resp_size);
> virtqueue_push(vq, &req->elem, req->qsgl.size + req->resp_iov.size);
> if (s->dataplane_started && !s->dataplane_fenced) {
> - virtio_scsi_dataplane_notify(vdev, req);
> + virtio_notify_irqfd(vdev, vq);
> } else {
> virtio_notify(vdev, vq);
> }
> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> index 8756cef..7b6f55e 100644
> --- a/hw/virtio/trace-events
> +++ b/hw/virtio/trace-events
> @@ -5,7 +5,7 @@ virtqueue_fill(void *vq, const void *elem, unsigned int len,
> unsigned int idx) "
> virtqueue_flush(void *vq, unsigned int count) "vq %p count %u"
> virtqueue_pop(void *vq, void *elem, unsigned int in_num, unsigned int
> out_num) "vq %p elem %p in_num %u out_num %u"
> virtio_queue_notify(void *vdev, int n, void *vq) "vdev %p n %d vq %p"
> -virtio_irq(void *vq) "vq %p"
> +virtio_notify_irqfd(void *vdev, void *vq) "vdev %p vq %p"
> virtio_notify(void *vdev, void *vq) "vdev %p vq %p"
> virtio_set_status(void *vdev, uint8_t val) "vdev %p val %u"
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index ecf13bd..860ebdb 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1326,13 +1326,6 @@ static void virtio_set_isr(VirtIODevice *vdev, int
> value)
> }
> }
>
> -void virtio_irq(VirtQueue *vq)
> -{
> - trace_virtio_irq(vq);
> - virtio_set_isr(vq->vdev, 0x1);
> - virtio_notify_vector(vq->vdev, vq->vector);
> -}
> -
> bool virtio_should_notify(VirtIODevice *vdev, VirtQueue *vq)
> {
> uint16_t old, new;
> @@ -1356,6 +1349,17 @@ bool virtio_should_notify(VirtIODevice *vdev,
> VirtQueue *vq)
> return !v || vring_need_event(vring_get_used_event(vq), new, old);
> }
>
> +void virtio_notify_irqfd(VirtIODevice *vdev, VirtQueue *vq)
> +{
> + if (!virtio_should_notify(vdev, vq)) {
> + return;
> + }
> +
> + trace_virtio_notify_irqfd(vdev, vq);
> + virtio_set_isr(vq->vdev, 0x1);
So here, I think we need a comment with parts of
the commit log.
/*
* virtio spec 1.0 says ISR bit 0 should be ignored with MSI, but
* windows drivers included in virtio-win 1.8.0 (circa 2015)
* for Windows 8.1 only are incorrectly polling this bit during shutdown
* in MSI mode, causing a hang if this bit is never updated.
* Next driver release from 2016 fixed this problem, so working around it
* is not a must, but it's easy to do so let's do it here.
*
* Note: it's safe to update ISR from any thread as it was switched
* to an atomic operation.
*/
> + event_notifier_set(&vq->guest_notifier);
> +}
> +
> void virtio_notify(VirtIODevice *vdev, VirtQueue *vq)
> {
> if (!virtio_should_notify(vdev, vq)) {
> @@ -1990,7 +1994,7 @@ static void
> virtio_queue_guest_notifier_read(EventNotifier *n)
> {
> VirtQueue *vq = container_of(n, VirtQueue, guest_notifier);
> if (event_notifier_test_and_clear(n)) {
> - virtio_irq(vq);
> + virtio_notify_vector(vq->vdev, vq->vector);
> }
> }
>
> diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
> index 9fbc7d7..7375196 100644
> --- a/include/hw/virtio/virtio-scsi.h
> +++ b/include/hw/virtio/virtio-scsi.h
> @@ -137,6 +137,5 @@ void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice
> *dev,
> void virtio_scsi_dataplane_setup(VirtIOSCSI *s, Error **errp);
> int virtio_scsi_dataplane_start(VirtIODevice *s);
> void virtio_scsi_dataplane_stop(VirtIODevice *s);
> -void virtio_scsi_dataplane_notify(VirtIODevice *vdev, VirtIOSCSIReq *req);
>
> #endif /* QEMU_VIRTIO_SCSI_H */
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 835b085..ab0e030 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -181,6 +181,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned
> int *in_bytes,
> unsigned max_in_bytes, unsigned
> max_out_bytes);
>
> bool virtio_should_notify(VirtIODevice *vdev, VirtQueue *vq);
> +void virtio_notify_irqfd(VirtIODevice *vdev, VirtQueue *vq);
> void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
>
> void virtio_save(VirtIODevice *vdev, QEMUFile *f);
> @@ -280,7 +281,6 @@ void virtio_queue_host_notifier_read(EventNotifier *n);
> void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext
> *ctx,
> void (*fn)(VirtIODevice *,
> VirtQueue *));
> -void virtio_irq(VirtQueue *vq);
> VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t vector);
> VirtQueue *virtio_vector_next_queue(VirtQueue *vq);
>
> --
> 2.9.3
