[PATCH] perf: riscv: fix PMU overflow counter restart state

[PATCH] perf: riscv: fix PMU overflow counter restart state

[Pengpeng Hou]

The PMU overflow handler keeps a 64-bit mask for all logical counters,
while SBI start calls consume an XLEN-sized mask relative to a counter
base.

On RV32, pmu_sbi_start_ovf_ctrs_sbi() applies the low overflow-mask word
to every used-counter word. Counters 32 through 63 can therefore be
classified using overflow bits 0 through 31.

The snapshot path and overflow handler also test and construct 64-bit
masks with XLEN-sized BIT(). In addition, snapshot restart uses the
completed inner-loop index instead of the outer word index to form
counter_idx_base. The latter selects an invalid counter base on both RV32
and RV64: for_each_set_bit() finishes with idx equal to BITS_PER_LONG, so
the current expression passes BITS_PER_LONG squared instead of the
expected i times BITS_PER_LONG.

Select the overflow word matching each SBI mask, use 64-bit bit operations
for global counter indices, and derive the snapshot start base from the
outer word index.

Fixes: b994cdfcdf7b ("drivers/perf: riscv: Fix counter mask iteration for RV32")
Fixes: a8625217a054 ("drivers/perf: riscv: Implement SBI PMU snapshot function")
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/perf/riscv_pmu_sbi.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
index 50220f7b46d9..6cfb1fc0ec6e 100644
--- a/drivers/perf/riscv_pmu_sbi.c
+++ b/drivers/perf/riscv_pmu_sbi.c
@@ -962,7 +962,9 @@ static inline void pmu_sbi_start_ovf_ctrs_sbi(struct cpu_hw_events *cpu_hw_evt,
 	u64 init_val = 0;
 
 	for (i = 0; i < BITS_TO_LONGS(RISCV_MAX_COUNTERS); i++) {
-		ctr_start_mask = cpu_hw_evt->used_hw_ctrs[i] & ~ctr_ovf_mask;
+		unsigned long word_ovf_mask =
+			ctr_ovf_mask >> (i * BITS_PER_LONG);
+		ctr_start_mask = cpu_hw_evt->used_hw_ctrs[i] & ~word_ovf_mask;
 		/* Start all the counters that did not overflow in a single shot */
 		if (ctr_start_mask) {
 			sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_START, i * BITS_PER_LONG,
@@ -1002,7 +1004,7 @@ static inline void pmu_sbi_start_ovf_ctrs_snapshot(struct cpu_hw_events *cpu_hw_
 	struct riscv_pmu_snapshot_data *sdata = cpu_hw_evt->snapshot_addr;
 
 	for_each_set_bit(idx, cpu_hw_evt->used_hw_ctrs, RISCV_MAX_COUNTERS) {
-		if (ctr_ovf_mask & BIT(idx)) {
+		if (ctr_ovf_mask & BIT_ULL(idx)) {
 			event = cpu_hw_evt->events[idx];
 			hwc = &event->hw;
 			max_period = riscv_pmu_ctr_get_width_mask(event);
@@ -1021,7 +1023,7 @@ static inline void pmu_sbi_start_ovf_ctrs_snapshot(struct cpu_hw_events *cpu_hw_
 			sdata->ctr_values[idx] =
 					cpu_hw_evt->snapshot_cval_shcopy[idx + i * BITS_PER_LONG];
 		/* Start all the counters in a single shot */
-		sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_START, idx * BITS_PER_LONG,
+		sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_START, i * BITS_PER_LONG,
 			  cpu_hw_evt->used_hw_ctrs[i], flag, 0, 0, 0);
 	}
 }
@@ -1109,14 +1111,14 @@ static irqreturn_t pmu_sbi_ovf_handler(int irq, void *dev)
 			hidx = info->csr - CSR_CYCLE;
 
 		/* check if the corresponding bit is set in scountovf or overflow mask in shmem */
-		if (!(overflow & BIT(hidx)))
+		if (!(overflow & BIT_ULL(hidx)))
 			continue;
 
 		/*
 		 * Keep a track of overflowed counters so that they can be started
 		 * with updated initial value.
 		 */
-		overflowed_ctrs |= BIT(lidx);
+		overflowed_ctrs |= BIT_ULL(lidx);
 		hw_evt = &event->hw;
 		/* Update the event states here so that we know the state while reading */
 		hw_evt->state |= PERF_HES_STOPPED;

base-commit: 1b78070aaef63512688aebfbc82365ef9d6660f1
-- 
2.43.0

Re: [PATCH] perf: riscv: fix PMU overflow counter restart state

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [High] Accumulation of bits in `ctr_overflow_mask` causes phantom PMU overflows and infinite loops.
- [High] Batch PMU operations clobber stopped counter snapshot values on RV32.
- [High] Using BIT() for a potentially large IRQ number causes undefined behavior and fails to clear the interrupt.
- [High] The 'cmask' variable is undersized on RV32, causing out-of-bounds reads and undefined behavior.
- [Medium] Mapping firmware counters unintentionally enables/disables userspace access to the hardware cycle counter.
--

commit 5c2027925f34b7a9d276d3526dd841300147c576
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

perf: riscv: fix PMU overflow counter restart state

This commit fixes PMU overflow counter restart states on RISC-V by applying
the correct overflow-mask word to each used-counter word. It updates the
snapshot path and overflow handler to use 64-bit bit operations for global
counter indices. Additionally, it ensures the snapshot start base is correctly
derived from the outer word index.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 50220f7b46d9b..6cfb1fc0ec6e1 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -962,7 +962,9 @@ static inline void pmu_sbi_start_ovf_ctrs_sbi(struct cpu_hw_events *cpu_hw_evt,
>  	u64 init_val = 0;
>  
>  	for (i = 0; i < BITS_TO_LONGS(RISCV_MAX_COUNTERS); i++) {
> -		ctr_start_mask = cpu_hw_evt->used_hw_ctrs[i] & ~ctr_ovf_mask;
> +		unsigned long word_ovf_mask =
> +			ctr_ovf_mask >> (i * BITS_PER_LONG);
> +		ctr_start_mask = cpu_hw_evt->used_hw_ctrs[i] & ~word_ovf_mask;
>  		/* Start all the counters that did not overflow in a single shot */
>  		if (ctr_start_mask) {
>  			sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_START, i * BITS_PER_LONG,

[ ... ]

[Severity: High]
This is a pre-existing issue, but can the 64-bit mask corruption cause phantom
PMU overflows and infinite loops?

On RV32, when stopping counters across multiple words:

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_stop_hw_ctrs() {
...
    temp_ctr_overflow_mask |= sdata->ctr_overflow_mask << (i * BITS_PER_LONG);
...
    sdata->ctr_overflow_mask = temp_ctr_overflow_mask;
...
}

The OS fails to clear the 64-bit mask sdata->ctr_overflow_mask before calling
SBI_EXT_PMU_COUNTER_STOP for i=1. Firmware ORs relative overflow bits into
this uncleared mask, mixing base 0 and base 32 overflows.

Does the kernel then shift and OR this corrupted mask into
temp_ctr_overflow_mask, accumulating stale bits indefinitely and causing
phantom interrupts?

[Severity: High]
This is a pre-existing issue, but does this batch PMU operation clobber
stopped counter snapshot values on RV32?

If all counters in used_hw_ctrs[0] are already stopped,
SBI_EXT_PMU_COUNTER_STOP returns an error, skipping the save of their
snapshot values to snapshot_cval_shcopy:

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_stop_hw_ctrs() {
...
    ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_STOP, i * BITS_PER_LONG,
                    cpu_hw_evt->used_hw_ctrs[i], flag, 0, 0, 0);
...
    sdata->ctr_values[idx] = cpu_hw_evt->snapshot_cval_shcopy[idx];
...
}

When i=1 runs for counters 32..63, does the firmware write their snapshot into
sdata->ctr_values[0..31], clobbering the values for counters 0..31?

Does the kernel then restore snapshot_cval_shcopy[0..31] (which remains 0)
into sdata->ctr_values, destroying the saved snapshot of the already stopped
counters?

[Severity: Medium]
This is a pre-existing issue, but does mapping firmware counters
unintentionally toggle userspace access to the hardware cycle counter?

When userspace maps a firmware counter, pmu_sbi_event_mapped() unconditionally
calls pmu_sbi_set_scounteren():

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_set_scounteren() {
...
    csr_write(CSR_SCOUNTEREN,
              csr_read(CSR_SCOUNTEREN) | BIT(pmu_sbi_csr_index(event)));
...
}

This calls pmu_sbi_csr_index(), which subtracts CSR_CYCLE from the
uninitialized csr field of the firmware counter:

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_csr_index() {
...
    return pmu_ctr_list[event->hw.idx].csr - CSR_CYCLE;
...
}

Since the csr field is 0, the result -3072 truncates to 0 in uint8_t.

Does this erroneously toggle SCOUNTEREN[0], inadvertently enabling or
disabling hardware access to the cycle counter?

[Severity: High]
This is a pre-existing issue, but does using BIT() for a potentially large IRQ
number cause undefined behavior here?

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_ovf_handler() {
...
    if (fidx == RISCV_MAX_COUNTERS) {
        csr_clear(CSR_SIP, BIT(riscv_pmu_irq_num));
        return IRQ_NONE;
    }
...
}

For the ANDES custom PMU, riscv_pmu_irq_num is 274. If this path executes,
does shifting by 274 trigger undefined behavior?

Does this also fail to clear the pending IRQ since it targets CSR_SIP instead
of the custom ANDES_CSR_SLIP register correctly handled by
ALT_SBI_PMU_OVF_CLEAR_PENDING?

[Severity: High]
This is a pre-existing issue, but is the 'cmask' variable undersized on RV32,
causing out-of-bounds reads and undefined behavior?

drivers/perf/riscv_pmu_sbi.c:
...
static unsigned long cmask;
...

In pmu_sbi_get_ctrinfo(), BIT(i) is used for i up to 63:

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_get_ctrinfo() {
...
    *mask |= BIT(i);
...
}

And in riscv_pmu_get_hpm_info():

drivers/perf/riscv_pmu_sbi.c:riscv_pmu_get_hpm_info() {
...
    for_each_set_bit(i, &cmask, RISCV_MAX_COUNTERS) {
...
}

Since cmask is an unsigned long, it is 32 bits on RV32.

Does for_each_set_bit() read 64 bits, overrunning the 32-bit cmask variable
into adjacent memory and causing undefined behavior when i >= 32?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828142120.12616-1-pengpeng@iscas.ac.cn?part=1