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
>>
>
next prev parent reply other threads:[~2019-08-23 13:33 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-23 1:06 [RESEND PATCH 00/13] KVM: x86: Remove emulation_result enums Sean Christopherson
2019-08-23 1:06 ` [RESEND PATCH 01/13] KVM: x86: Relocate MMIO exit stats counting Sean Christopherson
2019-08-23 9:15 ` Vitaly Kuznetsov
2019-08-23 14:37 ` Sean Christopherson
2019-08-23 1:06 ` [RESEND PATCH 02/13] KVM: x86: Clean up handle_emulation_failure() Sean Christopherson
2019-08-23 9:23 ` Vitaly Kuznetsov
2019-08-23 12:58 ` Liran Alon
2019-08-23 1:06 ` [RESEND PATCH 03/13] KVM: x86: Refactor kvm_vcpu_do_singlestep() to remove out param Sean Christopherson
2019-08-23 9:32 ` Vitaly Kuznetsov
2019-08-23 13:05 ` Liran Alon
2019-08-23 1:07 ` [RESEND PATCH 04/13] KVM: x86: Drop EMULTYPE_NO_UD_ON_FAIL as a standalone type Sean Christopherson
2019-08-23 9:34 ` Vitaly Kuznetsov
2019-08-23 13:21 ` Liran Alon
2019-08-23 13:32 ` Liran Alon [this message]
2019-08-23 21:55 ` Sean Christopherson
2019-08-23 1:07 ` [RESEND PATCH 05/13] KVM: x86: Don't attempt VMWare emulation on #GP with non-zero error code Sean Christopherson
2019-08-23 11:51 ` Vitaly Kuznetsov
2019-08-23 13:23 ` Liran Alon
2019-08-23 1:07 ` [RESEND PATCH 06/13] KVM: x86: Move #GP injection for VMware into x86_emulate_instruction() Sean Christopherson
2019-08-23 12:27 ` Vitaly Kuznetsov
2019-08-23 13:30 ` Liran Alon
2019-08-23 1:07 ` [RESEND PATCH 07/13] KVM: x86: Add explicit flag for forced emulation on #UD Sean Christopherson
2019-08-23 13:47 ` Liran Alon
2019-08-23 14:44 ` Sean Christopherson
2019-08-23 15:31 ` Liran Alon
2019-08-23 1:07 ` [RESEND PATCH 08/13] KVM: x86: Move #UD injection for failed emulation into emulation code Sean Christopherson
2019-08-23 13:48 ` Liran Alon
2019-08-27 20:22 ` Sean Christopherson
2019-08-23 1:07 ` [RESEND PATCH 09/13] KVM: x86: Exit to userspace on emulation skip failure Sean Christopherson
2019-08-23 1:07 ` [RESEND PATCH 10/13] KVM: x86: Handle emulation failure directly in kvm_task_switch() Sean Christopherson
2019-08-23 1:07 ` [RESEND PATCH 11/13] KVM: x86: Move triple fault request into RM int injection Sean Christopherson
2019-08-23 1:07 ` [RESEND PATCH 12/13] KVM: VMX: Remove EMULATE_FAIL handling in handle_invalid_guest_state() Sean Christopherson
2019-08-23 1:07 ` [RESEND PATCH 13/13] KVM: x86: Remove emulation_result enums, EMULATE_{DONE,FAIL,USER_EXIT} Sean Christopherson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CB2B4523-AA0C-4931-BD1E-9F63FA114EF0@oracle.com \
--to=liran.alon@oracle.com \
--cc=jmattson@google.com \
--cc=joro@8bytes.org \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pbonzini@redhat.com \
--cc=rkrcmar@redhat.com \
--cc=sean.j.christopherson@intel.com \
--cc=vkuznets@redhat.com \
--cc=wanpengli@tencent.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).