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>