Re: [Qemu-devel] [PATCH 3/3] virtio: set ISR on dataplane notifications

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1440 bytes --]

On Wed, Nov 16, 2016 at 10:15:25PM +0200, Michael S. Tsirkin wrote:
> On Wed, Nov 16, 2016 at 07:05:51PM +0100, Paolo Bonzini wrote:
> > @@ -1356,6 +1349,17 @@ bool virtio_should_notify(VirtIODevice *vdev, VirtQueue *vq)
> >      return !v || vring_need_event(vring_get_used_event(vq), new, old);
> >  }
> >  
> > +void virtio_notify_irqfd(VirtIODevice *vdev, VirtQueue *vq)
> > +{
> > +    if (!virtio_should_notify(vdev, vq)) {
> > +        return;
> > +    }
> > +
> > +    trace_virtio_notify_irqfd(vdev, vq);
> > +    virtio_set_isr(vq->vdev, 0x1);
> 
> So here, I think we need a comment with parts of
> the commit log.
> 
> /*
>  * virtio spec 1.0 says ISR bit 0 should be ignored with MSI, but
>  * windows drivers included in virtio-win 1.8.0 (circa 2015)
>  * for Windows 8.1 only are incorrectly polling this bit during shutdown
>  * in MSI mode, causing a hang if this bit is never updated.
>  * Next driver release from 2016 fixed this problem, so working around it
>  * is not a must, but it's easy to do so let's do it here.
>  *
>  * Note: it's safe to update ISR from any thread as it was switched
>  * to an atomic operation.
>  */

I agree.  The commit description is nice but the information needs to be
in the code.

I remember explicitly omitting the ISR update when writing the dataplane
code because I saw the spec does not require it for MSI.

A comment is necessary.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]