Re: [PATCH 3/3] VMX: Enhance invalid guest state emulation

[Avi Kivity <avi@redhat.com>]

On 08/28/2009 05:49 PM, Mohammed Gamal wrote:
> - Change returned handle_invalid_guest_state() to return relevant exit codes
> - Move triggering the emulation from vmx_vcpu_run() to vmx_handle_exit()
> - Return to userspace instead of repeatedly trying to emulate
> instructions that have already failed
>
> Signed-off-by: Mohammed Gamal<m.gamal005@gmail.com>
> ---
>   arch/x86/kvm/vmx.c |   24 ++++++++++++++----------
>   1 files changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 78101dd..e422470 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -3318,10 +3318,11 @@ static int handle_nmi_window(struct kvm_vcpu *vcpu)
>   	return 1;
>   }
>
> -static void handle_invalid_guest_state(struct kvm_vcpu *vcpu)
> +static int handle_invalid_guest_state(struct kvm_vcpu *vcpu)
>   {
>   	struct vcpu_vmx *vmx = to_vmx(vcpu);
>   	enum emulation_result err = EMULATE_DONE;
> +	int ret = 1;
>
>   	local_irq_enable();
>   	preempt_enable();
>    

These are now wrong, since handle_invalid_exit() is called with 
interrupts and preemption enabled.

> @@ -3329,11 +3330,16 @@ static void handle_invalid_guest_state(struct kvm_vcpu *vcpu)
>   	while (!guest_state_valid(vcpu)) {
>   		err = emulate_instruction(vcpu, 0, 0, 0);
>
> -		if (err == EMULATE_DO_MMIO)
> +		if (err == EMULATE_DO_MMIO) {
> +			ret = 0;
>   			break;
> +		}
>
>   		if (err != EMULATE_DONE) {
>   			kvm_report_emulation_failure(vcpu, "emulation failure");
> +			vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> +			vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
> +			ret = 0;
>   			break;
>   		}
>
> @@ -3347,6 +3353,7 @@ static void handle_invalid_guest_state(struct kvm_vcpu *vcpu)
>   	local_irq_disable();
>
>   	vmx->invalid_state_emulation_result = err;
> +	return ret;
>   }
>
>   /*
> @@ -3405,9 +3412,12 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu)
>   	/* If we need to emulate an MMIO from handle_invalid_guest_state
>   	 * we just return 0 */
>   	if (vmx->emulation_required&&  emulate_invalid_guest_state) {
> -		if (guest_state_valid(vcpu))
> +		if (guest_state_valid(vcpu)) {
>   			vmx->emulation_required = 0;
> -		return vmx->invalid_state_emulation_result != EMULATE_DO_MMIO;
> +			return vmx->invalid_state_emulation_result != EMULATE_DO_MMIO;	
>    

This looks fishy.  Can't say exactly why but vmx_handle_exit() should 
only depend on the current guest execution, not the previous guest 
execution.

> +		} else {
> +			return handle_invalid_guest_state(vcpu);
> +		}
>   	}
>
>   	/* Access CR3 don't cause VMExit in paging mode, so we need
> @@ -3603,12 +3613,6 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
>   	if (unlikely(!cpu_has_virtual_nmis()&&  vmx->soft_vnmi_blocked))
>   		vmx->entry_time = ktime_get();
>
> -	/* Handle invalid guest state instead of entering VMX */
> -	if (vmx->emulation_required&&  emulate_invalid_guest_state) {
> -		handle_invalid_guest_state(vcpu);
> -		return;
> -	}
> -
>    

Don't we still need to return here?  Otherwise we attempt guest entry 
needlessly.

>   	if (test_bit(VCPU_REGS_RSP, (unsigned long *)&vcpu->arch.regs_dirty))
>   		vmcs_writel(GUEST_RSP, vcpu->arch.regs[VCPU_REGS_RSP]);
>   	if (test_bit(VCPU_REGS_RIP, (unsigned long *)&vcpu->arch.regs_dirty))
>    


-- 
error compiling committee.c: too many arguments to function