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

[Ladi Prosek]

On Thu, May 5, 2016 at 5:20 PM, Michael S. Tsirkin <address@hidden> wrote:
> On Thu, May 05, 2016 at 05:15:01PM +0200, Ladi Prosek wrote:
>> On Thu, May 5, 2016 at 5:02 PM, Michael S. Tsirkin <address@hidden> wrote:
>> > 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.
>>
>> Should the below paragraph be worded differently then? Or explicitly
>> say that the contents of the status word is undefined?
>
> It says don't access. How would you word a driver requirement otherwise?

I would use a stronger mode than "SHOULD NOT". Playing devil's
advocate, I could interpret the sentence as a suggested perf
optimization - i.e. the interrupt message conveys the required
information, no need to consult ISR status.

>> Specified since
>> 1st version vs. actually behaving since 1st version.
>
> Yea. Yack. People just try whatever seems to work.
>
>> If you think it's
>> safe to remove the |= 0x01 from the regular path I'll fix the drivers
>> I know of, that's not a problem.
>
> Maybe we should keep the  |= 0x01 in QEMU for now, but let's
> not mess up dataplace.  Please fix the drivers.
>
> Maybe remove |= 0x01 if legacy is disabled.

Ok, sounds good. Thanks!

>> 4.1.4.5.2 Driver Requirements: ISR status capability
>>
>> If MSI-X capability is enabled, the driver SHOULD NOT access ISR
>> status upon detecting a Queue Interrupt.
>>
>> >> >
>> >> >> ---
>> >> >>  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