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.
> 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.
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.
