[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH] virtio: set ISR status when delivering queu
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [RFC PATCH] virtio: set ISR status when delivering queue MSI |
Date: |
Thu, 5 May 2016 19:04:52 +0300 |
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 <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.
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
- [Qemu-devel] [RFC PATCH] virtio: set ISR status when delivering queue MSI, Ladi Prosek, 2016/05/05
- Re: [Qemu-devel] [RFC PATCH] virtio: set ISR status when delivering queue MSI, Michael S. Tsirkin, 2016/05/05
- Re: [Qemu-devel] [RFC PATCH] virtio: set ISR status when delivering queue MSI, Ladi Prosek, 2016/05/05
- Re: [Qemu-devel] [RFC PATCH] virtio: set ISR status when delivering queue MSI, Michael S. Tsirkin, 2016/05/05
- Re: [Qemu-devel] [RFC PATCH] virtio: set ISR status when delivering queue MSI, Ladi Prosek, 2016/05/05
- Re: [Qemu-devel] [RFC PATCH] virtio: set ISR status when delivering queue MSI, Michael S. Tsirkin, 2016/05/05
- Re: [Qemu-devel] [RFC PATCH] virtio: set ISR status when delivering queue MSI, Ladi Prosek, 2016/05/05
- Re: [Qemu-devel] [RFC PATCH] virtio: set ISR status when delivering queue MSI,
Michael S. Tsirkin <=
- Re: [Qemu-devel] [RFC PATCH] virtio: set ISR status when delivering queue MSI, Ladi Prosek, 2016/05/05