Skip to contentSkip to conversation

KVM: Fix race between APIC TMR and IRR

PATCH Applied Avi Kivity 2 participants 3 messages Dec 29, 2009 – Dec 30, 2009

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Avi Kivity

author
#0 lore raw

PATCH 1/1 KVM: Fix race between APIC TMR and IRR

When we queue an interrupt to the local apic, we set the IRR before the TMR. The vcpu can pick up the IRR and inject the interrupt before setting the TMR, and perhaps even EOI it, causing incorrect behaviour.

The race is really insignificant since it can only occur on the first interrupt (usually following interrupts will not change TMR), but it's better closed than open.

Fixed by reordering setting the TMR vs IRR.

Signed-off-by Avi Kivity <[email protected]>
---
 arch/x86/kvm/lapic.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)
arch/x86/kvm/lapic.c +6 -5
index 3063a0c..ba8c045 100644
@@ -373,6 +373,12 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 		if (unlikely(!apic_enabled(apic)))
 			break;
 
+		if (trig_mode) {
+			apic_debug("level trig mode for vector %d", vector);
+			apic_set_vector(vector, apic->regs + APIC_TMR);
+		} else
+			apic_clear_vector(vector, apic->regs + APIC_TMR);
+
 		result = !apic_test_and_set_irr(vector, apic);
 		trace_kvm_apic_accept_irq(vcpu->vcpu_id, delivery_mode,
 					  trig_mode, vector, !result);

@@ -383,11 +389,6 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 			break;
 		}
 
-		if (trig_mode) {
-			apic_debug("level trig mode for vector %d", vector);
-			apic_set_vector(vector, apic->regs + APIC_TMR);
-		} else
-			apic_clear_vector(vector, apic->regs + APIC_TMR);
 		kvm_vcpu_kick(vcpu);
 		break;
Signature · 1 line
1.6.5.3
Applied
#1 lore raw
Quoted 7 lines · Avi Kivity

When we queue an interrupt to the local apic, we set the IRR before the TMR. The vcpu can pick up the IRR and inject the interrupt before setting the TMR, and perhaps even EOI it, causing incorrect behaviour.

The race is really insignificant since it can only occur on the first interrupt (usually following interrupts will not change TMR), but it's better closed than open.

The EOI handler clears TMR, so AFAICS it can occur not only on the first interrupt.

Applied, thanks.

author
#2 lore raw
Quoted 6 lines · Marcelo Tosatti
Quoted 4 lines · Avi Kivity

When we queue an interrupt to the local apic, we set the IRR before the TMR. The vcpu can pick up the IRR and inject the interrupt before setting the TMR, and perhaps even EOI it, causing incorrect behaviour.

Quoted 7 lines
Quoted 4 lines

The race is really insignificant since it can only occur on the first interrupt (usually following interrupts will not change TMR), but it's better closed than open.

The EOI handler clears TMR, so AFAICS it can occur not only on the first interrupt.

In that case, we ought to queue it for .33 and -stable.

Signature · 1 line
error compiling committee.c: too many arguments to function