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

[Michael S. Tsirkin]

On Thu, May 05, 2016 at 04:59:13PM +0200, Ladi Prosek wrote:
> On Thu, May 5, 2016 at 3:36 PM, Michael S. Tsirkin <address@hidden> wrote:
> > 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.
> 
> My bad, this is messed up. For some reason it works, or appears to be
> working at least.
> 
> The reason why I'd like to add setting the flag in dataplane is that
> if we go the other way we'll break drivers. I know for sure that there
> are drivers depending on this.

Rly? These are way out of spec then. I'd rather have these drivers
fixed: virtio said ISR can't be used with MSI since the 1st version
where MSI was introduced. virtio 1.0 allows using ISR for config
interrupts only.

> >
> >> ---
> >>  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