[Qemu-devel] [PATCH for-2.8 0/3] virtio fixes

[Qemu-devel] [PATCH for-2.8 0/3] virtio fixes

[Paolo Bonzini]

Patch 1 fixes vhost, patches 2-3 fix Windows hibernation.

Paolo

Paolo Bonzini (3):
  virtio: introduce grab/release_ioeventfd to fix vhost
  virtio: access ISR atomically
  virtio: set ISR on dataplane notifications

 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/vhost.c               | 11 +++------
 hw/virtio/virtio-bus.c          | 54 ++++++++++++++++++++++++++++++++---------
 hw/virtio/virtio-mmio.c         |  6 ++---
 hw/virtio/virtio-pci.c          |  9 +++----
 hw/virtio/virtio.c              | 46 ++++++++++++++++++++++++++++-------
 include/hw/virtio/virtio-bus.h  | 14 +++++++++++
 include/hw/virtio/virtio-scsi.h |  1 -
 include/hw/virtio/virtio.h      |  4 ++-
 12 files changed, 110 insertions(+), 50 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH 1/3] virtio: introduce grab/release_ioeventfd to fix vhost

[Paolo Bonzini]

Following the recent refactoring of virtio notifiers [1], more specifically
the patch ed08a2a0b ("virtio: use virtio_bus_set_host_notifier to
start/stop ioeventfd") that uses virtio_bus_set_host_notifier [2]
by default, core virtio code requires 'ioeventfd_started' to be set
to true/false when the host notifiers are configured.

When vhost is stopped and started, however, there is a stop followed by
another start. Since ioeventfd_started was never set to true, the 'stop'
operation triggered by virtio_bus_set_host_notifier() will not result
in a call to virtio_pci_ioeventfd_assign(assign=false). This leaves
the memory regions with stale notifiers and results on the next start
triggering the following assertion:

  kvm_mem_ioeventfd_add: error adding ioeventfd: File exists
  Aborted

This patch reintroduces (hopefully in a cleaner way) the concept
that was present with ioeventfd_disabled before the refactoring.
When ioeventfd_grabbed>0, ioeventfd_started tracks whether ioeventfd
should be enabled or not, but ioeventfd is actually not started at
all until vhost releases the host notifiers.

[1] http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg07748.html
[2] http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg07760.html

Reported-by: Felipe Franciosi <felipe@nutanix.com>
Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reported-by: Alex Williamson <alex.williamson@redhat.com>
Fixes: ed08a2a0b ("virtio: use virtio_bus_set_host_notifier to start/stop ioeventfd")
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <20161111192855.26350-1-pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio/vhost.c              | 11 ++++-----
 hw/virtio/virtio-bus.c         | 54 +++++++++++++++++++++++++++++++++---------
 hw/virtio/virtio.c             | 16 +++++++++++++
 include/hw/virtio/virtio-bus.h | 14 +++++++++++
 include/hw/virtio/virtio.h     |  2 ++
 5 files changed, 79 insertions(+), 18 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 131f164..a8b5ab8 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1186,17 +1186,14 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
 int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
 {
     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
-    VirtioBusState *vbus = VIRTIO_BUS(qbus);
-    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
     int i, r, e;
 
-    if (!k->ioeventfd_assign) {
+    r = virtio_device_grab_ioeventfd(vdev);
+    if (r < 0) {
         error_report("binding does not support host notifiers");
-        r = -ENOSYS;
         goto fail;
     }
 
-    virtio_device_stop_ioeventfd(vdev);
     for (i = 0; i < hdev->nvqs; ++i) {
         r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i,
                                          true);
@@ -1216,7 +1213,7 @@ fail_vq:
         }
         assert (e >= 0);
     }
-    virtio_device_start_ioeventfd(vdev);
+    virtio_device_release_ioeventfd(vdev);
 fail:
     return r;
 }
@@ -1239,7 +1236,7 @@ void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
         }
         assert (r >= 0);
     }
-    virtio_device_start_ioeventfd(vdev);
+    virtio_device_release_ioeventfd(vdev);
 }
 
 /* Test and clear event pending status.
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index bf61f66..c8a446e 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -147,6 +147,38 @@ void virtio_bus_set_vdev_config(VirtioBusState *bus, uint8_t *config)
     }
 }
 
+int virtio_bus_grab_ioeventfd(VirtioBusState *bus)
+{
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
+
+    /* vhost can be used even if ioeventfd=off in the proxy device,
+     * so do not check k->ioeventfd_enabled.
+     */
+    if (!k->ioeventfd_assign) {
+        return -ENOSYS;
+    }
+
+    if (bus->ioeventfd_grabbed == 0 && bus->ioeventfd_started) {
+        virtio_bus_stop_ioeventfd(bus);
+        /* Remember that we need to restart ioeventfd
+         * when ioeventfd_grabbed becomes zero.
+         */
+        bus->ioeventfd_started = true;
+    }
+    bus->ioeventfd_grabbed++;
+    return 0;
+}
+
+void virtio_bus_release_ioeventfd(VirtioBusState *bus)
+{
+    assert(bus->ioeventfd_grabbed != 0);
+    if (--bus->ioeventfd_grabbed == 0 && bus->ioeventfd_started) {
+        /* Force virtio_bus_start_ioeventfd to act.  */
+        bus->ioeventfd_started = false;
+        virtio_bus_start_ioeventfd(bus);
+    }
+}
+
 int virtio_bus_start_ioeventfd(VirtioBusState *bus)
 {
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
@@ -161,10 +193,12 @@ int virtio_bus_start_ioeventfd(VirtioBusState *bus)
     if (bus->ioeventfd_started) {
         return 0;
     }
-    r = vdc->start_ioeventfd(vdev);
-    if (r < 0) {
-        error_report("%s: failed. Fallback to userspace (slower).", __func__);
-        return r;
+    if (!bus->ioeventfd_grabbed) {
+        r = vdc->start_ioeventfd(vdev);
+        if (r < 0) {
+            error_report("%s: failed. Fallback to userspace (slower).", __func__);
+            return r;
+        }
     }
     bus->ioeventfd_started = true;
     return 0;
@@ -179,9 +213,11 @@ void virtio_bus_stop_ioeventfd(VirtioBusState *bus)
         return;
     }
 
-    vdev = virtio_bus_get_device(bus);
-    vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
-    vdc->stop_ioeventfd(vdev);
+    if (!bus->ioeventfd_grabbed) {
+        vdev = virtio_bus_get_device(bus);
+        vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
+        vdc->stop_ioeventfd(vdev);
+    }
     bus->ioeventfd_started = false;
 }
 
@@ -211,7 +247,6 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
     }
 
     if (assign) {
-        assert(!bus->ioeventfd_started);
         r = event_notifier_init(notifier, 1);
         if (r < 0) {
             error_report("%s: unable to init event notifier: %s (%d)",
@@ -225,9 +260,6 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
         }
         return 0;
     } else {
-        if (!bus->ioeventfd_started) {
-            return 0;
-        }
         k->ioeventfd_assign(proxy, notifier, n, false);
     }
 
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index bcbcfe0..89b0b80 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2202,6 +2202,22 @@ void virtio_device_stop_ioeventfd(VirtIODevice *vdev)
     virtio_bus_stop_ioeventfd(vbus);
 }
 
+int virtio_device_grab_ioeventfd(VirtIODevice *vdev)
+{
+    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
+    VirtioBusState *vbus = VIRTIO_BUS(qbus);
+
+    return virtio_bus_grab_ioeventfd(vbus);
+}
+
+void virtio_device_release_ioeventfd(VirtIODevice *vdev)
+{
+    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
+    VirtioBusState *vbus = VIRTIO_BUS(qbus);
+
+    virtio_bus_release_ioeventfd(vbus);
+}
+
 static void virtio_device_class_init(ObjectClass *klass, void *data)
 {
     /* Set the default value here. */
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index fdf7fda..8a51e2c 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -97,6 +97,16 @@ struct VirtioBusState {
      * Set if ioeventfd has been started.
      */
     bool ioeventfd_started;
+
+    /*
+     * Set if ioeventfd has been grabbed by vhost.  When ioeventfd
+     * is grabbed by vhost, we track its started/stopped state (which
+     * depends in turn on the virtio status register), but do not
+     * register a handler for the ioeventfd.  When ioeventfd is
+     * released, if ioeventfd_started is true we finally register
+     * the handler so that QEMU's device model can use ioeventfd.
+     */
+    int ioeventfd_grabbed;
 };
 
 void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp);
@@ -131,6 +141,10 @@ bool virtio_bus_ioeventfd_enabled(VirtioBusState *bus);
 int virtio_bus_start_ioeventfd(VirtioBusState *bus);
 /* Stop the ioeventfd. */
 void virtio_bus_stop_ioeventfd(VirtioBusState *bus);
