| Message ID | 20220221162243.683208-2-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM MMU refactoring part 2: role changes | expand |
On Mon, Feb 21, 2022, Paolo Bonzini wrote: > Most of the time, calls to get_guest_pgd result in calling kvm_read_cr3 > (the exception is only nested TDP). Check if that is the case if > retpolines are enabled, thus avoiding an expensive indirect call. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > arch/x86/kvm/mmu.h | 10 ++++++++++ > arch/x86/kvm/mmu/mmu.c | 15 ++++++++------- > arch/x86/kvm/mmu/paging_tmpl.h | 2 +- > arch/x86/kvm/x86.c | 2 +- > 4 files changed, 20 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h > index 1d0c1904d69a..6ee4436e46f1 100644 > --- a/arch/x86/kvm/mmu.h > +++ b/arch/x86/kvm/mmu.h > @@ -116,6 +116,16 @@ static inline void kvm_mmu_load_pgd(struct kvm_vcpu *vcpu) > vcpu->arch.mmu->shadow_root_level); > } > > +extern unsigned long kvm_get_guest_cr3(struct kvm_vcpu *vcpu); No extern please, it's superfluous and against KVM style. Moot point though, see below. > +static inline unsigned long kvm_mmu_get_guest_pgd(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu) Wrap the params, no reason to make this line so long. > +{ > +#ifdef CONFIG_RETPOLINE > + if (mmu->get_guest_pgd == kvm_get_guest_cr3) > + return kvm_read_cr3(vcpu); This is unnecessarily fragile and confusing at first glance. Compilers are smart enough to generate a non-inline version of functions if they're used for function pointers, while still inlining where appropriate. In other words, just drop kvm_get_guest_cr3() entirely, a al get_pdptr => kvm_pdptr_read(). --- arch/x86/kvm/mmu.h | 6 +++--- arch/x86/kvm/mmu/mmu.c | 11 +++-------- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h index 3af66b9df640..50528d39de8d 100644 --- a/arch/x86/kvm/mmu.h +++ b/arch/x86/kvm/mmu.h @@ -117,11 +117,11 @@ static inline void kvm_mmu_load_pgd(struct kvm_vcpu *vcpu) vcpu->arch.mmu->shadow_root_level); } -extern unsigned long kvm_get_guest_cr3(struct kvm_vcpu *vcpu); -static inline unsigned long kvm_mmu_get_guest_pgd(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu) +static inline unsigned long kvm_mmu_get_guest_pgd(struct kvm_vcpu *vcpu, + struct kvm_mmu *mmu) { #ifdef CONFIG_RETPOLINE - if (mmu->get_guest_pgd == kvm_get_guest_cr3) + if (mmu->get_guest_pgd == kvm_read_cr3) return kvm_read_cr3(vcpu); #endif return mmu->get_guest_pgd(vcpu); diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 995c3450c20f..cc2414397e4b 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -4234,11 +4234,6 @@ void kvm_mmu_new_pgd(struct kvm_vcpu *vcpu, gpa_t new_pgd) } EXPORT_SYMBOL_GPL(kvm_mmu_new_pgd); -unsigned long kvm_get_guest_cr3(struct kvm_vcpu *vcpu) -{ - return kvm_read_cr3(vcpu); -} - static bool sync_mmio_spte(struct kvm_vcpu *vcpu, u64 *sptep, gfn_t gfn, unsigned int access) { @@ -4793,7 +4788,7 @@ static void init_kvm_tdp_mmu(struct kvm_vcpu *vcpu) context->invlpg = NULL; context->shadow_root_level = kvm_mmu_get_tdp_level(vcpu); context->direct_map = true; - context->get_guest_pgd = kvm_get_guest_cr3; + context->get_guest_pgd = kvm_read_cr3; context->get_pdptr = kvm_pdptr_read; context->inject_page_fault = kvm_inject_page_fault; context->root_level = role_regs_to_root_level(®s); @@ -4968,7 +4963,7 @@ static void init_kvm_softmmu(struct kvm_vcpu *vcpu) kvm_init_shadow_mmu(vcpu, ®s); - context->get_guest_pgd = kvm_get_guest_cr3; + context->get_guest_pgd = kvm_read_cr3; context->get_pdptr = kvm_pdptr_read; context->inject_page_fault = kvm_inject_page_fault; } @@ -5000,7 +4995,7 @@ static void init_kvm_nested_mmu(struct kvm_vcpu *vcpu) return; g_context->mmu_role.as_u64 = new_role.as_u64; - g_context->get_guest_pgd = kvm_get_guest_cr3; + g_context->get_guest_pgd = kvm_read_cr3; g_context->get_pdptr = kvm_pdptr_read; g_context->inject_page_fault = kvm_inject_page_fault; g_context->root_level = new_role.base.level; base-commit: c31df3e63672c14d8b52e34606c823e2166024b8 --
On 3/8/22 17:16, Sean Christopherson wrote: > >> +static inline unsigned long kvm_mmu_get_guest_pgd(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu) > Wrap the params, no reason to make this line so long. > >> +{ >> +#ifdef CONFIG_RETPOLINE >> + if (mmu->get_guest_pgd == kvm_get_guest_cr3) >> + return kvm_read_cr3(vcpu); > This is unnecessarily fragile and confusing at first glance. Compilers are smart > enough to generate a non-inline version of functions if they're used for function > pointers, while still inlining where appropriate. In other words, just drop > kvm_get_guest_cr3() entirely, a al get_pdptr => kvm_pdptr_read(). Unfortunately this isn't entirely true. The function pointer will not match between compilation units, in this case between the one that calls kvm_mmu_get_guest_pgd and the one that assigned kvm_read_cr3 to the function pointer. Paolo
On Tue, Mar 08, 2022, Paolo Bonzini wrote: > On 3/8/22 17:16, Sean Christopherson wrote: > > > > > +static inline unsigned long kvm_mmu_get_guest_pgd(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu) > > Wrap the params, no reason to make this line so long. > > > > > +{ > > > +#ifdef CONFIG_RETPOLINE > > > + if (mmu->get_guest_pgd == kvm_get_guest_cr3) > > > + return kvm_read_cr3(vcpu); > > This is unnecessarily fragile and confusing at first glance. Compilers are smart > > enough to generate a non-inline version of functions if they're used for function > > pointers, while still inlining where appropriate. In other words, just drop > > kvm_get_guest_cr3() entirely, a al get_pdptr => kvm_pdptr_read(). > > Unfortunately this isn't entirely true. The function pointer will not match > between compilation units, in this case between the one that calls > kvm_mmu_get_guest_pgd and the one that assigned kvm_read_cr3 to the function > pointer. Ooh, that's a nasty gotcha. And that's why your v1 used a NULL entry as a sentinel for rerouting to kvm_read_cr3(). Hrm, I'm torn between disliking the NULL behavior and disliking the subtle redirect :-) Aha! An idea that would provide line of sight to avoiding retpoline in all cases once we use static_call() for nested_ops, which I really want to do... Drop the mmu hook entirely and replace it with: static inline kvm_mmu_get_guest_pgd(struct kvm_vcpu *vcpu) { if (!mmu_is_nested(vcpu)) return kvm_read_cr3(vcpu); else return kvm_x86_ops.nested_ops->get_guest_pgd(vcpu); }
On 3/8/22 17:32, Sean Christopherson wrote: > > Aha! An idea that would provide line of sight to avoiding retpoline in all cases > once we use static_call() for nested_ops, which I really want to do... Drop the > mmu hook entirely and replace it with: > > static inline kvm_mmu_get_guest_pgd(struct kvm_vcpu *vcpu) > { > if (!mmu_is_nested(vcpu)) > return kvm_read_cr3(vcpu); > else > return kvm_x86_ops.nested_ops->get_guest_pgd(vcpu); > } Makes sense, but I think you mean static inline unsigned long kvm_mmu_get_guest_pgd(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu) { if (unlikely(vcpu == &vcpu->arch.guest_mmu)) return kvm_x86_ops.nested_ops->get_guest_pgd(vcpu); else return kvm_read_cr3(vcpu); } ? Paolo
On Tue, Mar 08, 2022, Paolo Bonzini wrote: > On 3/8/22 17:32, Sean Christopherson wrote: > > > > Aha! An idea that would provide line of sight to avoiding retpoline in all cases > > once we use static_call() for nested_ops, which I really want to do... Drop the > > mmu hook entirely and replace it with: > > > > static inline kvm_mmu_get_guest_pgd(struct kvm_vcpu *vcpu) > > { > > if (!mmu_is_nested(vcpu)) > > return kvm_read_cr3(vcpu); > > else > > return kvm_x86_ops.nested_ops->get_guest_pgd(vcpu); > > } > > Makes sense, but I think you mean > > static inline unsigned long kvm_mmu_get_guest_pgd(struct kvm_vcpu *vcpu, > struct kvm_mmu *mmu) > { > if (unlikely(vcpu == &vcpu->arch.guest_mmu)) Well, not that certainly :-) if (mmu == &vcpu->arch.guest_mmu) But you're right, we need to be able to do kvm_read_cr3() for the actual nested_mmu. > return kvm_x86_ops.nested_ops->get_guest_pgd(vcpu); > else > return kvm_read_cr3(vcpu); > } > > ?
On 3/8/22 17:53, Sean Christopherson wrote: >> static inline unsigned long kvm_mmu_get_guest_pgd(struct kvm_vcpu *vcpu, >> struct kvm_mmu *mmu) >> { >> if (unlikely(vcpu == &vcpu->arch.guest_mmu)) > Well, not that certainly:-) > > if (mmu == &vcpu->arch.guest_mmu) > > But you're right, we need to be able to do kvm_read_cr3() for the actual nested_mmu. > Ok, I'll drop this patch for now since the end of the series actually introduces the nested_ops already. And then we can do it for both get_guest_pgd and get_pdptr. Paolo
diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h index 1d0c1904d69a..6ee4436e46f1 100644 --- a/arch/x86/kvm/mmu.h +++ b/arch/x86/kvm/mmu.h @@ -116,6 +116,16 @@ static inline void kvm_mmu_load_pgd(struct kvm_vcpu *vcpu) vcpu->arch.mmu->shadow_root_level); } +extern unsigned long kvm_get_guest_cr3(struct kvm_vcpu *vcpu); +static inline unsigned long kvm_mmu_get_guest_pgd(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu) +{ +#ifdef CONFIG_RETPOLINE + if (mmu->get_guest_pgd == kvm_get_guest_cr3) + return kvm_read_cr3(vcpu); +#endif + return mmu->get_guest_pgd(vcpu); +} + struct kvm_page_fault { /* arguments to kvm_mmu_do_page_fault. */ const gpa_t addr; diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index b2c1c4eb6007..7051040e15b3 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -3435,7 +3435,7 @@ static int mmu_alloc_shadow_roots(struct kvm_vcpu *vcpu) unsigned i; int r; - root_pgd = mmu->get_guest_pgd(vcpu); + root_pgd = kvm_mmu_get_guest_pgd(vcpu, mmu); root_gfn = root_pgd >> PAGE_SHIFT; if (mmu_check_root(vcpu, root_gfn)) @@ -3854,12 +3854,13 @@ static void shadow_page_table_clear_flood(struct kvm_vcpu *vcpu, gva_t addr) static bool kvm_arch_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, gfn_t gfn) { + struct kvm_mmu *mmu = vcpu->arch.mmu; struct kvm_arch_async_pf arch; arch.token = (vcpu->arch.apf.id++ << 12) | vcpu->vcpu_id; arch.gfn = gfn; - arch.direct_map = vcpu->arch.mmu->direct_map; - arch.cr3 = vcpu->arch.mmu->get_guest_pgd(vcpu); + arch.direct_map = mmu->direct_map; + arch.cr3 = kvm_mmu_get_guest_pgd(vcpu, mmu); return kvm_setup_async_pf(vcpu, cr2_or_gpa, kvm_vcpu_gfn_to_hva(vcpu, gfn), &arch); @@ -4208,7 +4209,7 @@ void kvm_mmu_new_pgd(struct kvm_vcpu *vcpu, gpa_t new_pgd) } EXPORT_SYMBOL_GPL(kvm_mmu_new_pgd); -static unsigned long get_cr3(struct kvm_vcpu *vcpu) +unsigned long kvm_get_guest_cr3(struct kvm_vcpu *vcpu) { return kvm_read_cr3(vcpu); } @@ -4767,7 +4768,7 @@ static void init_kvm_tdp_mmu(struct kvm_vcpu *vcpu) context->invlpg = NULL; context->shadow_root_level = kvm_mmu_get_tdp_level(vcpu); context->direct_map = true; - context->get_guest_pgd = get_cr3; + context->get_guest_pgd = kvm_get_guest_cr3; context->get_pdptr = kvm_pdptr_read; context->inject_page_fault = kvm_inject_page_fault; context->root_level = role_regs_to_root_level(®s); @@ -4942,7 +4943,7 @@ static void init_kvm_softmmu(struct kvm_vcpu *vcpu) kvm_init_shadow_mmu(vcpu, ®s); - context->get_guest_pgd = get_cr3; + context->get_guest_pgd = kvm_get_guest_cr3; context->get_pdptr = kvm_pdptr_read; context->inject_page_fault = kvm_inject_page_fault; } @@ -4974,7 +4975,7 @@ static void init_kvm_nested_mmu(struct kvm_vcpu *vcpu) return; g_context->mmu_role.as_u64 = new_role.as_u64; - g_context->get_guest_pgd = get_cr3; + g_context->get_guest_pgd = kvm_get_guest_cr3; g_context->get_pdptr = kvm_pdptr_read; g_context->inject_page_fault = kvm_inject_page_fault; g_context->root_level = new_role.base.level; diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h index 252c77805eb9..80b4b291002a 100644 --- a/arch/x86/kvm/mmu/paging_tmpl.h +++ b/arch/x86/kvm/mmu/paging_tmpl.h @@ -362,7 +362,7 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker, trace_kvm_mmu_pagetable_walk(addr, access); retry_walk: walker->level = mmu->root_level; - pte = mmu->get_guest_pgd(vcpu); + pte = kvm_mmu_get_guest_pgd(vcpu, mmu); have_ad = PT_HAVE_ACCESSED_DIRTY(mmu); #if PTTYPE == 64 diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 6552360d8888..da33d3a88a8d 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -12190,7 +12190,7 @@ void kvm_arch_async_page_ready(struct kvm_vcpu *vcpu, struct kvm_async_pf *work) return; if (!vcpu->arch.mmu->direct_map && - work->arch.cr3 != vcpu->arch.mmu->get_guest_pgd(vcpu)) + work->arch.cr3 != kvm_mmu_get_guest_pgd(vcpu, vcpu->arch.mmu)) return; kvm_mmu_do_page_fault(vcpu, work->cr2_or_gpa, 0, true);
Most of the time, calls to get_guest_pgd result in calling kvm_read_cr3 (the exception is only nested TDP). Check if that is the case if retpolines are enabled, thus avoiding an expensive indirect call. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- arch/x86/kvm/mmu.h | 10 ++++++++++ arch/x86/kvm/mmu/mmu.c | 15 ++++++++------- arch/x86/kvm/mmu/paging_tmpl.h | 2 +- arch/x86/kvm/x86.c | 2 +- 4 files changed, 20 insertions(+), 9 deletions(-)