[PATCH v2] lib: sbi: pmu: Return SBI_EINVAL if cidx_mask is 0 in ctr_cfg_match

[Atish Patra]

On 5/12/25 2:18 AM, James Raphael Tiovalen wrote:
> Currently, when configuring a matching programmable HPM counter with
> Sscofpmf being present, cidx_base > 2, and cidx_mask == 0 to monitor
> either the CPU_CYCLES or INSTRUCTIONS hardware event, ctr_cfg_match will
> succeed but it will configure the corresponding fixed counter instead of
> the counter specified by the cidx_base parameter.
>
> During counter configuration, the following issues may arise:
> - If the CLEAR_VALUE flag is set, the corresponding fixed counter will
> be reset, which could be considered unexpected behavior.
>
> - If the AUTO_START flag is set, ctr_start_hw will silently start the
> fixed counter, even though it has already started. From the supervisor's
> perspective, nothing has changed, which could be confusing. The
> supervisor will not see the SBI_ERR_ALREADY_STARTED error code since
> ctr_cfg_match does not return the error code of ctr_start_hw.
>
> The only way to detect these issues is to check the counter_idx return
> value of ctr_cfg_match and compare it with cidx_base.
>
> Fix these issues by returning the SBI_ERR_INVALID_PARAM error code if
> the cidx_mask parameter value being passed in is 0 since an invalid
> parameter should not lead to a successful ctr_cfg_match but with
> unexpected side effects. This also aligns OpenSBI's behavior with KVM's.
>
> Signed-off-by: James Raphael Tiovalen <jamestiotio at gmail.com>
> ---
>   lib/sbi/sbi_pmu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
> index 5983a78..62ce770 100644
> --- a/lib/sbi/sbi_pmu.c
> +++ b/lib/sbi/sbi_pmu.c
> @@ -840,7 +840,7 @@ int sbi_pmu_ctr_cfg_match(unsigned long cidx_base, unsigned long cidx_mask,
>   	u32 event_code;
>   
>   	/* Do a basic sanity check of counter base & mask */
> -	if ((cidx_base + sbi_fls(cidx_mask)) >= total_ctrs)
> +	if (!cidx_mask || ((cidx_base + sbi_fls(cidx_mask)) >= total_ctrs))
>   		return SBI_EINVAL;
>   
>   	event_type = pmu_event_validate(phs, event_idx, event_data);


Nice catch. Thanks for the fix. I think we need the same fix in 
sbi_pmu_ctr_start/stop.
sbi_fls behavior is undefined when the there are no set bits. So 
checking the mask is a correct thing do anyways.

Regards,
Atish