Re: [RFC PATCH v2 4/4] perf/riscv: Use Sspesa for precise sample attribution

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf/riscv: stale CSR_SHPMSDATA not cleared in pmu_sbi_ovf_handler()
--

commit 097e5fbeec62109e46a3171d644ba9f822fd792f
Author: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>

perf/riscv: Use Sspesa for precise sample attribution

This commit adds support for the RISC-V Sspesa extension in the SBI PMU
driver to record precise sample PCs from CSR_SHPMSPC and CSR_SHPMSDATA
during counter overflow handling.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 2991dd92def28..c42122f0e6760 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c

[ ... ]

> @@ -1115,6 +1229,21 @@ static irqreturn_t pmu_sbi_ovf_handler(int irq, void *dev)
>  	else
>  		ALT_SBI_PMU_OVERFLOW(overflow);
>  
> +	/*
> +	 * Sspesa records the PC and metadata of the overflowing counter in
> +	 * hardware. The PC is precise only for events that support precise
> +	 * attribution; otherwise it is best-effort.
> +	 */
> +	if (overflow && sspesa_available) {
> +		sample_pc = csr_read(CSR_SHPMSPC);
> +		sample_data = csr_read(CSR_SHPMSDATA);
> +		csr_write(CSR_SHPMSDATA, 0);
> +		sample_cntrid = sample_data & SHPMSDATA_CNTRID;
> +		if (pmu_sbi_sspesa_derive_pc)
> +			sample_pc_valid = pmu_sbi_sspesa_derive_pc(&sample_pc,
> +								sample_data);
> +	}

[Severity: Medium]
Can CSR_SHPMSDATA remain latched with a stale counter ID and sample PC if a
counter overflows while local interrupts are disabled during a context switch
or event teardown?

When a hardware counter overflows while local interrupts are disabled,
hardware latches CSR_SHPMSPC and CSR_SHPMSDATA (setting SHPMSDATA_CNTRID to
the overflowing counter index) and sets the pending PMU overflow interrupt.
Before interrupts are re-enabled, stopping or deleting the event via:

  perf_event_task_sched_out() -> riscv_pmu_stop() -> pmu_sbi_ctr_stop()
  or
  riscv_pmu_del() -> pmu_sbi_ctr_stop() / pmu_sbi_ctr_clear_idx()

stops or resets the counter in SBI (clearing its overflow bit in scountovf)
without clearing CSR_SHPMSDATA.

When local interrupts are later re-enabled and pmu_sbi_ovf_handler() runs, if
all hardware counters were removed then used_hw_ctrs is empty and
pmu_sbi_ovf_handler() returns early:

	fidx = find_first_bit(cpu_hw_evt->used_hw_ctrs, RISCV_MAX_COUNTERS);
	if (fidx == RISCV_MAX_COUNTERS) {
		csr_clear(CSR_SIP, BIT(riscv_pmu_irq_num));
		return IRQ_NONE;
	}

Or if other counters remain active, overflow can be 0 because stopping the
overflowed counter cleared its scountovf bit, which skips the
if (overflow && sspesa_available) block and returns IRQ_NONE after clearing
the pending interrupt. Similarly, pmu_sbi_starting_cpu() stops all counters
and clears pending overflow interrupts during CPU startup without zeroing
CSR_SHPMSDATA.

Because Sspesa hardware does not overwrite CSR_SHPMSPC or CSR_SHPMSDATA while
SHPMSDATA_CNTRID is non-zero, subsequent overflows on other counters will
fail to capture a sample PC. Furthermore, when the same hardware counter
index is later reused by another event or task and overflows,
pmu_sbi_ovf_handler() will see cur_cntrid == sample_cntrid and attribute the
previous task's stale sample_pc and privilege mode into perf_regs.epc:

		if (sample_pc_valid && cur_cntrid == sample_cntrid) {
			if (pmu_sbi_sspesa_regs(&perf_regs, regs, sample_pc, sample_data)) {
				sample_regs = &perf_regs;
				if (ssplcofi_available && pmu_sbi_is_pesa_event(event))
					sample_regs->status |= PERF_SR_PRECISE;
			}
		}

Should CSR_SHPMSDATA also be cleared when stopping a counter whose index
matches SHPMSDATA_CNTRID, on early exits in pmu_sbi_ovf_handler(), and during
CPU startup in pmu_sbi_starting_cpu()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029130917.3714691-1-mayuresh.chitale@oss.qualcomm.com?part=4