1. Patchew
  2. QEMU
  3. View series

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

Daniel Henrique Barboza posted 1 patch 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20250714133739.1248296-1-dbarboza@ventanamicro.com
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Weiwei Li <liwei1518@gmail.com>, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>
target/riscv/op_helper.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
[PATCH] target/riscv: do not call GETPC() in check_ret_from_m_mode()
Posted by Daniel Henrique Barboza 4 months ago 
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(-)

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
Re: [PATCH] target/riscv: do not call GETPC() in check_ret_from_m_mode()
Posted by Alistair Francis 3 months, 2 weeks ago 
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
>
>
Re: [PATCH] target/riscv: do not call GETPC() in check_ret_from_m_mode()
Posted by Nutty Liu 3 months, 3 weeks ago 
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);
Re: [PATCH] target/riscv: do not call GETPC() in check_ret_from_m_mode()
Posted by Philippe Mathieu-Daudé 4 months ago 
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>
Re: [PATCH] target/riscv: do not call GETPC() in check_ret_from_m_mode()
Posted by Richard Henderson 4 months ago 
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~
Re: [PATCH] target/riscv: do not call GETPC() in check_ret_from_m_mode()
Posted by Richard Henderson 4 months ago 
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~

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.