Re: A question of TDP unloading.

[Sean Christopherson <seanjc@google.com>]

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.