Re: [PATCH v1 08/13] KVM: x86/mmu: Refactor TDP MMU child page initialization

[Sean Christopherson <seanjc@google.com>]

On Mon, Dec 13, 2021, David Matlack wrote:
> Separate the allocation of child pages from the initialization. This is

"from their initialization" so that it's not a dangling sentence.

> in preparation for doing page splitting outside of the vCPU fault
> context which requires a different allocation mechanism.
> 
> No functional changed intended.
> 
> Signed-off-by: David Matlack <dmatlack@google.com>
> ---
>  arch/x86/kvm/mmu/tdp_mmu.c | 30 +++++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 582d9a798899..a8354d8578f1 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -157,13 +157,18 @@ static struct kvm_mmu_page *tdp_mmu_next_root(struct kvm *kvm,
>  		if (kvm_mmu_page_as_id(_root) != _as_id) {		\
>  		} else
>  
> -static struct kvm_mmu_page *alloc_tdp_mmu_page(struct kvm_vcpu *vcpu, gfn_t gfn,
> -					       union kvm_mmu_page_role role)
> +static struct kvm_mmu_page *alloc_tdp_mmu_page_from_caches(struct kvm_vcpu *vcpu)

Hrm, this ends up being a rather poor name because the "from_kernel" variant also
allocates from a cache, it's just a different cache:

  static struct kvm_mmu_page *alloc_tdp_mmu_page_from_kernel(gfp_t gfp)
  {
	struct kvm_mmu_page *sp;

	gfp |= __GFP_ZERO;

	sp = kmem_cache_alloc(mmu_page_header_cache, gfp);
	if (!sp)
		return NULL;

	...
  }

Given that the !vcpu path is the odd one, and the only user of the from_kernel
variant is the split, maybe this?  I.e. punt on naming until another user of the
"split" variant comes along.

  static struct kvm_mmu_page *__alloc_tdp_mmu_page(struct kvm_vcpu *vcpu)

and

  static struct kvm_mmu_page *__alloc_tdp_mmu_page_for_split(gfp_t gfp)

>  {
>  	struct kvm_mmu_page *sp;
>  
>  	sp = kvm_mmu_memory_cache_alloc(&vcpu->arch.mmu_page_header_cache);
>  	sp->spt = kvm_mmu_memory_cache_alloc(&vcpu->arch.mmu_shadow_page_cache);
> +
> +	return sp;
> +}
> +
> +static void init_tdp_mmu_page(struct kvm_mmu_page *sp, gfn_t gfn, union kvm_mmu_page_role role)

Newline.  I'm all in favor of running over when doing so improves readability, but
that's not the case here.

> +{
>  	set_page_private(virt_to_page(sp->spt), (unsigned long)sp);
>  
>  	sp->role = role;
> @@ -171,11 +176,9 @@ static struct kvm_mmu_page *alloc_tdp_mmu_page(struct kvm_vcpu *vcpu, gfn_t gfn,
>  	sp->tdp_mmu_page = true;
>  
>  	trace_kvm_mmu_get_page(sp, true);
> -
> -	return sp;
>  }
>  
> -static struct kvm_mmu_page *alloc_child_tdp_mmu_page(struct kvm_vcpu *vcpu, struct tdp_iter *iter)
> +static void init_child_tdp_mmu_page(struct kvm_mmu_page *child_sp, struct tdp_iter *iter)

Newline.

>  {
>  	struct kvm_mmu_page *parent_sp;
>  	union kvm_mmu_page_role role;
> @@ -185,7 +188,17 @@ static struct kvm_mmu_page *alloc_child_tdp_mmu_page(struct kvm_vcpu *vcpu, stru
>  	role = parent_sp->role;
>  	role.level--;
>  
> -	return alloc_tdp_mmu_page(vcpu, iter->gfn, role);
> +	init_tdp_mmu_page(child_sp, iter->gfn, role);
> +}
> +
> +static struct kvm_mmu_page *alloc_child_tdp_mmu_page(struct kvm_vcpu *vcpu, struct tdp_iter *iter)

Newline.

> +{
> +	struct kvm_mmu_page *child_sp;
> +
> +	child_sp = alloc_tdp_mmu_page_from_caches(vcpu);
> +	init_child_tdp_mmu_page(child_sp, iter);
> +
> +	return child_sp;
>  }
>  
>  hpa_t kvm_tdp_mmu_get_vcpu_root_hpa(struct kvm_vcpu *vcpu)
> @@ -210,7 +223,10 @@ hpa_t kvm_tdp_mmu_get_vcpu_root_hpa(struct kvm_vcpu *vcpu)
>  			goto out;
>  	}
>  
> -	root = alloc_tdp_mmu_page(vcpu, 0, role);
> +	root = alloc_tdp_mmu_page_from_caches(vcpu);
> +
> +	init_tdp_mmu_page(root, 0, role);
> +
>  	refcount_set(&root->tdp_mmu_root_count, 1);
>  
>  	spin_lock(&kvm->arch.tdp_mmu_pages_lock);
> -- 
> 2.34.1.173.g76aa8bc2d0-goog
>