[PATCH v2 RESEND] perf: arm_spe: Relax period restriction

[Leo Yan]

On Fri, Jun 27, 2025 at 05:10:47PM +0530, Anshuman Khandual wrote:
> On 27/06/25 2:46 PM, Leo Yan wrote:
> > The minimum interval specified the PMSIDR_EL1.Interval field is a
> > hardware recommendation. However, this value is set by hardware designer
> > before the production. It may not accurately reflects actual hardware
> > limitations, and tools currently have no way to test shorter periods.
> > 
> > This change relaxes the limitation by allowing any non-zero periods.
> > This gives chance for experimenting smaller periods.
> 
> Is this going to be safe on all SPE implementations ?

AFAIK, this change applies on all Arm SPE versions.

> > The downside is that small periods may increase the risk of AUX ring
> > buffer overruns. When an overrun occurs, the perf core layer will
> > trigger an irq work to disable the event and wake up the tool in user
> > space to read the trace data. After the tool finishes reading, it will
> > re-enable the AUX event.
> > 
> > Reviewed-by: James Clark <james.clark at linaro.org>
> > Signed-off-by: Leo Yan <leo.yan at arm.com>
> > ---
> >  drivers/perf/arm_spe_pmu.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> > index 3efed8839a4e..e40e5daa838d 100644
> > --- a/drivers/perf/arm_spe_pmu.c
> > +++ b/drivers/perf/arm_spe_pmu.c
> > @@ -308,12 +308,16 @@ static u64 arm_spe_event_to_pmscr(struct perf_event *event)
> >  
> >  static void arm_spe_event_sanitise_period(struct perf_event *event)
> >  {
> > -	struct arm_spe_pmu *spe_pmu = to_spe_pmu(event->pmu);
> >  	u64 period = event->hw.sample_period;
> >  	u64 max_period = PMSIRR_EL1_INTERVAL_MASK;
> >  
> > -	if (period < spe_pmu->min_period)
> > -		period = spe_pmu->min_period;
> > +	/*
> > +	 * As per the Arm ARM (DDI 0487 L.a), section D24.7.12 PMSIRR_EL1,
> > +	 * Sampling Interval Reload Register, the INTERVAL field (bits [31:8])
> > +	 * states: "Software must set this to a nonzero value."
> > +	 */The motivation (as explained in the commit message) for using arch permitted
> minimum value instead of the recommended (PMSIDR_EL1.Interval) value, should
> be explained in the comment here as well.

Will update the comment to make it clear.

> > +	if (period < FIELD_PREP(PMSIRR_EL1_INTERVAL_MASK, 1))
> > +		period = FIELD_PREP(PMSIRR_EL1_INTERVAL_MASK, 1);
> Given that FIELD_PREP(PMSIRR_EL1_INTERVAL_MASK, 1) is a constant - should
> not it be defined as a macro e.g PMSIRR_EL1_MINIMUM_INTERVAL or something
> similar instead of deriving it twice.

I assume the compiler will evaluate the macro, so defining a new macro
should make no difference.

For more readable, I will try to add the macro in next spin.

Thanks,
Leo

> >  	else if (period > max_period)
> >  		period = max_period;
> >  	else
>