Re: [Qemu-devel] [PATCH] virtio: introduce grab/release_ioeventfd to fix vhost

[Felipe Franciosi]

The trace you showed is exactly the same I encountered on vhost-scsi. And the 
one my (hacky) patched fixed. The summary of my investigation is on the patch 
description. I'm OOO so apologies for not being of more help. F.

Sent from my phone

> On 11 Nov 2016, at 20:38, Alex Williamson <address@hidden> wrote:
> 
> On Fri, 11 Nov 2016 20:28:55 +0100
> Paolo Bonzini <address@hidden> wrote:
> 
>> Following the recent refactoring of virtio notifiers [1], more specifically
>> the patch ed08a2a0b ("virtio: use virtio_bus_set_host_notifier to
>> start/stop ioeventfd") that uses virtio_bus_set_host_notifier [2]
>> by default, core virtio code requires 'ioeventfd_started' to be set
>> to true/false when the host notifiers are configured.
>> 
>> When vhost is stopped and started, however, there is a stop followed by
>> another start. Since ioeventfd_started was never set to true, the 'stop'
>> operation triggered by virtio_bus_set_host_notifier() will not result
>> in a call to virtio_pci_ioeventfd_assign(assign=false). This leaves
>> the memory regions with stale notifiers and results on the next start
>> triggering the following assertion:
>> 
>>  kvm_mem_ioeventfd_add: error adding ioeventfd: File exists
>>  Aborted
>> 
>> This patch reintroduces (hopefully in a cleaner way) the concept
>> that was present with ioeventfd_disabled before the refactoring.
>> When ioeventfd_grabbed>0, ioeventfd_started tracks whether ioeventfd
>> should be enabled or not, but ioeventfd is actually not started at
>> all until vhost releases the host notifiers.
>> 
>> [1] http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg07748.html
>> [2] http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg07760.html
>> 
>> Reported-by: Felipe Franciosi <address@hidden>
>> Reported-by: Christian Borntraeger <address@hidden>
>> Reported-by: Alex Williamson <address@hidden>
>> Fixes: ed08a2a0b ("virtio: use virtio_bus_set_host_notifier to start/stop 
>> ioeventfd")
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> ---
>>    You know you really screwed up when you get three Reported-bys...
>>    The patch is largish, but I've added lots of comments.
>>    It overrides Felipe's patch which did not for example fix vhost
>>    multiqueue (tests/vhost-user-test).
>> 
>> hw/virtio/vhost.c              | 11 ++++------
>> hw/virtio/virtio-bus.c         | 50 
>> ++++++++++++++++++++++++++++++++++++------
>> hw/virtio/virtio.c             | 16 ++++++++++++++
>> include/hw/virtio/virtio-bus.h | 14 ++++++++++++
>> include/hw/virtio/virtio.h     |  2 ++
>> 5 files changed, 79 insertions(+), 14 deletions(-)
> 
> I'm not entirely sure what to apply this to, it has conflicts with
> MST's last pull request, so I applied it to mainline, which for me was:
> 
> 6bbcb76 MAINTAINERS: Remove obsolete stable branches
> 
> But this just gives me a different failure, QEMU reports:
> 
> kvm_mem_ioeventfd_add: error adding ioeventfd: File exists
> 
> gdb backtrace:
> 
> Thread 4 "CPU 1/KVM" received signal SIGABRT, Aborted.
> [Switching to Thread 0x7f60e4ce9700 (LWP 12239)]
> 0x00007f60f582b765 in __GI_raise (address@hidden) at 
> ../sysdeps/unix/sysv/linux/raise.c:54
> 54      return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
> (gdb) bt
> #0  0x00007f60f582b765 in __GI_raise (address@hidden) at 
> ../sysdeps/unix/sysv/linux/raise.c:54
> #1  0x00007f60f582d36a in __GI_abort () at abort.c:89
> #2  0x000055af0a40a8f0 in kvm_mem_ioeventfd_add (listener=0x55af0b4d6e70, 
> section=0x7f60e4ce5ef0, match_data=false, data=0, e=0x55af0c2e3690)
>    at /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:898
> #3  0x000055af0a4111d6 in address_space_add_del_ioeventfds (as=0x55af0aedef00 
> <address_space_memory>, fds_new=0x7f60d8000c00, fds_new_nb=9, 
> fds_old=0x7f60d4319e80, fds_old_nb=8)
>    at /net/gimli/home/alwillia/Work/qemu.git/memory.c:757
> #4  0x000055af0a4114e4 in address_space_update_ioeventfds (as=0x55af0aedef00 
> <address_space_memory>) at /net/gimli/home/alwillia/Work/qemu.git/memory.c:803
> #5  0x000055af0a411d1c in memory_region_transaction_commit () at 
> /net/gimli/home/alwillia/Work/qemu.git/memory.c:932
> #6  0x000055af0a414881 in memory_region_add_eventfd (mr=0x55af0bbcf740, 
> addr=0, size=0, match_data=false, data=0, e=0x55af0c2e3690) at 
> /net/gimli/home/alwillia/Work/qemu.git/memory.c:1984
> #7  0x000055af0a71b0a8 in virtio_pci_ioeventfd_assign (d=0x55af0bbcea20, 
> notifier=0x55af0c2e3690, n=0, assign=true) at hw/virtio/virtio-pci.c:300
> #8  0x000055af0a7216f1 in virtio_bus_set_host_notifier (bus=0x55af0bbd6db8, 
> n=0, assign=true) at hw/virtio/virtio-bus.c:257
> #9  0x000055af0a48180b in vhost_dev_enable_notifiers (hdev=0x55af0b4d4f10, 
> vdev=0x55af0bbd6e30) at 
> /net/gimli/home/alwillia/Work/qemu.git/hw/virtio/vhost.c:1198
> #10 0x000055af0a457941 in vhost_net_start_one (net=0x55af0b4d4f10, 
> dev=0x55af0bbd6e30) at 
> /net/gimli/home/alwillia/Work/qemu.git/hw/net/vhost_net.c:227
> #11 0x000055af0a457e2d in vhost_net_start (dev=0x55af0bbd6e30, 
> ncs=0x55af0c7e1890, total_queues=1) at 
> /net/gimli/home/alwillia/Work/qemu.git/hw/net/vhost_net.c:324
> #12 0x000055af0a4523f2 in virtio_net_vhost_status (n=0x55af0bbd6e30, status=7 
> '\a') at /net/gimli/home/alwillia/Work/qemu.git/hw/net/virtio-net.c:156
> #13 0x000055af0a45268e in virtio_net_set_status (vdev=0x55af0bbd6e30, 
> status=7 '\a') at 
> /net/gimli/home/alwillia/Work/qemu.git/hw/net/virtio-net.c:229
> #14 0x000055af0a478874 in virtio_set_status (vdev=0x55af0bbd6e30, val=7 '\a') 
> at /net/gimli/home/alwillia/Work/qemu.git/hw/virtio/virtio.c:898
> #15 0x000055af0a71b3c0 in virtio_ioport_write (opaque=0x55af0bbcea20, 
> addr=18, val=7) at hw/virtio/virtio-pci.c:383
> #16 0x000055af0a71b824 in virtio_pci_config_write (opaque=0x55af0bbcea20, 
> addr=18, val=7, size=1) at hw/virtio/virtio-pci.c:508
> #17 0x000055af0a4102fd in memory_region_write_accessor (mr=0x55af0bbcf310, 
> addr=18, value=0x7f60e4ce64b8, size=1, shift=0, mask=255, attrs=...)
>    at /net/gimli/home/alwillia/Work/qemu.git/memory.c:526
> #18 0x000055af0a410515 in access_with_adjusted_size (addr=18, 
> value=0x7f60e4ce64b8, size=1, access_size_min=1, access_size_max=4, access=
>    0x55af0a410213 <memory_region_write_accessor>, mr=0x55af0bbcf310, 
> attrs=...) at /net/gimli/home/alwillia/Work/qemu.git/memory.c:592
> #19 0x000055af0a412c55 in memory_region_dispatch_write (mr=0x55af0bbcf310, 
> addr=18, data=7, size=1, attrs=...) at 
> /net/gimli/home/alwillia/Work/qemu.git/memory.c:1323
> #20 0x000055af0a3be583 in address_space_write_continue (as=0x55af0aedede0 
> <address_space_io>, addr=49458, attrs=..., buf=0x7f6110aa0000 "\a", len=1, 
> addr1=18, l=1, mr=0x55af0bbcf310)
>    at /net/gimli/home/alwillia/Work/qemu.git/exec.c:2621
> #21 0x000055af0a3be6cb in address_space_write (as=0x55af0aedede0 
> <address_space_io>, addr=49458, attrs=..., buf=0x7f6110aa0000 "\a", len=1)
>    at /net/gimli/home/alwillia/Work/qemu.git/exec.c:2666
> #22 0x000055af0a3bea57 in address_space_rw (as=0x55af0aedede0 
> <address_space_io>, addr=49458, attrs=..., buf=0x7f6110aa0000 "\a", len=1, 
> is_write=true)
>    at /net/gimli/home/alwillia/Work/qemu.git/exec.c:2768
> #23 0x000055af0a40c8d7 in kvm_handle_io (port=49458, attrs=..., 
> data=0x7f6110aa0000, direction=1, size=1, count=1) at 
> /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:1800
> #24 0x000055af0a40cddd in kvm_cpu_exec (cpu=0x55af0b9c33f0) at 
> /net/gimli/home/alwillia/Work/qemu.git/kvm-all.c:1958
> #25 0x000055af0a3f3c58 in qemu_kvm_cpu_thread_fn (arg=0x55af0b9c33f0) at 
> /net/gimli/home/alwillia/Work/qemu.git/cpus.c:998
> #26 0x00007f60f5bc05ca in start_thread (arg=0x7f60e4ce9700) at 
> pthread_create.c:333
> #27 0x00007f60f58fa0ed in clone () at 
> ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
> 
> If I'm applying it to the wrong place, let me know.  Thanks,
> 
> Alex
> 
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index 131f164..a8b5ab8 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -1186,17 +1186,14 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
>> int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
>> {
>>     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
>> -    VirtioBusState *vbus = VIRTIO_BUS(qbus);
>> -    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
>>     int i, r, e;
>> 
>> -    if (!k->ioeventfd_assign) {
>> +    r = virtio_device_grab_ioeventfd(vdev);
>> +    if (r < 0) {
>>         error_report("binding does not support host notifiers");
>> -        r = -ENOSYS;
>>         goto fail;
>>     }
>> 
>> -    virtio_device_stop_ioeventfd(vdev);
>>     for (i = 0; i < hdev->nvqs; ++i) {
>>         r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + 
>> i,
>>                                          true);
>> @@ -1216,7 +1213,7 @@ fail_vq:
>>         }
>>         assert (e >= 0);
>>     }
>> -    virtio_device_start_ioeventfd(vdev);
>> +    virtio_device_release_ioeventfd(vdev);
>> fail:
>>     return r;
>> }
>> @@ -1239,7 +1236,7 @@ void vhost_dev_disable_notifiers(struct vhost_dev 
>> *hdev, VirtIODevice *vdev)
>>         }
>>         assert (r >= 0);
>>     }
>> -    virtio_device_start_ioeventfd(vdev);
>> +    virtio_device_release_ioeventfd(vdev);
>> }
>> 
>> /* Test and clear event pending status.
>> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
>> index bf61f66..42467f1 100644
>> --- a/hw/virtio/virtio-bus.c
>> +++ b/hw/virtio/virtio-bus.c
>> @@ -147,6 +147,38 @@ void virtio_bus_set_vdev_config(VirtioBusState *bus, 
>> uint8_t *config)
>>     }
>> }
>> 
>> +int virtio_bus_grab_ioeventfd(VirtioBusState *bus)
>> +{
>> +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
>> +
>> +    /* vhost can be used even if ioeventfd=off in the proxy device,
>> +     * so do not check k->ioeventfd_enabled.
>> +     */
>> +    if (!k->ioeventfd_assign) {
>> +        return -ENOSYS;
>> +    }
>> +
>> +    if (bus->ioeventfd_grabbed == 0 && bus->ioeventfd_started) {
>> +        virtio_bus_stop_ioeventfd(bus);
>> +        /* Remember that we need to restart ioeventfd
>> +         * when ioeventfd_grabbed becomes zero.
>> +         */
>> +        bus->ioeventfd_started = true;
>> +    }
>> +    bus->ioeventfd_grabbed++;
>> +    return 0;
>> +}
>> +
>> +void virtio_bus_release_ioeventfd(VirtioBusState *bus)
>> +{
>> +    assert(bus->ioeventfd_grabbed != 0);
>> +    if (--bus->ioeventfd_grabbed == 0 && bus->ioeventfd_started) {
>> +        /* Force virtio_bus_start_ioeventfd to act.  */
>> +        bus->ioeventfd_started = false;
>> +        virtio_bus_start_ioeventfd(bus);
>> +    }
>> +}
>> +
>> int virtio_bus_start_ioeventfd(VirtioBusState *bus)
>> {
>>     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
>> @@ -161,10 +193,12 @@ int virtio_bus_start_ioeventfd(VirtioBusState *bus)
>>     if (bus->ioeventfd_started) {
>>         return 0;
>>     }
>> -    r = vdc->start_ioeventfd(vdev);
>> -    if (r < 0) {
>> -        error_report("%s: failed. Fallback to userspace (slower).", 
>> __func__);
>> -        return r;
>> +    if (!bus->ioeventfd_grabbed) {
>> +        r = vdc->start_ioeventfd(vdev);
>> +        if (r < 0) {
>> +            error_report("%s: failed. Fallback to userspace (slower).", 
>> __func__);
>> +            return r;
>> +        }
>>     }
>>     bus->ioeventfd_started = true;
>>     return 0;
>> @@ -179,9 +213,11 @@ void virtio_bus_stop_ioeventfd(VirtioBusState *bus)
>>         return;
>>     }
>> 
>> -    vdev = virtio_bus_get_device(bus);
>> -    vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
>> -    vdc->stop_ioeventfd(vdev);
>> +    if (!bus->ioeventfd_grabbed) {
>> +        vdev = virtio_bus_get_device(bus);
>> +        vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
>> +        vdc->stop_ioeventfd(vdev);
>> +    }
>>     bus->ioeventfd_started = false;
>> }
>> 
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index bcbcfe0..89b0b80 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -2202,6 +2202,22 @@ void virtio_device_stop_ioeventfd(VirtIODevice *vdev)
>>     virtio_bus_stop_ioeventfd(vbus);
>> }
>> 
>> +int virtio_device_grab_ioeventfd(VirtIODevice *vdev)
>> +{
>> +    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
>> +    VirtioBusState *vbus = VIRTIO_BUS(qbus);
>> +
>> +    return virtio_bus_grab_ioeventfd(vbus);
>> +}
>> +
>> +void virtio_device_release_ioeventfd(VirtIODevice *vdev)
>> +{
>> +    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
>> +    VirtioBusState *vbus = VIRTIO_BUS(qbus);
>> +
>> +    virtio_bus_release_ioeventfd(vbus);
>> +}
>> +
>> static void virtio_device_class_init(ObjectClass *klass, void *data)
>> {
>>     /* Set the default value here. */
>> diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
>> index fdf7fda..8a51e2c 100644
>> --- a/include/hw/virtio/virtio-bus.h
>> +++ b/include/hw/virtio/virtio-bus.h
>> @@ -97,6 +97,16 @@ struct VirtioBusState {
>>      * Set if ioeventfd has been started.
>>      */
>>     bool ioeventfd_started;
>> +
>> +    /*
>> +     * Set if ioeventfd has been grabbed by vhost.  When ioeventfd
>> +     * is grabbed by vhost, we track its started/stopped state (which
>> +     * depends in turn on the virtio status register), but do not
>> +     * register a handler for the ioeventfd.  When ioeventfd is
>> +     * released, if ioeventfd_started is true we finally register
>> +     * the handler so that QEMU's device model can use ioeventfd.
>> +     */
>> +    int ioeventfd_grabbed;
>> };
>> 
>> void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp);
>> @@ -131,6 +141,10 @@ bool virtio_bus_ioeventfd_enabled(VirtioBusState *bus);
>> int virtio_bus_start_ioeventfd(VirtioBusState *bus);
>> /* Stop the ioeventfd. */
>> void virtio_bus_stop_ioeventfd(VirtioBusState *bus);
>> +/* Tell the bus that vhost is grabbing the ioeventfd. */
>> +int virtio_bus_grab_ioeventfd(VirtioBusState *bus);
>> +/* bus that vhost is not using the ioeventfd anymore. */
>> +void virtio_bus_release_ioeventfd(VirtioBusState *bus);
>> /* Switch from/to the generic ioeventfd handler */
>> int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign);
>> 
>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> index ac65d6a..35ede30 100644
>> --- a/include/hw/virtio/virtio.h
>> +++ b/include/hw/virtio/virtio.h
>> @@ -270,6 +270,8 @@ void 
>> virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign,
>>                                                 bool with_irqfd);
>> int virtio_device_start_ioeventfd(VirtIODevice *vdev);
>> void virtio_device_stop_ioeventfd(VirtIODevice *vdev);
>> +int virtio_device_grab_ioeventfd(VirtIODevice *vdev);
>> +void virtio_device_release_ioeventfd(VirtIODevice *vdev);
>> bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev);
>> EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq);
>> void virtio_queue_host_notifier_read(EventNotifier *n);
>