Re: [PATCH] target/riscv: do not count ECALL in minstret

On 7/29/2026 12:22 AM, Zephyr Li wrote:
> With icount enabled, helper_raise_exception() leaves ECALL in
> icount_get_raw() because it exits without restoring the current TB
> state. This makes minstret count an instruction that does not retire.
> 
> Adjust only the fixed minstret baseline so that mcycle accounting
> remains unchanged.
> 
> Add an RV64 softmmu regression test for the issue.

That's a good habit that I need to incorporate as well.

> 

Fixes: 4fe8ae0906 ("target/riscv: Combine mhpmcounter and mhpmcounterh")

> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/4087
> Signed-off-by: Zephyr Li <[email protected]>
> ---

Reviewed-by: Daniel Henrique Barboza <[email protected]>

>   target/riscv/tcg/op_helper.c              |  6 +++
>   target/riscv/tcg/pmu.c                    | 44 +++++++++++++-----
>   target/riscv/tcg/pmu.h                    |  1 +
>   tests/tcg/riscv64/Makefile.softmmu-target |  4 ++
>   tests/tcg/riscv64/test-minstret-ecall.S   | 55 +++++++++++++++++++++++
>   5 files changed, 98 insertions(+), 12 deletions(-)
>   create mode 100644 tests/tcg/riscv64/test-minstret-ecall.S
> 
> diff --git a/target/riscv/tcg/op_helper.c b/target/riscv/tcg/op_helper.c
> index ba3c7da375..acdccd1a97 100644
> --- a/target/riscv/tcg/op_helper.c
> +++ b/target/riscv/tcg/op_helper.c
> @@ -21,6 +21,9 @@
>   #include "qemu/osdep.h"
>   #include "cpu.h"
>   #include "target/riscv/tcg/csr.h"
> +#ifndef CONFIG_USER_ONLY
> +#include "pmu.h"
> +#endif
>   #include "internals.h"
>   #include "exec/cputlb.h"
>   #include "accel/tcg/cpu-ldst.h"
> @@ -47,6 +50,9 @@ G_NORETURN void riscv_raise_exception(CPURISCVState *env,
>   
>   void helper_raise_exception(CPURISCVState *env, uint32_t exception)
>   {
> +#ifndef CONFIG_USER_ONLY
> +    riscv_pmu_decr_instret(env);
> +#endif
>       riscv_raise_exception(env, exception, 0);
>   }
>   
> diff --git a/target/riscv/tcg/pmu.c b/target/riscv/tcg/pmu.c
> index 38ad2737e1..1a4658319b 100644
> --- a/target/riscv/tcg/pmu.c
> +++ b/target/riscv/tcg/pmu.c
> @@ -49,6 +49,21 @@ static bool riscv_pmu_counter_enabled(RISCVCPU *cpu, uint32_t ctr_idx)
>       }
>   }
>   
> +static bool riscv_pmu_counter_filtered(CPURISCVState *env, uint64_t cfg)
> +{
> +    bool virt_on = env->virt_enabled;
> +
> +    return (env->priv == PRV_M && (cfg & MHPMEVENT_BIT_MINH)) ||
> +           (env->priv == PRV_S && virt_on &&
> +            (cfg & MHPMEVENT_BIT_VSINH)) ||
> +           (env->priv == PRV_U && virt_on &&
> +            (cfg & MHPMEVENT_BIT_VUINH)) ||
> +           (env->priv == PRV_S && !virt_on &&
> +            (cfg & MHPMEVENT_BIT_SINH)) ||
> +           (env->priv == PRV_U && !virt_on &&
> +            (cfg & MHPMEVENT_BIT_UINH));
> +}
> +
>   /*
>    * Information needed to update counters:
>    *  new_priv, new_virt: To correctly save starting snapshot for the newly
> @@ -147,12 +162,27 @@ void riscv_pmu_update_fixed_ctrs(CPURISCVState *env,
>       riscv_pmu_icount_update_priv(env, newpriv, new_virt);
>   }
>   
> +void riscv_pmu_decr_instret(CPURISCVState *env)
> +{
> +    if (!icount_enabled() ||
> +        (env->mcountinhibit & COUNTEREN_IR) ||
> +        riscv_pmu_counter_filtered(env, env->minstretcfg)) {
> +        return;
> +    }
> +
> +    /*
> +     * minstret is derived from icount, which includes the current
> +     * instruction.  Move the baseline forward to exclude an instruction
> +     * that raises an exception and therefore does not retire.
> +     */
> +    env->pmu_ctrs[2].mhpmcounter_prev++;
> +}
> +
>   int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum riscv_pmu_event_idx event_idx)
>   {
>       uint32_t ctr_idx;
>       CPURISCVState *env = &cpu->env;
>       uint64_t max_val = UINT64_MAX;
> -    bool virt_on = env->virt_enabled;
>       PMUCTRState *counter;
>       gpointer value;
>   
> @@ -170,17 +200,7 @@ int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum riscv_pmu_event_idx event_idx)
>           return -1;
>       }
>   
> -    /* Privilege mode filtering */
> -    if ((env->priv == PRV_M &&
> -        (env->mhpmevent_val[ctr_idx] & MHPMEVENT_BIT_MINH)) ||
> -        (env->priv == PRV_S && virt_on &&
> -        (env->mhpmevent_val[ctr_idx] & MHPMEVENT_BIT_VSINH)) ||
> -        (env->priv == PRV_U && virt_on &&
> -        (env->mhpmevent_val[ctr_idx] & MHPMEVENT_BIT_VUINH)) ||
> -        (env->priv == PRV_S && !virt_on &&
> -        (env->mhpmevent_val[ctr_idx] & MHPMEVENT_BIT_SINH)) ||
> -        (env->priv == PRV_U && !virt_on &&
> -        (env->mhpmevent_val[ctr_idx] & MHPMEVENT_BIT_UINH))) {
> +    if (riscv_pmu_counter_filtered(env, env->mhpmevent_val[ctr_idx])) {
>           return 0;
>       }
>   
> diff --git a/target/riscv/tcg/pmu.h b/target/riscv/tcg/pmu.h
> index b4f1e469a2..2429c01b77 100644
> --- a/target/riscv/tcg/pmu.h
> +++ b/target/riscv/tcg/pmu.h
> @@ -36,6 +36,7 @@ int riscv_pmu_setup_timer(CPURISCVState *env, uint64_t value,
>                             uint32_t ctr_idx);
>   void riscv_pmu_update_fixed_ctrs(CPURISCVState *env, privilege_mode_t newpriv,
>                                    bool new_virt);
> +void riscv_pmu_decr_instret(CPURISCVState *env);
>   RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
>                                     bool upper_half, uint32_t ctr_idx);
>   
> diff --git a/tests/tcg/riscv64/Makefile.softmmu-target b/tests/tcg/riscv64/Makefile.softmmu-target
> index 82be8a2c91..42038ce3b4 100644
> --- a/tests/tcg/riscv64/Makefile.softmmu-target
> +++ b/tests/tcg/riscv64/Makefile.softmmu-target
> @@ -24,6 +24,10 @@ EXTRA_RUNS += run-test-mepc-masking
>   run-test-mepc-masking: test-mepc-masking
>   	$(call run-test, $<, $(QEMU) $(QEMU_OPTS)$<)
>   
> +EXTRA_RUNS += run-test-minstret-ecall
> +run-test-minstret-ecall: test-minstret-ecall
> +	$(call run-test, $<, $(QEMU) -icount shift=1 $(QEMU_OPTS)$<)
> +
>   EXTRA_RUNS += run-plugin-doubletrap
>   run-plugin-doubletrap: doubletrap
>   	$(call run-test, $<, \
> diff --git a/tests/tcg/riscv64/test-minstret-ecall.S b/tests/tcg/riscv64/test-minstret-ecall.S
> new file mode 100644
> index 0000000000..ab268f7f22
> --- /dev/null
> +++ b/tests/tcg/riscv64/test-minstret-ecall.S
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +	.option	norvc
> +
> +	.text
> +	.global	_start
> +_start:
> +	lla	t0, trap
> +	csrw	mtvec, t0
> +
> +	/*
> +	 * The first CSR read retires after obtaining s0.  The ecall does not
> +	 * retire, so the trap handler must observe a difference of one.
> +	 */
> +	csrr	s0, minstret
> +	ecall
> +	sub	t0, s1, s0
> +	li	t1, 1
> +	bne	t0, t1, fail
> +
> +	li	a0, 0
> +	j	_exit
> +
> +trap:
> +	csrr	s1, minstret
> +	csrr	t0, mcause
> +	li	t1, 11		/* Environment call from M-mode */
> +	bne	t0, t1, fail
> +
> +	csrr	t0, mepc
> +	addi	t0, t0, 4
> +	csrw	mepc, t0
> +	mret
> +
> +fail:
> +	li	a0, 1
> +
> +_exit:
> +	lla	a1, semiargs
> +	li	t0, 0x20026	/* ADP_Stopped_ApplicationExit */
> +	sd	t0, 0(a1)
> +	sd	a0, 8(a1)
> +	li	a0, 0x20	/* TARGET_SYS_EXIT_EXTENDED */
> +
> +	/* Semihosting call sequence */
> +	.balign	16
> +	slli	zero, zero, 0x1f
> +	ebreak
> +	srai	zero, zero, 0x7
> +	j	.
> +
> +	.data
> +	.balign	16
> +semiargs:
> +	.space	16