From 310837de6c1e0badfd736b1b316b1698c53120a7 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 18 Nov 2016 16:07:00 +0100 Subject: 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 --- include/hw/virtio/virtio-bus.h | 14 ++++++++++++++ include/hw/virtio/virtio.h | 2 ++ 2 files changed, 16 insertions(+) (limited to 'include') 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); -- cgit v1.1 From 83d768b5640946b7da55ce8335509df297e2c7cd Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 18 Nov 2016 16:07:02 +0100 Subject: virtio: set ISR on dataplane notifications 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. Reviewed-by: Stefan Hajnoczi Tested-by: Farhan Ali Tested-by: Alex Williamson Signed-off-by: Paolo Bonzini Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- include/hw/virtio/virtio-scsi.h | 1 - include/hw/virtio/virtio.h | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) (limited to 'include') 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); -- cgit v1.1