* [Qemu-devel] [PATCH] virtio: revert host notifiers to old semantics
@ 2016-06-30 15:31 Cornelia Huck
2016-07-01 8:42 ` Marc-André Lureau
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Cornelia Huck @ 2016-06-30 15:31 UTC (permalink / raw)
To: qemu-devel
Cc: mst, famz, stefanha, jasowang, pl, marcandre.lureau, pbonzini,
Cornelia Huck
The host notifier rework tried both to unify host notifiers across
transports and plug a possible hole during host notifier
re-assignment. Unfortunately, this meant a change in semantics that
breaks vhost and iSCSI+dataplane.
As the minimal fix, keep the common host notifier code but revert
to the old semantics so that we have time to figure out the proper
fix.
Fixes: 6798e245a3 ("virtio-bus: common ioeventfd infrastructure")
Reported-by: Peter Lieven <pl@kamp.de>
Reported-by: Jason Wang <jasowang@redhat.com>
Reported-by: Marc-André Lureau <marcandre.lureau@gmail.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
hw/virtio/virtio-bus.c | 26 ++++++++++----------------
1 file changed, 10 insertions(+), 16 deletions(-)
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 1313760..a85b7c8 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -176,8 +176,8 @@ static int set_host_notifier_internal(DeviceState *proxy, VirtioBusState *bus,
return r;
}
} else {
- virtio_queue_set_host_notifier_fd_handler(vq, false, false);
k->ioeventfd_assign(proxy, notifier, n, assign);
+ virtio_queue_set_host_notifier_fd_handler(vq, false, false);
event_notifier_cleanup(notifier);
}
return r;
@@ -251,31 +251,25 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
{
VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
DeviceState *proxy = DEVICE(BUS(bus)->parent);
- VirtIODevice *vdev = virtio_bus_get_device(bus);
- VirtQueue *vq = virtio_get_queue(vdev, n);
if (!k->ioeventfd_started) {
return -ENOSYS;
}
+ k->ioeventfd_set_disabled(proxy, assign);
if (assign) {
/*
* Stop using the generic ioeventfd, we are doing eventfd handling
* ourselves below
+ *
+ * FIXME: We should just switch the handler and not deassign the
+ * ioeventfd.
+ * Otherwise, there's a window where we don't have an
+ * ioeventfd and we may end up with a notification where
+ * we don't expect one.
*/
- k->ioeventfd_set_disabled(proxy, true);
- }
- /*
- * Just switch the handler, don't deassign the ioeventfd.
- * Otherwise, there's a window where we don't have an
- * ioeventfd and we may end up with a notification where
- * we don't expect one.
- */
- virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign);
- if (!assign) {
- /* Use generic ioeventfd handler again. */
- k->ioeventfd_set_disabled(proxy, false);
+ virtio_bus_stop_ioeventfd(bus);
}
- return 0;
+ return set_host_notifier_internal(proxy, bus, n, assign, false);
}
static char *virtio_bus_get_dev_path(DeviceState *dev)
--
2.6.6
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: revert host notifiers to old semantics
2016-06-30 15:31 [Qemu-devel] [PATCH] virtio: revert host notifiers to old semantics Cornelia Huck
@ 2016-07-01 8:42 ` Marc-André Lureau
2016-07-04 6:26 ` Peter Lieven
2016-07-04 6:59 ` Jason Wang
2 siblings, 0 replies; 4+ messages in thread
From: Marc-André Lureau @ 2016-07-01 8:42 UTC (permalink / raw)
To: Cornelia Huck
Cc: QEMU, Michael S. Tsirkin, Fam Zheng, Stefan Hajnoczi, Jason Wang,
pl, Paolo Bonzini
Hi
On Thu, Jun 30, 2016 at 5:31 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> The host notifier rework tried both to unify host notifiers across
> transports and plug a possible hole during host notifier
> re-assignment. Unfortunately, this meant a change in semantics that
> breaks vhost and iSCSI+dataplane.
>
> As the minimal fix, keep the common host notifier code but revert
> to the old semantics so that we have time to figure out the proper
> fix.
>
> Fixes: 6798e245a3 ("virtio-bus: common ioeventfd infrastructure")
> Reported-by: Peter Lieven <pl@kamp.de>
> Reported-by: Jason Wang <jasowang@redhat.com>
> Reported-by: Marc-André Lureau <marcandre.lureau@gmail.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
> hw/virtio/virtio-bus.c | 26 ++++++++++----------------
> 1 file changed, 10 insertions(+), 16 deletions(-)
>
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index 1313760..a85b7c8 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -176,8 +176,8 @@ static int set_host_notifier_internal(DeviceState *proxy, VirtioBusState *bus,
> return r;
> }
> } else {
> - virtio_queue_set_host_notifier_fd_handler(vq, false, false);
> k->ioeventfd_assign(proxy, notifier, n, assign);
> + virtio_queue_set_host_notifier_fd_handler(vq, false, false);
> event_notifier_cleanup(notifier);
> }
> return r;
> @@ -251,31 +251,25 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
> {
> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
> DeviceState *proxy = DEVICE(BUS(bus)->parent);
> - VirtIODevice *vdev = virtio_bus_get_device(bus);
> - VirtQueue *vq = virtio_get_queue(vdev, n);
>
> if (!k->ioeventfd_started) {
> return -ENOSYS;
> }
> + k->ioeventfd_set_disabled(proxy, assign);
> if (assign) {
> /*
> * Stop using the generic ioeventfd, we are doing eventfd handling
> * ourselves below
> + *
> + * FIXME: We should just switch the handler and not deassign the
> + * ioeventfd.
> + * Otherwise, there's a window where we don't have an
> + * ioeventfd and we may end up with a notification where
> + * we don't expect one.
> */
> - k->ioeventfd_set_disabled(proxy, true);
> - }
> - /*
> - * Just switch the handler, don't deassign the ioeventfd.
> - * Otherwise, there's a window where we don't have an
> - * ioeventfd and we may end up with a notification where
> - * we don't expect one.
> - */
> - virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign);
> - if (!assign) {
> - /* Use generic ioeventfd handler again. */
> - k->ioeventfd_set_disabled(proxy, false);
> + virtio_bus_stop_ioeventfd(bus);
> }
> - return 0;
> + return set_host_notifier_internal(proxy, bus, n, assign, false);
> }
>
> static char *virtio_bus_get_dev_path(DeviceState *dev)
> --
> 2.6.6
>
Tested-by: Marc-André Lureau <marcandre.lureau@redhat.com>
thanks
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: revert host notifiers to old semantics
2016-06-30 15:31 [Qemu-devel] [PATCH] virtio: revert host notifiers to old semantics Cornelia Huck
2016-07-01 8:42 ` Marc-André Lureau
@ 2016-07-04 6:26 ` Peter Lieven
2016-07-04 6:59 ` Jason Wang
2 siblings, 0 replies; 4+ messages in thread
From: Peter Lieven @ 2016-07-04 6:26 UTC (permalink / raw)
To: Cornelia Huck, qemu-devel
Cc: mst, famz, stefanha, jasowang, marcandre.lureau, pbonzini
Am 30.06.2016 um 17:31 schrieb Cornelia Huck:
> The host notifier rework tried both to unify host notifiers across
> transports and plug a possible hole during host notifier
> re-assignment. Unfortunately, this meant a change in semantics that
> breaks vhost and iSCSI+dataplane.
>
> As the minimal fix, keep the common host notifier code but revert
> to the old semantics so that we have time to figure out the proper
> fix.
>
> Fixes: 6798e245a3 ("virtio-bus: common ioeventfd infrastructure")
> Reported-by: Peter Lieven <pl@kamp.de>
> Reported-by: Jason Wang <jasowang@redhat.com>
> Reported-by: Marc-André Lureau <marcandre.lureau@gmail.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Works for iscsi + dateplane.
Peter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: revert host notifiers to old semantics
2016-06-30 15:31 [Qemu-devel] [PATCH] virtio: revert host notifiers to old semantics Cornelia Huck
2016-07-01 8:42 ` Marc-André Lureau
2016-07-04 6:26 ` Peter Lieven
@ 2016-07-04 6:59 ` Jason Wang
2 siblings, 0 replies; 4+ messages in thread
From: Jason Wang @ 2016-07-04 6:59 UTC (permalink / raw)
To: Cornelia Huck, qemu-devel
Cc: famz, mst, pl, marcandre.lureau, stefanha, pbonzini
On 2016年06月30日 23:31, Cornelia Huck wrote:
> The host notifier rework tried both to unify host notifiers across
> transports and plug a possible hole during host notifier
> re-assignment. Unfortunately, this meant a change in semantics that
> breaks vhost and iSCSI+dataplane.
>
> As the minimal fix, keep the common host notifier code but revert
> to the old semantics so that we have time to figure out the proper
> fix.
>
> Fixes: 6798e245a3 ("virtio-bus: common ioeventfd infrastructure")
> Reported-by: Peter Lieven <pl@kamp.de>
> Reported-by: Jason Wang <jasowang@redhat.com>
> Reported-by: Marc-André Lureau <marcandre.lureau@gmail.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
> hw/virtio/virtio-bus.c | 26 ++++++++++----------------
> 1 file changed, 10 insertions(+), 16 deletions(-)
>
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index 1313760..a85b7c8 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -176,8 +176,8 @@ static int set_host_notifier_internal(DeviceState *proxy, VirtioBusState *bus,
> return r;
> }
> } else {
> - virtio_queue_set_host_notifier_fd_handler(vq, false, false);
> k->ioeventfd_assign(proxy, notifier, n, assign);
> + virtio_queue_set_host_notifier_fd_handler(vq, false, false);
> event_notifier_cleanup(notifier);
> }
> return r;
> @@ -251,31 +251,25 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
> {
> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
> DeviceState *proxy = DEVICE(BUS(bus)->parent);
> - VirtIODevice *vdev = virtio_bus_get_device(bus);
> - VirtQueue *vq = virtio_get_queue(vdev, n);
>
> if (!k->ioeventfd_started) {
> return -ENOSYS;
> }
> + k->ioeventfd_set_disabled(proxy, assign);
> if (assign) {
> /*
> * Stop using the generic ioeventfd, we are doing eventfd handling
> * ourselves below
> + *
> + * FIXME: We should just switch the handler and not deassign the
> + * ioeventfd.
> + * Otherwise, there's a window where we don't have an
> + * ioeventfd and we may end up with a notification where
> + * we don't expect one.
> */
> - k->ioeventfd_set_disabled(proxy, true);
> - }
> - /*
> - * Just switch the handler, don't deassign the ioeventfd.
> - * Otherwise, there's a window where we don't have an
> - * ioeventfd and we may end up with a notification where
> - * we don't expect one.
> - */
> - virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign);
> - if (!assign) {
> - /* Use generic ioeventfd handler again. */
> - k->ioeventfd_set_disabled(proxy, false);
> + virtio_bus_stop_ioeventfd(bus);
> }
> - return 0;
> + return set_host_notifier_internal(proxy, bus, n, assign, false);
> }
>
> static char *virtio_bus_get_dev_path(DeviceState *dev)
Reviewed-by: Jason Wang <jasowang@redhat.com>
Tested-by: Jason Wang <jasowang@redhat.com>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-07-04 6:59 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-30 15:31 [Qemu-devel] [PATCH] virtio: revert host notifiers to old semantics Cornelia Huck
2016-07-01 8:42 ` Marc-André Lureau
2016-07-04 6:26 ` Peter Lieven
2016-07-04 6:59 ` Jason Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.