On Thu, May 5, 2016 at 6:04 PM, Michael S. Tsirkin <m...@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 <m...@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 <m...@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 <m...@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 <lpro...@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