[RESEND,02/13] KVM: x86: Clean up handle_emulation_failure()
diff mbox series

Message ID 20190823010709.24879-3-sean.j.christopherson@intel.com
State New
Headers show
Series
  • KVM: x86: Remove emulation_result enums
Related show

Commit Message

Sean Christopherson Aug. 23, 2019, 1:06 a.m. UTC
When handling emulation failure, return the emulation result directly
instead of capturing it in a local variable.  Future patches will move
additional cases into handle_emulation_failure(), clean up the cruft
before so there isn't an ugly mix of setting a local variable and
returning directly.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/x86.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

Comments

Vitaly Kuznetsov Aug. 23, 2019, 9:23 a.m. UTC | #1
Sean Christopherson <sean.j.christopherson@intel.com> writes:

> When handling emulation failure, return the emulation result directly
> instead of capturing it in a local variable.  Future patches will move
> additional cases into handle_emulation_failure(), clean up the cruft
> before so there isn't an ugly mix of setting a local variable and
> returning directly.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/x86.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index cd425f54096a..c6de5bc4fa5e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6207,24 +6207,22 @@ EXPORT_SYMBOL_GPL(kvm_inject_realmode_interrupt);
>  
>  static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
>  {
> -	int r = EMULATE_DONE;
> -
>  	++vcpu->stat.insn_emulation_fail;
>  	trace_kvm_emulate_insn_failed(vcpu);
>  
>  	if (emulation_type & EMULTYPE_NO_UD_ON_FAIL)
>  		return EMULATE_FAIL;
>  
> +	kvm_queue_exception(vcpu, UD_VECTOR);
> +
>  	if (!is_guest_mode(vcpu) && kvm_x86_ops->get_cpl(vcpu) == 0) {
>  		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>  		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
>  		vcpu->run->internal.ndata = 0;
> -		r = EMULATE_USER_EXIT;
> +		return EMULATE_USER_EXIT;
>  	}
>  
> -	kvm_queue_exception(vcpu, UD_VECTOR);
> -
> -	return r;
> +	return EMULATE_DONE;
>  }
>  
>  static bool reexecute_instruction(struct kvm_vcpu *vcpu, gva_t cr2,

No functional change,

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Just for self-education, what sane userspace is supposed to do when it
sees KVM_EXIT_INTERNAL_ERROR other than kill the guest? Why does it make
sense to still prepare to inject '#UD'?
Liran Alon Aug. 23, 2019, 12:58 p.m. UTC | #2
> On 23 Aug 2019, at 12:23, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> 
> Sean Christopherson <sean.j.christopherson@intel.com> writes:
> 
>> When handling emulation failure, return the emulation result directly
>> instead of capturing it in a local variable.  Future patches will move
>> additional cases into handle_emulation_failure(), clean up the cruft
>> before so there isn't an ugly mix of setting a local variable and
>> returning directly.
>> 
>> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
>> ---
>> arch/x86/kvm/x86.c | 10 ++++------
>> 1 file changed, 4 insertions(+), 6 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index cd425f54096a..c6de5bc4fa5e 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -6207,24 +6207,22 @@ EXPORT_SYMBOL_GPL(kvm_inject_realmode_interrupt);
>> 
>> static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
>> {
>> -	int r = EMULATE_DONE;
>> -
>> 	++vcpu->stat.insn_emulation_fail;
>> 	trace_kvm_emulate_insn_failed(vcpu);
>> 
>> 	if (emulation_type & EMULTYPE_NO_UD_ON_FAIL)
>> 		return EMULATE_FAIL;
>> 
>> +	kvm_queue_exception(vcpu, UD_VECTOR);
>> +
>> 	if (!is_guest_mode(vcpu) && kvm_x86_ops->get_cpl(vcpu) == 0) {
>> 		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>> 		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
>> 		vcpu->run->internal.ndata = 0;
>> -		r = EMULATE_USER_EXIT;
>> +		return EMULATE_USER_EXIT;
>> 	}
>> 
>> -	kvm_queue_exception(vcpu, UD_VECTOR);
>> -
>> -	return r;
>> +	return EMULATE_DONE;
>> }
>> 
>> static bool reexecute_instruction(struct kvm_vcpu *vcpu, gva_t cr2,
> 
> No functional change,
> 
> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> 
> Just for self-education, what sane userspace is supposed to do when it
> sees KVM_EXIT_INTERNAL_ERROR other than kill the guest? Why does it make
> sense to still prepare to inject '#UD’
> 
> -- 
> Vitaly

The commit which introduced this behaviour seems to be
6d77dbfc88e3 ("KVM: inject #UD if instruction emulation fails and exit to userspace")

I actually agree with Vitaly. It made more sense that the ABI would be that
on internal emulation failure, we just return to userspace and allow it to handle
the scenario however it likes. If it wishes to queue #UD on vCPU and resume
guest in case CPL==3 then it made sense that this logic would only be in userspace.
Thus, there is no need for KVM to queue a #UD from kernel on this scenario...

What’s even weirder is that this ABI was then further broken by 2 later commits:
First, fc3a9157d314 ("KVM: X86: Don't report L2 emulation failures to user-space")
changed behaviour to avoid reporting emulation error in case vCPU in guest-mode.
Then, a2b9e6c1a35a ("KVM: x86: Don't report guest userspace emulation error to userspace")
Changed behaviour similarly to avoid reporting emulation error in case vCPU CPL!=0.
In both cases, only #UD is injected to guest without userspace being aware of it.

Problem is that if we would change this ABI to not queue #UD on emulation error,
we will definitely break userspace VMMs that rely on it when they re-enter into guest
in this scenario and expect #UD to be injected.
Therefore, the only way to change this behaviour is to introduce a new KVM_CAP
that needs to be explicitly enabled from userspace.
But because most likely most userspace VMMs just terminate guest in case
of emulation-failure, it’s probably not worth it and Sean’s commit is good enough.

For the commit itself:
Reviewed-by: Liran Alon <liran.alon@oracle.com>

-Liran

Patch
diff mbox series

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index cd425f54096a..c6de5bc4fa5e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6207,24 +6207,22 @@  EXPORT_SYMBOL_GPL(kvm_inject_realmode_interrupt);
 
 static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
 {
-	int r = EMULATE_DONE;
-
 	++vcpu->stat.insn_emulation_fail;
 	trace_kvm_emulate_insn_failed(vcpu);
 
 	if (emulation_type & EMULTYPE_NO_UD_ON_FAIL)
 		return EMULATE_FAIL;
 
+	kvm_queue_exception(vcpu, UD_VECTOR);
+
 	if (!is_guest_mode(vcpu) && kvm_x86_ops->get_cpl(vcpu) == 0) {
 		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
 		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
 		vcpu->run->internal.ndata = 0;
-		r = EMULATE_USER_EXIT;
+		return EMULATE_USER_EXIT;
 	}
 
-	kvm_queue_exception(vcpu, UD_VECTOR);
-
-	return r;
+	return EMULATE_DONE;
 }
 
 static bool reexecute_instruction(struct kvm_vcpu *vcpu, gva_t cr2,