| Message ID | 20230414172922.812640-8-rananta@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Add support for FEAT_TLBIRANGE | expand |
On Fri, May 12, 2023 at 10:02 AM Oliver Upton <oliver.upton@linux.dev> wrote: > > Hi Raghavendra, > > On Fri, Apr 14, 2023 at 05:29:22PM +0000, Raghavendra Rao Ananta wrote: > > The current implementation of the stage-2 unmap walker traverses > > the given range and, as a part of break-before-make, performs > > TLB invalidations with a DSB for every PTE. A multitude of this > > combination could cause a performance bottleneck. > > > > Hence, if the system supports FEAT_TLBIRANGE, defer the TLB > > invalidations until the entire walk is finished, and then > > use range-based instructions to invalidate the TLBs in one go. > > Condition this upon S2FWB in order to avoid walking the page-table > > again to perform the CMOs after issuing the TLBI. > > > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> > > Suggested-by: Oliver Upton <oliver.upton@linux.dev> > > --- > > arch/arm64/kvm/hyp/pgtable.c | 33 +++++++++++++++++++++++++++++---- > > 1 file changed, 29 insertions(+), 4 deletions(-) > > > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > > index 3f136e35feb5e..bcb748e3566c7 100644 > > --- a/arch/arm64/kvm/hyp/pgtable.c > > +++ b/arch/arm64/kvm/hyp/pgtable.c > > @@ -987,10 +987,16 @@ int kvm_pgtable_stage2_set_owner(struct kvm_pgtable *pgt, u64 addr, u64 size, > > return ret; > > } > > > > +struct stage2_unmap_data { > > + struct kvm_pgtable *pgt; > > + bool skip_pte_tlbis; > > +}; > > + > > static int stage2_unmap_walker(const struct kvm_pgtable_visit_ctx *ctx, > > enum kvm_pgtable_walk_flags visit) > > { > > - struct kvm_pgtable *pgt = ctx->arg; > > + struct stage2_unmap_data *unmap_data = ctx->arg; > > + struct kvm_pgtable *pgt = unmap_data->pgt; > > struct kvm_s2_mmu *mmu = pgt->mmu; > > struct kvm_pgtable_mm_ops *mm_ops = ctx->mm_ops; > > kvm_pte_t *childp = NULL; > > @@ -1018,7 +1024,7 @@ static int stage2_unmap_walker(const struct kvm_pgtable_visit_ctx *ctx, > > * block entry and rely on the remaining portions being faulted > > * back lazily. > > */ > > - stage2_put_pte(ctx, mmu, mm_ops, false); > > + stage2_put_pte(ctx, mmu, mm_ops, unmap_data->skip_pte_tlbis); > > > > if (need_flush && mm_ops->dcache_clean_inval_poc) > > mm_ops->dcache_clean_inval_poc(kvm_pte_follow(ctx->old, mm_ops), > > @@ -1032,13 +1038,32 @@ static int stage2_unmap_walker(const struct kvm_pgtable_visit_ctx *ctx, > > > > int kvm_pgtable_stage2_unmap(struct kvm_pgtable *pgt, u64 addr, u64 size) > > { > > + int ret; > > + struct stage2_unmap_data unmap_data = { > > + .pgt = pgt, > > + /* > > + * If FEAT_TLBIRANGE is implemented, defer the individial PTE > > + * TLB invalidations until the entire walk is finished, and > > + * then use the range-based TLBI instructions to do the > > + * invalidations. Condition this upon S2FWB in order to avoid > > + * a page-table walk again to perform the CMOs after TLBI. > > + */ > > + .skip_pte_tlbis = system_supports_tlb_range() && > > + stage2_has_fwb(pgt), > > Why can't the underlying walker just call these two helpers directly? > There are static keys behind these... > I wasn't aware of that. Although, I tried to look into the definitions, but couldn't understand how static keys are at play here. By any chance are you referring to the alternative_has_feature_*() implementations when checking for cpu capabilities? Thank you. Raghavendra > > + }; > > struct kvm_pgtable_walker walker = { > > .cb = stage2_unmap_walker, > > - .arg = pgt, > > + .arg = &unmap_data, > > .flags = KVM_PGTABLE_WALK_LEAF | KVM_PGTABLE_WALK_TABLE_POST, > > }; > > > > - return kvm_pgtable_walk(pgt, addr, size, &walker); > > + ret = kvm_pgtable_walk(pgt, addr, size, &walker); > > + if (unmap_data.skip_pte_tlbis) > > + /* Perform the deferred TLB invalidations */ > > + kvm_call_hyp(__kvm_tlb_flush_vmid_range, pgt->mmu, > > + addr, addr + size); > > + > > + return ret; > > } > > > > struct stage2_attr_data { > > -- > > 2.40.0.634.g4ca3ef3211-goog > > > > -- > Thanks, > Oliver
On Tue, May 16, 2023 at 10:21:33AM -0700, Raghavendra Rao Ananta wrote: > On Fri, May 12, 2023 at 10:02 AM Oliver Upton <oliver.upton@linux.dev> wrote: > > > int kvm_pgtable_stage2_unmap(struct kvm_pgtable *pgt, u64 addr, u64 size) > > > { > > > + int ret; > > > + struct stage2_unmap_data unmap_data = { > > > + .pgt = pgt, > > > + /* > > > + * If FEAT_TLBIRANGE is implemented, defer the individial PTE > > > + * TLB invalidations until the entire walk is finished, and > > > + * then use the range-based TLBI instructions to do the > > > + * invalidations. Condition this upon S2FWB in order to avoid > > > + * a page-table walk again to perform the CMOs after TLBI. > > > + */ > > > + .skip_pte_tlbis = system_supports_tlb_range() && > > > + stage2_has_fwb(pgt), > > > > Why can't the underlying walker just call these two helpers directly? > > There are static keys behind these... > > > I wasn't aware of that. Although, I tried to look into the > definitions, but couldn't understand how static keys are at play here. > By any chance are you referring to the alternative_has_feature_*() > implementations when checking for cpu capabilities? Ah, right, these were recently changed to rely on alternative patching in commit 21fb26bfb01f ("arm64: alternatives: add alternative_has_feature_*()"). Even still, the significance remains as the alternative patching completely eliminates a conditional branch on the presence of a particular feature. Initializing a local with the presence/absence of a feature defeats such an optimization.
On Tue, May 16, 2023 at 11:46 AM Oliver Upton <oliver.upton@linux.dev> wrote: > > On Tue, May 16, 2023 at 10:21:33AM -0700, Raghavendra Rao Ananta wrote: > > On Fri, May 12, 2023 at 10:02 AM Oliver Upton <oliver.upton@linux.dev> wrote: > > > > int kvm_pgtable_stage2_unmap(struct kvm_pgtable *pgt, u64 addr, u64 size) > > > > { > > > > + int ret; > > > > + struct stage2_unmap_data unmap_data = { > > > > + .pgt = pgt, > > > > + /* > > > > + * If FEAT_TLBIRANGE is implemented, defer the individial PTE > > > > + * TLB invalidations until the entire walk is finished, and > > > > + * then use the range-based TLBI instructions to do the > > > > + * invalidations. Condition this upon S2FWB in order to avoid > > > > + * a page-table walk again to perform the CMOs after TLBI. > > > > + */ > > > > + .skip_pte_tlbis = system_supports_tlb_range() && > > > > + stage2_has_fwb(pgt), > > > > > > Why can't the underlying walker just call these two helpers directly? > > > There are static keys behind these... > > > > > I wasn't aware of that. Although, I tried to look into the > > definitions, but couldn't understand how static keys are at play here. > > By any chance are you referring to the alternative_has_feature_*() > > implementations when checking for cpu capabilities? > > Ah, right, these were recently changed to rely on alternative patching > in commit 21fb26bfb01f ("arm64: alternatives: add alternative_has_feature_*()"). > Even still, the significance remains as the alternative patching > completely eliminates a conditional branch on the presence of a > particular feature. > > Initializing a local with the presence/absence of a feature defeats such > an optimization. > Thanks for the info! Introduction of stage2_unmap_defer_tlb_flush() (in patch-7/7) would call these functions as needed and get rid of 'skip_pte_tlbis'. - Raghavendra > -- > Thanks, > Oliver
diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c index 3f136e35feb5e..bcb748e3566c7 100644 --- a/arch/arm64/kvm/hyp/pgtable.c +++ b/arch/arm64/kvm/hyp/pgtable.c @@ -987,10 +987,16 @@ int kvm_pgtable_stage2_set_owner(struct kvm_pgtable *pgt, u64 addr, u64 size, return ret; } +struct stage2_unmap_data { + struct kvm_pgtable *pgt; + bool skip_pte_tlbis; +}; + static int stage2_unmap_walker(const struct kvm_pgtable_visit_ctx *ctx, enum kvm_pgtable_walk_flags visit) { - struct kvm_pgtable *pgt = ctx->arg; + struct stage2_unmap_data *unmap_data = ctx->arg; + struct kvm_pgtable *pgt = unmap_data->pgt; struct kvm_s2_mmu *mmu = pgt->mmu; struct kvm_pgtable_mm_ops *mm_ops = ctx->mm_ops; kvm_pte_t *childp = NULL; @@ -1018,7 +1024,7 @@ static int stage2_unmap_walker(const struct kvm_pgtable_visit_ctx *ctx, * block entry and rely on the remaining portions being faulted * back lazily. */ - stage2_put_pte(ctx, mmu, mm_ops, false); + stage2_put_pte(ctx, mmu, mm_ops, unmap_data->skip_pte_tlbis); if (need_flush && mm_ops->dcache_clean_inval_poc) mm_ops->dcache_clean_inval_poc(kvm_pte_follow(ctx->old, mm_ops), @@ -1032,13 +1038,32 @@ static int stage2_unmap_walker(const struct kvm_pgtable_visit_ctx *ctx, int kvm_pgtable_stage2_unmap(struct kvm_pgtable *pgt, u64 addr, u64 size) { + int ret; + struct stage2_unmap_data unmap_data = { + .pgt = pgt, + /* + * If FEAT_TLBIRANGE is implemented, defer the individial PTE + * TLB invalidations until the entire walk is finished, and + * then use the range-based TLBI instructions to do the + * invalidations. Condition this upon S2FWB in order to avoid + * a page-table walk again to perform the CMOs after TLBI. + */ + .skip_pte_tlbis = system_supports_tlb_range() && + stage2_has_fwb(pgt), + }; struct kvm_pgtable_walker walker = { .cb = stage2_unmap_walker, - .arg = pgt, + .arg = &unmap_data, .flags = KVM_PGTABLE_WALK_LEAF | KVM_PGTABLE_WALK_TABLE_POST, }; - return kvm_pgtable_walk(pgt, addr, size, &walker); + ret = kvm_pgtable_walk(pgt, addr, size, &walker); + if (unmap_data.skip_pte_tlbis) + /* Perform the deferred TLB invalidations */ + kvm_call_hyp(__kvm_tlb_flush_vmid_range, pgt->mmu, + addr, addr + size); + + return ret; } struct stage2_attr_data {
The current implementation of the stage-2 unmap walker traverses the given range and, as a part of break-before-make, performs TLB invalidations with a DSB for every PTE. A multitude of this combination could cause a performance bottleneck. Hence, if the system supports FEAT_TLBIRANGE, defer the TLB invalidations until the entire walk is finished, and then use range-based instructions to invalidate the TLBs in one go. Condition this upon S2FWB in order to avoid walking the page-table again to perform the CMOs after issuing the TLBI. Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> Suggested-by: Oliver Upton <oliver.upton@linux.dev> --- arch/arm64/kvm/hyp/pgtable.c | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-)