On Thu, Jan 6, 2022 at 12:59 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx>
> > ---
> > 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)
Will do.
>
> > {
> > 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.
Ah shoot. I had configured my editor to use a 100 char line limit for
kernel code, but reading the kernel style guide more closely I see
that 80 is still the preferred limit. I'll go back to preferring 80 and
only go over when it explicitly makes the code more readable.
>
> > +{
> > 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
> >
