[PATCH] KVM: x86: Don't deliver PIC interrupts to disabled APICs

[PATCH] KVM: x86: Don't deliver PIC interrupts to disabled APICs

[Jan Kiszka]

The locic of kvm_apic_accept_pic_intr has a minor, practically hardly
relevant incorrectness: PIC interrupts are still delivered even if the
APIC of VPU0 (BSP) is disabled. This does not comply with the Virtual
Wire mode according to the Intel MP spec.

To avoid side effects, the BSP APIC is now enabled on reset.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/kvm/lapic.c |   19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

Index: b/arch/x86/kvm/lapic.c
===================================================================
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -933,7 +933,8 @@ void kvm_lapic_reset(struct kvm_vcpu *vc
 	update_divide_count(apic);
 	atomic_set(&apic->timer.pending, 0);
 	if (vcpu->vcpu_id == 0)
-		vcpu->arch.apic_base |= MSR_IA32_APICBASE_BSP;
+		vcpu->arch.apic_base |=
+			MSR_IA32_APICBASE_BSP | MSR_IA32_APICBASE_ENABLE;
 	apic_update_ppr(apic);
 
 	apic_debug(KERN_INFO "%s: vcpu=%p, id=%d, base_msr="
@@ -1089,17 +1090,15 @@ int kvm_apic_has_interrupt(struct kvm_vc
 
 int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu)
 {
+	struct kvm_lapic *apic = vcpu->arch.apic;
 	u32 lvt0 = apic_get_reg(vcpu->arch.apic, APIC_LVT0);
-	int r = 0;
 
-	if (vcpu->vcpu_id == 0) {
-		if (!apic_hw_enabled(vcpu->arch.apic))
-			r = 1;
-		if ((lvt0 & APIC_LVT_MASKED) == 0 &&
-		    GET_APIC_DELIVERY_MODE(lvt0) == APIC_MODE_EXTINT)
-			r = 1;
-	}
-	return r;
+	/* Virtual Wire mode, but we only deliver to the BSP. */
+	if (vcpu->vcpu_id == 0 && apic_hw_enabled(apic)
+	    && !(lvt0 & APIC_LVT_MASKED)
+	    && GET_APIC_DELIVERY_MODE(lvt0) == APIC_MODE_EXTINT)
+		return 1;
+	return 0;
 }
 
 void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu)

Re: [PATCH] KVM: x86: Don't deliver PIC interrupts to disabled APICs

[Sheng Yang]

On Mon, Oct 20, 2008 at 10:50:57AM +0200, Jan Kiszka wrote:
> The locic of kvm_apic_accept_pic_intr has a minor, practically hardly

Typo...

> relevant incorrectness: PIC interrupts are still delivered even if the
> APIC of VPU0 (BSP) is disabled. This does not comply with the Virtual
> Wire mode according to the Intel MP spec.
> 
> To avoid side effects, the BSP APIC is now enabled on reset.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  arch/x86/kvm/lapic.c |   19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> Index: b/arch/x86/kvm/lapic.c
> ===================================================================
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -933,7 +933,8 @@ void kvm_lapic_reset(struct kvm_vcpu *vc
>  	update_divide_count(apic);
>  	atomic_set(&apic->timer.pending, 0);
>  	if (vcpu->vcpu_id == 0)
> -		vcpu->arch.apic_base |= MSR_IA32_APICBASE_BSP;
> +		vcpu->arch.apic_base |=
> +			MSR_IA32_APICBASE_BSP | MSR_IA32_APICBASE_ENABLE;

I think this have already been done in vmx_vcpu_reset()/svm_create_vcpu()?

>  	apic_update_ppr(apic);
>  
>  	apic_debug(KERN_INFO "%s: vcpu=%p, id=%d, base_msr="
> @@ -1089,17 +1090,15 @@ int kvm_apic_has_interrupt(struct kvm_vc
>  
>  int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu)
>  {
> +	struct kvm_lapic *apic = vcpu->arch.apic;
>  	u32 lvt0 = apic_get_reg(vcpu->arch.apic, APIC_LVT0);

Reuse just defined variable "apic"?

> -	int r = 0;
>  
> -	if (vcpu->vcpu_id == 0) {
> -		if (!apic_hw_enabled(vcpu->arch.apic))
> -			r = 1;
> -		if ((lvt0 & APIC_LVT_MASKED) == 0 &&
> -		    GET_APIC_DELIVERY_MODE(lvt0) == APIC_MODE_EXTINT)
> -			r = 1;
> -	}
> -	return r;
> +	/* Virtual Wire mode, but we only deliver to the BSP. */
> +	if (vcpu->vcpu_id == 0 && apic_hw_enabled(apic)

Good point! How about apic_enabled(apic)? Software disable apic should also
can't functional(hope this wouldn't expose some bugs...)

--
regards
Yang, Sheng

> +	    && !(lvt0 & APIC_LVT_MASKED)
> +	    && GET_APIC_DELIVERY_MODE(lvt0) == APIC_MODE_EXTINT)
> +		return 1;
> +	return 0;
>  }
>  
>  void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu)
> --
> 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

Re: [PATCH] KVM: x86: Don't deliver PIC interrupts to disabled APICs

[Jan Kiszka]

Sheng Yang wrote:
> On Mon, Oct 20, 2008 at 10:50:57AM +0200, Jan Kiszka wrote:
>> The locic of kvm_apic_accept_pic_intr has a minor, practically hardly
> 
> Typo...
> 
>> relevant incorrectness: PIC interrupts are still delivered even if the
>> APIC of VPU0 (BSP) is disabled. This does not comply with the Virtual
>> Wire mode according to the Intel MP spec.
>>
>> To avoid side effects, the BSP APIC is now enabled on reset.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  arch/x86/kvm/lapic.c |   19 +++++++++----------
>>  1 file changed, 9 insertions(+), 10 deletions(-)
>>
>> Index: b/arch/x86/kvm/lapic.c
>> ===================================================================
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -933,7 +933,8 @@ void kvm_lapic_reset(struct kvm_vcpu *vc
>>  	update_divide_count(apic);
>>  	atomic_set(&apic->timer.pending, 0);
>>  	if (vcpu->vcpu_id == 0)
>> -		vcpu->arch.apic_base |= MSR_IA32_APICBASE_BSP;
>> +		vcpu->arch.apic_base |=
>> +			MSR_IA32_APICBASE_BSP | MSR_IA32_APICBASE_ENABLE;
> 
> I think this have already been done in vmx_vcpu_reset()/svm_create_vcpu()?

Indeed, will drop that hunk.

> 
>>  	apic_update_ppr(apic);
>>  
>>  	apic_debug(KERN_INFO "%s: vcpu=%p, id=%d, base_msr="
>> @@ -1089,17 +1090,15 @@ int kvm_apic_has_interrupt(struct kvm_vc
>>  
>>  int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu)
>>  {
>> +	struct kvm_lapic *apic = vcpu->arch.apic;
>>  	u32 lvt0 = apic_get_reg(vcpu->arch.apic, APIC_LVT0);
> 
> Reuse just defined variable "apic"?

[ Cleaning my glasses ] Ahh, I see. :)

> 
>> -	int r = 0;
>>  
>> -	if (vcpu->vcpu_id == 0) {
>> -		if (!apic_hw_enabled(vcpu->arch.apic))
>> -			r = 1;
>> -		if ((lvt0 & APIC_LVT_MASKED) == 0 &&
>> -		    GET_APIC_DELIVERY_MODE(lvt0) == APIC_MODE_EXTINT)
>> -			r = 1;
>> -	}
>> -	return r;
>> +	/* Virtual Wire mode, but we only deliver to the BSP. */
>> +	if (vcpu->vcpu_id == 0 && apic_hw_enabled(apic)
> 
> Good point! How about apic_enabled(apic)? Software disable apic should also
> can't functional(hope this wouldn't expose some bugs...)

apic_sw_enable is also covered by the LVT_MASKED check we do on delivery
anyway. That's why I lest it out here. Can spend a comment, though.

> --
> regards
> Yang, Sheng
> 
>> +	    && !(lvt0 & APIC_LVT_MASKED)
>> +	    && GET_APIC_DELIVERY_MODE(lvt0) == APIC_MODE_EXTINT)
>> +		return 1;
>> +	return 0;
>>  }
>>  
>>  void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu)
>> --
>> 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

