Re: [PATCH v8 15/22] RISC-V: perf: Skip PMU SBI extension when not implemented

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Skipping standard event checking when SBI is unavailable leaves event maps with valid default encodings, causing false-positive event support.
--

commit b28b4646a82db47f9cf2c2d617a8c7ae78dfb038
Author: Charlie Jenkins <charlie@rivosinc.com>

RISC-V: perf: Skip PMU SBI extension when not implemented

This commit modifies the RISC-V perf driver to skip the SBI extension for
counter configuration matching and firmware counter reads when the PMU
SBI extension is not implemented. It also prevents sbi_v2_available from
being set to true when the extension is missing.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -495,27 +495,32 @@ static void rvpmu_sbi_check_event(struct sbi_pmu_event_data *edata)
[ ... ]
> -static void rvpmu_sbi_check_std_events(struct work_struct *work)
> +static void rvpmu_check_std_events(struct work_struct *work)
>  {
>  	int ret;
>  
> -	if (sbi_v3_available) {
> -		ret = pmu_sbi_check_event_info();
> -		if (ret)
> -			pr_err("pmu_sbi_check_event_info failed with error %d\n", ret);
> -		return;
> -	}
> +	if (riscv_pmu_sbi_available()) {
> +		if (sbi_v3_available) {
> +			ret = pmu_sbi_check_event_info();
> +			if (ret)
> +				pr_err("pmu_sbi_check_event_info failed with error %d\n", ret);
> +			return;
> +		}
>  
> -	for (int i = 0; i < ARRAY_SIZE(pmu_hw_event_sbi_map); i++)
> -		rvpmu_sbi_check_event(&pmu_hw_event_sbi_map[i]);
> +		for (int i = 0; i < ARRAY_SIZE(pmu_hw_event_sbi_map); i++)
> +			rvpmu_sbi_check_event(&pmu_hw_event_sbi_map[i]);
>  
> -	for (int i = 0; i < ARRAY_SIZE(pmu_cache_event_sbi_map); i++)
> -		for (int j = 0; j < ARRAY_SIZE(pmu_cache_event_sbi_map[i]); j++)
> -			for (int k = 0; k < ARRAY_SIZE(pmu_cache_event_sbi_map[i][j]); k++)
> -				rvpmu_sbi_check_event(&pmu_cache_event_sbi_map[i][j][k]);
> +		for (int i = 0; i < ARRAY_SIZE(pmu_cache_event_sbi_map); i++)
> +			for (int j = 0; j < ARRAY_SIZE(pmu_cache_event_sbi_map[i]); j++)
> +				for (int k = 0; k < ARRAY_SIZE(pmu_cache_event_sbi_map[i][j]); k++)
> +					rvpmu_sbi_check_event(&pmu_cache_event_sbi_map[i][j][k]);
> +	} else {
> +		DO_ONCE_LITE_IF(1, pr_info,
> +				"Boot time config matching not required for smcdeleg\n");
> +	}
>  }

[Severity: High]
When riscv_pmu_sbi_available() is false, does skipping this loop leave the
event map arrays like pmu_hw_event_sbi_map with valid default event codes?

If these standard events are not invalidated via the hardware verification
step, riscv_pmu_get_event_info() could unconditionally return the default
uninvalidated values or synthesize valid encodings for firmware events.
Would this incorrectly signal to callers, such as KVM, that the events
are supported?

[ ... ]
> @@ -708,6 +713,9 @@ static int rvpmu_sbi_ctr_get_idx(struct perf_event *event)
>  
>  	cflags = rvpmu_sbi_get_filter_flags(event);
>  
> +	if (!riscv_pmu_sbi_available())
> +		return -ENOENT;

[Severity: High]
If firmware events successfully map via riscv_pmu_get_event_info() because
the mappings were left intact during initialization, they would be accepted
by perf_event_open. 

Will they then fail silently here at schedule time instead of being
appropriately rejected during event initialization?

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