Re: [PATCH v3 6/7] KVM: arm64: Add 'skip_flush' arg to stage2_put_pte()

Hi Oliver,

On Fri, May 12, 2023 at 10:21 AM Oliver Upton [off-list ref] wrote:

Hi Raghavendra,

On Fri, Apr 14, 2023 at 05:29:21PM +0000, Raghavendra Rao Ananta wrote:

quoted

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 <redacted>
---
 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.

quoted

 {
      /*
       * 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel