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

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Improper handling of the HPMEVENT_OF bit causes non-sampling events to never count and leads to permanent PMU interrupt deadlocks for sampling events.
- [High] Counter delegation and SBI PMU snapshot features conflict, causing stale counter reads and broken overflow handling when both are present.

Pre-existing issues:
- [High] The PMU overflow handler unconditionally restarts counters, ignoring the throttling decision from perf_event_overflow().
--

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

RISC-V: perf: Implement supervisor counter delegation support

This commit adds support for the ssccfg, sscsrind, and smcntrpmf RISC-V ISA 
extensions to program hpmcounters directly from supervisor mode. It falls
back to the SBI PMU extension for firmware counters or if the ISA extensions
are unavailable.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 2568c6808f5d2..7995da4a98a1c 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c

[ ... ]

> @@ -821,7 +968,7 @@ static int pmu_sbi_snapshot_setup(struct riscv_pmu *pmu, int cpu)
>  	return 0;
>  }
>  
> -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]
If both counter delegation and SBI snapshot features are available, could
this return stale data?

When the counter is stopped via rvpmu_deleg_ctr_stop_mask(), the SBI snapshot
shared memory is not updated. Since sbi_pmu_snapshot_available() is still
true, it seems we might read a stale or zero value from sdata->ctr_values
instead of reading the hardware counter.

[ ... ]

> +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]
Will non-sampling events remain permanently halted here?

In update_deleg_hpmevent(), the HPMEVENT_OF bit is unconditionally set, which
architecturally inhibits the counter. This function is responsible for clearing
it. However, if this returns early for non-sampling events, the OF bit is
never cleared. Could this cause events like standard perf stat to perpetually
read zero?

Additionally, if sip_val & SIP_LCOFIP is true (e.g. if another counter
overflows while processing), does returning early permanently deadlock this
newly restarted sampling counter?

[ ... ]

> -static inline void rvpmu_sbi_start_ovf_ctrs_snapshot(struct cpu_hw_events *cpu_hw_evt,
> -						     u64 ctr_ovf_mask)
> +static void rvpmu_sbi_start_ovf_ctrs_snapshot(struct cpu_hw_events *cpu_hw_evt,
> +					      u64 ctr_ovf_mask)

[ ... ]

> -static void rvpmu_sbi_start_overflow_mask(struct riscv_pmu *pmu,
> -					  u64 ctr_ovf_mask)
> +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 bypass counter delegation entirely when SBI snapshots are present?

If sbi_pmu_snapshot_available() is true, this unconditionally delegates to
rvpmu_sbi_start_ovf_ctrs_snapshot(). This appears to use the SBI interface
to restart the counters and initializes them from the potentially stale
snapshot memory, which might clobber the hardware state managed by counter
delegation.

[ ... ]

> @@ -1228,22 +1427,183 @@ static irqreturn_t rvpmu_ovf_handler(int irq, void *dev)
>  		hw_evt->state = 0;
>  	}
>  
> -	rvpmu_sbi_start_overflow_mask(pmu, overflowed_ctrs);
> +	rvpmu_start_overflow_mask(pmu, overflowed_ctrs);

[Severity: High]
This is a pre-existing issue, but does this unconditionally restart counters
even if perf_event_overflow() requests throttling?

The core perf_event_overflow() returns non-zero to signal that an event has
exceeded the maximum sample rate and should be throttled. Since the return
value is ignored here, the counter is blindly restarted via
rvpmu_start_overflow_mask(). Could this allow a maliciously low sample period
to cause an unmitigated PMU interrupt storm?

>  	perf_sample_event_took(sched_clock() - start_clock);
>  
>  	return IRQ_HANDLED;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701-counter_delegation-v8-0-7909f863a645@meta.com?part=14