Re: [Qemu-devel] [PATCH 1/1] virtio: fail device if set_event_notifier fails

[Cornelia Huck]

On Thu,  2 Mar 2017 19:59:42 +0100
Halil Pasic <address@hidden> wrote:

> The function virtio_notify_irqfd used to ignore the return code of
> event_notifier_set. Let's fail the device should this occur.

I'm wondering if there are reasons for event_notifier_set() to fail
beyond "we've hit an internal race and should make an effort to fix
that one, or else we have completely messed up in qemu". Marking the
device broken tells the guest that there's something wrong with the
device, but I think we want qemu bug reports when there's something
broken with the irqfd.

> 
> Signed-off-by: Halil Pasic <address@hidden>
> 
> ---
> 
> This patch is most likely flawed because virtio_notify_irqfd
> is probably supposed to be thread-safe and neither strerror
> nor virtio_error are that. Anyway lets get the discussion started with this.
> 
> There was a suggestion by Michael to make event_notifier_set void
> and assert inside. After some thinking, I settled at the opinion,
> that neither doing nothing nor crashing the guest is a good idea
> should this failure happen in production. So I came up with this.
> 
> Looking forward to your feedback.
> ---
>  hw/virtio/virtio.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 23483c7..e05f3e5 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1555,6 +1555,8 @@ static bool virtio_should_notify(VirtIODevice *vdev, 
> VirtQueue *vq)
>  void virtio_notify_irqfd(VirtIODevice *vdev, VirtQueue *vq)
>  {
>      bool should_notify;
> +    int rc;
> +
>      rcu_read_lock();
>      should_notify = virtio_should_notify(vdev, vq);
>      rcu_read_unlock();
> @@ -1581,7 +1583,11 @@ void virtio_notify_irqfd(VirtIODevice *vdev, VirtQueue 
> *vq)
>       * to an atomic operation.
>       */
>      virtio_set_isr(vq->vdev, 0x1);
> -    event_notifier_set(&vq->guest_notifier);
> +    rc = event_notifier_set(&vq->guest_notifier);
> +    if (unlikely(rc)) {
> +        virtio_error(vdev, "guest notifier broken for vdev at %p (%s)", vdev,
> +                     strerror(-rc));
> +    }
>  }
> 
>  static void virtio_irq(VirtQueue *vq)