Re: [PATCH v10 045/108] KVM: x86/mmu: Add a private pointer to struct kvm_mmu_page

[Huang, Kai]

On Wed, 2022-11-16 at 10:32 +0000, Huang, Kai wrote:
> > +
> > +static inline void kvm_mmu_alloc_private_spt(struct kvm_vcpu *vcpu,
> > +					     struct kvm_mmu_memory_cache
> > *private_spt_cache,
> > +					     struct kvm_mmu_page *sp)
> 
> This function is very weird in the context of this patch.  _Only_ a new vcpu-
> scope 'mmu_private_spte_cache' is added in this patch, but here you allow
> caller
> to pass an additional argument of private_spt_cache.  So there must be another
> cache which is not introduced in this patch?
> 
> > +{
> > +	/*
> > +	 * vcpu == NULL means non-root SPT:
> > +	 * vcpu == NULL is used to split a large SPT into smaller SPT. 
> > Root SPT
> > +	 * is not a large SPT.
> 
> I am guessing this "vcpu == NULL" case is for "Eager Splitting"?
> 
> If so, why not adding a global MMU cache for private_spt allocation, and make
> vcpu->arch.mmu_private_spt_cache point to the global cache?  In this case, in
> the context where you only have 'kvm', you can use the global cache directly. 
> And in the context where you have a 'vcpu', you just use vcpu's cache.

So I went through all MMU related patches in this series, but I cannot find a
place where this function is called with 'vcpu == NULL' and a valid cache is
passed in, if I am reading correctly.

Also checked that "Eager Splitting" uses a kvm-scope cache for legacy MMU, but
just uses __get_free_page() for TDP MMU.  And in later patch "KVM: x86/tdp_mmu:
Support TDX private mapping for TDP MMU", __get_free_page() is also used to
allocate the private_spt (which is consistent with existing eager splitting
code).

So IIUC only legacy MMU code will call this function with 'vcpu == NULL' and a
valid cache.  In this case, please remove the 'private_spt_cache' argument for
now, and make the function always allocate from the vcpu-
>arch.mmu_private_spt_cache.  

You can add the additional argument when TDX gets legacy MMU support.

Also, I think you need to move eager splitting support part (whether that
handling is correct is another story) from the later patch to this patch. 
Otherwise this patch is not complete.


>  
> > +	 */
> > +	bool is_root = vcpu &&
> > +		vcpu->arch.root_mmu.root_role.level == sp->role.level;
> > +
> > +	if (vcpu)
> > +		private_spt_cache = &vcpu->arch.mmu_private_spt_cache;
> > +	KVM_BUG_ON(!kvm_mmu_page_role_is_private(sp->role), vcpu->kvm);
> > +	if (is_root)
> > +		/*
> > +		 * Because TDX module assigns root Secure-EPT page and set
> > it to
> > +		 * Secure-EPTP when TD vcpu is created, secure page table
> > for
> > +		 * root isn't needed.
> > +		 */
> > +		sp->private_spt = NULL;
> > +	else {
> > +		sp->private_spt =
> > kvm_mmu_memory_cache_alloc(private_spt_cache);
> > +		/*
> > +		 * Because mmu_private_spt_cache is topped up before
> > staring kvm
> > +		 * page fault resolving, the allocation above shouldn't
> > fail.
> > +		 */
> > +		WARN_ON_ONCE(!sp->private_spt);
> > +	}
> > +}
> > +