Re: [PATCH] target/riscv: fix RV32 fixed counter accesses

[Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>]

On 9/5/2026 6:28 AM, Zephyr Li wrote:
> Since commit cfc96df65e01, riscv_pmu_ctr_get_fixed_counters_val()
> returns the complete 64-bit fixed-counter value. The RV32 counter
> access paths, however, still perform parts of the offset calculation
> on separately extracted 32-bit halves.
> 
> In particular, riscv_pmu_write_ctrh() deposits the low 32 bits of the
> complete fixed-counter value into the high half of mhpmcounter_prev.
> riscv_pmu_read_ctr() also subtracts a 32-bit half of the previous value
> from the complete 64-bit fixed-counter value. Consequently, writes to
> mcycleh can be lost and carries between the low and high halves are not
> handled correctly.
> 
> Keep the fixed-counter offset calculation entirely in 64 bits. Before
> a running counter is partially written, materialize its current
> architectural value and reset the fixed-counter baseline. On reads,
> calculate the complete 64-bit counter value before extracting the half
> requested by RV32.
> 
> Add an RV32 system TCG test for high-half writes, low-to-high carry, and
> preserving the carried high half across a subsequent low-half write.
> 
> Fixes: cfc96df65e01 ("target/riscv: Remove upper_half from riscv_pmu_ctr_get_fixed_counters_val")
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4219
> Signed-off-by: Zephyr Li <fritchleybohrer@gmail.com>
> ---

Reviewed-by: Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>

