[3/3] Cleanup vmx_intr_assist()

Message ID	20090407090822.2074.27147.stgit@trex.usersys.redhat.com (mailing list archive)
State	Not Applicable
Headers	show Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by demeter.kernel.org (8.14.2/8.14.2) with ESMTP id n37993ot007224 for <patchwork-kvm@patchwork.kernel.org>; Tue, 7 Apr 2009 09:09:03 GMT From: Gleb Natapov <gleb@redhat.com> Subject: [PATCH 3/3] Cleanup vmx_intr_assist() To: avi@redhat.com Cc: kvm@vger.kernel.org Date: Tue, 07 Apr 2009 12:08:22 +0300 Message-ID: <20090407090822.2074.27147.stgit@trex.usersys.redhat.com> In-Reply-To: <20090407090811.2074.19043.stgit@trex.usersys.redhat.com> References: <20090407090811.2074.19043.stgit@trex.usersys.redhat.com> User-Agent: StGIT/0.14.2 MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Sender: kvm-owner@vger.kernel.org Precedence: bulk

Message ID

20090407090822.2074.27147.stgit@trex.usersys.redhat.com (mailing list archive)

State

Not Applicable

Headers

From: Gleb Natapov <gleb@redhat.com>
Subject: [PATCH 3/3] Cleanup vmx_intr_assist()
To: avi@redhat.com
Cc: kvm@vger.kernel.org
Date: Tue, 07 Apr 2009 12:08:22 +0300
Message-ID: <20090407090822.2074.27147.stgit@trex.usersys.redhat.com>
In-Reply-To: <20090407090811.2074.19043.stgit@trex.usersys.redhat.com>
References: <20090407090811.2074.19043.stgit@trex.usersys.redhat.com>
User-Agent: StGIT/0.14.2
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
Sender: kvm-owner@vger.kernel.org
Precedence: bulk

Commit Message

Gleb Natapov April 7, 2009, 9:08 a.m. UTC

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---

 arch/x86/kvm/vmx.c |   55 ++++++++++++++++++++++++++++------------------------
 1 files changed, 30 insertions(+), 25 deletions(-)


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

Comments

Avi Kivity April 11, 2009, 11:30 a.m. UTC | #1