+/* Tell the bus that vhost is grabbing the ioeventfd. */
+int virtio_bus_grab_ioeventfd(VirtioBusState *bus);
+/* bus that vhost is not using the ioeventfd anymore. */
+void virtio_bus_release_ioeventfd(VirtioBusState *bus);
 /* Switch from/to the generic ioeventfd handler */
 int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign);
 
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index ac65d6a..35ede30 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -270,6 +270,8 @@ void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign,
                                                 bool with_irqfd);
 int virtio_device_start_ioeventfd(VirtIODevice *vdev);
 void virtio_device_stop_ioeventfd(VirtIODevice *vdev);
+int virtio_device_grab_ioeventfd(VirtIODevice *vdev);
+void virtio_device_release_ioeventfd(VirtIODevice *vdev);
 bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev);
 EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq);
 void virtio_queue_host_notifier_read(EventNotifier *n);
-- 
2.9.3

[Qemu-devel] [PATCH 2/3] virtio: access ISR atomically

[Paolo Bonzini]

This will be needed once dataplane will be able to set it outside
the big QEMU lock.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio/virtio-mmio.c |  6 +++---
 hw/virtio/virtio-pci.c  |  9 +++------
 hw/virtio/virtio.c      | 18 +++++++++++++-----
 3 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index a30270f..17412cb 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -191,7 +191,7 @@ static uint64_t virtio_mmio_read(void *opaque, hwaddr offset, unsigned size)
         return virtio_queue_get_addr(vdev, vdev->queue_sel)
             >> proxy->guest_page_shift;
     case VIRTIO_MMIO_INTERRUPTSTATUS:
-        return vdev->isr;
+        return atomic_read(&vdev->isr);
     case VIRTIO_MMIO_STATUS:
         return vdev->status;
     case VIRTIO_MMIO_HOSTFEATURESSEL:
@@ -299,7 +299,7 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value,
         }
         break;
     case VIRTIO_MMIO_INTERRUPTACK:
-        vdev->isr &= ~value;
+        atomic_and(&vdev->isr, ~value);
         virtio_update_irq(vdev);
         break;
     case VIRTIO_MMIO_STATUS:
@@ -347,7 +347,7 @@ static void virtio_mmio_update_irq(DeviceState *opaque, uint16_t vector)
     if (!vdev) {
         return;
     }
-    level = (vdev->isr != 0);
+    level = (atomic_read(&vdev->isr) != 0);
     DPRINTF("virtio_mmio setting IRQ %d\n", level);
     qemu_set_irq(proxy->irq, level);
 }
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 62001b4..d5e99b0 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -73,7 +73,7 @@ static void virtio_pci_notify(DeviceState *d, uint16_t vector)
         msix_notify(&proxy->pci_dev, vector);
     else {
         VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
-        pci_set_irq(&proxy->pci_dev, vdev->isr & 1);
+        pci_set_irq(&proxy->pci_dev, atomic_read(&vdev->isr) & 1);
     }
 }
 
@@ -449,8 +449,7 @@ static uint32_t virtio_ioport_read(VirtIOPCIProxy *proxy, uint32_t addr)
         break;
     case VIRTIO_PCI_ISR:
         /* reading from the ISR also clears it. */
-        ret = vdev->isr;
-        vdev->isr = 0;
+        ret = atomic_xchg(&vdev->isr, 0);
         pci_irq_deassert(&proxy->pci_dev);
         break;
     case VIRTIO_MSI_CONFIG_VECTOR:
@@ -1377,9 +1376,7 @@ static uint64_t virtio_pci_isr_read(void *opaque, hwaddr addr,
 {
     VirtIOPCIProxy *proxy = opaque;
     VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
-    uint64_t val = vdev->isr;
-
-    vdev->isr = 0;
+    uint64_t val = atomic_xchg(&vdev->isr, 0);
     pci_irq_deassert(&proxy->pci_dev);
 
     return val;
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 89b0b80..35255ad 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -945,7 +945,7 @@ void virtio_reset(void *opaque)
     vdev->guest_features = 0;
     vdev->queue_sel = 0;
     vdev->status = 0;
-    vdev->isr = 0;
+    atomic_set(&vdev->isr, 0);
     vdev->config_vector = VIRTIO_NO_VECTOR;
     virtio_notify_vector(vdev, vdev->config_vector);
 
@@ -1318,10 +1318,18 @@ void virtio_del_queue(VirtIODevice *vdev, int n)
     vdev->vq[n].vring.num_default = 0;
 }
 
+static void virtio_set_isr(VirtIODevice *vdev, int value)
+{
+    uint8_t old = atomic_read(&vdev->isr);
+    if ((old & value) != value) {
+        atomic_or(&vdev->isr, value);
+    }
+}
+
 void virtio_irq(VirtQueue *vq)
 {
     trace_virtio_irq(vq);
-    vq->vdev->isr |= 0x01;
+    virtio_set_isr(vq->vdev, 0x1);
     virtio_notify_vector(vq->vdev, vq->vector);
 }
 
@@ -1355,7 +1363,7 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq)
     }
 
     trace_virtio_notify(vdev, vq);
-    vdev->isr |= 0x01;
+    virtio_set_isr(vq->vdev, 0x1);
     virtio_notify_vector(vdev, vq->vector);
 }
 
@@ -1364,7 +1372,7 @@ void virtio_notify_config(VirtIODevice *vdev)
     if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK))
         return;
 
-    vdev->isr |= 0x03;
+    virtio_set_isr(vq->vdev, 0x3);
     vdev->generation++;
     virtio_notify_vector(vdev, vdev->config_vector);
 }
@@ -1895,7 +1903,7 @@ void virtio_init(VirtIODevice *vdev, const char *name,
 
     vdev->device_id = device_id;
     vdev->status = 0;
-    vdev->isr = 0;
+    atomic_set(&vdev->isr, 0);
     vdev->queue_sel = 0;
     vdev->config_vector = VIRTIO_NO_VECTOR;
     vdev->vq = g_malloc0(sizeof(VirtQueue) * VIRTIO_QUEUE_MAX);
-- 
2.9.3

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

[Paolo Bonzini]

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 <pbonzini@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              | 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

Re: [Qemu-devel] [PATCH for-2.8 0/3] virtio fixes

[Stefan Hajnoczi]

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

On Tue, Nov 15, 2016 at 02:46:26PM +0100, Paolo Bonzini wrote:
> Patch 1 fixes vhost, patches 2-3 fix Windows hibernation.
> 
> Paolo
> 
> Paolo Bonzini (3):
>   virtio: introduce grab/release_ioeventfd to fix vhost
>   virtio: access ISR atomically
>   virtio: set ISR on dataplane notifications
> 
>  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/vhost.c               | 11 +++------
>  hw/virtio/virtio-bus.c          | 54 ++++++++++++++++++++++++++++++++---------
>  hw/virtio/virtio-mmio.c         |  6 ++---
>  hw/virtio/virtio-pci.c          |  9 +++----
>  hw/virtio/virtio.c              | 46 ++++++++++++++++++++++++++++-------
>  include/hw/virtio/virtio-bus.h  | 14 +++++++++++
>  include/hw/virtio/virtio-scsi.h |  1 -
>  include/hw/virtio/virtio.h      |  4 ++-
>  12 files changed, 110 insertions(+), 50 deletions(-)
> 
> -- 
> 2.9.3
> 
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 2/3] virtio: access ISR atomically

[Christian Borntraeger]

On 11/15/2016 02:46 PM, Paolo Bonzini wrote:
> This will be needed once dataplane will be able to set it outside
> the big QEMU lock.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

This is fixed by the followup patch, but this patch alone gives me

/home/cborntra/REPOS/qemu/hw/virtio/virtio.c: In function ‘virtio_notify_config’:
/home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1375:20: error: ‘vq’ undeclared (first use in this function)
     virtio_set_isr(vq->vdev, 0x3);
                   ^
/home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1375:20: note: each undeclared identifier is reported only once for each function it appears in
/home/cborntra/REPOS/qemu/rules.mak:60: recipe for target 'hw/virtio/virtio.o' failed
make[1]: *** [hw/virtio/virtio.o] Error 1
make[1]: *** Waiting for unfinished jobs....


