[PATCH 2/3] perf: arm_spe: Disable buffer before writing to PMBPTR_EL1 or PMBSR_EL1
Leo Yan
leo.yan at arm.com
Fri Jul 4 08:50:16 PDT 2025
On Tue, Jul 01, 2025 at 04:31:58PM +0100, James Clark wrote:
[...]
> @@ -661,16 +666,24 @@ static irqreturn_t arm_spe_pmu_irq_handler(int irq, void *dev)
> */
> irq_work_run();
>
> + /*
> + * arm_spe_pmu_buf_get_fault_act() already drained, and PMBSR_EL1.S == 1
> + * means that StatisticalProfilingEnabled() == false. So now we can
> + * safely disable the buffer.
> + */
> + write_sysreg_s(0, SYS_PMBLIMITR_EL1);
> + isb();
> +
> + /* Status can be cleared now that PMBLIMITR_EL1.E == 0 */
> + write_sysreg_s(0, SYS_PMBSR_EL1);
> +
An important thing is about sequence:
As described in arm_spe_pmu_disable_and_drain_local(), should we always
clear ELs bits in PMSCR_EL1 before clear PMBLIMITR_EL1.E bit? As a
reference, we could see TRBE always clear ELx bits before disable trace
buffer.
And a trivial flaw:
If the TRUNCATED flag has been set, the irq_work_run() above runs the
IRQ work to invoke the arm_spe_pmu_stop() to disable trace buffer, which
clear SYS_PMBLIMITR_EL1.E bit. This is why the current code does not
explictly clear SYS_PMBLIMITR_EL1.E bit.
With this patch, the interrupt handler will clear SYS_PMBLIMITR_EL1.E
bit twice for a trunacated case.
> switch (act) {
> case SPE_PMU_BUF_FAULT_ACT_FATAL:
> /*
> - * If a fatal exception occurred then leaving the profiling
> - * buffer enabled is a recipe waiting to happen. Since
> - * fatal faults don't always imply truncation, make sure
> - * that the profiling buffer is disabled explicitly before
> - * clearing the syndrome register.
> + * To complete the full disable sequence, also disable profiling
> + * at EL0 and EL1, we don't want to continue at all anymore.
> */
> - arm_spe_pmu_disable_and_drain_local();
> + write_sysreg_s(0, SYS_PMSCR_EL1);
> break;
> case SPE_PMU_BUF_FAULT_ACT_OK:
> /*
> @@ -679,18 +692,14 @@ static irqreturn_t arm_spe_pmu_irq_handler(int irq, void *dev)
> * PMBPTR might be misaligned, but we'll burn that bridge
> * when we get to it.
> */
> - if (!(handle->aux_flags & PERF_AUX_FLAG_TRUNCATED)) {
> + if (!(handle->aux_flags & PERF_AUX_FLAG_TRUNCATED))
> arm_spe_perf_aux_output_begin(handle, event);
> - isb();
I am a bit suspecious we can remove this isb().
As a reference to the software usage PKLXF in Arm ARM (DDI 0487 L.a),
after enable TRBE trace unit, an ISB is mandatory. Maybe check a bit
for this?
Thanks,
Leo
> - }
> break;
> case SPE_PMU_BUF_FAULT_ACT_SPURIOUS:
> /* We've seen you before, but GCC has the memory of a sieve. */
> break;
> }
>
> - /* The buffer pointers are now sane, so resume profiling. */
> - write_sysreg_s(0, SYS_PMBSR_EL1);
> return IRQ_HANDLED;
> }
>
>
> --
> 2.34.1
>
>
More information about the linux-arm-kernel
mailing list