Re: [PATCH v6 13/21] RISC-V: perf: Implement supervisor counter delegation support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Bitwise shift of signed 32-bit integer causes overflow and corrupts SCOUNTINHIBIT on 64-bit systems.
- [High] Non-sampling events are permanently inhibited from counting because HPMEVENT_OF is unconditionally set but never cleared.
- [High] On 32-bit architectures, `rvpmu_deleg_ctr_enable_irq` writes modified event configuration to the wrong CSR (`CSR_SIREG4` instead of `CSR_SIREG5`).
- [High] SBI PMU snapshot logic breaks supervisor counter delegation by unconditionally bypassing S-mode CSR writes and reading stale shared memory.
--

commit ff1e171673cfe94a12c443765bd5b68dacf3b687
Author: Atish Patra <atishp@rivosinc.com>

RISC-V: perf: Implement supervisor counter delegation support

This patch adds support for RISC-V ISA extensions (ssccfg, sscsrind,
smcntrpmf) that allow programming hpmcounters directly from S-mode.
It falls back to the SBI PMU extension if the hardware extensions are
unavailable or for firmware counters.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 46a25979e95e9..1f16df9d0dd00 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c

[ ... ]

> -static u64 rvpmu_sbi_ctr_read(struct perf_event *event)
> +static u64 rvpmu_ctr_read(struct perf_event *event)
>  {
>  	struct hw_perf_event *hwc = &event->hw;
>  	int idx = hwc->idx;

[Severity: High]
Does this function still read stale data from the SBI shared memory when
using supervisor counter delegation?

If a platform supports both SBI PMU snapshots and S-mode counter delegation,
the code unconditionally reads from the SBI shared memory for stopped
hardware counters. Since S-mode counters are stopped via direct CSR writes,
the SBI firmware is bypassed and the shared memory is never updated.

[ ... ]

> +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: High]
Will this early return cause non-sampling events to never be counted?

In update_deleg_hpmevent(), the HPMEVENT_OF bit is unconditionally set for
all events. The Sscofpmf specification states a counter does not increment
while its OF bit is 1. If we return early here for non-sampling events
(like perf stat), the OF bit is never cleared, which would leave these
counters permanently inhibited.

>  
> -	/* Reinitialize and start all the counter that overflowed */
> -	while (ctr_ovf_mask) {
> -		if (ctr_ovf_mask & 0x01) {
> -			event = cpu_hw_evt->events[idx];
> -			hwc = &event->hw;
> -			max_period = riscv_pmu_ctr_get_width_mask(event);
> -			init_val = local64_read(&hwc->prev_count) & max_period;
>  #if defined(CONFIG_32BIT)
> -			sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_START, idx, 1,
> -				  flag, init_val, init_val >> 32, 0);
> +	hpmevent_curr = csr_ind_read(CSR_SIREG5, SISELECT_SSCCFG_BASE, counter_idx);
> +	of_mask = (u32)~HPMEVENTH_OF;
>  #else
> -			sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_START, idx, 1,
> -				  flag, init_val, 0, 0);
> +	hpmevent_curr = csr_ind_read(CSR_SIREG2, SISELECT_SSCCFG_BASE, counter_idx);
> +	of_mask = ~HPMEVENT_OF;
>  #endif
> -			perf_event_update_userpage(event);
> -		}
> -		ctr_ovf_mask = ctr_ovf_mask >> 1;
> -		idx++;
> -	}
> +
> +	hpmevent_curr &= of_mask;
> +#if defined(CONFIG_32BIT)
> +	csr_ind_write(CSR_SIREG4, SISELECT_SSCCFG_BASE, counter_idx, hpmevent_curr);

[Severity: High]
Is this writing the event configuration to the wrong CSR?

The code reads the high 32 bits of the event configuration from CSR_SIREG5
and clears the HPMEVENTH_OF bit, but it writes the modified value back to
CSR_SIREG4. Since CSR_SIREG4 maps to hpmcounterh, this might overwrite the
running counter's upper 32 bits with the event configuration value instead
of clearing the OF bit in the hpmevent register.

