Re: [PATCH] Search the LAPIC's for one that will accept a PIC interrupt.

[Gleb Natapov <gleb@redhat.com>]

On Tue, Jun 22, 2010 at 02:34:29PM +0300, Avi Kivity wrote:
> On 06/22/2010 11:10 AM, Gleb Natapov wrote:
> >On Mon, Jun 21, 2010 at 11:29:40AM -0400, Chris Lalancette wrote:
> >>Older versions of 32-bit linux have a "Checking 'hlt' instruction"
> >>test where they repeatedly call the 'hlt' instruction, and then
> >>expect a timer interrupt to kick the CPU out of halt.  This happens
> >>before any LAPIC or IOAPIC setup happens, which means that all of
> >>the APIC's are in virtual wire mode at this point.  Unfortunately,
> >>the current implementation of virtual wire mode is hardcoded to
> >>only kick the BSP, so if a crash+kexec occurs on a different
> >>vcpu, it will never get kicked.
> >>
> >>This patch makes pic_unlock() do the equivalent of
> >>kvm_irq_delivery_to_apic() for the IOAPIC code.  That is, it runs
> >>through all of the vcpus looking for one that is in virtual wire
> >>mode.  In the normal case where LAPICs and IOAPICs are configured,
> >>this won't be used at all.  In the bootstrap phase of a modern
> >>OS, before the LAPICs and IOAPICs are configured, this will have
> >>exactly the same behavior as today; VCPU0 is always looked at
> >>first, so it will always get out of the loop after the first
> >>iteration.  This will only go through the loop more than once
> >>during a kexec/kdump, in which case it will only do it a few times
> >>until the kexec'ed kernel programs the LAPIC and IOAPIC.
> >>
> >>Signed-off-by: Chris Lalancette<clalance@redhat.com>
> >>---
> >>  arch/x86/kvm/i8259.c |   17 +++++++++++++----
> >>  1 files changed, 13 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> >>index 2c73f44..85ecabc 100644
> >>--- a/arch/x86/kvm/i8259.c
> >>+++ b/arch/x86/kvm/i8259.c
> >>@@ -44,16 +44,25 @@ static void pic_unlock(struct kvm_pic *s)
> >>  	__releases(&s->lock)
> >>  {
> >>  	bool wakeup = s->wakeup_needed;
> >>-	struct kvm_vcpu *vcpu;
> >>+	struct kvm_vcpu *vcpu, *found = NULL;
> >>+	int i;
> >>
> >>  	s->wakeup_needed = false;
> >>
> >>  	raw_spin_unlock(&s->lock);
> >>
> >>  	if (wakeup) {
> >>-		vcpu = s->kvm->bsp_vcpu;
> >>-		if (vcpu)
> >>-			kvm_vcpu_kick(vcpu);
> >>+		kvm_for_each_vcpu(i, vcpu, s->kvm) {
> >>+			if (kvm_apic_accept_pic_intr(vcpu)) {
> >>+				found = vcpu;
> >>+				break;
> >>+			}
> >>+		}
> >Shouldn't we kick all vcpus that are in virtual write mode, not just
> >first one found?
> 
> If two lapics are in ExtInt mode, both will perform the IntAck cycle
> and the PIC might get confused.  I don't think it's a valid
> configuration.  So I think the patch is fine.
> 
May be, interesting what would happen on real HW. How kdump kernel knows
that other cpu's lapics configured correctly?

> There's a slight issue in that if an interrupt happens while a vcpu
> is turning off LVT0.ExtInt, the interrupt gets lost.  But this is
> better than what we have now.
> 
We can check pic output after LVT0.ExtInt is configured.

> btw, I think virtual wire refers to:
> 
>   pic -> ioapic(ExtInt) -> (apic bus) -> lapic
> 
> (virtual wire since the interrupt is passed over the apic bus, not a
> real wire)
> 
> while our configuration is
> 
>   pic -> lint0 -> lapic lvt0 (ExtInt)
> 

I saw both referred as virtual wire, may be erroneous.

How is the mode where lapic is disabled and pic interrupts are delivered
directly to cpu is called?

--
			Gleb.