From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50226) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ayL56-0002y7-N7 for qemu-devel@nongnu.org; Thu, 05 May 2016 11:20:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ayL4t-0004b6-PE for qemu-devel@nongnu.org; Thu, 05 May 2016 11:20:43 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57405) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ayL4t-0004Sv-IU for qemu-devel@nongnu.org; Thu, 05 May 2016 11:20:35 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5094D46212 for ; Thu, 5 May 2016 15:20:24 +0000 (UTC) Date: Thu, 5 May 2016 18:20:21 +0300 From: "Michael S. Tsirkin" Message-ID: <20160505181743-mutt-send-email-mst@redhat.com> References: <1462439617-11387-1-git-send-email-lprosek@redhat.com> <20160505163432-mutt-send-email-mst@redhat.com> <20160505175957-mutt-send-email-mst@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [RFC PATCH] virtio: set ISR status when delivering queue MSI List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ladi Prosek Cc: qemu-devel@nongnu.org, Vadim Rozenfeld 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 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 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 > >> > > >> > 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