* [PATCH v2 2/4] target/ppc: Ensure stcx size matches larx
2023-06-05 2:54 [PATCH v2 1/4] target/ppc: Fix lqarx to set cpu_reserve Nicholas Piggin
@ 2023-06-05 2:54 ` Nicholas Piggin
2023-06-05 13:42 ` Daniel Henrique Barboza
2023-06-05 2:54 ` [PATCH v2 3/4] target/ppc: Remove larx/stcx. memory barrier semantics Nicholas Piggin
` (3 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Nicholas Piggin @ 2023-06-05 2:54 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: Nicholas Piggin, Richard Henderson, qemu-ppc, qemu-devel,
qemu-stable
Differently-sized larx/stcx. pairs can succeed if the starting address
matches. Add a check to require the size of stcx. exactly match the larx
that established the reservation. Use the term "reserve_length" for this
state, which matches the terminology used in the ISA.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
v2:
- Changed lqarx/stqcx. reservation size to 16 [Richard]
- Changed name to reserve_length [Richard]
target/ppc/cpu.h | 5 +++--
target/ppc/cpu_init.c | 4 ++--
target/ppc/translate.c | 9 +++++++++
3 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 7959bfed0a..45d84ce06a 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1123,8 +1123,9 @@ struct CPUArchState {
target_ulong ov32;
target_ulong ca32;
- target_ulong reserve_addr; /* Reservation address */
- target_ulong reserve_val; /* Reservation value */
+ target_ulong reserve_addr; /* Reservation address */
+ target_ulong reserve_length; /* Reservation larx op size (bytes) */
+ target_ulong reserve_val; /* Reservation value */
target_ulong reserve_val2;
/* These are used in supervisor mode only */
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 944a74befe..c3dd7052a3 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -7421,8 +7421,8 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
}
qemu_fprintf(f, " %c%c", a, env->crf[i] & 0x01 ? 'O' : ' ');
}
- qemu_fprintf(f, " ] RES " TARGET_FMT_lx "\n",
- env->reserve_addr);
+ qemu_fprintf(f, " ] RES %03x@" TARGET_FMT_lx "\n",
+ (int)env->reserve_length, env->reserve_addr);
if (flags & CPU_DUMP_FPU) {
for (i = 0; i < 32; i++) {
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 7a5bf1d820..538f757dec 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -71,6 +71,7 @@ static TCGv cpu_cfar;
#endif
static TCGv cpu_xer, cpu_so, cpu_ov, cpu_ca, cpu_ov32, cpu_ca32;
static TCGv cpu_reserve;
+static TCGv cpu_reserve_length;
static TCGv cpu_reserve_val;
static TCGv cpu_reserve_val2;
static TCGv cpu_fpscr;
@@ -141,6 +142,10 @@ void ppc_translate_init(void)
cpu_reserve = tcg_global_mem_new(cpu_env,
offsetof(CPUPPCState, reserve_addr),
"reserve_addr");
+ cpu_reserve_length = tcg_global_mem_new(cpu_env,
+ offsetof(CPUPPCState,
+ reserve_length),
+ "reserve_length");
cpu_reserve_val = tcg_global_mem_new(cpu_env,
offsetof(CPUPPCState, reserve_val),
"reserve_val");
@@ -3585,6 +3590,7 @@ static void gen_load_locked(DisasContext *ctx, MemOp memop)
gen_addr_reg_index(ctx, t0);
tcg_gen_qemu_ld_tl(gpr, t0, ctx->mem_idx, memop | MO_ALIGN);
tcg_gen_mov_tl(cpu_reserve, t0);
+ tcg_gen_movi_tl(cpu_reserve_length, memop_size(memop));
tcg_gen_mov_tl(cpu_reserve_val, gpr);
tcg_gen_mb(TCG_MO_ALL | TCG_BAR_LDAQ);
}
@@ -3816,6 +3822,7 @@ static void gen_conditional_store(DisasContext *ctx, MemOp memop)
gen_set_access_type(ctx, ACCESS_RES);
gen_addr_reg_index(ctx, t0);
tcg_gen_brcond_tl(TCG_COND_NE, t0, cpu_reserve, l1);
+ tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, memop_size(memop), l1);
t0 = tcg_temp_new();
tcg_gen_atomic_cmpxchg_tl(t0, cpu_reserve, cpu_reserve_val,
@@ -3882,6 +3889,7 @@ static void gen_lqarx(DisasContext *ctx)
tcg_gen_extr_i128_i64(lo, hi, t16);
tcg_gen_mov_tl(cpu_reserve, EA);
+ tcg_gen_movi_tl(cpu_reserve_length, 16);
tcg_gen_st_tl(hi, cpu_env, offsetof(CPUPPCState, reserve_val));
tcg_gen_st_tl(lo, cpu_env, offsetof(CPUPPCState, reserve_val2));
}
@@ -3907,6 +3915,7 @@ static void gen_stqcx_(DisasContext *ctx)
gen_addr_reg_index(ctx, EA);
tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lab_fail);
+ tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, 16, lab_fail);
cmp = tcg_temp_new_i128();
val = tcg_temp_new_i128();
--
2.40.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2 2/4] target/ppc: Ensure stcx size matches larx
2023-06-05 2:54 ` [PATCH v2 2/4] target/ppc: Ensure stcx size matches larx Nicholas Piggin
@ 2023-06-05 13:42 ` Daniel Henrique Barboza
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Henrique Barboza @ 2023-06-05 13:42 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: Richard Henderson, qemu-ppc, qemu-devel, qemu-stable
On 6/4/23 23:54, Nicholas Piggin wrote:
> Differently-sized larx/stcx. pairs can succeed if the starting address
> matches. Add a check to require the size of stcx. exactly match the larx
> that established the reservation. Use the term "reserve_length" for this
> state, which matches the terminology used in the ISA.
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
Queued. Thanks,
Daniel
> v2:
> - Changed lqarx/stqcx. reservation size to 16 [Richard]
> - Changed name to reserve_length [Richard]
>
> target/ppc/cpu.h | 5 +++--
> target/ppc/cpu_init.c | 4 ++--
> target/ppc/translate.c | 9 +++++++++
> 3 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 7959bfed0a..45d84ce06a 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1123,8 +1123,9 @@ struct CPUArchState {
> target_ulong ov32;
> target_ulong ca32;
>
> - target_ulong reserve_addr; /* Reservation address */
> - target_ulong reserve_val; /* Reservation value */
> + target_ulong reserve_addr; /* Reservation address */
> + target_ulong reserve_length; /* Reservation larx op size (bytes) */
> + target_ulong reserve_val; /* Reservation value */
> target_ulong reserve_val2;
>
> /* These are used in supervisor mode only */
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 944a74befe..c3dd7052a3 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -7421,8 +7421,8 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
> }
> qemu_fprintf(f, " %c%c", a, env->crf[i] & 0x01 ? 'O' : ' ');
> }
> - qemu_fprintf(f, " ] RES " TARGET_FMT_lx "\n",
> - env->reserve_addr);
> + qemu_fprintf(f, " ] RES %03x@" TARGET_FMT_lx "\n",
> + (int)env->reserve_length, env->reserve_addr);
>
> if (flags & CPU_DUMP_FPU) {
> for (i = 0; i < 32; i++) {
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 7a5bf1d820..538f757dec 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -71,6 +71,7 @@ static TCGv cpu_cfar;
> #endif
> static TCGv cpu_xer, cpu_so, cpu_ov, cpu_ca, cpu_ov32, cpu_ca32;
> static TCGv cpu_reserve;
> +static TCGv cpu_reserve_length;
> static TCGv cpu_reserve_val;
> static TCGv cpu_reserve_val2;
> static TCGv cpu_fpscr;
> @@ -141,6 +142,10 @@ void ppc_translate_init(void)
> cpu_reserve = tcg_global_mem_new(cpu_env,
> offsetof(CPUPPCState, reserve_addr),
> "reserve_addr");
> + cpu_reserve_length = tcg_global_mem_new(cpu_env,
> + offsetof(CPUPPCState,
> + reserve_length),
> + "reserve_length");
> cpu_reserve_val = tcg_global_mem_new(cpu_env,
> offsetof(CPUPPCState, reserve_val),
> "reserve_val");
> @@ -3585,6 +3590,7 @@ static void gen_load_locked(DisasContext *ctx, MemOp memop)
> gen_addr_reg_index(ctx, t0);
> tcg_gen_qemu_ld_tl(gpr, t0, ctx->mem_idx, memop | MO_ALIGN);
> tcg_gen_mov_tl(cpu_reserve, t0);
> + tcg_gen_movi_tl(cpu_reserve_length, memop_size(memop));
> tcg_gen_mov_tl(cpu_reserve_val, gpr);
> tcg_gen_mb(TCG_MO_ALL | TCG_BAR_LDAQ);
> }
> @@ -3816,6 +3822,7 @@ static void gen_conditional_store(DisasContext *ctx, MemOp memop)
> gen_set_access_type(ctx, ACCESS_RES);
> gen_addr_reg_index(ctx, t0);
> tcg_gen_brcond_tl(TCG_COND_NE, t0, cpu_reserve, l1);
> + tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, memop_size(memop), l1);
>
> t0 = tcg_temp_new();
> tcg_gen_atomic_cmpxchg_tl(t0, cpu_reserve, cpu_reserve_val,
> @@ -3882,6 +3889,7 @@ static void gen_lqarx(DisasContext *ctx)
> tcg_gen_extr_i128_i64(lo, hi, t16);
>
> tcg_gen_mov_tl(cpu_reserve, EA);
> + tcg_gen_movi_tl(cpu_reserve_length, 16);
> tcg_gen_st_tl(hi, cpu_env, offsetof(CPUPPCState, reserve_val));
> tcg_gen_st_tl(lo, cpu_env, offsetof(CPUPPCState, reserve_val2));
> }
> @@ -3907,6 +3915,7 @@ static void gen_stqcx_(DisasContext *ctx)
> gen_addr_reg_index(ctx, EA);
>
> tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lab_fail);
> + tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, 16, lab_fail);
>
> cmp = tcg_temp_new_i128();
> val = tcg_temp_new_i128();
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 3/4] target/ppc: Remove larx/stcx. memory barrier semantics
2023-06-05 2:54 [PATCH v2 1/4] target/ppc: Fix lqarx to set cpu_reserve Nicholas Piggin
2023-06-05 2:54 ` [PATCH v2 2/4] target/ppc: Ensure stcx size matches larx Nicholas Piggin
@ 2023-06-05 2:54 ` Nicholas Piggin
2023-06-05 13:42 ` Daniel Henrique Barboza
2023-06-05 2:54 ` [PATCH v2 4/4] target/ppc: Rework store conditional to avoid branch Nicholas Piggin
` (2 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Nicholas Piggin @ 2023-06-05 2:54 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: Nicholas Piggin, Richard Henderson, qemu-ppc, qemu-devel,
qemu-stable
larx and stcx. are not defined to order any memory operations.
Remove the barriers.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
target/ppc/translate.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 538f757dec..acb99d8691 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -3592,7 +3592,6 @@ static void gen_load_locked(DisasContext *ctx, MemOp memop)
tcg_gen_mov_tl(cpu_reserve, t0);
tcg_gen_movi_tl(cpu_reserve_length, memop_size(memop));
tcg_gen_mov_tl(cpu_reserve_val, gpr);
- tcg_gen_mb(TCG_MO_ALL | TCG_BAR_LDAQ);
}
#define LARX(name, memop) \
@@ -3836,11 +3835,6 @@ static void gen_conditional_store(DisasContext *ctx, MemOp memop)
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_i32(cpu_crf[0], cpu_so);
gen_set_label(l2);
@@ -3944,11 +3938,6 @@ static void gen_stqcx_(DisasContext *ctx)
tcg_gen_br(lab_over);
gen_set_label(lab_fail);
- /*
- * 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_i32(cpu_crf[0], cpu_so);
gen_set_label(lab_over);
--
2.40.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2 3/4] target/ppc: Remove larx/stcx. memory barrier semantics
2023-06-05 2:54 ` [PATCH v2 3/4] target/ppc: Remove larx/stcx. memory barrier semantics Nicholas Piggin
@ 2023-06-05 13:42 ` Daniel Henrique Barboza
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Henrique Barboza @ 2023-06-05 13:42 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: Richard Henderson, qemu-ppc, qemu-devel, qemu-stable
On 6/4/23 23:54, Nicholas Piggin wrote:
> larx and stcx. are not defined to order any memory operations.
> Remove the barriers.
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
Queued. Thanks,
Daniel
> target/ppc/translate.c | 11 -----------
> 1 file changed, 11 deletions(-)
>
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 538f757dec..acb99d8691 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -3592,7 +3592,6 @@ static void gen_load_locked(DisasContext *ctx, MemOp memop)
> tcg_gen_mov_tl(cpu_reserve, t0);
> tcg_gen_movi_tl(cpu_reserve_length, memop_size(memop));
> tcg_gen_mov_tl(cpu_reserve_val, gpr);
> - tcg_gen_mb(TCG_MO_ALL | TCG_BAR_LDAQ);
> }
>
> #define LARX(name, memop) \
> @@ -3836,11 +3835,6 @@ static void gen_conditional_store(DisasContext *ctx, MemOp memop)
>
> 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_i32(cpu_crf[0], cpu_so);
>
> gen_set_label(l2);
> @@ -3944,11 +3938,6 @@ static void gen_stqcx_(DisasContext *ctx)
> tcg_gen_br(lab_over);
> gen_set_label(lab_fail);
>
> - /*
> - * 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_i32(cpu_crf[0], cpu_so);
>
> gen_set_label(lab_over);
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 4/4] target/ppc: Rework store conditional to avoid branch
2023-06-05 2:54 [PATCH v2 1/4] target/ppc: Fix lqarx to set cpu_reserve Nicholas Piggin
2023-06-05 2:54 ` [PATCH v2 2/4] target/ppc: Ensure stcx size matches larx Nicholas Piggin
2023-06-05 2:54 ` [PATCH v2 3/4] target/ppc: Remove larx/stcx. memory barrier semantics Nicholas Piggin
@ 2023-06-05 2:54 ` Nicholas Piggin
2023-06-05 13:42 ` Daniel Henrique Barboza
2023-06-05 3:09 ` [PATCH v2 1/4] target/ppc: Fix lqarx to set cpu_reserve Richard Henderson
2023-06-05 13:42 ` Daniel Henrique Barboza
4 siblings, 1 reply; 9+ messages in thread
From: Nicholas Piggin @ 2023-06-05 2:54 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: Nicholas Piggin, Richard Henderson, qemu-ppc, qemu-devel,
qemu-stable
Rework store conditional to avoid a branch in the success case.
Change some of the variable names and layout while here so
gen_conditional_store more closely matches gen_stqcx_.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
v2:
- Reinstate lost DEF_MEMOP [Richard]
I think the DEF_MEMOP is redundant here, but admit that's not something
that should be changed with this patch. I will look at cleaning those up
later.
Thanks,
Nick
target/ppc/translate.c | 63 ++++++++++++++++++++----------------------
1 file changed, 30 insertions(+), 33 deletions(-)
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index acb99d8691..434caad258 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -3813,31 +3813,32 @@ static void gen_stdat(DisasContext *ctx)
static void gen_conditional_store(DisasContext *ctx, MemOp memop)
{
- TCGLabel *l1 = gen_new_label();
- TCGLabel *l2 = gen_new_label();
- TCGv t0 = tcg_temp_new();
- int reg = rS(ctx->opcode);
+ TCGLabel *lfail;
+ TCGv EA;
+ TCGv cr0;
+ TCGv t0;
+ int rs = rS(ctx->opcode);
+ lfail = gen_new_label();
+ EA = tcg_temp_new();
+ cr0 = tcg_temp_new();
+ t0 = tcg_temp_new();
+
+ tcg_gen_mov_tl(cr0, cpu_so);
gen_set_access_type(ctx, ACCESS_RES);
- gen_addr_reg_index(ctx, t0);
- tcg_gen_brcond_tl(TCG_COND_NE, t0, cpu_reserve, l1);
- tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, memop_size(memop), l1);
+ gen_addr_reg_index(ctx, EA);
+ tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lfail);
+ tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, memop_size(memop), lfail);
- t0 = tcg_temp_new();
tcg_gen_atomic_cmpxchg_tl(t0, cpu_reserve, cpu_reserve_val,
- cpu_gpr[reg], ctx->mem_idx,
+ cpu_gpr[rs], 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_i32(cpu_crf[0], t0);
- tcg_gen_br(l2);
+ tcg_gen_or_tl(cr0, cr0, t0);
- gen_set_label(l1);
-
- tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
-
- gen_set_label(l2);
+ gen_set_label(lfail);
+ tcg_gen_trunc_tl_i32(cpu_crf[0], cr0);
tcg_gen_movi_tl(cpu_reserve, -1);
}
@@ -3891,25 +3892,26 @@ static void gen_lqarx(DisasContext *ctx)
/* stqcx. */
static void gen_stqcx_(DisasContext *ctx)
{
- TCGLabel *lab_fail, *lab_over;
- int rs = rS(ctx->opcode);
+ TCGLabel *lfail;
TCGv EA, t0, t1;
+ TCGv cr0;
TCGv_i128 cmp, val;
+ int rs = rS(ctx->opcode);
if (unlikely(rs & 1)) {
gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
return;
}
- lab_fail = gen_new_label();
- lab_over = gen_new_label();
+ lfail = gen_new_label();
+ EA = tcg_temp_new();
+ cr0 = tcg_temp_new();
+ tcg_gen_mov_tl(cr0, cpu_so);
gen_set_access_type(ctx, ACCESS_RES);
- EA = tcg_temp_new();
gen_addr_reg_index(ctx, EA);
-
- tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lab_fail);
- tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, 16, lab_fail);
+ tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lfail);
+ tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, 16, lfail);
cmp = tcg_temp_new_i128();
val = tcg_temp_new_i128();
@@ -3932,15 +3934,10 @@ static void gen_stqcx_(DisasContext *ctx)
tcg_gen_setcondi_tl(TCG_COND_EQ, t0, t0, 0);
tcg_gen_shli_tl(t0, t0, CRF_EQ_BIT);
- tcg_gen_or_tl(t0, t0, cpu_so);
- tcg_gen_trunc_tl_i32(cpu_crf[0], t0);
-
- tcg_gen_br(lab_over);
- gen_set_label(lab_fail);
-
- tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
+ tcg_gen_or_tl(cr0, cr0, t0);
- gen_set_label(lab_over);
+ gen_set_label(lfail);
+ tcg_gen_trunc_tl_i32(cpu_crf[0], cr0);
tcg_gen_movi_tl(cpu_reserve, -1);
}
#endif /* defined(TARGET_PPC64) */
--
2.40.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2 4/4] target/ppc: Rework store conditional to avoid branch
2023-06-05 2:54 ` [PATCH v2 4/4] target/ppc: Rework store conditional to avoid branch Nicholas Piggin
@ 2023-06-05 13:42 ` Daniel Henrique Barboza
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Henrique Barboza @ 2023-06-05 13:42 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: Richard Henderson, qemu-ppc, qemu-devel, qemu-stable
On 6/4/23 23:54, Nicholas Piggin wrote:
> Rework store conditional to avoid a branch in the success case.
> Change some of the variable names and layout while here so
> gen_conditional_store more closely matches gen_stqcx_.
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
Queued. Thanks,
Daniel
> v2:
> - Reinstate lost DEF_MEMOP [Richard]
>
> I think the DEF_MEMOP is redundant here, but admit that's not something
> that should be changed with this patch. I will look at cleaning those up
> later.
>
> Thanks,
> Nick
>
> target/ppc/translate.c | 63 ++++++++++++++++++++----------------------
> 1 file changed, 30 insertions(+), 33 deletions(-)
>
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index acb99d8691..434caad258 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -3813,31 +3813,32 @@ static void gen_stdat(DisasContext *ctx)
>
> static void gen_conditional_store(DisasContext *ctx, MemOp memop)
> {
> - TCGLabel *l1 = gen_new_label();
> - TCGLabel *l2 = gen_new_label();
> - TCGv t0 = tcg_temp_new();
> - int reg = rS(ctx->opcode);
> + TCGLabel *lfail;
> + TCGv EA;
> + TCGv cr0;
> + TCGv t0;
> + int rs = rS(ctx->opcode);
>
> + lfail = gen_new_label();
> + EA = tcg_temp_new();
> + cr0 = tcg_temp_new();
> + t0 = tcg_temp_new();
> +
> + tcg_gen_mov_tl(cr0, cpu_so);
> gen_set_access_type(ctx, ACCESS_RES);
> - gen_addr_reg_index(ctx, t0);
> - tcg_gen_brcond_tl(TCG_COND_NE, t0, cpu_reserve, l1);
> - tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, memop_size(memop), l1);
> + gen_addr_reg_index(ctx, EA);
> + tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lfail);
> + tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, memop_size(memop), lfail);
>
> - t0 = tcg_temp_new();
> tcg_gen_atomic_cmpxchg_tl(t0, cpu_reserve, cpu_reserve_val,
> - cpu_gpr[reg], ctx->mem_idx,
> + cpu_gpr[rs], 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_i32(cpu_crf[0], t0);
> - tcg_gen_br(l2);
> + tcg_gen_or_tl(cr0, cr0, t0);
>
> - gen_set_label(l1);
> -
> - tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
> -
> - gen_set_label(l2);
> + gen_set_label(lfail);
> + tcg_gen_trunc_tl_i32(cpu_crf[0], cr0);
> tcg_gen_movi_tl(cpu_reserve, -1);
> }
>
> @@ -3891,25 +3892,26 @@ static void gen_lqarx(DisasContext *ctx)
> /* stqcx. */
> static void gen_stqcx_(DisasContext *ctx)
> {
> - TCGLabel *lab_fail, *lab_over;
> - int rs = rS(ctx->opcode);
> + TCGLabel *lfail;
> TCGv EA, t0, t1;
> + TCGv cr0;
> TCGv_i128 cmp, val;
> + int rs = rS(ctx->opcode);
>
> if (unlikely(rs & 1)) {
> gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
> return;
> }
>
> - lab_fail = gen_new_label();
> - lab_over = gen_new_label();
> + lfail = gen_new_label();
> + EA = tcg_temp_new();
> + cr0 = tcg_temp_new();
>
> + tcg_gen_mov_tl(cr0, cpu_so);
> gen_set_access_type(ctx, ACCESS_RES);
> - EA = tcg_temp_new();
> gen_addr_reg_index(ctx, EA);
> -
> - tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lab_fail);
> - tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, 16, lab_fail);
> + tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lfail);
> + tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, 16, lfail);
>
> cmp = tcg_temp_new_i128();
> val = tcg_temp_new_i128();
> @@ -3932,15 +3934,10 @@ static void gen_stqcx_(DisasContext *ctx)
>
> tcg_gen_setcondi_tl(TCG_COND_EQ, t0, t0, 0);
> tcg_gen_shli_tl(t0, t0, CRF_EQ_BIT);
> - tcg_gen_or_tl(t0, t0, cpu_so);
> - tcg_gen_trunc_tl_i32(cpu_crf[0], t0);
> -
> - tcg_gen_br(lab_over);
> - gen_set_label(lab_fail);
> -
> - tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
> + tcg_gen_or_tl(cr0, cr0, t0);
>
> - gen_set_label(lab_over);
> + gen_set_label(lfail);
> + tcg_gen_trunc_tl_i32(cpu_crf[0], cr0);
> tcg_gen_movi_tl(cpu_reserve, -1);
> }
> #endif /* defined(TARGET_PPC64) */
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/4] target/ppc: Fix lqarx to set cpu_reserve
2023-06-05 2:54 [PATCH v2 1/4] target/ppc: Fix lqarx to set cpu_reserve Nicholas Piggin
` (2 preceding siblings ...)
2023-06-05 2:54 ` [PATCH v2 4/4] target/ppc: Rework store conditional to avoid branch Nicholas Piggin
@ 2023-06-05 3:09 ` Richard Henderson
2023-06-05 13:42 ` Daniel Henrique Barboza
4 siblings, 0 replies; 9+ messages in thread
From: Richard Henderson @ 2023-06-05 3:09 UTC (permalink / raw)
To: Nicholas Piggin, Daniel Henrique Barboza
Cc: qemu-ppc, qemu-devel, qemu-stable
On 6/4/23 19:54, Nicholas Piggin wrote:
> lqarx does not set cpu_reserve, which causes stqcx. to never succeed.
>
> Cc:qemu-stable@nongnu.org
> Fixes: 94bf2658676 ("target/ppc: Use atomic load for LQ and LQARX")
> Fixes: 57b38ffd0c6 ("target/ppc: Use tcg_gen_qemu_{ld,st}_i128 for LQARX, LQ, STQ")
> Signed-off-by: Nicholas Piggin<npiggin@gmail.com>
> ---
> v2:
> - Fix bugs vs memory access fault [Richard]
>
> target/ppc/translate.c | 1 +
> 1 file changed, 1 insertion(+)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/4] target/ppc: Fix lqarx to set cpu_reserve
2023-06-05 2:54 [PATCH v2 1/4] target/ppc: Fix lqarx to set cpu_reserve Nicholas Piggin
` (3 preceding siblings ...)
2023-06-05 3:09 ` [PATCH v2 1/4] target/ppc: Fix lqarx to set cpu_reserve Richard Henderson
@ 2023-06-05 13:42 ` Daniel Henrique Barboza
4 siblings, 0 replies; 9+ messages in thread
From: Daniel Henrique Barboza @ 2023-06-05 13:42 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: Richard Henderson, qemu-ppc, qemu-devel, qemu-stable
On 6/4/23 23:54, Nicholas Piggin wrote:
> lqarx does not set cpu_reserve, which causes stqcx. to never succeed.
>
> Cc: qemu-stable@nongnu.org
> Fixes: 94bf2658676 ("target/ppc: Use atomic load for LQ and LQARX")
> Fixes: 57b38ffd0c6 ("target/ppc: Use tcg_gen_qemu_{ld,st}_i128 for LQARX, LQ, STQ")
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
Queued. Thanks,
Daniel
> v2:
> - Fix bugs vs memory access fault [Richard]
>
> target/ppc/translate.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 3650d2985d..7a5bf1d820 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -3881,6 +3881,7 @@ static void gen_lqarx(DisasContext *ctx)
> tcg_gen_qemu_ld_i128(t16, EA, ctx->mem_idx, DEF_MEMOP(MO_128 | MO_ALIGN));
> tcg_gen_extr_i128_i64(lo, hi, t16);
>
> + tcg_gen_mov_tl(cpu_reserve, EA);
> tcg_gen_st_tl(hi, cpu_env, offsetof(CPUPPCState, reserve_val));
> tcg_gen_st_tl(lo, cpu_env, offsetof(CPUPPCState, reserve_val2));
> }
^ permalink raw reply [flat|nested] 9+ messages in thread