On Thu, Jan 6, 2022 at 12:45 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx>
> > ---
> > 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.
All great suggestions. I'll include these changes in the next version,
including an additional patch to eliminate the redundant role
overrides.
