[Qemu-devel] [PULL 2/7] virtio: introduce grab/release_ioeventfd to fix vhost

[Michael S. Tsirkin]

From: Paolo Bonzini <address@hidden>

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 <address@hidden>
Reported-by: Christian Borntraeger <address@hidden>
Reported-by: Alex Williamson <address@hidden>
Fixes: ed08a2a0b ("virtio: use virtio_bus_set_host_notifier to start/stop 
ioeventfd")
Reviewed-by: Cornelia Huck <address@hidden>
Reviewed-by: Stefan Hajnoczi <address@hidden>
Tested-by: Alexey Kardashevskiy <address@hidden>
Tested-by: Farhan Ali <address@hidden>
Tested-by: Alex Williamson <address@hidden>
Signed-off-by: Paolo Bonzini <address@hidden>
Reviewed-by: Michael S. Tsirkin <address@hidden>
Signed-off-by: Michael S. Tsirkin <address@hidden>
---
 include/hw/virtio/virtio-bus.h | 14 ++++++++++
 include/hw/virtio/virtio.h     |  2 ++
 hw/virtio/vhost.c              | 14 +++++-----
 hw/virtio/virtio-bus.c         | 58 ++++++++++++++++++++++++++++++++++--------
 hw/virtio/virtio.c             | 16 ++++++++++++
 5 files changed, 86 insertions(+), 18 deletions(-)

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 5951997..835b085 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -272,6 +272,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);
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 30aee88..f7f7023 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1214,17 +1214,17 @@ 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) {
+    /* We will pass the notifiers to the kernel, make sure that QEMU
+     * 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);
@@ -1244,7 +1244,7 @@ fail_vq:
         }
         assert (e >= 0);
     }
-    virtio_device_start_ioeventfd(vdev);
+    virtio_device_release_ioeventfd(vdev);
 fail:
     return r;
 }
@@ -1267,7 +1267,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..d6c0c72 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -147,6 +147,39 @@ 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 +194,14 @@ 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 +216,12 @@ 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 notifier 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;
 }
 
@@ -211,7 +251,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 +264,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 55a00cd..b7d5828 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2191,6 +2191,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. */
-- 
MST