Gleb Natapov wrote:
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
>
>  arch/x86/kvm/vmx.c |   55 ++++++++++++++++++++++++++++------------------------
>  1 files changed, 30 insertions(+), 25 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 06252f7..9eb518f 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -3309,6 +3309,34 @@ static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
>  	}
>  }
>  
> +static void vmx_intr_inject(struct kvm_vcpu *vcpu)
> +{
> +	/* try to reinject previous events if any */
> +	if (vcpu->arch.nmi_injected) {
> +		vmx_inject_nmi(vcpu);
> +		return;
> +	}
> +
> +	if (vcpu->arch.interrupt.pending) {
> +		vmx_inject_irq(vcpu, vcpu->arch.interrupt.nr);
> +		return;
> +	}
> +
> +	/* try to inject new event if pending */
> +	if (vcpu->arch.nmi_pending) {
> +		if (vcpu->arch.nmi_window_open) {
> +			vcpu->arch.nmi_pending = false;
> +			vcpu->arch.nmi_injected = true;
> +			vmx_inject_nmi(vcpu);
> +		}
> +	} else if (kvm_cpu_has_interrupt(vcpu)) {
> +		if (vcpu->arch.interrupt_window_open) {
> +			kvm_queue_interrupt(vcpu, kvm_cpu_get_interrupt(vcpu));
> +			vmx_inject_irq(vcpu, vcpu->arch.interrupt.nr);
> +		}
> +	}
> +}
> +
>  static void vmx_intr_assist(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  {
>  	bool req_int_win = !irqchip_in_kernel(vcpu->kvm) &&
> @@ -3323,32 +3351,9 @@ static void vmx_intr_assist(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  				GUEST_INTR_STATE_STI |
>  				GUEST_INTR_STATE_MOV_SS);
>  
> -	if (vcpu->arch.nmi_pending && !vcpu->arch.nmi_injected) {
> -		if (vcpu->arch.interrupt.pending) {
> -			enable_nmi_window(vcpu);
> -		} else if (vcpu->arch.nmi_window_open) {
> -			vcpu->arch.nmi_pending = false;
> -			vcpu->arch.nmi_injected = true;
> -		} else {
> -			enable_nmi_window(vcpu);
> -			return;
> -		}
> -	}
> -
> -	if (vcpu->arch.nmi_injected) {
> -		vmx_inject_nmi(vcpu);
> -		goto out;
> -	}
> -
> -	if (!vcpu->arch.interrupt.pending && kvm_cpu_has_interrupt(vcpu)) {
> -		if (vcpu->arch.interrupt_window_open)
> -			kvm_queue_interrupt(vcpu, kvm_cpu_get_interrupt(vcpu));
> -	}
> -
> -	if (vcpu->arch.interrupt.pending)
> -		vmx_inject_irq(vcpu, vcpu->arch.interrupt.nr);
> +	vmx_intr_inject(vcpu);
>  
> -out:
> +	/* enable NMI/IRQ window open exits if needed */
>  	if (vcpu->arch.nmi_pending)
>  		enable_nmi_window(vcpu);
>  	else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
>   

Not sure I understand the motivation.  Just replace a 'goto out' with a 
return?

Gleb Natapov April 11, 2009, 7:52 p.m. UTC | #2

On Sat, Apr 11, 2009 at 02:30:30PM +0300, Avi Kivity wrote:
> Gleb Natapov wrote:
>> Signed-off-by: Gleb Natapov <gleb@redhat.com>
>> ---
>>
>>  arch/x86/kvm/vmx.c |   55 ++++++++++++++++++++++++++++------------------------
>>  1 files changed, 30 insertions(+), 25 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 06252f7..9eb518f 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -3309,6 +3309,34 @@ static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
>>  	}
>>  }
>>  +static void vmx_intr_inject(struct kvm_vcpu *vcpu)
>> +{
>> +	/* try to reinject previous events if any */
>> +	if (vcpu->arch.nmi_injected) {
>> +		vmx_inject_nmi(vcpu);
>> +		return;
>> +	}
>> +
>> +	if (vcpu->arch.interrupt.pending) {
>> +		vmx_inject_irq(vcpu, vcpu->arch.interrupt.nr);
>> +		return;
>> +	}
>> +
>> +	/* try to inject new event if pending */
>> +	if (vcpu->arch.nmi_pending) {
>> +		if (vcpu->arch.nmi_window_open) {
>> +			vcpu->arch.nmi_pending = false;
>> +			vcpu->arch.nmi_injected = true;
>> +			vmx_inject_nmi(vcpu);
>> +		}
>> +	} else if (kvm_cpu_has_interrupt(vcpu)) {
>> +		if (vcpu->arch.interrupt_window_open) {
>> +			kvm_queue_interrupt(vcpu, kvm_cpu_get_interrupt(vcpu));
>> +			vmx_inject_irq(vcpu, vcpu->arch.interrupt.nr);
>> +		}
>> +	}
>> +}
>> +
>>  static void vmx_intr_assist(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>>  {
>>  	bool req_int_win = !irqchip_in_kernel(vcpu->kvm) &&
>> @@ -3323,32 +3351,9 @@ static void vmx_intr_assist(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>>  				GUEST_INTR_STATE_STI |
>>  				GUEST_INTR_STATE_MOV_SS);
>>  -	if (vcpu->arch.nmi_pending && !vcpu->arch.nmi_injected) {
>> -		if (vcpu->arch.interrupt.pending) {
>> -			enable_nmi_window(vcpu);
>> -		} else if (vcpu->arch.nmi_window_open) {
>> -			vcpu->arch.nmi_pending = false;
>> -			vcpu->arch.nmi_injected = true;
>> -		} else {
>> -			enable_nmi_window(vcpu);
>> -			return;
>> -		}
>> -	}
>> -
>> -	if (vcpu->arch.nmi_injected) {
>> -		vmx_inject_nmi(vcpu);
>> -		goto out;
>> -	}
>> -
>> -	if (!vcpu->arch.interrupt.pending && kvm_cpu_has_interrupt(vcpu)) {
>> -		if (vcpu->arch.interrupt_window_open)
>> -			kvm_queue_interrupt(vcpu, kvm_cpu_get_interrupt(vcpu));
>> -	}
>> -
>> -	if (vcpu->arch.interrupt.pending)
>> -		vmx_inject_irq(vcpu, vcpu->arch.interrupt.nr);
>> +	vmx_intr_inject(vcpu);
>>  -out:
>> +	/* enable NMI/IRQ window open exits if needed */
>>  	if (vcpu->arch.nmi_pending)
>>  		enable_nmi_window(vcpu);
>>  	else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
>>   
>
> Not sure I understand the motivation.  Just replace a 'goto out' with a  
> return?
>
Yes. The code is much cleaner this way. It is easy to see that no matter what happened
during injection irq/nmi windows is enabled if there are pending events. With gotos
and returns scattered over the function you'll need to check every single path that may
or may not lead to the window enable code.

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

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 06252f7..9eb518f 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3309,6 +3309,34 @@  static void vmx_complete_interrupts(struct vcpu_vmx *vmx)
 	}
 }
 
