From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48074) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1akuSV-0002AA-PT for qemu-devel@nongnu.org; Tue, 29 Mar 2016 10:17:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1akuSQ-00060V-ME for qemu-devel@nongnu.org; Tue, 29 Mar 2016 10:17:27 -0400 Received: from e06smtp11.uk.ibm.com ([195.75.94.107]:52735) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1akuSQ-00060G-9P for qemu-devel@nongnu.org; Tue, 29 Mar 2016 10:17:22 -0400 Received: from localhost by e06smtp11.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 29 Mar 2016 15:17:21 +0100 Received: from b06cxnps4075.portsmouth.uk.ibm.com (d06relay12.portsmouth.uk.ibm.com [9.149.109.197]) by d06dlp03.portsmouth.uk.ibm.com (Postfix) with ESMTP id E1B211B08069 for ; Tue, 29 Mar 2016 15:17:45 +0100 (BST) Received: from d06av05.portsmouth.uk.ibm.com (d06av05.portsmouth.uk.ibm.com [9.149.37.229]) by b06cxnps4075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u2TEHBNZ7012630 for ; Tue, 29 Mar 2016 14:17:11 GMT Received: from d06av05.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av05.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u2TEHAmf021703 for ; Tue, 29 Mar 2016 08:17:11 -0600 From: Cornelia Huck Date: Tue, 29 Mar 2016 16:17:07 +0200 Message-Id: <1459261027-4398-2-git-send-email-cornelia.huck@de.ibm.com> In-Reply-To: <1459261027-4398-1-git-send-email-cornelia.huck@de.ibm.com> References: <1459261027-4398-1-git-send-email-cornelia.huck@de.ibm.com> Subject: [Qemu-devel] [PATCH 1/1] virtio: fix ioeventfd assignment race List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: famz@redhat.com, borntraeger@de.ibm.com, mst@redhat.com, tubo@linux.vnet.ibm.com, stefanha@redhat.com, Cornelia Huck , pbonzini@redhat.com The ->set_host_notifier() callback is invoked whenever we want to switch from or to the generic ioeventfd handler. Currently, all transports deregister the ioeventfd backing and then re-register it. This opens a race window where we are without ioeventfd backing for a time period: In the virtio-blk dataplane case, we observed notifications coming in from both the vcpu thread and the iothread. Let's change pci, mmio and ccw to keep the ioeventfd during ->set_host_notifier() and only switch the ioeventfd handler. Signed-off-by: Cornelia Huck --- hw/s390x/virtio-ccw.c | 22 +++++++++++++++++----- hw/virtio/virtio-mmio.c | 27 +++++++++++++++++---------- hw/virtio/virtio-pci.c | 28 ++++++++++++++++++---------- include/hw/virtio/virtio-bus.h | 4 ++++ 4 files changed, 56 insertions(+), 25 deletions(-) diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c index cb887ba..7b1088e 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -1196,14 +1196,26 @@ static bool virtio_ccw_query_guest_notifiers(DeviceState *d) static int virtio_ccw_set_host_notifier(DeviceState *d, int n, bool assign) { VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus); + VirtQueue *vq = virtio_get_queue(vdev, n); - /* Stop using the generic ioeventfd, we are doing eventfd handling - * ourselves below */ - dev->ioeventfd_disabled = assign; if (assign) { - virtio_ccw_stop_ioeventfd(dev); + /* Stop using the generic ioeventfd, we are doing eventfd handling + * ourselves below */ + dev->ioeventfd_disabled = true; + }; + /* + * Just switch the handler, don't deassign the ioeventfd. + * 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. + */ + virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign); + if (!assign) { + /* Use generic ioeventfd handler again. */ + dev->ioeventfd_disabled = false; } - return virtio_ccw_set_guest2host_notifier(dev, n, assign, false); + return 0; } static int virtio_ccw_get_mappings(VirtioCcwDevice *dev) diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c index d4cd91f..aafebdf 100644 --- a/hw/virtio/virtio-mmio.c +++ b/hw/virtio/virtio-mmio.c @@ -502,19 +502,26 @@ static int virtio_mmio_set_host_notifier(DeviceState *opaque, int n, bool assign) { VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque); + VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); + VirtQueue *vq = virtio_get_queue(vdev, n); - /* Stop using ioeventfd for virtqueue kick if the device starts using host - * notifiers. This makes it easy to avoid stepping on each others' toes. - */ - proxy->ioeventfd_disabled = assign; if (assign) { - virtio_mmio_stop_ioeventfd(proxy); + /* Stop using the generic ioeventfd, we are doing eventfd handling + * ourselves below */ + proxy->ioeventfd_disabled = true; + }; + /* + * Just switch the handler, don't deassign the ioeventfd. + * 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. + */ + virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign); + if (!assign) { + /* Use generic ioeventfd handler again. */ + proxy->ioeventfd_disabled = false; } - /* We don't need to start here: it's not needed because backend - * currently only stops on status change away from ok, - * reset, vmstop and such. If we do add code to start here, - * need to check vmstate, device state etc. */ - return virtio_mmio_set_host_notifier_internal(proxy, n, assign, false); + return 0; } /* virtio-mmio device */ diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 0dadb66..a91c1e8 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1115,18 +1115,26 @@ static int virtio_pci_set_host_notifier(DeviceState *d, int n, bool assign) { VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d); - /* Stop using ioeventfd for virtqueue kick if the device starts using host - * notifiers. This makes it easy to avoid stepping on each others' toes. - */ - proxy->ioeventfd_disabled = assign; + VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); + VirtQueue *vq = virtio_get_queue(vdev, n); + if (assign) { - virtio_pci_stop_ioeventfd(proxy); + /* Stop using the generic ioeventfd, we are doing eventfd handling + * ourselves below */ + proxy->ioeventfd_disabled = true; + }; + /* + * Just switch the handler, don't deassign the ioeventfd. + * 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. + */ + virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign); + if (!assign) { + /* Use generic ioeventfd handler again. */ + proxy->ioeventfd_disabled = false; } - /* We don't need to start here: it's not needed because backend - * currently only stops on status change away from ok, - * reset, vmstop and such. If we do add code to start here, - * need to check vmstate, device state etc. */ - return virtio_pci_set_host_notifier_internal(proxy, n, assign, false); + return 0; } static void virtio_pci_vmstate_change(DeviceState *d, bool running) diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h index 3f2c136..98c660a 100644 --- a/include/hw/virtio/virtio-bus.h +++ b/include/hw/virtio/virtio-bus.h @@ -52,6 +52,10 @@ typedef struct VirtioBusClass { bool (*has_extra_state)(DeviceState *d); bool (*query_guest_notifiers)(DeviceState *d); int (*set_guest_notifiers)(DeviceState *d, int nvqs, bool assign); + /* + * Switch from/to the generic ioeventfd handler. + * assigned==false means 'use generic ioeventfd handler'. + */ int (*set_host_notifier)(DeviceState *d, int n, bool assigned); void (*vmstate_change)(DeviceState *d, bool running); /* -- 2.8.0