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

[Avi Kivity <avi@redhat.com>]

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.

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.

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)

-- 
error compiling committee.c: too many arguments to function