Re: [PATCH v2 3/5] target/s390x: Fix LAALG not updating cc_src

[David Hildenbrand <david@redhat.com>]

On 06.11.23 10:31, Ilya Leoshkevich wrote:
> LAALG uses op_laa() and wout_addu64(). The latter expects cc_src to be
> set, but the former does not do it. This can lead to assertion failures
> if something sets cc_src to neither 0 nor 1 before.
> 
> Fix by introducing op_laa_addu64(), which sets cc_src, and using it for
> LAALG.
> 
> Fixes: 4dba4d6fef61 ("target/s390x: Use atomic operations for LOAD AND OP")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   target/s390x/tcg/insn-data.h.inc |  2 +-
>   target/s390x/tcg/translate.c     | 19 +++++++++++++++++--
>   2 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/target/s390x/tcg/insn-data.h.inc b/target/s390x/tcg/insn-data.h.inc
> index 0bfd88d3c3a..2f07f39d9cb 100644
> --- a/target/s390x/tcg/insn-data.h.inc
> +++ b/target/s390x/tcg/insn-data.h.inc
> @@ -442,7 +442,7 @@
>       D(0xebe8, LAAG,    RSY_a, ILA, r3, a2, new, in2_r1, laa, adds64, MO_TEUQ)
>   /* LOAD AND ADD LOGICAL */
>       D(0xebfa, LAAL,    RSY_a, ILA, r3_32u, a2, new, in2_r1_32, laa, addu32, MO_TEUL)
> -    D(0xebea, LAALG,   RSY_a, ILA, r3, a2, new, in2_r1, laa, addu64, MO_TEUQ)
> +    D(0xebea, LAALG,   RSY_a, ILA, r3, a2, new, in2_r1, laa_addu64, addu64, MO_TEUQ)
>   /* LOAD AND AND */
>       D(0xebf4, LAN,     RSY_a, ILA, r3_32s, a2, new, in2_r1_32, lan, nz32, MO_TESL)
>       D(0xebe4, LANG,    RSY_a, ILA, r3, a2, new, in2_r1, lan, nz64, MO_TEUQ)
> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
> index a0d6a2a35dd..62ab2be8b12 100644
> --- a/target/s390x/tcg/translate.c
> +++ b/target/s390x/tcg/translate.c
> @@ -2677,17 +2677,32 @@ static DisasJumpType op_kxb(DisasContext *s, DisasOps *o)
>       return DISAS_NEXT;
>   }
>   
> -static DisasJumpType op_laa(DisasContext *s, DisasOps *o)
> +static DisasJumpType help_laa(DisasContext *s, DisasOps *o, bool addu64)
>   {
>       /* The real output is indeed the original value in memory;
>          recompute the addition for the computation of CC.  */
>       tcg_gen_atomic_fetch_add_i64(o->in2, o->in2, o->in1, get_mem_index(s),
>                                    s->insn->data | MO_ALIGN);
>       /* However, we need to recompute the addition for setting CC.  */
> -    tcg_gen_add_i64(o->out, o->in1, o->in2);
> +    if (addu64) {
> +        tcg_gen_movi_i64(cc_src, 0);
> +        tcg_gen_add2_i64(o->out, cc_src, o->in1, cc_src, o->in2, cc_src);
> +    } else {
> +        tcg_gen_add_i64(o->out, o->in1, o->in2);
> +    }
>       return DISAS_NEXT;
>   }
>   
> +static DisasJumpType op_laa(DisasContext *s, DisasOps *o)
> +{
> +    return help_laa(s, o, false);
> +}
> +
> +static DisasJumpType op_laa_addu64(DisasContext *s, DisasOps *o)
> +{
> +    return help_laa(s, o, true);
> +}
> +
>   static DisasJumpType op_lan(DisasContext *s, DisasOps *o)
>   {
>       /* The real output is indeed the original value in memory;

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb