Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
target/ppc/translate.c | 5 +++++
1 file changed, 5 insertions(+)
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 */
--
2.9.3
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~
© 2016 - 2026 Red Hat, Inc.