Messages in this thread | | | Date | Wed, 8 Dec 2021 12:00:38 +0800 | Subject | Re: [PATCH 17/15] KVM: X86: Ensure pae_root to be reconstructed for shadow paging if the guest PDPTEs is changed | From | Lai Jiangshan <> |
| |
On 2021/12/8 08:15, Sean Christopherson wrote: >> >> The commit 21823fbda552("KVM: x86: Invalidate all PGDs for the current >> PCID on MOV CR3 w/ flush") skips kvm_mmu_free_roots() after >> load_pdptrs() when rewriting the CR3 with the same value. > > This isn't accurate, prior to that commit KVM wasn't guaranteed to do > kvm_mmu_free_roots() if it got a hit on the current CR3 or if a previous CR3 in > the cache matched the new CR3 (the "cache" has done some odd things in the past). > > So I think this particular flavor would be: > > Fixes: 7c390d350f8b ("kvm: x86: Add fast CR3 switch code path")
If guest is 32bit, fast_cr3_switch() always return false, and kvm_mmu_free_roots() is always called, and no cr3 goes in prev_root.
And from 21823fbda552, fast_cr3_switch() and kvm_mmu_free_roots() are both skipped when cr3 is unchanged.
> >> The commit a91a7c709600("KVM: X86: Don't reset mmu context when >> toggling X86_CR4_PGE") skips kvm_mmu_reset_context() after >> load_pdptrs() when changing CR4.PGE. >> >> Normally, the guest doesn't change the PDPTEs before doing only the >> above operation without touching other bits that can force pae_root to >> be reconstructed. Guests like linux would keep the PDPTEs unchaged >> for every instance of pagetable. >> >> Fixes: d81135a57aa6("KVM: x86: do not reset mmu if CR0.CD and CR0.NW are changed") >> Fixes: 21823fbda552("KVM: x86: Invalidate all PGDs for the current PCID on MOV CR3 w/ flush") >> Fixes: a91a7c709600("KVM: X86: Don't reset mmu context when toggling X86_CR4_PGE") >> Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com> >> --- >> arch/x86/kvm/x86.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index 0176eaa86a35..cfba337e46ab 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -832,8 +832,14 @@ int load_pdptrs(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, unsigned long cr3) >> if (memcmp(mmu->pdptrs, pdpte, sizeof(mmu->pdptrs))) { >> memcpy(mmu->pdptrs, pdpte, sizeof(mmu->pdptrs)); >> kvm_register_mark_dirty(vcpu, VCPU_EXREG_PDPTR); >> - /* Ensure the dirty PDPTEs to be loaded. */ >> - kvm_make_request(KVM_REQ_LOAD_MMU_PGD, vcpu); >> + /* >> + * Ensure the dirty PDPTEs to be loaded for VMX with EPT >> + * enabled or pae_root to be reconstructed for shadow paging. >> + */ >> + if (tdp_enabled) >> + kvm_make_request(KVM_REQ_LOAD_MMU_PGD, vcpu); >> + else >> + kvm_mmu_free_roots(vcpu, vcpu->arch.mmu, KVM_MMU_ROOT_CURRENT); > > Shouldn't matter since it's legacy shadow paging, but @mmu should be used instead > of vcpu->arch.mmuvcpu->arch.mmu.
@mmu is the "guest mmu" (vcpu->arch.walk_mmu), which is used to walk including loading pdptr.
vcpu->arch.mmu is for host constructing mmu for shadowed or tdp mmu which is used in host side management including kvm_mmu_free_roots().
Even they are the same pointer now for !tdp, the meaning is different. I prefer to distinguish them even before kvm_mmu is split different for guest mmu (vcpu->arch.walk_mmu) and host constructing mmu (vcpu->arch.mmu).
(I once searched all the usage of undistinguished usage of kvm_mmu *mmu, and found a bug, see "Use vcpu->arch.walk_mmu for kvm_mmu_invlpg()")
I think Paolo is doing the splitting, unless I would take the job because I have some patches pending depended them.
> > To avoid a dependency on the previous patch, I think it makes sense to have this be: > > if (!tdp_enabled && memcmp(mmu->pdptrs, pdpte, sizeof(mmu->pdptrs))) > kvm_mmu_free_roots(vcpu, mmu, KVM_MMU_ROOT_CURRENT); >
Yes, it is a good idea to add this first.
Thanks for review and suggestion. Lai
| |