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);
> > + }
> > +}
> > +
