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

[Will Deacon <will@kernel.org>]

On Tue, Feb 03, 2026 at 09:29:56AM +0000, James Clark wrote:
> 
> 
> On 02/02/2026 7:14 pm, Leo Yan wrote:
> > On Mon, Feb 02, 2026 at 06:57:11PM +0000, Will Deacon wrote:
> > 
> > [...]
> > 
> > 
> > > > > 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."
> > > 
> > > Hmm, so let's say we've executed the first ISB. At that point, the
> > > Profiling Buffer is disabled (PMBLIMITR_EL1.E = 0) and profiling is
> > > stopped (PMBSR_EL1.S = 1).
> > 
> > This is not true.  PMBLIMITR_EL1.E is always 1 during interrupt
> > handling.

Ah, yes, thank you for correcting me here.

> > > If we *don't* have the second ISB then either
> > > PMBLIMITR_EL1 is written first or PMBSR_EL1 is written first. But the
> > > text you quoted will only come into effect once they've both happened,
> > > right? In which case, why does the order matter for the success case?
> > 
> > Yes, both PMBLIMITR_EL1.E == 1 and PMBSR_EL1.S == 0 must be true to
> > enable tracing.
> > 
> > However, the tricky part is that PMBLIMITR_EL1.E remains 1 throughout
> > the sequence.  Writing PMBLIMITR_EL1 effectively only sets the limit,
> > while clearing PMBSR_EL1 is the distinct step that enables tracing.
> > 
> > Thanks,
> > Leo
> 
> I think Leo is correct that the old isb() is still needed. I removed it
> under the assumption that PMBLIMITR_EL1.E was unset in the interrupt
> handler. Possibly because the previous version re-arranged the handler to do
> that.
> 
> If PMBLIMITR_EL1.E is set, we have to make sure clearing PMBSR_EL1 comes
> last as it's the thing that defines the point where both pointers must be
> correct by.

Agreed that we can't remove the existing isb, but please see my other
reply as I'm not entirely sure we need to add an extra isb to handle the
arch relaxation.

Will