+static void vmx_intr_inject(struct kvm_vcpu *vcpu)
+{
+	/* try to reinject previous events if any */
+	if (vcpu->arch.nmi_injected) {
+		vmx_inject_nmi(vcpu);
+		return;
+	}
+
+	if (vcpu->arch.interrupt.pending) {
+		vmx_inject_irq(vcpu, vcpu->arch.interrupt.nr);
+		return;
+	}
+
+	/* try to inject new event if pending */
+	if (vcpu->arch.nmi_pending) {
+		if (vcpu->arch.nmi_window_open) {
+			vcpu->arch.nmi_pending = false;
+			vcpu->arch.nmi_injected = true;
+			vmx_inject_nmi(vcpu);
+		}
+	} else if (kvm_cpu_has_interrupt(vcpu)) {
+		if (vcpu->arch.interrupt_window_open) {
+			kvm_queue_interrupt(vcpu, kvm_cpu_get_interrupt(vcpu));
+			vmx_inject_irq(vcpu, vcpu->arch.interrupt.nr);
+		}
+	}
+}
+
 static void vmx_intr_assist(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 {
 	bool req_int_win = !irqchip_in_kernel(vcpu->kvm) &&
@@ -3323,32 +3351,9 @@  static void vmx_intr_assist(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 				GUEST_INTR_STATE_STI |
 				GUEST_INTR_STATE_MOV_SS);
 
-	if (vcpu->arch.nmi_pending && !vcpu->arch.nmi_injected) {
-		if (vcpu->arch.interrupt.pending) {
-			enable_nmi_window(vcpu);
-		} else if (vcpu->arch.nmi_window_open) {
-			vcpu->arch.nmi_pending = false;
-			vcpu->arch.nmi_injected = true;
-		} else {
-			enable_nmi_window(vcpu);
-			return;
-		}
-	}
-
-	if (vcpu->arch.nmi_injected) {
-		vmx_inject_nmi(vcpu);
-		goto out;
-	}
-
-	if (!vcpu->arch.interrupt.pending && kvm_cpu_has_interrupt(vcpu)) {
-		if (vcpu->arch.interrupt_window_open)
-			kvm_queue_interrupt(vcpu, kvm_cpu_get_interrupt(vcpu));
-	}
-
-	if (vcpu->arch.interrupt.pending)
-		vmx_inject_irq(vcpu, vcpu->arch.interrupt.nr);
+	vmx_intr_inject(vcpu);
 
-out:
+	/* enable NMI/IRQ window open exits if needed */
 	if (vcpu->arch.nmi_pending)
 		enable_nmi_window(vcpu);
 	else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)

[3/3] Cleanup vmx_intr_assist()

Commit Message

Comments

Patch