From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52015) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6e4N-0002R8-86 for qemu-devel@nongnu.org; Tue, 15 Nov 2016 08:46:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c6e4L-0002t6-QD for qemu-devel@nongnu.org; Tue, 15 Nov 2016 08:46:39 -0500 Received: from mail-wm0-x242.google.com ([2a00:1450:400c:c09::242]:35204) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1c6e4L-0002sU-BN for qemu-devel@nongnu.org; Tue, 15 Nov 2016 08:46:37 -0500 Received: by mail-wm0-x242.google.com with SMTP id a20so26127566wme.2 for ; Tue, 15 Nov 2016 05:46:37 -0800 (PST) Sender: Paolo Bonzini From: Paolo Bonzini Date: Tue, 15 Nov 2016 14:46:29 +0100 Message-Id: <20161115134629.23161-4-pbonzini@redhat.com> In-Reply-To: <20161115134629.23161-1-pbonzini@redhat.com> References: <20161115134629.23161-1-pbonzini@redhat.com> Subject: [Qemu-devel] [PATCH 3/3] virtio: set ISR on dataplane notifications List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: mst@redhat.com, alex.williamson@redhat.com, borntraeger@de.ibm.com, felipe@nutanix.com Dataplane has been omitting forever the step of setting ISR when an interrupt is raised. This causes surprisingly little breakage, because most polling-mode drivers look at the used ring's index field rather than the ISR register. Some versions of the Windows drivers are an exception---and they use polling mode with ISR for crashdump and hibernation. And because recent releases of Windows do not really shut down, but rather log out and hibernate to make the next startup faster, this manifested as a hang during shutdown with e.g. Windows 8.1 and virtio-win 1.8.0 RPMs. Newer versions probably poll the used index; older versions do not use MSI and therefore go through the emulated irqfd path (virtio_queue_guest_notifier_read), which handled ISR correctly. 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), which removed the non-dataplane ioeventfd path for virtio-blk and virtio-scsi. The good news therefore is that it was not a bug in the patches---they did exactly what they were meant for, i.e. shake out remaining dataplane bugs. The fix is not hard. 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 --- 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 | 22 +++++++++++++--------- include/hw/virtio/virtio-scsi.h | 1 - include/hw/virtio/virtio.h | 2 +- 7 files changed, 17 insertions(+), 23 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 35255ad..b43fe5a 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); + event_notifier_set(&vq->guest_notifier); +} + void virtio_notify(VirtIODevice *vdev, VirtQueue *vq) { if (!virtio_should_notify(vdev, vq)) { @@ -1372,7 +1376,7 @@ void virtio_notify_config(VirtIODevice *vdev) if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) return; - virtio_set_isr(vq->vdev, 0x3); + virtio_set_isr(vdev, 0x3); vdev->generation++; virtio_notify_vector(vdev, vdev->config_vector); } @@ -2001,7 +2005,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 35ede30..f4bface 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -177,6 +177,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); @@ -278,7 +279,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