* [Qemu-devel] [PATCH] virtio: introduce grab/release_ioeventfd to fix vhost
@ 2016-11-11 19:28 Paolo Bonzini
2016-11-11 20:38 ` Alex Williamson
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2016-11-11 19:28 UTC (permalink / raw)
To: qemu-devel; +Cc: felipe, borntraeger, alex.williamson
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 <felipe@nutanix.com>
Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reported-by: Alex Williamson <alex.williamson@redhat.com>
Fixes: ed08a2a0b ("virtio: use virtio_bus_set_host_notifier to start/stop ioeventfd")
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
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(-)
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);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: introduce grab/release_ioeventfd to fix vhost
2016-11-11 19:28 [Qemu-devel] [PATCH] virtio: introduce grab/release_ioeventfd to fix vhost Paolo Bonzini
@ 2016-11-11 20:38 ` Alex Williamson
2016-11-11 20:46 ` Felipe Franciosi
0 siblings, 1 reply; 6+ messages in thread
From: Alex Williamson @ 2016-11-11 20:38 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, felipe, borntraeger
On Fri, 11 Nov 2016 20:28:55 +0100
Paolo Bonzini <pbonzini@redhat.com> 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 <felipe@nutanix.com>
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Reported-by: Alex Williamson <alex.williamson@redhat.com>
> Fixes: ed08a2a0b ("virtio: use virtio_bus_set_host_notifier to start/stop ioeventfd")
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> 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 (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
54 return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
(gdb) bt
#0 0x00007f60f582b765 in __GI_raise (sig=sig@entry=6) 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);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: introduce grab/release_ioeventfd to fix vhost
2016-11-11 20:38 ` Alex Williamson
@ 2016-11-11 20:46 ` Felipe Franciosi
2016-11-11 21:11 ` Alex Williamson
0 siblings, 1 reply; 6+ messages in thread
From: Felipe Franciosi @ 2016-11-11 20:46 UTC (permalink / raw)
To: Alex Williamson
Cc: Paolo Bonzini, qemu-devel@nongnu.org, borntraeger@de.ibm.com
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 <alex.williamson@redhat.com> wrote:
>
> On Fri, 11 Nov 2016 20:28:55 +0100
> Paolo Bonzini <pbonzini@redhat.com> 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 <felipe@nutanix.com>
>> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Reported-by: Alex Williamson <alex.williamson@redhat.com>
>> Fixes: ed08a2a0b ("virtio: use virtio_bus_set_host_notifier to start/stop ioeventfd")
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>> 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 (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
> 54 return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
> (gdb) bt
> #0 0x00007f60f582b765 in __GI_raise (sig=sig@entry=6) 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);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: introduce grab/release_ioeventfd to fix vhost
2016-11-11 20:46 ` Felipe Franciosi
@ 2016-11-11 21:11 ` Alex Williamson
2016-11-11 23:03 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Alex Williamson @ 2016-11-11 21:11 UTC (permalink / raw)
To: Felipe Franciosi
Cc: Paolo Bonzini, qemu-devel@nongnu.org, borntraeger@de.ibm.com
On Fri, 11 Nov 2016 20:46:53 +0000
Felipe Franciosi <felipe@nutanix.com> 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,
Alex
> > On 11 Nov 2016, at 20:38, Alex Williamson <alex.williamson@redhat.com> wrote:
> >
> > On Fri, 11 Nov 2016 20:28:55 +0100
> > Paolo Bonzini <pbonzini@redhat.com> 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 <felipe@nutanix.com>
> >> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >> Reported-by: Alex Williamson <alex.williamson@redhat.com>
> >> Fixes: ed08a2a0b ("virtio: use virtio_bus_set_host_notifier to start/stop ioeventfd")
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >> 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 (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
> > 54 return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
> > (gdb) bt
> > #0 0x00007f60f582b765 in __GI_raise (sig=sig@entry=6) 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);
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: introduce grab/release_ioeventfd to fix vhost
2016-11-11 21:11 ` Alex Williamson
@ 2016-11-11 23:03 ` Paolo Bonzini
2016-11-11 23:33 ` Alex Williamson
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2016-11-11 23:03 UTC (permalink / raw)
To: Alex Williamson, Felipe Franciosi
Cc: qemu-devel@nongnu.org, borntraeger@de.ibm.com, Michael S. Tsirkin
On 11/11/2016 22:11, Alex Williamson wrote:
> On Fri, 11 Nov 2016 20:46:53 +0000
> Felipe Franciosi <felipe@nutanix.com> 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...
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 <alex.williamson@redhat.com> wrote:
>>>
>>> On Fri, 11 Nov 2016 20:28:55 +0100
>>> Paolo Bonzini <pbonzini@redhat.com> 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 <felipe@nutanix.com>
>>>> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> Reported-by: Alex Williamson <alex.williamson@redhat.com>
>>>> Fixes: ed08a2a0b ("virtio: use virtio_bus_set_host_notifier to start/stop ioeventfd")
>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>> ---
>>>> 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 (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
>>> 54 return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
>>> (gdb) bt
>>> #0 0x00007f60f582b765 in __GI_raise (sig=sig@entry=6) 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);
>>>
>
>
>
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: introduce grab/release_ioeventfd to fix vhost
2016-11-11 23:03 ` Paolo Bonzini
@ 2016-11-11 23:33 ` Alex Williamson
0 siblings, 0 replies; 6+ messages in thread
From: Alex Williamson @ 2016-11-11 23:33 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Felipe Franciosi, qemu-devel@nongnu.org, borntraeger@de.ibm.com,
Michael S. Tsirkin
On Sat, 12 Nov 2016 00:03:14 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 11/11/2016 22:11, Alex Williamson wrote:
> > On Fri, 11 Nov 2016 20:46:53 +0000
> > Felipe Franciosi <felipe@nutanix.com> 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 <alex.williamson@redhat.com> wrote:
> >>>
> >>> On Fri, 11 Nov 2016 20:28:55 +0100
> >>> Paolo Bonzini <pbonzini@redhat.com> 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 <felipe@nutanix.com>
> >>>> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >>>> Reported-by: Alex Williamson <alex.williamson@redhat.com>
> >>>> Fixes: ed08a2a0b ("virtio: use virtio_bus_set_host_notifier to start/stop ioeventfd")
> >>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >>>> ---
> >>>> 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 (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
> >>> 54 return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
> >>> (gdb) bt
> >>> #0 0x00007f60f582b765 in __GI_raise (sig=sig@entry=6) 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);
> >>>
> >
> >
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-11-11 23:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-11 19:28 [Qemu-devel] [PATCH] virtio: introduce grab/release_ioeventfd to fix vhost Paolo Bonzini
2016-11-11 20:38 ` Alex Williamson
2016-11-11 20:46 ` Felipe Franciosi
2016-11-11 21:11 ` Alex Williamson
2016-11-11 23:03 ` Paolo Bonzini
2016-11-11 23:33 ` Alex Williamson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).