[PATCHv3] VMX: Enhance invalid guest state emulation

[PATCHv3] 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 |   44 ++++++++++++++++++++------------------------
 1 files changed, 20 insertions(+), 24 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 78101dd..6265098 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -107,7 +107,6 @@ struct vcpu_vmx {
 	} rmode;
 	int vpid;
 	bool emulation_required;
-	enum emulation_result invalid_state_emulation_result;
 
 	/* Support for vnmi-less CPUs */
 	int soft_vnmi_blocked;
@@ -3318,35 +3317,37 @@ 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;
-
-	local_irq_enable();
-	preempt_enable();
+	int ret = 1;
 
 	while (!guest_state_valid(vcpu)) {
 		err = emulate_instruction(vcpu, 0, 0, 0);
 
-		if (err == EMULATE_DO_MMIO)
-			break;
+		if (err == EMULATE_DO_MMIO) {
+			ret = 0;
+			goto out;
+		}
 
 		if (err != EMULATE_DONE) {
 			kvm_report_emulation_failure(vcpu, "emulation failure");
-			break;
+			vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
+			vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
+			ret = 0;
+			goto out;
 		}
 
 		if (signal_pending(current))
-			break;
+			goto out;
 		if (need_resched())
 			schedule();
 	}
 
-	preempt_disable();
-	local_irq_disable();
-
-	vmx->invalid_state_emulation_result = err;
+	vmx->emulation_required = 0;
+out:
+	return ret;
 }
 
 /*
@@ -3402,13 +3403,9 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu)
 
 	trace_kvm_exit(exit_reason, kvm_rip_read(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))
-			vmx->emulation_required = 0;
-		return vmx->invalid_state_emulation_result != EMULATE_DO_MMIO;
-	}
+	/* If guest state is invalid, start emulating */
+	if (vmx->emulation_required && emulate_invalid_guest_state)
+		return handle_invalid_guest_state(vcpu);
 
 	/* Access CR3 don't cause VMExit in paging mode, so we need
 	 * to sync with guest real CR3. */
@@ -3603,11 +3600,10 @@ 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);
+	/* Don't enter VMX if guest state is invalid, let the exit handler
+	   start emulation until we arrive back to a valid state */
+	if (vmx->emulation_required && emulate_invalid_guest_state)
 		return;
-	}
 
 	if (test_bit(VCPU_REGS_RSP, (unsigned long *)&vcpu->arch.regs_dirty))
 		vmcs_writel(GUEST_RSP, vcpu->arch.regs[VCPU_REGS_RSP]);
-- 
1.6.0.4

Re: [PATCHv3] VMX: Enhance invalid guest state emulation

[Marcelo Tosatti]

On Tue, Sep 01, 2009 at 12:48:18PM +0200, 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>

Mohammed,

The handle_invalid_guest_state loop is potentially problematic. It would
be more appropriate to use the __vcpu_run loop.

Can't you set vmx->emulation_required depending on the result
of one call to emulate_instruction and get rid of the while
(!guest_state_valid(vcpu)) loop?

