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-20T01:32:54+08:00, James R T <jamestiotio@gmail.com>:
> On Mon, Jan 19, 2026 at 7:57 PM Radim Krčmář
> <radim.krcmar@oss.qualcomm.com> wrote:
>>
>> 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 discovered this when I was writing some PMU SBI tests for
> KVM-Unit-Tests and, at least to me, it was unexpected. I understand
> the intention behind implementing it in the simplest way possible, but
> it did seem to deviate from the specification, at least with my way of
> reading it.

If you can argue that the current behavior is against the specification,
then we obviously should fix it, but I cannot find any ground for it.

Similarly, I cannot find anything wrong with your logic, but it took a
while -- at first, I didn't think that we can apply the SBI clause that
allows exceptional functions to return specified value along an error
code.

The fun with informal specifications is that all valid interpretations
are correct, even if not expected nor intended.

Since the explicitly returned value of 0 is meaningless, and not related
to any counter_idx, we do have a lot of freedom.
I just prefer the current 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.
>>
>
> If it is better to return {SBI_EINVAL, 0}, then let's do it. That
> said, I think we should only return that on an invalid counter index,
> not always.

I agree with you that always returning SBI_EINVAL is definitely not the
expected reading of the specification.
I wouldn't change it to always SBI_EINVAL either as someone might depend
on the current behavior, and I don't think the benefit is worth the risk
of breakage.

Thanks.

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