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

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

[Yang, Sheng]

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

From 650cad44069541fcd9fea8be6a78837e812b3dfd Mon Sep 17 00:00:00 2001
From: Sheng Yang <sheng.yang@intel.com>
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 <sheng.yang@intel.com>
---
 arch/x86/kvm/lapic.c |   69 +++++++++++++++++++++++--------------------------
 1 files changed, 32 insertions(+), 37 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 7652f88..6226fe0 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -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


[-- Attachment #2: 0001-KVM-LAPIC-Unified-the-duplicate-calling-of-setting.patch --]
[-- Type: text/x-diff, Size: 3509 bytes --]

From 650cad44069541fcd9fea8be6a78837e812b3dfd Mon Sep 17 00:00:00 2001
From: Sheng Yang <sheng.yang@intel.com>
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 <sheng.yang@intel.com>
---
 arch/x86/kvm/lapic.c |   69 +++++++++++++++++++++++--------------------------
 1 files changed, 32 insertions(+), 37 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 7652f88..6226fe0 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -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


[-- Attachment #3: Type: text/plain, Size: 320 bytes --]

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

[-- Attachment #4: Type: text/plain, Size: 158 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel

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

[Avi Kivity]

Yang, Sheng wrote:
> From 650cad44069541fcd9fea8be6a78837e812b3dfd Mon Sep 17 00:00:00 2001
> From: Sheng Yang <sheng.yang@intel.com>
> 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 <sheng.yang@intel.com>
> ---
>  arch/x86/kvm/lapic.c |   69 +++++++++++++++++++++++--------------------------
>  1 files changed, 32 insertions(+), 37 deletions(-)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 7652f88..6226fe0 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -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);
> +	}
>   

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.

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


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