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

[Paolo Bonzini]

----- Original Message -----
> From: "Michael S. Tsirkin" <address@hidden>
> To: "Paolo Bonzini" <address@hidden>
> Cc: address@hidden, "alex williamson" <address@hidden>, address@hidden, 
> address@hidden
> Sent: Wednesday, November 16, 2016 9:39:24 PM
> Subject: Re: [PATCH 3/3] virtio: set ISR on dataplane notifications
> 
> On Wed, Nov 16, 2016 at 03:38:11PM -0500, Paolo Bonzini wrote:
> > > > +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
> >          ^^^^^^^^^^^^^^^^
> > 
> > Not sure it's only for Windows 8.1, in fact probably not.
> 
> 8.1 on shutdown and others on crashdump or hibernation?

Even 8.1 is really a hibernation hidden behind a "Shut down" menu item.

Paolo

> > Looks good if you replace this line with
> > 
> > "are incorrectly polling this bit during crashdump or hibernation"
> > 
> > Paolo
> > 
> > >  * 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.
> > >  */
> > 
> > 
> > > 
> > > 
> > > 
> > > > +    event_notifier_set(&vq->guest_notifier);
> > > > +}
> > > > +
> > > >  void virtio_notify(VirtIODevice *vdev, VirtQueue *vq)
> > > >  {
> > > >      if (!virtio_should_notify(vdev, vq)) {
> > > > @@ -1990,7 +1994,7 @@ static void
> > > > virtio_queue_guest_notifier_read(EventNotifier *n)
> > > >  {
> > > >      VirtQueue *vq = container_of(n, VirtQueue, guest_notifier);
> > > >      if (event_notifier_test_and_clear(n)) {
> > > > -        virtio_irq(vq);
> > > > +        virtio_notify_vector(vq->vdev, vq->vector);
> > > >      }
> > > >  }
> > > >  
> > > > diff --git a/include/hw/virtio/virtio-scsi.h
> > > > b/include/hw/virtio/virtio-scsi.h
> > > > index 9fbc7d7..7375196 100644
> > > > --- a/include/hw/virtio/virtio-scsi.h
> > > > +++ b/include/hw/virtio/virtio-scsi.h
> > > > @@ -137,6 +137,5 @@ void virtio_scsi_push_event(VirtIOSCSI *s,
> > > > SCSIDevice
> > > > *dev,
> > > >  void virtio_scsi_dataplane_setup(VirtIOSCSI *s, Error **errp);
> > > >  int virtio_scsi_dataplane_start(VirtIODevice *s);
> > > >  void virtio_scsi_dataplane_stop(VirtIODevice *s);
> > > > -void virtio_scsi_dataplane_notify(VirtIODevice *vdev, VirtIOSCSIReq
> > > > *req);
> > > >  
> > > >  #endif /* QEMU_VIRTIO_SCSI_H */
> > > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > > > index 835b085..ab0e030 100644
> > > > --- a/include/hw/virtio/virtio.h
> > > > +++ b/include/hw/virtio/virtio.h
> > > > @@ -181,6 +181,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq,
> > > > unsigned
> > > > int *in_bytes,
> > > >                                 unsigned max_in_bytes, unsigned
> > > >                                 max_out_bytes);
> > > >  
> > > >  bool virtio_should_notify(VirtIODevice *vdev, VirtQueue *vq);
> > > > +void virtio_notify_irqfd(VirtIODevice *vdev, VirtQueue *vq);
> > > >  void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
> > > >  
> > > >  void virtio_save(VirtIODevice *vdev, QEMUFile *f);
> > > > @@ -280,7 +281,6 @@ void virtio_queue_host_notifier_read(EventNotifier
> > > > *n);
> > > >  void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq,
> > > >  AioContext
> > > >  *ctx,
> > > >                                                  void
> > > >                                                  (*fn)(VirtIODevice *,
> > > >                                                             VirtQueue
> > > >                                                             *));
> > > > -void virtio_irq(VirtQueue *vq);
> > > >  VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t
> > > >  vector);
> > > >  VirtQueue *virtio_vector_next_queue(VirtQueue *vq);
> > > >  
> > > > --
> > > > 2.9.3
> > > 
>