| Message ID | 20240206131911.10998-1-jason.chien@sifive.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/riscv: Update $pc after linking to $ra in trans_cm_jalt() | expand |
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 > > >
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~
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~ >
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;
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(-)