Re: [PATCH v9 14/20] RISC-V: perf: Implement supervisor counter delegation support

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Unconditionally setting the HPMEVENT_OF bit causes an unrecoverable interrupt storm for non-sampling events.
- [High] Bailing out of `rvpmu_deleg_ctr_enable_irq()` when `SIP_LCOFIP` is set causes newly started counters to falsely trigger spurious overflows.
- [High] Calling `__ffs()` on a zero mask when all hardware PMU counters are in use invokes undefined behavior and silently corrupts active perf events.
--

--- Patch [14]: [PATCH v9 14/20] RISC-V: perf: Implement supervisor counter delegation support ---
commit c2b448f2df09243c89d8cc4f0c47edd59230b3d1
Author: Atish Patra <[email protected]>

RISC-V: perf: Implement supervisor counter delegation support

This commit adds runtime detection and support for RISC-V ISA extensions
(ssccfg, sscsrind, smcntrpmf) to program hpmcounter/hpmevents directly from
S-mode. It falls back to the SBI PMU extension if the new extensions are
unavailable or for firmware counters.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 7afd79b0a29b8..fd4bf8804c9a6 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
[ ... ]
> @@ -1038,55 +1187,75 @@ static inline void rvpmu_sbi_stop_hw_ctrs(struct riscv_pmu *pmu)
[ ... ]
> +static void rvpmu_deleg_ctr_enable_irq(struct perf_event *event)
> +{
> +	unsigned long hpmevent_curr;
> +	unsigned long of_mask;
> +	struct hw_perf_event *hwc = &event->hw;
> +	int counter_idx = hwc->idx;
> +	unsigned long sip_val = csr_read(CSR_SIP);
> +
> +	if (!is_sampling_event(event) || (sip_val & SIP_LCOFIP))
> +		return;

[Severity: Critical]
write_deleg_hpmevent() unconditionally sets the HPMEVENT_OF flag on the
counter. If this returns early for non-sampling events, the OF flag remains
set. When the counter is uninhibited, won't the set OF flag immediately assert
an interrupt?

And since rvpmu_ovf_handler() skips non-sampling events:

	/* Skip if invalid event or user did not request a sampling */
	if (!event || !is_sampling_event(event))
		continue;

Will this result in an unrecoverable interrupt storm that hangs the CPU?

[Severity: High]
If SIP_LCOFIP is set from another counter's pending overflow, returning early
here means this new counter starts with its OF bit still set. When local
interrupts are re-enabled, won't the overflow handler falsely detect this new
counter as having instantly overflowed, corrupting the event state?

[ ... ]
> @@ -1232,22 +1448,194 @@ static irqreturn_t rvpmu_ovf_handler(int irq, void *dev)
[ ... ]
> +static int get_deleg_next_hpm_hw_idx(struct cpu_hw_events *cpuc, struct perf_event *event)
> +{
> +	unsigned long hw_ctr_mask = 0;
> +
> +	/*
> +	 * TODO: Treat every hpmcounter can monitor every event for now.
> +	 * The event to counter mapping should come from the json file.
> +	 * The mapping should also tell if sampling is supported or not.
> +	 */
> +
> +	/* Select only hpmcounters */
> +	hw_ctr_mask = cmask & (~0x7);
> +	hw_ctr_mask &= ~(cpuc->used_hw_ctrs[0]);
> +	return __ffs(hw_ctr_mask);
> +}

[Severity: High]
If all eligible PMU counters are currently active, hw_ctr_mask will be 0.
Calling __ffs(0) invokes undefined behavior, which on RV32 without the Zbb
extension evaluates to 31.

Then in rvpmu_deleg_ctr_get_idx():

	idx = get_deleg_next_hpm_hw_idx(cpuc, event);
	if (idx < 3 || idx > hw_ctr_max_id)
		goto out_err;
found_idx:
	priv_filter = get_deleg_priv_filter_bits(event);
	write_deleg_hpmevent(idx, hwc->config, priv_filter);
skip_update:
	if (!test_and_set_bit(idx, cpuc->used_hw_ctrs))
		return idx;

Won't this overwrite the configuration of the active counter (e.g. index 31)
in write_deleg_hpmevent() before verifying its availability with
test_and_set_bit(), silently corrupting the active counter's
configuration?

[ ... ]

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