Re: [PATCH v2 16/18] KVM: x86: introduce KVM_REQ_MMU_UPDATE_ROOT

[Sean Christopherson <seanjc@google.com>]

On Sat, Feb 19, 2022, Paolo Bonzini wrote:
> On 2/18/22 22:45, Sean Christopherson wrote:
> > On Thu, Feb 17, 2022, Paolo Bonzini wrote:
> > > Whenever KVM knows the page role flags have changed, it needs to drop
> > > the current MMU root and possibly load one from the prev_roots cache.
> > > Currently it is papering over some overly simplistic code by just
> > > dropping _all_ roots, so that the root will be reloaded by
> > > kvm_mmu_reload, but this has bad performance for the TDP MMU
> > > (which drops the whole of the page tables when freeing a root,
> > > without the performance safety net of a hash table).
> > > 
> > > To do this, KVM needs to do a more kvm_mmu_update_root call from
> > > kvm_mmu_reset_context.  Introduce a new request bit so that the call
> > > can be delayed until after a possible KVM_REQ_MMU_RELOAD, which would
> > > kill all hopes of finding a cached PGD.
> > > 
> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > > ---
> > 
> > Please no.
> > 
> > I really, really do not want to add yet another deferred-load in the nested
> > virtualization paths.
> 
> This is not a deferred load, is it?  It's only kvm_mmu_new_pgd that is
> deferred, but the PDPTR load is not.

Yeah, I'm referring to kvm_mmu_new_pgd().

> > I strongly prefer that we take a more conservative approach and fix 7+8, and then
> > tackle 1, 3, and 4+5 separately if someone cares enough about those flows to avoid
> > dropping roots.
> 
> The thing is, I want to get rid of kvm_mmu_reset_context() altogether. I
> dislike the fact that it kills the roots but still keeps them in the hash
> table, thus relying on separate syncing to avoid future bugs.  It's very
> unintuitive what is "reset" and what isn't.

I agree with all of the above, I just don't think that forcing the issue is going
to be a net positive in the long run.

> > Regarding KVM_REQ_MMU_RELOAD, that mess mostly goes away with my series to replace
> > that with KVM_REQ_MMU_FREE_OBSOLETE_ROOTS.  Obsolete TDP MMU roots will never get
> > a cache hit because the obsolete root will have an "invalid" role.  And if we care
> > about optimizing this with respect to a memslot (highly unlikely), then we could
> > add an MMU generation check in the cache lookup.  I was planning on posting that
> > series as soon as this one is queued, but I'm more than happy to speculatively send
> > a refreshed version that applies on top of this series.
> 
> Yes, please send a version on top of patches 1-13.  That can be reviewed and
> committed in parallel with the root_role changes.

Will do.