Re: [PATCH 17/15] KVM: X86: Ensure pae_root to be reconstructed for shadow paging if the guest PDPTEs is changed

[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@xxxxxxxxxxxxxxxxx>
> > ---
> >   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