* [PATCH] target/riscv: fix RV32 fixed counter accesses
@ 2026-09-05 9:28 Zephyr Li
2026-09-08 11:17 ` Daniel Henrique Barboza
0 siblings, 1 reply; 4+ messages in thread
From: Zephyr Li @ 2026-09-05 9:28 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-riscv, Palmer Dabbelt, Alistair Francis, Weiwei Li,
Daniel Henrique Barboza, Liu Zhiwei, Chao Liu, Zephyr Li
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>
---
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 .
--
2.43.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] target/riscv: fix RV32 fixed counter accesses
2026-09-05 9:28 [PATCH] target/riscv: fix RV32 fixed counter accesses Zephyr Li
@ 2026-09-08 11:17 ` Daniel Henrique Barboza
2026-09-08 16:00 ` Daniel Henrique Barboza
0 siblings, 1 reply; 4+ messages in thread
From: Daniel Henrique Barboza @ 2026-09-08 11:17 UTC (permalink / raw)
To: Zephyr Li, qemu-devel
Cc: qemu-riscv, Palmer Dabbelt, Alistair Francis, Weiwei Li,
Liu Zhiwei, Chao Liu
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 .
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] target/riscv: fix RV32 fixed counter accesses
2026-09-08 11:17 ` Daniel Henrique Barboza
@ 2026-09-08 16:00 ` Daniel Henrique Barboza
2026-09-09 1:40 ` Zephyr Li
0 siblings, 1 reply; 4+ messages in thread
From: Daniel Henrique Barboza @ 2026-09-08 16:00 UTC (permalink / raw)
To: Zephyr Li, qemu-devel
Cc: qemu-riscv, Palmer Dabbelt, Alistair Francis, Weiwei Li,
Liu Zhiwei, Chao Liu
On 9/8/2026 8:17 AM, Daniel Henrique Barboza wrote:
>
>
> 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>
Actually there's a test related change I would like to suggest:
>
>> 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
Note that you're adding specific rv32 tests in tests/tcg/riscv64/Makefile.softmmu-target.
A better place is the tests/tcg/riscv32/ dir - we want to avoid mixing rv32 stuff in
rv64 makefiles and vice-versa.
I suggest keeping tests/tcg/riscv64/Makefile.softmmu-target untouched and
use tests/tcg/riscv32/ for this new test.
Thanks,
Daniel
>> +
>> +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 .
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] target/riscv: fix RV32 fixed counter accesses
2026-09-08 16:00 ` Daniel Henrique Barboza
@ 2026-09-09 1:40 ` Zephyr Li
0 siblings, 0 replies; 4+ messages in thread
From: Zephyr Li @ 2026-09-09 1:40 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: qemu-devel, qemu-riscv, Palmer Dabbelt, Alistair Francis,
Weiwei Li, Liu Zhiwei, Chao Liu
[-- Attachment #1: Type: text/plain, Size: 10069 bytes --]
Thanks for the suggestion. I'll move the test to tests/tcg/riscv32/ in v2.
Thanks,
Zephyr
On Wed, Sep 9, 2026 at 12:00 AM Daniel Henrique Barboza <
daniel.barboza@oss.qualcomm.com> wrote:
>
>
> On 9/8/2026 8:17 AM, Daniel Henrique Barboza wrote:
> >
> >
> > 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>
>
> Actually there's a test related change I would like to suggest:
>
>
>
> >
> >> 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
>
> Note that you're adding specific rv32 tests in
> tests/tcg/riscv64/Makefile.softmmu-target.
> A better place is the tests/tcg/riscv32/ dir - we want to avoid mixing
> rv32 stuff in
> rv64 makefiles and vice-versa.
>
> I suggest keeping tests/tcg/riscv64/Makefile.softmmu-target untouched and
> use tests/tcg/riscv32/ for this new test.
>
>
> Thanks,
> Daniel
>
> >> +
> >> +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 .
> >
>
>
[-- Attachment #2: Type: text/html, Size: 13672 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-09-09 1:41 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-05 9:28 [PATCH] target/riscv: fix RV32 fixed counter accesses Zephyr Li
2026-09-08 11:17 ` Daniel Henrique Barboza
2026-09-08 16:00 ` Daniel Henrique Barboza
2026-09-09 1:40 ` Zephyr Li
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.