Re: [PATCH v8 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 3 potential issue(s) to consider:
- [High] Legacy perf tools fail to profile hardware cycle and instruction events on platforms with counter delegation, rendering fixed counters and vendor mappings unusable.
- [High] Standard hardware events (PERF_TYPE_HARDWARE) fail unconditionally on platforms supporting counter delegation unless a vendor-specific event table is present.
- [Medium] The 64-bit `event->attr.config2` mask is truncated to a 32-bit integer `temp_mask`, causing invalid user constraints to be silently bypassed.
--

commit 25bb8be42a29d054e5ce833c3ed3ff8935990ee1
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
performance events and hardware counters via the config2 attribute. Legacy
events can either map to ISA-defined fixed counters or fall back to
alternate programmable counters as specified by the platform driver.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index fcf8fbb6fd864..19d9e47504247 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 {
> +		return -EINVAL;
> +	}

[Severity: High]
Does this break legacy perf tools (like 'perf stat -e cycles') on platforms
with counter delegation?

Older tools don't set config2, so legacy events without config2 set are
explicitly rejected here. The code then falls back to
get_deleg_next_hpm_hw_idx(), which excludes fixed counters:

    hw_ctr_mask = cmask & (~0x7) & ~(cpuc->used_hw_ctrs[0]);

If the vendor table mapped the event to a fixed counter, the mask evaluation
will result in 0, causing the allocation to fail with -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]);
> +
> +	switch (type) {
> +	case PERF_TYPE_HARDWARE:
> +		temp_mask = current_pmu_hw_event_map[config].counter_mask;

[Severity: High]
Do standard hardware events (PERF_TYPE_HARDWARE) fail unconditionally on
platforms supporting counter delegation if a vendor-specific event table is
missing?

In rvpmu_event_map(), it appears that all non-firmware events are intercepted
when counter delegation is available:

    if (riscv_pmu_cdeleg_available() && !pmu_sbi_is_fw_event(event))
        return rvpmu_cdeleg_event_map(event, econfig);

Then in rvpmu_cdeleg_event_map(), it hard-fails standard hardware events if
the vendor table is missing:

    case PERF_TYPE_HARDWARE:
        if (config >= PERF_COUNT_HW_MAX)
            return -EINVAL;
        if (!current_pmu_hw_event_map)
            return -ENOENT;

Without a fallback to the SBI PMU, does this completely break legacy hardware
events on new platforms that don't yet have a vendor-specific driver table?

> +		break;
> +	case PERF_TYPE_HW_CACHE:
> +		ret = cdeleg_pmu_event_find_cache(config, NULL, &temp_mask);
> +		if (ret)
> +			return ret;
> +		break;
> +	case PERF_TYPE_RAW:
> +		/*
> +		 * Mask off the counters that can't monitor this event (specified via json)
> +		 * The counter mask for this event is set in config2 via the property 'Counter'
> +		 * in the json file or manual configuration of config2. If the config2 is not set,
> +		 * it is assumed all the available hpmcounters can monitor this event.
> +		 * Note: This assumption may fail for virtualization use case where they hypervisor
> +		 * (e.g. KVM) virtualizes the counter. Any event to counter mapping provided by the
> +		 * guest is meaningless from a hypervisor perspective. Thus, the hypervisor doesn't
> +		 * set config2 when creating kernel counter and relies default host mapping.
> +		 */
> +		if (event->attr.config2)
> +			temp_mask = event->attr.config2;

[Severity: Medium]
Will this silently bypass user-provided counter constraints if only bits above
31 are set in config2?

event->attr.config2 is a u64, but temp_mask is defined as a u32. When config2
is assigned to temp_mask, the upper 32 bits are truncated. If the resulting
temp_mask evaluates to 0, it skips the constraint application entirely:

    if (temp_mask)
        hw_ctr_mask &= temp_mask;

Would this result in the kernel incorrectly programming the event on any
available programmable counter instead of rejecting the invalid constraint?

> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (temp_mask)
> +		hw_ctr_mask &= temp_mask;
> +
> +	if (!hw_ctr_mask)
> +		return -EINVAL;
>  
> -	/* Select only hpmcounters */
> -	hw_ctr_mask = cmask & (~0x7);
> -	hw_ctr_mask &= ~(cpuc->used_hw_ctrs[0]);
>  	return __ffs(hw_ctr_mask);
>  }

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