Jan

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

Re: [PATCH] KVM: x86: Don't deliver PIC interrupts to disabled APICs

[Jan Kiszka]

Jan Kiszka wrote:
> Sheng Yang wrote:
>> On Mon, Oct 20, 2008 at 10:50:57AM +0200, Jan Kiszka wrote:
>>> The locic of kvm_apic_accept_pic_intr has a minor, practically hardly
>> Typo...
>>
>>> relevant incorrectness: PIC interrupts are still delivered even if the
>>> APIC of VPU0 (BSP) is disabled. This does not comply with the Virtual
>>> Wire mode according to the Intel MP spec.
>>>
>>> To avoid side effects, the BSP APIC is now enabled on reset.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>>  arch/x86/kvm/lapic.c |   19 +++++++++----------
>>>  1 file changed, 9 insertions(+), 10 deletions(-)
>>>
>>> Index: b/arch/x86/kvm/lapic.c
>>> ===================================================================
>>> --- a/arch/x86/kvm/lapic.c
>>> +++ b/arch/x86/kvm/lapic.c
>>> @@ -933,7 +933,8 @@ void kvm_lapic_reset(struct kvm_vcpu *vc
>>>  	update_divide_count(apic);
>>>  	atomic_set(&apic->timer.pending, 0);
>>>  	if (vcpu->vcpu_id == 0)
>>> -		vcpu->arch.apic_base |= MSR_IA32_APICBASE_BSP;
>>> +		vcpu->arch.apic_base |=
>>> +			MSR_IA32_APICBASE_BSP | MSR_IA32_APICBASE_ENABLE;
>> I think this have already been done in vmx_vcpu_reset()/svm_create_vcpu()?
> 
> Indeed, will drop that hunk.
> 
>>>  	apic_update_ppr(apic);
>>>  
>>>  	apic_debug(KERN_INFO "%s: vcpu=%p, id=%d, base_msr="
>>> @@ -1089,17 +1090,15 @@ int kvm_apic_has_interrupt(struct kvm_vc
>>>  
>>>  int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu)
>>>  {
>>> +	struct kvm_lapic *apic = vcpu->arch.apic;
>>>  	u32 lvt0 = apic_get_reg(vcpu->arch.apic, APIC_LVT0);
>> Reuse just defined variable "apic"?
> 
> [ Cleaning my glasses ] Ahh, I see. :)
> 
>>> -	int r = 0;
>>>  
>>> -	if (vcpu->vcpu_id == 0) {
>>> -		if (!apic_hw_enabled(vcpu->arch.apic))
>>> -			r = 1;
>>> -		if ((lvt0 & APIC_LVT_MASKED) == 0 &&
>>> -		    GET_APIC_DELIVERY_MODE(lvt0) == APIC_MODE_EXTINT)
>>> -			r = 1;
>>> -	}
>>> -	return r;
>>> +	/* Virtual Wire mode, but we only deliver to the BSP. */
>>> +	if (vcpu->vcpu_id == 0 && apic_hw_enabled(apic)
>> Good point! How about apic_enabled(apic)? Software disable apic should also
>> can't functional(hope this wouldn't expose some bugs...)
> 
> apic_sw_enable is also covered by the LVT_MASKED check we do on delivery
> anyway. That's why I lest it out here. Can spend a comment, though.

Correction: LVT_MASKED is already checked here, one line below.

> 
>> --
>> regards
>> Yang, Sheng
>>
>>> +	    && !(lvt0 & APIC_LVT_MASKED)
>>> +	    && GET_APIC_DELIVERY_MODE(lvt0) == APIC_MODE_EXTINT)
>>> +		return 1;
>>> +	return 0;
>>>  }
>>>  
>>>  void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu)

