| Message ID | 20170406102249.20383-4-nikunj@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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~
Richard Henderson <rth@twiddle.net> writes: > 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. Sure > >> @@ -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. Sure. > 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? We do that in the macro: if (len > 1) { \ gen_check_align(ctx, t0, (len) - 1); \ } \ Would we still need a barrier before the alignment check? > I suspect that we do. Regards Nikunj
On 04/06/2017 10:21 PM, Nikunj A Dadhania wrote: > We do that in the macro: > > if (len > 1) { \ > gen_check_align(ctx, t0, (len) - 1); \ > } \ > > Would we still need a barrier before the alignment check? Ah, that's where it's been placed. So, the MO_ALIGN flag to tcg_gen_atomic_* takes care of the alignment check too. So we could move this down into the failure path. r~
diff --git a/target/ppc/translate.c b/target/ppc/translate.c index a9c733d..87b4fe4 100644 --- a/target/ppc/translate.c +++ b/target/ppc/translate.c @@ -2971,6 +2971,7 @@ static void gen_stswx(DisasContext *ctx) /* eieio */ static void gen_eieio(DisasContext *ctx) { + tcg_gen_mb(TCG_MO_LD_ST | TCG_BAR_SC); } #if !defined(CONFIG_USER_ONLY) @@ -3008,6 +3009,7 @@ static void gen_isync(DisasContext *ctx) if (!ctx->pr) { gen_check_tlb_flush(ctx, false); } + tcg_gen_mb(TCG_MO_ALL | TCG_BAR_SC); gen_stop_exception(ctx); } @@ -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); \ tcg_temp_free(t0); \ } @@ -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); \ } @@ -3309,6 +3313,7 @@ static void gen_sync(DisasContext *ctx) if (((l == 2) || !(ctx->insns_flags & PPC_64B)) && !ctx->pr) { gen_check_tlb_flush(ctx, true); } + tcg_gen_mb(TCG_MO_ALL | TCG_BAR_SC); } /* wait */
Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> --- target/ppc/translate.c | 5 +++++ 1 file changed, 5 insertions(+)