Re: [PATCH] lib: sbi_pmu: Add FW counter index validation when reading high bits on RV64

["Radim Krčmář" <radim.krcmar@oss.qualcomm.com>]

2026-01-17T20:50:31+08:00, James Raphael Tiovalen <jamestiotio@gmail.com>:
> Currently, when we attempt to read the upper 32 bits of a firmware
> counter on RV64 or higher, we just set `sbiret.value` to 0 without
> validating the counter index. The SBI specification requires us to set
> `sbiret.error` to `SBI_ERR_INVALID_PARAM` if the counter index points to
> a hardware counter or an invalid counter. Add a validation check to
> ensure compliance with the specification on RV64 or higher.
>
> Fixes: 51951d9e9af8 ("lib: sbi_pmu: Implement sbi_pmu_counter_fw_read_hi")
> Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
> ---

While the call is poorly specified and your interpretation is valid,
I think the intention is to make this function do nothing else than
return {err, 0} on RV64 as there isn't much reason to complicate the
implementation.

I think always returning {SBI_EINVAL, 0} would be even better than
the current {SBI_SUCCESS, 0}, but it doesn't matter much since
legitimate RV64 software shouldn't ever invoke the ecall.

> diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
> @@ -227,6 +227,14 @@ static bool pmu_ctr_idx_validate(unsigned long cbase, unsigned long cmask)
>  	return cmask && cbase + sbi_fls(cmask) < total_ctrs;
>  }

In case of disagreement with the above:

> +int sbi_pmu_is_fw_ctr_idx(uint32_t cidx)
> +{

This functions gets passed regs->a0, so cidx should be an xlen sized
type.
(I understand trying to be consistent, as sbi_pmu_ctr_fw_read has the
 same bug, but better fix related code when touching it.)

> +	if (cidx < num_hw_ctrs || cidx >= total_ctrs)
> +		return SBI_EINVAL;

Please refactor the check in sbi_pmu_ctr_fw_read, and use it instead of
adding more logic.

Thanks.

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi