Thanks a lot for your reply, Sean.
On Tue, Jul 27, 2021 at 06:07:35PM +0000, Sean Christopherson wrote:
> On Wed, Jul 28, 2021, Yu Zhang wrote:
> > Hi all,
> >
> > I'd like to ask a question about kvm_reset_context(): is there any
> > reason that we must alway unload TDP root in kvm_mmu_reset_context()?
>
> The short answer is that mmu_role is changing, thus a new root shadow page is
> needed.
I saw the mmu_role is recalculated, but I have not figured out how this
change would affect TDP. May I ask a favor to give an example? Thanks!
I realized that if we only recalculate the mmu role, but do not unload
the TDP root(e.g., when guest efer.nx flips), base role of the SPs will
be inconsistent with the mmu context. But I do not understand why this
shall affect TDP.
>
> > As you know, KVM MMU needs to track guest paging mode changes, to
> > recalculate the mmu roles and reset callback routines(e.g., guest
> > page table walker). These are done in kvm_mmu_reset_context(). Also,
> > entering SMM, cpuid updates, and restoring L1 VMM's host state will
> > trigger kvm_mmu_reset_context() too.
> >
> > Meanwhile, another job done by kvm_mmu_reset_context() is to unload
> > the KVM MMU:
> >
> > - For shadow & legacy TDP, it means to unload the root shadow/TDP
> > page and reconstruct another one in kvm_mmu_reload(), before
> > entering guest. Old shadow/TDP pages will probably be reused later,
> > after future guest paging mode switches.
> >
> > - For TDP MMU, it is even more aggressive, all TDP pages will be
> > zapped, meaning a whole new TDP page table will be recontrustred,
> > with each paging mode change in the guest. I witnessed dozens of
> > rebuildings of TDP when booting a Linux guest(besides the ones
> > caused by memslots rearrangement).
> >
> > However, I am wondering, why do we need the unloading, if GPA->HPA
> > relationship is not changed? And if this is not a must, could we
> > find a way to refactor kvm_mmu_reset_context(), so that unloading
> > of TDP root is only performed when necessary(e.g, SMM switches and
> > maybe after cpuid updates which may change the level of TDP)?
> >
> > I tried to add a parameter in kvm_mmu_reset_context(), to make the
> > unloading optional:
> >
> > +void kvm_mmu_reset_context(struct kvm_vcpu *vcpu, bool force_tdp_unload)
> > {
> > - kvm_mmu_unload(vcpu);
> > + if (!tdp_enabled || force_tdp_unload)
> > + kvm_mmu_unload(vcpu);
> > +
> > kvm_init_mmu(vcpu);
> > }
> >
> > But this change brings another problem - if we keep the TDP root, the
> > role of existing SPs will be obsolete after guest paging mode changes.
> > Altough I guess most role flags are irrelevant in TDP, I am not sure
> > if this could cause any trouble.
> >
> > Is there anyone looking at this issue? Or do you have any suggestion?
>
> What's the problem you're trying to solve? kvm_mmu_reset_context() is most
> definitely a big hammer, e.g. kvm_post_set_cr0() and kvm_post_set_cr4() in
> particular could be reworked to do something like kvm_mmu_new_pgd() + kvm_init_mmu(),
> but modifying mmu_role bits in CR0/CR4 should be a rare event, i.e. there hasn't
> sufficient motivation to optimize CR0/CR4 changes.
Well, I noticed this when I was trying to find the reason why a single GFN
can have multiple rmaps in TDP(not the SMM case, which uses different memslot).
And the fact that guest paging mode change will cause the unloading of TDP
looks counter-intuitive. I know I must have missed something important, and
I have a strong desire to figure out why. :)
Then I tried with TDP MMU, yet to find the unloading is performed even more
aggressively...
>
> Note, most CR4 bits and CR0.PG are tracked in kvm_mmu_extended_role, not
> kvm_mmu_page_role, which adds a minor wrinkle to the logic.
>
The extended role is another pain point for me when reading KVM MMU code.
I can understand it is useful in shadow, but does it also matters in TDP?
B.R.
Yu
