> On 23 Aug 2019, at 16:21, Liran Alon <liran.alon@xxxxxxxxxx> wrote:
>
>
>
>> On 23 Aug 2019, at 4:07, Sean Christopherson <sean.j.christopherson@xxxxxxxxx> 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@xxxxxxxxx>
>
> 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@xxxxxxxxxx>
>
> -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
>>
>
