Re: [PATCH v9 16/20] RISC-V: perf: Use config2/vendor table for event to counter mapping

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Standard host legacy hardware events (e.g., 'perf stat -e cycles') fail to open and cannot map to fixed counters via the vendor driver table.
- [High] Raw events with broad counter masks are unconditionally forced to count cycles or instructions on fixed counters, returning incorrect metrics.
--

--- Patch [16]: [PATCH v9 16/20] RISC-V: perf: Use config2/vendor table for event to counter mapping ---
commit 450780026222524d85a7b60f40dbdc3d52fed837
Author: Atish Patra <[email protected]>
Subject: RISC-V: perf: Use config2/vendor table for event to counter mapping

This patch allows platform vendors to specify custom mapping between
events and hpmcounters using the config2 parameter in perf attributes.
Legacy events are mapped via the vendor event table in the driver, while
the fixed cycle and instruction counters map to legacy events.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index f0b8b6eff2ea7..ee0c077859273 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
[ ... ]
> @@ -1522,24 +1524,85 @@ static int rvpmu_deleg_find_ctrs(void)
>  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 */

[Severity: High]
Does this unconditionally force raw events with broad counter masks to count
cycles instead of the requested metric?

If a raw event is provided with a broad counterid_mask (e.g., 0xFFFFFFFF)
that allows any counter, config2 will have bit 0 set. This causes
get_deleg_fixed_hw_idx() to immediately assign it to fixed counter 0.

Since fixed counter 0 is architecturally hardwired to count cycles, the
actual raw event ID is ignored and the PMU returns cycle counts, leading to
incorrect metrics.

> +	} else if (event->attr.config2 & RISCV_PMU_INSTRUCTION_FIXED_CTR_MASK) {
> +		idx = 2; /* IR counter */
> +	} else {
> +		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]
Will standard host legacy hardware events fail to map to fixed counters if
the vendor map mandates them?

When a user runs a command like "perf stat -e cycles", config2 is 0.
As a result, get_deleg_fixed_hw_idx() ignores current_pmu_hw_event_map and
returns -EINVAL without finding a fixed counter mapping.

This fallback function then explicitly strips out fixed counter bits (~0x7),
which prevents assigning fixed counters even if the vendor table specifically
requests them. If the vendor map requires counter 0 via counterid_mask, the
resulting hw_ctr_mask becomes 0, and the allocation fails entirely with
-ENOENT.

> +
> +	switch (type) {
> +	case PERF_TYPE_HARDWARE:
> +		temp_mask = current_pmu_hw_event_map[config].counterid_mask;
> +		break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=16