> ---
>  hw/virtio/virtio-mmio.c |  6 +++---
>  hw/virtio/virtio-pci.c  |  9 +++------
>  hw/virtio/virtio.c      | 18 +++++++++++++-----
>  3 files changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
> index a30270f..17412cb 100644
> --- a/hw/virtio/virtio-mmio.c
> +++ b/hw/virtio/virtio-mmio.c
> @@ -191,7 +191,7 @@ static uint64_t virtio_mmio_read(void *opaque, hwaddr offset, unsigned size)
>          return virtio_queue_get_addr(vdev, vdev->queue_sel)
>              >> proxy->guest_page_shift;
>      case VIRTIO_MMIO_INTERRUPTSTATUS:
> -        return vdev->isr;
> +        return atomic_read(&vdev->isr);
>      case VIRTIO_MMIO_STATUS:
>          return vdev->status;
>      case VIRTIO_MMIO_HOSTFEATURESSEL:
> @@ -299,7 +299,7 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value,
>          }
>          break;
>      case VIRTIO_MMIO_INTERRUPTACK:
> -        vdev->isr &= ~value;
> +        atomic_and(&vdev->isr, ~value);
>          virtio_update_irq(vdev);
>          break;
>      case VIRTIO_MMIO_STATUS:
> @@ -347,7 +347,7 @@ static void virtio_mmio_update_irq(DeviceState *opaque, uint16_t vector)
>      if (!vdev) {
>          return;
>      }
> -    level = (vdev->isr != 0);
> +    level = (atomic_read(&vdev->isr) != 0);
>      DPRINTF("virtio_mmio setting IRQ %d\n", level);
>      qemu_set_irq(proxy->irq, level);
>  }
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 62001b4..d5e99b0 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -73,7 +73,7 @@ static void virtio_pci_notify(DeviceState *d, uint16_t vector)
>          msix_notify(&proxy->pci_dev, vector);
>      else {
>          VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> -        pci_set_irq(&proxy->pci_dev, vdev->isr & 1);
> +        pci_set_irq(&proxy->pci_dev, atomic_read(&vdev->isr) & 1);
>      }
>  }
> 
> @@ -449,8 +449,7 @@ static uint32_t virtio_ioport_read(VirtIOPCIProxy *proxy, uint32_t addr)
>          break;
>      case VIRTIO_PCI_ISR:
>          /* reading from the ISR also clears it. */
> -        ret = vdev->isr;
> -        vdev->isr = 0;
> +        ret = atomic_xchg(&vdev->isr, 0);
>          pci_irq_deassert(&proxy->pci_dev);
>          break;
>      case VIRTIO_MSI_CONFIG_VECTOR:
> @@ -1377,9 +1376,7 @@ static uint64_t virtio_pci_isr_read(void *opaque, hwaddr addr,
>  {
>      VirtIOPCIProxy *proxy = opaque;
>      VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> -    uint64_t val = vdev->isr;
> -
> -    vdev->isr = 0;
> +    uint64_t val = atomic_xchg(&vdev->isr, 0);
>      pci_irq_deassert(&proxy->pci_dev);
> 
>      return val;
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 89b0b80..35255ad 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -945,7 +945,7 @@ void virtio_reset(void *opaque)
>      vdev->guest_features = 0;
>      vdev->queue_sel = 0;
>      vdev->status = 0;
> -    vdev->isr = 0;
> +    atomic_set(&vdev->isr, 0);
>      vdev->config_vector = VIRTIO_NO_VECTOR;
>      virtio_notify_vector(vdev, vdev->config_vector);
> 
> @@ -1318,10 +1318,18 @@ void virtio_del_queue(VirtIODevice *vdev, int n)
>      vdev->vq[n].vring.num_default = 0;
>  }
> 
> +static void virtio_set_isr(VirtIODevice *vdev, int value)
> +{
> +    uint8_t old = atomic_read(&vdev->isr);
> +    if ((old & value) != value) {
> +        atomic_or(&vdev->isr, value);
> +    }
> +}
> +
>  void virtio_irq(VirtQueue *vq)
>  {
>      trace_virtio_irq(vq);
> -    vq->vdev->isr |= 0x01;
> +    virtio_set_isr(vq->vdev, 0x1);
>      virtio_notify_vector(vq->vdev, vq->vector);
>  }
> 
> @@ -1355,7 +1363,7 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq)
>      }
> 
>      trace_virtio_notify(vdev, vq);
> -    vdev->isr |= 0x01;
> +    virtio_set_isr(vq->vdev, 0x1);
>      virtio_notify_vector(vdev, vq->vector);
>  }
> 
> @@ -1364,7 +1372,7 @@ void virtio_notify_config(VirtIODevice *vdev)
>      if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK))
>          return;
> 
> -    vdev->isr |= 0x03;
> +    virtio_set_isr(vq->vdev, 0x3);
>      vdev->generation++;
>      virtio_notify_vector(vdev, vdev->config_vector);
>  }
> @@ -1895,7 +1903,7 @@ void virtio_init(VirtIODevice *vdev, const char *name,
> 
>      vdev->device_id = device_id;
>      vdev->status = 0;
> -    vdev->isr = 0;
> +    atomic_set(&vdev->isr, 0);
>      vdev->queue_sel = 0;
>      vdev->config_vector = VIRTIO_NO_VECTOR;
>      vdev->vq = g_malloc0(sizeof(VirtQueue) * VIRTIO_QUEUE_MAX);
> 

Re: [Qemu-devel] [PATCH 2/3] virtio: access ISR atomically

[Paolo Bonzini]

On 15/11/2016 16:03, Christian Borntraeger wrote:
> On 11/15/2016 02:46 PM, Paolo Bonzini wrote:
>> This will be needed once dataplane will be able to set it outside
>> the big QEMU lock.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> This is fixed by the followup patch, but this patch alone gives me
> 
> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c: In function ‘virtio_notify_config’:
> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1375:20: error: ‘vq’ undeclared (first use in this function)
>      virtio_set_isr(vq->vdev, 0x3);
>                    ^
> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1375:20: note: each undeclared identifier is reported only once for each function it appears in
> /home/cborntra/REPOS/qemu/rules.mak:60: recipe for target 'hw/virtio/virtio.o' failed
> make[1]: *** [hw/virtio/virtio.o] Error 1
> make[1]: *** Waiting for unfinished jobs....

Oops, will post v2.

Paolo

> 
>> ---
>>  hw/virtio/virtio-mmio.c |  6 +++---
>>  hw/virtio/virtio-pci.c  |  9 +++------
>>  hw/virtio/virtio.c      | 18 +++++++++++++-----
>>  3 files changed, 19 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
>> index a30270f..17412cb 100644
>> --- a/hw/virtio/virtio-mmio.c
>> +++ b/hw/virtio/virtio-mmio.c
>> @@ -191,7 +191,7 @@ static uint64_t virtio_mmio_read(void *opaque, hwaddr offset, unsigned size)
>>          return virtio_queue_get_addr(vdev, vdev->queue_sel)
>>              >> proxy->guest_page_shift;
>>      case VIRTIO_MMIO_INTERRUPTSTATUS:
>> -        return vdev->isr;
>> +        return atomic_read(&vdev->isr);
>>      case VIRTIO_MMIO_STATUS:
>>          return vdev->status;
>>      case VIRTIO_MMIO_HOSTFEATURESSEL:
>> @@ -299,7 +299,7 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value,
>>          }
>>          break;
>>      case VIRTIO_MMIO_INTERRUPTACK:
>> -        vdev->isr &= ~value;
>> +        atomic_and(&vdev->isr, ~value);
>>          virtio_update_irq(vdev);
>>          break;
>>      case VIRTIO_MMIO_STATUS:
>> @@ -347,7 +347,7 @@ static void virtio_mmio_update_irq(DeviceState *opaque, uint16_t vector)
>>      if (!vdev) {
>>          return;
>>      }
>> -    level = (vdev->isr != 0);
>> +    level = (atomic_read(&vdev->isr) != 0);
>>      DPRINTF("virtio_mmio setting IRQ %d\n", level);
>>      qemu_set_irq(proxy->irq, level);
>>  }
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index 62001b4..d5e99b0 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -73,7 +73,7 @@ static void virtio_pci_notify(DeviceState *d, uint16_t vector)
>>          msix_notify(&proxy->pci_dev, vector);
>>      else {
>>          VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
>> -        pci_set_irq(&proxy->pci_dev, vdev->isr & 1);
>> +        pci_set_irq(&proxy->pci_dev, atomic_read(&vdev->isr) & 1);
>>      }
>>  }
>>
>> @@ -449,8 +449,7 @@ static uint32_t virtio_ioport_read(VirtIOPCIProxy *proxy, uint32_t addr)
>>          break;
>>      case VIRTIO_PCI_ISR:
>>          /* reading from the ISR also clears it. */
>> -        ret = vdev->isr;
>> -        vdev->isr = 0;
>> +        ret = atomic_xchg(&vdev->isr, 0);
>>          pci_irq_deassert(&proxy->pci_dev);
>>          break;
>>      case VIRTIO_MSI_CONFIG_VECTOR:
>> @@ -1377,9 +1376,7 @@ static uint64_t virtio_pci_isr_read(void *opaque, hwaddr addr,
>>  {
>>      VirtIOPCIProxy *proxy = opaque;
>>      VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
>> -    uint64_t val = vdev->isr;
>> -
>> -    vdev->isr = 0;
>> +    uint64_t val = atomic_xchg(&vdev->isr, 0);
>>      pci_irq_deassert(&proxy->pci_dev);
>>
>>      return val;
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 89b0b80..35255ad 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -945,7 +945,7 @@ void virtio_reset(void *opaque)
>>      vdev->guest_features = 0;
>>      vdev->queue_sel = 0;
>>      vdev->status = 0;
>> -    vdev->isr = 0;
>> +    atomic_set(&vdev->isr, 0);
>>      vdev->config_vector = VIRTIO_NO_VECTOR;
>>      virtio_notify_vector(vdev, vdev->config_vector);
>>
>> @@ -1318,10 +1318,18 @@ void virtio_del_queue(VirtIODevice *vdev, int n)
>>      vdev->vq[n].vring.num_default = 0;
>>  }
>>
>> +static void virtio_set_isr(VirtIODevice *vdev, int value)
>> +{
>> +    uint8_t old = atomic_read(&vdev->isr);
>> +    if ((old & value) != value) {
>> +        atomic_or(&vdev->isr, value);
>> +    }
>> +}
>> +
>>  void virtio_irq(VirtQueue *vq)
>>  {
>>      trace_virtio_irq(vq);
>> -    vq->vdev->isr |= 0x01;
>> +    virtio_set_isr(vq->vdev, 0x1);
>>      virtio_notify_vector(vq->vdev, vq->vector);
>>  }
>>
>> @@ -1355,7 +1363,7 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq)
>>      }
>>
>>      trace_virtio_notify(vdev, vq);
>> -    vdev->isr |= 0x01;
>> +    virtio_set_isr(vq->vdev, 0x1);
>>      virtio_notify_vector(vdev, vq->vector);
>>  }
>>
>> @@ -1364,7 +1372,7 @@ void virtio_notify_config(VirtIODevice *vdev)
>>      if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK))
>>          return;
>>
>> -    vdev->isr |= 0x03;
>> +    virtio_set_isr(vq->vdev, 0x3);
>>      vdev->generation++;
>>      virtio_notify_vector(vdev, vdev->config_vector);
>>  }
>> @@ -1895,7 +1903,7 @@ void virtio_init(VirtIODevice *vdev, const char *name,
>>
>>      vdev->device_id = device_id;
>>      vdev->status = 0;
>> -    vdev->isr = 0;
>> +    atomic_set(&vdev->isr, 0);
>>      vdev->queue_sel = 0;
>>      vdev->config_vector = VIRTIO_NO_VECTOR;
>>      vdev->vq = g_malloc0(sizeof(VirtQueue) * VIRTIO_QUEUE_MAX);
>>
> 

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

[Michael S. Tsirkin]

On Tue, Nov 15, 2016 at 02:46:29PM +0100, Paolo Bonzini wrote:
> 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.

Confused. virtio spec says ISR shouldn't be set on
ring activity in MSI mode. Is this a driver bug?


> 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 <pbonzini@redhat.com>

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

[Paolo Bonzini]

On 15/11/2016 16:26, Michael S. Tsirkin wrote:
> On Tue, Nov 15, 2016 at 02:46:29PM +0100, Paolo Bonzini wrote:
>> 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.
> 
> Confused. virtio spec says ISR shouldn't be set on
> ring activity in MSI mode. Is this a driver bug?

Huh, then probably it is, and this is why it works with newer versions
of the driver.  They probably forgot to disable MSI mode when entering
hibernation mode.

But in the end QEMU is doing different things in non-dataplane vs.
dataplane mode, at least for virtio-blk and virtio-scsi (I'm sure
virtio-net vs. vhost would have the same issue, just no driver that is
affected).  Is it SHOULD NOT or MUST NOT or MAY NOT?

Paolo

> 
>> 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 <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 1/3] virtio: introduce grab/release_ioeventfd to fix vhost

