From 310837de6c1e0badfd736b1b316b1698c53120a7 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 18 Nov 2016 16:07:00 +0100 Subject: [PATCH] virtio: introduce grab/release_ioeventfd to fix vhost 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 Reported-by: Christian Borntraeger Reported-by: Alex Williamson Fixes: ed08a2a0b ("virtio: use virtio_bus_set_host_notifier to start/stop ioeventfd") Reviewed-by: Cornelia Huck Reviewed-by: Stefan Hajnoczi Tested-by: Alexey Kardashevskiy Tested-by: Farhan Ali Tested-by: Alex Williamson Signed-off-by: Paolo Bonzini Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/virtio/vhost.c | 14 +++++----- hw/virtio/virtio-bus.c | 58 ++++++++++++++++++++++++++++++++++-------- hw/virtio/virtio.c | 16 ++++++++++++ include/hw/virtio/virtio-bus.h | 14 ++++++++++ include/hw/virtio/virtio.h | 2 ++ 5 files changed, 86 insertions(+), 18 deletions(-) diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 30aee88a3e..f7f70237db 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 bf61f66a04..d6c0c72bd2 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 55a00cdf9e..b7d5828601 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. */ diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h index fdf7fdab81..8a51e2c564 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 5951997f22..835b085d11 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); -- 2.11.4.GIT