Re: [Qemu-devel] [PATCH 1/3] virtio: introduce grab/release_ioeventfd to fix vhost

[Alexey Kardashevskiy]

On 17/11/16 05:05, Paolo Bonzini 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 <address@hidden>
> Reported-by: Christian Borntraeger <address@hidden>
> Reported-by: Alex Williamson <address@hidden>
> Fixes: ed08a2a0b ("virtio: use virtio_bus_set_host_notifier to start/stop 
> ioeventfd")
> Signed-off-by: Paolo Bonzini <address@hidden>
> Message-Id: <address@hidden>
> Signed-off-by: Paolo Bonzini <address@hidden>


As mentioned in another thread, this fixed vhost on ppc64/pseries.


Tested-by: Alexey Kardashevskiy <address@hidden>


> ---
>         v1->v2: more comments [Cornelia]
> 
>  hw/virtio/vhost.c              | 14 +++++-----
>  hw/virtio/virtio-bus.c         | 58 
> ++++++++++++++++++++++++++++++++++--------
>  hw/virtio/virtio.c             | 16 ++++++++++++
>  include/hw/virtio/virtio-bus.h | 14 ++++++++++
>  include/hw/virtio/virtio.h     |  2 ++
>  5 files changed, 86 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 30aee88..f7f7023 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1214,17 +1214,17 @@ 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) {
> +    /* We will pass the notifiers to the kernel, make sure that QEMU
> +     * 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);
> @@ -1244,7 +1244,7 @@ fail_vq:
>          }
>          assert (e >= 0);
>      }
> -    virtio_device_start_ioeventfd(vdev);
> +    virtio_device_release_ioeventfd(vdev);
>  fail:
>      return r;
>  }
> @@ -1267,7 +1267,7 @@ void vhost_dev_disable_notifiers(struct vhost_dev 
> *hdev, VirtIODevice *vdev)
>          }
>          assert (r >= 0);
>      }
> -    virtio_device_start_ioeventfd(vdev);
> +    virtio_device_release_ioeventfd(vdev);
>  }
>  
>  /* Test and clear event pending status.
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index bf61f66..d6c0c72 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -147,6 +147,39 @@ 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 +194,14 @@ 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 +216,12 @@ 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 notifier 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;
>  }
>  
> @@ -211,7 +251,6 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int 
> n, bool assign)
>      }
>  
>      if (assign) {
> -        assert(!bus->ioeventfd_started);
>          r = event_notifier_init(notifier, 1);
>          if (r < 0) {
>              error_report("%s: unable to init event notifier: %s (%d)",
> @@ -225,9 +264,6 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int 
> n, bool assign)
>          }
>          return 0;
>      } else {
> -        if (!bus->ioeventfd_started) {
> -            return 0;
> -        }
>          k->ioeventfd_assign(proxy, notifier, n, false);
>      }
>  
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 55a00cd..b7d5828 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2191,6 +2191,22 @@ void virtio_device_stop_ioeventfd(VirtIODevice *vdev)
>      virtio_bus_stop_ioeventfd(vbus);
>  }
>  
> +int virtio_device_grab_ioeventfd(VirtIODevice *vdev)
> +{
> +    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
> +    VirtioBusState *vbus = VIRTIO_BUS(qbus);
> +
> +    return virtio_bus_grab_ioeventfd(vbus);
> +}
> +
> +void virtio_device_release_ioeventfd(VirtIODevice *vdev)
> +{
> +    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
> +    VirtioBusState *vbus = VIRTIO_BUS(qbus);
> +
> +    virtio_bus_release_ioeventfd(vbus);
> +}
> +
>  static void virtio_device_class_init(ObjectClass *klass, void *data)
>  {
>      /* Set the default value here. */
> 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.
>       */
>      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);
> @@ -131,6 +141,10 @@ bool virtio_bus_ioeventfd_enabled(VirtioBusState *bus);
>  int virtio_bus_start_ioeventfd(VirtioBusState *bus);
>  /* Stop the ioeventfd. */
>  void virtio_bus_stop_ioeventfd(VirtioBusState *bus);
> +/* Tell the bus that vhost is grabbing the ioeventfd. */
> +int virtio_bus_grab_ioeventfd(VirtioBusState *bus);
> +/* bus that vhost is not using the ioeventfd anymore. */
> +void virtio_bus_release_ioeventfd(VirtioBusState *bus);
>  /* Switch from/to the generic ioeventfd handler */
>  int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign);
>  
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 5951997..835b085 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -272,6 +272,8 @@ void virtio_queue_set_guest_notifier_fd_handler(VirtQueue 
> *vq, bool assign,
>                                                  bool with_irqfd);
>  int virtio_device_start_ioeventfd(VirtIODevice *vdev);
>  void virtio_device_stop_ioeventfd(VirtIODevice *vdev);
> +int virtio_device_grab_ioeventfd(VirtIODevice *vdev);
> +void virtio_device_release_ioeventfd(VirtIODevice *vdev);
>  bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev);
>  EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq);
>  void virtio_queue_host_notifier_read(EventNotifier *n);
> 


-- 
Alexey