Re: [RESEND PATCH 04/13] KVM: x86: Drop EMULTYPE_NO_UD_ON_FAIL as a standalone type

From: Liran Alon <liran.alon@oracle.com>
To: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: "Paolo Bonzini" <pbonzini@redhat.com>,
	"Radim Krčmář" <rkrcmar@redhat.com>,
	"Vitaly Kuznetsov" <vkuznets@redhat.com>,
	"Wanpeng Li" <wanpengli@tencent.com>,
	"Jim Mattson" <jmattson@google.com>,
	"Joerg Roedel" <joro@8bytes.org>,
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RESEND PATCH 04/13] KVM: x86: Drop EMULTYPE_NO_UD_ON_FAIL as a standalone type
Date: Fri, 23 Aug 2019 16:32:05 +0300	[thread overview]
Message-ID: <CB2B4523-AA0C-4931-BD1E-9F63FA114EF0@oracle.com> (raw)
In-Reply-To: <4993FDBF-6641-43E9-BCEE-7F5FE58561E9@oracle.com>

> 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
>> 
> 