Re: [PATCH v1 07/13] KVM: x86/mmu: Derive page role from parent

[Sean Christopherson <seanjc@google.com>]

Please include "TDP MMU" somewhere in the shortlog.  It's a nice to have, e.g. not
worth forcing if there's more interesting info to put in the shortlog, but in this
case there are plenty of chars to go around.  E.g.

  KVM: x86/mmu: Derive page role for TDP MMU shadow pages from parent

On Mon, Dec 13, 2021, David Matlack wrote:
> Derive the page role from the parent shadow page, since the only thing
> that changes is the level. This is in preparation for eagerly splitting
> large pages during VM-ioctls which does not have access to the vCPU

s/does/do since VM-ioctls is plural.

> MMU context.
> 
> No functional change intended.
> 
> Signed-off-by: David Matlack <dmatlack@google.com>
> ---
>  arch/x86/kvm/mmu/tdp_mmu.c | 43 ++++++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 2fb2d7677fbf..582d9a798899 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -157,23 +157,8 @@ static struct kvm_mmu_page *tdp_mmu_next_root(struct kvm *kvm,
>  		if (kvm_mmu_page_as_id(_root) != _as_id) {		\
>  		} else
>  
> -static union kvm_mmu_page_role page_role_for_level(struct kvm_vcpu *vcpu,
> -						   int level)
> -{
> -	union kvm_mmu_page_role role;
> -
> -	role = vcpu->arch.mmu->mmu_role.base;
> -	role.level = level;
> -	role.direct = true;
> -	role.has_4_byte_gpte = false;
> -	role.access = ACC_ALL;
> -	role.ad_disabled = !shadow_accessed_mask;
> -
> -	return role;
> -}
> -
>  static struct kvm_mmu_page *alloc_tdp_mmu_page(struct kvm_vcpu *vcpu, gfn_t gfn,
> -					       int level)
> +					       union kvm_mmu_page_role role)
>  {
>  	struct kvm_mmu_page *sp;
>  
> @@ -181,7 +166,7 @@ static struct kvm_mmu_page *alloc_tdp_mmu_page(struct kvm_vcpu *vcpu, gfn_t gfn,
>  	sp->spt = kvm_mmu_memory_cache_alloc(&vcpu->arch.mmu_shadow_page_cache);
>  	set_page_private(virt_to_page(sp->spt), (unsigned long)sp);
>  
> -	sp->role.word = page_role_for_level(vcpu, level).word;
> +	sp->role = role;
>  	sp->gfn = gfn;
>  	sp->tdp_mmu_page = true;
>  
> @@ -190,6 +175,19 @@ static struct kvm_mmu_page *alloc_tdp_mmu_page(struct kvm_vcpu *vcpu, gfn_t gfn,
>  	return sp;
>  }
>  
> +static struct kvm_mmu_page *alloc_child_tdp_mmu_page(struct kvm_vcpu *vcpu, struct tdp_iter *iter)

Newline please, this is well over 80 chars.

static struct kvm_mmu_page *alloc_child_tdp_mmu_page(struct kvm_vcpu *vcpu,
						     struct tdp_iter *iter)
> +{
> +	struct kvm_mmu_page *parent_sp;
> +	union kvm_mmu_page_role role;
> +
> +	parent_sp = sptep_to_sp(rcu_dereference(iter->sptep));
> +
> +	role = parent_sp->role;
> +	role.level--;
> +
> +	return alloc_tdp_mmu_page(vcpu, iter->gfn, role);
> +}
> +
>  hpa_t kvm_tdp_mmu_get_vcpu_root_hpa(struct kvm_vcpu *vcpu)
>  {
>  	union kvm_mmu_page_role role;
> @@ -198,7 +196,12 @@ hpa_t kvm_tdp_mmu_get_vcpu_root_hpa(struct kvm_vcpu *vcpu)
>  
>  	lockdep_assert_held_write(&kvm->mmu_lock);
>  
> -	role = page_role_for_level(vcpu, vcpu->arch.mmu->shadow_root_level);
> +	role = vcpu->arch.mmu->mmu_role.base;
> +	role.level = vcpu->arch.mmu->shadow_root_level;
> +	role.direct = true;
> +	role.has_4_byte_gpte = false;
> +	role.access = ACC_ALL;
> +	role.ad_disabled = !shadow_accessed_mask;

Hmm, so _all_ of this unnecessary, i.e. this can simply be:

	role = vcpu->arch.mmu->mmu_role.base;

Probably better to handle everything except .level in a separate prep commit.

I'm not worried about the cost, I want to avoid potential confusion as to why the
TDP MMU is apparently "overriding" these fields.