[RESEND,04/13] KVM: x86: Drop EMULTYPE_NO_UD_ON_FAIL as a standalone type
diff mbox series

Message ID 20190823010709.24879-5-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:07 a.m. UTC
The "no #UD on fail" is used only in the VMWare case, and for the VMWare
scenario it really means "#GP instead of #UD on fail".  Remove the flag
in preparation for moving all fault injection into the emulation flow
itself, which in turn will allow eliminating EMULATE_DONE and company.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/include/asm/kvm_host.h | 1 -
 arch/x86/kvm/svm.c              | 3 +--
 arch/x86/kvm/vmx/vmx.c          | 3 +--
 arch/x86/kvm/x86.c              | 2 +-
 4 files changed, 3 insertions(+), 6 deletions(-)

Comments

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

> The "no #UD on fail" is used only in the VMWare case, and for the VMWare
> scenario it really means "#GP instead of #UD on fail".  Remove the flag
> in preparation for moving all fault injection into the emulation flow
> itself, which in turn will allow eliminating EMULATE_DONE and company.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/include/asm/kvm_host.h | 1 -
>  arch/x86/kvm/svm.c              | 3 +--
>  arch/x86/kvm/vmx/vmx.c          | 3 +--
>  arch/x86/kvm/x86.c              | 2 +-
>  4 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 44a5ce57a905..dd6bd9ed0839 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1318,7 +1318,6 @@ enum emulation_result {
>  #define EMULTYPE_TRAP_UD	    (1 << 1)
>  #define EMULTYPE_SKIP		    (1 << 2)
>  #define EMULTYPE_ALLOW_RETRY	    (1 << 3)
> -#define EMULTYPE_NO_UD_ON_FAIL	    (1 << 4)
>  #define EMULTYPE_VMWARE		    (1 << 5)
>  int kvm_emulate_instruction(struct kvm_vcpu *vcpu, int emulation_type);
>  int kvm_emulate_instruction_from_buffer(struct kvm_vcpu *vcpu,
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 1f220a85514f..5a42f9c70014 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2772,8 +2772,7 @@ static int gp_interception(struct vcpu_svm *svm)
>  
>  	WARN_ON_ONCE(!enable_vmware_backdoor);
>  
> -	er = kvm_emulate_instruction(vcpu,
> -		EMULTYPE_VMWARE | EMULTYPE_NO_UD_ON_FAIL);
> +	er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
>  	if (er == EMULATE_USER_EXIT)
>  		return 0;
>  	else if (er != EMULATE_DONE)
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 18286e5b5983..6ecf773825e2 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4509,8 +4509,7 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
>  
>  	if (!vmx->rmode.vm86_active && is_gp_fault(intr_info)) {
>  		WARN_ON_ONCE(!enable_vmware_backdoor);
> -		er = kvm_emulate_instruction(vcpu,
> -			EMULTYPE_VMWARE | EMULTYPE_NO_UD_ON_FAIL);
> +		er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
>  		if (er == EMULATE_USER_EXIT)
>  			return 0;
>  		else if (er != EMULATE_DONE)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index fe847f8eb947..e0f0e14d8fac 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6210,7 +6210,7 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
>  	++vcpu->stat.insn_emulation_fail;
>  	trace_kvm_emulate_insn_failed(vcpu);
>  
> -	if (emulation_type & EMULTYPE_NO_UD_ON_FAIL)
> +	if (emulation_type & EMULTYPE_VMWARE)
>  		return EMULATE_FAIL;
>  
>  	kvm_queue_exception(vcpu, UD_VECTOR);

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Liran Alon Aug. 23, 2019, 1:21 p.m. UTC | #2
> On 23 Aug 2019, at 4:07, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> 
> The "no #UD on fail" is used only in the VMWare case, and for the VMWare
> scenario it really means "#GP instead of #UD on fail".  Remove the flag
> in preparation for moving all fault injection into the emulation flow
> itself, which in turn will allow eliminating EMULATE_DONE and company.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

When I created the commit which introduced this
e23661712005 ("KVM: x86: Add emulation_type to not raise #UD on emulation failure")
I intentionally introduced a new flag to emulation_type instead of using EMULTYPE_VMWARE
as I thought it’s weird to couple this behaviour specifically with VMware emulation.
As it made sense to me that there could be more scenarios in which some VMExit handler
would like to use the x86 emulator but in case of failure want to decide what would be
the failure handling from the outside. I also didn’t want the x86 emulator to be aware
of VMware interception internals.

Having said that, one could argue that the x86 emulator already knows about the VMware
interception internals because of how x86_emulate_instruction() use is_vmware_backdoor_opcode()
and from the mere existence of EMULTYPE_VMWARE. So I think it’s legit to decide
that we will just move all the VMware interception logic into the x86 emulator. Including
handling emulation failures. But then, I would make this patch of yours to also
modify handle_emulation_failure() to queue #GP to guest directly instead of #GP intercept
in VMX/SVM to do so.
I see you do it in a later patch "KVM: x86: Move #GP injection for VMware into x86_emulate_instruction()"
but I think this should just be squashed with this patch to make sense.

To sum-up, I agree with your approach but I recommend you squash this patch and patch 6 of the series to one
and change commit message to explain that you just move entire handling of VMware interception into
the x86 emulator. Instead of providing explanations such as VMware emulation is the only one that use
“no #UD on fail”.

The diff itself looks fine to me, therefore:
Reviewed-by: Liran Alon <liran.alon@oracle.com>

-Liran


> ---
> arch/x86/include/asm/kvm_host.h | 1 -
> arch/x86/kvm/svm.c              | 3 +--
> arch/x86/kvm/vmx/vmx.c          | 3 +--
> arch/x86/kvm/x86.c              | 2 +-
> 4 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 44a5ce57a905..dd6bd9ed0839 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1318,7 +1318,6 @@ enum emulation_result {
> #define EMULTYPE_TRAP_UD	    (1 << 1)
> #define EMULTYPE_SKIP		    (1 << 2)
> #define EMULTYPE_ALLOW_RETRY	    (1 << 3)
> -#define EMULTYPE_NO_UD_ON_FAIL	    (1 << 4)
> #define EMULTYPE_VMWARE		    (1 << 5)
> int kvm_emulate_instruction(struct kvm_vcpu *vcpu, int emulation_type);
> int kvm_emulate_instruction_from_buffer(struct kvm_vcpu *vcpu,
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 1f220a85514f..5a42f9c70014 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2772,8 +2772,7 @@ static int gp_interception(struct vcpu_svm *svm)
> 
> 	WARN_ON_ONCE(!enable_vmware_backdoor);
> 
> -	er = kvm_emulate_instruction(vcpu,
> -		EMULTYPE_VMWARE | EMULTYPE_NO_UD_ON_FAIL);
> +	er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
> 	if (er == EMULATE_USER_EXIT)
> 		return 0;
> 	else if (er != EMULATE_DONE)
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 18286e5b5983..6ecf773825e2 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4509,8 +4509,7 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
> 
> 	if (!vmx->rmode.vm86_active && is_gp_fault(intr_info)) {
> 		WARN_ON_ONCE(!enable_vmware_backdoor);
> -		er = kvm_emulate_instruction(vcpu,
> -			EMULTYPE_VMWARE | EMULTYPE_NO_UD_ON_FAIL);
> +		er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
> 		if (er == EMULATE_USER_EXIT)
> 			return 0;
> 		else if (er != EMULATE_DONE)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index fe847f8eb947..e0f0e14d8fac 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6210,7 +6210,7 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
> 	++vcpu->stat.insn_emulation_fail;
> 	trace_kvm_emulate_insn_failed(vcpu);
> 
> -	if (emulation_type & EMULTYPE_NO_UD_ON_FAIL)
> +	if (emulation_type & EMULTYPE_VMWARE)
> 		return EMULATE_FAIL;
> 
> 	kvm_queue_exception(vcpu, UD_VECTOR);
> -- 
> 2.22.0
>
Liran Alon Aug. 23, 2019, 1:32 p.m. UTC | #3
> On 23 Aug 2019, at 16:21, Liran Alon <liran.alon@oracle.com> wrote:
> 
> 
> 
>> On 23 Aug 2019, at 4:07, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
>> 
>> The "no #UD on fail" is used only in the VMWare case, and for the VMWare
>> scenario it really means "#GP instead of #UD on fail".  Remove the flag
>> in preparation for moving all fault injection into the emulation flow
>> itself, which in turn will allow eliminating EMULATE_DONE and company.
>> 
>> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> When I created the commit which introduced this
> e23661712005 ("KVM: x86: Add emulation_type to not raise #UD on emulation failure")
> I intentionally introduced a new flag to emulation_type instead of using EMULTYPE_VMWARE
> as I thought it’s weird to couple this behaviour specifically with VMware emulation.
> As it made sense to me that there could be more scenarios in which some VMExit handler
> would like to use the x86 emulator but in case of failure want to decide what would be
> the failure handling from the outside. I also didn’t want the x86 emulator to be aware
> of VMware interception internals.
> 
> Having said that, one could argue that the x86 emulator already knows about the VMware
> interception internals because of how x86_emulate_instruction() use is_vmware_backdoor_opcode()
> and from the mere existence of EMULTYPE_VMWARE. So I think it’s legit to decide
> that we will just move all the VMware interception logic into the x86 emulator. Including
> handling emulation failures. But then, I would make this patch of yours to also
> modify handle_emulation_failure() to queue #GP to guest directly instead of #GP intercept
> in VMX/SVM to do so.
> I see you do it in a later patch "KVM: x86: Move #GP injection for VMware into x86_emulate_instruction()"
> but I think this should just be squashed with this patch to make sense.
> 
> To sum-up, I agree with your approach but I recommend you squash this patch and patch 6 of the series to one
> and change commit message to explain that you just move entire handling of VMware interception into
> the x86 emulator. Instead of providing explanations such as VMware emulation is the only one that use
> “no #UD on fail”.

After reading patch 5 as-well, I would recommend to first apply patch 5 (filter out #GP with error-code != 0)
and only then apply 4+6.

-Liran

> 
> The diff itself looks fine to me, therefore:
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> 
> -Liran
> 
> 
>> ---
>> arch/x86/include/asm/kvm_host.h | 1 -
>> arch/x86/kvm/svm.c              | 3 +--
>> arch/x86/kvm/vmx/vmx.c          | 3 +--
>> arch/x86/kvm/x86.c              | 2 +-
>> 4 files changed, 3 insertions(+), 6 deletions(-)
>> 
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 44a5ce57a905..dd6bd9ed0839 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -1318,7 +1318,6 @@ enum emulation_result {
>> #define EMULTYPE_TRAP_UD	    (1 << 1)
>> #define EMULTYPE_SKIP		    (1 << 2)
>> #define EMULTYPE_ALLOW_RETRY	    (1 << 3)
>> -#define EMULTYPE_NO_UD_ON_FAIL	    (1 << 4)
>> #define EMULTYPE_VMWARE		    (1 << 5)
>> int kvm_emulate_instruction(struct kvm_vcpu *vcpu, int emulation_type);
>> int kvm_emulate_instruction_from_buffer(struct kvm_vcpu *vcpu,
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 1f220a85514f..5a42f9c70014 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -2772,8 +2772,7 @@ static int gp_interception(struct vcpu_svm *svm)
>> 
>> 	WARN_ON_ONCE(!enable_vmware_backdoor);
>> 
>> -	er = kvm_emulate_instruction(vcpu,
>> -		EMULTYPE_VMWARE | EMULTYPE_NO_UD_ON_FAIL);
>> +	er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
>> 	if (er == EMULATE_USER_EXIT)
>> 		return 0;
>> 	else if (er != EMULATE_DONE)
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index 18286e5b5983..6ecf773825e2 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -4509,8 +4509,7 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
>> 
>> 	if (!vmx->rmode.vm86_active && is_gp_fault(intr_info)) {
>> 		WARN_ON_ONCE(!enable_vmware_backdoor);
>> -		er = kvm_emulate_instruction(vcpu,
>> -			EMULTYPE_VMWARE | EMULTYPE_NO_UD_ON_FAIL);
>> +		er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
>> 		if (er == EMULATE_USER_EXIT)
>> 			return 0;
>> 		else if (er != EMULATE_DONE)
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index fe847f8eb947..e0f0e14d8fac 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -6210,7 +6210,7 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
>> 	++vcpu->stat.insn_emulation_fail;
>> 	trace_kvm_emulate_insn_failed(vcpu);
>> 
>> -	if (emulation_type & EMULTYPE_NO_UD_ON_FAIL)
>> +	if (emulation_type & EMULTYPE_VMWARE)
>> 		return EMULATE_FAIL;
>> 
>> 	kvm_queue_exception(vcpu, UD_VECTOR);
>> -- 
>> 2.22.0
>> 
>
Sean Christopherson Aug. 23, 2019, 9:55 p.m. UTC | #4
On Fri, Aug 23, 2019 at 04:32:05PM +0300, Liran Alon wrote:
> 
> > On 23 Aug 2019, at 16:21, Liran Alon <liran.alon@oracle.com> wrote:
> > 
> >> On 23 Aug 2019, at 4:07, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> >> 
> >> The "no #UD on fail" is used only in the VMWare case, and for the VMWare
> >> scenario it really means "#GP instead of #UD on fail".  Remove the flag
> >> in preparation for moving all fault injection into the emulation flow
> >> itself, which in turn will allow eliminating EMULATE_DONE and company.
> >> 
> >> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > When I created the commit which introduced this e23661712005 ("KVM: x86:
> > Add emulation_type to not raise #UD on emulation failure") I intentionally
> > introduced a new flag to emulation_type instead of using EMULTYPE_VMWARE as
> > I thought it’s weird to couple this behaviour specifically with VMware
> > emulation.  As it made sense to me that there could be more scenarios in
> > which some VMExit handler would like to use the x86 emulator but in case of
> > failure want to decide what would be the failure handling from the outside.
> > I also didn’t want the x86 emulator to be aware of VMware interception
> > internals.
> > 
> > Having said that, one could argue that the x86 emulator already knows about
> > the VMware interception internals because of how x86_emulate_instruction()
> > use is_vmware_backdoor_opcode() and from the mere existence of
> > EMULTYPE_VMWARE. So I think it’s legit to decide that we will just move all
> > the VMware interception logic into the x86 emulator. Including handling
> > emulation failures. But then, I would make this patch of yours to also
> > modify handle_emulation_failure() to queue #GP to guest directly instead of
> > #GP intercept in VMX/SVM to do so.  I see you do it in a later patch "KVM:
> > x86: Move #GP injection for VMware into x86_emulate_instruction()" but I
> > think this should just be squashed with this patch to make sense.
> > 
> > To sum-up, I agree with your approach but I recommend you squash this patch
> > and patch 6 of the series to one and change commit message to explain that
> > you just move entire handling of VMware interception into the x86 emulator.
> > Instead of providing explanations such as VMware emulation is the only one
> > that use “no #UD on fail”.
> 
> After reading patch 5 as-well, I would recommend to first apply patch 5
> (filter out #GP with error-code != 0) and only then apply 4+6.

Works for me.

Patch
diff mbox series

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 44a5ce57a905..dd6bd9ed0839 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1318,7 +1318,6 @@  enum emulation_result {
 #define EMULTYPE_TRAP_UD	    (1 << 1)
 #define EMULTYPE_SKIP		    (1 << 2)
 #define EMULTYPE_ALLOW_RETRY	    (1 << 3)
-#define EMULTYPE_NO_UD_ON_FAIL	    (1 << 4)
 #define EMULTYPE_VMWARE		    (1 << 5)
 int kvm_emulate_instruction(struct kvm_vcpu *vcpu, int emulation_type);
 int kvm_emulate_instruction_from_buffer(struct kvm_vcpu *vcpu,
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 1f220a85514f..5a42f9c70014 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2772,8 +2772,7 @@  static int gp_interception(struct vcpu_svm *svm)
 
 	WARN_ON_ONCE(!enable_vmware_backdoor);
 
-	er = kvm_emulate_instruction(vcpu,
-		EMULTYPE_VMWARE | EMULTYPE_NO_UD_ON_FAIL);
+	er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
 	if (er == EMULATE_USER_EXIT)
 		return 0;
 	else if (er != EMULATE_DONE)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 18286e5b5983..6ecf773825e2 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4509,8 +4509,7 @@  static int handle_exception_nmi(struct kvm_vcpu *vcpu)
 
 	if (!vmx->rmode.vm86_active && is_gp_fault(intr_info)) {
 		WARN_ON_ONCE(!enable_vmware_backdoor);
-		er = kvm_emulate_instruction(vcpu,
-			EMULTYPE_VMWARE | EMULTYPE_NO_UD_ON_FAIL);
+		er = kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE);
 		if (er == EMULATE_USER_EXIT)
 			return 0;
 		else if (er != EMULATE_DONE)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index fe847f8eb947..e0f0e14d8fac 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6210,7 +6210,7 @@  static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
 	++vcpu->stat.insn_emulation_fail;
 	trace_kvm_emulate_insn_failed(vcpu);
 
-	if (emulation_type & EMULTYPE_NO_UD_ON_FAIL)
+	if (emulation_type & EMULTYPE_VMWARE)
 		return EMULATE_FAIL;
 
 	kvm_queue_exception(vcpu, UD_VECTOR);