Re: [PATCH 3/3] Cleanup vmx_intr_assist()

[Gleb Natapov <gleb@redhat.com>]

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.