lib: sbi_pmu: Add FW counter index validation when reading high bits on RV64
PATCH 1/1 lib: sbi_pmu: Add FW counter index validation when reading high bits on RV64
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.
--- include/sbi/sbi_pmu.h | 2 ++ lib/sbi/sbi_ecall_pmu.c | 1 + lib/sbi/sbi_pmu.c | 8 ++++++++ 3 files changed, 11 insertions(+)
include/sbi/sbi_pmu.h +2 -0
@@ -138,6 +138,8 @@ int sbi_pmu_add_raw_event_counter_map(uint64_t select, uint64_t select_mask, u32int 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);
lib/sbi/sbi_ecall_pmu.c +1 -0
@@ -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 ;
lib/sbi/sbi_pmu.c +8 -0
@@ -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;
Signature · 7 lines
2.43.0 -- opensbi mailing list [email protected] http://lists.infradead.org/mailman/listinfo/opensbi
2026-01-17T20:50:31+08:00, James Raphael Tiovalen <[email protected]>:
Quoted 10 lines
Currently, when we attempt to read the upper 32 bits of a firmware counter on RV64 or higher, we just set
sbiret.valueto 0 without validating the counter index. The SBI specification requires us to setsbiret.errortoSBI_ERR_INVALID_PARAMif 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 <[email protected]>---
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.
lib/sbi/sbi_pmu.c static bool pmu_ctr_idx_validate(unsigned long cbase, unsigned long cmask) · line 228
Quoted 4 lines
lib/sbi/sbi_pmu.c +0
-0@@ -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:
lib/sbi/sbi_pmu.c static bool pmu_ctr_idx_validate(unsigned long cbase, unsigned long cmask) · line 231
Quoted 2 lines
lib/sbi/sbi_pmu.c +2
-0+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.)
lib/sbi/sbi_pmu.c static bool pmu_ctr_idx_validate(unsigned long cbase, unsigned long cmask) · line 233
Quoted 2 lines
lib/sbi/sbi_pmu.c +2
-0+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.
Signature · 3 lines
opensbi mailing list [email protected] http://lists.infradead.org/mailman/listinfo/opensbi
Quoted 9 lines · Radim Krčmář
2026-01-17T20:50:31+08:00, James Raphael Tiovalen <[email protected]>:Quoted 7 lines
Currently, when we attempt to read the upper 32 bits of a firmware counter on RV64 or higher, we just set
sbiret.valueto 0 without validating the counter index. The SBI specification requires us to setsbiret.errortoSBI_ERR_INVALID_PARAMif 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.
Quoted 9 lines
Quoted 3 lines
Fixes 51951d9e9af8 ("lib: sbi_pmu: Implement sbi_pmu_counter_fw_read_hi")Signed-off-by James Raphael Tiovalen <[email protected]>---
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.
Quoted 4 lines
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.
Quoted 7 lines
lib/sbi/sbi_pmu.c static bool pmu_ctr_idx_validate(unsigned long cbase, unsigned long cmask) · line 228
Quoted 4 lines
lib/sbi/sbi_pmu.c +0
-0@@ -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:
Quoted 8 lines
lib/sbi/sbi_pmu.c static bool pmu_ctr_idx_validate(unsigned long cbase, unsigned long cmask) · line 231
Quoted 2 lines
lib/sbi/sbi_pmu.c +2
-0+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, assbi_pmu_ctr_fw_readhas the
same bug, but better fix related code when touching it.)
Sure, I will include the fix for this in v2 then.
Quoted 6 lines
lib/sbi/sbi_pmu.c static bool pmu_ctr_idx_validate(unsigned long cbase, unsigned long cmask) · line 233
Quoted 2 lines
lib/sbi/sbi_pmu.c +2
-0+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.
Quoted 1 line
Thanks.
Thank you.
Best regards,
James Raphael Tiovalen
Signature · 3 lines
opensbi mailing list [email protected] http://lists.infradead.org/mailman/listinfo/opensbi
2026-01-20T01:32:54+08:00, James R T <[email protected]>:
Quoted 21 lines
Quoted 9 lines · Radim Krčmář
2026-01-17T20:50:31+08:00, James Raphael Tiovalen <[email protected]>:Quoted 7 lines
Currently, when we attempt to read the upper 32 bits of a firmware counter on RV64 or higher, we just set
sbiret.valueto 0 without validating the counter index. The SBI specification requires us to setsbiret.errortoSBI_ERR_INVALID_PARAMif 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.Quoted 9 lines
Quoted 3 lines
Fixes 51951d9e9af8 ("lib: sbi_pmu: Implement sbi_pmu_counter_fw_read_hi")Signed-off-by James Raphael Tiovalen <[email protected]>---
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.
Quoted 5 lines
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.
Quoted 8 lines
Quoted 4 lines
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.
Signature · 3 lines
opensbi mailing list [email protected] http://lists.infradead.org/mailman/listinfo/opensbi