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

[Michael S. Tsirkin]

On Tue, Nov 15, 2016 at 04:28:37PM +0100, Paolo Bonzini wrote:
> 
> 
> On 15/11/2016 16:26, Michael S. Tsirkin wrote:
> > On Tue, Nov 15, 2016 at 02:46:29PM +0100, Paolo Bonzini wrote:
> >> Dataplane has been omitting forever the step of setting ISR when an
> >> interrupt is raised.  This causes surprisingly little breakage,
> >> because most polling-mode drivers look at the used ring's index field
> >> rather than the ISR register.
> >>
> >> Some versions of the Windows drivers are an exception---and they use
> >> polling mode with ISR for crashdump and hibernation.  And because
> >> recent releases of Windows do not really shut down, but rather log
> >> out and hibernate to make the next startup faster, this manifested
> >> as a hang during shutdown with e.g. Windows 8.1 and virtio-win 1.8.0
> >> RPMs.  Newer versions probably poll the used index; older versions
> >> do not use MSI and therefore go through the emulated irqfd path
> >> (virtio_queue_guest_notifier_read), which handled ISR correctly.
> > 
> > Confused. virtio spec says ISR shouldn't be set on
> > ring activity in MSI mode. Is this a driver bug?
> 
> Huh, then probably it is, and this is why it works with newer versions
> of the driver.  They probably forgot to disable MSI mode when entering
> hibernation mode.
> 
> But in the end QEMU is doing different things in non-dataplane vs.
> dataplane mode, at least for virtio-blk and virtio-scsi (I'm sure
> virtio-net vs. vhost would have the same issue, just no driver that is
> affected).  Is it SHOULD NOT or MUST NOT or MAY NOT?
> 
> Paolo

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.


What we have in spec is:

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.


> > 
> >> The failure has always been there for virtio dataplane, but it
> >> became visible after commits 9ffe337 ("virtio-blk: always use
> >> dataplane path if ioeventfd is active", 2016-10-30) and
> >> ad07cd6 ("virtio-scsi: always use dataplane path if ioeventfd
> >> is active", 2016-10-30), which removed the non-dataplane ioeventfd
> >> path for virtio-blk and virtio-scsi.  The good news therefore
> >> is that it was not a bug in the patches---they did exactly what they
> >> were meant for, i.e. shake out remaining dataplane bugs.
> >>
> >> The fix is not hard.  The virtio_should_notify+event_notifier_set
> >> pair that is common to virtio-blk and virtio-scsi dataplane
> >> is replaced with a new public function virtio_notify_irqfd
> >> that also sets ISR.  The irqfd emulation code now need not
> >> set ISR anymore, so virtio_irq is removed.
> >>
> >> Signed-off-by: Paolo Bonzini <address@hidden>