Re: [PATCH v6 15/21] RISC-V: perf: Use config2/vendor table for event to counter mapping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] KVM guest events for CPU cycles and instructions will fail to map to fixed hardware counters when counter delegation is enabled.
- [High] Fixed counter allocation unconditionally prioritizes the fixed counter index without checking its availability, breaking fallback to programmable counters.
- [High] Legacy events restricted to fixed counters via the vendor map fail to schedule.
- [High] `get_deleg_fixed_hw_idx()` assigns fixed counters without verifying if they are actually delegated by hardware (`cmask`), enabling unprivileged DoS.
- [Medium] Missing checks for zero before calling `__fls()` leads to undefined behavior.
--

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

RISC-V: perf: Use config2/vendor table for event to counter mapping

The counter restriction specified in the json file is passed to
the drivers via config2 paarameter in perf attributes. This allows
any platform vendor to define their custom mapping between event and
hpmcounters without any rules defined in the ISA.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 5bfcd3821f572..4b4f151a0744d 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c

[ ... ]

> @@ -1480,24 +1482,80 @@ static int rvpmu_deleg_find_ctrs(void)
>  	return num_hw_ctr;
>  }
>  
> +/*
> + * The json file must correctly specify counter 0 or counter 2 is available
> + * in the counter lists for cycle/instret events. Otherwise, the drivers have
> + * no way to figure out if a fixed counter must be used and pick a programmable
> + * counter if available.
> + */
>  static int get_deleg_fixed_hw_idx(struct cpu_hw_events *cpuc, struct perf_event *event)
>  {
> -	return -EINVAL;
> +	struct hw_perf_event *hwc = &event->hw;
> +	bool guest_events = event->attr.config1 & RISCV_PMU_CONFIG1_GUEST_EVENTS;
> +
> +	if (guest_events) {
> +		if (hwc->event_base == SBI_PMU_HW_CPU_CYCLES)
> +			return 0;

[Severity: High]
Does this check fail to map KVM guest events to fixed hardware counters
when counter delegation is enabled?

The check relies on hwc->event_base being populated with generic SBI
event IDs. However, under counter delegation, rvpmu_cdeleg_event_map()
sets econfig but returns 0 on success. This return value is assigned to
hwc->event_base, making it permanently 0.

Could this cause the comparison against SBI_PMU_HW_CPU_CYCLES (1) to
always fail and return -EINVAL?

> +		if (hwc->event_base == SBI_PMU_HW_INSTRUCTIONS)
> +			return 2;
> +		else
> +			return -EINVAL;
> +	}
> +
> +	if (!event->attr.config2)
> +		return -EINVAL;

[Severity: High]
Are we inadvertently rejecting fixed counters for legacy events here
unless userspace explicitly provides config2?

For standard legacy events (like perf stat -e cycles), config2 is 0, so
it falls through to get_deleg_next_hpm_hw_idx(). If a vendor restricted
an event to fixed counters via the vendor map, would returning -EINVAL
here cause standard perf commands to fail?

> +
> +	if (event->attr.config2 & RISCV_PMU_CYCLE_FIXED_CTR_MASK)
> +		return 0; /* CY counter */

[Severity: High]
Is it safe to assign fixed counters based directly on userspace config
without validating if they are actually delegated by hardware (cmask)?

If the platform did not delegate these counters, they remain
uninitialized in pmu_ctr_list. Unprivileged users could request these
counters and cause perf to read CSR 0, triggering
pr_err("Invalid performance counter csr...") on every PMU read.

Could this flood the kernel log and enable a Denial of Service?

[Severity: High]
This block unconditionally prioritizes the fixed counter index (0 or 2)
without checking if the counter is already in use.

If the fixed counter is busy, rvpmu_deleg_ctr_get_idx() attempts
test_and_set_bit() on it, fails, and jumps directly to out_err.

Could this break the fallback logic in get_deleg_next_hpm_hw_idx(),
which might have successfully allocated an available programmable
counter allowed by the config2 mask?

> +	else if (event->attr.config2 & RISCV_PMU_INSTRUCTION_FIXED_CTR_MASK)
> +		return 2; /* IR counter */
> +	else
> +		return -EINVAL;
>  }
>  
>  static int get_deleg_next_hpm_hw_idx(struct cpu_hw_events *cpuc, struct perf_event *event)
>  {
> -	unsigned long hw_ctr_mask = 0;
> +	u32 hw_ctr_mask = 0, temp_mask = 0;
> +	u32 type = event->attr.type;
> +	u64 config = event->attr.config;
> +	int ret;
>  
> -	/*
> -	 * 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 available hpmcounters */
> +	hw_ctr_mask = cmask & (~0x7) & ~(cpuc->used_hw_ctrs[0]);

[Severity: High]
By explicitly excluding fixed counters (~0x7) from the available mask
here, do we prevent legacy events from scheduling if they are restricted
to fixed counters via the vendor map?

If the event falls back from get_deleg_fixed_hw_idx() due to config2
being 0, and the vendor mapped the event exclusively to a fixed counter
(temp_mask = 1 or 4), hw_ctr_mask &= temp_mask will result in 0.

Would this cause the function to return -EINVAL and break standard perf
commands?

[ ... ]

> @@ -1526,10 +1584,6 @@ static int rvpmu_deleg_ctr_get_idx(struct perf_event *event)
>  	u64 priv_filter;
>  	int idx;
>  
> -	/*
> -	 * TODO: We should not rely on SBI Perf encoding to check if the event
> -	 * is a fixed one or not.
> -	 */
>  	if (!is_sampling_event(event)) {
>  		idx = get_deleg_fixed_hw_idx(cpuc, event);

[Severity: High]
Will KVM guest events that map to fixed hardware counters be skipped
here since they define a sample period?

Because is_sampling_event() evaluates to true for guest events, it
appears they will bypass this fixed counter assignment block entirely.

>  		if (idx == 0 || idx == 2) {

[ ... ]

> @@ -1547,10 +1601,14 @@ static int rvpmu_deleg_ctr_get_idx(struct perf_event *event)
>  		goto out_err;
>  found_idx:
>  	priv_filter = get_deleg_priv_filter_bits(event);
> +	if (test_and_set_bit(idx, cpuc->used_hw_ctrs))
> +		goto out_err;
>  	update_deleg_hpmevent(idx, hwc->config, priv_filter);
> +	return idx;
>  skip_update:
> -	if (!test_and_set_bit(idx, cpuc->used_hw_ctrs))
> -		return idx;
> +	if (test_and_set_bit(idx, cpuc->used_hw_ctrs))
> +		goto out_err;
> +	return idx;

[Severity: Medium]
Are there missing checks for zero before calling __fls() in the
surrounding code within rvpmu_deleg_ctr_get_idx() and
get_deleg_hw_ctr_width()?

The macro __fls() maps directly to __builtin_clzl(), which results in
undefined behavior if the input is 0.

Could this happen in rvpmu_deleg_ctr_get_idx() if no hardware counters
are delegated and cmask is 0, or in get_deleg_hw_ctr_width() if a
hypervisor configures the WARL CSR as hardwired to 0?

>  out_err:
>  	return -ENOENT;
>  }

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