Re: [Qemu-devel] [RFC PATCH] virtio: set ISR status when delivering queue MSI

[Michael S. Tsirkin]

On Thu, May 05, 2016 at 11:13:37AM +0200, Ladi Prosek wrote:
> There is a discrepancy between dataplane and no-dataplane virtio
> behavior with respect to the ISR status register and MSI-X
> capability.
> 
> Without dataplane the Queue interrupt ISR status bit is set
> regardless of how the notification is delivered to the guest.
> 
> With dataplane the Queue interrupt ISR status bit is set only
> if the notification is delivered as an IRQ.
> 
> While both conform to the spec, QEMU should be consistent to
> minimize surprises with broken drivers.
> 
> This RFC patch shows the relevant code location and contains a
> possible fix which makes QEMU set the bit on the MSI dataplane
> code path.
> 
> Signed-off-by: Ladi Prosek <address@hidden>

How is this supposed to work when injecting interrupts using irqfd?
If anything, I would rather drop setting the flag on non-dataplane.


> ---
>  hw/virtio/virtio.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 08275a9..4053313 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1192,7 +1192,7 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq)
>      }
>  
>      trace_virtio_notify(vdev, vq);
> -    vdev->isr |= 0x01;
> +    vdev->isr |= 0x01; // <= here we set ISR status even if we don't raise 
> IRQ
>      virtio_notify_vector(vdev, vq->vector);
>  }
>  
> @@ -1757,16 +1757,25 @@ static void 
> virtio_queue_guest_notifier_read(EventNotifier *n)
>      }
>  }
>  
> +static void virtio_queue_guest_notifier_read_irqfd(EventNotifier *n)
> +{
> +    VirtQueue *vq = container_of(n, VirtQueue, guest_notifier);
> +    if (event_notifier_test_and_clear(n)) {
> +        vq->vdev->isr |= 0x01;
> +    }
> +}
> +
>  void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign,
>                                                  bool with_irqfd)
>  {
> -    if (assign && !with_irqfd) {
> +    if (assign) {
>          event_notifier_set_handler(&vq->guest_notifier,
> -                                   virtio_queue_guest_notifier_read);
> +             with_irqfd ?
> +                virtio_queue_guest_notifier_read_irqfd :
> +                virtio_queue_guest_notifier_read);
>      } else {
>          event_notifier_set_handler(&vq->guest_notifier, NULL);
> -    }
> -    if (!assign) {
> +
>          /* Test and clear notifier before closing it,
>           * in case poll callback didn't have time to run. */
>          virtio_queue_guest_notifier_read(&vq->guest_notifier);
> -- 
> 2.5.5