[Qemu-devel] [PATCH v2] virtio: Fix setting up host notifiers for vhost

* [Qemu-devel] [PATCH v2] virtio: Fix setting up host notifiers for vhost
@ 2016-06-30 11:52 Cornelia Huck
  2016-06-30 12:04 ` Peter Lieven
  2016-06-30 14:12 ` Marc-André Lureau
  0 siblings, 2 replies; 4+ messages in thread
From: Cornelia Huck @ 2016-06-30 11:52 UTC (permalink / raw)
  To: qemu-devel
  Cc: mst, famz, stefanha, jasowang, pl, marcandre.lureau, pbonzini,
	Cornelia Huck

When setting up host notifiers, virtio_bus_set_host_notifier()
simply switches the handler. This will only work, however, if
the ioeventfd has already been setup; this is true for dataplane,
but not for vhost, and will completely break things if the
ioeventfd is disabled for the device.

Fix this by starting the ioeventfd on assign if that has not
happened before, and only switch the handler if the ioeventfd
has really been started.

While we're at it, also fixup the unsetting path of
set_host_notifier_internal().

Fixes: 6798e245a3 ("virtio-bus: common ioeventfd infrastructure")
Reported-by: Jason Wang <jasowang@redhat.com>
Reported-by: Marc-André Lureau <marcandre.lureau@gmail.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
Changes v1->v2:
- don't switch the handler if the eventfd has not been setup - this
  fixes the failure of vhost-user-test for me
- add more comments
---
 hw/virtio/virtio-bus.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 1313760..dfe24fc 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -176,8 +176,8 @@ static int set_host_notifier_internal(DeviceState *proxy, VirtioBusState *bus,
             return r;
         }
     } else {
-        virtio_queue_set_host_notifier_fd_handler(vq, false, false);
         k->ioeventfd_assign(proxy, notifier, n, assign);
+        virtio_queue_set_host_notifier_fd_handler(vq, false, false);
         event_notifier_cleanup(notifier);
     }
     return r;
@@ -246,6 +246,9 @@ void virtio_bus_stop_ioeventfd(VirtioBusState *bus)
 /*
  * This function switches from/to the generic ioeventfd handler.
  * assign==false means 'use generic ioeventfd handler'.
+ * It is only considered an error if the binding does not support
+ * host notifiers at all, not when they are not available for the
+ * individual device.
  */
 int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
 {
@@ -259,6 +262,13 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
     }
     if (assign) {
         /*
+         * Not yet started? assign==true implies we want to use an
+         * eventfd, so let's do the requisite setup.
+         */
+        if (!k->ioeventfd_started(proxy)) {
+            virtio_bus_start_ioeventfd(bus);
+        }
+        /*
          * Stop using the generic ioeventfd, we are doing eventfd handling
          * ourselves below
          */
@@ -269,8 +279,13 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
      * Otherwise, there's a window where we don't have an
      * ioeventfd and we may end up with a notification where
      * we don't expect one.
+     *
+     * Also note that we should only do something with the eventfd
+     * handler if the eventfd has actually been setup.
      */
-    virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign);
+    if (k->ioeventfd_started(proxy)) {
+        virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign);
+    }
     if (!assign) {
         /* Use generic ioeventfd handler again. */
         k->ioeventfd_set_disabled(proxy, false);
-- 
2.6.6

^ permalink raw reply related	[flat|nested] 4+ messages in thread