From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47175) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ayJS6-0001Ab-Ec for qemu-devel@nongnu.org; Thu, 05 May 2016 09:36:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ayJRu-00066l-LT for qemu-devel@nongnu.org; Thu, 05 May 2016 09:36:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46717) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ayJRu-000632-G7 for qemu-devel@nongnu.org; Thu, 05 May 2016 09:36:14 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (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 1F9CC80F72 for ; Thu, 5 May 2016 13:36:03 +0000 (UTC) Date: Thu, 5 May 2016 16:36:00 +0300 From: "Michael S. Tsirkin" Message-ID: <20160505163432-mutt-send-email-mst@redhat.com> References: <1462439617-11387-1-git-send-email-lprosek@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1462439617-11387-1-git-send-email-lprosek@redhat.com> 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, vrozenfe@redhat.com 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. > --- > 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