Re: [Qemu-devel] [PATCH v2] virtio: Fix setting up host notifiers for vhost

[Marc-André Lureau]

Hi

On Thu, Jun 30, 2016 at 1:52 PM, Cornelia Huck <address@hidden> 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 <address@hidden>
> Reported-by: Marc-André Lureau <address@hidden>
> Signed-off-by: Cornelia Huck <address@hidden>

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