Re: [PATCH 5/5] Fix kdump under KVM.

[Chris Lalancette <clalance@redhat.com>]

Avi Kivity wrote:
> On 10/30/2009 02:23 PM, Chris Lalancette wrote:
>> In the meantime, I've gotten the "set_irq from IRQ context" that Avi 
>> suggested
>> working, and the fixing up of this IOAPIC check is the last bit to actually get
>> kdump working.
>>    
> 
> There are two problems with that:
> 
> - kvm_set_irq() calls some notifiers (irq_mask_notifiers, at least, but 
> possibly more); need to make sure those are safe from irq context
> - kvm_set_irq() will loop on all vcpus, usually incurring a cache miss 
> or two; with large vcpu counts this can make the irq-off time quite 
> large, hurting our worst-case latency
> 
> we can defer the second problem since it's only a performance issue, but 
> how did you deal with the first?
> 

Actually, there is a problem here, but I don't think it's as you describe.
kvm_set_irq() doesn't directly call the mask notifiers; all it really does is
call the i8259/ioapic callback, which causes an interrupt to be raised on the
LAPIC in question, which causes the interrupt to be injected into the VCPU on
the next run.  That doesn't call the irq_mask_notifiers, and seems to be
IRQ-safe (with the conversion from mutex's to spin_locks).

The irq mask notifiers are called later on during an EOI.  In particular,
ioapic_mmio_write() -> ioapic_write_indirect(), which is protected by the
ioapic_lock.  I don't see a problem here (although please point it out if I
missed it).

The problem I do see has to do with the irq_ack notifiers.  In particular, if
you are on ia64 and ioapic_mmio_write() gets called for the IOAPIC_REG_EOI
address, we call __kvm_ioapic_update_eoi().  In that case, we drop the spin_lock
and call kvm_notify_acked_irq(), which can re-enter the ioapic code.  This could
be IRQ unsafe, after we return from kvm_notify_acked_irq() we re-acquire the
spin-lock, but in an irq-unsafe fashion.  Therefore we could take the spin-lock,
get interrupted for the timer interrupt, and try to re-acquire the lock, causing
a deadlock.  I'll have to think a bit more on how to solve that one (there's a
similar situation going on in the i8259, which is not ia64 specific).

So, either I'm missing exactly what you are talking about, or there's no
deadlock with my patch (except with ia64).  Can you explain further?

-- 
Chris Lalancette