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

[Cornelia Huck]

On Thu, 30 Jun 2016 16:12:31 +0200
Marc-André Lureau <address@hidden> wrote:

> 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.

Grgh, the whole semantics of host notifiers are a mess.

Before, the host notifier callbacks would (on assign) deregister the
old eventfd and then register a new notifier - regardless whether the
device had disabled ioeventfd. Now, we try to keep a preexisting
eventfd registered, but don't try to force eventfds on a device that
disabled ioeventfd - and that is what breaks vhost-user, apparently.

I think the best way to deal with this is to have the now common host
notifier setter revert to the old semantics for now. This re-introduces
the 'no ioeventfd for a while' hole (which does not seem to show up in
the wild) but fixes vhost in its various incarnations. We have time to
come up with a better solution then (while still keeping the benefits
of the unified host notifier handling).

I'll cook up a patch.