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

[Nutty Liu]

On 7/14/2025 9:37 PM, Daniel Henrique Barboza 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>
> ---
>   target/riscv/op_helper.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)

Reviewed-by: Nutty Liu <liujin...@lanxincomputing.com>

Thanks,
Nutty
>
> 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);