[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/1] virtio: fix ioeventfd assignment race
From: |
Christian Borntraeger |
Subject: |
Re: [Qemu-devel] [PATCH 1/1] virtio: fix ioeventfd assignment race |
Date: |
Tue, 29 Mar 2016 17:53:01 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.0 |
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 <address@hidden>
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);
> /*
>