[PATCH] target/riscv: Update $pc after linking to $ra in trans_cm_jalt()

[PATCH] target/riscv: Update $pc after linking to $ra in trans_cm_jalt()

[Jason Chien]

The original implementation sets $pc to the address read from the jump
vector table first and links $ra with the address of the next instruction
after the updated $pc. After jumping to the updated $pc and executing the
next ret instruction, the program jumps to $ra, which is in the same
function currently executing, which results in an infinite loop.
This commit reverses the two action. Firstly, $ra is updated with the
address of the next instruction after $pc, and sets $pc to the address
read from the jump vector table.

Signed-off-by: Jason Chien <jason.chien@sifive.com>
---
 target/riscv/insn_trans/trans_rvzce.c.inc | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/target/riscv/insn_trans/trans_rvzce.c.inc b/target/riscv/insn_trans/trans_rvzce.c.inc
index 8d8a64f493..a185e2315f 100644
--- a/target/riscv/insn_trans/trans_rvzce.c.inc
+++ b/target/riscv/insn_trans/trans_rvzce.c.inc
@@ -293,13 +293,6 @@ static bool trans_cm_jalt(DisasContext *ctx, arg_cm_jalt *a)
 {
     REQUIRE_ZCMT(ctx);
 
-    /*
-     * Update pc to current for the non-unwinding exception
-     * that might come from cpu_ld*_code() in the helper.
-     */
-    gen_update_pc(ctx, 0);
-    gen_helper_cm_jalt(cpu_pc, cpu_env, tcg_constant_i32(a->index));
-
     /* c.jt vs c.jalt depends on the index. */
     if (a->index >= 32) {
         TCGv succ_pc = dest_gpr(ctx, xRA);
@@ -307,6 +300,13 @@ static bool trans_cm_jalt(DisasContext *ctx, arg_cm_jalt *a)
         gen_set_gpr(ctx, xRA, succ_pc);
     }
 
+    /*
+     * Update pc to current for the non-unwinding exception
+     * that might come from cpu_ld*_code() in the helper.
+     */
+    gen_update_pc(ctx, 0);
+    gen_helper_cm_jalt(cpu_pc, cpu_env, tcg_constant_i32(a->index));
+
     tcg_gen_lookup_and_goto_ptr();
     ctx->base.is_jmp = DISAS_NORETURN;
     return true;
-- 
2.43.0

Re: [PATCH] target/riscv: Update $pc after linking to $ra in trans_cm_jalt()

[Frank Chang]

[-- Attachment #1: Type: text/plain, Size: 2157 bytes --]

Reviewed-by: Frank Chang <frank.chang@sifive.com>

On Tue, Feb 6, 2024 at 9:19 PM Jason Chien <jason.chien@sifive.com> wrote:

> The original implementation sets $pc to the address read from the jump
> vector table first and links $ra with the address of the next instruction
> after the updated $pc. After jumping to the updated $pc and executing the
> next ret instruction, the program jumps to $ra, which is in the same
> function currently executing, which results in an infinite loop.
> This commit reverses the two action. Firstly, $ra is updated with the
> address of the next instruction after $pc, and sets $pc to the address
> read from the jump vector table.
>
> Signed-off-by: Jason Chien <jason.chien@sifive.com>
> ---
>  target/riscv/insn_trans/trans_rvzce.c.inc | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/insn_trans/trans_rvzce.c.inc
> b/target/riscv/insn_trans/trans_rvzce.c.inc
> index 8d8a64f493..a185e2315f 100644
> --- a/target/riscv/insn_trans/trans_rvzce.c.inc
> +++ b/target/riscv/insn_trans/trans_rvzce.c.inc
> @@ -293,13 +293,6 @@ static bool trans_cm_jalt(DisasContext *ctx,
> arg_cm_jalt *a)
>  {
>      REQUIRE_ZCMT(ctx);
>
> -    /*
> -     * Update pc to current for the non-unwinding exception
> -     * that might come from cpu_ld*_code() in the helper.
> -     */
> -    gen_update_pc(ctx, 0);
> -    gen_helper_cm_jalt(cpu_pc, cpu_env, tcg_constant_i32(a->index));
> -
>      /* c.jt vs c.jalt depends on the index. */
>      if (a->index >= 32) {
>          TCGv succ_pc = dest_gpr(ctx, xRA);
> @@ -307,6 +300,13 @@ static bool trans_cm_jalt(DisasContext *ctx,
> arg_cm_jalt *a)
>          gen_set_gpr(ctx, xRA, succ_pc);
>      }
>
> +    /*
> +     * Update pc to current for the non-unwinding exception
> +     * that might come from cpu_ld*_code() in the helper.
> +     */
> +    gen_update_pc(ctx, 0);
> +    gen_helper_cm_jalt(cpu_pc, cpu_env, tcg_constant_i32(a->index));
> +
>      tcg_gen_lookup_and_goto_ptr();
>      ctx->base.is_jmp = DISAS_NORETURN;
>      return true;
> --
> 2.43.0
>
>
>

[-- Attachment #2: Type: text/html, Size: 2725 bytes --]

Re: [PATCH] target/riscv: Update $pc after linking to $ra in trans_cm_jalt()

[Richard Henderson]

On 2/6/24 23:18, Jason Chien wrote:
> The original implementation sets $pc to the address read from the jump
> vector table first and links $ra with the address of the next instruction
> after the updated $pc. After jumping to the updated $pc and executing the
> next ret instruction, the program jumps to $ra, which is in the same
> function currently executing, which results in an infinite loop.
> This commit reverses the two action. Firstly, $ra is updated with the
> address of the next instruction after $pc, and sets $pc to the address
> read from the jump vector table.

This is unlikely to be correct in the case the vector table read faults,
leaving $ra updated.

I guess this got broken with CF_PCREL.  Anyway, the solution is to use a temporary...

> -    /*
> -     * Update pc to current for the non-unwinding exception
> -     * that might come from cpu_ld*_code() in the helper.
> -     */
> -    gen_update_pc(ctx, 0);
> -    gen_helper_cm_jalt(cpu_pc, cpu_env, tcg_constant_i32(a->index));

... here and then ...

> @@ -307,6 +300,13 @@ static bool trans_cm_jalt(DisasContext *ctx, arg_cm_jalt *a)
>           gen_set_gpr(ctx, xRA, succ_pc);
>       }
>   

... copy the temp to cpu_pc here.

>       tcg_gen_lookup_and_goto_ptr();
>       ctx->base.is_jmp = DISAS_NORETURN;
>       return true;



r~

Re: [PATCH] target/riscv: Update $pc after linking to $ra in trans_cm_jalt()

[Jason Chien]

[-- Attachment #1: Type: text/plain, Size: 1601 bytes --]

You are right. I'll send patch v2 shortly. Thank you for the reply.

Richard Henderson <richard.henderson@linaro.org> 於 2024年2月7日 週三 上午4:24寫道：

> On 2/6/24 23:18, Jason Chien wrote:
> > The original implementation sets $pc to the address read from the jump
> > vector table first and links $ra with the address of the next instruction
> > after the updated $pc. After jumping to the updated $pc and executing the
> > next ret instruction, the program jumps to $ra, which is in the same
> > function currently executing, which results in an infinite loop.
> > This commit reverses the two action. Firstly, $ra is updated with the
> > address of the next instruction after $pc, and sets $pc to the address
> > read from the jump vector table.
>
> This is unlikely to be correct in the case the vector table read faults,
> leaving $ra updated.
>
> I guess this got broken with CF_PCREL.  Anyway, the solution is to use a
> temporary...
>
> > -    /*
> > -     * Update pc to current for the non-unwinding exception
> > -     * that might come from cpu_ld*_code() in the helper.
> > -     */
> > -    gen_update_pc(ctx, 0);
> > -    gen_helper_cm_jalt(cpu_pc, cpu_env, tcg_constant_i32(a->index));
>
> ... here and then ...
>
> > @@ -307,6 +300,13 @@ static bool trans_cm_jalt(DisasContext *ctx,
> arg_cm_jalt *a)
> >           gen_set_gpr(ctx, xRA, succ_pc);
> >       }
> >
>
> ... copy the temp to cpu_pc here.
>
> >       tcg_gen_lookup_and_goto_ptr();
> >       ctx->base.is_jmp = DISAS_NORETURN;
> >       return true;
>
>
>
> r~
>

[-- Attachment #2: Type: text/html, Size: 2081 bytes --]