| Message ID | 1467300702-97031-1-git-send-email-cornelia.huck@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
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
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
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>
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)
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(-)