Jan

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

[PATCH] KVM: x86: Don't deliver PIC interrupts to disabled APICs - v2

[Jan Kiszka]

[ taking Sheng's comments into account ]

The logic of kvm_apic_accept_pic_intr has a minor, practically hardly
relevant incorrectness: PIC interrupts are still delivered even if the
APIC of VPU0 (BSP) is disabled. This does not comply with the Virtual
Wire mode according to the Intel MP spec.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/kvm/lapic.c |   21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

Index: b/arch/x86/kvm/lapic.c
===================================================================
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1089,17 +1089,18 @@ int kvm_apic_has_interrupt(struct kvm_vc
 
 int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu)
 {
-	u32 lvt0 = apic_get_reg(vcpu->arch.apic, APIC_LVT0);
-	int r = 0;
+	struct kvm_lapic *apic = vcpu->arch.apic;
+	u32 lvt0 = apic_get_reg(apic, APIC_LVT0);
 
-	if (vcpu->vcpu_id == 0) {
-		if (!apic_hw_enabled(vcpu->arch.apic))
-			r = 1;
-		if ((lvt0 & APIC_LVT_MASKED) == 0 &&
-		    GET_APIC_DELIVERY_MODE(lvt0) == APIC_MODE_EXTINT)
-			r = 1;
-	}
-	return r;
+	/*
+	 * Virtual Wire mode, but we only deliver to the BSP.
+	 * Note that apic_sw_enabled() is covered by testing for !LVT_MASKED.
+	 */
+	if (vcpu->vcpu_id == 0 && apic_hw_enabled(apic)
+	    && !(lvt0 & APIC_LVT_MASKED)
+	    && GET_APIC_DELIVERY_MODE(lvt0) == APIC_MODE_EXTINT)
+		return 1;
+	return 0;
 }
 
 void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu)

