| Message ID | 20221006034421.1179141-22-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/s390x: pc-relative translation blocks | expand |
On Wed, Oct 05, 2022 at 08:44:16PM -0700, Richard Henderson wrote: > While it is common for the PC update to happen in the > shadow of a goto_tb, it is not required to be there. > By moving it before the goto_tb, we can also place the > call to helper_per_branch there, and then afterward > chain to the next tb. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/s390x/tcg/translate.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c > index a2315ac73e..e6c7c2a6ae 100644 > --- a/target/s390x/tcg/translate.c > +++ b/target/s390x/tcg/translate.c > @@ -654,9 +654,6 @@ static void gen_op_calc_cc(DisasContext *s) > > static bool use_goto_tb(DisasContext *s, uint64_t dest) > { > - if (per_enabled(s)) { > - return false; > - } > return translator_use_goto_tb(&s->base, dest); > } > > @@ -1157,15 +1154,16 @@ static DisasJumpType help_goto_direct(DisasContext *s, int64_t disp) > per_branch_disp(s, disp); > return DISAS_NEXT; > } > + > + update_psw_addr_disp(s, disp); > + per_branch_dest(s, psw_addr); > + > if (use_goto_tb(s, s->base.pc_next + disp)) { > update_cc_op(s); > tcg_gen_goto_tb(0); > - update_psw_addr_disp(s, disp); > tcg_gen_exit_tb(s->base.tb, 0); > return DISAS_NORETURN; > } else { > - update_psw_addr_disp(s, disp); > - per_branch_dest(s, psw_addr); > return DISAS_PC_UPDATED; > } > } > -- > 2.34.1 While looking at this, I had a hard time convincing myself that successful-branch PER events work at all. The code does set per_perc_atmid, but afterwards it does goto_tb/exit_tb, and does not reach per_check_exception() added by translate_one(). I wrote a small test for this theory by turning on PER for successful-branch events and looping 10 times. It passes in KVM, but fails in TCG. Here is the relevant IR fragment: IN: 0x00000212: a706 0000 brct %r0, 0x212 OP: ld_i32 tmp0,env,$0xfffffffffffffff0 brcond_i32 tmp0,$0x0,lt,$L0 ---- 0000000000000212 0000000000000004 0000000000000004 mov_i64 tmp2,psw_addr call per_ifetch,$0x1,$0,env,tmp2 sub_i64 tmp2,r0,$0x1 extract2_i64 r0,r0,tmp2,$0x20 rotl_i64 r0,r0,$0x20 mov_i32 tmp0,tmp2 brcond_i32 tmp0,$0x0,eq,$L1 /* branch taken: set per_perc_atmid and exit */ mov_i64 gbea,psw_addr call per_branch,$0x1,$0,env,gbea,psw_addr goto_tb $0x0 exit_tb $0x7f73fc000400 set_label $L1 add_i64 psw_addr,psw_addr,$0x4 goto_tb $0x1 exit_tb $0x7f73fc000401 /* check per_perc_atmid */ call per_check_exception,$0x0,$0,env set_label $L0 exit_tb $0x7f73fc000403 I will post the proposed fix and the test itself shortly. That said, the patch makes sense to me and does not make things worse, so: Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>
diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c index a2315ac73e..e6c7c2a6ae 100644 --- a/target/s390x/tcg/translate.c +++ b/target/s390x/tcg/translate.c @@ -654,9 +654,6 @@ static void gen_op_calc_cc(DisasContext *s) static bool use_goto_tb(DisasContext *s, uint64_t dest) { - if (per_enabled(s)) { - return false; - } return translator_use_goto_tb(&s->base, dest); } @@ -1157,15 +1154,16 @@ static DisasJumpType help_goto_direct(DisasContext *s, int64_t disp) per_branch_disp(s, disp); return DISAS_NEXT; } + + update_psw_addr_disp(s, disp); + per_branch_dest(s, psw_addr); + if (use_goto_tb(s, s->base.pc_next + disp)) { update_cc_op(s); tcg_gen_goto_tb(0); - update_psw_addr_disp(s, disp); tcg_gen_exit_tb(s->base.tb, 0); return DISAS_NORETURN; } else { - update_psw_addr_disp(s, disp); - per_branch_dest(s, psw_addr); return DISAS_PC_UPDATED; } }
While it is common for the PC update to happen in the shadow of a goto_tb, it is not required to be there. By moving it before the goto_tb, we can also place the call to helper_per_branch there, and then afterward chain to the next tb. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/s390x/tcg/translate.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)