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

[tu bo <tubo@linux.vnet.ibm.com>]

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);
>       /*
>