Re: [PATCH v8 12/22] RISC-V: perf: Modify the counter discovery mechanism

[sashiko-bot@kernel.org]

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

New issues:
- [High] Hardware counter CSR indices and firmware counter SBI logical indices share the same index namespace, leading to silent overwriting of hardware counters in `pmu_ctr_list`.
- [High] Bitwise shift overflow on 32-bit architectures when processing PMU firmware counters with indices >= 32.
- [Medium] A failure to discover firmware counters unconditionally aborts PMU initialization, erroneously discarding successfully delegated hardware counters.

Pre-existing issues:
- [High] Invalid counter base index passed to `sbi_ecall` in `rvpmu_sbi_start_ovf_ctrs_snapshot`.
--

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

RISC-V: perf: Modify the counter discovery mechanism

This commit modifies the PMU counter discovery mechanism to support a hybrid
scheme where hardware counters use counter delegation and firmware counters
use the SBI PMU extension. It queries the counters info via SBI PMU to
distinguish the firmware counters and track them in separate bitmasks.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 74d9342388212..c20f1e33c65d5 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
[ ... ]
> @@ -868,34 +885,38 @@ static int rvpmu_sbi_find_num_ctrs(void)
>  		return sbi_err_map_linux_errno(ret.error);
>  }
>  
> -static int rvpmu_sbi_get_ctrinfo(int nctr, unsigned long *mask)
> +static u32 rvpmu_deleg_find_ctrs(void)
> +{
> +	/* TODO */
> +	return 0;
> +}
> +
> +static int rvpmu_sbi_get_ctrinfo(u32 nsbi_ctr, u32 *num_fw_ctr, u32 *num_hw_ctr)
>  {
>  	struct sbiret ret;
> -	int i, num_hw_ctr = 0, num_fw_ctr = 0;
> +	int i;
>  	union sbi_pmu_ctr_info cinfo;
>  
> -	for (i = 0; i < nctr; i++) {
> +	for (i = 0; i < nsbi_ctr; i++) {
>  		ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_GET_INFO, i, 0, 0, 0, 0, 0);
>  		if (ret.error)
>  			/* The logical counter ids are not expected to be contiguous */
>  			continue;
>  
>  		cinfo.value = ret.value;
> -		if (cinfo.type == SBI_PMU_CTR_TYPE_FW)
> -			num_fw_ctr++;
> -		else
> -			num_hw_ctr++;
> -		pmu_ctr_list[i].value = cinfo.value;
> +		if (cinfo.type == SBI_PMU_CTR_TYPE_FW) {
> +			/* Track firmware counters in a different mask */
> +			firmware_cmask |= BIT(i);

[Severity: High]
Does this code trigger a bitwise shift out of bounds on 32-bit architectures?

The firmware_cmask variable is an unsigned long, which is 32 bits wide on RV32.
Since the SBI PMU specification allows up to 64 counters, and standard firmware
implementations (like OpenSBI) typically assign firmware counters to logical
indices 32 and above, the BIT(i) macro will execute a shift greater than or
equal to the width of the type when i >= 32. 

Will this cause firmware counters to be dropped or corrupted on 32-bit
platforms?

> +			pmu_ctr_list[i].value = cinfo.value;

[Severity: High]
Can this overwrite hardware counter entries in the pmu_ctr_list array?

The rvpmu_deleg_find_ctrs() function iterates over hardware counters and maps
them using their architectural CSR indices (0-31). The rvpmu_sbi_get_ctrinfo()
function then maps firmware counters using their SBI logical indices ('i'
from 0 to N-1).

Since the SBI PMU specification does not restrict firmware counters to indices
>= 32, if a platform assigns a firmware counter to a logical index <= 31,
wouldn't using 'i' directly here silently overwrite the delegated hardware
counter entry?

> +			*num_fw_ctr = *num_fw_ctr + 1;
> +		} else if (cinfo.type == SBI_PMU_CTR_TYPE_HW &&
> +			   !riscv_pmu_cdeleg_available_boot()) {
> +			*num_hw_ctr = *num_hw_ctr + 1;
> +			cmask |= BIT(i);
> +			pmu_ctr_list[i].value = cinfo.value;
> +		}
>  	}
>  
>  	return 0;
>  }
[ ... ]
> @@ -1159,16 +1180,48 @@ static void rvpmu_ctr_stop(struct perf_event *event, unsigned long flag)
>  	/* TODO: Counter delegation implementation */
>  }
>  
> -static int rvpmu_find_num_ctrs(void)
> +static int rvpmu_find_ctrs(void)
>  {
> -	return rvpmu_sbi_find_num_ctrs();
> -	/* TODO: Counter delegation implementation */
> -}
> +	int num_sbi_counters = 0;
> +	u32 num_deleg_counters = 0;
> +	u32 num_hw_ctr = 0, num_fw_ctr = 0, num_ctr = 0;
> +	/*
> +	 * We don't know how many firmware counters are available. Just allocate
> +	 * for maximum counters the driver can support. The default is 64 anyways.
> +	 */
> +	pmu_ctr_list = kcalloc(RISCV_MAX_COUNTERS, sizeof(*pmu_ctr_list),
> +			       GFP_KERNEL);
> +	if (!pmu_ctr_list)
> +		return -ENOMEM;
>  
> -static int rvpmu_get_ctrinfo(int nctr, unsigned long *mask)
> -{
> -	return rvpmu_sbi_get_ctrinfo(nctr, mask);
> -	/* TODO: Counter delegation implementation */
> +	if (riscv_pmu_cdeleg_available_boot())
> +		num_deleg_counters = rvpmu_deleg_find_ctrs();
> +
> +	/* This is required for firmware counters even if the above is true */
> +	if (riscv_pmu_sbi_available_boot()) {
> +		num_sbi_counters = rvpmu_sbi_find_num_ctrs();
> +		if (num_sbi_counters < 0) {
> +			kfree(pmu_ctr_list);
> +			pmu_ctr_list = NULL;
> +			return num_sbi_counters;

[Severity: Medium]
Should this gracefully continue with the available delegated hardware counters
instead of aborting PMU initialization?

If counter delegation for hardware performance counters succeeds, but the
optional firmware extension returns an error via rvpmu_sbi_find_num_ctrs(),
this error path frees the entire pmu_ctr_list array and returns an error.

This unnecessarily disables the system's hardware performance monitoring
instead of simply ignoring the broken firmware counters.

> +		}
> +		if (num_sbi_counters > RISCV_MAX_COUNTERS)
> +			num_sbi_counters = RISCV_MAX_COUNTERS;
> +	}
[ ... ]

[Severity: High]
This is a pre-existing issue, but there appears to be a logic error in
rvpmu_sbi_start_ovf_ctrs_snapshot() when passing the counter base index to
the firmware:

drivers/perf/riscv_pmu_sbi.c:rvpmu_sbi_start_ovf_ctrs_snapshot() {
    ...
    for (i = 0; i < BITS_TO_LONGS(RISCV_MAX_COUNTERS); i++) {
        ...
        for_each_set_bit(idx, &cpu_hw_evt->used_hw_ctrs[i], BITS_PER_LONG)
            sdata->ctr_values[idx] =
                cpu_hw_evt->snapshot_cval_shcopy[idx + i * BITS_PER_LONG];
        
        /* Start all the counters in a single shot */
        sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_START, idx * BITS_PER_LONG,
                  cpu_hw_evt->used_hw_ctrs[i], flag, 0, 0, 0);
    ...
}

Because idx is the exhausted iterator of for_each_set_bit(), its value will be
BITS_PER_LONG. When multiplied by BITS_PER_LONG, the base index evaluates to
an invalid out-of-bounds value (e.g., 4096 on RV64). 

Should this pass i * BITS_PER_LONG instead of idx * BITS_PER_LONG to correctly
start the chunk?

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