Re: [Qemu-devel] [PATCH 1/1] virtio: fix ioeventfd assignment race

[Paolo Bonzini <pbonzini@redhat.com>]

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