Re: [qemu-s390x] [PATCH v3 9/9] target/s390x: Check HAVE_ATOMIC128 and HAVE_CMPXCHG128 at translate

[David Hildenbrand]

On 03/10/2018 21:39, Richard Henderson wrote:
> Cc: address@hidden
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target/s390x/mem_helper.c | 40 +++++++++++++++++++--------------------
>  target/s390x/translate.c  | 25 +++++++++++++++++-------
>  2 files changed, 38 insertions(+), 27 deletions(-)
> 
> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
> index b5858d2fa2..490c43e6e6 100644
> --- a/target/s390x/mem_helper.c
> +++ b/target/s390x/mem_helper.c
> @@ -1420,9 +1420,7 @@ void HELPER(cdsg_parallel)(CPUS390XState *env, uint64_t 
> addr,
>      Int128 oldv;
>      bool fail;
>  
> -    if (!HAVE_CMPXCHG128) {
> -        cpu_loop_exit_atomic(ENV_GET_CPU(env), ra);
> -    }
> +    assert(HAVE_CMPXCHG128);
>  
>      mem_idx = cpu_mmu_index(env, false);
>      oi = make_memop_idx(MO_TEQ | MO_ALIGN_16, mem_idx);
> @@ -2115,16 +2113,17 @@ uint64_t HELPER(lpq_parallel)(CPUS390XState *env, 
> uint64_t addr)
>  {
>      uintptr_t ra = GETPC();
>      uint64_t hi, lo;
> +    int mem_idx;
> +    TCGMemOpIdx oi;
> +    Int128 v;
>  
> -    if (HAVE_ATOMIC128) {
> -        int mem_idx = cpu_mmu_index(env, false);
> -        TCGMemOpIdx oi = make_memop_idx(MO_TEQ | MO_ALIGN_16, mem_idx);
> -        Int128 v = helper_atomic_ldo_be_mmu(env, addr, oi, ra);
> -        hi = int128_gethi(v);
> -        lo = int128_getlo(v);
> -    } else {
> -        cpu_loop_exit_atomic(ENV_GET_CPU(env), ra);
> -    }
> +    assert(HAVE_ATOMIC128);
> +
> +    mem_idx = cpu_mmu_index(env, false);
> +    oi = make_memop_idx(MO_TEQ | MO_ALIGN_16, mem_idx);
> +    v = helper_atomic_ldo_be_mmu(env, addr, oi, ra);
> +    hi = int128_gethi(v);
> +    lo = int128_getlo(v);
>  
>      env->retxl = lo;
>      return hi;
> @@ -2145,15 +2144,16 @@ void HELPER(stpq_parallel)(CPUS390XState *env, 
> uint64_t addr,
>                             uint64_t low, uint64_t high)
>  {
>      uintptr_t ra = GETPC();
> +    int mem_idx;
> +    TCGMemOpIdx oi;
> +    Int128 v;
>  
> -    if (HAVE_ATOMIC128) {
> -        int mem_idx = cpu_mmu_index(env, false);
> -        TCGMemOpIdx oi = make_memop_idx(MO_TEQ | MO_ALIGN_16, mem_idx);
> -        Int128 v = int128_make128(low, high);
> -        helper_atomic_sto_be_mmu(env, addr, v, oi, ra);
> -    } else {
> -        cpu_loop_exit_atomic(ENV_GET_CPU(env), ra);
> -    }
> +    assert(HAVE_ATOMIC128);
> +
> +    mem_idx = cpu_mmu_index(env, false);
> +    oi = make_memop_idx(MO_TEQ | MO_ALIGN_16, mem_idx);
> +    v = int128_make128(low, high);
> +    helper_atomic_sto_be_mmu(env, addr, v, oi, ra);
>  }
>  
>  /* Execute instruction.  This instruction executes an insn modified with
> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index 7fad3ad8e9..57fe74e4a0 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -44,6 +44,7 @@
>  #include "trace-tcg.h"
>  #include "exec/translator.h"
>  #include "exec/log.h"
> +#include "qemu/atomic128.h"
>  
>  
>  /* Information that (most) every instruction needs to manipulate.  */
> @@ -2032,6 +2033,7 @@ static DisasJumpType op_cdsg(DisasContext *s, DisasOps 
> *o)
>      int r3 = get_field(s->fields, r3);
>      int d2 = get_field(s->fields, d2);
>      int b2 = get_field(s->fields, b2);
> +    DisasJumpType ret = DISAS_NEXT;
>      TCGv_i64 addr;
>      TCGv_i32 t_r1, t_r3;
>  
> @@ -2039,17 +2041,20 @@ static DisasJumpType op_cdsg(DisasContext *s, 
> DisasOps *o)
>      addr = get_address(s, 0, b2, d2);
>      t_r1 = tcg_const_i32(r1);
>      t_r3 = tcg_const_i32(r3);
> -    if (tb_cflags(s->base.tb) & CF_PARALLEL) {
> +    if (!(tb_cflags(s->base.tb) & CF_PARALLEL)) {
> +        gen_helper_cdsg(cpu_env, addr, t_r1, t_r3);
> +    } else if (HAVE_CMPXCHG128) {
>          gen_helper_cdsg_parallel(cpu_env, addr, t_r1, t_r3);
>      } else {
> -        gen_helper_cdsg(cpu_env, addr, t_r1, t_r3);
> +        gen_helper_exit_atomic(cpu_env);
> +        ret = DISAS_NORETURN;
>      }
>      tcg_temp_free_i64(addr);
>      tcg_temp_free_i32(t_r1);
>      tcg_temp_free_i32(t_r3);
>  
>      set_cc_static(s);
> -    return DISAS_NEXT;
> +    return ret;
>  }
>  
>  static DisasJumpType op_csst(DisasContext *s, DisasOps *o)
> @@ -3036,10 +3041,13 @@ static DisasJumpType op_lpd(DisasContext *s, DisasOps 
> *o)
>  
>  static DisasJumpType op_lpq(DisasContext *s, DisasOps *o)
>  {
> -    if (tb_cflags(s->base.tb) & CF_PARALLEL) {
> +    if (!(tb_cflags(s->base.tb) & CF_PARALLEL)) {
> +        gen_helper_lpq(o->out, cpu_env, o->in2);
> +    } else if (HAVE_ATOMIC128) {
>          gen_helper_lpq_parallel(o->out, cpu_env, o->in2);
>      } else {
> -        gen_helper_lpq(o->out, cpu_env, o->in2);
> +        gen_helper_exit_atomic(cpu_env);
> +        return DISAS_NORETURN;
>      }
>      return_low128(o->out2);
>      return DISAS_NEXT;
> @@ -4462,10 +4470,13 @@ static DisasJumpType op_stmh(DisasContext *s, 
> DisasOps *o)
>  
>  static DisasJumpType op_stpq(DisasContext *s, DisasOps *o)
>  {
> -    if (tb_cflags(s->base.tb) & CF_PARALLEL) {
> +    if (!(tb_cflags(s->base.tb) & CF_PARALLEL)) {
> +        gen_helper_stpq(cpu_env, o->in2, o->out2, o->out);
> +    } else if (HAVE_ATOMIC128) {
>          gen_helper_stpq_parallel(cpu_env, o->in2, o->out2, o->out);
>      } else {
> -        gen_helper_stpq(cpu_env, o->in2, o->out2, o->out);
> +        gen_helper_exit_atomic(cpu_env);
> +        return DISAS_NORETURN;
>      }
>      return DISAS_NEXT;
>  }
> 

Reviewed-by: David Hildenbrand <address@hidden>

-- 

Thanks,

David / dhildenb