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

[Michael S. Tsirkin]

On Tue, Nov 15, 2016 at 10:48:15AM -0700, Alex Williamson wrote:
> On Tue, 15 Nov 2016 19:38:30 +0200
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
> > On Tue, Nov 15, 2016 at 05:22:49PM +0100, Paolo Bonzini wrote:
> > > 
> > > 
> > > On 15/11/2016 16:44, Michael S. Tsirkin wrote:  
> > > > True. We could drop it from non-data plane, it's just that we never had
> > > > a reason to. vhost in kernel does not set ISR in MSI mode, either.  
> > > 
> > > Yeah, I suspected that.  But dropping it from non-dataplane would break
> > > Windows hibernation and crashdump, just like it did for Alex.  
> > 
> > I guess it's just a question of updating the drivers,
> > isn't it? To me, hibernation/crashdump doesn't sound important
> > enough to warrant work-arounds, but if you feel otherwise,
> > I'm fine with doing this work-around for dataplane.
> 
> The fact that Windows is trying to do some sort of hibernation is not
> visible to the user, I'm simply trying to shutdown the VM.  That's
> rather important on my scale of functionality.  If we have drivers in
> the wild doing this, does it matter what's in the spec?

It matters that latest drivers are already OK. "Update drivers"
has been the advice for any kind of windows problem for years.

> Smells like a
> regression from an end user perspective. Thanks,
> 
> Alex

This exposes a driver bug, yes. The right fix is easy to point
out, whether we want a work-around I'm not sure - I understand that
you feel strongly that we do, is that right? OK, just let's document
what's going on and which versions are affected.


> > > > 
> > > > The device MUST set the Device Configuration Interrupt bit in ISR status
> > > > before sending a device configu-
> > > > ration change notification to the driver.
> > > > If MSI-X capability is disabled, the device MUST set the Queue Interrupt
> > > > bit in ISR status before sending a
> > > > virtqueue notification to the driver.
> > > > If MSI-X capability is disabled, the device MUST set the Interrupt
> > > > Status bit in the PCI Status register in the
> > > > PCI Configuration Header of the device to the logical OR of all bits in
> > > > ISR status of the device. The device
> > > > then asserts/deasserts INT#x interrupts unless masked according to
> > > > standard PCI rules [PCI].
> > > > The device MUST reset ISR status to 0 on driver read.
> > > > 
> > > > 
> > > > 
> > > > 
> > > > If MSI-X capability is enabled, the driver SHOULD NOT access ISR status
> > > > upon detecting a Queue Interrupt.
> > > > 
> > > > 
> > > > 
> > > > It can be clearer, but IMHO it's reasonably clear that devices
> > > > do not have to set this bit in MSI mode.  
> > > 
> > > Yes, it is.  We can just document it in the release notes, but then the
> > > fix is not particularly intrusive.
> > > 
> > > Paolo  
> > 
> > It has a slight performance cost but it's not too bad.
> > 
> > I'd rather document it in a code comment though.
> > Explain the motivation and which driver versions are affected.
> >