> ---
>  arch/x86/kvm/vmx.c |   44 ++++++++++++++++++++------------------------
>  1 files changed, 20 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 78101dd..6265098 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -107,7 +107,6 @@ struct vcpu_vmx {
>  	} rmode;
>  	int vpid;
>  	bool emulation_required;
> -	enum emulation_result invalid_state_emulation_result;
>  
>  	/* Support for vnmi-less CPUs */
>  	int soft_vnmi_blocked;
> @@ -3318,35 +3317,37 @@ 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;
> -
> -	local_irq_enable();
> -	preempt_enable();
> +	int ret = 1;
>  
>  	while (!guest_state_valid(vcpu)) {
>  		err = emulate_instruction(vcpu, 0, 0, 0);
>  
> -		if (err == EMULATE_DO_MMIO)
> -			break;
> +		if (err == EMULATE_DO_MMIO) {
> +			ret = 0;
> +			goto out;
> +		}
>  
>  		if (err != EMULATE_DONE) {
>  			kvm_report_emulation_failure(vcpu, "emulation failure");
> -			break;
> +			vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> +			vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
> +			ret = 0;
> +			goto out;
>  		}
>  
>  		if (signal_pending(current))
> -			break;
> +			goto out;
>  		if (need_resched())
>  			schedule();
>  	}
>  
> -	preempt_disable();
> -	local_irq_disable();
> -
> -	vmx->invalid_state_emulation_result = err;
> +	vmx->emulation_required = 0;
> +out:
> +	return ret;
>  }
>  
>  /*
> @@ -3402,13 +3403,9 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu)
>  
>  	trace_kvm_exit(exit_reason, kvm_rip_read(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))
> -			vmx->emulation_required = 0;
> -		return vmx->invalid_state_emulation_result != EMULATE_DO_MMIO;
> -	}
> +	/* If guest state is invalid, start emulating */
> +	if (vmx->emulation_required && emulate_invalid_guest_state)
> +		return handle_invalid_guest_state(vcpu);
>  
>  	/* Access CR3 don't cause VMExit in paging mode, so we need
>  	 * to sync with guest real CR3. */
> @@ -3603,11 +3600,10 @@ 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);
> +	/* Don't enter VMX if guest state is invalid, let the exit handler
> +	   start emulation until we arrive back to a valid state */
> +	if (vmx->emulation_required && emulate_invalid_guest_state)
>  		return;
> -	}
>  
>  	if (test_bit(VCPU_REGS_RSP, (unsigned long *)&vcpu->arch.regs_dirty))
>  		vmcs_writel(GUEST_RSP, vcpu->arch.regs[VCPU_REGS_RSP]);
> -- 
> 1.6.0.4

Re: [PATCHv3] VMX: Enhance invalid guest state emulation

[Mohammed Gamal]

On Tue, Sep 1, 2009 at 1:48 PM, Marcelo Tosatti<mtosatti@redhat.com> wrote:
> On Tue, Sep 01, 2009 at 12:48:18PM +0200, 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>
>
> Mohammed,
>
> The handle_invalid_guest_state loop is potentially problematic. It would
> be more appropriate to use the __vcpu_run loop.
>
> Can't you set vmx->emulation_required depending on the result
> of one call to emulate_instruction and get rid of the while
> (!guest_state_valid(vcpu)) loop?
>

Invalid state emulation is VMX-specfic, while the __vcpu_run loop is
independent of the virtualization extension (defined in x86.c), no?
AMD SVM can comforably run hosts in big-real mode and thus it doesn't
have the notion of a guest going to an invalid state because of mode
switching, so I don't think it'd be a good idea to move emulation into
a generic layer. Please correct me if I am wrong

Re: [PATCHv3] VMX: Enhance invalid guest state emulation

[Marcelo Tosatti]

On Tue, Sep 01, 2009 at 02:14:17PM +0200, Mohammed Gamal wrote:
> On Tue, Sep 1, 2009 at 1:48 PM, Marcelo Tosatti<mtosatti@redhat.com> wrote:
> > On Tue, Sep 01, 2009 at 12:48:18PM +0200, 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>
> >
> > Mohammed,
> >
> > The handle_invalid_guest_state loop is potentially problematic. It would
> > be more appropriate to use the __vcpu_run loop.
> >
> > Can't you set vmx->emulation_required depending on the result
> > of one call to emulate_instruction and get rid of the while
> > (!guest_state_valid(vcpu)) loop?
> >
> 
> Invalid state emulation is VMX-specfic, while the __vcpu_run loop is
> independent of the virtualization extension (defined in x86.c), no?
> AMD SVM can comforably run hosts in big-real mode and thus it doesn't
> have the notion of a guest going to an invalid state because of mode
> switching, so I don't think it'd be a good idea to move emulation into
> a generic layer. Please correct me if I am wrong

Right. But all i am asking is to emulate one instruction at a
time in handle_invalid_guest_state, instead of looping until
guest_state_valid(vcpu).

So you get rid of schedule(), the check for signal_pending, etc.

Re: [PATCHv3] VMX: Enhance invalid guest state emulation

[Mohammed Gamal]

