* Re: [PATCH] target/riscv: do not call GETPC() in check_ret_from_m_mode()
2025-07-14 13:37 [PATCH] target/riscv: do not call GETPC() in check_ret_from_m_mode() Daniel Henrique Barboza
@ 2025-07-14 13:40 ` Richard Henderson
2025-07-14 13:42 ` Richard Henderson
2025-07-14 19:42 ` Philippe Mathieu-Daudé
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Richard Henderson @ 2025-07-14 13:40 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-devel
Cc: qemu-riscv, alistair.francis, liwei1518, zhiwei_liu, palmer
On 7/14/25 07:37, 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.henderson@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<dbarboza@ventanamicro.com>
> ---
> target/riscv/op_helper.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] target/riscv: do not call GETPC() in check_ret_from_m_mode()
2025-07-14 13:40 ` Richard Henderson
@ 2025-07-14 13:42 ` Richard Henderson
0 siblings, 0 replies; 6+ messages in thread
From: Richard Henderson @ 2025-07-14 13:42 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-devel
Cc: qemu-riscv, alistair.francis, liwei1518, zhiwei_liu, palmer,
qemu-stable
On 7/14/25 07:40, Richard Henderson wrote:
> On 7/14/25 07:37, 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.henderson@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<dbarboza@ventanamicro.com>
>> ---
>> target/riscv/op_helper.c | 15 +++++++++------
>> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Cc: qemu-stable@nongnu.org
r~
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] target/riscv: do not call GETPC() in check_ret_from_m_mode()
2025-07-14 13:37 [PATCH] target/riscv: do not call GETPC() in check_ret_from_m_mode() Daniel Henrique Barboza
2025-07-14 13:40 ` Richard Henderson
@ 2025-07-14 19:42 ` Philippe Mathieu-Daudé
2025-07-21 7:44 ` Nutty Liu
2025-07-29 3:20 ` Alistair Francis
3 siblings, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-07-14 19:42 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-devel
Cc: qemu-riscv, alistair.francis, liwei1518, zhiwei_liu, palmer,
Richard Henderson
On 14/7/25 15:37, 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.henderson@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 <dbarboza@ventanamicro.com>
> ---
> target/riscv/op_helper.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] target/riscv: do not call GETPC() in check_ret_from_m_mode()
2025-07-14 13:37 [PATCH] target/riscv: do not call GETPC() in check_ret_from_m_mode() Daniel Henrique Barboza
2025-07-14 13:40 ` Richard Henderson
2025-07-14 19:42 ` Philippe Mathieu-Daudé
@ 2025-07-21 7:44 ` Nutty Liu
2025-07-29 3:20 ` Alistair Francis
3 siblings, 0 replies; 6+ messages in thread
From: Nutty Liu @ 2025-07-21 7:44 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-devel
Cc: qemu-riscv, alistair.francis, liwei1518, zhiwei_liu, palmer,
Richard Henderson
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.henderson@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 <dbarboza@ventanamicro.com>
> ---
> target/riscv/op_helper.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
Reviewed-by: Nutty Liu <liujingqi@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);
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] target/riscv: do not call GETPC() in check_ret_from_m_mode()
2025-07-14 13:37 [PATCH] target/riscv: do not call GETPC() in check_ret_from_m_mode() Daniel Henrique Barboza
` (2 preceding siblings ...)
2025-07-21 7:44 ` Nutty Liu
@ 2025-07-29 3:20 ` Alistair Francis
3 siblings, 0 replies; 6+ messages in thread
From: Alistair Francis @ 2025-07-29 3:20 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: qemu-devel, qemu-riscv, alistair.francis, liwei1518, zhiwei_liu,
palmer, Richard Henderson
On Tue, Jul 15, 2025 at 1:05 AM Daniel Henrique Barboza
<dbarboza@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.henderson@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 <dbarboza@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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread