Re: [PATCH v4 7/7] KVM: x86/mmu: Lazily allocate memslot rmaps

[Sean Christopherson]

On Tue, May 11, 2021, Sean Christopherson wrote:
> On Tue, May 11, 2021, Ben Gardon wrote:
> > If the TDP MMU is in use, wait to allocate the rmaps until the shadow
> > MMU is actually used. (i.e. a nested VM is launched.) This saves memory
> > equal to 0.2% of guest memory in cases where the TDP MMU is used and
> > there are no nested guests involved.
> > 
> > Signed-off-by: Ben Gardon <bgardon@xxxxxxxxxx>
> > ---
> >  arch/x86/include/asm/kvm_host.h |  2 ++
> >  arch/x86/kvm/mmu/mmu.c          | 53 +++++++++++++++++++++++----------
> >  arch/x86/kvm/mmu/tdp_mmu.c      |  6 ++--
> >  arch/x86/kvm/mmu/tdp_mmu.h      |  4 +--
> >  arch/x86/kvm/x86.c              | 45 +++++++++++++++++++++++++++-
> >  5 files changed, 89 insertions(+), 21 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index fc75ed49bfee..7b65f82ade1c 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1868,4 +1868,6 @@ static inline int kvm_cpu_get_apicid(int mps_cpu)
> >  
> >  int kvm_cpu_dirty_log_size(void);
> >  
> > +int alloc_all_memslots_rmaps(struct kvm *kvm);
> > +
> >  #endif /* _ASM_X86_KVM_HOST_H */
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index b0bdb924d519..183afccd2944 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -1190,7 +1190,8 @@ static void kvm_mmu_write_protect_pt_masked(struct kvm *kvm,
> >  		kvm_tdp_mmu_clear_dirty_pt_masked(kvm, slot,
> >  				slot->base_gfn + gfn_offset, mask, true);
> >  
> > -	if (!kvm->arch.memslots_have_rmaps)
> > +	/* Read memslots_have_rmaps before the rmaps themselves */
> 
> IIRC, you open coded reading memslots_have_rmaps because of a circular
> dependency, but you can solve that simply by defining the helper in mmu.h
> instead of kvm_host.h.
> 
> And I think you could even make it static in mmu.c and omit the smp_load_acuquire
> from the three users in x86.c, though that's probably not worth it.
> 
> Either way, reading the same comment over and over and over, just to make
> checkpatch happy, gets more than a bit tedious.
> 
> That would also allow you to elaborate on why the smp_load_acquire() is
> necessary, and preferably what it pairs with.

Belated thought: you could also introduce the helper in patch 06 in order to
miminize thrash in this patch.