| Message ID | 20190727055214.9282-9-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: KVM: Add SGX virtualization | expand |
On Fri, Jul 26, 2019 at 10:52 PM Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > Similar to the existing AMD #NPF case where emulation of the current > instruction is not possible due to lack of information, virtualization > of Intel SGX will introduce a scenario where emulation is not possible > due to the VMExit occurring in an SGX enclave. And again similar to > the AMD case, emulation can be initiated by kvm_mmu_page_fault(), i.e. > outside of the control of the vendor-specific code. > > While the cause and architecturally visible behavior of the two cases > is different, e.g. Intel SGX will inject a #UD whereas AMD #NPF is a > clean resume or complete shutdown, the impact on the common emulation > code is identical: KVM must stop emulation immediately and resume the > guest. > > Replace the exisiting need_emulation_on_page_fault() with a more generic > is_emulatable() kvm_x86_ops callback, which is called unconditionally > by x86_emulate_instruction(). > Having recently noticed that emulate_ud() is broken when the guest's TF is set, I suppose I should ask: does your new code function sensibly when TF is set? Also, anyone want to fix that emulate_ud() bug? The test case is merged now: # ./tools/testing/selftests/x86/syscall_arg_fault_32 [RUN] SYSENTER with invalid state [OK] Seems okay [RUN] SYSCALL with invalid state [SKIP] Illegal instruction [RUN] SYSENTER with TF and invalid state [OK] Seems okay [RUN] SYSCALL with TF and invalid state [WARN] Got stuck single-stepping -- you probably have a KVM bug
On Sat, Jul 27, 2019 at 10:38:03AM -0700, Andy Lutomirski wrote: > On Fri, Jul 26, 2019 at 10:52 PM Sean Christopherson > <sean.j.christopherson@intel.com> wrote: > > > > Similar to the existing AMD #NPF case where emulation of the current > > instruction is not possible due to lack of information, virtualization > > of Intel SGX will introduce a scenario where emulation is not possible > > due to the VMExit occurring in an SGX enclave. And again similar to > > the AMD case, emulation can be initiated by kvm_mmu_page_fault(), i.e. > > outside of the control of the vendor-specific code. > > > > While the cause and architecturally visible behavior of the two cases > > is different, e.g. Intel SGX will inject a #UD whereas AMD #NPF is a > > clean resume or complete shutdown, the impact on the common emulation > > code is identical: KVM must stop emulation immediately and resume the > > guest. > > > > Replace the exisiting need_emulation_on_page_fault() with a more generic > > is_emulatable() kvm_x86_ops callback, which is called unconditionally > > by x86_emulate_instruction(). > > > > Having recently noticed that emulate_ud() is broken when the guest's > TF is set, I suppose I should ask: does your new code function > sensibly when TF is set? Barring a VMX fault injection interaction I'm not thinking of, yes. The SGX reaction to the #UD VM-Exit is to inject a #UD and resume the guest, pending breakpoints shouldn't be affected in any way (unless some other part of KVM mucks with them, e.g. when guest single-stepping is enabled).
On Fri, Jul 26, 2019 at 10:52:01PM -0700, Sean Christopherson wrote: > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index 48c865a4e5dd..0fb8b60eb136 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -7115,10 +7115,25 @@ static int nested_enable_evmcs(struct kvm_vcpu *vcpu, > return -ENODEV; > } > > -static bool svm_need_emulation_on_page_fault(struct kvm_vcpu *vcpu) > +static bool svm_is_emulatable(struct kvm_vcpu *vcpu, void *insn, int insn_len) > { > bool is_user, smap; > > + if (likely(!insn || insn_len)) > + return true; > + > + /* > + * Under certain conditions insn_len may be zero on #NPF. This can > + * happen if a guest gets a page-fault on data access but the HW table > + * walker is not able to read the instruction page (e.g instruction > + * page is not present in memory). In those cases we simply restart the > + * guest, with the exception of AMD Erratum 1096 which is unrecoverable. > + */ > + if (unlikely(insn && !insn_len)) { > + if (!kvm_x86_ops->need_emulation_on_page_fault(vcpu)) > + return 1; > + } Doh, obviously forgot to compile for SVM when rebasing.
>> On Jul 29, 2019, at 7:49 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote: >> >> On Sat, Jul 27, 2019 at 10:38:03AM -0700, Andy Lutomirski wrote: >> On Fri, Jul 26, 2019 at 10:52 PM Sean Christopherson >> <sean.j.christopherson@intel.com> wrote: >>> >>> Similar to the existing AMD #NPF case where emulation of the current >>> instruction is not possible due to lack of information, virtualization >>> of Intel SGX will introduce a scenario where emulation is not possible >>> due to the VMExit occurring in an SGX enclave. And again similar to >>> the AMD case, emulation can be initiated by kvm_mmu_page_fault(), i.e. >>> outside of the control of the vendor-specific code. >>> >>> While the cause and architecturally visible behavior of the two cases >>> is different, e.g. Intel SGX will inject a #UD whereas AMD #NPF is a >>> clean resume or complete shutdown, the impact on the common emulation >>> code is identical: KVM must stop emulation immediately and resume the >>> guest. >>> >>> Replace the exisiting need_emulation_on_page_fault() with a more generic >>> is_emulatable() kvm_x86_ops callback, which is called unconditionally >>> by x86_emulate_instruction(). >> >> Having recently noticed that emulate_ud() is broken when the guest's >> TF is set, I suppose I should ask: does your new code function >> sensibly when TF is set? > > Barring a VMX fault injection interaction I'm not thinking of, yes. The > SGX reaction to the #UD VM-Exit is to inject a #UD and resume the guest, > pending breakpoints shouldn't be affected in any way (unless some other > part of KVM mucks with them, e.g. when guest single-stepping is enabled). What I mean is: does the code actually do what you think it does if TF is set? Right now, as I understand it, the KVM emulation code has a bug in which some emulated faults also inject #DB despite the fact that the instruction faulted, and the #DB seems to take precedence over the original fault. This confuses the guest.
On Thu, Aug 15, 2019 at 05:47:12PM -0700, Andy Lutomirski wrote: > > > >> On Jul 29, 2019, at 7:49 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > >> > >> On Sat, Jul 27, 2019 at 10:38:03AM -0700, Andy Lutomirski wrote: > >> On Fri, Jul 26, 2019 at 10:52 PM Sean Christopherson > >> <sean.j.christopherson@intel.com> wrote: > >>> > >>> Similar to the existing AMD #NPF case where emulation of the current > >>> instruction is not possible due to lack of information, virtualization > >>> of Intel SGX will introduce a scenario where emulation is not possible > >>> due to the VMExit occurring in an SGX enclave. And again similar to > >>> the AMD case, emulation can be initiated by kvm_mmu_page_fault(), i.e. > >>> outside of the control of the vendor-specific code. > >>> > >>> While the cause and architecturally visible behavior of the two cases > >>> is different, e.g. Intel SGX will inject a #UD whereas AMD #NPF is a > >>> clean resume or complete shutdown, the impact on the common emulation > >>> code is identical: KVM must stop emulation immediately and resume the > >>> guest. > >>> > >>> Replace the exisiting need_emulation_on_page_fault() with a more generic > >>> is_emulatable() kvm_x86_ops callback, which is called unconditionally > >>> by x86_emulate_instruction(). > >> > >> Having recently noticed that emulate_ud() is broken when the guest's > >> TF is set, I suppose I should ask: does your new code function > >> sensibly when TF is set? > > > > Barring a VMX fault injection interaction I'm not thinking of, yes. The > > SGX reaction to the #UD VM-Exit is to inject a #UD and resume the guest, > > pending breakpoints shouldn't be affected in any way (unless some other > > part of KVM mucks with them, e.g. when guest single-stepping is enabled). > > What I mean is: does the code actually do what you think it does if TF is > set? Right now, as I understand it, the KVM emulation code has a bug in > which some emulated faults also inject #DB despite the fact that the > instruction faulted, and the #DB seems to take precedence over the original > fault. This confuses the guest. Yes. The proposed change is to inject the #UD instead of calling into the emulator, and by inspection I've verified that all code that injects a #DB is either contained within the emulator or is mutually exclusive with an intercepted #UD. It's a qualified yes because I don't have an actual testcase to verify my literacy. I'll look into adding a test, either to the selftest/x86/sgx or to kvm-unit-tests.
On Mon, Aug 19, 2019 at 3:01 PM Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Thu, Aug 15, 2019 at 05:47:12PM -0700, Andy Lutomirski wrote: > > > > > > >> On Jul 29, 2019, at 7:49 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > >> > > >> On Sat, Jul 27, 2019 at 10:38:03AM -0700, Andy Lutomirski wrote: > > >> On Fri, Jul 26, 2019 at 10:52 PM Sean Christopherson > > >> <sean.j.christopherson@intel.com> wrote: > > >>> > > >>> Similar to the existing AMD #NPF case where emulation of the current > > >>> instruction is not possible due to lack of information, virtualization > > >>> of Intel SGX will introduce a scenario where emulation is not possible > > >>> due to the VMExit occurring in an SGX enclave. And again similar to > > >>> the AMD case, emulation can be initiated by kvm_mmu_page_fault(), i.e. > > >>> outside of the control of the vendor-specific code. > > >>> > > >>> While the cause and architecturally visible behavior of the two cases > > >>> is different, e.g. Intel SGX will inject a #UD whereas AMD #NPF is a > > >>> clean resume or complete shutdown, the impact on the common emulation > > >>> code is identical: KVM must stop emulation immediately and resume the > > >>> guest. > > >>> > > >>> Replace the exisiting need_emulation_on_page_fault() with a more generic > > >>> is_emulatable() kvm_x86_ops callback, which is called unconditionally > > >>> by x86_emulate_instruction(). > > >> > > >> Having recently noticed that emulate_ud() is broken when the guest's > > >> TF is set, I suppose I should ask: does your new code function > > >> sensibly when TF is set? > > > > > > Barring a VMX fault injection interaction I'm not thinking of, yes. The > > > SGX reaction to the #UD VM-Exit is to inject a #UD and resume the guest, > > > pending breakpoints shouldn't be affected in any way (unless some other > > > part of KVM mucks with them, e.g. when guest single-stepping is enabled). > > > > What I mean is: does the code actually do what you think it does if TF is > > set? Right now, as I understand it, the KVM emulation code has a bug in > > which some emulated faults also inject #DB despite the fact that the > > instruction faulted, and the #DB seems to take precedence over the original > > fault. This confuses the guest. > > Yes. The proposed change is to inject the #UD instead of calling into the > emulator, and by inspection I've verified that all code that injects a #DB > is either contained within the emulator or is mutually exclusive with an > intercepted #UD. It's a qualified yes because I don't have an actual > testcase to verify my literacy. I'll look into adding a test, either to > the selftest/x86/sgx or to kvm-unit-tests. I wrote one, and it fails: # ./tools/testing/selftests/x86/syscall_arg_fault_32 [RUN] SYSENTER with invalid state [OK] Seems okay [RUN] SYSCALL with invalid state [SKIP] Illegal instruction [RUN] SYSENTER with TF and invalid state [OK] Seems okay [RUN] SYSCALL with TF and invalid state [WARN] Got stuck single-stepping -- you probably have a KVM bug emulate_ud() is buggy.
On Mon, Aug 19, 2019 at 06:34:07PM -0700, Andy Lutomirski wrote: > On Mon, Aug 19, 2019 at 3:01 PM Sean Christopherson > <sean.j.christopherson@intel.com> wrote: > > > > On Thu, Aug 15, 2019 at 05:47:12PM -0700, Andy Lutomirski wrote: > > > > > > > > > >> On Jul 29, 2019, at 7:49 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > > >> > > > >> On Sat, Jul 27, 2019 at 10:38:03AM -0700, Andy Lutomirski wrote: > > > >> On Fri, Jul 26, 2019 at 10:52 PM Sean Christopherson > > > >> <sean.j.christopherson@intel.com> wrote: > > > >>> > > > >>> Similar to the existing AMD #NPF case where emulation of the current > > > >>> instruction is not possible due to lack of information, virtualization > > > >>> of Intel SGX will introduce a scenario where emulation is not possible > > > >>> due to the VMExit occurring in an SGX enclave. And again similar to > > > >>> the AMD case, emulation can be initiated by kvm_mmu_page_fault(), i.e. > > > >>> outside of the control of the vendor-specific code. > > > >>> > > > >>> While the cause and architecturally visible behavior of the two cases > > > >>> is different, e.g. Intel SGX will inject a #UD whereas AMD #NPF is a > > > >>> clean resume or complete shutdown, the impact on the common emulation > > > >>> code is identical: KVM must stop emulation immediately and resume the > > > >>> guest. > > > >>> > > > >>> Replace the exisiting need_emulation_on_page_fault() with a more generic > > > >>> is_emulatable() kvm_x86_ops callback, which is called unconditionally > > > >>> by x86_emulate_instruction(). > > > >> > > > >> Having recently noticed that emulate_ud() is broken when the guest's > > > >> TF is set, I suppose I should ask: does your new code function > > > >> sensibly when TF is set? > > > > > > > > Barring a VMX fault injection interaction I'm not thinking of, yes. The > > > > SGX reaction to the #UD VM-Exit is to inject a #UD and resume the guest, > > > > pending breakpoints shouldn't be affected in any way (unless some other > > > > part of KVM mucks with them, e.g. when guest single-stepping is enabled). > > > > > > What I mean is: does the code actually do what you think it does if TF is > > > set? Right now, as I understand it, the KVM emulation code has a bug in > > > which some emulated faults also inject #DB despite the fact that the > > > instruction faulted, and the #DB seems to take precedence over the original > > > fault. This confuses the guest. > > > > Yes. The proposed change is to inject the #UD instead of calling into the > > emulator, and by inspection I've verified that all code that injects a #DB > > is either contained within the emulator or is mutually exclusive with an > > intercepted #UD. It's a qualified yes because I don't have an actual > > testcase to verify my literacy. I'll look into adding a test, either to > > the selftest/x86/sgx or to kvm-unit-tests. > > I wrote one, and it fails: > > # ./tools/testing/selftests/x86/syscall_arg_fault_32 > [RUN] SYSENTER with invalid state > [OK] Seems okay > [RUN] SYSCALL with invalid state > [SKIP] Illegal instruction > [RUN] SYSENTER with TF and invalid state > [OK] Seems okay > [RUN] SYSCALL with TF and invalid state > [WARN] Got stuck single-stepping -- you probably have a KVM bug > > emulate_ud() is buggy. Heh, yeah, I meant for the SGX case, e.g. SYSCALL from inside an enclave with RFLAGS.TF=1.
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 26d1eb83f72a..1341d8390ebe 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1198,7 +1198,7 @@ struct kvm_x86_ops { uint16_t *vmcs_version); uint16_t (*nested_get_evmcs_version)(struct kvm_vcpu *vcpu); - bool (*need_emulation_on_page_fault)(struct kvm_vcpu *vcpu); + bool (*is_emulatable)(struct kvm_vcpu *vcpu, void *insn, int insn_len); }; struct kvm_arch_async_pf { diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c index 98f6e4f88b04..bf6952f8f330 100644 --- a/arch/x86/kvm/mmu.c +++ b/arch/x86/kvm/mmu.c @@ -5412,18 +5412,6 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u64 error_code, if (!mmio_info_in_cache(vcpu, cr2, direct) && !is_guest_mode(vcpu)) emulation_type = EMULTYPE_ALLOW_RETRY; emulate: - /* - * On AMD platforms, under certain conditions insn_len may be zero on #NPF. - * This can happen if a guest gets a page-fault on data access but the HW - * table walker is not able to read the instruction page (e.g instruction - * page is not present in memory). In those cases we simply restart the - * guest, with the exception of AMD Erratum 1096 which is unrecoverable. - */ - if (unlikely(insn && !insn_len)) { - if (!kvm_x86_ops->need_emulation_on_page_fault(vcpu)) - return 1; - } - er = x86_emulate_instruction(vcpu, cr2, emulation_type, insn, insn_len); switch (er) { diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index 48c865a4e5dd..0fb8b60eb136 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -7115,10 +7115,25 @@ static int nested_enable_evmcs(struct kvm_vcpu *vcpu, return -ENODEV; } -static bool svm_need_emulation_on_page_fault(struct kvm_vcpu *vcpu) +static bool svm_is_emulatable(struct kvm_vcpu *vcpu, void *insn, int insn_len) { bool is_user, smap; + if (likely(!insn || insn_len)) + return true; + + /* + * Under certain conditions insn_len may be zero on #NPF. This can + * happen if a guest gets a page-fault on data access but the HW table + * walker is not able to read the instruction page (e.g instruction + * page is not present in memory). In those cases we simply restart the + * guest, with the exception of AMD Erratum 1096 which is unrecoverable. + */ + if (unlikely(insn && !insn_len)) { + if (!kvm_x86_ops->need_emulation_on_page_fault(vcpu)) + return 1; + } + is_user = svm_get_cpl(vcpu) == 3; smap = !kvm_read_cr4_bits(vcpu, X86_CR4_SMAP); @@ -7279,7 +7294,7 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init = { .nested_enable_evmcs = nested_enable_evmcs, .nested_get_evmcs_version = nested_get_evmcs_version, - .need_emulation_on_page_fault = svm_need_emulation_on_page_fault, + .is_emulatable = svm_is_emulatable, }; static int __init svm_init(void) diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index d98eac371c0a..f48fc990ca6d 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -1458,6 +1458,10 @@ static int vmx_rtit_ctl_check(struct kvm_vcpu *vcpu, u64 data) return 0; } +static bool vmx_is_emulatable(struct kvm_vcpu *vcpu, void *insn, int insn_len) +{ + return true; +} static void skip_emulated_instruction(struct kvm_vcpu *vcpu) { @@ -7416,11 +7420,6 @@ static int enable_smi_window(struct kvm_vcpu *vcpu) return 0; } -static bool vmx_need_emulation_on_page_fault(struct kvm_vcpu *vcpu) -{ - return 0; -} - static __init int hardware_setup(void) { unsigned long host_bndcfgs; @@ -7723,7 +7722,7 @@ static struct kvm_x86_ops vmx_x86_ops __ro_after_init = { .set_nested_state = NULL, .get_vmcs12_pages = NULL, .nested_enable_evmcs = NULL, - .need_emulation_on_page_fault = vmx_need_emulation_on_page_fault, + .is_emulatable = vmx_is_emulatable, }; static void vmx_cleanup_l1d_flush(void) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 63bb1ee8258e..afcc01a59421 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -5274,6 +5274,9 @@ int handle_ud(struct kvm_vcpu *vcpu) char sig[5]; /* ud2; .ascii "kvm" */ struct x86_exception e; + if (unlikely(!kvm_x86_ops->is_emulatable(vcpu, NULL, 0))) + return 1; + if (force_emulation_prefix && kvm_read_guest_virt(vcpu, kvm_get_linear_rip(vcpu), sig, sizeof(sig), &e) == 0 && @@ -6430,7 +6433,10 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, int r; struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt; bool writeback = true; - bool write_fault_to_spt = vcpu->arch.write_fault_to_shadow_pgtable; + bool write_fault_to_spt; + + if (unlikely(!kvm_x86_ops->is_emulatable(vcpu, insn, insn_len))) + return EMULATE_DONE; vcpu->arch.l1tf_flush_l1d = true; @@ -6438,6 +6444,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, * Clear write_fault_to_shadow_pgtable here to ensure it is * never reused. */ + write_fault_to_spt = vcpu->arch.write_fault_to_shadow_pgtable; vcpu->arch.write_fault_to_shadow_pgtable = false; kvm_clear_exception_queue(vcpu);
Similar to the existing AMD #NPF case where emulation of the current instruction is not possible due to lack of information, virtualization of Intel SGX will introduce a scenario where emulation is not possible due to the VMExit occurring in an SGX enclave. And again similar to the AMD case, emulation can be initiated by kvm_mmu_page_fault(), i.e. outside of the control of the vendor-specific code. While the cause and architecturally visible behavior of the two cases is different, e.g. Intel SGX will inject a #UD whereas AMD #NPF is a clean resume or complete shutdown, the impact on the common emulation code is identical: KVM must stop emulation immediately and resume the guest. Replace the exisiting need_emulation_on_page_fault() with a more generic is_emulatable() kvm_x86_ops callback, which is called unconditionally by x86_emulate_instruction(). Query is_emulatable() in handle_ud() as well so that the force_emulation_prefix code doesn't incorrectly modify RIP before calling emulate_instruction() in the absurdly unlikely scenario that we encounter forced emulation in conjunction with "do not emulate". Do this for both Intel and AMD so that any future changes to AMD's emulation logic take effect as expected for handle_ud(). Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/include/asm/kvm_host.h | 2 +- arch/x86/kvm/mmu.c | 12 ------------ arch/x86/kvm/svm.c | 19 +++++++++++++++++-- arch/x86/kvm/vmx/vmx.c | 11 +++++------ arch/x86/kvm/x86.c | 9 ++++++++- 5 files changed, 31 insertions(+), 22 deletions(-)