Re: [PATCH] KVM: x86: Don't deliver PIC interrupts to disabled APICs - v2

[Sheng Yang]

On Mon, Oct 20, 2008 at 02:06:13PM +0200, Jan Kiszka wrote:
> [ taking Sheng's comments into account ]
> 
> The logic of kvm_apic_accept_pic_intr has a minor, practically hardly
> relevant incorrectness: PIC interrupts are still delivered even if the
> APIC of VPU0 (BSP) is disabled. This does not comply with the Virtual
> Wire mode according to the Intel MP spec.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Acked-by: Sheng Yang <sheng@linux.intel.com>

--
regards
Yang, Sheng
> ---
>  arch/x86/kvm/lapic.c |   21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> Index: b/arch/x86/kvm/lapic.c
> ===================================================================
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1089,17 +1089,18 @@ int kvm_apic_has_interrupt(struct kvm_vc
>  
>  int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu)
>  {
> -	u32 lvt0 = apic_get_reg(vcpu->arch.apic, APIC_LVT0);
> -	int r = 0;
> +	struct kvm_lapic *apic = vcpu->arch.apic;
> +	u32 lvt0 = apic_get_reg(apic, APIC_LVT0);
>  
> -	if (vcpu->vcpu_id == 0) {
> -		if (!apic_hw_enabled(vcpu->arch.apic))
> -			r = 1;
> -		if ((lvt0 & APIC_LVT_MASKED) == 0 &&
> -		    GET_APIC_DELIVERY_MODE(lvt0) == APIC_MODE_EXTINT)
> -			r = 1;
> -	}
> -	return r;
> +	/*
> +	 * Virtual Wire mode, but we only deliver to the BSP.
> +	 * Note that apic_sw_enabled() is covered by testing for !LVT_MASKED.
> +	 */
> +	if (vcpu->vcpu_id == 0 && apic_hw_enabled(apic)
> +	    && !(lvt0 & APIC_LVT_MASKED)
> +	    && GET_APIC_DELIVERY_MODE(lvt0) == APIC_MODE_EXTINT)
> +		return 1;
> +	return 0;
>  }
>  
>  void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu)
> --
> 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

Re: [PATCH] KVM: x86: Don't deliver PIC interrupts to disabled APICs - v2

[Avi Kivity]

