| Message ID | 20211213225918.672507-8-dmatlack@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86/mmu: Eager Page Splitting for the TDP MMU | expand |
On Mon, Dec 13, 2021 at 10:59:12PM +0000, 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 > MMU context. > > No functional change intended. > > Signed-off-by: David Matlack <dmatlack@google.com> Reviewed-by: Peter Xu <peterx@redhat.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.
On Thu, Jan 6, 2022 at 12:45 PM Sean Christopherson <seanjc@google.com> 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@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. All great suggestions. I'll include these changes in the next version, including an additional patch to eliminate the redundant role overrides.
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) +{ + 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; /* Check for an existing root before allocating a new one. */ for_each_tdp_mmu_root(kvm, root, kvm_mmu_role_as_id(role)) { @@ -207,7 +210,7 @@ hpa_t kvm_tdp_mmu_get_vcpu_root_hpa(struct kvm_vcpu *vcpu) goto out; } - root = alloc_tdp_mmu_page(vcpu, 0, vcpu->arch.mmu->shadow_root_level); + root = alloc_tdp_mmu_page(vcpu, 0, role); refcount_set(&root->tdp_mmu_root_count, 1); spin_lock(&kvm->arch.tdp_mmu_pages_lock); @@ -1033,7 +1036,7 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) if (is_removed_spte(iter.old_spte)) break; - sp = alloc_tdp_mmu_page(vcpu, iter.gfn, iter.level - 1); + sp = alloc_child_tdp_mmu_page(vcpu, &iter); if (!tdp_mmu_install_sp_atomic(vcpu->kvm, &iter, sp, account_nx)) { tdp_mmu_free_sp(sp); break;
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 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(-)