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

PATCH In review Jan Kiszka 5 participants 14 messages Oct 20, 2008 – Oct 23, 2008

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Jan Kiszka

author

2008-10-20 08:51 UTC #0 lore raw

PATCH 1/1 KVM: x86: Don't deliver PIC interrupts to disabled APICs

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 <[email protected]>

---
 arch/x86/kvm/lapic.c |   19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

arch/x86/kvm/lapic.c +9 ~~-10~~

Index: 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)

Sheng Yang

2008-10-20 11:51 UTC #1 lore raw

Quoted 1 line · Jan Kiszka

The locic of kvm_apic_accept_pic_intr has a minor, practically hardly

Typo...

Quoted 16 lines

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 <[email protected]>
---
 arch/x86/kvm/lapic.c |   19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)
Index: b/arch/x86/kvm/lapic.c
===================================================================

arch/x86/kvm/lapic.c void kvm_lapic_reset(struct kvm_vcpu *vc · line 937

Quoted 6 lines

arch/x86/kvm/lapic.c +2 -1

@@ -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()?

arch/x86/kvm/lapic.c int kvm_apic_has_interrupt(struct kvm_vc · line 1094

Quoted 9 lines

arch/x86/kvm/lapic.c +1 -0

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

arch/x86/kvm/lapic.c int kvm_apic_has_interrupt(struct kvm_vc · line 1097

Quoted 12 lines

arch/x86/kvm/lapic.c +2 -9

-	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

Quoted 11 lines

arch/x86/kvm/lapic.c +4 -1
+	    && !(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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jan Kiszka

author

2008-10-20 11:57 UTC #2 lore raw

Quoted 16 lines · Sheng Yang

Quoted 1 line · Jan Kiszka
The locic of kvm_apic_accept_pic_intr has a minor, practically hardly
Typo...
Quoted 11 lines
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 <[email protected]>
---
 arch/x86/kvm/lapic.c |   19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

Quoted 13 lines

arch/x86/kvm/lapic.c void kvm_lapic_reset(struct kvm_vcpu *vc · line 937
Quoted 11 lines
Index: b/arch/x86/kvm/lapic.c
===================================================================
arch/x86/kvm/lapic.c +2 -1
@@ -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.

Quoted 6 lines

arch/x86/kvm/lapic.c +0 -0

 	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

Quoted 6 lines

arch/x86/kvm/lapic.c int kvm_apic_has_interrupt(struct kvm_vc · line 1094
Quoted 4 lines
arch/x86/kvm/lapic.c +1 -0
 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. :)

Quoted 16 lines

arch/x86/kvm/lapic.c int kvm_apic_has_interrupt(struct kvm_vc · line 1097
Quoted 13 lines
arch/x86/kvm/lapic.c +2 -9
-	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.

Quoted 10 lines

--
regards
Yang, Sheng

Quoted 6 lines

arch/x86/kvm/lapic.c +4 -0

+	    && !(lvt0 & APIC_LVT_MASKED)
+	    && GET_APIC_DELIVERY_MODE(lvt0) == APIC_MODE_EXTINT)
+		return 1;
+	return 0;
 }

Quoted 5 lines

Quoted 5 lines
arch/x86/kvm/lapic.c +0 -1
 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jan

Signature · 2 lines

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

Jan Kiszka

author

2008-10-20 12:02 UTC #3 lore raw

Quoted 59 lines · Jan Kiszka

Quoted 15 lines · Sheng Yang
Quoted 1 line · Jan Kiszka
The locic of kvm_apic_accept_pic_intr has a minor, practically hardly
Typo...
Quoted 11 lines
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 <[email protected]>
---
 arch/x86/kvm/lapic.c |   19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)
Quoted 12 lines
arch/x86/kvm/lapic.c void kvm_lapic_reset(struct kvm_vcpu *vc · line 937
Quoted 11 lines
Index: b/arch/x86/kvm/lapic.c
===================================================================
arch/x86/kvm/lapic.c +2 -1
@@ -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.
Quoted 10 lines
arch/x86/kvm/lapic.c int kvm_apic_has_interrupt(struct kvm_vc · line 1094
Quoted 9 lines
arch/x86/kvm/lapic.c +1 -0
 	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. :)
Quoted 14 lines
arch/x86/kvm/lapic.c int kvm_apic_has_interrupt(struct kvm_vc · line 1097
Quoted 12 lines
arch/x86/kvm/lapic.c +2 -9
-	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...)

Quoted 2 lines

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.

Quoted 5 lines

Quoted 5 lines
--
regards
Yang, Sheng

Quoted 7 lines

Quoted 7 lines
arch/x86/kvm/lapic.c int kvm_apic_has_interrupt(struct kvm_vc · line 1104
Quoted 7 lines
arch/x86/kvm/lapic.c +4 -0
+	    && !(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

Signature · 2 lines

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

Jan Kiszka

author

2008-10-20 12:06 UTC #4 lore raw

PATCH 1/1 KVM: x86: Don't deliver PIC interrupts to disabled APICs - v2

In reply to 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 <[email protected]>

---
 arch/x86/kvm/lapic.c |   21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

arch/x86/kvm/lapic.c +11 ~~-10~~

Index: 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)

Sheng Yang

Acked-by on v1 1/1

2008-10-20 12:22 UTC #5 lore raw

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

Quoted 8 lines · 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 <[email protected]>

Acked-by Sheng Yang <[email protected]>

--
regards
Yang, Sheng

Quoted 41 lines

---
 arch/x86/kvm/lapic.c |   21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

Index: b/arch/x86/kvm/lapic.c
===================================================================

arch/x86/kvm/lapic.c +11 ~~-11~~

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Avi Kivity

Applied

2008-10-22 10:22 UTC #6 lore raw

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

In reply to Jan Kiszka

Quoted 7 lines · 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.

Applied, thanks.

Signature · 1 line

error compiling committee.c: too many arguments to function

Avi Kivity

2008-10-22 14:08 UTC #7 lore raw

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

In reply to Jan Kiszka

Quoted 7 lines · 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.

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

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2008-10-22 15:10 UTC #8 lore raw

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

Quoted 11 lines · Avi Kivity

Quoted 7 lines · 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.
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

Signature · 2 lines

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

Jan Kiszka

author

2008-10-22 15:12 UTC #9 lore raw

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

Quoted 9 lines · Jan Kiszka

Quoted 8 lines · Avi Kivity
Quoted 7 lines · 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.

Quoted 7 lines

Quoted 2 lines
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

Signature · 2 lines

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

Jan Kiszka

author

2008-10-22 15:40 UTC #10 lore raw

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

Quoted 12 lines · Jan Kiszka

Quoted 11 lines · Jan Kiszka
Quoted 10 lines · Avi Kivity
Quoted 7 lines · 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.
This breaks Windows XP with the Standard PC HAL, so I am unapplying this patch.

Quoted 7 lines

Quoted 4 lines
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

Signature · 2 lines

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

Jan Kiszka

2008-10-22 20:44 UTC #11 lore raw

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

Quoted 20 lines · Jan Kiszka

Quoted 10 lines · Jan Kiszka
Quoted 9 lines · Jan Kiszka
Quoted 8 lines · Avi Kivity
Quoted 7 lines · 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.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
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.

Quoted 8 lines

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

Yang, Sheng

2008-10-23 01:52 UTC #12 lore raw

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

Quoted 39 lines · On Thursday 23 October 2008 04:44:48 Jan Kiszka wrote:

Quoted 22 lines · Jan Kiszka
Quoted 13 lines · Jan Kiszka
Quoted 12 lines · Jan Kiszka
Quoted 10 lines · Avi Kivity
Quoted 6 lines · 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.
This breaks Windows XP with the Standard PC HAL, so I am unapplying this patch.
Quoted 7 lines
Quoted 4 lines
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.
Quoted 8 lines
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.

Quoted 2 lines

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

Quoted 4 lines

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

Jan Kiszka

author

2008-10-23 08:12 UTC #13 lore raw

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

Quoted 38 lines · Sheng

Quoted 34 lines · On Thursday 23 October 2008 04:44:48 Jan Kiszka wrote:
Quoted 18 lines · Jan Kiszka
Quoted 9 lines · Jan Kiszka
Quoted 8 lines · Jan Kiszka
Quoted 7 lines · Avi Kivity
Quoted 6 lines · 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.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
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.
Quoted 8 lines
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.
Quoted 2 lines
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).

Quoted 7 lines

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

Quoted 5 lines

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

Signature · 2 lines

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