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

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

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