Re: [PATCH v10 7/9] perf: RISC-V: add support for SSE event

[sashiko-bot@kernel.org]

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

New issues:
- [High] The SSE overflow handler early bailout checks `events[fidx]` where `fidx` is the lowest active counter in `used_hw_ctrs`. Because `used_hw_ctrs` is updated non-atomically relative to `events[]` during event addition and deletion, an SSE NMI arriving in this window observes a NULL event and permanently stops all PMU counters.
- [High] Synchronously calling `riscv_pmu_stop` from `pmu_sbi_fail_sse` in NMI context corrupts PMU state if it interrupts perf core operations.
- [Medium] Overloaded use of `ret == 0` causes `irq_requested` to be true even when SSE setup succeeds (which does not request an IRQ). This leads to an invalid `free_percpu_irq` on IRQ 0 in the probe error path.

Pre-existing issues:
- [High] On 32-bit RISC-V architectures, the driver uses a 32-bit `unsigned long` for `cmask` and invokes `BIT(i)` in a loop up to `RISCV_MAX_COUNTERS` (64). This causes undefined behavior when `i >= 32` and fails to track or stop PMU counters with IDs >= 32.
--

commit 9148146c03e3720e4a1ce81cb1e6c77ab87c87df
Author: Clément Léger <cleger@rivosinc.com>
Subject: perf: RISC-V: add support for SSE event

This commit adds support for the local PMU overflow SSE event so that RISC-V
perf can receive overflows even when normal S-mode interrupts are masked. It
selects the delivery mechanism during PMU probe and configures SSE event
masking across CPU power management transitions.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 50220f7b46d9b..7e971b47730c4 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -896,14 +898,24 @@ static int pmu_sbi_get_ctrinfo(int nctr, unsigned long *mask)
>  	return 0;
>  }
>  
> -static inline void pmu_sbi_stop_all(struct riscv_pmu *pmu)
> +static inline void pmu_sbi_stop_all_mask(unsigned long ctr_mask)
>  {
>  	/*
>  	 * No need to check the error because we are disabling all the counters
>  	 * which may include counters that are not enabled yet.
>  	 */
>  	sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_STOP,
> -		  0, pmu->cmask, SBI_PMU_STOP_FLAG_RESET, 0, 0, 0);
> +		  0, ctr_mask, SBI_PMU_STOP_FLAG_RESET, 0, 0, 0);
> +}

[Severity: High]
This is a pre-existing issue, but on 32-bit RISC-V architectures, cmask is
an unsigned long, which truncates to 32 bits on RV32. In pmu_sbi_get_ctrinfo(),
the driver also invokes BIT(i) in a loop up to RISCV_MAX_COUNTERS (64). Does
this cause undefined behavior and fail to track or stop firmware/hardware PMU
counters with IDs >= 32 during PMU teardown or SSE early-exit paths?

[ ... ]

> +#ifdef CONFIG_RISCV_PMU_SBI_SSE
> +/*
> + * A local SSE delivery failure makes the current PMU state unsafe to resume.
> + * Latch the failure before stopping mapped events so the SSE transition and
> + * overflow restart paths cannot undo the fail-safe while they are quiesced.
> + */
> +static void pmu_sbi_fail_sse(struct riscv_pmu *pmu, const char *op, int ret)
> +{
> +	struct cpu_hw_events *cpu_hw_evt = this_cpu_ptr(pmu->hw_events);
> +	struct perf_event *event;
> +	int idx;
> +
> +	if (READ_ONCE(cpu_hw_evt->sse_failed))
> +		return;
> +
> +	WRITE_ONCE(cpu_hw_evt->sse_failed, true);
> +	pr_err_ratelimited("failed to %s local PMU SSE event: %d; stopping counters\n",
> +			   op, ret);
> +
> +	for (idx = 0; idx < RISCV_MAX_COUNTERS; idx++) {
> +		event = cpu_hw_evt->events[idx];
> +		if (event)
> +			riscv_pmu_stop(event, PERF_EF_UPDATE);

[Severity: High]
Can calling riscv_pmu_stop() synchronously from pmu_sbi_fail_sse() corrupt
PMU state if executed in NMI context? 

When pmu_sbi_ovf_sse_handler() receives a NULL regs pointer, it calls
pmu_sbi_fail_sse(). If this NMI interrupts perf core operations like
riscv_pmu_start(), the software state could become desynchronized from the
hardware counter state.

> +	}
> +}

[ ... ]

> @@ -1048,28 +1154,38 @@ static irqreturn_t pmu_sbi_ovf_handler(int irq, void *dev)
>  	struct perf_event *event;
>  	u64 overflow;
>  	u64 overflowed_ctrs = 0;
> -	struct cpu_hw_events *cpu_hw_evt = dev;
>  	u64 start_clock = sched_clock();
>  	struct riscv_pmu_snapshot_data *sdata = cpu_hw_evt->snapshot_addr;
>  
>  	if (WARN_ON_ONCE(!cpu_hw_evt))
>  		return IRQ_NONE;
>  
> -	/* Firmware counter don't support overflow yet */
> +	/*
> +	 * SSE can arrive before perf installs an event. The early exits below
> +	 * must stop the PMU source before firmware completes the SSE.
> +	 */
>  	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));
> +		if (from_sse)
> +			pmu_sbi_stop_all_mask(cmask);
> +		else
> +			csr_clear(CSR_SIP, BIT(riscv_pmu_irq_num));
>  		return IRQ_NONE;
>  	}
>  
>  	event = cpu_hw_evt->events[fidx];
>  	if (!event) {
> -		ALT_SBI_PMU_OVF_CLEAR_PENDING(riscv_pmu_irq_mask);
> +		if (from_sse)
> +			pmu_sbi_stop_all_mask(cmask);
> +		else
> +			ALT_SBI_PMU_OVF_CLEAR_PENDING(riscv_pmu_irq_mask);
>  		return IRQ_NONE;
>  	}

[Severity: High]
Does this early bailout race with concurrent event addition or deletion?

The used_hw_ctrs bit is set inside riscv_pmu_add() via ctr_get_idx(), which
exposes a window where events[fidx] is NULL while the bit is active. If an
SSE NMI arrives in this window, it observes a NULL event and triggers the
!event fallback path, permanently stopping all PMU counters via
pmu_sbi_stop_all_mask().

[ ... ]

> @@ -1453,6 +1806,7 @@ static int pmu_sbi_device_probe(struct platform_device *pdev)
>  	/* cache all the information about counters now */
>  	if (pmu_sbi_get_ctrinfo(num_counters, &cmask))
>  		goto out_free;
> +	pmu->cmask = cmask;
>  
>  	ret = pmu_sbi_setup_irqs(pmu, pdev);
>  	if (ret < 0) {
> @@ -1462,9 +1816,15 @@ static int pmu_sbi_device_probe(struct platform_device *pdev)
>  	}
>  	irq_requested = (ret == 0);

[Severity: Medium]
Could this attempt to free an unallocated IRQ on the error path?

Because pmu_sbi_setup_irqs() now returns 0 when SSE setup succeeds,
irq_requested is set to true even when no IRQ was requested. If a subsequent
initialization step fails, the out_destroy error path would call
free_percpu_irq() for IRQ 0.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911121456.33223-1-zhangzhanpeng.jasper@bytedance.com?part=7