> +#else
> +	csr_ind_write(CSR_SIREG2, SISELECT_SSCCFG_BASE, counter_idx, hpmevent_curr);
> +#endif
> +}
> +
> +static void rvpmu_deleg_ctr_start(struct perf_event *event, u64 ival)
> +{
> +	unsigned long scountinhibit_val = 0;
> +	struct hw_perf_event *hwc = &event->hw;
> +
> +#if defined(CONFIG_32BIT)
> +	csr_ind_write(CSR_SIREG, SISELECT_SSCCFG_BASE, hwc->idx, ival & 0xFFFFFFFF);
> +	csr_ind_write(CSR_SIREG4, SISELECT_SSCCFG_BASE, hwc->idx, ival >> BITS_PER_LONG);
> +#else
> +	csr_ind_write(CSR_SIREG, SISELECT_SSCCFG_BASE, hwc->idx, ival);
> +#endif
> +
> +	rvpmu_deleg_ctr_enable_irq(event);
> +
> +	scountinhibit_val = csr_read(CSR_SCOUNTINHIBIT);
> +	scountinhibit_val &= ~(1 << hwc->idx);

[Severity: High]
Could this bitwise shift result in sign-extension and corrupt SCOUNTINHIBIT
on 64-bit systems?

Because 1 is a signed 32-bit integer, (1 << 31) results in -2147483648.
When this is negated by a bitwise NOT or cast to an unsigned long on a
64-bit architecture, it gets sign-extended (e.g., 0x000000007FFFFFFF).
This could erroneously clear bits 32-63 of the SCOUNTINHIBIT CSR.

Should this use 1UL << hwc->idx or the BIT() macro instead?

[ ... ]

> -static void rvpmu_sbi_start_overflow_mask(struct riscv_pmu *pmu,
> -					  u64 ctr_ovf_mask)
> +/*
> + * This function starts all the used counters in two step approach.
> + * Any counter that did not overflow can be start in a single step
> + * while the overflowed counters need to be started with updated initialization
> + * value.
> + */
> +static void rvpmu_start_overflow_mask(struct riscv_pmu *pmu, u64 ctr_ovf_mask)
>  {
> +	int idx = 0, i;
> +	struct perf_event *event;
> +	unsigned long ctr_start_mask = 0;
> +	u64 max_period, init_val = 0;
> +	struct hw_perf_event *hwc;
>  	struct cpu_hw_events *cpu_hw_evt = this_cpu_ptr(pmu->hw_events);
>  
>  	if (sbi_pmu_snapshot_available())
> -		rvpmu_sbi_start_ovf_ctrs_snapshot(cpu_hw_evt, ctr_ovf_mask);
> -	else
> -		rvpmu_sbi_start_ovf_ctrs_sbi(cpu_hw_evt, ctr_ovf_mask);
> +		return rvpmu_sbi_start_ovf_ctrs_snapshot(cpu_hw_evt, ctr_ovf_mask);

[Severity: High]
Does this check bypass S-mode delegation when both snapshot and delegation
are available?

If a platform supports both, this unconditionally calls
rvpmu_sbi_start_ovf_ctrs_snapshot(), which starts S-mode delegated hardware
counters using SBI ECALLs rather than direct S-mode CSR writes.

[ ... ]

> +static void update_deleg_hpmevent(int counter_idx, uint64_t event_value, uint64_t filter_bits)
> +{
> +	u64 hpmevent_value = 0;
> +
> +	/* OF bit should be enable during the start if sampling is requested */
> +	hpmevent_value = (event_value & ~HPMEVENT_MASK) | filter_bits | HPMEVENT_OF;

[Severity: High]
Are we unconditionally setting the HPMEVENT_OF bit here?

Although the comment states it should be enabled if sampling is requested,
the bit is added for all events. Combined with the early return in
rvpmu_deleg_ctr_enable_irq(), this causes non-sampling events to remain
inhibited permanently.

[ ... ]

>  static void rvpmu_ctr_stop(struct perf_event *event, unsigned long flag)
>  {
> -	rvpmu_sbi_ctr_stop(event, flag);
> -	/* TODO: Counter delegation implementation */
> +	struct hw_perf_event *hwc = &event->hw;
> +
> +	if ((hwc->flags & PERF_EVENT_FLAG_USER_ACCESS) &&
> +	    (hwc->flags & PERF_EVENT_FLAG_USER_READ_CNT))
> +		rvpmu_reset_scounteren((void *)event);
> +
> +	if (riscv_pmu_cdeleg_available() && !pmu_sbi_is_fw_event(event)) {
> +		/*
> +		 * The counter is already stopped. No need to stop again. Counter
> +		 * mapping will be reset in clear_idx function.
> +		 */
> +		if (flag != RISCV_PMU_STOP_FLAG_RESET)
> +			rvpmu_deleg_ctr_stop_mask((1 << hwc->idx));

[Severity: High]
Can this 32-bit shift overflow and corrupt the upper 32 bits of SCOUNTINHIBIT?

Passing (1 << 31) will sign-extend to 0xFFFFFFFF80000000 on 64-bit systems.
When passed to rvpmu_deleg_ctr_stop_mask, it ORs this mask into SCOUNTINHIBIT,
which erroneously sets bits 32-63 to 1.

Consider using 1UL << hwc->idx or the BIT() macro to prevent overflow.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608-counter_delegation-v6-0-285b72ed65a9@meta.com?part=13