Re: [PATCH 1/3] perf: arm_spe: Factor aux output and flags out of buffer fault handler

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing immediate `isb()` after writing to control-plane system register `SYS_PMBLIMITR_EL1`.
- [High] Missing immediate `isb()` after writing to control-plane system register `SYS_PMSCR_EL1`.
--

--- Patch [1]: [PATCH 1/3] perf: arm_spe: Factor aux output and flags out of buffer fault handler ---
commit 454b555e21bef7b51b93cb02a78486de3845b1b9
Author: James Clark <[email protected]>
Subject: perf: arm_spe: Factor aux output and flags out of buffer fault handler

This patch refactors the ARM SPE PMU buffer fault handling by separating the
auxiliary buffer output management and flags from the fault decoding logic.
This simplifies the stop handling routine as it no longer conditionally
calls the aux output end function. No functional changes are intended.

> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> index dbd0da1116390..10d28ad81256f 100644
> --- a/drivers/perf/arm_spe_pmu.c
> +++ b/drivers/perf/arm_spe_pmu.c
> @@ -677,11 +677,12 @@ static void arm_spe_pmu_disable_and_drain_local(void)
>  
>  /* IRQ handling */
>  static enum arm_spe_pmu_buf_fault_action
> -arm_spe_pmu_buf_get_fault_act(struct perf_output_handle *handle)
> +arm_spe_pmu_buf_get_fault_act(u64 *aux_flags)
>  {

[ ... ]

> out_write_limit:
> 	write_sysreg_s(limit, SYS_PMBLIMITR_EL1);
> 	return (limit & PMBLIMITR_EL1_E) ? 0 : -EIO;

[Severity: High]
This is a pre-existing issue, but is an immediate isb() required here?

The ARM64 architecture guidelines state that writes to control-plane system
registers like SYS_PMBLIMITR_EL1 must be followed by an isb() as the very
next instruction. In arm_spe_perf_aux_output_begin() in 
drivers/perf/arm_spe_pmu.c, the function returns immediately after the write,
which could leave the CPU pipeline in an architecturally undefined state for
subsequent instructions.

[ ... ]

> 	reg = arm_spe_event_to_pmscr(event);
> 	isb();
> 	write_sysreg_s(reg, SYS_PMSCR_EL1);
> }

[Severity: High]
This isn't a bug introduced by this patch, but shouldn't there be an isb()
after this system register write?

In arm_spe_pmu_start() in drivers/perf/arm_spe_pmu.c, an isb() is issued
before writing to SYS_PMSCR_EL1, but it appears missing immediately after
the write. Missing context synchronization after a system register write
might leave the pipeline exposed to a stale configuration state.

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