On Tue, 15 Nov 2016 14:46:27 +0100
Paolo Bonzini <pbonz...@redhat.com> wrote:
> Following the recent refactoring of virtio notifiers [1], more specifically
> the patch ed08a2a0b ("virtio: use virtio_bus_set_host_notifier to
> start/stop ioeventfd") that uses virtio_bus_set_host_notifier [2]
> by default, core virtio code requires 'ioeventfd_started' to be set
> to true/false when the host notifiers are configured.
>
> When vhost is stopped and started, however, there is a stop followed by
> another start. Since ioeventfd_started was never set to true, the 'stop'
> operation triggered by virtio_bus_set_host_notifier() will not result
> in a call to virtio_pci_ioeventfd_assign(assign=false). This leaves
> the memory regions with stale notifiers and results on the next start
> triggering the following assertion:
>
> kvm_mem_ioeventfd_add: error adding ioeventfd: File exists
> Aborted
>
> This patch reintroduces (hopefully in a cleaner way) the concept
> that was present with ioeventfd_disabled before the refactoring.
> When ioeventfd_grabbed>0, ioeventfd_started tracks whether ioeventfd
> should be enabled or not, but ioeventfd is actually not started at
> all until vhost releases the host notifiers.
>
> [1] http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg07748.html
> [2] http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg07760.html
>
> Reported-by: Felipe Franciosi <fel...@nutanix.com>
> Reported-by: Christian Borntraeger <borntrae...@de.ibm.com>
> Reported-by: Alex Williamson <alex.william...@redhat.com>
> Fixes: ed08a2a0b ("virtio: use virtio_bus_set_host_notifier to start/stop
> ioeventfd")
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> Message-Id: <20161111192855.26350-1-pbonz...@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> ---
> hw/virtio/vhost.c | 11 ++++-----
> hw/virtio/virtio-bus.c | 54
> +++++++++++++++++++++++++++++++++---------
> hw/virtio/virtio.c | 16 +++++++++++++
> include/hw/virtio/virtio-bus.h | 14 +++++++++++
> include/hw/virtio/virtio.h | 2 ++
> 5 files changed, 79 insertions(+), 18 deletions(-)
This basically looks sane to me, but it is really hard to wrap one's
brain around this, so maybe this needs more comments? (I have added
some suggestions.)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 131f164..a8b5ab8 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1186,17 +1186,14 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
> int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
> {
> BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
> - VirtioBusState *vbus = VIRTIO_BUS(qbus);
> - VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
> int i, r, e;
>
> - if (!k->ioeventfd_assign) {
/* About to use our notifiers; make sure the core doesn't interfere. */
> + r = virtio_device_grab_ioeventfd(vdev);
> + if (r < 0) {
> error_report("binding does not support host notifiers");
> - r = -ENOSYS;
> goto fail;
> }
>
> - virtio_device_stop_ioeventfd(vdev);
> for (i = 0; i < hdev->nvqs; ++i) {
> r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index +
> i,
> true);
(...)
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index bf61f66..c8a446e 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -147,6 +147,38 @@ void virtio_bus_set_vdev_config(VirtioBusState *bus,
> uint8_t *config)
> }
> }
>
/* On success, ioeventfd ownership belongs to the caller. */
> +int virtio_bus_grab_ioeventfd(VirtioBusState *bus)
> +{
> + VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
> +
> + /* vhost can be used even if ioeventfd=off in the proxy device,
> + * so do not check k->ioeventfd_enabled.
> + */
> + if (!k->ioeventfd_assign) {
> + return -ENOSYS;
> + }
> +
> + if (bus->ioeventfd_grabbed == 0 && bus->ioeventfd_started) {
> + virtio_bus_stop_ioeventfd(bus);
> + /* Remember that we need to restart ioeventfd
> + * when ioeventfd_grabbed becomes zero.
> + */
> + bus->ioeventfd_started = true;
> + }
> + bus->ioeventfd_grabbed++;
> + return 0;
> +}
> +
> +void virtio_bus_release_ioeventfd(VirtioBusState *bus)
> +{
> + assert(bus->ioeventfd_grabbed != 0);
> + if (--bus->ioeventfd_grabbed == 0 && bus->ioeventfd_started) {
> + /* Force virtio_bus_start_ioeventfd to act. */
> + bus->ioeventfd_started = false;
> + virtio_bus_start_ioeventfd(bus);
> + }
> +}
> +
> int virtio_bus_start_ioeventfd(VirtioBusState *bus)
> {
> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
> @@ -161,10 +193,12 @@ int virtio_bus_start_ioeventfd(VirtioBusState *bus)
> if (bus->ioeventfd_started) {
> return 0;
> }
> - r = vdc->start_ioeventfd(vdev);
> - if (r < 0) {
> - error_report("%s: failed. Fallback to userspace (slower).",
> __func__);
> - return r;
/* Only set our notifier if we have ownership. */
> + if (!bus->ioeventfd_grabbed) {
> + r = vdc->start_ioeventfd(vdev);
> + if (r < 0) {
> + error_report("%s: failed. Fallback to userspace (slower).",
> __func__);
> + return r;
> + }
> }
> bus->ioeventfd_started = true;
> return 0;
> @@ -179,9 +213,11 @@ void virtio_bus_stop_ioeventfd(VirtioBusState *bus)
> return;
> }
>
> - vdev = virtio_bus_get_device(bus);
> - vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
> - vdc->stop_ioeventfd(vdev);
/* Only remove our notifiers if we have ownership. */
> + if (!bus->ioeventfd_grabbed) {
> + vdev = virtio_bus_get_device(bus);
> + vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
> + vdc->stop_ioeventfd(vdev);
> + }
> bus->ioeventfd_started = false;
> }
>
(...)
> diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
> index fdf7fda..8a51e2c 100644
> --- a/include/hw/virtio/virtio-bus.h
> +++ b/include/hw/virtio/virtio-bus.h
> @@ -97,6 +97,16 @@ struct VirtioBusState {
> * Set if ioeventfd has been started.
* This is independent of who has ioeventfd ownership (core or vhost).
> */
> bool ioeventfd_started;
> +
> + /*
> + * Set if ioeventfd has been grabbed by vhost. When ioeventfd
> + * is grabbed by vhost, we track its started/stopped state (which
> + * depends in turn on the virtio status register), but do not
> + * register a handler for the ioeventfd. When ioeventfd is
> + * released, if ioeventfd_started is true we finally register
> + * the handler so that QEMU's device model can use ioeventfd.
> + */
> + int ioeventfd_grabbed;
> };
>
> void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp);
