| Message ID | 20230414172922.812640-7-rananta@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Add support for FEAT_TLBIRANGE | expand |
Hi Raghavendra, On Fri, Apr 14, 2023 at 05:29:21PM +0000, Raghavendra Rao Ananta wrote: > Add a 'skip_flush' argument in stage2_put_pte() to > control the TLB invalidations. This will be leveraged > by the upcoming patch to defer the individual PTE > invalidations until the entire walk is finished. > > No functional change intended. > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> > --- > arch/arm64/kvm/hyp/pgtable.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > index b8f0dbd12f773..3f136e35feb5e 100644 > --- a/arch/arm64/kvm/hyp/pgtable.c > +++ b/arch/arm64/kvm/hyp/pgtable.c > @@ -772,7 +772,7 @@ static void stage2_make_pte(const struct kvm_pgtable_visit_ctx *ctx, kvm_pte_t n > } > > static void stage2_put_pte(const struct kvm_pgtable_visit_ctx *ctx, struct kvm_s2_mmu *mmu, > - struct kvm_pgtable_mm_ops *mm_ops) > + struct kvm_pgtable_mm_ops *mm_ops, bool skip_flush) Assuming you are going to pull the cpufeature checks into this helper, it might me helpful to narrow the scope of it. 'stage2_put_pte()' sounds very generic, but it is about to have a very precise meaning in relation to kvm_pgtable_stage2_unmap(). So maybe stage2_unmap_put_pte()? While at it, you'd want to have a shared helper for the deferral check: static bool stage2_unmap_defer_tlb_flush(struct kvm_pgtable *pgt) { /* your blurb for why FWB is required too */ return system_supports_tlb_range() && stage2_has_fwb(pgt); } The 'flush' part is annoying, because the exact term is an invalidation, but we already have that pattern in all of our TLB invalidation helpers. > { > /* > * Clear the existing PTE, and perform break-before-make with > @@ -780,7 +780,10 @@ static void stage2_put_pte(const struct kvm_pgtable_visit_ctx *ctx, struct kvm_s > */ > if (kvm_pte_valid(ctx->old)) { > kvm_clear_pte(ctx->ptep); > - kvm_call_hyp(__kvm_tlb_flush_vmid_ipa, mmu, ctx->addr, ctx->level); > + > + if (!skip_flush) > + kvm_call_hyp(__kvm_tlb_flush_vmid_ipa, mmu, > + ctx->addr, ctx->level); > } > > mm_ops->put_page(ctx->ptep); > @@ -1015,7 +1018,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); > + stage2_put_pte(ctx, mmu, mm_ops, false); > > if (need_flush && mm_ops->dcache_clean_inval_poc) > mm_ops->dcache_clean_inval_poc(kvm_pte_follow(ctx->old, mm_ops), > -- > 2.40.0.634.g4ca3ef3211-goog >
Hi Oliver, On Fri, May 12, 2023 at 10:21 AM Oliver Upton <oliver.upton@linux.dev> wrote: > > Hi Raghavendra, > > On Fri, Apr 14, 2023 at 05:29:21PM +0000, Raghavendra Rao Ananta wrote: > > Add a 'skip_flush' argument in stage2_put_pte() to > > control the TLB invalidations. This will be leveraged > > by the upcoming patch to defer the individual PTE > > invalidations until the entire walk is finished. > > > > No functional change intended. > > > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> > > --- > > arch/arm64/kvm/hyp/pgtable.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > > index b8f0dbd12f773..3f136e35feb5e 100644 > > --- a/arch/arm64/kvm/hyp/pgtable.c > > +++ b/arch/arm64/kvm/hyp/pgtable.c > > @@ -772,7 +772,7 @@ static void stage2_make_pte(const struct kvm_pgtable_visit_ctx *ctx, kvm_pte_t n > > } > > > > static void stage2_put_pte(const struct kvm_pgtable_visit_ctx *ctx, struct kvm_s2_mmu *mmu, > > - struct kvm_pgtable_mm_ops *mm_ops) > > + struct kvm_pgtable_mm_ops *mm_ops, bool skip_flush) > > Assuming you are going to pull the cpufeature checks into this helper, > it might me helpful to narrow the scope of it. 'stage2_put_pte()' sounds > very generic, but it is about to have a very precise meaning in relation > to kvm_pgtable_stage2_unmap(). > > So maybe stage2_unmap_put_pte()? While at it, you'd want to have a > shared helper for the deferral check: > Yeah, stage2_unmap_put_pte() sounds better. I'll change that. > static bool stage2_unmap_defer_tlb_flush(struct kvm_pgtable *pgt) > { > /* your blurb for why FWB is required too */ > return system_supports_tlb_range() && stage2_has_fwb(pgt); > } > Good idea; I can introduce the helper, now that we'll get rid of stage2_unmap_data.skip_pte_tlbis (patch 7/7) as per your comments. Also, since we are now making stage2_put_pte() specific to unmap, I can also get rid of the 'skip_flush' arg and call stage2_unmap_defer_tlb_flush() directly, or do you have a preference for the additional arg? Thank you. Raghavendra > The 'flush' part is annoying, because the exact term is an invalidation, > but we already have that pattern in all of our TLB invalidation helpers. > > > { > > /* > > * Clear the existing PTE, and perform break-before-make with > > @@ -780,7 +780,10 @@ static void stage2_put_pte(const struct kvm_pgtable_visit_ctx *ctx, struct kvm_s > > */ > > if (kvm_pte_valid(ctx->old)) { > > kvm_clear_pte(ctx->ptep); > > - kvm_call_hyp(__kvm_tlb_flush_vmid_ipa, mmu, ctx->addr, ctx->level); > > + > > + if (!skip_flush) > > + kvm_call_hyp(__kvm_tlb_flush_vmid_ipa, mmu, > > + ctx->addr, ctx->level); > > } > > > > mm_ops->put_page(ctx->ptep); > > @@ -1015,7 +1018,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); > > + stage2_put_pte(ctx, mmu, mm_ops, false); > > > > if (need_flush && mm_ops->dcache_clean_inval_poc) > > mm_ops->dcache_clean_inval_poc(kvm_pte_follow(ctx->old, mm_ops), > > -- > > 2.40.0.634.g4ca3ef3211-goog > > > > -- > Thanks, > Oliver
diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c index b8f0dbd12f773..3f136e35feb5e 100644 --- a/arch/arm64/kvm/hyp/pgtable.c +++ b/arch/arm64/kvm/hyp/pgtable.c @@ -772,7 +772,7 @@ static void stage2_make_pte(const struct kvm_pgtable_visit_ctx *ctx, kvm_pte_t n } static void stage2_put_pte(const struct kvm_pgtable_visit_ctx *ctx, struct kvm_s2_mmu *mmu, - struct kvm_pgtable_mm_ops *mm_ops) + struct kvm_pgtable_mm_ops *mm_ops, bool skip_flush) { /* * Clear the existing PTE, and perform break-before-make with @@ -780,7 +780,10 @@ static void stage2_put_pte(const struct kvm_pgtable_visit_ctx *ctx, struct kvm_s */ if (kvm_pte_valid(ctx->old)) { kvm_clear_pte(ctx->ptep); - kvm_call_hyp(__kvm_tlb_flush_vmid_ipa, mmu, ctx->addr, ctx->level); + + if (!skip_flush) + kvm_call_hyp(__kvm_tlb_flush_vmid_ipa, mmu, + ctx->addr, ctx->level); } mm_ops->put_page(ctx->ptep); @@ -1015,7 +1018,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); + stage2_put_pte(ctx, mmu, mm_ops, false); if (need_flush && mm_ops->dcache_clean_inval_poc) mm_ops->dcache_clean_inval_poc(kvm_pte_follow(ctx->old, mm_ops),
Add a 'skip_flush' argument in stage2_put_pte() to control the TLB invalidations. This will be leveraged by the upcoming patch to defer the individual PTE invalidations until the entire walk is finished. No functional change intended. Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> --- arch/arm64/kvm/hyp/pgtable.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)