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

[Leo Yan <leo.yan@arm.com>]

On Mon, Feb 02, 2026 at 04:53:57PM +0000, Will Deacon wrote:

[...]

> > > 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?

I think the ISB after arm_spe_perf_aux_output_begin() in the irq
handler is required for both the failure and success cases.

For a normal maintenance interrupt, an ISB is inserted between writing
PMBLIMITR_EL1 and PMBSR_EL1 to ensure that a valid limit write is
visible before tracing restarts.  This ensures that the following
conditions are safely met:

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

   PMBPTR_EL1.PTR[63:56] must equal PMBLIMITR_EL1.LIMIT[63:56],
   regardless of the value of the applicable TBI bit."

Thanks,
Leo