Re: [PATCH v2 4/5] target/riscv: Add support for PC-relative translation

[liweiwei]

On 2023/3/30 00:27, Richard Henderson wrote:
> On 3/28/23 20:23, Weiwei Li wrote:
> >   static bool trans_auipc(DisasContext *ctx, arg_auipc *a)
> >   {
> > -    gen_set_gpri(ctx, a->rd, a->imm + ctx->base.pc_next);
> > +    assert(ctx->pc_save != -1);
> > +    if (tb_cflags(ctx->base.tb) & CF_PCREL) {
> > +        TCGv target_pc = tcg_temp_new();
>
> dest_gpr(s, a->rd)
OK. I'll fix this.
> > @@ -51,26 +59,43 @@ static bool trans_jal(DisasContext *ctx, arg_jal *a)
> >   static bool trans_jalr(DisasContext *ctx, arg_jalr *a)
> >   {
> >       TCGLabel *misaligned = NULL;
> > +    TCGv succ_pc = tcg_temp_new();
>
> succ_pc can by null for !CF_PCREL...
I think this is OK since it's only used for CF_PCREL.
> > +    TCGv target_pc = tcg_temp_new();
> > +
> > +    if (tb_cflags(ctx->base.tb) & CF_PCREL) {
> > +        tcg_gen_addi_tl(succ_pc, cpu_pc, ctx->pc_succ_insn - 
> > ctx->pc_save);
> > +    }
>
> ... or initialized like
>
>        } else {
>            succ_pc = tcg_constant_tl(ctx->pc_succ_insn);
>        }
>
> > -    gen_set_pc(ctx, cpu_pc);
> >       if (!has_ext(ctx, RVC)) {
> >           TCGv t0 = tcg_temp_new();
> >             misaligned = gen_new_label();
> > -        tcg_gen_andi_tl(t0, cpu_pc, 0x2);
> > +        tcg_gen_andi_tl(t0, target_pc, 0x2);
> >           tcg_gen_brcondi_tl(TCG_COND_NE, t0, 0x0, misaligned);
> >       }
> ...
> >       if (misaligned) {
> >           gen_set_label(misaligned);
> > -        gen_exception_inst_addr_mis(ctx);
> > +        gen_exception_inst_addr_mis(ctx, target_pc);
> >       }
>
> This is what I expected from patch 3: cpu_pc is unchanged, with the 
> new (incorrect) address passed to inst_addr_mis for assigning to 
> badaddr.  Bug being fixed here, thus should really be a separate patch.

It's OK to update cpu_pc before gen_exception_inst_addr_mis() since it 
will restore the current pc by gen_set_pc_imm() after update cpu_pc into 
badaddr.

However, after PC-relative translation is enabled, we cannot use 
gen_set_pc to directly update cpu_pc in above case, since gen_set_pc() 
will break the pc_save, and make gen_set_pc_imm() in 
gen_exception_inst_addr_mis() failed. So we introduce a temp target_pc  
instead of cpu_pc to compute the destination pc and use it to do 
misaligned check.

> > @@ -172,7 +197,7 @@ static bool gen_branch(DisasContext *ctx, arg_b 
> > *a, TCGCond cond)
> >       if (!has_ext(ctx, RVC) && ((ctx->base.pc_next + a->imm) & 0x3)) {
> >           /* misaligned */
> >           gen_set_pc_imm(ctx, ctx->base.pc_next + a->imm);
> > -        gen_exception_inst_addr_mis(ctx);
> > +        gen_exception_inst_addr_mis(ctx, cpu_pc);
>
> But this one's different and (probably) incorrect.
>
> > @@ -552,13 +567,21 @@ static void gen_jal(DisasContext *ctx, int rd, 
> > target_ulong imm)
> >       if (!has_ext(ctx, RVC)) {
> >           if ((next_pc & 0x3) != 0) {
> >               gen_set_pc_imm(ctx, next_pc);
> > -            gen_exception_inst_addr_mis(ctx);
> > +            gen_exception_inst_addr_mis(ctx, cpu_pc);
>
> Likewise.
>
> > +    assert(ctx->pc_save != -1);
> > +    if (tb_cflags(ctx->base.tb) & CF_PCREL) {
> > +        TCGv succ_pc = tcg_temp_new();
> > +        tcg_gen_addi_tl(succ_pc, cpu_pc, ctx->pc_succ_insn - 
> > ctx->pc_save);
> > +        gen_set_gpr(ctx, rd, succ_pc);
>
> dest_gpr.

OK. I'll fix this.

Regards,

Weiwei Li

> r~