* [PATCH 0/2] KVM: nVMX: Alternative no-EPT GUEST_CR3 fix @ 2019-05-20 20:10 Sean Christopherson 2019-05-20 20:10 ` [PATCH 1/2] KVM: nVMX: Stash L1's CR3 in vmcs01.GUEST_CR3 on nested entry w/o EPT Sean Christopherson 2019-05-20 20:10 ` [PATCH 2/2] Revert "KVM: nVMX: always use early vmcs check when EPT is disabled" Sean Christopherson 0 siblings, 2 replies; 9+ messages in thread From: Sean Christopherson @ 2019-05-20 20:10 UTC (permalink / raw) To: Paolo Bonzini, Radim Krčmář; +Cc: kvm As an alternative to forcing early consistency checks in hardware (to avoid reaching nested_vmx_restore_host_state() due to a missed VM-FAIL), stuff vmcs01.GUEST_CR3 with L1's desired CR3 prior to nested VM-Entry so that nested_vmx_restore_host_state() loads the correct L1 state when EPT is disabled in L0. Code complexity in the two approaches is roughly similar, although the GUEST_CR3 stuffing is definitely more subtle. The primary motiviation is performance, e.g. VMWRITE is less than 30 cyles, whereas doing consistency checks via hardware is several hundred cycles. Arguably performance may be somewhat of a moot point when EPT is disabled, but Nehalem hardware isn't *that* old. :-) Sean Christopherson (2): KVM: nVMX: Stash L1's CR3 in vmcs01.GUEST_CR3 on nested entry w/o EPT Revert "KVM: nVMX: always use early vmcs check when EPT is disabled" arch/x86/include/uapi/asm/vmx.h | 1 - arch/x86/kvm/vmx/nested.c | 27 ++++++--------------------- 2 files changed, 6 insertions(+), 22 deletions(-) -- 2.21.0 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] KVM: nVMX: Stash L1's CR3 in vmcs01.GUEST_CR3 on nested entry w/o EPT 2019-05-20 20:10 [PATCH 0/2] KVM: nVMX: Alternative no-EPT GUEST_CR3 fix Sean Christopherson @ 2019-05-20 20:10 ` Sean Christopherson 2019-06-06 12:24 ` Paolo Bonzini 2019-05-20 20:10 ` [PATCH 2/2] Revert "KVM: nVMX: always use early vmcs check when EPT is disabled" Sean Christopherson 1 sibling, 1 reply; 9+ messages in thread From: Sean Christopherson @ 2019-05-20 20:10 UTC (permalink / raw) To: Paolo Bonzini, Radim Krčmář; +Cc: kvm KVM does not have 100% coverage of VMX consistency checks, i.e. some checks that cause VM-Fail may only be detected by hardware during a nested VM-Entry. In such a case, KVM must restore L1's state to the pre-VM-Enter state as L2's state has already been loaded into KVM's software model. L1's CR3 and PDPTRs in particular are loaded from vmcs01.GUEST_*. But when EPT is disabled, the associated fields hold KVM's shadow values, not L1's "real" values. Fortunately, when EPT is disabled the PDPTRs come from memory, i.e. are not cached in the VMCS. Which leaves CR3 as the sole anomaly. Handle CR3 by overwriting vmcs01.GUEST_CR3 with L1's CR3 during the nested VM-Entry when EPT is disabled *and* nested early checks are disabled, so that nested_vmx_restore_host_state() will naturally restore the correct vcpu->arch.cr3 from vmcs01.GUEST_CR3. Note, these shenanigans work because nested_vmx_restore_host_state() does a full kvm_mmu_reset_context(), i.e. unloads the current MMU, which guarantees vmcs01.GUEST_CR3 will be rewritten with a new shadow CR3 prior to re-entering L1. Writing vmcs01.GUEST_CR3 is done if and only if nested early checks are disabled as "late" VM-Fail should never happen in that case (KVM WARNs), and the conditional write avoids the need to restore the correct GUEST_CR3 when nested_vmx_check_vmentry_hw() fails. Reported-by: Paolo Bonzini <pbonzini@redhat.com> Fixes: bd18bffca353 ("KVM: nVMX: restore host state in nested_vmx_vmexit for VMFail") Cc: stable@vger.kernel.org Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kvm/vmx/nested.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 1032f068f0b9..92117092f6e9 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -2963,6 +2963,8 @@ int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, bool from_vmentry) if (kvm_mpx_supported() && !(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS)) vmx->nested.vmcs01_guest_bndcfgs = vmcs_read64(GUEST_BNDCFGS); + if (!enable_ept && !nested_early_check) + vmcs_writel(GUEST_CR3, vcpu->arch.cr3); vmx_switch_vmcs(vcpu, &vmx->nested.vmcs02); @@ -3794,7 +3796,8 @@ static void nested_vmx_restore_host_state(struct kvm_vcpu *vcpu) * VMFail, like everything else we just need to ensure our * software model is up-to-date. */ - ept_save_pdptrs(vcpu); + if (enable_ept) + ept_save_pdptrs(vcpu); kvm_mmu_reset_context(vcpu); -- 2.21.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] KVM: nVMX: Stash L1's CR3 in vmcs01.GUEST_CR3 on nested entry w/o EPT 2019-05-20 20:10 ` [PATCH 1/2] KVM: nVMX: Stash L1's CR3 in vmcs01.GUEST_CR3 on nested entry w/o EPT Sean Christopherson @ 2019-06-06 12:24 ` Paolo Bonzini 0 siblings, 0 replies; 9+ messages in thread From: Paolo Bonzini @ 2019-06-06 12:24 UTC (permalink / raw) To: Sean Christopherson, Radim Krčmář; +Cc: kvm On 20/05/19 22:10, Sean Christopherson wrote: > KVM does not have 100% coverage of VMX consistency checks, i.e. some > checks that cause VM-Fail may only be detected by hardware during a > nested VM-Entry. In such a case, KVM must restore L1's state to the > pre-VM-Enter state as L2's state has already been loaded into KVM's > software model. > > L1's CR3 and PDPTRs in particular are loaded from vmcs01.GUEST_*. But > when EPT is disabled, the associated fields hold KVM's shadow values, > not L1's "real" values. Fortunately, when EPT is disabled the PDPTRs > come from memory, i.e. are not cached in the VMCS. Which leaves CR3 > as the sole anomaly. Handle CR3 by overwriting vmcs01.GUEST_CR3 with > L1's CR3 during the nested VM-Entry when EPT is disabled *and* nested > early checks are disabled, so that nested_vmx_restore_host_state() will > naturally restore the correct vcpu->arch.cr3 from vmcs01.GUEST_CR3. > > Note, these shenanigans work because nested_vmx_restore_host_state() > does a full kvm_mmu_reset_context(), i.e. unloads the current MMU, > which guarantees vmcs01.GUEST_CR3 will be rewritten with a new shadow > CR3 prior to re-entering L1. Writing vmcs01.GUEST_CR3 is done if and > only if nested early checks are disabled as "late" VM-Fail should never > happen in that case (KVM WARNs), and the conditional write avoids the > need to restore the correct GUEST_CR3 when nested_vmx_check_vmentry_hw() > fails. > > Reported-by: Paolo Bonzini <pbonzini@redhat.com> > Fixes: bd18bffca353 ("KVM: nVMX: restore host state in nested_vmx_vmexit for VMFail") > Cc: stable@vger.kernel.org > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/kvm/vmx/nested.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index 1032f068f0b9..92117092f6e9 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -2963,6 +2963,8 @@ int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, bool from_vmentry) > if (kvm_mpx_supported() && > !(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS)) > vmx->nested.vmcs01_guest_bndcfgs = vmcs_read64(GUEST_BNDCFGS); This hunk needs a comment that says basically the same things that are in the commit message. /* Temporarily overwrite vmcs01.GUEST_CR3 with L1's CR3 during * the nested VM-Entry when EPT is disabled *and* nested early * checks are disabled, because nested_vmx_restore_host_state() * wants to restore the correct vcpu->arch.cr3 from * vmcs01.GUEST_CR3. On nested vmexit, kvm_mmu_reset_context() * will overwrite GUEST_CR3 with the shadow CR3 prior to * re-entering L1. This is not needed when nested early checks * are enabled, because it doesn't reload the MMU until * after the checks have succeeded. */ And also, the restore of the correct vcpu->arch.cr3 from vmcs01.GUEST_CR3 can be moved to this patch otherwise the comment would not be accurate. But, as I said in the review of patch 2, I'm a bit lost as to how kvm_mmu_reset_context() is enough to reload the shadow CR3 into the vmcs01. Paolo > + if (!enable_ept && !nested_early_check) > + vmcs_writel(GUEST_CR3, vcpu->arch.cr3); > > vmx_switch_vmcs(vcpu, &vmx->nested.vmcs02); > > @@ -3794,7 +3796,8 @@ static void nested_vmx_restore_host_state(struct kvm_vcpu *vcpu) > * VMFail, like everything else we just need to ensure our > * software model is up-to-date. > */ > - ept_save_pdptrs(vcpu); > + if (enable_ept) > + ept_save_pdptrs(vcpu); > > kvm_mmu_reset_context(vcpu); > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] Revert "KVM: nVMX: always use early vmcs check when EPT is disabled" 2019-05-20 20:10 [PATCH 0/2] KVM: nVMX: Alternative no-EPT GUEST_CR3 fix Sean Christopherson 2019-05-20 20:10 ` [PATCH 1/2] KVM: nVMX: Stash L1's CR3 in vmcs01.GUEST_CR3 on nested entry w/o EPT Sean Christopherson @ 2019-05-20 20:10 ` Sean Christopherson 2019-06-06 12:22 ` Paolo Bonzini 1 sibling, 1 reply; 9+ messages in thread From: Sean Christopherson @ 2019-05-20 20:10 UTC (permalink / raw) To: Paolo Bonzini, Radim Krčmář; +Cc: kvm Now that KVM stuffs vmc01.GUEST_CR3 prior to nested VM-Entry, there is no need to avoid nested_vmx_restore_host_state() when EPT is disabled as refreshing vcpu->arch.cr3 from vmcs01.GUEST_CR3 will automagically obtain the correct value. This reverts commit 2b27924bb1d48e3775f432b70bdad5e6dd4e7798. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/include/uapi/asm/vmx.h | 1 - arch/x86/kvm/vmx/nested.c | 22 ++-------------------- 2 files changed, 2 insertions(+), 21 deletions(-) diff --git a/arch/x86/include/uapi/asm/vmx.h b/arch/x86/include/uapi/asm/vmx.h index d213ec5c3766..f0b0c90dd398 100644 --- a/arch/x86/include/uapi/asm/vmx.h +++ b/arch/x86/include/uapi/asm/vmx.h @@ -146,7 +146,6 @@ #define VMX_ABORT_SAVE_GUEST_MSR_FAIL 1 #define VMX_ABORT_LOAD_HOST_PDPTE_FAIL 2 -#define VMX_ABORT_VMCS_CORRUPTED 3 #define VMX_ABORT_LOAD_HOST_MSR_FAIL 4 #endif /* _UAPIVMX_H */ diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 92117092f6e9..c17fbe3cc2fb 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -3777,18 +3777,8 @@ static void nested_vmx_restore_host_state(struct kvm_vcpu *vcpu) vmx_set_cr4(vcpu, vmcs_readl(CR4_READ_SHADOW)); nested_ept_uninit_mmu_context(vcpu); - - /* - * This is only valid if EPT is in use, otherwise the vmcs01 GUEST_CR3 - * points to shadow pages! Fortunately we only get here after a WARN_ON - * if EPT is disabled, so a VMabort is perfectly fine. - */ - if (enable_ept) { - vcpu->arch.cr3 = vmcs_readl(GUEST_CR3); - __set_bit(VCPU_EXREG_CR3, (ulong *)&vcpu->arch.regs_avail); - } else { - nested_vmx_abort(vcpu, VMX_ABORT_VMCS_CORRUPTED); - } + vcpu->arch.cr3 = vmcs_readl(GUEST_CR3); + __set_bit(VCPU_EXREG_CR3, (ulong *)&vcpu->arch.regs_avail); /* * Use ept_save_pdptrs(vcpu) to load the MMU's cached PDPTRs @@ -5729,14 +5719,6 @@ __init int nested_vmx_hardware_setup(int (*exit_handlers[])(struct kvm_vcpu *)) { int i; - /* - * Without EPT it is not possible to restore L1's CR3 and PDPTR on - * VMfail, because they are not available in vmcs01. Just always - * use hardware checks. - */ - if (!enable_ept) - nested_early_check = 1; - if (!cpu_has_vmx_shadow_vmcs()) enable_shadow_vmcs = 0; if (enable_shadow_vmcs) { -- 2.21.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Revert "KVM: nVMX: always use early vmcs check when EPT is disabled" 2019-05-20 20:10 ` [PATCH 2/2] Revert "KVM: nVMX: always use early vmcs check when EPT is disabled" Sean Christopherson @ 2019-06-06 12:22 ` Paolo Bonzini 2019-06-06 17:08 ` Sean Christopherson 0 siblings, 1 reply; 9+ messages in thread From: Paolo Bonzini @ 2019-06-06 12:22 UTC (permalink / raw) To: Sean Christopherson, Radim Krčmář; +Cc: kvm On 20/05/19 22:10, Sean Christopherson wrote: > @@ -3777,18 +3777,8 @@ static void nested_vmx_restore_host_state(struct kvm_vcpu *vcpu) > vmx_set_cr4(vcpu, vmcs_readl(CR4_READ_SHADOW)); > > nested_ept_uninit_mmu_context(vcpu); > - > - /* > - * This is only valid if EPT is in use, otherwise the vmcs01 GUEST_CR3 > - * points to shadow pages! Fortunately we only get here after a WARN_ON > - * if EPT is disabled, so a VMabort is perfectly fine. > - */ > - if (enable_ept) { > - vcpu->arch.cr3 = vmcs_readl(GUEST_CR3); > - __set_bit(VCPU_EXREG_CR3, (ulong *)&vcpu->arch.regs_avail); > - } else { > - nested_vmx_abort(vcpu, VMX_ABORT_VMCS_CORRUPTED); > - } > + vcpu->arch.cr3 = vmcs_readl(GUEST_CR3); > + __set_bit(VCPU_EXREG_CR3, (ulong *)&vcpu->arch.regs_avail); > > /* > * Use ept_save_pdptrs(vcpu) to load the MMU's cached PDPTRs This hunk needs to be moved to patch 1, which then becomes much easier to understand... I'm still missing however the place where kvm_mmu_new_cr3 is called in the nested_vmx_restore_host_state path. Paolo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Revert "KVM: nVMX: always use early vmcs check when EPT is disabled" 2019-06-06 12:22 ` Paolo Bonzini @ 2019-06-06 17:08 ` Sean Christopherson 2019-06-06 17:31 ` Paolo Bonzini 0 siblings, 1 reply; 9+ messages in thread From: Sean Christopherson @ 2019-06-06 17:08 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Radim Krčmář, kvm On Thu, Jun 06, 2019 at 02:22:56PM +0200, Paolo Bonzini wrote: > On 20/05/19 22:10, Sean Christopherson wrote: > > @@ -3777,18 +3777,8 @@ static void nested_vmx_restore_host_state(struct kvm_vcpu *vcpu) > > vmx_set_cr4(vcpu, vmcs_readl(CR4_READ_SHADOW)); > > > > nested_ept_uninit_mmu_context(vcpu); > > - > > - /* > > - * This is only valid if EPT is in use, otherwise the vmcs01 GUEST_CR3 > > - * points to shadow pages! Fortunately we only get here after a WARN_ON > > - * if EPT is disabled, so a VMabort is perfectly fine. > > - */ > > - if (enable_ept) { > > - vcpu->arch.cr3 = vmcs_readl(GUEST_CR3); > > - __set_bit(VCPU_EXREG_CR3, (ulong *)&vcpu->arch.regs_avail); > > - } else { > > - nested_vmx_abort(vcpu, VMX_ABORT_VMCS_CORRUPTED); > > - } > > + vcpu->arch.cr3 = vmcs_readl(GUEST_CR3); > > + __set_bit(VCPU_EXREG_CR3, (ulong *)&vcpu->arch.regs_avail); > > > > /* > > * Use ept_save_pdptrs(vcpu) to load the MMU's cached PDPTRs > > This hunk needs to be moved to patch 1, which then becomes much easier > to understand... I kept the revert in a separate patch so that the bug fix could be easily backported to stable branches (commit 2b27924bb1d4 ("KVM: nVMX: always use early vmcs check when EPT is disabled" wasn't tagged for stable). > I'm still missing however the place where kvm_mmu_new_cr3 is called > in the nested_vmx_restore_host_state path. vcpu->arch.root_mmu.root_hpa is set to INVALID_PAGE via: nested_vmx_restore_host_state() -> kvm_mmu_reset_context() -> kvm_mmu_unload() -> kvm_mmu_free_roots() kvm_mmu_unload() has WARN_ON(root_hpa != INVALID_PAGE), i.e. we can bank on 'root_hpa == INVALID_PAGE' unless the implementation of kvm_mmu_reset_context() is changed. On the way into L1, VMCS.GUEST_CR3 is guaranteed to be written (on a successful entry) via: vcpu_enter_guest() -> kvm_mmu_reload() -> kvm_mmu_load() -> kvm_mmu_load_cr3() -> vmx_set_cr3() The optimization in kvm_mmu_reload() will fail because kvm_mmu_unload() set vcpu->arch.root_mmu.root_hpa=INVALID_PAGE, and vcpu->arch.mmu is guaranteed to point at root_mmu (via nested_ept_uninit_mmu_context()). ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Revert "KVM: nVMX: always use early vmcs check when EPT is disabled" 2019-06-06 17:08 ` Sean Christopherson @ 2019-06-06 17:31 ` Paolo Bonzini 2019-06-06 17:49 ` Sean Christopherson 0 siblings, 1 reply; 9+ messages in thread From: Paolo Bonzini @ 2019-06-06 17:31 UTC (permalink / raw) To: Sean Christopherson; +Cc: Radim Krčmář, kvm On 06/06/19 19:08, Sean Christopherson wrote: >> This hunk needs to be moved to patch 1, which then becomes much easier >> to understand... > I kept the revert in a separate patch so that the bug fix could be > easily backported to stable branches (commit 2b27924bb1d4 ("KVM: nVMX: > always use early vmcs check when EPT is disabled" wasn't tagged for > stable). > Yeah, I didn't mark it because of the mess involving the vmx.c split (basically wait and see if someone report it). There was quite some churn so I am a bit wary to do stable backports where I haven't explicitly tested the backport on the oldest affected version. Paolo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Revert "KVM: nVMX: always use early vmcs check when EPT is disabled" 2019-06-06 17:31 ` Paolo Bonzini @ 2019-06-06 17:49 ` Sean Christopherson 2019-06-07 13:26 ` Paolo Bonzini 0 siblings, 1 reply; 9+ messages in thread From: Sean Christopherson @ 2019-06-06 17:49 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Radim Krčmář, kvm On Thu, Jun 06, 2019 at 07:31:13PM +0200, Paolo Bonzini wrote: > On 06/06/19 19:08, Sean Christopherson wrote: > >> This hunk needs to be moved to patch 1, which then becomes much easier > >> to understand... > > I kept the revert in a separate patch so that the bug fix could be > > easily backported to stable branches (commit 2b27924bb1d4 ("KVM: nVMX: > > always use early vmcs check when EPT is disabled" wasn't tagged for > > stable). > > > > Yeah, I didn't mark it because of the mess involving the vmx.c split > (basically wait and see if someone report it). There was quite some > churn so I am a bit wary to do stable backports where I haven't > explicitly tested the backport on the oldest affected version. Do you want me to send a v2 as a single patch? If so, presumably without cc'ing stable? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Revert "KVM: nVMX: always use early vmcs check when EPT is disabled" 2019-06-06 17:49 ` Sean Christopherson @ 2019-06-07 13:26 ` Paolo Bonzini 0 siblings, 0 replies; 9+ messages in thread From: Paolo Bonzini @ 2019-06-07 13:26 UTC (permalink / raw) To: Sean Christopherson; +Cc: Radim Krčmář, kvm On 06/06/19 19:49, Sean Christopherson wrote: > On Thu, Jun 06, 2019 at 07:31:13PM +0200, Paolo Bonzini wrote: >> On 06/06/19 19:08, Sean Christopherson wrote: >>>> This hunk needs to be moved to patch 1, which then becomes much easier >>>> to understand... >>> I kept the revert in a separate patch so that the bug fix could be >>> easily backported to stable branches (commit 2b27924bb1d4 ("KVM: nVMX: >>> always use early vmcs check when EPT is disabled" wasn't tagged for >>> stable). >>> >> >> Yeah, I didn't mark it because of the mess involving the vmx.c split >> (basically wait and see if someone report it). There was quite some >> churn so I am a bit wary to do stable backports where I haven't >> explicitly tested the backport on the oldest affected version. > > Do you want me to send a v2 as a single patch? If so, presumably without > cc'ing stable? Yes, please do, adding in the comment as well. Paolo ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-06-07 13:26 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-05-20 20:10 [PATCH 0/2] KVM: nVMX: Alternative no-EPT GUEST_CR3 fix Sean Christopherson 2019-05-20 20:10 ` [PATCH 1/2] KVM: nVMX: Stash L1's CR3 in vmcs01.GUEST_CR3 on nested entry w/o EPT Sean Christopherson 2019-06-06 12:24 ` Paolo Bonzini 2019-05-20 20:10 ` [PATCH 2/2] Revert "KVM: nVMX: always use early vmcs check when EPT is disabled" Sean Christopherson 2019-06-06 12:22 ` Paolo Bonzini 2019-06-06 17:08 ` Sean Christopherson 2019-06-06 17:31 ` Paolo Bonzini 2019-06-06 17:49 ` Sean Christopherson 2019-06-07 13:26 ` Paolo Bonzini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox