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