Re: [PATCH 1/2] target/s390x: Fix EXECUTE of relative branches

[Richard Henderson]

On 4/27/23 00:58, Ilya Leoshkevich wrote:
> Fix a problem similar to the one fixed by commit 703d03a4aaf3
> ("target/s390x: Fix EXECUTE of relative long instructions"), but now
> for relative branches.
>
> Reported-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   target/s390x/tcg/translate.c | 81 ++++++++++++++++++++++++++----------
>   1 file changed, 58 insertions(+), 23 deletions(-)
>
> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
> index 46b874e94da..2a681428af1 100644
> --- a/target/s390x/tcg/translate.c
> +++ b/target/s390x/tcg/translate.c
> @@ -1534,18 +1534,51 @@ static DisasJumpType op_bal(DisasContext *s, DisasOps 
> *o)
>       }
>   }
>   
> +/*
> + * Disassemble the target of a branch. The results are returned in a form
> + * suitable for passing into help_branch():
> + *
> + * - bool IS_IMM reflects whether the target is fixed or computed. Non-EXECUTEd
> + *   branches, whose DisasContext *S contains the relative immediate field RI,
> + *   are considered fixed. All the other branches are considered computed.
> + * - int IMM is the value of RI.
> + * - TCGv_i64 CDEST is the address of the computed target.
> + */
> +#define disas_jdest(s, ri, is_imm, imm, cdest) do {                            
> \
> +    if (have_field(s, ri)) {                                                   
> \
> +        if (unlikely(s->ex_value)) {                                           
> \
> +            cdest = tcg_temp_new_i64();                                        
> \
> +            tcg_gen_ld_i64(cdest, cpu_env, offsetof(CPUS390XState, 
> ex_target));\
> +            tcg_gen_addi_i64(cdest, cdest, (int64_t)get_field(s, ri) * 2);     
> \
> +            is_imm = false;                                                    
> \
> +        } else {                                                               
> \
> +            is_imm = true;                                                     
> \
> +        }                                                                      
> \
> +    } else {                                                                   
> \
> +        is_imm = false;                                                        
> \
> +    }                                                                          
> \
> +    imm = is_imm ? get_field(s, ri) : 0;                                       
> \
> +} while (false)

I'm not especially fond of the macro, but given the variable field, I don't know how else 
to structure it.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~