Re: [PATCH 21/26] target/s390x: Remove PER check from use_goto_tb

[Ilya Leoshkevich]

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>