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

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2