[PATCH] perf: arm_spe: Add barrier before enabling profiling buffer

[Will Deacon]

On Fri, Jan 30, 2026 at 08:24:37PM +0000, Leo Yan wrote:
> On Fri, Jan 23, 2026 at 04:03:53PM +0000, James Clark wrote:
> > The Arm ARM known issues document [1] states that the architecture will
> > be relaxed so that the profiling buffer must be correctly configured
> > when ProfilingBufferEnabled() && !SPEProfilingStopped() &&
> > PMBLIMITR_EL1.FM != DISCARD:
> > 
> >   R24557
> > 
> >   While the Profiling Buffer is enabled, profiling is not stopped, and
> >   Discard mode is not enabled, all of the following must be true:
> > 
> >   * The current write pointer must be at least one sample record below
> >     the write limit pointer.
> > 
> > The same relaxation also says that writes may be completely ignored:
> > 
> >   When the Profiling Buffer is enabled, profiling is not stopped, and
> >   Discard mode is not enabled, the PE might ignore a direct write to any
> >   of the following Profiling Buffer registers, other than a direct write
> >   to PMBLIMITR_EL1 that clears PMBLIMITR_EL1.E from 1 to 0:
> > 
> >   * The current write pointer, PMBPTR_EL1.
> >   * The Limit pointer, PMBLIMITR_EL1.
> >   * PMBSR_EL1.

Oh nice, since when was it ok to relax the architecture and break
existing drivers that were perfectly fine before? The SPE spec's not
worth the paper it's written on...

Anyway, we're not changing the driver without a comment next to the new
isb() explaining the backwards incompatible change.

> > When arm_spe_pmu_start() occurs, SPEProfilingStopped() is false
> > (PMBSR_EL1.S == 0) meaning that the write to PMBLIMITR_EL1 now becomes
> > the point where the buffer configuration must be correct by, rather than
> > the "When profiling becomes enabled" (StatisticalProfilingEnabled())
> > from the old rule which is much later when PMSCR_EL1 is written.
> > 
> > If the writes to PMBLIMITR_EL1 and PMBPTR_EL1 are re-ordered then a
> > misconfigured state could be observed, resulting in a buffer management
> > event. Or the write to PMBPTR_EL1 could be ignored.
> > 
> > Fix it by adding an isb() after the write to PMBPTR_EL1 to ensure that
> > this completes before enabling the buffer.
> 
> Makes sense.
> 
> > To avoid redundant isb()s in the IRQ handler, remove the isb() between
> > the PMBLIMITR_EL1 write and SYS_PMBSR_EL1 as it doesn't matter which
> > order these happen in now that all the previous configuration is covered
> > by the new isb().
> 
> The isb() in the interrupt handler is useful and should not be removed.
> 
> See the sequence in the interrupt handler:
> 
>     arm_spe_perf_aux_output_begin() {
>         write_sysreg_s(base, SYS_PMBPTR_EL1);
> 
>         // Ensure the write pointer is ordered
>         isb();
> 
>         write_sysreg_s(limit, SYS_PMBLIMITR_EL1);
>     }
> 
>     // Ensure the limit pointer is ordered
>     isb();
> 
>     // Profiling is enabled:
>     //   (PMBLIMITR_EL1.E==1) && (PMBSR_EL1.S==0) && (not discard mode)
>     write_sysreg_s(0, SYS_PMBSR_EL1);
> 
> The first isb() ensures that the write pointer update is ordered.  The
> second isb() ensures that the limit pointer is visible before profiling
> is enabled by clearing the PMBSR_EL1.S bit.  When handling a normal
> maintenance interrupt, PMBSR_EL1.S is set by the SPE to stop tracing,
> while PMBLIMITR_EL1.E remains set.  Clearing PMBSR_EL1.S therefore
> effectively re-enables profiling.
> 
> Since the second isb() is a synchronization for both the write pointer
> and the limit pointer before profiling is enabled, it could be argued
> that the first isb() is redundant in the interrupt handling.  However,
> the first isb() is crucial for the arm_spe_pmu_start() case, and keeping
> it in the interrupt handler does no harm and simplifies code maintenance.
> 
> In short, if preserves the second isb() instead of removing it, the
> change looks good to me.

I'm not sure I follow your logic as to why both ISBs are required, but
I'd have thought that if perf_aux_output_begin() fails when called from
arm_spe_perf_aux_output_begin() in the irqhandler, we need the ISB
because we're going to clear pmblimitr_el1 to 0 and that surely has
to be ordered before clearing pmbsr?

Will