>   target/riscv/tcg/csr.c                    | 32 ++++++++--------
>   tests/tcg/riscv64/Makefile.softmmu-target | 12 ++++++
>   tests/tcg/riscv64/test-mcycle-rv32.S      | 45 +++++++++++++++++++++++
>   3 files changed, 74 insertions(+), 15 deletions(-)
>   create mode 100644 tests/tcg/riscv64/test-mcycle-rv32.S
> 
> diff --git a/target/riscv/tcg/csr.c b/target/riscv/tcg/csr.c
> index 002f7e69c1..ffdd5c4aa6 100644
> --- a/target/riscv/tcg/csr.c
> +++ b/target/riscv/tcg/csr.c
> @@ -1337,23 +1337,23 @@ static RISCVException riscv_pmu_write_ctr(CPURISCVState *env, target_ulong val,
>       int deposit_size = rv32 ? 32 : 64;
>       uint64_t ctr;
>   
> -    counter->mhpmcounter_val = deposit64(counter->mhpmcounter_val,
> -                                         0, deposit_size, val);
> -
>       if (!get_field(env->mcountinhibit, BIT(ctr_idx)) &&
>           (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) ||
>            riscv_pmu_ctr_monitor_instructions(env, ctr_idx))) {
>           ctr = riscv_pmu_ctr_get_fixed_counters_val(env, ctr_idx);
> -        counter->mhpmcounter_prev = deposit64(counter->mhpmcounter_prev,
> -                                              0, deposit_size, ctr);
> +        counter->mhpmcounter_val += ctr - counter->mhpmcounter_prev;
> +        counter->mhpmcounter_val = deposit64(counter->mhpmcounter_val,
> +                                             0, deposit_size, val);
> +        counter->mhpmcounter_prev = ctr;
>           if (ctr_idx > 2) {
>               riscv_pmu_setup_timer(env, counter->mhpmcounter_val, ctr_idx);
>           }
>        } else {
> +        counter->mhpmcounter_val = deposit64(counter->mhpmcounter_val,
> +                                             0, deposit_size, val);
>           /* Other counters can keep incrementing from the given value */
>           counter->mhpmcounter_prev = deposit64(counter->mhpmcounter_prev,
>                                                 0, deposit_size, val);
> -
>       }
>   
>       return RISCV_EXCP_NONE;
> @@ -1363,20 +1363,22 @@ static RISCVException riscv_pmu_write_ctrh(CPURISCVState *env, target_ulong val,
>                                             uint32_t ctr_idx)
>   {
>       PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
> -    uint64_t ctrh;
> +    uint64_t ctr;
>   
> -    counter->mhpmcounter_val = deposit64(counter->mhpmcounter_val,
> -                                         32, 32, val);
>       if (!get_field(env->mcountinhibit, BIT(ctr_idx)) &&
>           (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) ||
>            riscv_pmu_ctr_monitor_instructions(env, ctr_idx))) {
> -        ctrh = riscv_pmu_ctr_get_fixed_counters_val(env, ctr_idx);
> -        counter->mhpmcounter_prev = deposit64(counter->mhpmcounter_prev,
> -                                              32, 32, ctrh);
> +        ctr = riscv_pmu_ctr_get_fixed_counters_val(env, ctr_idx);
> +        counter->mhpmcounter_val += ctr - counter->mhpmcounter_prev;
> +        counter->mhpmcounter_val = deposit64(counter->mhpmcounter_val,
> +                                             32, 32, val);
> +        counter->mhpmcounter_prev = ctr;
>           if (ctr_idx > 2) {
>               riscv_pmu_setup_timer(env, counter->mhpmcounter_val, ctr_idx);
>           }
>       } else {
> +        counter->mhpmcounter_val = deposit64(counter->mhpmcounter_val,
> +                                             32, 32, val);
>           counter->mhpmcounter_prev = deposit64(counter->mhpmcounter_prev,
>                                                 32, 32, val);
>       }
> @@ -1407,12 +1409,11 @@ RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
>       bool rv32 = riscv_cpu_mxl(env) == MXL_RV32;
>       int start = upper_half ? 32 : 0;
>       int length = rv32 ? 32 : 64;
> -    uint64_t ctr_prev, ctr_val;
> +    uint64_t ctr_val;
>   
>       /* Ensure upper_half is only set for XLEN == 32 */
>       g_assert(rv32 || !upper_half);
>   
> -    ctr_prev = extract64(counter->mhpmcounter_prev, start, length);
>       ctr_val  = extract64(counter->mhpmcounter_val, start, length);
>   
>       if (get_field(env->mcountinhibit, BIT(ctr_idx))) {
> @@ -1431,7 +1432,8 @@ RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
>       if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) ||
>           riscv_pmu_ctr_monitor_instructions(env, ctr_idx)) {
>           uint64_t cntr = riscv_pmu_ctr_get_fixed_counters_val(env, ctr_idx) -
> -                                                             ctr_prev + ctr_val;
> +                        counter->mhpmcounter_prev +
> +                        counter->mhpmcounter_val;
>           *val = extract64(cntr, start, length);
>       } else {
>           *val = ctr_val;
> diff --git a/tests/tcg/riscv64/Makefile.softmmu-target b/tests/tcg/riscv64/Makefile.softmmu-target
> index 6a219c306c..15c7371acd 100644
> --- a/tests/tcg/riscv64/Makefile.softmmu-target
> +++ b/tests/tcg/riscv64/Makefile.softmmu-target
> @@ -28,6 +28,18 @@ EXTRA_RUNS += run-test-minstret-ecall
>   run-test-minstret-ecall: test-minstret-ecall
>   	$(call run-test, $<, $(QEMU) -icount shift=1 $(QEMU_OPTS)$<)
>   
> +RV32_CFLAGS = -march=rv32im_zicsr -mabi=ilp32
> +CLEANFILES += test-mcycle-rv32
> +
> +test-mcycle-rv32: test-mcycle-rv32.S $(LINK_SCRIPT)
> +	$(CC) $(CFLAGS) $(RV32_CFLAGS) $< -Wa,--noexecstack -c -o $@.o
> +	$(LD) -m elf32lriscv $(LDFLAGS) $@.o -o $@
> +
> +EXTRA_RUNS += run-test-mcycle-rv32
> +run-test-mcycle-rv32: test-mcycle-rv32
> +	$(call run-test, $<, \
> +	  $(QEMU) -cpu rv32 -icount shift=1 $(QEMU_OPTS)$<)
> +
>   EXTRA_RUNS += run-plugin-doubletrap
>   run-plugin-doubletrap: doubletrap
>   	$(call run-test, $<, \
> diff --git a/tests/tcg/riscv64/test-mcycle-rv32.S b/tests/tcg/riscv64/test-mcycle-rv32.S
> new file mode 100644
> index 0000000000..189d1e13a2
> --- /dev/null
> +++ b/tests/tcg/riscv64/test-mcycle-rv32.S
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +	.option	norvc
> +
> +	.text
> +	.global	_start
> +_start:
> +	/* Exercise writes while mcycle is running. */
> +	csrw	mcountinhibit, zero
> +	csrw	mcycle, zero
> +
> +	/* A write to the high half must be immediately observable. */
> +	li	s0, 0x1234ffff
> +	csrw	mcycleh, s0
> +	csrr	t0, mcycleh
> +	bne	t0, s0, fail
> +
> +	/* Check carry from the low half into the high half. */
> +	li	s0, 0x12345678
> +	csrw	mcycleh, s0
> +	li	t0, 0xfffffff0
> +	csrw	mcycle, t0
> +	.rept	32
> +	nop
> +	.endr
> +	csrr	t0, mcycleh
> +	addi	s0, s0, 1
> +	bne	t0, s0, fail
> +
> +	/* A low-half write must preserve the carried high half. */
> +	li	t0, 0x22222222
> +	csrw	mcycle, t0
> +	csrr	t0, mcycleh
> +	bne	t0, s0, fail
> +
> +	li	t0, 0x100000
> +	li	t1, 0x5555	/* FINISHER_PASS */
> +	sw	t1, 0(t0)
> +	j	.
> +
> +fail:
> +	li	t0, 0x100000
> +	li	t1, 0x13333	/* status = FINISHER_FAIL, code = 1 */
> +	sw	t1, 0(t0)
> +	j	.