On Tue, Sep 1, 2009 at 2:18 PM, Marcelo Tosatti<mtosatti@redhat.com> wrote:
> On Tue, Sep 01, 2009 at 02:14:17PM +0200, Mohammed Gamal wrote:
>> On Tue, Sep 1, 2009 at 1:48 PM, Marcelo Tosatti<mtosatti@redhat.com> wrote:
>> > On Tue, Sep 01, 2009 at 12:48:18PM +0200, 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>
>> >
>> > Mohammed,
>> >
>> > The handle_invalid_guest_state loop is potentially problematic. It would
>> > be more appropriate to use the __vcpu_run loop.
>> >
>> > Can't you set vmx->emulation_required depending on the result
>> > of one call to emulate_instruction and get rid of the while
>> > (!guest_state_valid(vcpu)) loop?
>> >
>>
>> Invalid state emulation is VMX-specfic, while the __vcpu_run loop is
>> independent of the virtualization extension (defined in x86.c), no?
>> AMD SVM can comforably run hosts in big-real mode and thus it doesn't
>> have the notion of a guest going to an invalid state because of mode
>> switching, so I don't think it'd be a good idea to move emulation into
>> a generic layer. Please correct me if I am wrong
>
> Right. But all i am asking is to emulate one instruction at a
> time in handle_invalid_guest_state, instead of looping until
> guest_state_valid(vcpu).
>
> So you get rid of schedule(), the check for signal_pending, etc.

But we'll still need to enter the guest when it's in a valid state, so
we need to move that loop somewhere, and now that we still have a loop
we'll also still need to do the pending signals and scheduling checks,
no?

I'd appreciate any suggestions you have to alleviate this.
>
>

Re: [PATCHv3] VMX: Enhance invalid guest state emulation

[Marcelo Tosatti]

On Tue, Sep 01, 2009 at 03:08:55PM +0200, Mohammed Gamal wrote:
> On Tue, Sep 1, 2009 at 2:18 PM, Marcelo Tosatti<mtosatti@redhat.com> wrote:
> > On Tue, Sep 01, 2009 at 02:14:17PM +0200, Mohammed Gamal wrote:
> >> On Tue, Sep 1, 2009 at 1:48 PM, Marcelo Tosatti<mtosatti@redhat.com> wrote:
> >> > On Tue, Sep 01, 2009 at 12:48:18PM +0200, 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>
> >> >
> >> > Mohammed,
> >> >
> >> > The handle_invalid_guest_state loop is potentially problematic. It would
> >> > be more appropriate to use the __vcpu_run loop.
> >> >
> >> > Can't you set vmx->emulation_required depending on the result
> >> > of one call to emulate_instruction and get rid of the while
> >> > (!guest_state_valid(vcpu)) loop?
> >> >
> >>
> >> Invalid state emulation is VMX-specfic, while the __vcpu_run loop is
> >> independent of the virtualization extension (defined in x86.c), no?
> >> AMD SVM can comforably run hosts in big-real mode and thus it doesn't
> >> have the notion of a guest going to an invalid state because of mode
> >> switching, so I don't think it'd be a good idea to move emulation into
> >> a generic layer. Please correct me if I am wrong
> >
> > Right. But all i am asking is to emulate one instruction at a
> > time in handle_invalid_guest_state, instead of looping until
> > guest_state_valid(vcpu).
> >
> > So you get rid of schedule(), the check for signal_pending, etc.
> 
> But we'll still need to enter the guest when it's in a valid state, so
> we need to move that loop somewhere, 

Sure, just set vmx->emulation_required = guest_state_valid(vcpu). When
the state is good, the entry handler will vmentry.

> and now that we still have a loop
> we'll also still need to do the pending signals and scheduling checks,
> no?

Point is you can use the __vcpu_run loop.

In the latest patch you do:

+       /* Don't enter VMX if guest state is invalid, let the exit handler
+          start emulation until we arrive back to a valid state */
+       if (vmx->emulation_required && emulate_invalid_guest_state)
               	return;

And then emulate in the exit handler.

> I'd appreciate any suggestions you have to alleviate this.

I fail to see why you need an internal loop if you can use the external
(__vcpu_run) one.

Re: [PATCHv3] VMX: Enhance invalid guest state emulation

[Mohammed Gamal]