Jan Kiszka wrote:
> [ taking Sheng's comments into account ]
>
> The logic of kvm_apic_accept_pic_intr has a minor, practically hardly
> relevant incorrectness: PIC interrupts are still delivered even if the
> APIC of VPU0 (BSP) is disabled. This does not comply with the Virtual
> Wire mode according to the Intel MP spec.
>   

Applied, thanks.

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

Re: [PATCH] KVM: x86: Don't deliver PIC interrupts to disabled APICs - v2

[Avi Kivity]

Jan Kiszka wrote:
> [ taking Sheng's comments into account ]
>
> The logic of kvm_apic_accept_pic_intr has a minor, practically hardly
> relevant incorrectness: PIC interrupts are still delivered even if the
> APIC of VPU0 (BSP) is disabled. This does not comply with the Virtual
> Wire mode according to the Intel MP spec.
>   

This breaks Windows XP with the Standard PC HAL, so I am unapplying this 
patch.

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

Re: [PATCH] KVM: x86: Don't deliver PIC interrupts to disabled APICs - v2

[Jan Kiszka]

Avi Kivity wrote:
> Jan Kiszka wrote:
>> [ taking Sheng's comments into account ]
>>
>> The logic of kvm_apic_accept_pic_intr has a minor, practically hardly
>> relevant incorrectness: PIC interrupts are still delivered even if the
>> APIC of VPU0 (BSP) is disabled. This does not comply with the Virtual
>> Wire mode according to the Intel MP spec.
>>   
> 
> This breaks Windows XP with the Standard PC HAL, so I am unapplying this
> patch.

Hmm, this points to either an APIC setup or BIOS bug. To my
understanding, the Standard PC HAL should not fiddle with the APIC, so
what the BIOS leaves behind should counts. But I think I found no traces
of APIC manipulation in rombios32.c.

Jan

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

Re: [PATCH] KVM: x86: Don't deliver PIC interrupts to disabled APICs - v2

[Jan Kiszka]

Jan Kiszka wrote:
> Avi Kivity wrote:
>> Jan Kiszka wrote:
>>> [ taking Sheng's comments into account ]
>>>
>>> The logic of kvm_apic_accept_pic_intr has a minor, practically hardly
>>> relevant incorrectness: PIC interrupts are still delivered even if the
>>> APIC of VPU0 (BSP) is disabled. This does not comply with the Virtual
>>> Wire mode according to the Intel MP spec.
>>>   
>> This breaks Windows XP with the Standard PC HAL, so I am unapplying this
>> patch.
> 
> Hmm, this points to either an APIC setup or BIOS bug. To my
> understanding, the Standard PC HAL should not fiddle with the APIC, so
> what the BIOS leaves behind should counts. But I think I found no traces
> of APIC manipulation in rombios32.c.

Manipulation on UP systems. There is fiddling for SMP. But I will check
again.

Jan

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

Re: [PATCH] KVM: x86: Don't deliver PIC interrupts to disabled APICs - v2

[Jan Kiszka]

Jan Kiszka wrote:
> Jan Kiszka wrote:
>> Avi Kivity wrote:
>>> Jan Kiszka wrote:
>>>> [ taking Sheng's comments into account ]
>>>>
>>>> The logic of kvm_apic_accept_pic_intr has a minor, practically hardly
>>>> relevant incorrectness: PIC interrupts are still delivered even if the
>>>> APIC of VPU0 (BSP) is disabled. This does not comply with the Virtual
>>>> Wire mode according to the Intel MP spec.
>>>>   
>>> This breaks Windows XP with the Standard PC HAL, so I am unapplying this
>>> patch.
>> Hmm, this points to either an APIC setup or BIOS bug. To my
>> understanding, the Standard PC HAL should not fiddle with the APIC, so
>> what the BIOS leaves behind should counts. But I think I found no traces
>> of APIC manipulation in rombios32.c.
> 
> Manipulation on UP systems. There is fiddling for SMP. But I will check
> again.

I take everything back: For yet unknown reasons Windows' standard HAL
actually decides to disable the APIC actively. Either there is a
short-path around a disabled APIC for Virtual Wire mode in Real Live
(though I fail to read this out of the spec), or Windows simply has a
bug here (MS insists on NOT supporting the Standard HAL on APIC systems
[1] - precisely the setup KVM is providing). Sheng, any comments on
this? Guess we have to live with the previous version, maybe with some
refactoring + commenting.

Jan

[1] http://support.microsoft.com/kb/309283/en
-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [PATCH] KVM: x86: Don't deliver PIC interrupts to disabled APICs - v2

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2073 bytes --]

Jan Kiszka wrote:
> Jan Kiszka wrote:
>> Jan Kiszka wrote:
>>> Avi Kivity wrote:
>>>> Jan Kiszka wrote:
>>>>> [ taking Sheng's comments into account ]
>>>>>
>>>>> The logic of kvm_apic_accept_pic_intr has a minor, practically hardly
>>>>> relevant incorrectness: PIC interrupts are still delivered even if the
>>>>> APIC of VPU0 (BSP) is disabled. This does not comply with the Virtual
>>>>> Wire mode according to the Intel MP spec.
>>>>>   
>>>> This breaks Windows XP with the Standard PC HAL, so I am unapplying this
>>>> patch.
>>> Hmm, this points to either an APIC setup or BIOS bug. To my
>>> understanding, the Standard PC HAL should not fiddle with the APIC, so
>>> what the BIOS leaves behind should counts. But I think I found no traces
>>> of APIC manipulation in rombios32.c.
>> Manipulation on UP systems. There is fiddling for SMP. But I will check
>> again.
> 
> I take everything back: For yet unknown reasons Windows' standard HAL
> actually decides to disable the APIC actively. Either there is a
> short-path around a disabled APIC for Virtual Wire mode in Real Live
> (though I fail to read this out of the spec), or Windows simply has a
> bug here (MS insists on NOT supporting the Standard HAL on APIC systems
> [1] - precisely the setup KVM is providing). Sheng, any comments on
> this? Guess we have to live with the previous version, maybe with some
> refactoring + commenting.

I was curious and played with my corresponding qemu patch [1]: It works
with the same Windows image that hangs under KVM. Then I looked at a
prominent guest-visible difference: the reported CPU type. QEMU claims
to provide a CPU called "qemu64" by default. Changing this to Pentium2
or newer makes Windows issue the lethal APIC disable command. On the
other hand, calling kvm with "-cpu pentium" makes Windows boot again.

However, I still can't tell from this if we see a Windows bug or if the
change is incorrect (but me feeling tends to the former).

Jan

[1] http://permalink.gmane.org/gmane.comp.emulators.qemu/31429


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH] KVM: x86: Don't deliver PIC interrupts to disabled APICs - v2

[Yang, Sheng]

On Thursday 23 October 2008 04:44:48 Jan Kiszka wrote:
> Jan Kiszka wrote:
> > Jan Kiszka wrote:
> >> Jan Kiszka wrote:
> >>> Avi Kivity wrote:
> >>>> Jan Kiszka wrote:
> >>>>> [ taking Sheng's comments into account ]
> >>>>>
> >>>>> The logic of kvm_apic_accept_pic_intr has a minor, practically hardly
> >>>>> relevant incorrectness: PIC interrupts are still delivered even if
> >>>>> the APIC of VPU0 (BSP) is disabled. This does not comply with the
> >>>>> Virtual Wire mode according to the Intel MP spec.
> >>>>
> >>>> This breaks Windows XP with the Standard PC HAL, so I am unapplying
> >>>> this patch.
> >>>
> >>> Hmm, this points to either an APIC setup or BIOS bug. To my
> >>> understanding, the Standard PC HAL should not fiddle with the APIC, so
> >>> what the BIOS leaves behind should counts. But I think I found no
> >>> traces of APIC manipulation in rombios32.c.
> >>
> >> Manipulation on UP systems. There is fiddling for SMP. But I will check
> >> again.
> >
> > I take everything back: For yet unknown reasons Windows' standard HAL
> > actually decides to disable the APIC actively. Either there is a
> > short-path around a disabled APIC for Virtual Wire mode in Real Live
> > (though I fail to read this out of the spec), or Windows simply has a
> > bug here (MS insists on NOT supporting the Standard HAL on APIC systems
> > [1] - precisely the setup KVM is providing). Sheng, any comments on
> > this? Guess we have to live with the previous version, maybe with some
> > refactoring + commenting.
>
> I was curious and played with my corresponding qemu patch [1]: It works
> with the same Windows image that hangs under KVM. Then I looked at a
> prominent guest-visible difference: the reported CPU type. QEMU claims
> to provide a CPU called "qemu64" by default. Changing this to Pentium2
> or newer makes Windows issue the lethal APIC disable command. On the
> other hand, calling kvm with "-cpu pentium" makes Windows boot again.
>
> However, I still can't tell from this if we see a Windows bug or if the
> change is incorrect (but me feeling tends to the former).

Confirmed that "-cpu pentium" solve the problem. But...

Sorry for that I've found that I neglected some info on the spec. SDM 3B 
5.3.1 "External Interrupts".

"When the local APIC is global/hardware disabled, these pins are configured as 
INTR and NMI pins, respectively."

So, it's right to inject PIC interrupt when LAPIC is hardware disabled. I 
think we can drop this patch...

(I will be more careful on such kind of issues next time...)
--
regards
Yang, Sheng

>
> Jan
>
> [1] http://permalink.gmane.org/gmane.comp.emulators.qemu/31429

Re: [PATCH] KVM: x86: Don't deliver PIC interrupts to disabled APICs - v2

[Jan Kiszka]

Yang, Sheng wrote:
> On Thursday 23 October 2008 04:44:48 Jan Kiszka wrote:
>> Jan Kiszka wrote:
>>> Jan Kiszka wrote:
>>>> Jan Kiszka wrote:
>>>>> Avi Kivity wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> [ taking Sheng's comments into account ]
>>>>>>>
>>>>>>> The logic of kvm_apic_accept_pic_intr has a minor, practically hardly
>>>>>>> relevant incorrectness: PIC interrupts are still delivered even if
>>>>>>> the APIC of VPU0 (BSP) is disabled. This does not comply with the
>>>>>>> Virtual Wire mode according to the Intel MP spec.
>>>>>> This breaks Windows XP with the Standard PC HAL, so I am unapplying
>>>>>> this patch.
>>>>> Hmm, this points to either an APIC setup or BIOS bug. To my
>>>>> understanding, the Standard PC HAL should not fiddle with the APIC, so
>>>>> what the BIOS leaves behind should counts. But I think I found no
>>>>> traces of APIC manipulation in rombios32.c.
>>>> Manipulation on UP systems. There is fiddling for SMP. But I will check
>>>> again.
>>> I take everything back: For yet unknown reasons Windows' standard HAL
>>> actually decides to disable the APIC actively. Either there is a
>>> short-path around a disabled APIC for Virtual Wire mode in Real Live
>>> (though I fail to read this out of the spec), or Windows simply has a
>>> bug here (MS insists on NOT supporting the Standard HAL on APIC systems
>>> [1] - precisely the setup KVM is providing). Sheng, any comments on
>>> this? Guess we have to live with the previous version, maybe with some
>>> refactoring + commenting.
>> I was curious and played with my corresponding qemu patch [1]: It works
>> with the same Windows image that hangs under KVM. Then I looked at a
>> prominent guest-visible difference: the reported CPU type. QEMU claims
>> to provide a CPU called "qemu64" by default. Changing this to Pentium2
>> or newer makes Windows issue the lethal APIC disable command. On the
>> other hand, calling kvm with "-cpu pentium" makes Windows boot again.
>>
>> However, I still can't tell from this if we see a Windows bug or if the
>> change is incorrect (but me feeling tends to the former).
> 
> Confirmed that "-cpu pentium" solve the problem. But...
> 
> Sorry for that I've found that I neglected some info on the spec. SDM 3B 
> 5.3.1 "External Interrupts".
> 
> "When the local APIC is global/hardware disabled, these pins are configured as 
> INTR and NMI pins, respectively."

Ah, that's the short-path...

> 
> So, it's right to inject PIC interrupt when LAPIC is hardware disabled. I 
> think we can drop this patch...
> 
> (I will be more careful on such kind of issues next time...)

Well, first of all, I brought this up. Sorry for all the fuss.

Jan

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