Re: [Qemu-devel] [PATCH 12/13] virtio: inline virtio_queue_set_host_notifier_fd_handler

[Cornelia Huck]

On Mon, 10 Oct 2016 13:53:40 +0200
Paolo Bonzini <address@hidden> wrote:

> Of the three possible parameter combinations for
> virtio_queue_set_host_notifier_fd_handler:
> 
> - assign=true/set_handler=true is only called from
>   virtio_device_start_ioeventfd
> 
> - assign=false/set_handler=false is called from
>   set_host_notifier_internal but it only does something when
>   reached from virtio_device_stop_ioeventfd_impl; otherwise
>   there is no EventNotifier set on qemu_get_aio_context().
> 
> - assign=true/set_handler=false is called from
>   set_host_notifier_internal, but it is not doing anything:
>   with the new start_ioeventfd and stop_ioeventfd methods,
>   there is never an EventNotifier set on qemu_get_aio_context()
>   at this point.  This is enforced by the assertion in
>   virtio_bus_set_host_notifier.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  hw/virtio/virtio-bus.c     | 19 +++++++++++--------
>  hw/virtio/virtio.c         | 27 +++++++++------------------
>  include/hw/virtio/virtio.h |  3 +--
>  3 files changed, 21 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index d6feaaf..96b7e76 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -163,19 +163,22 @@ static int set_host_notifier_internal(DeviceState 
> *proxy, VirtioBusState *bus,
>                           __func__, strerror(-r), r);
>              return r;
>          }
> -        virtio_queue_set_host_notifier_fd_handler(vq, true, false);
> -        r = k->ioeventfd_assign(proxy, notifier, n, assign);
> +        r = k->ioeventfd_assign(proxy, notifier, n, true);
>          if (r < 0) {
>              error_report("%s: unable to assign ioeventfd: %d", __func__, r);
> -            virtio_queue_set_host_notifier_fd_handler(vq, false, false);
> -            event_notifier_cleanup(notifier);
> -            return r;
> +            goto cleanup_event_notifier;
>          }
> +        return 0;
>      } else {
> -        k->ioeventfd_assign(proxy, notifier, n, assign);
> -        virtio_queue_set_host_notifier_fd_handler(vq, false, false);
> -        event_notifier_cleanup(notifier);
> +        k->ioeventfd_assign(proxy, notifier, n, false);
>      }
> +
> +cleanup_event_notifier:
> +    /* Test and clear notifier before after disabling event,

Now is the time to get rid of "before after" :)

> +     * in case poll callback didn't have time to run.
> +     */
> +    virtio_queue_host_notifier_read(notifier);
> +    event_notifier_cleanup(notifier);
>      return r;
>  }

Reviewed-by: Cornelia Huck <address@hidden>