From mboxrd@z Thu Jan  1 00:00:00 1970
From: Gleb Natapov <gleb@redhat.com>
Subject: Re: [PATCH 3/3] Cleanup vmx_intr_assist()
Date: Sat, 11 Apr 2009 22:52:07 +0300
Message-ID: <20090411195207.GA24630@redhat.com>
References: <20090407090811.2074.19043.stgit@trex.usersys.redhat.com> <20090407090822.2074.27147.stgit@trex.usersys.redhat.com> <49E07F56.30107@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvm@vger.kernel.org
To: Avi Kivity <avi@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx2.redhat.com ([66.187.237.31]:43116 "EHLO mx2.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1755584AbZDKTwK (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sat, 11 Apr 2009 15:52:10 -0400
Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26])
	by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n3BJq949026537
	for <kvm@vger.kernel.org>; Sat, 11 Apr 2009 15:52:09 -0400
Content-Disposition: inline
In-Reply-To: <49E07F56.30107@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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.