[Cornelia Huck]

On Tue, 15 Nov 2016 14:46:27 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Following the recent refactoring of virtio notifiers [1], more specifically
> the patch ed08a2a0b ("virtio: use virtio_bus_set_host_notifier to
> start/stop ioeventfd") that uses virtio_bus_set_host_notifier [2]
> by default, core virtio code requires 'ioeventfd_started' to be set
> to true/false when the host notifiers are configured.
> 
> When vhost is stopped and started, however, there is a stop followed by
> another start. Since ioeventfd_started was never set to true, the 'stop'
> operation triggered by virtio_bus_set_host_notifier() will not result
> in a call to virtio_pci_ioeventfd_assign(assign=false). This leaves
> the memory regions with stale notifiers and results on the next start
> triggering the following assertion:
> 
>   kvm_mem_ioeventfd_add: error adding ioeventfd: File exists
>   Aborted
> 
> This patch reintroduces (hopefully in a cleaner way) the concept
> that was present with ioeventfd_disabled before the refactoring.
> When ioeventfd_grabbed>0, ioeventfd_started tracks whether ioeventfd
> should be enabled or not, but ioeventfd is actually not started at
> all until vhost releases the host notifiers.
> 
> [1] http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg07748.html
> [2] http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg07760.html
> 
> Reported-by: Felipe Franciosi <felipe@nutanix.com>
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Reported-by: Alex Williamson <alex.williamson@redhat.com>
> Fixes: ed08a2a0b ("virtio: use virtio_bus_set_host_notifier to start/stop ioeventfd")
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Message-Id: <20161111192855.26350-1-pbonzini@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/virtio/vhost.c              | 11 ++++-----
>  hw/virtio/virtio-bus.c         | 54 +++++++++++++++++++++++++++++++++---------
>  hw/virtio/virtio.c             | 16 +++++++++++++
>  include/hw/virtio/virtio-bus.h | 14 +++++++++++
>  include/hw/virtio/virtio.h     |  2 ++
>  5 files changed, 79 insertions(+), 18 deletions(-)

This basically looks sane to me, but it is really hard to wrap one's
brain around this, so maybe this needs more comments? (I have added
some suggestions.)

> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 131f164..a8b5ab8 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1186,17 +1186,14 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
>  int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
>  {
>      BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
> -    VirtioBusState *vbus = VIRTIO_BUS(qbus);
> -    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
>      int i, r, e;
> 
> -    if (!k->ioeventfd_assign) {

/* About to use our notifiers; make sure the core doesn't interfere. */

> +    r = virtio_device_grab_ioeventfd(vdev);
> +    if (r < 0) {
>          error_report("binding does not support host notifiers");
> -        r = -ENOSYS;
>          goto fail;
>      }
> 
> -    virtio_device_stop_ioeventfd(vdev);
>      for (i = 0; i < hdev->nvqs; ++i) {
>          r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i,
>                                           true);

(...)

> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index bf61f66..c8a446e 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -147,6 +147,38 @@ void virtio_bus_set_vdev_config(VirtioBusState *bus, uint8_t *config)
>      }
>  }
> 

/* On success, ioeventfd ownership belongs to the caller. */

> +int virtio_bus_grab_ioeventfd(VirtioBusState *bus)
> +{
> +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
> +
> +    /* vhost can be used even if ioeventfd=off in the proxy device,
> +     * so do not check k->ioeventfd_enabled.
> +     */
> +    if (!k->ioeventfd_assign) {
> +        return -ENOSYS;
> +    }
> +
> +    if (bus->ioeventfd_grabbed == 0 && bus->ioeventfd_started) {
> +        virtio_bus_stop_ioeventfd(bus);
> +        /* Remember that we need to restart ioeventfd
> +         * when ioeventfd_grabbed becomes zero.
> +         */
> +        bus->ioeventfd_started = true;
> +    }
> +    bus->ioeventfd_grabbed++;
> +    return 0;
> +}
> +
> +void virtio_bus_release_ioeventfd(VirtioBusState *bus)
> +{
> +    assert(bus->ioeventfd_grabbed != 0);
> +    if (--bus->ioeventfd_grabbed == 0 && bus->ioeventfd_started) {
> +        /* Force virtio_bus_start_ioeventfd to act.  */
> +        bus->ioeventfd_started = false;
> +        virtio_bus_start_ioeventfd(bus);
> +    }
> +}
> +
>  int virtio_bus_start_ioeventfd(VirtioBusState *bus)
>  {
>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
> @@ -161,10 +193,12 @@ int virtio_bus_start_ioeventfd(VirtioBusState *bus)
>      if (bus->ioeventfd_started) {
>          return 0;
>      }
> -    r = vdc->start_ioeventfd(vdev);
> -    if (r < 0) {
> -        error_report("%s: failed. Fallback to userspace (slower).", __func__);
> -        return r;

/* Only set our notifier if we have ownership. */

> +    if (!bus->ioeventfd_grabbed) {
> +        r = vdc->start_ioeventfd(vdev);
> +        if (r < 0) {
> +            error_report("%s: failed. Fallback to userspace (slower).", __func__);
> +            return r;
> +        }
>      }
>      bus->ioeventfd_started = true;
>      return 0;
> @@ -179,9 +213,11 @@ void virtio_bus_stop_ioeventfd(VirtioBusState *bus)
>          return;
>      }
> 
> -    vdev = virtio_bus_get_device(bus);
> -    vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
> -    vdc->stop_ioeventfd(vdev);

/* Only remove our notifiers if we have ownership. */

> +    if (!bus->ioeventfd_grabbed) {
> +        vdev = virtio_bus_get_device(bus);
> +        vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
> +        vdc->stop_ioeventfd(vdev);
> +    }
>      bus->ioeventfd_started = false;
>  }
> 

(...)

> diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
> index fdf7fda..8a51e2c 100644
> --- a/include/hw/virtio/virtio-bus.h
> +++ b/include/hw/virtio/virtio-bus.h
> @@ -97,6 +97,16 @@ struct VirtioBusState {
>       * Set if ioeventfd has been started.

        * This is independent of who has ioeventfd ownership (core or vhost).

>       */
>      bool ioeventfd_started;
> +
> +    /*
> +     * Set if ioeventfd has been grabbed by vhost.  When ioeventfd
> +     * is grabbed by vhost, we track its started/stopped state (which
> +     * depends in turn on the virtio status register), but do not
> +     * register a handler for the ioeventfd.  When ioeventfd is
> +     * released, if ioeventfd_started is true we finally register
> +     * the handler so that QEMU's device model can use ioeventfd.
> +     */
> +    int ioeventfd_grabbed;
>  };
> 
>  void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp);

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

