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

On Thu, 2026-07-30 at 11:21 +0800, 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.
> 
> Fixes: 4fe8ae09062d ("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]>

Reviewed-by: Alistair Francis <[email protected]>

Alistair

> ---
> Changes in v2:
> - Add the Fixes tag suggested by Daniel.
> 
> ---
>  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