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

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

[Ladi Prosek]

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 <lprosek@redhat.com>
---
 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

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 <lprosek@redhat.com>

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

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

[Ladi Prosek]

On Thu, May 5, 2016 at 3:36 PM, Michael S. Tsirkin <mst@redhat.com> 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 <lprosek@redhat.com>
>
> 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.

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

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 <mst@redhat.com> 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 <lprosek@redhat.com>
> >
> > 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

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

[Ladi Prosek]

On Thu, May 5, 2016 at 5:02 PM, Michael S. Tsirkin <mst@redhat.com> 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 <mst@redhat.com> 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 <lprosek@redhat.com>
>> >
>> > 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? Specified since
1st version vs. actually behaving since 1st version. 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.

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

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

[Michael S. Tsirkin]

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 <mst@redhat.com> 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 <mst@redhat.com> 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 <lprosek@redhat.com>
> >> >
> >> > 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?

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

> 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

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 <mst@redhat.com> 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 <mst@redhat.com> 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 <mst@redhat.com> 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 <lprosek@redhat.com>
>> >> >
>> >> > 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

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

[Michael S. Tsirkin]

On Thu, May 05, 2016 at 05:27:03PM +0200, Ladi Prosek wrote:
> On Thu, May 5, 2016 at 5:20 PM, Michael S. Tsirkin <mst@redhat.com> 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 <mst@redhat.com> 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 <mst@redhat.com> 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 <lprosek@redhat.com>
> >> >> >
> >> >> > 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.

OK, maybe it should be "MUST NOT".
Older virtio said it is not valid.
Want to reports this on virtio comments mailing list?


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

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

[Ladi Prosek]

On Thu, May 5, 2016 at 6:04 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, May 05, 2016 at 05:27:03PM +0200, Ladi Prosek wrote:
>> On Thu, May 5, 2016 at 5:20 PM, Michael S. Tsirkin <mst@redhat.com> 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 <mst@redhat.com> 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 <mst@redhat.com> 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 <lprosek@redhat.com>
>> >> >> >
>> >> >> > 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.
>
> OK, maybe it should be "MUST NOT".
> Older virtio said it is not valid.
> Want to reports this on virtio comments mailing list?

Sure, will do.

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