[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] virtio: introduce grab/release_ioeventfd to fix
|
From: |
Alex Williamson |
|
Subject: |
Re: [Qemu-devel] [PATCH] virtio: introduce grab/release_ioeventfd to fix vhost |
|
Date: |
Fri, 11 Nov 2016 16:33:12 -0700 |
On Sat, 12 Nov 2016 00:03:14 +0100
Paolo Bonzini <address@hidden> wrote:
> On 11/11/2016 22:11, Alex Williamson wrote:
> > On Fri, 11 Nov 2016 20:46:53 +0000
> > Felipe Franciosi <address@hidden> wrote:
> >
> >> 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.
> >>
> >
> > Paolo's patch states that it overrides your patch, so I used Paolo's
> > patch alone, but adding yours doesn't change anything, same assertion.
> > Thanks,
>
> Hmm it's late here and I could only reproduce with multiqueue. Until I
> reinstall the Windows VM with OVMF I'm grasping at straws, but these lines
> should not be needed anymore now, so perhaps you can try this on top...
Yes, 6bbcb76 + your original patch below + this additional patch let
the VM boot with vhost enabled. Thanks,
Alex
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index 42467f1..c8a446e 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -247,7 +247,6 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int
> n, bool assign)
> }
>
> if (assign) {
> - assert(!bus->ioeventfd_started);
> r = event_notifier_init(notifier, 1);
> if (r < 0) {
> error_report("%s: unable to init event notifier: %s (%d)",
> @@ -261,9 +260,6 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int
> n, bool assign)
> }
> return 0;
> } else {
> - if (!bus->ioeventfd_started) {
> - return 0;
> - }
> k->ioeventfd_assign(proxy, notifier, n, false);
> }
>
>
> Michael, not yet s-o-b since I at least have to test vhost-scsi.
>
> Paolo
>
> > Alex
> >
> >>> 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);
> >>>
> >
> >
> >