Skip to contentSkip to conversation

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

PATCH Applied James Raphael Tiovalen 2 participants 2 messages Jan 25 – Feb 22

lore
Share & tools
lore
#0 lore raw

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

Fixes 51951d9e9af8 ("lib: sbi_pmu: Implement sbi_pmu_counter_fw_read_hi")

---
v1 -> v2:
- Implemented Radim's suggestion to refactor sbi_pmu_ctr_fw_read() instead of adding a new function.
- Updated the type of cidx in the function signature of

    `sbi_pmu_ctr_fw_read()`.
---
 include/sbi/sbi_pmu.h   |  2 +-
 lib/sbi/sbi_ecall_pmu.c |  4 ++--
 lib/sbi/sbi_pmu.c       | 10 +++++++++-
 3 files changed, 12 insertions(+), 4 deletions(-)
include/sbi/sbi_pmu.h +1 -1
index c0e25f5a..3fff4627 100644
@@ -136,7 +136,7 @@ int sbi_pmu_add_hw_event_counter_map(u32 eidx_start, u32 eidx_end, u32 cmap);
 
 int sbi_pmu_add_raw_event_counter_map(uint64_t select, uint64_t select_mask, u32 cmap);
 
-int sbi_pmu_ctr_fw_read(uint32_t cidx, uint64_t *cval);
+int sbi_pmu_ctr_fw_read(unsigned long cidx, uint64_t *cval, bool high_bits);
 
 int sbi_pmu_ctr_stop(unsigned long cidx_base, unsigned long cidx_mask,
 		     unsigned long flag);
lib/sbi/sbi_ecall_pmu.c +2 -2
index 868e8665..41bb1624 100644
@@ -50,12 +50,12 @@ static int sbi_ecall_pmu_handler(unsigned long extid, unsigned long funcid,
 
 		break;
 	case SBI_EXT_PMU_COUNTER_FW_READ:
-		ret = sbi_pmu_ctr_fw_read(regs->a0, &temp);
+		ret = sbi_pmu_ctr_fw_read(regs->a0, &temp, false);
 		out->value = temp;
 		break;
 	case SBI_EXT_PMU_COUNTER_FW_READ_HI:
+		ret = sbi_pmu_ctr_fw_read(regs->a0, &temp, true);
 #if __riscv_xlen == 32
-		ret = sbi_pmu_ctr_fw_read(regs->a0, &temp);
 		out->value = temp >> 32;
 #else
 		out->value = 0;
lib/sbi/sbi_pmu.c +9 -1
index e084005d..a310553e 100644
@@ -227,7 +227,7 @@ static bool pmu_ctr_idx_validate(unsigned long cbase, unsigned long cmask)
 	return cmask && cbase + sbi_fls(cmask) < total_ctrs;
 }
 
-int sbi_pmu_ctr_fw_read(uint32_t cidx, uint64_t *cval)
+int sbi_pmu_ctr_fw_read(unsigned long cidx, uint64_t *cval, bool high_bits)
 {
 	int event_idx_type;
 	uint32_t event_code;
@@ -236,6 +236,14 @@ int sbi_pmu_ctr_fw_read(uint32_t cidx, uint64_t *cval)
 	if (unlikely(!phs))
 		return SBI_EINVAL;
 
+	if (cidx < num_hw_ctrs || cidx >= total_ctrs)
+		return SBI_EINVAL;
+
+#if __riscv_xlen > 32
+	if (high_bits)
+		return 0;
+#endif
+
 	event_idx_type = pmu_ctr_validate(phs, cidx, &event_code);
 	if (event_idx_type != SBI_PMU_EVENT_TYPE_FW)
 		return SBI_EINVAL;
Signature · 7 lines
2.43.0


-- 
opensbi mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/opensbi
Reviewed-by on v2 1/1 Applied
#1 lore raw
Quoted 10 lines · 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")

LGTM.

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

Quoted 80 lines

---
v1 -> v2:
- Implemented Radim's suggestion to refactor sbi_pmu_ctr_fw_read() instead of adding a new function.
- Updated the type of cidx in the function signature of

    `sbi_pmu_ctr_fw_read()`.
---
 include/sbi/sbi_pmu.h   |  2 +-
 lib/sbi/sbi_ecall_pmu.c |  4 ++--
 lib/sbi/sbi_pmu.c       | 10 +++++++++-
 3 files changed, 12 insertions(+), 4 deletions(-)
include/sbi/sbi_pmu.h +1 -1
index c0e25f5a..3fff4627 100644
@@ -136,7 +136,7 @@ int sbi_pmu_add_hw_event_counter_map(u32 eidx_start, u32 eidx_end, u32 cmap);
 
 int sbi_pmu_add_raw_event_counter_map(uint64_t select, uint64_t select_mask, u32 cmap);
 
-int sbi_pmu_ctr_fw_read(uint32_t cidx, uint64_t *cval);
+int sbi_pmu_ctr_fw_read(unsigned long cidx, uint64_t *cval, bool high_bits);
 
 int sbi_pmu_ctr_stop(unsigned long cidx_base, unsigned long cidx_mask,
                     unsigned long flag);
lib/sbi/sbi_ecall_pmu.c +2 -2
index 868e8665..41bb1624 100644
@@ -50,12 +50,12 @@ static int sbi_ecall_pmu_handler(unsigned long extid, unsigned long funcid,
 
                break;
        case SBI_EXT_PMU_COUNTER_FW_READ:
-               ret = sbi_pmu_ctr_fw_read(regs->a0, &temp);
+               ret = sbi_pmu_ctr_fw_read(regs->a0, &temp, false);
                out->value = temp;
                break;
        case SBI_EXT_PMU_COUNTER_FW_READ_HI:
+               ret = sbi_pmu_ctr_fw_read(regs->a0, &temp, true);
 #if __riscv_xlen == 32
-               ret = sbi_pmu_ctr_fw_read(regs->a0, &temp);
                out->value = temp >> 32;
 #else
                out->value = 0;
lib/sbi/sbi_pmu.c +9 -2
index e084005d..a310553e 100644
@@ -227,7 +227,7 @@ static bool pmu_ctr_idx_validate(unsigned long cbase, unsigned long cmask)
        return cmask && cbase + sbi_fls(cmask) < total_ctrs;
 }
 
-int sbi_pmu_ctr_fw_read(uint32_t cidx, uint64_t *cval)
+int sbi_pmu_ctr_fw_read(unsigned long cidx, uint64_t *cval, bool high_bits)
 {
        int event_idx_type;
        uint32_t event_code;
@@ -236,6 +236,14 @@ int sbi_pmu_ctr_fw_read(uint32_t cidx, uint64_t *cval)
        if (unlikely(!phs))
                return SBI_EINVAL;
 
+       if (cidx < num_hw_ctrs || cidx >= total_ctrs)
+               return SBI_EINVAL;
+
+#if __riscv_xlen > 32
+       if (high_bits)
+               return 0;
+#endif
+
        event_idx_type = pmu_ctr_validate(phs, cidx, &event_code);
        if (event_idx_type != SBI_PMU_EVENT_TYPE_FW)
                return SBI_EINVAL;
--

2.43.0

--
opensbi mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/opensbi

Signature · 3 lines
opensbi mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/opensbi