Re: [PATCH] target/riscv: do not call GETPC() in check_ret_from_m_mode()

[Alistair Francis]

On Tue, Jul 15, 2025 at 1:05 AM Daniel Henrique Barboza
<dbarb...@ventanamicro.com> wrote:
>
> GETPC() should always be called from the top level helper, e.g. the
> first helper that is called by the translation code. We stopped doing
> that in commit 3157a553ec, and then we introduced problems when
> unwinding the exceptions being thrown by helper_mret(), as reported by
> [1].
>
> Call GETPC() at the top level helper and pass the value along.
>
> [1] https://gitlab.com/qemu-project/qemu/-/issues/3020
>
> Suggested-by: Richard Henderson <richard.hender...@linaro.org>
> Fixes: 3157a553ec ("target/riscv: Add Smrnmi mnret instruction")
> Closes: https://gitlab.com/qemu-project/qemu/-/issues/3020
> Signed-off-by: Daniel Henrique Barboza <dbarb...@ventanamicro.com>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  target/riscv/op_helper.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index 15460bf84b..110292e84d 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -355,21 +355,22 @@ target_ulong helper_sret(CPURISCVState *env)
>  }
>
>  static void check_ret_from_m_mode(CPURISCVState *env, target_ulong retpc,
> -                                  target_ulong prev_priv)
> +                                  target_ulong prev_priv,
> +                                  uintptr_t ra)
>  {
>      if (!(env->priv >= PRV_M)) {
> -        riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> +        riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, ra);
>      }
>
>      if (!riscv_cpu_allow_16bit_insn(&env_archcpu(env)->cfg,
>                                      env->priv_ver,
>                                      env->misa_ext) && (retpc & 0x3)) {
> -        riscv_raise_exception(env, RISCV_EXCP_INST_ADDR_MIS, GETPC());
> +        riscv_raise_exception(env, RISCV_EXCP_INST_ADDR_MIS, ra);
>      }
>
>      if (riscv_cpu_cfg(env)->pmp &&
>          !pmp_get_num_rules(env) && (prev_priv != PRV_M)) {
> -        riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT, GETPC());
> +        riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT, ra);
>      }
>  }
>  static target_ulong ssdbltrp_mxret(CPURISCVState *env, target_ulong mstatus,
> @@ -394,8 +395,9 @@ target_ulong helper_mret(CPURISCVState *env)
>      target_ulong retpc = env->mepc & get_xepc_mask(env);
>      uint64_t mstatus = env->mstatus;
>      target_ulong prev_priv = get_field(mstatus, MSTATUS_MPP);
> +    uintptr_t ra = GETPC();
>
> -    check_ret_from_m_mode(env, retpc, prev_priv);
> +    check_ret_from_m_mode(env, retpc, prev_priv, ra);
>
>      target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV) &&
>                               (prev_priv != PRV_M);
> @@ -443,8 +445,9 @@ target_ulong helper_mnret(CPURISCVState *env)
>      target_ulong retpc = env->mnepc;
>      target_ulong prev_priv = get_field(env->mnstatus, MNSTATUS_MNPP);
>      target_ulong prev_virt;
> +    uintptr_t ra = GETPC();
>
> -    check_ret_from_m_mode(env, retpc, prev_priv);
> +    check_ret_from_m_mode(env, retpc, prev_priv, ra);
>
>      prev_virt = get_field(env->mnstatus, MNSTATUS_MNPV) &&
>                  (prev_priv != PRV_M);
> --
> 2.50.1
>
>