[Michael S. Tsirkin]

On Tue, Nov 15, 2016 at 04:28:37PM +0100, Paolo Bonzini wrote:
> 
> 
> On 15/11/2016 16:26, Michael S. Tsirkin wrote:
> > On Tue, Nov 15, 2016 at 02:46:29PM +0100, Paolo Bonzini wrote:
> >> 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.
> > 
> > Confused. virtio spec says ISR shouldn't be set on
> > ring activity in MSI mode. Is this a driver bug?
> 
> Huh, then probably it is, and this is why it works with newer versions
> of the driver.  They probably forgot to disable MSI mode when entering
> hibernation mode.
> 
> But in the end QEMU is doing different things in non-dataplane vs.
> dataplane mode, at least for virtio-blk and virtio-scsi (I'm sure
> virtio-net vs. vhost would have the same issue, just no driver that is
> affected).  Is it SHOULD NOT or MUST NOT or MAY NOT?
> 
> Paolo

True. We could drop it from non-data plane, it's just that we never had
a reason to. vhost in kernel does not set ISR in MSI mode, either.


What we have in spec is:

The device MUST set the Device Configuration Interrupt bit in ISR status
before sending a device configu-
ration change notification to the driver.
If MSI-X capability is disabled, the device MUST set the Queue Interrupt
bit in ISR status before sending a
virtqueue notification to the driver.
If MSI-X capability is disabled, the device MUST set the Interrupt
Status bit in the PCI Status register in the
PCI Configuration Header of the device to the logical OR of all bits in
ISR status of the device. The device
then asserts/deasserts INT#x interrupts unless masked according to
standard PCI rules [PCI].
The device MUST reset ISR status to 0 on driver read.




If MSI-X capability is enabled, the driver SHOULD NOT access ISR status
upon detecting a Queue Interrupt.



It can be clearer, but IMHO it's reasonably clear that devices
do not have to set this bit in MSI mode.


> > 
> >> 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 <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 1/3] virtio: introduce grab/release_ioeventfd to fix vhost

[Paolo Bonzini]

On 15/11/2016 16:32, Cornelia Huck wrote:
> This basically looks sane to me, but it is really hard to wrap one's
> brain around this, so maybe this needs more comments? (I have added
> some suggestions.)

Very good - I'll include them in v2.

Paolo

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

[Paolo Bonzini]

On 15/11/2016 16:44, Michael S. Tsirkin wrote:
> True. We could drop it from non-data plane, it's just that we never had
> a reason to. vhost in kernel does not set ISR in MSI mode, either.

Yeah, I suspected that.  But dropping it from non-dataplane would break
Windows hibernation and crashdump, just like it did for Alex.

> What we have in spec is:
> 
> The device MUST set the Device Configuration Interrupt bit in ISR status
> before sending a device configu-
> ration change notification to the driver.
> If MSI-X capability is disabled, the device MUST set the Queue Interrupt
> bit in ISR status before sending a
> virtqueue notification to the driver.
> If MSI-X capability is disabled, the device MUST set the Interrupt
> Status bit in the PCI Status register in the
> PCI Configuration Header of the device to the logical OR of all bits in
> ISR status of the device. The device
> then asserts/deasserts INT#x interrupts unless masked according to
> standard PCI rules [PCI].
> The device MUST reset ISR status to 0 on driver read.
> 
> 
> 
> 
> If MSI-X capability is enabled, the driver SHOULD NOT access ISR status
> upon detecting a Queue Interrupt.
> 
> 
> 
> It can be clearer, but IMHO it's reasonably clear that devices
> do not have to set this bit in MSI mode.

Yes, it is.  We can just document it in the release notes, but then the
fix is not particularly intrusive.

Paolo

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

[Michael S. Tsirkin]

On Tue, Nov 15, 2016 at 05:22:49PM +0100, Paolo Bonzini wrote:
> 
> 
> On 15/11/2016 16:44, Michael S. Tsirkin wrote:
> > True. We could drop it from non-data plane, it's just that we never had
> > a reason to. vhost in kernel does not set ISR in MSI mode, either.
> 
> Yeah, I suspected that.  But dropping it from non-dataplane would break
> Windows hibernation and crashdump, just like it did for Alex.

I guess it's just a question of updating the drivers,
isn't it? To me, hibernation/crashdump doesn't sound important
enough to warrant work-arounds, but if you feel otherwise,
I'm fine with doing this work-around for dataplane.


> > What we have in spec is:
> > 
> > The device MUST set the Device Configuration Interrupt bit in ISR status
> > before sending a device configu-
> > ration change notification to the driver.
> > If MSI-X capability is disabled, the device MUST set the Queue Interrupt
> > bit in ISR status before sending a
> > virtqueue notification to the driver.
> > If MSI-X capability is disabled, the device MUST set the Interrupt
> > Status bit in the PCI Status register in the
> > PCI Configuration Header of the device to the logical OR of all bits in
> > ISR status of the device. The device
> > then asserts/deasserts INT#x interrupts unless masked according to
> > standard PCI rules [PCI].
> > The device MUST reset ISR status to 0 on driver read.
> > 
> > 
> > 
> > 
> > If MSI-X capability is enabled, the driver SHOULD NOT access ISR status
> > upon detecting a Queue Interrupt.
> > 
> > 
> > 
> > It can be clearer, but IMHO it's reasonably clear that devices
> > do not have to set this bit in MSI mode.
> 
> Yes, it is.  We can just document it in the release notes, but then the
> fix is not particularly intrusive.
> 
> Paolo

It has a slight performance cost but it's not too bad.

I'd rather document it in a code comment though.
Explain the motivation and which driver versions are affected.

-- 
MST

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

[Alex Williamson]

On Tue, 15 Nov 2016 19:38:30 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Nov 15, 2016 at 05:22:49PM +0100, Paolo Bonzini wrote:
> > 
> > 
> > On 15/11/2016 16:44, Michael S. Tsirkin wrote:  
> > > True. We could drop it from non-data plane, it's just that we never had
> > > a reason to. vhost in kernel does not set ISR in MSI mode, either.  
> > 
> > Yeah, I suspected that.  But dropping it from non-dataplane would break
> > Windows hibernation and crashdump, just like it did for Alex.  
> 
> I guess it's just a question of updating the drivers,
> isn't it? To me, hibernation/crashdump doesn't sound important
> enough to warrant work-arounds, but if you feel otherwise,
> I'm fine with doing this work-around for dataplane.

The fact that Windows is trying to do some sort of hibernation is not
visible to the user, I'm simply trying to shutdown the VM.  That's
rather important on my scale of functionality.  If we have drivers in
the wild doing this, does it matter what's in the spec?  Smells like a
regression from an end user perspective. Thanks,

Alex

