| Message ID | 20190520201029.7126-3-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: nVMX: Alternative no-EPT GUEST_CR3 fix | expand |
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
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()).
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
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?
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
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) {
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(-)