* 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