> > > What we have in spec is:
> > > 
> > > The device MUST set the Device Configuration Interrupt bit in ISR status
> > > before sending a device configu-
> > > ration change notification to the driver.
> > > If MSI-X capability is disabled, the device MUST set the Queue Interrupt
> > > bit in ISR status before sending a
> > > virtqueue notification to the driver.
> > > If MSI-X capability is disabled, the device MUST set the Interrupt
> > > Status bit in the PCI Status register in the
> > > PCI Configuration Header of the device to the logical OR of all bits in
> > > ISR status of the device. The device
> > > then asserts/deasserts INT#x interrupts unless masked according to
> > > standard PCI rules [PCI].
> > > The device MUST reset ISR status to 0 on driver read.
> > > 
> > > 
> > > 
> > > 
> > > If MSI-X capability is enabled, the driver SHOULD NOT access ISR status
> > > upon detecting a Queue Interrupt.
> > > 
> > > 
> > > 
> > > It can be clearer, but IMHO it's reasonably clear that devices
> > > do not have to set this bit in MSI mode.  
> > 
> > Yes, it is.  We can just document it in the release notes, but then the
> > fix is not particularly intrusive.
> > 
> > Paolo  
> 
> It has a slight performance cost but it's not too bad.
> 
> I'd rather document it in a code comment though.
> Explain the motivation and which driver versions are affected.
> 

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

[Michael S. Tsirkin]

On Tue, Nov 15, 2016 at 10:48:15AM -0700, Alex Williamson wrote:
> On Tue, 15 Nov 2016 19:38:30 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Nov 15, 2016 at 05:22:49PM +0100, Paolo Bonzini wrote:
> > > 
> > > 
> > > On 15/11/2016 16:44, Michael S. Tsirkin wrote:  
> > > > True. We could drop it from non-data plane, it's just that we never had
> > > > a reason to. vhost in kernel does not set ISR in MSI mode, either.  
> > > 
> > > Yeah, I suspected that.  But dropping it from non-dataplane would break
> > > Windows hibernation and crashdump, just like it did for Alex.  
> > 
> > I guess it's just a question of updating the drivers,
> > isn't it? To me, hibernation/crashdump doesn't sound important
> > enough to warrant work-arounds, but if you feel otherwise,
> > I'm fine with doing this work-around for dataplane.
> 
> The fact that Windows is trying to do some sort of hibernation is not
> visible to the user, I'm simply trying to shutdown the VM.  That's
> rather important on my scale of functionality.  If we have drivers in
> the wild doing this, does it matter what's in the spec?

It matters that latest drivers are already OK. "Update drivers"
has been the advice for any kind of windows problem for years.

> Smells like a
> regression from an end user perspective. Thanks,
> 
> Alex

This exposes a driver bug, yes. The right fix is easy to point
out, whether we want a work-around I'm not sure - I understand that
you feel strongly that we do, is that right? OK, just let's document
what's going on and which versions are affected.


> > > > 
> > > > The device MUST set the Device Configuration Interrupt bit in ISR status
> > > > before sending a device configu-
> > > > ration change notification to the driver.
> > > > If MSI-X capability is disabled, the device MUST set the Queue Interrupt
> > > > bit in ISR status before sending a
> > > > virtqueue notification to the driver.
> > > > If MSI-X capability is disabled, the device MUST set the Interrupt
> > > > Status bit in the PCI Status register in the
> > > > PCI Configuration Header of the device to the logical OR of all bits in
> > > > ISR status of the device. The device
> > > > then asserts/deasserts INT#x interrupts unless masked according to
> > > > standard PCI rules [PCI].
> > > > The device MUST reset ISR status to 0 on driver read.
> > > > 
> > > > 
> > > > 
> > > > 
> > > > If MSI-X capability is enabled, the driver SHOULD NOT access ISR status
> > > > upon detecting a Queue Interrupt.
> > > > 
> > > > 
> > > > 
> > > > It can be clearer, but IMHO it's reasonably clear that devices
> > > > do not have to set this bit in MSI mode.  
> > > 
> > > Yes, it is.  We can just document it in the release notes, but then the
> > > fix is not particularly intrusive.
> > > 
> > > Paolo  
> > 
> > It has a slight performance cost but it's not too bad.
> > 
> > I'd rather document it in a code comment though.
> > Explain the motivation and which driver versions are affected.
> > 

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

[Alex Williamson]

On Tue, 15 Nov 2016 19:58:52 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Nov 15, 2016 at 10:48:15AM -0700, Alex Williamson wrote:
> > On Tue, 15 Nov 2016 19:38:30 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Tue, Nov 15, 2016 at 05:22:49PM +0100, Paolo Bonzini wrote:  
> > > > 
> > > > 
> > > > On 15/11/2016 16:44, Michael S. Tsirkin wrote:    
> > > > > True. We could drop it from non-data plane, it's just that we never had
> > > > > a reason to. vhost in kernel does not set ISR in MSI mode, either.    
> > > > 
> > > > Yeah, I suspected that.  But dropping it from non-dataplane would break
> > > > Windows hibernation and crashdump, just like it did for Alex.    
> > > 
> > > I guess it's just a question of updating the drivers,
> > > isn't it? To me, hibernation/crashdump doesn't sound important
> > > enough to warrant work-arounds, but if you feel otherwise,
> > > I'm fine with doing this work-around for dataplane.  
> > 
> > The fact that Windows is trying to do some sort of hibernation is not
> > visible to the user, I'm simply trying to shutdown the VM.  That's
> > rather important on my scale of functionality.  If we have drivers in
> > the wild doing this, does it matter what's in the spec?  
> 
> It matters that latest drivers are already OK. "Update drivers"
> has been the advice for any kind of windows problem for years.
> 
> > Smells like a
> > regression from an end user perspective. Thanks,
> > 
> > Alex  
> 
> This exposes a driver bug, yes. The right fix is easy to point
> out, whether we want a work-around I'm not sure - I understand that
> you feel strongly that we do, is that right? OK, just let's document
> what's going on and which versions are affected.

I don't mind updating my drivers, I'm just guessing that the average
user doesn't know what virtio driver version they're running, doesn't
read release notes, and will be at least slightly annoyed by this
behavior that looks and smells like a regression from v2.7.  If it
hit me, then there are probably others affected as well.  It's up to you
to weigh maintaining not-entirely-spec-complaint behavior vs user angst.
Thanks,

Alex

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

[Michael S. Tsirkin]

On Tue, Nov 15, 2016 at 11:21:55AM -0700, Alex Williamson wrote:
> On Tue, 15 Nov 2016 19:58:52 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Nov 15, 2016 at 10:48:15AM -0700, Alex Williamson wrote:
> > > On Tue, 15 Nov 2016 19:38:30 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > On Tue, Nov 15, 2016 at 05:22:49PM +0100, Paolo Bonzini wrote:  
> > > > > 
> > > > > 
> > > > > On 15/11/2016 16:44, Michael S. Tsirkin wrote:    
> > > > > > True. We could drop it from non-data plane, it's just that we never had
> > > > > > a reason to. vhost in kernel does not set ISR in MSI mode, either.    
> > > > > 
> > > > > Yeah, I suspected that.  But dropping it from non-dataplane would break
> > > > > Windows hibernation and crashdump, just like it did for Alex.    
> > > > 
> > > > I guess it's just a question of updating the drivers,
> > > > isn't it? To me, hibernation/crashdump doesn't sound important
> > > > enough to warrant work-arounds, but if you feel otherwise,
> > > > I'm fine with doing this work-around for dataplane.  
> > > 
> > > The fact that Windows is trying to do some sort of hibernation is not
> > > visible to the user, I'm simply trying to shutdown the VM.  That's
> > > rather important on my scale of functionality.  If we have drivers in
> > > the wild doing this, does it matter what's in the spec?  
> > 
> > It matters that latest drivers are already OK. "Update drivers"
> > has been the advice for any kind of windows problem for years.
> > 
> > > Smells like a
> > > regression from an end user perspective. Thanks,
> > > 
> > > Alex  
> > 
> > This exposes a driver bug, yes. The right fix is easy to point
> > out, whether we want a work-around I'm not sure - I understand that
> > you feel strongly that we do, is that right? OK, just let's document
> > what's going on and which versions are affected.
> 
> I don't mind updating my drivers, I'm just guessing that the average
> user doesn't know what virtio driver version they're running, doesn't
> read release notes, and will be at least slightly annoyed by this
> behavior that looks and smells like a regression from v2.7.  If it
> hit me, then there are probably others affected as well.  It's up to you
> to weigh maintaining not-entirely-spec-complaint behavior vs user angst.
> Thanks,
> 
> Alex

I think I'll merge the work-around if it's better documented.
If you think it should go in, can you provide your tested-by tag?

-- 
MST

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

[Alex Williamson]

