* [PATCH v1 0/2] perf: RISC-V: fix SBI PMU masks for RV32
@ 2026-08-07 8:50 Xixin Liu
2026-08-07 8:50 ` [PATCH v1 1/2] perf: RISC-V: use BIT_ULL for u64 overflow masks Xixin Liu
2026-08-07 8:50 ` [PATCH v1 2/2] perf: RISC-V: store available counter mask as bitmap Xixin Liu
0 siblings, 2 replies; 7+ messages in thread
From: Xixin Liu @ 2026-08-07 8:50 UTC (permalink / raw)
To: linux-riscv
Cc: atish.patra, anup, will, mark.rutland, pjw, palmer, aou, alex,
linux-arm-kernel, linux-perf-users, linux-kernel, liuxixin
Hi,
This series fixes two RV32 mask-width issues in the SBI PMU driver:
1) Overflow status / restart tracking is u64 but used BIT(), which is
an unsigned long shift and breaks for indices >= 32 on RV32
2) The available-counter mask was a single unsigned long while
iteration uses RISCV_MAX_COUNTERS (64), so RV32 can read past the
object; store it as a DECLARE_BITMAP
Patches are independent. Please review.
Thanks,
Xixin Liu
---
Xixin Liu (2):
perf: RISC-V: use BIT_ULL for u64 overflow masks
perf: RISC-V: store available counter mask as bitmap
drivers/perf/riscv_pmu_legacy.c | 5 +++--
drivers/perf/riscv_pmu_sbi.c | 35 ++++++++++++++++++++++------------
include/linux/perf/riscv_pmu.h | 2 +-
3 files changed, 27 insertions(+), 15 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v1 1/2] perf: RISC-V: use BIT_ULL for u64 overflow masks 2026-08-07 8:50 [PATCH v1 0/2] perf: RISC-V: fix SBI PMU masks for RV32 Xixin Liu @ 2026-08-07 8:50 ` Xixin Liu 2026-08-07 9:17 ` sashiko-bot 2026-08-08 0:46 ` Paul Walmsley 2026-08-07 8:50 ` [PATCH v1 2/2] perf: RISC-V: store available counter mask as bitmap Xixin Liu 1 sibling, 2 replies; 7+ messages in thread From: Xixin Liu @ 2026-08-07 8:50 UTC (permalink / raw) To: linux-riscv Cc: atish.patra, anup, will, mark.rutland, pjw, palmer, aou, alex, linux-arm-kernel, linux-perf-users, linux-kernel, liuxixin Overflow status and restart masks are u64, but bits were built with BIT(). On RV32 that is an unsigned long shift, so indices >= 32 truncate or wrap and corrupt the mask. Use BIT_ULL() for those u64 bitops. Signed-off-by: Xixin Liu <liuxixin@kylinos.cn> --- drivers/perf/riscv_pmu_sbi.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c index dfc886dee..93a31d5d5 100644 --- a/drivers/perf/riscv_pmu_sbi.c +++ b/drivers/perf/riscv_pmu_sbi.c @@ -1002,7 +1002,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); @@ -1109,14 +1109,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; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/2] perf: RISC-V: use BIT_ULL for u64 overflow masks 2026-08-07 8:50 ` [PATCH v1 1/2] perf: RISC-V: use BIT_ULL for u64 overflow masks Xixin Liu @ 2026-08-07 9:17 ` sashiko-bot 2026-08-08 0:46 ` Paul Walmsley 1 sibling, 0 replies; 7+ messages in thread From: sashiko-bot @ 2026-08-07 9:17 UTC (permalink / raw) To: Xixin Liu; +Cc: linux-perf-users Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: Pre-existing issues: - [High] Invalid SBI base index due to out-of-scope loop variable in pmu_sbi_start_ovf_ctrs_snapshot(). - [High] Incorrect mask arithmetic in pmu_sbi_start_ovf_ctrs_sbi() on RV32 mixes up counter indices. - [High] Buffer over-read and undefined shift behavior due to global cmask being an unsigned long. - [High] Early exit path in pmu_sbi_ovf_handler() triggers undefined shift and incorrect CSR access on Andes custom PMUs. - [High] Absolute 64-bit SBI PMU snapshot shared memory structures are incorrectly shifted or indexed relative to cbase on RV32. -- --- Patch [1]: [PATCH v1 1/2] perf: RISC-V: use BIT_ULL for u64 overflow masks --- commit d452f12b31f1955f29f737a7fc0bc5084ebd71af Author: Xixin Liu <liuxixin@kylinos.cn> Subject: perf: RISC-V: use BIT_ULL for u64 overflow masks This patch replaces BIT() with BIT_ULL() for u64 overflow status and restart masks in the RISC-V SBI PMU driver. This fixes truncation and wrapping issues on RV32 where BIT() resolves to an unsigned long shift. > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c > index 50220f7b46d9b..8ea5ae6173471 100644 > --- a/drivers/perf/riscv_pmu_sbi.c > +++ b/drivers/perf/riscv_pmu_sbi.c > @@ -1002,7 +1002,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); [Severity: High] This isn't a bug introduced by this patch, but since we're fixing issues in pmu_sbi_start_ovf_ctrs_snapshot(), there is an SBI base index problem slightly lower in this same function: pmu_sbi_start_ovf_ctrs_snapshot() { ... for (i = 0; i < BITS_TO_LONGS(RISCV_MAX_COUNTERS); i++) { for_each_set_bit(idx, &cpu_hw_evt->used_hw_ctrs[i], BITS_PER_LONG) ... sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_START, idx * BITS_PER_LONG, cpu_hw_evt->used_hw_ctrs[i], flag, 0, 0, 0); } } Does this pass an invalid base index to the SBI firmware? The for_each_set_bit() macro terminates with idx equal to BITS_PER_LONG. So idx * BITS_PER_LONG would evaluate to something like 4096 on RV64, which seems like it would cause an SBI_ERR_INVALID_PARAM error. Should this use i * BITS_PER_LONG instead? [Severity: High] This is another pre-existing issue in pmu_sbi_start_ovf_ctrs_snapshot() related to RV32 shared memory structures: pmu_sbi_start_ovf_ctrs_snapshot() { ... for (i = 0; i < BITS_TO_LONGS(RISCV_MAX_COUNTERS); i++) { for_each_set_bit(idx, &cpu_hw_evt->used_hw_ctrs[i], BITS_PER_LONG) sdata->ctr_values[idx] = cpu_hw_evt->snapshot_cval_shcopy[idx + i * BITS_PER_LONG]; ... } Since ctr_values is a 64-element array indexed by absolute counter IDs, does using the relative idx (0-31) incorrectly overwrite the states of counters 0-31 when processing chunk i=1 (counters 32-63)? [ ... ] > @@ -1109,14 +1109,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; [Severity: High] This is a pre-existing issue, but while looking at pmu_sbi_ovf_handler(), the early exit path near the beginning of the function caught my eye: pmu_sbi_ovf_handler() { ... fidx = find_first_bit(cpu_hw_evt->used_hw_ctrs, RISCV_MAX_COUNTERS); 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 can be assigned values like 274 (ANDES_SLI_CAUSE_BASE + ANDES_RV_IRQ_PMOVI). Does BIT(274) cause an undefined out-of-bounds shift? Furthermore, does clearing CSR_SIP work for Andes PMUs which use custom SLIE CSRs for these IRQs? Could this lead to an unacknowledged interrupt storm? [Severity: High] This is a pre-existing issue, but I noticed another mask arithmetic problem on RV32 that might cause broken PMU statistics. In pmu_sbi_start_ovf_ctrs_sbi(): pmu_sbi_start_ovf_ctrs_sbi() { ... for (i = 0; i < BITS_TO_LONGS(RISCV_MAX_COUNTERS); i++) { ... ctr_start_mask = cpu_hw_evt->used_hw_ctrs[i] & ~ctr_ovf_mask; ... } On RV32, when i = 1 (counters 32-63), used_hw_ctrs[1] is a 32-bit unsigned long, but ctr_ovf_mask is a 64-bit absolute mask. Since ~ctr_ovf_mask isn't shifted down by i * BITS_PER_LONG, doesn't this evaluate the usage bits for counters 32-63 against the overflow status of counters 0-31? [Severity: High] This is also a pre-existing issue, but related to 32-bit mask truncation, is the global cmask variable affected? drivers/perf/riscv_pmu_sbi.c: static unsigned long cmask; pmu_sbi_get_hpm_info() { ... for_each_set_bit(i, &cmask, RISCV_MAX_COUNTERS) { ... } On RV32, unsigned long is 32 bits, but RISCV_MAX_COUNTERS is 64. Does calling for_each_set_bit() here force a 64-bit read that accesses out-of-bounds memory adjacent to cmask? Should cmask be declared with DECLARE_BITMAP()? [Severity: High] Finally, this is a pre-existing issue, but similar to the ctr_values array, the overflow mask is also shifted out-of-bounds on RV32 in pmu_sbi_stop_hw_ctrs(): pmu_sbi_stop_hw_ctrs() { ... for (i = 0; i < BITS_TO_LONGS(RISCV_MAX_COUNTERS); i++) { ... temp_ctr_overflow_mask |= sdata->ctr_overflow_mask << (i * BITS_PER_LONG); ... } On RV32, shifting the 64-bit ctr_overflow_mask by 32 bits when i=1 pushes the absolute bits 32-63 out of bounds. Doesn't this discard the overflow state for counters 32-63? -- Sashiko AI review · https://sashiko.dev/#/patchset/cover.1786092600.git.liuxixin@kylinos.cn?part=1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/2] perf: RISC-V: use BIT_ULL for u64 overflow masks 2026-08-07 8:50 ` [PATCH v1 1/2] perf: RISC-V: use BIT_ULL for u64 overflow masks Xixin Liu 2026-08-07 9:17 ` sashiko-bot @ 2026-08-08 0:46 ` Paul Walmsley 1 sibling, 0 replies; 7+ messages in thread From: Paul Walmsley @ 2026-08-08 0:46 UTC (permalink / raw) To: Xixin Liu Cc: linux-riscv, atish.patra, anup, will, mark.rutland, pjw, palmer, aou, alex, linux-arm-kernel, linux-perf-users, linux-kernel Hi, On Fri, 7 Aug 2026, Xixin Liu wrote: > Overflow status and restart masks are u64, but bits were built with > BIT(). On RV32 that is an unsigned long shift, so indices >= 32 truncate > or wrap and corrupt the mask. > > Use BIT_ULL() for those u64 bitops. > > Signed-off-by: Xixin Liu <liuxixin@kylinos.cn> Thanks for the patch, but, same comments as on https://lore.kernel.org/linux-riscv/9109b689-1145-c714-ebe0-ec368f242a78@kernel.org/T/#m6fa97e46bde0d100fa0ec2854d47a57bd271c0b3 - Paul ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v1 2/2] perf: RISC-V: store available counter mask as bitmap 2026-08-07 8:50 [PATCH v1 0/2] perf: RISC-V: fix SBI PMU masks for RV32 Xixin Liu 2026-08-07 8:50 ` [PATCH v1 1/2] perf: RISC-V: use BIT_ULL for u64 overflow masks Xixin Liu @ 2026-08-07 8:50 ` Xixin Liu 2026-08-07 9:11 ` sashiko-bot 2026-08-08 0:32 ` Paul Walmsley 1 sibling, 2 replies; 7+ messages in thread From: Xixin Liu @ 2026-08-07 8:50 UTC (permalink / raw) To: linux-riscv Cc: atish.patra, anup, will, mark.rutland, pjw, palmer, aou, alex, linux-arm-kernel, linux-perf-users, linux-kernel, liuxixin The available-counter mask was a single unsigned long, but iteration uses RISCV_MAX_COUNTERS (64). On RV32 that reads past the object. Filling with BIT(i) is also wrong for i >= 32. Use DECLARE_BITMAP, set_bit/bitmap helpers, and stop counters one word at a time. Signed-off-by: Xixin Liu <liuxixin@kylinos.cn> --- drivers/perf/riscv_pmu_legacy.c | 5 +++-- drivers/perf/riscv_pmu_sbi.c | 29 ++++++++++++++++++----------- include/linux/perf/riscv_pmu.h | 2 +- 3 files changed, 22 insertions(+), 14 deletions(-) diff --git a/drivers/perf/riscv_pmu_legacy.c b/drivers/perf/riscv_pmu_legacy.c index 4d6461d6a..1b8e4789c 100644 --- a/drivers/perf/riscv_pmu_legacy.c +++ b/drivers/perf/riscv_pmu_legacy.c @@ -110,8 +110,9 @@ static void pmu_legacy_init(struct riscv_pmu *pmu) { pr_info("Legacy PMU implementation is available\n"); - pmu->cmask = BIT(RISCV_PMU_LEGACY_CYCLE) | - BIT(RISCV_PMU_LEGACY_INSTRET); + bitmap_zero(pmu->cmask, RISCV_MAX_COUNTERS); + set_bit(RISCV_PMU_LEGACY_CYCLE, pmu->cmask); + set_bit(RISCV_PMU_LEGACY_INSTRET, pmu->cmask); pmu->ctr_start = pmu_legacy_ctr_start; pmu->ctr_stop = NULL; pmu->event_map = pmu_legacy_event_map; diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c index d6c66e375..c913d73f8 100644 --- a/drivers/perf/riscv_pmu_sbi.c +++ b/drivers/perf/riscv_pmu_sbi.c @@ -97,7 +97,7 @@ static unsigned int riscv_pmu_irq_mask; static unsigned int riscv_pmu_irq; /* Cache the available counters in a bitmask */ -static unsigned long cmask; +static DECLARE_BITMAP(cmask, RISCV_MAX_COUNTERS); static int pmu_event_find_cache(u64 config); struct sbi_pmu_event_data { @@ -364,7 +364,7 @@ static void pmu_sbi_check_event(struct sbi_pmu_event_data *edata) struct sbiret ret; ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_CFG_MATCH, - 0, cmask, 0, edata->event_idx, 0, 0); + 0, cmask[0], 0, edata->event_idx, 0, 0); if (!ret.error) { sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_STOP, ret.value, 0x1, SBI_PMU_STOP_FLAG_RESET, 0, 0, 0); @@ -488,10 +488,10 @@ int riscv_pmu_get_hpm_info(u32 *hw_ctr_width, u32 *num_hw_ctr) union sbi_pmu_ctr_info *info; u32 hpm_width = 0, hpm_count = 0; - if (!cmask) + if (bitmap_empty(cmask, RISCV_MAX_COUNTERS)) return -EINVAL; - for_each_set_bit(i, &cmask, RISCV_MAX_COUNTERS) { + for_each_set_bit(i, cmask, RISCV_MAX_COUNTERS) { info = &pmu_ctr_list[i]; if (!info) continue; @@ -541,7 +541,7 @@ static int pmu_sbi_ctr_get_idx(struct perf_event *event) struct cpu_hw_events *cpuc = this_cpu_ptr(rvpmu->hw_events); struct sbiret ret; int idx; - uint64_t cbase = 0, cmask = rvpmu->cmask; + uint64_t cbase = 0, cmask = rvpmu->cmask[0]; unsigned long cflags = 0; cflags = pmu_sbi_get_filter_flags(event); @@ -577,7 +577,7 @@ static int pmu_sbi_ctr_get_idx(struct perf_event *event) } idx = ret.value; - if (!test_bit(idx, &rvpmu->cmask) || !pmu_ctr_list[idx].value) + if (!test_bit(idx, rvpmu->cmask) || !pmu_ctr_list[idx].value) return -ENOENT; /* Additional sanity check for the counter id */ @@ -881,7 +881,7 @@ static int pmu_sbi_get_ctrinfo(int nctr, unsigned long *mask) /* The logical counter ids are not expected to be contiguous */ continue; - *mask |= BIT(i); + set_bit(i, mask); cinfo.value = ret.value; if (cinfo.type == SBI_PMU_CTR_TYPE_FW) @@ -898,12 +898,19 @@ static int pmu_sbi_get_ctrinfo(int nctr, unsigned long *mask) static inline void pmu_sbi_stop_all(struct riscv_pmu *pmu) { + int i; + /* * No need to check the error because we are disabling all the counters * which may include counters that are not enabled yet. */ - sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_STOP, - 0, pmu->cmask, SBI_PMU_STOP_FLAG_RESET, 0, 0, 0); + for (i = 0; i < BITS_TO_LONGS(RISCV_MAX_COUNTERS); i++) { + if (!pmu->cmask[i]) + continue; + sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_STOP, + i * BITS_PER_LONG, pmu->cmask[i], + SBI_PMU_STOP_FLAG_RESET, 0, 0, 0); + } } static inline void pmu_sbi_stop_hw_ctrs(struct riscv_pmu *pmu) @@ -1442,7 +1449,7 @@ static int pmu_sbi_device_probe(struct platform_device *pdev) } /* cache all the information about counters now */ - if (pmu_sbi_get_ctrinfo(num_counters, &cmask)) + if (pmu_sbi_get_ctrinfo(num_counters, cmask)) goto out_free; ret = pmu_sbi_setup_irqs(pmu, pdev); @@ -1454,7 +1461,7 @@ static int pmu_sbi_device_probe(struct platform_device *pdev) pmu->pmu.attr_groups = riscv_pmu_attr_groups; pmu->pmu.parent = &pdev->dev; - pmu->cmask = cmask; + bitmap_copy(pmu->cmask, cmask, RISCV_MAX_COUNTERS); pmu->ctr_start = pmu_sbi_ctr_start; pmu->ctr_stop = pmu_sbi_ctr_stop; pmu->event_map = pmu_sbi_event_map; diff --git a/include/linux/perf/riscv_pmu.h b/include/linux/perf/riscv_pmu.h index f82a28040..ecaa40370 100644 --- a/include/linux/perf/riscv_pmu.h +++ b/include/linux/perf/riscv_pmu.h @@ -55,7 +55,7 @@ struct riscv_pmu { irqreturn_t (*handle_irq)(int irq_num, void *dev); - unsigned long cmask; + DECLARE_BITMAP(cmask, RISCV_MAX_COUNTERS); u64 (*ctr_read)(struct perf_event *event); int (*ctr_get_idx)(struct perf_event *event); int (*ctr_get_width)(int idx); ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v1 2/2] perf: RISC-V: store available counter mask as bitmap 2026-08-07 8:50 ` [PATCH v1 2/2] perf: RISC-V: store available counter mask as bitmap Xixin Liu @ 2026-08-07 9:11 ` sashiko-bot 2026-08-08 0:32 ` Paul Walmsley 1 sibling, 0 replies; 7+ messages in thread From: sashiko-bot @ 2026-08-07 9:11 UTC (permalink / raw) To: Xixin Liu; +Cc: linux-perf-users Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: New issues: - [High] The patch fails to fully support matching 64 counters on RV32, as it hardcodes `cmask[0]` and `cbase = 0` in SBI counter match queries, effectively ignoring counters 32-63. Pre-existing issues: - [High] An out-of-bounds counter base index is passed to `SBI_EXT_PMU_COUNTER_START` in the snapshot overflow handler. - [High] Global `cmask` is not cleared on probe failure, leading to a NULL pointer dereference in `riscv_pmu_get_hpm_info()`. - [High] Incorrect snapshot array indexing and overflow mask shifting for PMU counters >= 32 on RV32. -- --- Patch [2]: [PATCH v1 2/2] perf: RISC-V: store available counter mask as bitmap --- commit 0c40d37d24749711315dcc35e091383a23b8eeb6 Author: Xixin Liu <liuxixin@kylinos.cn> Subject: perf: RISC-V: store available counter mask as bitmap The available-counter mask was previously a single unsigned long, which caused out-of-bounds reads on RV32 when iterating up to RISCV_MAX_COUNTERS. This commit transitions the mask to use DECLARE_BITMAP and related helpers to prevent reads past the object and properly stop counters across words. > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c > --- a/drivers/perf/riscv_pmu_sbi.c > +++ b/drivers/perf/riscv_pmu_sbi.c > @@ -364,7 +364,7 @@ static void pmu_sbi_check_event(struct sbi_pmu_event_data *edata) > struct sbiret ret; > > ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_CFG_MATCH, > - 0, cmask, 0, edata->event_idx, 0, 0); > + 0, cmask[0], 0, edata->event_idx, 0, 0); [Severity: High] Does this code fail to match all 64 counters on RV32? By hardcoding cmask[0] and a base of 0, it appears to ignore counters 32-63 during the SBI counter match queries. Should this issue subsequent sbi_ecall queries for cmask[1] with cbase = 32 to ensure the upper half of the counters are checked? [ ... ] > @@ -541,7 +541,7 @@ static int pmu_sbi_ctr_get_idx(struct perf_event *event) > struct cpu_hw_events *cpuc = this_cpu_ptr(rvpmu->hw_events); > struct sbiret ret; > int idx; > - uint64_t cbase = 0, cmask = rvpmu->cmask; > + uint64_t cbase = 0, cmask = rvpmu->cmask[0]; [Severity: High] Does this code also fail to support matching counters >= 32 on RV32? Similar to pmu_sbi_check_event(), initializing cmask to rvpmu->cmask[0] and using it as the sole mask for the match call will result in -ENOENT for events that could be satisfied by the upper counters. [ ... ] > @@ -1451,7 +1458,7 @@ static int pmu_sbi_device_probe(struct platform_device *pdev) > } > > /* cache all the information about counters now */ > - if (pmu_sbi_get_ctrinfo(num_counters, &cmask)) > + if (pmu_sbi_get_ctrinfo(num_counters, cmask)) > goto out_free; [Severity: High] This is a pre-existing issue, but does the global cmask get left populated with stale data on probe failure? If pmu_sbi_device_probe() fails after pmu_sbi_get_ctrinfo() (for instance, during IRQ setup), the out_free error path frees and nullifies pmu_ctr_list: drivers/perf/riscv_pmu_sbi.c:pmu_sbi_device_probe() { ... out_free: free_percpu(pmu->hw_events); kfree(pmu_ctr_list); pmu_ctr_list = NULL; ... } However, the global cmask bitmap retains its bits. Later, if KVM calls riscv_pmu_get_hpm_info(), it will bypass the bitmap_empty() check, iterate over the stale bits, and dereference the NULL pmu_ctr_list array. [Severity: High] This is a pre-existing issue, but can the PMU counters fail to restart after an overflow when snapshots are enabled? In pmu_sbi_start_ovf_ctrs_snapshot(), the loop-exhausted idx variable is used to calculate the base counter index instead of the outer loop variable i: drivers/perf/riscv_pmu_sbi.c:pmu_sbi_start_ovf_ctrs_snapshot() { ... for_each_set_bit(idx, &cpu_hw_evt->used_hw_ctrs[i], BITS_PER_LONG) { ... } sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_START, idx * BITS_PER_LONG, ... } The for_each_set_bit() loop terminates with idx equal to BITS_PER_LONG. Passing idx * BITS_PER_LONG (e.g., 1024 on RV32) as the counter_idx_base to SBI_EXT_PMU_COUNTER_START will cause the firmware to reject the out-of-bounds base index. [Severity: High] This is a pre-existing issue, but does this code corrupt snapshot memory for counters >= 32 on RV32? When stopping hardware counters, the snapshot counter values are read using relative indices instead of absolute ones: drivers/perf/riscv_pmu_sbi.c:pmu_sbi_stop_hw_ctrs() { ... for_each_set_bit(idx, &cpu_hw_evt->used_hw_ctrs[i], BITS_PER_LONG) cpu_hw_evt->snapshot_cval_shcopy[i * BITS_PER_LONG + idx] = sdata->ctr_values[idx]; ... } Additionally, the absolute 64-bit overflow mask is incorrectly shifted: temp_ctr_overflow_mask |= sdata->ctr_overflow_mask << (i * BITS_PER_LONG); For i = 1 (counters 32-63), this shifts the valid overflow bits out of bounds. Furthermore, in pmu_sbi_start_ovf_ctrs_snapshot(), the snapshot counter values are written back using relative indices: sdata->ctr_values[idx] = ... This overwrites the first 32 counters' values instead of writing to the counters 32-63 region. -- Sashiko AI review · https://sashiko.dev/#/patchset/cover.1786092600.git.liuxixin@kylinos.cn?part=2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 2/2] perf: RISC-V: store available counter mask as bitmap 2026-08-07 8:50 ` [PATCH v1 2/2] perf: RISC-V: store available counter mask as bitmap Xixin Liu 2026-08-07 9:11 ` sashiko-bot @ 2026-08-08 0:32 ` Paul Walmsley 1 sibling, 0 replies; 7+ messages in thread From: Paul Walmsley @ 2026-08-08 0:32 UTC (permalink / raw) To: Xixin Liu Cc: linux-riscv, atish.patra, anup, will, mark.rutland, pjw, palmer, aou, alex, linux-arm-kernel, linux-perf-users, linux-kernel Hi, On Fri, 7 Aug 2026, Xixin Liu wrote: > The available-counter mask was a single unsigned long, but iteration > uses RISCV_MAX_COUNTERS (64). On RV32 that reads past the object. Filling > with BIT(i) is also wrong for i >= 32. > > Use DECLARE_BITMAP, set_bit/bitmap helpers, and stop counters one word > at a time. > > Signed-off-by: Xixin Liu <liuxixin@kylinos.cn> This patch contains several array indexes to element 0 of the cmask bitmap, but that element is going to be XLEN bits wide. Does that actually work on RV32 systems? - Paul ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2026-08-08 0:46 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-08-07 8:50 [PATCH v1 0/2] perf: RISC-V: fix SBI PMU masks for RV32 Xixin Liu 2026-08-07 8:50 ` [PATCH v1 1/2] perf: RISC-V: use BIT_ULL for u64 overflow masks Xixin Liu 2026-08-07 9:17 ` sashiko-bot 2026-08-08 0:46 ` Paul Walmsley 2026-08-07 8:50 ` [PATCH v1 2/2] perf: RISC-V: store available counter mask as bitmap Xixin Liu 2026-08-07 9:11 ` sashiko-bot 2026-08-08 0:32 ` Paul Walmsley
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox