On 14/12/2023 12:30, Robin Murphy wrote:

On 2023-12-14 12:13 pm, Will Deacon wrote:

quoted

On Thu, Dec 14, 2023 at 11:53:52AM +0000, Ryan Roberts wrote:

quoted

On 12/12/2023 11:47, Ryan Roberts wrote:

quoted

On 12/12/2023 11:35, Will Deacon wrote:

quoted

On Mon, Dec 04, 2023 at 10:54:37AM +0000, Ryan Roberts wrote:

quoted

diff --git a/arch/arm64/include/asm/tlbflush.h

b/arch/arm64/include/asm/tlbflush.h
index bb2c2833a987..925ef3bdf9ed 100644

--- a/arch/arm64/include/asm/tlbflush.h
+++ b/arch/arm64/include/asm/tlbflush.h

@@ -399,7 +399,7 @@ do {                                    \

  #define __flush_s2_tlb_range_op(op, start, pages, stride, tlb_level) \
      __flush_tlb_range_op(op, start, pages, stride, 0, tlb_level, false)
  -static inline void __flush_tlb_range(struct vm_area_struct *vma,
+static inline void __flush_tlb_range_nosync(struct vm_area_struct *vma,
                       unsigned long start, unsigned long end,
                       unsigned long stride, bool last_level,
                       int tlb_level)

@@ -431,10 +431,19 @@ static inline void __flush_tlb_range(struct

vm_area_struct *vma,
      else
          __flush_tlb_range_op(vae1is, start, pages, stride, asid,
tlb_level, true);
  -    dsb(ish);
      mmu_notifier_arch_invalidate_secondary_tlbs(vma->vm_mm, start, end);
  }
  +static inline void __flush_tlb_range(struct vm_area_struct *vma,
+                     unsigned long start, unsigned long end,
+                     unsigned long stride, bool last_level,
+                     int tlb_level)
+{
+    __flush_tlb_range_nosync(vma, start, end, stride,
+                 last_level, tlb_level);
+    dsb(ish);
+}

Hmm, are you sure it's safe to defer the DSB until after the secondary TLB
invalidation? It will have a subtle effect on e.g. an SMMU participating
in broadcast TLB maintenance, because now the ATC will be invalidated
before completion of the TLB invalidation and it's not obviously safe to me.

I'll be honest; I don't know that it's safe. The notifier calls turned up
during
a rebase and I stared at it for a while, before eventually concluding that I
should just follow the existing pattern in __flush_tlb_page_nosync(): That one
calls the mmu notifier without the dsb, then flush_tlb_page() does the dsb
after. So I assumed it was safe.

If you think it's not safe, I guess there is a bug to fix in
__flush_tlb_page_nosync()?

Did you have an opinion on this? I'm just putting together a v4 of this series,
and I'll remove this optimization if you think it's unsound. But in that case, I
guess we have an existing bug to fix too?

Sorry, Ryan, I've not had a chance to look into it in more detail. But as
you rightly point out, you're not introducing the issue (assuming it is
one), so I don't think it needs to hold you up. Your code just makes the
thing more "obvious" to me.

OK thanks. I'll leave my code as is for now then - that makes it easier to do
A/B performance comparison with the existing code. And I can change it if/when
mainline changes (presumably to add the dsb between the tlbi and the mmu
notifier callback).

quoted

Robin, Jean-Philippe -- do we need to make sure that the SMMU has completed
its TLB invalidation before issuing an ATC invalidate? My half-baked worry
is whether or not an ATS request could refill the ATC before the TLBI
has completed, therefore rendering the ATC invalidation useless.

I would agree, and the spec for CMD_ATC_INV does call out a
TLBI->sync->ATCI->sync sequence. At the moment the SVA notifier is issuing its
own command-based TLBIs anyway so the necessary sync is implicit there, but if
and when we get BTM support wired up properly it would be nice not to have to
bodge in an additional sync/DSB.

Cheers,
Robin.

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