Re: [PATCH v7 16/22] 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 2 potential issue(s) to consider:
- [High] Generic legacy hardware events (e.g. `cycles`) on the host cannot use fixed counters, contradicting the commit message and breaking standard profiling APIs.
- [High] Arbitrary events can be forced to silently count CPU cycles or instructions by setting `config2`, leading to silent data corruption.
--

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

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

This commit allows platform vendors to define custom mappings between
events and hpmcounters by passing counter restrictions from the json file
to drivers via the config2 parameter. It also aims to let vendors specify
if fixed cycle and instruction counters should be used for legacy events.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 4f3a30143db11..1c846cdc96cf4 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c

[ ... ]

> @@ -1501,24 +1503,85 @@ 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;
> +	bool guest_events = event->attr.config1 & RISCV_PMU_CONFIG1_GUEST_EVENTS;
> +	int idx;
> +
> +	/* event_base is 0 on the delegation path; match via the original perf attrs. */
> +	if (guest_events) {
> +		if (event->attr.type != PERF_TYPE_HARDWARE)
> +			return -EINVAL;
> +		if (event->attr.config == PERF_COUNT_HW_CPU_CYCLES)
> +			idx = 0; /* CY counter */
> +		else if (event->attr.config == PERF_COUNT_HW_INSTRUCTIONS)
> +			idx = 2; /* IR counter */
> +		else
> +			return -EINVAL;
> +	} else if (event->attr.config2 & RISCV_PMU_CYCLE_FIXED_CTR_MASK) {
> +		idx = 0; /* CY counter */
> +	} else if (event->attr.config2 & RISCV_PMU_INSTRUCTION_FIXED_CTR_MASK) {
> +		idx = 2; /* IR counter */
> +	} else {

[Severity: High]
Here config2 is evaluated without validating that the event is actually a
cycle/instruction event, or at least a PERF_TYPE_RAW event.

Since counterid_mask exposes config2:0-31 to userspace, what happens if a
user passes counterid_mask=1 for ANY event type, such as PERF_TYPE_HW_CACHE?

Will the kernel see bit 0 set, map the event to the fixed cycle counter
(idx 0), and silently return CPU cycle counts instead of rejecting the
invalid counter constraint?

> +		return -EINVAL;
> +	}
> +
> +	/* Take the fixed counter only if delegated and free, else fall back. */
> +	if (!(cmask & BIT(idx)) || test_bit(idx, cpuc->used_hw_ctrs))
> +		return -EINVAL;
> +
> +	return idx;
>  }
>  
>  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]
For host events, get_deleg_fixed_hw_idx() above allocates fixed counters
solely based on event->attr.config2. If a standard tool requests
PERF_TYPE_HARDWARE (like generic cycles) where config2=0, won't it fall
back here?

Then in get_deleg_next_hpm_hw_idx(), the fixed counters are explicitly
masked out (~0x7). If a vendor correctly maps cycles exclusively to the
fixed counter in the driver table, the fallback path results in a mask
of 0, returning -EINVAL.

Does this prevent generic legacy hardware events from using fixed
counters on the host, contrary to the commit message?

> +
> +	switch (type) {

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622-counter_delegation-v7-0-0ba2fd34614e@meta.com?part=16