Re: [PATCH 2/3] perf: arm_spe: Factor HW discard mode out of arm_spe_perf_aux_output_begin()

On 05/08/2026 16:37, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] Unhandled Interrupt Storm / Unreachable Code due to uninitialized perf_output_handle in discard mode
> 
> Pre-existing issues:
> - [High] Missing Context Synchronization Event (ISB) after writing to SYS_PMBLIMITR_EL1
> --
> 
> --- Patch [2]: [PATCH 2/3] perf: arm_spe: Factor HW discard mode out of arm_spe_perf_aux_output_begin() ---
> commit 61201ea6d6b4e69de62f9788efe1db4fd64b4fd9
> Author: James Clark <[email protected]>
> 
> perf: arm_spe: Factor HW discard mode out of arm_spe_perf_aux_output_begin()
> 
> Discard mode doesn't call perf_aux_output_begin() or emit aux records,
> and enabling it never fails. We'll add a second discard mode in the
> next commit so it will be cleaner if there is a separate begin function
> just for discard mode.
> 
>> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
>> --- a/drivers/perf/arm_spe_pmu.c
>> +++ b/drivers/perf/arm_spe_pmu.c
> [ ... ]
>> @@ -609,18 +614,23 @@ static u64 arm_spe_pmu_next_off(struct perf_output_handle *handle)
>>   	return limit;
>>   }
>>   
>> +static void arm_spe_pmu_begin_discard(struct perf_output_handle *handle,
>> +				      struct perf_event *event)
>> +{
>> +	u64 limit;
>> +
>> +	limit = FIELD_PREP(PMBLIMITR_EL1_FM, PMBLIMITR_EL1_FM_DISCARD);
>> +	limit |= PMBLIMITR_EL1_E;
>> +
>> +	write_sysreg_s(limit, SYS_PMBLIMITR_EL1);
>> +}
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but writing to a control-plane
> system register like SYS_PMBLIMITR_EL1 requires an immediate isb() to avoid
> leaving the CPU pipeline in an architecturally undefined state.
> 
> Looking at the caller in arm_spe_pmu_irq_handler(), the isb() is issued after
> this function returns, which means the CPU executes the function return
> instructions before the barrier. In arm_spe_pmu_start(), execution proceeds to
> even more instructions before an isb() is eventually issued.
> 
> Should the isb() be placed immediately after the write_sysreg_s() directly
> inside arm_spe_pmu_begin_discard() to guarantee architectural compliance?
> 
> [ ... ]
>> @@ -884,7 +900,9 @@ static void arm_spe_pmu_start(struct perf_event *event, int flags)
>>   	struct perf_output_handle *handle = this_cpu_ptr(spe_pmu->handle);
>>   
>>   	hwc->state = 0;
>> -	if (arm_spe_perf_aux_output_begin(handle, event)) {
>> +	if (arm_spe_discard_mode(event)) {
>> +		arm_spe_pmu_begin_discard(handle, event);
>> +	} else if (arm_spe_perf_aux_output_begin(handle, event)) {
>>   		arm_spe_pmu_stop(event, 0);
>>   		return;
>>   	}
> 
> [Severity: High]
> Because discard mode explicitly bypasses perf_aux_output_begin() here, the
> per-CPU perf_output_handle is never initialized with the ring buffer or event.
> 

If this is an issue then it's an existing one because this patch is just 
a refactor.

> If the PMU hardware asserts an interrupt during this mode, wouldn't
> arm_spe_pmu_irq_handler() see an uninitialized (or previously NULLed) handle?
> 
> It looks like early checks in the IRQ handler, such as checking if the event
> is present or if perf_get_aux(handle) is valid, would evaluate to true and
> return IRQ_NONE immediately without clearing the hardware interrupt syndrome.
> Could this lead to an unhandled interrupt storm that locks up the CPU?
> 

HW discard mode doesn't trigger interrupts. If it does then a separate 
fixes commit is required unrelated to these patches.