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

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

[James Raphael Tiovalen]

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>
---
 include/sbi/sbi_pmu.h   | 2 ++
 lib/sbi/sbi_ecall_pmu.c | 1 +
 lib/sbi/sbi_pmu.c       | 8 ++++++++
 3 files changed, 11 insertions(+)

diff --git a/include/sbi/sbi_pmu.h b/include/sbi/sbi_pmu.h
index c0e25f5a..51d4a345 100644
--- a/include/sbi/sbi_pmu.h
+++ b/include/sbi/sbi_pmu.h
@@ -138,6 +138,8 @@ int sbi_pmu_add_raw_event_counter_map(uint64_t select, uint64_t select_mask, u32
 
 int sbi_pmu_ctr_fw_read(uint32_t cidx, uint64_t *cval);
 
+int sbi_pmu_is_fw_ctr_idx(uint32_t cidx);
+
 int sbi_pmu_ctr_stop(unsigned long cidx_base, unsigned long cidx_mask,
 		     unsigned long flag);
 
diff --git a/lib/sbi/sbi_ecall_pmu.c b/lib/sbi/sbi_ecall_pmu.c
index 868e8665..0765d959 100644
--- a/lib/sbi/sbi_ecall_pmu.c
+++ b/lib/sbi/sbi_ecall_pmu.c
@@ -58,6 +58,7 @@ static int sbi_ecall_pmu_handler(unsigned long extid, unsigned long funcid,
 		ret = sbi_pmu_ctr_fw_read(regs->a0, &temp);
 		out->value = temp >> 32;
 #else
+		ret = sbi_pmu_is_fw_ctr_idx(regs->a0);
 		out->value = 0;
 #endif
 		break;
diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
index e084005d..d7ebe4e2 100644
--- 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;
 }
 
+int sbi_pmu_is_fw_ctr_idx(uint32_t cidx)
+{
+	if (cidx < num_hw_ctrs || cidx >= total_ctrs)
+		return SBI_EINVAL;
+
+	return 0;
+}
+
 int sbi_pmu_ctr_fw_read(uint32_t cidx, uint64_t *cval)
 {
 	int event_idx_type;
-- 
2.43.0


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

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

[Radim Krčmář]

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

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

[James R T]

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.

> 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.

> > 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.)
>

Sure, I will include the fix for this in v2 then.

> > +     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.
>

Got it. Will do that in v2.

> Thanks.

Thank you.

Best regards,
James Raphael Tiovalen

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

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

[Radim Krčmář]

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