Hi
On Thu, Jun 30, 2016 at 1:52 PM, Cornelia Huck <cornelia.h...@de.ibm.com> wrote:
> When setting up host notifiers, virtio_bus_set_host_notifier()
> simply switches the handler. This will only work, however, if
> the ioeventfd has already been setup; this is true for dataplane,
> but not for vhost, and will completely break things if the
> ioeventfd is disabled for the device.
>
> Fix this by starting the ioeventfd on assign if that has not
> happened before, and only switch the handler if the ioeventfd
> has really been started.
>
> While we're at it, also fixup the unsetting path of
> set_host_notifier_internal().
>
> Fixes: 6798e245a3 ("virtio-bus: common ioeventfd infrastructure")
> Reported-by: Jason Wang <jasow...@redhat.com>
> Reported-by: Marc-André Lureau <marcandre.lur...@gmail.com>
> Signed-off-by: Cornelia Huck <cornelia.h...@de.ibm.com>
That's indeed enough to fix vhost-user-test, however, vhost-user is still broken
Start tests/vhost-user-bridge and then qemu -enable-kvm -m 1024
-object memory-backend-file,id=mem,size=1024M,mem-path=/tmp,share=on
-numa node,memdev=mem -mem-prealloc -chardev
socket,id=char0,path=/tmp/vubr.sock -netdev
type=vhost-user,id=mynet1,chardev=char0,vhostforce -device
virtio-net-pci,netdev=mynet1
Before your series, you can see data coming after init completed, now
it stops at:
vhost-user-bridge: tests/vhost-user-bridge.c:1014:
vubr_set_vring_kick_exec: Assertion `(u64_arg &
VHOST_USER_VRING_NOFD_MASK) == 0' failed.
> ---
> Changes v1->v2:
> - don't switch the handler if the eventfd has not been setup - this
> fixes the failure of vhost-user-test for me
> - add more comments
> ---
> hw/virtio/virtio-bus.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index 1313760..dfe24fc 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;
> @@ -246,6 +246,9 @@ void virtio_bus_stop_ioeventfd(VirtioBusState *bus)
> /*
> * This function switches from/to the generic ioeventfd handler.
> * assign==false means 'use generic ioeventfd handler'.
> + * It is only considered an error if the binding does not support
> + * host notifiers at all, not when they are not available for the
> + * individual device.
> */
> int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
> {
> @@ -259,6 +262,13 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus,
> int n, bool assign)
> }
> if (assign) {
> /*
> + * Not yet started? assign==true implies we want to use an
> + * eventfd, so let's do the requisite setup.
> + */
> + if (!k->ioeventfd_started(proxy)) {
> + virtio_bus_start_ioeventfd(bus);
> + }
> + /*
> * Stop using the generic ioeventfd, we are doing eventfd handling
> * ourselves below
> */
> @@ -269,8 +279,13 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus,
> int n, bool assign)
> * 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.
> + *
> + * Also note that we should only do something with the eventfd
> + * handler if the eventfd has actually been setup.
> */
> - virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign);
> + if (k->ioeventfd_started(proxy)) {
> + virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign);
> + }
> if (!assign) {
> /* Use generic ioeventfd handler again. */
> k->ioeventfd_set_disabled(proxy, false);
> --
> 2.6.6
>
--
Marc-André Lureau
