Re: [PATCH] KVM: VMX: Fix race between pending IRQ and NMI

[Jan Kiszka <jan.kiszka@siemens.com>]

Avi Kivity wrote:
> Avi Kivity wrote:
>> Jan Kiszka wrote:
>>> Jiajun kindly provided me a RHEL kernel and initrd (2.6.18-53-el5) which
>>> I ran for a while (or booted a few times) to trigger the hang. Basically
>>> you need high IRQ load (preferably via LAPIC, to exploit that un-acked
>>> IRQs will block low-prio IRQs as well) + high NMI load (e.g. via NMI
>>> watchdog).
>>>   
>>
>> I was able to reproduce it easily by zapping the mmu every second.
>>
>> Attached is a patch the fixes it for me.  Basically it avoids the nmi
>> path if an interrupt is being injected.  This is closer to my event
>> queue plan, and also is similar to what the code does today with
>> exceptions (avoid ->inject_pending_irq() if an exception is pending).
>>
> 
> Oh, and I think this is more correct than the previous approach of
> letting the nmi preempt the interrupt.
> 
> The nmi handler could change the tpr to mask the preempted interrupt;
> but the code would not notice that.  Once the interrupt was injected the
> guest would see an interrupt at a higher priority than it has programmed
> the hardware to allow.

I consider this a bit far fetch. What sane NMI handler would fiddle with
the APIC? It would be fairly tricky to properly synchronize this with
the rest of the OS.

> 
> Basically, once we commit to an interrupt via kvm_cpu_get_interrupt(),
> we must inject it before the any instruction gets executed.
> 
> I don't think any real guest would notice, though.
> 

Well, I have no problems with your approach (when also applied on the
user space irqchip path) of keeping the order *if* we can ensure that
only the first instruction of the IRQ handler is executed and we will
then inject the NMI. Otherwise this opens a prio inversion between IRQs
and NMIs. The point is that, unless I'm overseeing some detail right
now, your approach will inject the pending NMI only once the guest
/happens/ to exit the VM, right? If yes, then it's a no-go IMHO, also
for keeping this property with the queue approach.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2 ES-OS
Corporate Competence Center Embedded Linux