On Sun, Aug 07, 2022 at 03:01:24PM -0700, isaku.yamahata@xxxxxxxxx wrote:
> From: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
>
> Refactor tdp_mmu_alloc_sp() and tdp_mmu_init_sp and eliminate
> tdp_mmu_init_child_sp(). Currently tdp_mmu_init_sp() (or
> tdp_mmu_init_child_sp()) sets kvm_mmu_page.role after tdp_mmu_alloc_sp()
> allocating struct kvm_mmu_page and its page table page. This patch makes
> tdp_mmu_alloc_sp() initialize kvm_mmu_page.role instead of
> tdp_mmu_init_sp().
>
> To handle private page tables, argument of is_private needs to be passed
> down. Given that already page level is passed down, it would be cumbersome
> to add one more parameter about sp. Instead replace the level argument with
> union kvm_mmu_page_role. Thus the number of argument won't be increased
> and more info about sp can be passed down.
Please update the description, no level argument is replaced in this
patch.
>
> For private sp, secure page table will be also allocated in addition to
> struct kvm_mmu_page and page table (spt member). The allocation functions
> (tdp_mmu_alloc_sp() and __tdp_mmu_alloc_sp_for_split()) need to know if the
> allocation is for the conventional page table or private page table. Pass
> union kvm_mmu_role to those functions and initialize role member of struct
> kvm_mmu_page.
>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> ---
> arch/x86/kvm/mmu/tdp_iter.h | 12 ++++++++++
> arch/x86/kvm/mmu/tdp_mmu.c | 45 +++++++++++++++++--------------------
> 2 files changed, 32 insertions(+), 25 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu/tdp_iter.h b/arch/x86/kvm/mmu/tdp_iter.h
> index f0af385c56e0..9e56a5b1024c 100644
> --- a/arch/x86/kvm/mmu/tdp_iter.h
> +++ b/arch/x86/kvm/mmu/tdp_iter.h
> @@ -115,4 +115,16 @@ void tdp_iter_start(struct tdp_iter *iter, struct kvm_mmu_page *root,
> void tdp_iter_next(struct tdp_iter *iter);
> void tdp_iter_restart(struct tdp_iter *iter);
>
> +static inline union kvm_mmu_page_role tdp_iter_child_role(struct tdp_iter *iter)
> +{
> + union kvm_mmu_page_role child_role;
> + struct kvm_mmu_page *parent_sp;
> +
> + parent_sp = sptep_to_sp(rcu_dereference(iter->sptep));
> +
> + child_role = parent_sp->role;
> + child_role.level--;
> + return child_role;
> +}
> +
> #endif /* __KVM_X86_MMU_TDP_ITER_H */
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 90b468a3a1a2..ce69535754ff 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -271,22 +271,28 @@ static struct kvm_mmu_page *tdp_mmu_next_root(struct kvm *kvm,
> kvm_mmu_page_as_id(_root) != _as_id) { \
> } else
>
> -static struct kvm_mmu_page *tdp_mmu_alloc_sp(struct kvm_vcpu *vcpu)
> +static struct kvm_mmu_page *tdp_mmu_alloc_sp(struct kvm_vcpu *vcpu,
> + union kvm_mmu_page_role role)
> {
> 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);
> + sp->role = role;
>
> return sp;
> }
>
> static void tdp_mmu_init_sp(struct kvm_mmu_page *sp, tdp_ptep_t sptep,
> - gfn_t gfn, union kvm_mmu_page_role role)
> + gfn_t gfn)
> {
> set_page_private(virt_to_page(sp->spt), (unsigned long)sp);
>
> - sp->role = role;
> + /*
> + * role must be set before calling this function. At least role.level
> + * is not 0 (PG_LEVEL_NONE).
> + */
> + WARN_ON(!sp->role.word);
> sp->gfn = gfn;
> sp->ptep = sptep;
> sp->tdp_mmu_page = true;
> @@ -294,20 +300,6 @@ static void tdp_mmu_init_sp(struct kvm_mmu_page *sp, tdp_ptep_t sptep,
> trace_kvm_mmu_get_page(sp, true);
> }
>
> -static void tdp_mmu_init_child_sp(struct kvm_mmu_page *child_sp,
> - 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--;
> -
> - tdp_mmu_init_sp(child_sp, iter->sptep, iter->gfn, role);
> -}
> -
> hpa_t kvm_tdp_mmu_get_vcpu_root_hpa(struct kvm_vcpu *vcpu)
> {
> union kvm_mmu_page_role role = vcpu->arch.mmu->root_role;
> @@ -326,8 +318,8 @@ hpa_t kvm_tdp_mmu_get_vcpu_root_hpa(struct kvm_vcpu *vcpu)
> goto out;
> }
>
> - root = tdp_mmu_alloc_sp(vcpu);
> - tdp_mmu_init_sp(root, NULL, 0, role);
> + root = tdp_mmu_alloc_sp(vcpu, role);
> + tdp_mmu_init_sp(root, NULL, 0);
>
> refcount_set(&root->tdp_mmu_root_count, 1);
>
> @@ -1154,8 +1146,8 @@ static int tdp_mmu_populate_nonleaf(
> WARN_ON(is_shadow_present_pte(iter->old_spte));
> WARN_ON(is_removed_spte(iter->old_spte));
>
> - sp = tdp_mmu_alloc_sp(vcpu);
> - tdp_mmu_init_child_sp(sp, iter);
> + sp = tdp_mmu_alloc_sp(vcpu, tdp_iter_child_role(iter));
> + tdp_mmu_init_sp(sp, iter->sptep, iter->gfn);
>
> ret = tdp_mmu_link_sp(vcpu->kvm, iter, sp, account_nx, true);
> if (ret)
> @@ -1423,7 +1415,8 @@ bool kvm_tdp_mmu_wrprot_slot(struct kvm *kvm,
> return spte_set;
> }
>
> -static struct kvm_mmu_page *__tdp_mmu_alloc_sp_for_split(gfp_t gfp)
> +static struct kvm_mmu_page *__tdp_mmu_alloc_sp_for_split(
> + gfp_t gfp, union kvm_mmu_page_role role)
> {
> struct kvm_mmu_page *sp;
>
> @@ -1433,6 +1426,7 @@ static struct kvm_mmu_page *__tdp_mmu_alloc_sp_for_split(gfp_t gfp)
> if (!sp)
> return NULL;
>
> + sp->role = role;
> sp->spt = (void *)__get_free_page(gfp);
> if (!sp->spt) {
> kmem_cache_free(mmu_page_header_cache, sp);
> @@ -1446,6 +1440,7 @@ static struct kvm_mmu_page *tdp_mmu_alloc_sp_for_split(struct kvm *kvm,
> struct tdp_iter *iter,
> bool shared)
> {
> + union kvm_mmu_page_role role = tdp_iter_child_role(iter);
> struct kvm_mmu_page *sp;
>
> /*
> @@ -1457,7 +1452,7 @@ static struct kvm_mmu_page *tdp_mmu_alloc_sp_for_split(struct kvm *kvm,
> * If this allocation fails we drop the lock and retry with reclaim
> * allowed.
> */
> - sp = __tdp_mmu_alloc_sp_for_split(GFP_NOWAIT | __GFP_ACCOUNT);
> + sp = __tdp_mmu_alloc_sp_for_split(GFP_NOWAIT | __GFP_ACCOUNT, role);
> if (sp)
> return sp;
>
> @@ -1469,7 +1464,7 @@ static struct kvm_mmu_page *tdp_mmu_alloc_sp_for_split(struct kvm *kvm,
> write_unlock(&kvm->mmu_lock);
>
> iter->yielded = true;
> - sp = __tdp_mmu_alloc_sp_for_split(GFP_KERNEL_ACCOUNT);
> + sp = __tdp_mmu_alloc_sp_for_split(GFP_KERNEL_ACCOUNT, role);
>
> if (shared)
> read_lock(&kvm->mmu_lock);
> @@ -1488,7 +1483,7 @@ static int tdp_mmu_split_huge_page(struct kvm *kvm, struct tdp_iter *iter,
> const int level = iter->level;
> int ret, i;
>
> - tdp_mmu_init_child_sp(sp, iter);
> + tdp_mmu_init_sp(sp, iter->sptep, iter->gfn);
>
> /*
> * No need for atomics when writing to sp->spt since the page table has
> --
> 2.25.1
>