On Tue, 15 Nov 2016 21:17:56 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Nov 15, 2016 at 11:21:55AM -0700, Alex Williamson wrote:
> > On Tue, 15 Nov 2016 19:58:52 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Tue, Nov 15, 2016 at 10:48:15AM -0700, Alex Williamson wrote:  
> > > > On Tue, 15 Nov 2016 19:38:30 +0200
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >     
> > > > > On Tue, Nov 15, 2016 at 05:22:49PM +0100, Paolo Bonzini wrote:    
> > > > > > 
> > > > > > 
> > > > > > On 15/11/2016 16:44, Michael S. Tsirkin wrote:      
> > > > > > > True. We could drop it from non-data plane, it's just that we never had
> > > > > > > a reason to. vhost in kernel does not set ISR in MSI mode, either.      
> > > > > > 
> > > > > > Yeah, I suspected that.  But dropping it from non-dataplane would break
> > > > > > Windows hibernation and crashdump, just like it did for Alex.      
> > > > > 
> > > > > I guess it's just a question of updating the drivers,
> > > > > isn't it? To me, hibernation/crashdump doesn't sound important
> > > > > enough to warrant work-arounds, but if you feel otherwise,
> > > > > I'm fine with doing this work-around for dataplane.    
> > > > 
> > > > The fact that Windows is trying to do some sort of hibernation is not
> > > > visible to the user, I'm simply trying to shutdown the VM.  That's
> > > > rather important on my scale of functionality.  If we have drivers in
> > > > the wild doing this, does it matter what's in the spec?    
> > > 
> > > It matters that latest drivers are already OK. "Update drivers"
> > > has been the advice for any kind of windows problem for years.
> > >   
> > > > Smells like a
> > > > regression from an end user perspective. Thanks,
> > > > 
> > > > Alex    
> > > 
> > > This exposes a driver bug, yes. The right fix is easy to point
> > > out, whether we want a work-around I'm not sure - I understand that
> > > you feel strongly that we do, is that right? OK, just let's document
> > > what's going on and which versions are affected.  
> > 
> > I don't mind updating my drivers, I'm just guessing that the average
> > user doesn't know what virtio driver version they're running, doesn't
> > read release notes, and will be at least slightly annoyed by this
> > behavior that looks and smells like a regression from v2.7.  If it
> > hit me, then there are probably others affected as well.  It's up to you
> > to weigh maintaining not-entirely-spec-complaint behavior vs user angst.
> > Thanks,
> > 
> > Alex  
> 
> I think I'll merge the work-around if it's better documented.
> If you think it should go in, can you provide your tested-by tag?
> 

Tested series against 97e53cf82ca0ffa9abe2def2fabc5fc75b914d90, solves
both the vhost issue and the hung shutdown on my VM.

Tested-by: Alex Williamson <alex.williamson@redhat.com>

Re: [Qemu-devel] [PATCH for-2.8 0/3] virtio fixes

[Christian Borntraeger]

On 11/15/2016 02:46 PM, Paolo Bonzini wrote:
> Patch 1 fixes vhost, patches 2-3 fix Windows hibernation.
> 
> Paolo
> 
> Paolo Bonzini (3):
>   virtio: introduce grab/release_ioeventfd to fix vhost
>   virtio: access ISR atomically
>   virtio: set ISR on dataplane notifications
> 
>  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/vhost.c               | 11 +++------
>  hw/virtio/virtio-bus.c          | 54 ++++++++++++++++++++++++++++++++---------
>  hw/virtio/virtio-mmio.c         |  6 ++---
>  hw/virtio/virtio-pci.c          |  9 +++----
>  hw/virtio/virtio.c              | 46 ++++++++++++++++++++++++++++-------
>  include/hw/virtio/virtio-bus.h  | 14 +++++++++++
>  include/hw/virtio/virtio-scsi.h |  1 -
>  include/hw/virtio/virtio.h      |  4 ++-
>  12 files changed, 110 insertions(+), 50 deletions(-)
> 

Farhan,

it was this mail thread.

Re: [Qemu-devel] [PATCH for-2.8 0/3] virtio fixes

[Farhan Ali]

Hi Paolo,

I was able to test your patches in our s390 environment. I don't see the 
qemu crashes anymore which I noticed before.

Testing a guest running high stress I/O workload, without iothreads does 
show a delay in guest response time. But running

the same test with iothreads seems to solve the issue.

Tested-by : Farhan Ali <alifm@linux.vnet.ibm.com>


Thank you

Farhan


On 11/16/2016 02:50 PM, Christian Borntraeger wrote:
> On 11/15/2016 02:46 PM, Paolo Bonzini wrote:
>> Patch 1 fixes vhost, patches 2-3 fix Windows hibernation.
>>
>> Paolo
>>
>> Paolo Bonzini (3):
>>    virtio: introduce grab/release_ioeventfd to fix vhost
>>    virtio: access ISR atomically
>>    virtio: set ISR on dataplane notifications
>>
>>   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/vhost.c               | 11 +++------
>>   hw/virtio/virtio-bus.c          | 54 ++++++++++++++++++++++++++++++++---------
>>   hw/virtio/virtio-mmio.c         |  6 ++---
>>   hw/virtio/virtio-pci.c          |  9 +++----
>>   hw/virtio/virtio.c              | 46 ++++++++++++++++++++++++++++-------
>>   include/hw/virtio/virtio-bus.h  | 14 +++++++++++
>>   include/hw/virtio/virtio-scsi.h |  1 -
>>   include/hw/virtio/virtio.h      |  4 ++-
>>   12 files changed, 110 insertions(+), 50 deletions(-)
>>
> Farhan,
>
> it was this mail thread.

Re: [Qemu-devel] [PATCH for-2.8 0/3] virtio fixes

[Michael S. Tsirkin]

On Wed, Nov 16, 2016 at 03:03:13PM -0500, Farhan Ali wrote:
> Hi Paolo,
> 
> I was able to test your patches in our s390 environment. I don't see the
> qemu crashes anymore which I noticed before.
> 
> Testing a guest running high stress I/O workload, without iothreads does
> show a delay in guest response time.

Compared to which version?

> But running
> 
> the same test with iothreads seems to solve the issue.
> 
> Tested-by : Farhan Ali <alifm@linux.vnet.ibm.com>

Could you also test just patches 1 and 2 pls?

> 
> Thank you
> 
> Farhan
> 
> 
> On 11/16/2016 02:50 PM, Christian Borntraeger wrote:
> > On 11/15/2016 02:46 PM, Paolo Bonzini wrote:
> > > Patch 1 fixes vhost, patches 2-3 fix Windows hibernation.
> > > 
> > > Paolo
> > > 
> > > Paolo Bonzini (3):
> > >    virtio: introduce grab/release_ioeventfd to fix vhost
> > >    virtio: access ISR atomically
> > >    virtio: set ISR on dataplane notifications
> > > 
> > >   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/vhost.c               | 11 +++------
> > >   hw/virtio/virtio-bus.c          | 54 ++++++++++++++++++++++++++++++++---------
> > >   hw/virtio/virtio-mmio.c         |  6 ++---
> > >   hw/virtio/virtio-pci.c          |  9 +++----
> > >   hw/virtio/virtio.c              | 46 ++++++++++++++++++++++++++++-------
> > >   include/hw/virtio/virtio-bus.h  | 14 +++++++++++
> > >   include/hw/virtio/virtio-scsi.h |  1 -
> > >   include/hw/virtio/virtio.h      |  4 ++-
> > >   12 files changed, 110 insertions(+), 50 deletions(-)
> > > 
> > Farhan,
> > 
> > it was this mail thread.

Re: [Qemu-devel] [PATCH for-2.8 0/3] virtio fixes

[Farhan Ali]

On 11/16/2016 03:16 PM, Michael S. Tsirkin wrote:
> On Wed, Nov 16, 2016 at 03:03:13PM -0500, Farhan Ali wrote:
>> Hi Paolo,
>>
>> I was able to test your patches in our s390 environment. I don't see the
>> qemu crashes anymore which I noticed before.
>>
>> Testing a guest running high stress I/O workload, without iothreads does
>> show a delay in guest response time.
> Compared to which version?
Compared to 2.7.0

>
>> But running
>>
>> the same test with iothreads seems to solve the issue.
>>
>> Tested-by : Farhan Ali <alifm@linux.vnet.ibm.com>
> Could you also test just patches 1 and 2 pls?

Okay will do
>
>> Thank you
>>
>> Farhan
>>
>>
>> On 11/16/2016 02:50 PM, Christian Borntraeger wrote:
>>> On 11/15/2016 02:46 PM, Paolo Bonzini wrote:
>>>> Patch 1 fixes vhost, patches 2-3 fix Windows hibernation.
>>>>
>>>> Paolo
>>>>
>>>> Paolo Bonzini (3):
>>>>     virtio: introduce grab/release_ioeventfd to fix vhost
>>>>     virtio: access ISR atomically
>>>>     virtio: set ISR on dataplane notifications
>>>>
>>>>    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/vhost.c               | 11 +++------
>>>>    hw/virtio/virtio-bus.c          | 54 ++++++++++++++++++++++++++++++++---------
>>>>    hw/virtio/virtio-mmio.c         |  6 ++---
>>>>    hw/virtio/virtio-pci.c          |  9 +++----
>>>>    hw/virtio/virtio.c              | 46 ++++++++++++++++++++++++++++-------
>>>>    include/hw/virtio/virtio-bus.h  | 14 +++++++++++
>>>>    include/hw/virtio/virtio-scsi.h |  1 -
>>>>    include/hw/virtio/virtio.h      |  4 ++-
>>>>    12 files changed, 110 insertions(+), 50 deletions(-)
>>>>
>>> Farhan,
>>>
>>> it was this mail thread.

