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

[Atish Patra <atish.patra@linux.dev>]

On 6/8/26 11:23 PM, sashiko-bot@kernel.org wrote:
> 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,
That's not a likely scenario as S-mode counter delegation is not 
available for VS mode.
> 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.
N/A. Incorrect spec understanding.
Sscofpmf's OF bit gates the LCOFI interrupt only. it does NOT stop the 
counter.
>>   
>> -	/* 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.

Good catch. Fixed it in v7.


>> +#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.
There are only 32 bit counters defined by architecture.
> Should this use 1UL << hwc->idx or the BIT() macro instead?
It's better coding standard to use BIT anyways. Fixed in v7.
> [ ... ]
>
>> -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?
N/A. Not a likely scenario.
> 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.
N/A OF bit is for IRQ overflow not for inhibition.
> [ ... ]
>
>>   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.
Yes. Changed to use BIT macro.