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

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

[Mohammed Gamal]

- 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();
@@ -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;	
+		} 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;
-	}
-
 	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))
-- 
1.6.0.4

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

[Avi Kivity]

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

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

[Avi Kivity]

On 08/31/2009 04:39 PM, Mohammed Gamal wrote:
>>>         local_irq_enable();
>>>         preempt_enable();
>>>
>>>        
>> These are now wrong, since handle_invalid_exit() is called with interrupts
>> and preemption enabled.
>>
>>      
> Do you mean vmx_handle_exit() ?
>    

Yes.

>>>   /*
>>> @@ -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.
>>      
> I still can't quite get the problem here!
>    

The design of the main loop is that you can have a save/restore cycle 
(or live migration) after __vcpu_run().  So abything in __vcpu_run() and 
the function it calls can only depend on state set previously in 
__vcpu_run(), or on state that is loaded from KVM_SET_REGS and similar 
ioctls.

In this case I think vmx->invalid_state_emulation_result is not set 
previously to its use within __vcpu_run().

>>>         /* 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.
>>      
> But how would a vmexit be triggered (thus calling vmx_handle_exit() )?
>    

vmx_handle_exit() will be called in any case.

> I personally prefer if we can start emulation before attempting guest
> entry, but how can we tell vmx_vcpu_run() to return to userspace if it
> doesn't return a value, I don't feel that changing it to return a
> value would be a wise thing to do too, no?
>    

vmx_vcpu_run() can simply return, setting an internal vmx flag.  Then 
vmx_handle_exit() can notice the flag, emulate, and handle the result of 
this emulation.

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