KVM: LAPIC: Unified the duplicate calling of setting IRR

RFC PATCH In review Yang, Sheng 2 participants 2 messages May 8, 2008 – May 9, 2008

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Yang, Sheng

author

2008-05-08 08:55 UTC #0 lore raw

RFC PATCH 1/4 KVM: LAPIC: Unified the duplicate calling of setting IRR

From 650cad44069541fcd9fea8be6a78837e812b3dfd Mon Sep 17 00:00:00 2001
From: Sheng Yang <[email protected]>
Date: Thu, 8 May 2008 09:58:50 +0800
Subject: [PATCH 1/4] KVM: LAPIC: Unified the duplicate calling of setting IRR

It's strange got two callings of setting IRR seperately for IOAPIC and IPI in lapic. The patch unified them into __apic_set_irq().

Signed-off-by Sheng Yang <[email protected]>

---
 arch/x86/kvm/lapic.c |   69 +++++++++++++++++++++++--------------------------
 1 files changed, 32 insertions(+), 37 deletions(-)

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

index 7652f88..6226fe0 100644
@@ -184,20 +184,40 @@ int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_lapic_find_highest_irr);
 
-int kvm_apic_set_irq(struct kvm_vcpu *vcpu, u8 vec, u8 trig)
+static int __apic_set_irq(struct kvm_vcpu *vcpu, u8 vector, u8 trig_mode)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
 
-	if (!apic_test_and_set_irr(vec, apic)) {
-		/* a new pending irq is set in IRR */
-		if (trig)
-			apic_set_vector(vec, apic->regs + APIC_TMR);
-		else
-			apic_clear_vector(vec, apic->regs + APIC_TMR);
-		kvm_vcpu_kick(apic->vcpu);
-		return 1;
+	/* FIXME add logic for vcpu on reset */
+	if (unlikely(!apic_enabled(apic)))
+		return 0;
+
+	if (apic_test_and_set_irr(vector, apic)) {
+		if (trig_mode)
+			apic_debug("level trig mode repeatedly for vector %d\n",
+					vector);
+		return 0;
 	}
-	return 0;
+
+	if (trig_mode) {
+		apic_debug("level trig mode for vector %d\n", vector);
+		apic_set_vector(vector, apic->regs + APIC_TMR);
+	} else
+		apic_clear_vector(vector, apic->regs + APIC_TMR);
+
+	if (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE)
+		kvm_vcpu_kick(vcpu);
+	else if (vcpu->arch.mp_state == KVM_MP_STATE_HALTED) {
+		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
+		if (waitqueue_active(&vcpu->wq))
+			wake_up_interruptible(&vcpu->wq);
+	}
+	return 1;
+}
+
+int kvm_apic_set_irq(struct kvm_vcpu *vcpu, u8 vec, u8 trig)
+{
+	return __apic_set_irq(vcpu, vec, trig);
 }
 
 static inline int apic_find_highest_isr(struct kvm_lapic *apic)

@@ -315,38 +335,13 @@ static int apic_match_dest(struct kvm_vcpu *vcpu, struct

kvm_lapic *source,
static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
int vector, int level, int trig_mode)
{
- int orig_irr, result = 0;
+ int result = 0;
struct kvm_vcpu *vcpu = apic->vcpu;

switch (delivery_mode) {
case APIC_DM_FIXED:
case APIC_DM_LOWEST:
- /* FIXME add logic for vcpu on reset */
- if (unlikely(!apic_enabled(apic)))
- break;
-
- orig_irr = apic_test_and_set_irr(vector, apic);
- if (orig_irr && trig_mode) {
- apic_debug("level trig mode repeatedly for vector %d",
- vector);
- 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);
-
- if (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE)
- kvm_vcpu_kick(vcpu);
- else if (vcpu->arch.mp_state == KVM_MP_STATE_HALTED) {
- vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
- if (waitqueue_active(&vcpu->wq))
- wake_up_interruptible(&vcpu->wq);
- }
-
- result = (orig_irr == 0);
+ result = __apic_set_irq(vcpu, vector, trig_mode);
break;

case APIC_DM_REMRD:
--
1.5.5

It's strange got two callings of setting IRR seperately for IOAPIC and IPI in lapic. The patch unified them into __apic_set_irq().

Signed-off-by Sheng Yang <[email protected]>

---
 arch/x86/kvm/lapic.c |   69 +++++++++++++++++++++++--------------------------
 1 files changed, 32 insertions(+), 37 deletions(-)

arch/x86/kvm/lapic.c +32 ~~-37~~

90 lines not on this page · raw message

Signature · 1 line

1.5.5

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

_______________________________________________
kvm-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Avi Kivity

2008-05-09 15:43 UTC #1 lore raw

Quoted 51 lines · Sheng

It's strange got two callings of setting IRR seperately for IOAPIC and IPI in lapic. The patch unified them into __apic_set_irq().

Signed-off-by Sheng Yang <[email protected]>

---
 arch/x86/kvm/lapic.c |   69 +++++++++++++++++++++++--------------------------
 1 files changed, 32 insertions(+), 37 deletions(-)

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

index 7652f88..6226fe0 100644
@@ -184,20 +184,40 @@ int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_lapic_find_highest_irr);
 
-int kvm_apic_set_irq(struct kvm_vcpu *vcpu, u8 vec, u8 trig)
+static int __apic_set_irq(struct kvm_vcpu *vcpu, u8 vector, u8 trig_mode)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
 
-	if (!apic_test_and_set_irr(vec, apic)) {
-		/* a new pending irq is set in IRR */
-		if (trig)
-			apic_set_vector(vec, apic->regs + APIC_TMR);
-		else
-			apic_clear_vector(vec, apic->regs + APIC_TMR);
-		kvm_vcpu_kick(apic->vcpu);
-		return 1;
+	/* FIXME add logic for vcpu on reset */
+	if (unlikely(!apic_enabled(apic)))
+		return 0;
+
+	if (apic_test_and_set_irr(vector, apic)) {
+		if (trig_mode)
+			apic_debug("level trig mode repeatedly for vector %d\n",
+					vector);
+		return 0;
 	}
-	return 0;
+
+	if (trig_mode) {
+		apic_debug("level trig mode for vector %d\n", vector);
+		apic_set_vector(vector, apic->regs + APIC_TMR);
+	} else
+		apic_clear_vector(vector, apic->regs + APIC_TMR);

arch/x86/kvm/lapic.c int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu) · line 216

Quoted 10 lines

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

+
+	if (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE)
+		kvm_vcpu_kick(vcpu);
 
+	else if (vcpu->arch.mp_state == KVM_MP_STATE_HALTED) {
+		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
+		if (waitqueue_active(&vcpu->wq))
+			wake_up_interruptible(&vcpu->wq);
+	}

This can race against the vcpu executing hlt if the compiler reorders the tests (and maybe against waking up from hlt if it does not). It's best to be conservative like the original code and not optimize away kicks, unless many of them are unnecessary.

arch/x86/kvm/lapic.c int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu) · line 221

Quoted 8 lines

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

+	return 1;
+}
+
+int kvm_apic_set_irq(struct kvm_vcpu *vcpu, u8 vec, u8 trig)
+{
+	return __apic_set_irq(vcpu, vec, trig);
 }

Why create a new function instead of modifying the original?

Signature · 8 lines

Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone