* [Qemu-devel] [PATCH 0/1] virtio: keep ioeventfd assigned
@ 2016-03-29 14:17 Cornelia Huck
2016-03-29 14:17 ` [Qemu-devel] [PATCH 1/1] virtio: fix ioeventfd assignment race Cornelia Huck
0 siblings, 1 reply; 6+ messages in thread
From: Cornelia Huck @ 2016-03-29 14:17 UTC (permalink / raw)
To: qemu-devel
Cc: famz, borntraeger, mst, tubo, stefanha, Cornelia Huck, pbonzini
This is just the ioventfd assignment fix, factored out into the
individual transports. This (+ the assertion patch) has been
surviving some minutes of doing a reboot loop so far.
While this is a bit larger than the aio handler changes, I think
it is easier to understand.
Cornelia Huck (1):
virtio: fix ioeventfd assignment race
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(-)
--
2.8.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 1/1] virtio: fix ioeventfd assignment race
2016-03-29 14:17 [Qemu-devel] [PATCH 0/1] virtio: keep ioeventfd assigned Cornelia Huck
@ 2016-03-29 14:17 ` Cornelia Huck
2016-03-29 15:07 ` Paolo Bonzini
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Cornelia Huck @ 2016-03-29 14:17 UTC (permalink / raw)
To: qemu-devel
Cc: famz, borntraeger, mst, tubo, stefanha, Cornelia Huck, pbonzini
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 <cornelia.huck@de.ibm.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] virtio: fix ioeventfd assignment race
2016-03-29 14:17 ` [Qemu-devel] [PATCH 1/1] virtio: fix ioeventfd assignment race Cornelia Huck
@ 2016-03-29 15:07 ` Paolo Bonzini
2016-03-29 15:53 ` Christian Borntraeger
2016-03-31 2:43 ` tu bo
2 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2016-03-29 15:07 UTC (permalink / raw)
To: Cornelia Huck, qemu-devel; +Cc: tubo, borntraeger, famz, stefanha, mst
On 29/03/2016 16:17, Cornelia Huck wrote:
> 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 <cornelia.huck@de.ibm.com>
> ---
> 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);
> /*
>
Tested-by: Paolo Bonzini <pbonzini@redhat.com>
(survives about 15 minutes of reboot loops)
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] virtio: fix ioeventfd assignment race
2016-03-29 14:17 ` [Qemu-devel] [PATCH 1/1] virtio: fix ioeventfd assignment race Cornelia Huck
2016-03-29 15:07 ` Paolo Bonzini
@ 2016-03-29 15:53 ` Christian Borntraeger
2016-03-31 2:43 ` tu bo
2 siblings, 0 replies; 6+ messages in thread
From: Christian Borntraeger @ 2016-03-29 15:53 UTC (permalink / raw)
To: Cornelia Huck, qemu-devel; +Cc: tubo, pbonzini, famz, stefanha, mst
On 03/29/2016 04:17 PM, Cornelia Huck wrote:
> 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 <cornelia.huck@de.ibm.com>
Seems to work fine. Next I will test Michaels patches.
> ---
> 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);
> /*
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] virtio: fix ioeventfd assignment race
2016-03-29 14:17 ` [Qemu-devel] [PATCH 1/1] virtio: fix ioeventfd assignment race Cornelia Huck
2016-03-29 15:07 ` Paolo Bonzini
2016-03-29 15:53 ` Christian Borntraeger
@ 2016-03-31 2:43 ` tu bo
2016-03-31 7:41 ` tu bo
2 siblings, 1 reply; 6+ messages in thread
From: tu bo @ 2016-03-31 2:43 UTC (permalink / raw)
To: Cornelia Huck, qemu-devel; +Cc: pbonzini, famz, borntraeger, stefanha, mst
Hi Cornelia:
I saw qemu crash for this patch on qemu master yesterday.
However, scsi disks of my lpar is not available because of s38 firmware
update. I'll double check the test when it's ready. thx
On 03/29/2016 10:17 PM, Cornelia Huck wrote:
> 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 <cornelia.huck@de.ibm.com>
> ---
> 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);
> /*
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] virtio: fix ioeventfd assignment race
2016-03-31 2:43 ` tu bo
@ 2016-03-31 7:41 ` tu bo
0 siblings, 0 replies; 6+ messages in thread
From: tu bo @ 2016-03-31 7:41 UTC (permalink / raw)
To: Cornelia Huck, qemu-devel; +Cc: pbonzini, famz, borntraeger, stefanha, mst
Hi Cornelia:
On 03/31/2016 10:43 AM, tu bo wrote:
> Hi Cornelia:
>
> I saw qemu crash for this patch on qemu master yesterday.
>
> However, scsi disks of my lpar is not available because of s38 firmware
> update. I'll double check the test when it's ready. thx
I still can see crash like below,
(gdb) bt
#0 blk_aio_read_entry (opaque=0x0) at block/block-backend.c:916
#1 0x000002aa0d6e873e in coroutine_trampoline (i0=<optimized out>,
i1=1275094848) at util/coroutine-ucontext.c:78
#2 0x000003ffab4d150a in __makecontext_ret () from /lib64/libc.so.6
(gdb) list
911 static void blk_aio_read_entry(void *opaque)
912 {
913 BlkAioEmAIOCB *acb = opaque;
914 BlkRwCo *rwco = &acb->rwco;
915
916 rwco->ret = blk_co_preadv(rwco->blk, rwco->offset, rwco->qiov->size,
917 rwco->qiov, rwco->flags);
918 blk_aio_complete(acb);
919 }
920
>
> On 03/29/2016 10:17 PM, Cornelia Huck wrote:
>> 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 <cornelia.huck@de.ibm.com>
>> ---
>> 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);
>> /*
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-03-31 7:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-29 14:17 [Qemu-devel] [PATCH 0/1] virtio: keep ioeventfd assigned Cornelia Huck
2016-03-29 14:17 ` [Qemu-devel] [PATCH 1/1] virtio: fix ioeventfd assignment race Cornelia Huck
2016-03-29 15:07 ` Paolo Bonzini
2016-03-29 15:53 ` Christian Borntraeger
2016-03-31 2:43 ` tu bo
2016-03-31 7:41 ` tu bo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).