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

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

[Chris Lalancette]

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;
+			}
+		}
+
+		if (!found)
+			found = s->kvm->bsp_vcpu;
+
+		kvm_vcpu_kick(found);
 	}
 }
 
-- 
1.6.6.1

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

[Gleb Natapov]

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 (!found)
> +			found = s->kvm->bsp_vcpu;
> +
> +		kvm_vcpu_kick(found);
>  	}
>  }
>  
> -- 
> 1.6.6.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

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

[Avi Kivity]

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

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

[Gleb Natapov]

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.

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

[Avi Kivity]

On 06/22/2010 02:49 PM, Gleb Natapov wrote:
>
>
>> 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.
>
>    

Right.

>> 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?
>    

I'm just guessing based on the name, no idea really.

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

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

[Avi Kivity]

On 06/21/2010 06:29 PM, 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.
>    

Applied, thanks.

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

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

[Avi Kivity]

On 06/23/2010 11:56 PM, Chris Lalancette wrote:
>
> Did you forget to push out a tree with this patch applied, or are you doing
> some testing before doing so?  I don't see this patch in the current kvm
> tree (git head is a63e16c655f9e68d49d6fae4275ffda16b1888b2).
>    

No and yes.  Under testing patches are queued in the 'next' branch; 
after testing they are merged into 'master'.

Use 'git fetch' instead of 'git pull' to see all branches.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.