Re: A question of TDP unloading.

[Sean Christopherson <seanjc@google.com>]

On Thu, Jul 29, 2021, Yan Zhao wrote:
> On Thu, Jul 29, 2021 at 11:00:56AM +0800, Yu Zhang wrote:
> > > 
> > > Ooof that's a lot of resets, though if there are only a handful of
> > > pages mapped, it might not be a noticeable performance impact. I think
> > > it'd be worth collecting some performance data to quantify the impact.
> > 
> > Yes. Too many reset will definitely hurt the performance, though I did not see
> > obvious delay.
> >
> 
> if I add below limits before unloading mmu, and with
> enable_unrestricted_guest=0, the boot time can be reduced to 31 secs
> from more than 5 minutes. 
> 
>  void kvm_mmu_reset_context(struct kvm_vcpu *vcpu)
>  {
> -       kvm_mmu_unload(vcpu);
> -       kvm_init_mmu(vcpu, true);
> +       union kvm_mmu_role new_role =
> +               kvm_calc_tdp_mmu_root_page_role(vcpu, false);
> +       struct kvm_mmu *context = &vcpu->arch.root_mmu;
> +       bool reset = false;
> +
> +       if (new_role.as_u64 != context->mmu_role.as_u64) {

Aha!  A clue!

This hack indicates that the call to kvm_mmu_reset_context() is spurious, i.e.
none of the MMU role bits in CR0/CR4/EFER are changing.  Dollars to donuts says
this is due to the long-standing hack-a-fix in enter_rmode() that unconditionally
reset the MMU when the guest entered real mode.

Prior to commit 5babbb43a58a ("KVM: VMX: Remove explicit MMU reset in enter_rmode()").
(sitting in kvm/queue), enter_rmode() to deal with unrestricted_guest=0 would
unconditionally do kvm_mmu_reset_context().  Based on the above, it sounds like
your guest is going in and out of RM/PM, i.e. toggling CR0.PE.  CR0.PE isn't a
MMU role bit, so the kvm_mmu_reset_context() is spurious unless CR0.PG is also
being changed.

TL;DR: Try the current kvm/queue, or at least after commit 5babbb43a58a
       ("KVM: VMX: Remove explicit MMU reset in enter_rmode()").

> +               kvm_mmu_unload(vcpu);
> +               reset = true;
> +       }
> +       kvm_init_mmu(vcpu, reset);
> 
> But with enable_unrestricted_guest=0, if I further modify the limits to
> "if (new_role.base.word != context->mmu_role.base.word)", the VM would
> fail to boot.
> so, with mmu extended role changes, unload the mmu is necessary in some
> situation, or at least we need to zap related sptes.
> 
> Thanks
> Yan