Re: [Qemu-devel] [PATCH 3/3] virtio: set ISR on dataplane notifications

[Michael S. Tsirkin]

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 <address@hidden>
> ---
>  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