Re: [Qemu-devel] [PATCH for-2.8 0/3] virtio fixes

[Farhan Ali]

On 11/16/2016 03:32 PM, Farhan Ali wrote:
>
>
> On 11/16/2016 03:16 PM, Michael S. Tsirkin wrote:
>> On Wed, Nov 16, 2016 at 03:03:13PM -0500, Farhan Ali wrote:
>>> Hi Paolo,
>>>
>>> I was able to test your patches in our s390 environment. I don't see 
>>> the
>>> qemu crashes anymore which I noticed before.
>>>
>>> Testing a guest running high stress I/O workload, without iothreads 
>>> does
>>> show a delay in guest response time.
>> Compared to which version?
> Compared to 2.7.0
>
>>
>>> But running
>>>
>>> the same test with iothreads seems to solve the issue.
>>>
>>> Tested-by : Farhan Ali <alifm@linux.vnet.ibm.com>
>> Could you also test just patches 1 and 2 pls?
>
> Okay will do

Testing with just patches 1 and 2, I did not see any change.

>>
>>> Thank you
>>>
>>> Farhan
>>>
>>>
>>> On 11/16/2016 02:50 PM, Christian Borntraeger wrote:
>>>> On 11/15/2016 02:46 PM, Paolo Bonzini wrote:
>>>>> Patch 1 fixes vhost, patches 2-3 fix Windows hibernation.
>>>>>
>>>>> Paolo
>>>>>
>>>>> Paolo Bonzini (3):
>>>>>     virtio: introduce grab/release_ioeventfd to fix vhost
>>>>>     virtio: access ISR atomically
>>>>>     virtio: set ISR on dataplane notifications
>>>>>
>>>>>    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/vhost.c               | 11 +++------
>>>>>    hw/virtio/virtio-bus.c          | 54 
>>>>> ++++++++++++++++++++++++++++++++---------
>>>>>    hw/virtio/virtio-mmio.c         |  6 ++---
>>>>>    hw/virtio/virtio-pci.c          |  9 +++----
>>>>>    hw/virtio/virtio.c              | 46 
>>>>> ++++++++++++++++++++++++++++-------
>>>>>    include/hw/virtio/virtio-bus.h  | 14 +++++++++++
>>>>>    include/hw/virtio/virtio-scsi.h |  1 -
>>>>>    include/hw/virtio/virtio.h      |  4 ++-
>>>>>    12 files changed, 110 insertions(+), 50 deletions(-)
>>>>>
>>>> Farhan,
>>>>
>>>> it was this mail thread.
>

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

[Michael S. Tsirkin]

On Thu, Nov 17, 2016 at 04:04:26AM -0500, Paolo Bonzini wrote:
> > > > > > /*
> > > > > >  * 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
> > > > >          ^^^^^^^^^^^^^^^^
> > > > > 
> > > > > Not sure it's only for Windows 8.1, in fact probably not.
> > > > 
> > > > 8.1 on shutdown and others on crashdump or hibernation?
> > > 
> > > Even 8.1 is really a hibernation hidden behind a "Shut down" menu item.
> > > 
> > > Paolo
> > 
> > what does "hang during shutdown" in your commit log refer to then?
> 
> The full text from the commit log is:
> 
>     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.
> 
> Shutdown in the commit log just means "clicking Shut down".  The previous
> sentence explains why.  Also note the "e.g.", I've not tested other versions
> of Windows.
> 
> Paolo

AFAIK the specific version of drivers did not support windows 10,
and IIRC windows 7 does shutdown normally.
So that only leaves windows 8 and 8.1.


> > > > > Looks good if you replace this line with
> > > > > 
> > > > > "are incorrectly polling this bit during crashdump or hibernation"

... which on windows 8 and 8.1 is activated by default through the
shutdown menu

> > > > > Paolo
> > > > > 
> > > > > >  * 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.
> > > > > >  */
> > > > > 

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

[Stefan Hajnoczi]

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

On Wed, Nov 16, 2016 at 10:15:25PM +0200, Michael S. Tsirkin wrote:
> On Wed, Nov 16, 2016 at 07:05:51PM +0100, Paolo Bonzini wrote:
> > @@ -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.
>  */

I agree.  The commit description is nice but the information needs to be
in the code.

I remember explicitly omitting the ISR update when writing the dataplane
code because I saw the spec does not require it for MSI.

A comment is necessary.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Paolo Bonzini]

> > > > > /*
> > > > >  * 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
> > > >          ^^^^^^^^^^^^^^^^
> > > > 
> > > > Not sure it's only for Windows 8.1, in fact probably not.
> > > 
> > > 8.1 on shutdown and others on crashdump or hibernation?
> > 
> > Even 8.1 is really a hibernation hidden behind a "Shut down" menu item.
> > 
> > Paolo
> 
> what does "hang during shutdown" in your commit log refer to then?

The full text from the commit log is:

    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.

Shutdown in the commit log just means "clicking Shut down".  The previous
sentence explains why.  Also note the "e.g.", I've not tested other versions
of Windows.

Paolo

> > > > Looks good if you replace this line with
> > > > 
> > > > "are incorrectly polling this bit during crashdump or hibernation"
> > > > 
> > > > Paolo
> > > > 
> > > > >  * 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
> > > > > 
> > > 
> 

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

[Michael S. Tsirkin]

On Wed, Nov 16, 2016 at 04:05:31PM -0500, Paolo Bonzini wrote:
> 
> 
> ----- Original Message -----
> > From: "Michael S. Tsirkin" <mst@redhat.com>
> > To: "Paolo Bonzini" <pbonzini@redhat.com>
> > Cc: qemu-devel@nongnu.org, "alex williamson" <alex.williamson@redhat.com>, borntraeger@de.ibm.com, felipe@nutanix.com
> > Sent: Wednesday, November 16, 2016 9:39:24 PM
> > Subject: Re: [PATCH 3/3] virtio: set ISR on dataplane notifications
> > 
> > On Wed, Nov 16, 2016 at 03:38:11PM -0500, Paolo Bonzini wrote:
> > > > > +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
> > >          ^^^^^^^^^^^^^^^^
> > > 
> > > Not sure it's only for Windows 8.1, in fact probably not.
> > 
> > 8.1 on shutdown and others on crashdump or hibernation?
> 
> Even 8.1 is really a hibernation hidden behind a "Shut down" menu item.
> 
> Paolo

what does "hang during shutdown" in your commit log refer to then?

> > > Looks good if you replace this line with
> > > 
> > > "are incorrectly polling this bit during crashdump or hibernation"
> > > 
> > > Paolo
> > > 
> > > >  * 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
> > > > 
> > 

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

[Paolo Bonzini]

----- Original Message -----
> From: "Michael S. Tsirkin" <mst@redhat.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: qemu-devel@nongnu.org, "alex williamson" <alex.williamson@redhat.com>, borntraeger@de.ibm.com, felipe@nutanix.com
> Sent: Wednesday, November 16, 2016 9:39:24 PM
> Subject: Re: [PATCH 3/3] virtio: set ISR on dataplane notifications
> 
> On Wed, Nov 16, 2016 at 03:38:11PM -0500, Paolo Bonzini wrote:
> > > > +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
> >          ^^^^^^^^^^^^^^^^
> > 
> > Not sure it's only for Windows 8.1, in fact probably not.
> 
> 8.1 on shutdown and others on crashdump or hibernation?

Even 8.1 is really a hibernation hidden behind a "Shut down" menu item.

Paolo

> > Looks good if you replace this line with
> > 
> > "are incorrectly polling this bit during crashdump or hibernation"
> > 
> > Paolo
> > 
> > >  * 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
> > > 
> 

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

[Michael S. Tsirkin]

On Wed, Nov 16, 2016 at 03:38:11PM -0500, Paolo Bonzini wrote:
> > > +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
>          ^^^^^^^^^^^^^^^^
> 
> Not sure it's only for Windows 8.1, in fact probably not.

8.1 on shutdown and others on crashdump or hibernation?

> Looks good if you replace this line with
> 
> "are incorrectly polling this bit during crashdump or hibernation"
> 
> Paolo
> 
> >  * 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
> > 

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

[Paolo Bonzini]

> > +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
         ^^^^^^^^^^^^^^^^

Not sure it's only for Windows 8.1, in fact probably not.
Looks good if you replace this line with

"are incorrectly polling this bit during crashdump or hibernation"

Paolo

>  * 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
> 

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 <pbonzini@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

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

[Paolo Bonzini]

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 <pbonzini@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);
+    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