On Tue, Sep 1, 2009 at 3:29 PM, Marcelo Tosatti<mtosatti@redhat.com> wrote:
> On Tue, Sep 01, 2009 at 03:08:55PM +0200, Mohammed Gamal wrote:
>> On Tue, Sep 1, 2009 at 2:18 PM, Marcelo Tosatti<mtosatti@redhat.com> wrote:
>> > On Tue, Sep 01, 2009 at 02:14:17PM +0200, Mohammed Gamal wrote:
>> >> On Tue, Sep 1, 2009 at 1:48 PM, Marcelo Tosatti<mtosatti@redhat.com> wrote:
>> >> > On Tue, Sep 01, 2009 at 12:48:18PM +0200, 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>
>> >> >
>> >> > Mohammed,
>> >> >
>> >> > The handle_invalid_guest_state loop is potentially problematic. It would
>> >> > be more appropriate to use the __vcpu_run loop.
>> >> >
>> >> > Can't you set vmx->emulation_required depending on the result
>> >> > of one call to emulate_instruction and get rid of the while
>> >> > (!guest_state_valid(vcpu)) loop?
>> >> >
>> >>
>> >> Invalid state emulation is VMX-specfic, while the __vcpu_run loop is
>> >> independent of the virtualization extension (defined in x86.c), no?
>> >> AMD SVM can comforably run hosts in big-real mode and thus it doesn't
>> >> have the notion of a guest going to an invalid state because of mode
>> >> switching, so I don't think it'd be a good idea to move emulation into
>> >> a generic layer. Please correct me if I am wrong
>> >
>> > Right. But all i am asking is to emulate one instruction at a
>> > time in handle_invalid_guest_state, instead of looping until
>> > guest_state_valid(vcpu).
>> >
>> > So you get rid of schedule(), the check for signal_pending, etc.
>>
>> But we'll still need to enter the guest when it's in a valid state, so
>> we need to move that loop somewhere,
>
> Sure, just set vmx->emulation_required = guest_state_valid(vcpu). When
> the state is good, the entry handler will vmentry.
>
>> and now that we still have a loop
>> we'll also still need to do the pending signals and scheduling checks,
>> no?
>
> Point is you can use the __vcpu_run loop.
>
> In the latest patch you do:
>
> +       /* Don't enter VMX if guest state is invalid, let the exit handler
> +          start emulation until we arrive back to a valid state */
> +       if (vmx->emulation_required && emulate_invalid_guest_state)
>                return;
>
> And then emulate in the exit handler.
>
>> I'd appreciate any suggestions you have to alleviate this.
>
> I fail to see why you need an internal loop if you can use the external
> (__vcpu_run) one.

Because it's not just used by VMX. So I don't think it'd be wise to
use it for something that's VMX-specific.

Re: [PATCHv3] VMX: Enhance invalid guest state emulation

[Marcelo Tosatti]

On Tue, Sep 01, 2009 at 03:32:29PM +0200, Mohammed Gamal wrote:
> > +       if (vmx->emulation_required && emulate_invalid_guest_state)
> >                return;
> >
> > And then emulate in the exit handler.
> >
> >> I'd appreciate any suggestions you have to alleviate this.
> >
> > I fail to see why you need an internal loop if you can use the external
> > (__vcpu_run) one.
> 
> Because it's not just used by VMX. So I don't think it'd be wise to
> use it for something that's VMX-specific.

OK, it can be done incrementally. This is already an improvement.
Applied, thanks.

Re: [PATCHv3] VMX: Enhance invalid guest state emulation

[Avi Kivity]

On 09/01/2009 04:32 PM, Mohammed Gamal wrote:
>>> I'd appreciate any suggestions you have to alleviate this.
>>>        
>> I fail to see why you need an internal loop if you can use the external
>> (__vcpu_run) one.
>>      
> Because it's not just used by VMX. So I don't think it'd be wise to
> use it for something that's VMX-specific.
>    

The loop is there anyway.  The only question is whether 
vmx_handle_exit() emulates on instruction or many.

Emulating one instruction is slower, but will get interrupt injection 
more accurate (once we have emulated real mode interrupt injection).

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