Re: [Qemu-ppc] [PATCH RFC v1 3/3] target/ppc: Generate fence operations

[Richard Henderson]

On 04/06/2017 03:22 AM, Nikunj A Dadhania wrote:
> @@ -3028,6 +3030,7 @@ static void gen_##name(DisasContext *ctx)                 
>            \
>      tcg_gen_qemu_ld_tl(gpr, t0, ctx->mem_idx, memop);                \
>      tcg_gen_mov_tl(cpu_reserve, t0);                                 \
>      tcg_gen_mov_tl(cpu_reserve_val, gpr);                            \
> +    tcg_gen_mb(TCG_MO_ALL | TCG_BAR_LDAQ);                           \

Please put the barrier next to the load.
I hope we can merge these soon.


> @@ -3196,6 +3199,7 @@ static void gen_##name(DisasContext *ctx)                 
>   \
>      if (len > 1) {                                          \
>          gen_check_align(ctx, t0, (len) - 1);                \
>      }                                                       \
> +    tcg_gen_mb(TCG_MO_ALL | TCG_BAR_STRL);                  \
>      gen_conditional_store(ctx, t0, rS(ctx->opcode), memop); \
>      tcg_temp_free(t0);                                      \
>  }

This one is more complicated...

The success path includes an atomic_cmpxchg, which itself is a SC barrier. 
However, the fail path branches across the cmpxchg and so sees no barrier, but 
one is still required by the architecture.  How about a gen_conditional_store 
that looks like this:

{
  TCGLabel *l1 = gen_new_label();
  TCGLabel *l2 = gen_new_label();
  TCGv t0;

  tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, l1);

  t0 = tcg_temp_new();
  tcg_gen_atomic_cmpxchg_tl(t0, cpu_reserve, cpu_reserve_val,
                            cpu_gpr[reg], ctx->mem_idx,
                            DEF_MEMOP(memop) | MO_ALIGN);
  tcg_gen_setcond_tl(TCG_COND_EQ, t0, t0, cpu_reserve_val);
  tcg_gen_shli_tl(t0, t0, CRF_EQ_BIT);
  tcg_gen_or_tl(t0, t0, cpu_so);
  tcg_gen_trunc_tl(cpu_crf[0], t0);
  tcg_temp_free(t0);
  tcg_gen_br(l2);

  gen_set_label(l1);

  /* Address mismatch implies failure.  But we still need to provide the
     memory barrier semantics of the instruction.  */
  tcg_gen_mb(TCG_MO_ALL | TCG_BAR_STRL);
  tcg_gen_trunc_tl(cpu_crf[0], cpu_so);

  gen_set_label(l2);
  tcg_gen_movi_tl(cpu_reserve, -1);
}

Note that you don't need to reset cpu_reserve_val at all.

I just thought of something we might need to check for this and other ll/sc 
implemetations -- do we need to check for address misalignment along the 
address comparison failure path?  I suspect that we do.


r~