* [PATCH for-8.0 0/1] target/ppc: Use tcg_gen_atomic_cmpxchg_i128
@ 2022-11-12 6:11 Richard Henderson
2022-11-12 6:11 ` [PATCH for-8.0 1/1] target/ppc: Use tcg_gen_atomic_cmpxchg_i128 for STQCX Richard Henderson
0 siblings, 1 reply; 7+ messages in thread
From: Richard Henderson @ 2022-11-12 6:11 UTC (permalink / raw)
To: qemu-devel; +Cc: david, danielhb413, qemu-ppc
Use the new common function and avoid rolling our own helper(s).
r~
Based-on: 20221111074101.2069454-1-richard.henderson@linaro.org
("tcg: Support for Int128 with helpers")
Richard Henderson (1):
target/ppc: Use tcg_gen_atomic_cmpxchg_i128 for STQCX
target/ppc/helper.h | 2 -
target/ppc/mem_helper.c | 44 -----------------
target/ppc/translate.c | 102 ++++++++++++++++++----------------------
3 files changed, 47 insertions(+), 101 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH for-8.0 1/1] target/ppc: Use tcg_gen_atomic_cmpxchg_i128 for STQCX
2022-11-12 6:11 [PATCH for-8.0 0/1] target/ppc: Use tcg_gen_atomic_cmpxchg_i128 Richard Henderson
@ 2022-11-12 6:11 ` Richard Henderson
2022-11-16 13:38 ` Daniel Henrique Barboza
2022-11-21 22:37 ` Daniel Henrique Barboza
0 siblings, 2 replies; 7+ messages in thread
From: Richard Henderson @ 2022-11-12 6:11 UTC (permalink / raw)
To: qemu-devel; +Cc: david, danielhb413, qemu-ppc
Note that the previous direct reference to reserve_val,
- tcg_gen_ld_i64(t1, cpu_env, (ctx->le_mode
- ? offsetof(CPUPPCState, reserve_val2)
- : offsetof(CPUPPCState, reserve_val)));
was incorrect because all references should have gone through
cpu_reserve_val. Create a cpu_reserve_val2 tcg temp to fix this.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/ppc/helper.h | 2 -
target/ppc/mem_helper.c | 44 -----------------
target/ppc/translate.c | 102 ++++++++++++++++++----------------------
3 files changed, 47 insertions(+), 101 deletions(-)
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index 8dd22a35e4..0beaca5c7a 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -818,6 +818,4 @@ DEF_HELPER_FLAGS_5(stq_le_parallel, TCG_CALL_NO_WG,
void, env, tl, i64, i64, i32)
DEF_HELPER_FLAGS_5(stq_be_parallel, TCG_CALL_NO_WG,
void, env, tl, i64, i64, i32)
-DEF_HELPER_5(stqcx_le_parallel, i32, env, tl, i64, i64, i32)
-DEF_HELPER_5(stqcx_be_parallel, i32, env, tl, i64, i64, i32)
#endif
diff --git a/target/ppc/mem_helper.c b/target/ppc/mem_helper.c
index d1163f316c..1578887a8f 100644
--- a/target/ppc/mem_helper.c
+++ b/target/ppc/mem_helper.c
@@ -413,50 +413,6 @@ void helper_stq_be_parallel(CPUPPCState *env, target_ulong addr,
val = int128_make128(lo, hi);
cpu_atomic_sto_be_mmu(env, addr, val, opidx, GETPC());
}
-
-uint32_t helper_stqcx_le_parallel(CPUPPCState *env, target_ulong addr,
- uint64_t new_lo, uint64_t new_hi,
- uint32_t opidx)
-{
- bool success = false;
-
- /* We will have raised EXCP_ATOMIC from the translator. */
- assert(HAVE_CMPXCHG128);
-
- if (likely(addr == env->reserve_addr)) {
- Int128 oldv, cmpv, newv;
-
- cmpv = int128_make128(env->reserve_val2, env->reserve_val);
- newv = int128_make128(new_lo, new_hi);
- oldv = cpu_atomic_cmpxchgo_le_mmu(env, addr, cmpv, newv,
- opidx, GETPC());
- success = int128_eq(oldv, cmpv);
- }
- env->reserve_addr = -1;
- return env->so + success * CRF_EQ_BIT;
-}
-
-uint32_t helper_stqcx_be_parallel(CPUPPCState *env, target_ulong addr,
- uint64_t new_lo, uint64_t new_hi,
- uint32_t opidx)
-{
- bool success = false;
-
- /* We will have raised EXCP_ATOMIC from the translator. */
- assert(HAVE_CMPXCHG128);
-
- if (likely(addr == env->reserve_addr)) {
- Int128 oldv, cmpv, newv;
-
- cmpv = int128_make128(env->reserve_val2, env->reserve_val);
- newv = int128_make128(new_lo, new_hi);
- oldv = cpu_atomic_cmpxchgo_be_mmu(env, addr, cmpv, newv,
- opidx, GETPC());
- success = int128_eq(oldv, cmpv);
- }
- env->reserve_addr = -1;
- return env->so + success * CRF_EQ_BIT;
-}
#endif
/*****************************************************************************/
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 19c1d17cb0..85f95a9045 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -72,6 +72,7 @@ static TCGv cpu_cfar;
static TCGv cpu_xer, cpu_so, cpu_ov, cpu_ca, cpu_ov32, cpu_ca32;
static TCGv cpu_reserve;
static TCGv cpu_reserve_val;
+static TCGv cpu_reserve_val2;
static TCGv cpu_fpscr;
static TCGv_i32 cpu_access_type;
@@ -141,8 +142,11 @@ void ppc_translate_init(void)
offsetof(CPUPPCState, reserve_addr),
"reserve_addr");
cpu_reserve_val = tcg_global_mem_new(cpu_env,
- offsetof(CPUPPCState, reserve_val),
- "reserve_val");
+ offsetof(CPUPPCState, reserve_val),
+ "reserve_val");
+ cpu_reserve_val2 = tcg_global_mem_new(cpu_env,
+ offsetof(CPUPPCState, reserve_val2),
+ "reserve_val2");
cpu_fpscr = tcg_global_mem_new(cpu_env,
offsetof(CPUPPCState, fpscr), "fpscr");
@@ -3979,78 +3983,66 @@ static void gen_lqarx(DisasContext *ctx)
/* stqcx. */
static void gen_stqcx_(DisasContext *ctx)
{
+ TCGLabel *lab_fail, *lab_over;
int rs = rS(ctx->opcode);
- TCGv EA, hi, lo;
+ TCGv EA, t0, t1;
+ TCGv_i128 cmp, val;
if (unlikely(rs & 1)) {
gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
return;
}
+ lab_fail = gen_new_label();
+ lab_over = gen_new_label();
+
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_temp_free(EA);
+
+ cmp = tcg_temp_new_i128();
+ val = tcg_temp_new_i128();
+
+ tcg_gen_concat_i64_i128(cmp, cpu_reserve_val2, cpu_reserve_val);
+
/* Note that the low part is always in RS+1, even in LE mode. */
- lo = cpu_gpr[rs + 1];
- hi = cpu_gpr[rs];
+ tcg_gen_concat_i64_i128(val, cpu_gpr[rs + 1], cpu_gpr[rs]);
- if (tb_cflags(ctx->base.tb) & CF_PARALLEL) {
- if (HAVE_CMPXCHG128) {
- TCGv_i32 oi = tcg_const_i32(DEF_MEMOP(MO_128) | MO_ALIGN);
- if (ctx->le_mode) {
- gen_helper_stqcx_le_parallel(cpu_crf[0], cpu_env,
- EA, lo, hi, oi);
- } else {
- gen_helper_stqcx_be_parallel(cpu_crf[0], cpu_env,
- EA, lo, hi, oi);
- }
- tcg_temp_free_i32(oi);
- } else {
- /* Restart with exclusive lock. */
- gen_helper_exit_atomic(cpu_env);
- ctx->base.is_jmp = DISAS_NORETURN;
- }
- tcg_temp_free(EA);
- } else {
- TCGLabel *lab_fail = gen_new_label();
- TCGLabel *lab_over = gen_new_label();
- TCGv_i64 t0 = tcg_temp_new_i64();
- TCGv_i64 t1 = tcg_temp_new_i64();
+ tcg_gen_atomic_cmpxchg_i128(val, cpu_reserve, cmp, val, ctx->mem_idx,
+ DEF_MEMOP(MO_128 | MO_ALIGN));
+ tcg_temp_free_i128(cmp);
- tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lab_fail);
- tcg_temp_free(EA);
+ t0 = tcg_temp_new();
+ t1 = tcg_temp_new();
+ tcg_gen_extr_i128_i64(t1, t0, val);
+ tcg_temp_free_i128(val);
- gen_qemu_ld64_i64(ctx, t0, cpu_reserve);
- tcg_gen_ld_i64(t1, cpu_env, (ctx->le_mode
- ? offsetof(CPUPPCState, reserve_val2)
- : offsetof(CPUPPCState, reserve_val)));
- tcg_gen_brcond_i64(TCG_COND_NE, t0, t1, lab_fail);
+ tcg_gen_xor_tl(t1, t1, cpu_reserve_val2);
+ tcg_gen_xor_tl(t0, t0, cpu_reserve_val);
+ tcg_gen_or_tl(t0, t0, t1);
+ tcg_temp_free(t1);
- tcg_gen_addi_i64(t0, cpu_reserve, 8);
- gen_qemu_ld64_i64(ctx, t0, t0);
- tcg_gen_ld_i64(t1, cpu_env, (ctx->le_mode
- ? offsetof(CPUPPCState, reserve_val)
- : offsetof(CPUPPCState, reserve_val2)));
- tcg_gen_brcond_i64(TCG_COND_NE, t0, t1, lab_fail);
+ 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_temp_free(t0);
- /* Success */
- gen_qemu_st64_i64(ctx, ctx->le_mode ? lo : hi, cpu_reserve);
- tcg_gen_addi_i64(t0, cpu_reserve, 8);
- gen_qemu_st64_i64(ctx, ctx->le_mode ? hi : lo, t0);
+ tcg_gen_br(lab_over);
+ gen_set_label(lab_fail);
- tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
- tcg_gen_ori_i32(cpu_crf[0], cpu_crf[0], CRF_EQ);
- tcg_gen_br(lab_over);
+ /*
+ * 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_fail);
- tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
-
- gen_set_label(lab_over);
- tcg_gen_movi_tl(cpu_reserve, -1);
- tcg_temp_free_i64(t0);
- tcg_temp_free_i64(t1);
- }
+ gen_set_label(lab_over);
+ tcg_gen_movi_tl(cpu_reserve, -1);
}
#endif /* defined(TARGET_PPC64) */
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH for-8.0 1/1] target/ppc: Use tcg_gen_atomic_cmpxchg_i128 for STQCX
2022-11-12 6:11 ` [PATCH for-8.0 1/1] target/ppc: Use tcg_gen_atomic_cmpxchg_i128 for STQCX Richard Henderson
@ 2022-11-16 13:38 ` Daniel Henrique Barboza
2022-11-17 2:48 ` Richard Henderson
2022-11-21 22:37 ` Daniel Henrique Barboza
1 sibling, 1 reply; 7+ messages in thread
From: Daniel Henrique Barboza @ 2022-11-16 13:38 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: david, qemu-ppc
Richard,
I believe the ppc64-linux-user target didn't like what you did in this
patch. Here's the error:
$ ../configure --target-list=ppc64-softmmu,ppc64-linux-user,ppc-softmmu,ppc-linux-user,ppc64le-linux-user
$ make -j
(...)
[15/133] Compiling C object libqemu-ppc64-linux-user.fa.p/target_ppc_translate.c.o
FAILED: libqemu-ppc64-linux-user.fa.p/target_ppc_translate.c.o
cc -m64 -mcx16 -Ilibqemu-ppc64-linux-user.fa.p -I. -I.. -Itarget/ppc -I../target/ppc -I../common-user/host/x86_64 -I../linux-user/include/host/x86_64 -I../linux-user/include -Ilinux-user -I../linux-user -Ilinux-user/ppc -I../linux-user/ppc -Iqapi -Itrace -Iui -Iui/shader -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem /home/danielhb/kvm-project/qemu/linux-headers -isystem linux-headers -iquote . -iquote /home/danielhb/kvm-project/qemu -iquote /home/danielhb/kvm-project/qemu/include -iquote /home/danielhb/kvm-project/qemu/tcg/i386 -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE -isystem../linux-headers -isystemlinux-headers -DNEED_CPU_H '-DCONFIG_TARGET="ppc64-linux-user-config-target.h"' '-DCONFIG_DEVICES="ppc64-linux-user-config-devices.h"' -MD -MQ libqemu-ppc64-linux-user.fa.p/target_ppc_translate.c.o -MF libqemu-ppc64-linux-user.fa.p/target_ppc_translate.c.o.d -o libqemu-ppc64-linux-user.fa.p/target_ppc_translate.c.o -c ../target/ppc/translate.c
../target/ppc/translate.c: In function ‘gen_stqcx_’:
../target/ppc/translate.c:3989:5: error: unknown type name ‘TCGv_i128’; did you mean ‘TCGv_i32’?
3989 | TCGv_i128 cmp, val;
| ^~~~~~~~~
| TCGv_i32
../target/ppc/translate.c:4006:11: error: implicit declaration of function ‘tcg_temp_new_i128’; did you mean ‘tcg_temp_new_i32’? [-Werror=implicit-function-declaration]
4006 | cmp = tcg_temp_new_i128();
| ^~~~~~~~~~~~~~~~~
| tcg_temp_new_i32
../target/ppc/translate.c:4006:11: error: nested extern declaration of ‘tcg_temp_new_i128’ [-Werror=nested-externs]
../target/ppc/translate.c:4009:5: error: implicit declaration of function ‘tcg_gen_concat_i64_i128’; did you mean ‘tcg_gen_concat_i32_i64’? [-Werror=implicit-function-declaration]
4009 | tcg_gen_concat_i64_i128(cmp, cpu_reserve_val2, cpu_reserve_val);
| ^~~~~~~~~~~~~~~~~~~~~~~
| tcg_gen_concat_i32_i64
../target/ppc/translate.c:4009:5: error: nested extern declaration of ‘tcg_gen_concat_i64_i128’ [-Werror=nested-externs]
../target/ppc/translate.c:4014:5: error: implicit declaration of function ‘tcg_gen_atomic_cmpxchg_i128’; did you mean ‘tcg_gen_atomic_cmpxchg_i32’? [-Werror=implicit-function-declaration]
4014 | tcg_gen_atomic_cmpxchg_i128(val, cpu_reserve, cmp, val, ctx->mem_idx,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
| tcg_gen_atomic_cmpxchg_i32
../target/ppc/translate.c:4014:5: error: nested extern declaration of ‘tcg_gen_atomic_cmpxchg_i128’ [-Werror=nested-externs]
../target/ppc/translate.c:4016:5: error: implicit declaration of function ‘tcg_temp_free_i128’; did you mean ‘tcg_temp_free_i32’? [-Werror=implicit-function-declaration]
4016 | tcg_temp_free_i128(cmp);
| ^~~~~~~~~~~~~~~~~~
| tcg_temp_free_i32
../target/ppc/translate.c:4016:5: error: nested extern declaration of ‘tcg_temp_free_i128’ [-Werror=nested-externs]
../target/ppc/translate.c:4020:5: error: implicit declaration of function ‘tcg_gen_extr_i128_i64’; did you mean ‘tcg_gen_ext_i32_i64’? [-Werror=implicit-function-declaration]
4020 | tcg_gen_extr_i128_i64(t1, t0, val);
| ^~~~~~~~~~~~~~~~~~~~~
| tcg_gen_ext_i32_i64
../target/ppc/translate.c:4020:5: error: nested extern declaration of ‘tcg_gen_extr_i128_i64’ [-Werror=nested-externs]
cc1: all warnings being treated as errors
[16/133] Compiling C object libqemu-ppc64-softmmu.fa.p/target_ppc_mmu_helper.c.o
[17/133] Compiling C object libqemu-ppc64-softmmu.fa.p/target_ppc_translate.c.o
FAILED: libqemu-ppc64-softmmu.fa.p/target_ppc_translate.c.o
Thanks,
Daniel
On 11/12/22 03:11, Richard Henderson wrote:
> Note that the previous direct reference to reserve_val,
>
> - tcg_gen_ld_i64(t1, cpu_env, (ctx->le_mode
> - ? offsetof(CPUPPCState, reserve_val2)
> - : offsetof(CPUPPCState, reserve_val)));
>
> was incorrect because all references should have gone through
> cpu_reserve_val. Create a cpu_reserve_val2 tcg temp to fix this.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/ppc/helper.h | 2 -
> target/ppc/mem_helper.c | 44 -----------------
> target/ppc/translate.c | 102 ++++++++++++++++++----------------------
> 3 files changed, 47 insertions(+), 101 deletions(-)
>
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index 8dd22a35e4..0beaca5c7a 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -818,6 +818,4 @@ DEF_HELPER_FLAGS_5(stq_le_parallel, TCG_CALL_NO_WG,
> void, env, tl, i64, i64, i32)
> DEF_HELPER_FLAGS_5(stq_be_parallel, TCG_CALL_NO_WG,
> void, env, tl, i64, i64, i32)
> -DEF_HELPER_5(stqcx_le_parallel, i32, env, tl, i64, i64, i32)
> -DEF_HELPER_5(stqcx_be_parallel, i32, env, tl, i64, i64, i32)
> #endif
> diff --git a/target/ppc/mem_helper.c b/target/ppc/mem_helper.c
> index d1163f316c..1578887a8f 100644
> --- a/target/ppc/mem_helper.c
> +++ b/target/ppc/mem_helper.c
> @@ -413,50 +413,6 @@ void helper_stq_be_parallel(CPUPPCState *env, target_ulong addr,
> val = int128_make128(lo, hi);
> cpu_atomic_sto_be_mmu(env, addr, val, opidx, GETPC());
> }
> -
> -uint32_t helper_stqcx_le_parallel(CPUPPCState *env, target_ulong addr,
> - uint64_t new_lo, uint64_t new_hi,
> - uint32_t opidx)
> -{
> - bool success = false;
> -
> - /* We will have raised EXCP_ATOMIC from the translator. */
> - assert(HAVE_CMPXCHG128);
> -
> - if (likely(addr == env->reserve_addr)) {
> - Int128 oldv, cmpv, newv;
> -
> - cmpv = int128_make128(env->reserve_val2, env->reserve_val);
> - newv = int128_make128(new_lo, new_hi);
> - oldv = cpu_atomic_cmpxchgo_le_mmu(env, addr, cmpv, newv,
> - opidx, GETPC());
> - success = int128_eq(oldv, cmpv);
> - }
> - env->reserve_addr = -1;
> - return env->so + success * CRF_EQ_BIT;
> -}
> -
> -uint32_t helper_stqcx_be_parallel(CPUPPCState *env, target_ulong addr,
> - uint64_t new_lo, uint64_t new_hi,
> - uint32_t opidx)
> -{
> - bool success = false;
> -
> - /* We will have raised EXCP_ATOMIC from the translator. */
> - assert(HAVE_CMPXCHG128);
> -
> - if (likely(addr == env->reserve_addr)) {
> - Int128 oldv, cmpv, newv;
> -
> - cmpv = int128_make128(env->reserve_val2, env->reserve_val);
> - newv = int128_make128(new_lo, new_hi);
> - oldv = cpu_atomic_cmpxchgo_be_mmu(env, addr, cmpv, newv,
> - opidx, GETPC());
> - success = int128_eq(oldv, cmpv);
> - }
> - env->reserve_addr = -1;
> - return env->so + success * CRF_EQ_BIT;
> -}
> #endif
>
> /*****************************************************************************/
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 19c1d17cb0..85f95a9045 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -72,6 +72,7 @@ static TCGv cpu_cfar;
> static TCGv cpu_xer, cpu_so, cpu_ov, cpu_ca, cpu_ov32, cpu_ca32;
> static TCGv cpu_reserve;
> static TCGv cpu_reserve_val;
> +static TCGv cpu_reserve_val2;
> static TCGv cpu_fpscr;
> static TCGv_i32 cpu_access_type;
>
> @@ -141,8 +142,11 @@ void ppc_translate_init(void)
> offsetof(CPUPPCState, reserve_addr),
> "reserve_addr");
> cpu_reserve_val = tcg_global_mem_new(cpu_env,
> - offsetof(CPUPPCState, reserve_val),
> - "reserve_val");
> + offsetof(CPUPPCState, reserve_val),
> + "reserve_val");
> + cpu_reserve_val2 = tcg_global_mem_new(cpu_env,
> + offsetof(CPUPPCState, reserve_val2),
> + "reserve_val2");
>
> cpu_fpscr = tcg_global_mem_new(cpu_env,
> offsetof(CPUPPCState, fpscr), "fpscr");
> @@ -3979,78 +3983,66 @@ static void gen_lqarx(DisasContext *ctx)
> /* stqcx. */
> static void gen_stqcx_(DisasContext *ctx)
> {
> + TCGLabel *lab_fail, *lab_over;
> int rs = rS(ctx->opcode);
> - TCGv EA, hi, lo;
> + TCGv EA, t0, t1;
> + TCGv_i128 cmp, val;
>
> if (unlikely(rs & 1)) {
> gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
> return;
> }
>
> + lab_fail = gen_new_label();
> + lab_over = gen_new_label();
> +
> 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_temp_free(EA);
> +
> + cmp = tcg_temp_new_i128();
> + val = tcg_temp_new_i128();
> +
> + tcg_gen_concat_i64_i128(cmp, cpu_reserve_val2, cpu_reserve_val);
> +
> /* Note that the low part is always in RS+1, even in LE mode. */
> - lo = cpu_gpr[rs + 1];
> - hi = cpu_gpr[rs];
> + tcg_gen_concat_i64_i128(val, cpu_gpr[rs + 1], cpu_gpr[rs]);
>
> - if (tb_cflags(ctx->base.tb) & CF_PARALLEL) {
> - if (HAVE_CMPXCHG128) {
> - TCGv_i32 oi = tcg_const_i32(DEF_MEMOP(MO_128) | MO_ALIGN);
> - if (ctx->le_mode) {
> - gen_helper_stqcx_le_parallel(cpu_crf[0], cpu_env,
> - EA, lo, hi, oi);
> - } else {
> - gen_helper_stqcx_be_parallel(cpu_crf[0], cpu_env,
> - EA, lo, hi, oi);
> - }
> - tcg_temp_free_i32(oi);
> - } else {
> - /* Restart with exclusive lock. */
> - gen_helper_exit_atomic(cpu_env);
> - ctx->base.is_jmp = DISAS_NORETURN;
> - }
> - tcg_temp_free(EA);
> - } else {
> - TCGLabel *lab_fail = gen_new_label();
> - TCGLabel *lab_over = gen_new_label();
> - TCGv_i64 t0 = tcg_temp_new_i64();
> - TCGv_i64 t1 = tcg_temp_new_i64();
> + tcg_gen_atomic_cmpxchg_i128(val, cpu_reserve, cmp, val, ctx->mem_idx,
> + DEF_MEMOP(MO_128 | MO_ALIGN));
> + tcg_temp_free_i128(cmp);
>
> - tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lab_fail);
> - tcg_temp_free(EA);
> + t0 = tcg_temp_new();
> + t1 = tcg_temp_new();
> + tcg_gen_extr_i128_i64(t1, t0, val);
> + tcg_temp_free_i128(val);
>
> - gen_qemu_ld64_i64(ctx, t0, cpu_reserve);
> - tcg_gen_ld_i64(t1, cpu_env, (ctx->le_mode
> - ? offsetof(CPUPPCState, reserve_val2)
> - : offsetof(CPUPPCState, reserve_val)));
> - tcg_gen_brcond_i64(TCG_COND_NE, t0, t1, lab_fail);
> + tcg_gen_xor_tl(t1, t1, cpu_reserve_val2);
> + tcg_gen_xor_tl(t0, t0, cpu_reserve_val);
> + tcg_gen_or_tl(t0, t0, t1);
> + tcg_temp_free(t1);
>
> - tcg_gen_addi_i64(t0, cpu_reserve, 8);
> - gen_qemu_ld64_i64(ctx, t0, t0);
> - tcg_gen_ld_i64(t1, cpu_env, (ctx->le_mode
> - ? offsetof(CPUPPCState, reserve_val)
> - : offsetof(CPUPPCState, reserve_val2)));
> - tcg_gen_brcond_i64(TCG_COND_NE, t0, t1, lab_fail);
> + 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_temp_free(t0);
>
> - /* Success */
> - gen_qemu_st64_i64(ctx, ctx->le_mode ? lo : hi, cpu_reserve);
> - tcg_gen_addi_i64(t0, cpu_reserve, 8);
> - gen_qemu_st64_i64(ctx, ctx->le_mode ? hi : lo, t0);
> + tcg_gen_br(lab_over);
> + gen_set_label(lab_fail);
>
> - tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
> - tcg_gen_ori_i32(cpu_crf[0], cpu_crf[0], CRF_EQ);
> - tcg_gen_br(lab_over);
> + /*
> + * 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_fail);
> - tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
> -
> - gen_set_label(lab_over);
> - tcg_gen_movi_tl(cpu_reserve, -1);
> - tcg_temp_free_i64(t0);
> - tcg_temp_free_i64(t1);
> - }
> + gen_set_label(lab_over);
> + tcg_gen_movi_tl(cpu_reserve, -1);
> }
> #endif /* defined(TARGET_PPC64) */
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH for-8.0 1/1] target/ppc: Use tcg_gen_atomic_cmpxchg_i128 for STQCX
2022-11-16 13:38 ` Daniel Henrique Barboza
@ 2022-11-17 2:48 ` Richard Henderson
2022-11-17 10:03 ` Daniel Henrique Barboza
0 siblings, 1 reply; 7+ messages in thread
From: Richard Henderson @ 2022-11-17 2:48 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: qemu-devel@nongnu.org Developers, David Gibson, open list:PowerPC
[-- Attachment #1: Type: text/plain, Size: 14934 bytes --]
I think you missed the Based-on tag.
r~
On Wed, 16 Nov 2022, 05:38 Daniel Henrique Barboza, <danielhb413@gmail.com>
wrote:
> Richard,
>
> I believe the ppc64-linux-user target didn't like what you did in this
> patch. Here's the error:
>
> $ ../configure
> --target-list=ppc64-softmmu,ppc64-linux-user,ppc-softmmu,ppc-linux-user,ppc64le-linux-user
> $ make -j
>
> (...)
>
> [15/133] Compiling C object
> libqemu-ppc64-linux-user.fa.p/target_ppc_translate.c.o
> FAILED: libqemu-ppc64-linux-user.fa.p/target_ppc_translate.c.o
> cc -m64 -mcx16 -Ilibqemu-ppc64-linux-user.fa.p -I. -I.. -Itarget/ppc
> -I../target/ppc -I../common-user/host/x86_64
> -I../linux-user/include/host/x86_64 -I../linux-user/include -Ilinux-user
> -I../linux-user -Ilinux-user/ppc -I../linux-user/ppc -Iqapi -Itrace -Iui
> -Iui/shader -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include
> -I/usr/include/sysprof-4 -fdiagnostics-color=auto -Wall -Winvalid-pch
> -Werror -std=gnu11 -O2 -g -isystem
> /home/danielhb/kvm-project/qemu/linux-headers -isystem linux-headers
> -iquote . -iquote /home/danielhb/kvm-project/qemu -iquote
> /home/danielhb/kvm-project/qemu/include -iquote
> /home/danielhb/kvm-project/qemu/tcg/i386 -pthread -U_FORTIFY_SOURCE
> -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64
> -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wundef
> -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common
> -fwrapv -Wold-style-declaration -Wold-style-definition -Wtype-limits
> -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers
> -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined
> -Wimplicit-fallthrough=2 -Wno-missing-include-dirs
> -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE
> -isystem../linux-headers -isystemlinux-headers -DNEED_CPU_H
> '-DCONFIG_TARGET="ppc64-linux-user-config-target.h"'
> '-DCONFIG_DEVICES="ppc64-linux-user-config-devices.h"' -MD -MQ
> libqemu-ppc64-linux-user.fa.p/target_ppc_translate.c.o -MF
> libqemu-ppc64-linux-user.fa.p/target_ppc_translate.c.o.d -o
> libqemu-ppc64-linux-user.fa.p/target_ppc_translate.c.o -c
> ../target/ppc/translate.c
> ../target/ppc/translate.c: In function ‘gen_stqcx_’:
> ../target/ppc/translate.c:3989:5: error: unknown type name ‘TCGv_i128’;
> did you mean ‘TCGv_i32’?
> 3989 | TCGv_i128 cmp, val;
> | ^~~~~~~~~
> | TCGv_i32
> ../target/ppc/translate.c:4006:11: error: implicit declaration of function
> ‘tcg_temp_new_i128’; did you mean ‘tcg_temp_new_i32’?
> [-Werror=implicit-function-declaration]
> 4006 | cmp = tcg_temp_new_i128();
> | ^~~~~~~~~~~~~~~~~
> | tcg_temp_new_i32
> ../target/ppc/translate.c:4006:11: error: nested extern declaration of
> ‘tcg_temp_new_i128’ [-Werror=nested-externs]
> ../target/ppc/translate.c:4009:5: error: implicit declaration of function
> ‘tcg_gen_concat_i64_i128’; did you mean ‘tcg_gen_concat_i32_i64’?
> [-Werror=implicit-function-declaration]
> 4009 | tcg_gen_concat_i64_i128(cmp, cpu_reserve_val2,
> cpu_reserve_val);
> | ^~~~~~~~~~~~~~~~~~~~~~~
> | tcg_gen_concat_i32_i64
> ../target/ppc/translate.c:4009:5: error: nested extern declaration of
> ‘tcg_gen_concat_i64_i128’ [-Werror=nested-externs]
> ../target/ppc/translate.c:4014:5: error: implicit declaration of function
> ‘tcg_gen_atomic_cmpxchg_i128’; did you mean ‘tcg_gen_atomic_cmpxchg_i32’?
> [-Werror=implicit-function-declaration]
> 4014 | tcg_gen_atomic_cmpxchg_i128(val, cpu_reserve, cmp, val,
> ctx->mem_idx,
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> | tcg_gen_atomic_cmpxchg_i32
> ../target/ppc/translate.c:4014:5: error: nested extern declaration of
> ‘tcg_gen_atomic_cmpxchg_i128’ [-Werror=nested-externs]
> ../target/ppc/translate.c:4016:5: error: implicit declaration of function
> ‘tcg_temp_free_i128’; did you mean ‘tcg_temp_free_i32’?
> [-Werror=implicit-function-declaration]
> 4016 | tcg_temp_free_i128(cmp);
> | ^~~~~~~~~~~~~~~~~~
> | tcg_temp_free_i32
> ../target/ppc/translate.c:4016:5: error: nested extern declaration of
> ‘tcg_temp_free_i128’ [-Werror=nested-externs]
> ../target/ppc/translate.c:4020:5: error: implicit declaration of function
> ‘tcg_gen_extr_i128_i64’; did you mean ‘tcg_gen_ext_i32_i64’?
> [-Werror=implicit-function-declaration]
> 4020 | tcg_gen_extr_i128_i64(t1, t0, val);
> | ^~~~~~~~~~~~~~~~~~~~~
> | tcg_gen_ext_i32_i64
> ../target/ppc/translate.c:4020:5: error: nested extern declaration of
> ‘tcg_gen_extr_i128_i64’ [-Werror=nested-externs]
> cc1: all warnings being treated as errors
> [16/133] Compiling C object
> libqemu-ppc64-softmmu.fa.p/target_ppc_mmu_helper.c.o
> [17/133] Compiling C object
> libqemu-ppc64-softmmu.fa.p/target_ppc_translate.c.o
> FAILED: libqemu-ppc64-softmmu.fa.p/target_ppc_translate.c.o
>
>
> Thanks,
>
>
> Daniel
>
>
> On 11/12/22 03:11, Richard Henderson wrote:
> > Note that the previous direct reference to reserve_val,
> >
> > - tcg_gen_ld_i64(t1, cpu_env, (ctx->le_mode
> > - ? offsetof(CPUPPCState, reserve_val2)
> > - : offsetof(CPUPPCState, reserve_val)));
> >
> > was incorrect because all references should have gone through
> > cpu_reserve_val. Create a cpu_reserve_val2 tcg temp to fix this.
> >
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> > target/ppc/helper.h | 2 -
> > target/ppc/mem_helper.c | 44 -----------------
> > target/ppc/translate.c | 102 ++++++++++++++++++----------------------
> > 3 files changed, 47 insertions(+), 101 deletions(-)
> >
> > diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> > index 8dd22a35e4..0beaca5c7a 100644
> > --- a/target/ppc/helper.h
> > +++ b/target/ppc/helper.h
> > @@ -818,6 +818,4 @@ DEF_HELPER_FLAGS_5(stq_le_parallel, TCG_CALL_NO_WG,
> > void, env, tl, i64, i64, i32)
> > DEF_HELPER_FLAGS_5(stq_be_parallel, TCG_CALL_NO_WG,
> > void, env, tl, i64, i64, i32)
> > -DEF_HELPER_5(stqcx_le_parallel, i32, env, tl, i64, i64, i32)
> > -DEF_HELPER_5(stqcx_be_parallel, i32, env, tl, i64, i64, i32)
> > #endif
> > diff --git a/target/ppc/mem_helper.c b/target/ppc/mem_helper.c
> > index d1163f316c..1578887a8f 100644
> > --- a/target/ppc/mem_helper.c
> > +++ b/target/ppc/mem_helper.c
> > @@ -413,50 +413,6 @@ void helper_stq_be_parallel(CPUPPCState *env,
> target_ulong addr,
> > val = int128_make128(lo, hi);
> > cpu_atomic_sto_be_mmu(env, addr, val, opidx, GETPC());
> > }
> > -
> > -uint32_t helper_stqcx_le_parallel(CPUPPCState *env, target_ulong addr,
> > - uint64_t new_lo, uint64_t new_hi,
> > - uint32_t opidx)
> > -{
> > - bool success = false;
> > -
> > - /* We will have raised EXCP_ATOMIC from the translator. */
> > - assert(HAVE_CMPXCHG128);
> > -
> > - if (likely(addr == env->reserve_addr)) {
> > - Int128 oldv, cmpv, newv;
> > -
> > - cmpv = int128_make128(env->reserve_val2, env->reserve_val);
> > - newv = int128_make128(new_lo, new_hi);
> > - oldv = cpu_atomic_cmpxchgo_le_mmu(env, addr, cmpv, newv,
> > - opidx, GETPC());
> > - success = int128_eq(oldv, cmpv);
> > - }
> > - env->reserve_addr = -1;
> > - return env->so + success * CRF_EQ_BIT;
> > -}
> > -
> > -uint32_t helper_stqcx_be_parallel(CPUPPCState *env, target_ulong addr,
> > - uint64_t new_lo, uint64_t new_hi,
> > - uint32_t opidx)
> > -{
> > - bool success = false;
> > -
> > - /* We will have raised EXCP_ATOMIC from the translator. */
> > - assert(HAVE_CMPXCHG128);
> > -
> > - if (likely(addr == env->reserve_addr)) {
> > - Int128 oldv, cmpv, newv;
> > -
> > - cmpv = int128_make128(env->reserve_val2, env->reserve_val);
> > - newv = int128_make128(new_lo, new_hi);
> > - oldv = cpu_atomic_cmpxchgo_be_mmu(env, addr, cmpv, newv,
> > - opidx, GETPC());
> > - success = int128_eq(oldv, cmpv);
> > - }
> > - env->reserve_addr = -1;
> > - return env->so + success * CRF_EQ_BIT;
> > -}
> > #endif
> >
> >
> /*****************************************************************************/
> > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > index 19c1d17cb0..85f95a9045 100644
> > --- a/target/ppc/translate.c
> > +++ b/target/ppc/translate.c
> > @@ -72,6 +72,7 @@ static TCGv cpu_cfar;
> > static TCGv cpu_xer, cpu_so, cpu_ov, cpu_ca, cpu_ov32, cpu_ca32;
> > static TCGv cpu_reserve;
> > static TCGv cpu_reserve_val;
> > +static TCGv cpu_reserve_val2;
> > static TCGv cpu_fpscr;
> > static TCGv_i32 cpu_access_type;
> >
> > @@ -141,8 +142,11 @@ void ppc_translate_init(void)
> > offsetof(CPUPPCState,
> reserve_addr),
> > "reserve_addr");
> > cpu_reserve_val = tcg_global_mem_new(cpu_env,
> > - offsetof(CPUPPCState, reserve_val),
> > - "reserve_val");
> > + offsetof(CPUPPCState,
> reserve_val),
> > + "reserve_val");
> > + cpu_reserve_val2 = tcg_global_mem_new(cpu_env,
> > + offsetof(CPUPPCState,
> reserve_val2),
> > + "reserve_val2");
> >
> > cpu_fpscr = tcg_global_mem_new(cpu_env,
> > offsetof(CPUPPCState, fpscr),
> "fpscr");
> > @@ -3979,78 +3983,66 @@ static void gen_lqarx(DisasContext *ctx)
> > /* stqcx. */
> > static void gen_stqcx_(DisasContext *ctx)
> > {
> > + TCGLabel *lab_fail, *lab_over;
> > int rs = rS(ctx->opcode);
> > - TCGv EA, hi, lo;
> > + TCGv EA, t0, t1;
> > + TCGv_i128 cmp, val;
> >
> > if (unlikely(rs & 1)) {
> > gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
> > return;
> > }
> >
> > + lab_fail = gen_new_label();
> > + lab_over = gen_new_label();
> > +
> > 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_temp_free(EA);
> > +
> > + cmp = tcg_temp_new_i128();
> > + val = tcg_temp_new_i128();
> > +
> > + tcg_gen_concat_i64_i128(cmp, cpu_reserve_val2, cpu_reserve_val);
> > +
> > /* Note that the low part is always in RS+1, even in LE mode. */
> > - lo = cpu_gpr[rs + 1];
> > - hi = cpu_gpr[rs];
> > + tcg_gen_concat_i64_i128(val, cpu_gpr[rs + 1], cpu_gpr[rs]);
> >
> > - if (tb_cflags(ctx->base.tb) & CF_PARALLEL) {
> > - if (HAVE_CMPXCHG128) {
> > - TCGv_i32 oi = tcg_const_i32(DEF_MEMOP(MO_128) | MO_ALIGN);
> > - if (ctx->le_mode) {
> > - gen_helper_stqcx_le_parallel(cpu_crf[0], cpu_env,
> > - EA, lo, hi, oi);
> > - } else {
> > - gen_helper_stqcx_be_parallel(cpu_crf[0], cpu_env,
> > - EA, lo, hi, oi);
> > - }
> > - tcg_temp_free_i32(oi);
> > - } else {
> > - /* Restart with exclusive lock. */
> > - gen_helper_exit_atomic(cpu_env);
> > - ctx->base.is_jmp = DISAS_NORETURN;
> > - }
> > - tcg_temp_free(EA);
> > - } else {
> > - TCGLabel *lab_fail = gen_new_label();
> > - TCGLabel *lab_over = gen_new_label();
> > - TCGv_i64 t0 = tcg_temp_new_i64();
> > - TCGv_i64 t1 = tcg_temp_new_i64();
> > + tcg_gen_atomic_cmpxchg_i128(val, cpu_reserve, cmp, val,
> ctx->mem_idx,
> > + DEF_MEMOP(MO_128 | MO_ALIGN));
> > + tcg_temp_free_i128(cmp);
> >
> > - tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lab_fail);
> > - tcg_temp_free(EA);
> > + t0 = tcg_temp_new();
> > + t1 = tcg_temp_new();
> > + tcg_gen_extr_i128_i64(t1, t0, val);
> > + tcg_temp_free_i128(val);
> >
> > - gen_qemu_ld64_i64(ctx, t0, cpu_reserve);
> > - tcg_gen_ld_i64(t1, cpu_env, (ctx->le_mode
> > - ? offsetof(CPUPPCState,
> reserve_val2)
> > - : offsetof(CPUPPCState,
> reserve_val)));
> > - tcg_gen_brcond_i64(TCG_COND_NE, t0, t1, lab_fail);
> > + tcg_gen_xor_tl(t1, t1, cpu_reserve_val2);
> > + tcg_gen_xor_tl(t0, t0, cpu_reserve_val);
> > + tcg_gen_or_tl(t0, t0, t1);
> > + tcg_temp_free(t1);
> >
> > - tcg_gen_addi_i64(t0, cpu_reserve, 8);
> > - gen_qemu_ld64_i64(ctx, t0, t0);
> > - tcg_gen_ld_i64(t1, cpu_env, (ctx->le_mode
> > - ? offsetof(CPUPPCState,
> reserve_val)
> > - : offsetof(CPUPPCState,
> reserve_val2)));
> > - tcg_gen_brcond_i64(TCG_COND_NE, t0, t1, lab_fail);
> > + 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_temp_free(t0);
> >
> > - /* Success */
> > - gen_qemu_st64_i64(ctx, ctx->le_mode ? lo : hi, cpu_reserve);
> > - tcg_gen_addi_i64(t0, cpu_reserve, 8);
> > - gen_qemu_st64_i64(ctx, ctx->le_mode ? hi : lo, t0);
> > + tcg_gen_br(lab_over);
> > + gen_set_label(lab_fail);
> >
> > - tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
> > - tcg_gen_ori_i32(cpu_crf[0], cpu_crf[0], CRF_EQ);
> > - tcg_gen_br(lab_over);
> > + /*
> > + * 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_fail);
> > - tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
> > -
> > - gen_set_label(lab_over);
> > - tcg_gen_movi_tl(cpu_reserve, -1);
> > - tcg_temp_free_i64(t0);
> > - tcg_temp_free_i64(t1);
> > - }
> > + gen_set_label(lab_over);
> > + tcg_gen_movi_tl(cpu_reserve, -1);
> > }
> > #endif /* defined(TARGET_PPC64) */
> >
>
[-- Attachment #2: Type: text/html, Size: 17701 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH for-8.0 1/1] target/ppc: Use tcg_gen_atomic_cmpxchg_i128 for STQCX
2022-11-17 2:48 ` Richard Henderson
@ 2022-11-17 10:03 ` Daniel Henrique Barboza
0 siblings, 0 replies; 7+ messages in thread
From: Daniel Henrique Barboza @ 2022-11-17 10:03 UTC (permalink / raw)
To: Richard Henderson
Cc: qemu-devel@nongnu.org Developers, David Gibson, open list:PowerPC
On 11/16/22 23:48, Richard Henderson wrote:
> I think you missed the Based-on tag.
>
> r~
Duh. Sorry about that. Let me try it again.
Daniel
>
> On Wed, 16 Nov 2022, 05:38 Daniel Henrique Barboza, <danielhb413@gmail.com <mailto:danielhb413@gmail.com>> wrote:
>
> Richard,
>
> I believe the ppc64-linux-user target didn't like what you did in this
> patch. Here's the error:
>
> $ ../configure --target-list=ppc64-softmmu,ppc64-linux-user,ppc-softmmu,ppc-linux-user,ppc64le-linux-user
> $ make -j
>
> (...)
>
> [15/133] Compiling C object libqemu-ppc64-linux-user.fa.p/target_ppc_translate.c.o
> FAILED: libqemu-ppc64-linux-user.fa.p/target_ppc_translate.c.o
> cc -m64 -mcx16 -Ilibqemu-ppc64-linux-user.fa.p -I. -I.. -Itarget/ppc -I../target/ppc -I../common-user/host/x86_64 -I../linux-user/include/host/x86_64 -I../linux-user/include -Ilinux-user -I../linux-user -Ilinux-user/ppc -I../linux-user/ppc -Iqapi -Itrace -Iui -Iui/shader -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem /home/danielhb/kvm-project/qemu/linux-headers -isystem linux-headers -iquote . -iquote /home/danielhb/kvm-project/qemu -iquote /home/danielhb/kvm-project/qemu/include -iquote /home/danielhb/kvm-project/qemu/tcg/i386 -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self
> -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE -isystem../linux-headers -isystemlinux-headers -DNEED_CPU_H '-DCONFIG_TARGET="ppc64-linux-user-config-target.h"' '-DCONFIG_DEVICES="ppc64-linux-user-config-devices.h"' -MD -MQ libqemu-ppc64-linux-user.fa.p/target_ppc_translate.c.o -MF libqemu-ppc64-linux-user.fa.p/target_ppc_translate.c.o.d -o libqemu-ppc64-linux-user.fa.p/target_ppc_translate.c.o -c ../target/ppc/translate.c
> ../target/ppc/translate.c: In function ‘gen_stqcx_’:
> ../target/ppc/translate.c:3989:5: error: unknown type name ‘TCGv_i128’; did you mean ‘TCGv_i32’?
> 3989 | TCGv_i128 cmp, val;
> | ^~~~~~~~~
> | TCGv_i32
> ../target/ppc/translate.c:4006:11: error: implicit declaration of function ‘tcg_temp_new_i128’; did you mean ‘tcg_temp_new_i32’? [-Werror=implicit-function-declaration]
> 4006 | cmp = tcg_temp_new_i128();
> | ^~~~~~~~~~~~~~~~~
> | tcg_temp_new_i32
> ../target/ppc/translate.c:4006:11: error: nested extern declaration of ‘tcg_temp_new_i128’ [-Werror=nested-externs]
> ../target/ppc/translate.c:4009:5: error: implicit declaration of function ‘tcg_gen_concat_i64_i128’; did you mean ‘tcg_gen_concat_i32_i64’? [-Werror=implicit-function-declaration]
> 4009 | tcg_gen_concat_i64_i128(cmp, cpu_reserve_val2, cpu_reserve_val);
> | ^~~~~~~~~~~~~~~~~~~~~~~
> | tcg_gen_concat_i32_i64
> ../target/ppc/translate.c:4009:5: error: nested extern declaration of ‘tcg_gen_concat_i64_i128’ [-Werror=nested-externs]
> ../target/ppc/translate.c:4014:5: error: implicit declaration of function ‘tcg_gen_atomic_cmpxchg_i128’; did you mean ‘tcg_gen_atomic_cmpxchg_i32’? [-Werror=implicit-function-declaration]
> 4014 | tcg_gen_atomic_cmpxchg_i128(val, cpu_reserve, cmp, val, ctx->mem_idx,
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> | tcg_gen_atomic_cmpxchg_i32
> ../target/ppc/translate.c:4014:5: error: nested extern declaration of ‘tcg_gen_atomic_cmpxchg_i128’ [-Werror=nested-externs]
> ../target/ppc/translate.c:4016:5: error: implicit declaration of function ‘tcg_temp_free_i128’; did you mean ‘tcg_temp_free_i32’? [-Werror=implicit-function-declaration]
> 4016 | tcg_temp_free_i128(cmp);
> | ^~~~~~~~~~~~~~~~~~
> | tcg_temp_free_i32
> ../target/ppc/translate.c:4016:5: error: nested extern declaration of ‘tcg_temp_free_i128’ [-Werror=nested-externs]
> ../target/ppc/translate.c:4020:5: error: implicit declaration of function ‘tcg_gen_extr_i128_i64’; did you mean ‘tcg_gen_ext_i32_i64’? [-Werror=implicit-function-declaration]
> 4020 | tcg_gen_extr_i128_i64(t1, t0, val);
> | ^~~~~~~~~~~~~~~~~~~~~
> | tcg_gen_ext_i32_i64
> ../target/ppc/translate.c:4020:5: error: nested extern declaration of ‘tcg_gen_extr_i128_i64’ [-Werror=nested-externs]
> cc1: all warnings being treated as errors
> [16/133] Compiling C object libqemu-ppc64-softmmu.fa.p/target_ppc_mmu_helper.c.o
> [17/133] Compiling C object libqemu-ppc64-softmmu.fa.p/target_ppc_translate.c.o
> FAILED: libqemu-ppc64-softmmu.fa.p/target_ppc_translate.c.o
>
>
> Thanks,
>
>
> Daniel
>
>
> On 11/12/22 03:11, Richard Henderson wrote:
> > Note that the previous direct reference to reserve_val,
> >
> > - tcg_gen_ld_i64(t1, cpu_env, (ctx->le_mode
> > - ? offsetof(CPUPPCState, reserve_val2)
> > - : offsetof(CPUPPCState, reserve_val)));
> >
> > was incorrect because all references should have gone through
> > cpu_reserve_val. Create a cpu_reserve_val2 tcg temp to fix this.
> >
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org <mailto:richard.henderson@linaro.org>>
> > ---
> > target/ppc/helper.h | 2 -
> > target/ppc/mem_helper.c | 44 -----------------
> > target/ppc/translate.c | 102 ++++++++++++++++++----------------------
> > 3 files changed, 47 insertions(+), 101 deletions(-)
> >
> > diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> > index 8dd22a35e4..0beaca5c7a 100644
> > --- a/target/ppc/helper.h
> > +++ b/target/ppc/helper.h
> > @@ -818,6 +818,4 @@ DEF_HELPER_FLAGS_5(stq_le_parallel, TCG_CALL_NO_WG,
> > void, env, tl, i64, i64, i32)
> > DEF_HELPER_FLAGS_5(stq_be_parallel, TCG_CALL_NO_WG,
> > void, env, tl, i64, i64, i32)
> > -DEF_HELPER_5(stqcx_le_parallel, i32, env, tl, i64, i64, i32)
> > -DEF_HELPER_5(stqcx_be_parallel, i32, env, tl, i64, i64, i32)
> > #endif
> > diff --git a/target/ppc/mem_helper.c b/target/ppc/mem_helper.c
> > index d1163f316c..1578887a8f 100644
> > --- a/target/ppc/mem_helper.c
> > +++ b/target/ppc/mem_helper.c
> > @@ -413,50 +413,6 @@ void helper_stq_be_parallel(CPUPPCState *env, target_ulong addr,
> > val = int128_make128(lo, hi);
> > cpu_atomic_sto_be_mmu(env, addr, val, opidx, GETPC());
> > }
> > -
> > -uint32_t helper_stqcx_le_parallel(CPUPPCState *env, target_ulong addr,
> > - uint64_t new_lo, uint64_t new_hi,
> > - uint32_t opidx)
> > -{
> > - bool success = false;
> > -
> > - /* We will have raised EXCP_ATOMIC from the translator. */
> > - assert(HAVE_CMPXCHG128);
> > -
> > - if (likely(addr == env->reserve_addr)) {
> > - Int128 oldv, cmpv, newv;
> > -
> > - cmpv = int128_make128(env->reserve_val2, env->reserve_val);
> > - newv = int128_make128(new_lo, new_hi);
> > - oldv = cpu_atomic_cmpxchgo_le_mmu(env, addr, cmpv, newv,
> > - opidx, GETPC());
> > - success = int128_eq(oldv, cmpv);
> > - }
> > - env->reserve_addr = -1;
> > - return env->so + success * CRF_EQ_BIT;
> > -}
> > -
> > -uint32_t helper_stqcx_be_parallel(CPUPPCState *env, target_ulong addr,
> > - uint64_t new_lo, uint64_t new_hi,
> > - uint32_t opidx)
> > -{
> > - bool success = false;
> > -
> > - /* We will have raised EXCP_ATOMIC from the translator. */
> > - assert(HAVE_CMPXCHG128);
> > -
> > - if (likely(addr == env->reserve_addr)) {
> > - Int128 oldv, cmpv, newv;
> > -
> > - cmpv = int128_make128(env->reserve_val2, env->reserve_val);
> > - newv = int128_make128(new_lo, new_hi);
> > - oldv = cpu_atomic_cmpxchgo_be_mmu(env, addr, cmpv, newv,
> > - opidx, GETPC());
> > - success = int128_eq(oldv, cmpv);
> > - }
> > - env->reserve_addr = -1;
> > - return env->so + success * CRF_EQ_BIT;
> > -}
> > #endif
> >
> > /*****************************************************************************/
> > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > index 19c1d17cb0..85f95a9045 100644
> > --- a/target/ppc/translate.c
> > +++ b/target/ppc/translate.c
> > @@ -72,6 +72,7 @@ static TCGv cpu_cfar;
> > static TCGv cpu_xer, cpu_so, cpu_ov, cpu_ca, cpu_ov32, cpu_ca32;
> > static TCGv cpu_reserve;
> > static TCGv cpu_reserve_val;
> > +static TCGv cpu_reserve_val2;
> > static TCGv cpu_fpscr;
> > static TCGv_i32 cpu_access_type;
> >
> > @@ -141,8 +142,11 @@ void ppc_translate_init(void)
> > offsetof(CPUPPCState, reserve_addr),
> > "reserve_addr");
> > cpu_reserve_val = tcg_global_mem_new(cpu_env,
> > - offsetof(CPUPPCState, reserve_val),
> > - "reserve_val");
> > + offsetof(CPUPPCState, reserve_val),
> > + "reserve_val");
> > + cpu_reserve_val2 = tcg_global_mem_new(cpu_env,
> > + offsetof(CPUPPCState, reserve_val2),
> > + "reserve_val2");
> >
> > cpu_fpscr = tcg_global_mem_new(cpu_env,
> > offsetof(CPUPPCState, fpscr), "fpscr");
> > @@ -3979,78 +3983,66 @@ static void gen_lqarx(DisasContext *ctx)
> > /* stqcx. */
> > static void gen_stqcx_(DisasContext *ctx)
> > {
> > + TCGLabel *lab_fail, *lab_over;
> > int rs = rS(ctx->opcode);
> > - TCGv EA, hi, lo;
> > + TCGv EA, t0, t1;
> > + TCGv_i128 cmp, val;
> >
> > if (unlikely(rs & 1)) {
> > gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
> > return;
> > }
> >
> > + lab_fail = gen_new_label();
> > + lab_over = gen_new_label();
> > +
> > 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_temp_free(EA);
> > +
> > + cmp = tcg_temp_new_i128();
> > + val = tcg_temp_new_i128();
> > +
> > + tcg_gen_concat_i64_i128(cmp, cpu_reserve_val2, cpu_reserve_val);
> > +
> > /* Note that the low part is always in RS+1, even in LE mode. */
> > - lo = cpu_gpr[rs + 1];
> > - hi = cpu_gpr[rs];
> > + tcg_gen_concat_i64_i128(val, cpu_gpr[rs + 1], cpu_gpr[rs]);
> >
> > - if (tb_cflags(ctx->base.tb) & CF_PARALLEL) {
> > - if (HAVE_CMPXCHG128) {
> > - TCGv_i32 oi = tcg_const_i32(DEF_MEMOP(MO_128) | MO_ALIGN);
> > - if (ctx->le_mode) {
> > - gen_helper_stqcx_le_parallel(cpu_crf[0], cpu_env,
> > - EA, lo, hi, oi);
> > - } else {
> > - gen_helper_stqcx_be_parallel(cpu_crf[0], cpu_env,
> > - EA, lo, hi, oi);
> > - }
> > - tcg_temp_free_i32(oi);
> > - } else {
> > - /* Restart with exclusive lock. */
> > - gen_helper_exit_atomic(cpu_env);
> > - ctx->base.is_jmp = DISAS_NORETURN;
> > - }
> > - tcg_temp_free(EA);
> > - } else {
> > - TCGLabel *lab_fail = gen_new_label();
> > - TCGLabel *lab_over = gen_new_label();
> > - TCGv_i64 t0 = tcg_temp_new_i64();
> > - TCGv_i64 t1 = tcg_temp_new_i64();
> > + tcg_gen_atomic_cmpxchg_i128(val, cpu_reserve, cmp, val, ctx->mem_idx,
> > + DEF_MEMOP(MO_128 | MO_ALIGN));
> > + tcg_temp_free_i128(cmp);
> >
> > - tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lab_fail);
> > - tcg_temp_free(EA);
> > + t0 = tcg_temp_new();
> > + t1 = tcg_temp_new();
> > + tcg_gen_extr_i128_i64(t1, t0, val);
> > + tcg_temp_free_i128(val);
> >
> > - gen_qemu_ld64_i64(ctx, t0, cpu_reserve);
> > - tcg_gen_ld_i64(t1, cpu_env, (ctx->le_mode
> > - ? offsetof(CPUPPCState, reserve_val2)
> > - : offsetof(CPUPPCState, reserve_val)));
> > - tcg_gen_brcond_i64(TCG_COND_NE, t0, t1, lab_fail);
> > + tcg_gen_xor_tl(t1, t1, cpu_reserve_val2);
> > + tcg_gen_xor_tl(t0, t0, cpu_reserve_val);
> > + tcg_gen_or_tl(t0, t0, t1);
> > + tcg_temp_free(t1);
> >
> > - tcg_gen_addi_i64(t0, cpu_reserve, 8);
> > - gen_qemu_ld64_i64(ctx, t0, t0);
> > - tcg_gen_ld_i64(t1, cpu_env, (ctx->le_mode
> > - ? offsetof(CPUPPCState, reserve_val)
> > - : offsetof(CPUPPCState, reserve_val2)));
> > - tcg_gen_brcond_i64(TCG_COND_NE, t0, t1, lab_fail);
> > + 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_temp_free(t0);
> >
> > - /* Success */
> > - gen_qemu_st64_i64(ctx, ctx->le_mode ? lo : hi, cpu_reserve);
> > - tcg_gen_addi_i64(t0, cpu_reserve, 8);
> > - gen_qemu_st64_i64(ctx, ctx->le_mode ? hi : lo, t0);
> > + tcg_gen_br(lab_over);
> > + gen_set_label(lab_fail);
> >
> > - tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
> > - tcg_gen_ori_i32(cpu_crf[0], cpu_crf[0], CRF_EQ);
> > - tcg_gen_br(lab_over);
> > + /*
> > + * 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_fail);
> > - tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
> > -
> > - gen_set_label(lab_over);
> > - tcg_gen_movi_tl(cpu_reserve, -1);
> > - tcg_temp_free_i64(t0);
> > - tcg_temp_free_i64(t1);
> > - }
> > + gen_set_label(lab_over);
> > + tcg_gen_movi_tl(cpu_reserve, -1);
> > }
> > #endif /* defined(TARGET_PPC64) */
> >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH for-8.0 1/1] target/ppc: Use tcg_gen_atomic_cmpxchg_i128 for STQCX
2022-11-12 6:11 ` [PATCH for-8.0 1/1] target/ppc: Use tcg_gen_atomic_cmpxchg_i128 for STQCX Richard Henderson
2022-11-16 13:38 ` Daniel Henrique Barboza
@ 2022-11-21 22:37 ` Daniel Henrique Barboza
2022-11-21 22:53 ` Richard Henderson
1 sibling, 1 reply; 7+ messages in thread
From: Daniel Henrique Barboza @ 2022-11-21 22:37 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: david, qemu-ppc
On 11/12/22 03:11, Richard Henderson wrote:
> Note that the previous direct reference to reserve_val,
>
> - tcg_gen_ld_i64(t1, cpu_env, (ctx->le_mode
> - ? offsetof(CPUPPCState, reserve_val2)
> - : offsetof(CPUPPCState, reserve_val)));
>
> was incorrect because all references should have gone through
> cpu_reserve_val. Create a cpu_reserve_val2 tcg temp to fix this.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Richard, since this depends on your int128 with helpers, do you want to
queue this in the same queue?
Daniel
> target/ppc/helper.h | 2 -
> target/ppc/mem_helper.c | 44 -----------------
> target/ppc/translate.c | 102 ++++++++++++++++++----------------------
> 3 files changed, 47 insertions(+), 101 deletions(-)
>
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index 8dd22a35e4..0beaca5c7a 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -818,6 +818,4 @@ DEF_HELPER_FLAGS_5(stq_le_parallel, TCG_CALL_NO_WG,
> void, env, tl, i64, i64, i32)
> DEF_HELPER_FLAGS_5(stq_be_parallel, TCG_CALL_NO_WG,
> void, env, tl, i64, i64, i32)
> -DEF_HELPER_5(stqcx_le_parallel, i32, env, tl, i64, i64, i32)
> -DEF_HELPER_5(stqcx_be_parallel, i32, env, tl, i64, i64, i32)
> #endif
> diff --git a/target/ppc/mem_helper.c b/target/ppc/mem_helper.c
> index d1163f316c..1578887a8f 100644
> --- a/target/ppc/mem_helper.c
> +++ b/target/ppc/mem_helper.c
> @@ -413,50 +413,6 @@ void helper_stq_be_parallel(CPUPPCState *env, target_ulong addr,
> val = int128_make128(lo, hi);
> cpu_atomic_sto_be_mmu(env, addr, val, opidx, GETPC());
> }
> -
> -uint32_t helper_stqcx_le_parallel(CPUPPCState *env, target_ulong addr,
> - uint64_t new_lo, uint64_t new_hi,
> - uint32_t opidx)
> -{
> - bool success = false;
> -
> - /* We will have raised EXCP_ATOMIC from the translator. */
> - assert(HAVE_CMPXCHG128);
> -
> - if (likely(addr == env->reserve_addr)) {
> - Int128 oldv, cmpv, newv;
> -
> - cmpv = int128_make128(env->reserve_val2, env->reserve_val);
> - newv = int128_make128(new_lo, new_hi);
> - oldv = cpu_atomic_cmpxchgo_le_mmu(env, addr, cmpv, newv,
> - opidx, GETPC());
> - success = int128_eq(oldv, cmpv);
> - }
> - env->reserve_addr = -1;
> - return env->so + success * CRF_EQ_BIT;
> -}
> -
> -uint32_t helper_stqcx_be_parallel(CPUPPCState *env, target_ulong addr,
> - uint64_t new_lo, uint64_t new_hi,
> - uint32_t opidx)
> -{
> - bool success = false;
> -
> - /* We will have raised EXCP_ATOMIC from the translator. */
> - assert(HAVE_CMPXCHG128);
> -
> - if (likely(addr == env->reserve_addr)) {
> - Int128 oldv, cmpv, newv;
> -
> - cmpv = int128_make128(env->reserve_val2, env->reserve_val);
> - newv = int128_make128(new_lo, new_hi);
> - oldv = cpu_atomic_cmpxchgo_be_mmu(env, addr, cmpv, newv,
> - opidx, GETPC());
> - success = int128_eq(oldv, cmpv);
> - }
> - env->reserve_addr = -1;
> - return env->so + success * CRF_EQ_BIT;
> -}
> #endif
>
> /*****************************************************************************/
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 19c1d17cb0..85f95a9045 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -72,6 +72,7 @@ static TCGv cpu_cfar;
> static TCGv cpu_xer, cpu_so, cpu_ov, cpu_ca, cpu_ov32, cpu_ca32;
> static TCGv cpu_reserve;
> static TCGv cpu_reserve_val;
> +static TCGv cpu_reserve_val2;
> static TCGv cpu_fpscr;
> static TCGv_i32 cpu_access_type;
>
> @@ -141,8 +142,11 @@ void ppc_translate_init(void)
> offsetof(CPUPPCState, reserve_addr),
> "reserve_addr");
> cpu_reserve_val = tcg_global_mem_new(cpu_env,
> - offsetof(CPUPPCState, reserve_val),
> - "reserve_val");
> + offsetof(CPUPPCState, reserve_val),
> + "reserve_val");
> + cpu_reserve_val2 = tcg_global_mem_new(cpu_env,
> + offsetof(CPUPPCState, reserve_val2),
> + "reserve_val2");
>
> cpu_fpscr = tcg_global_mem_new(cpu_env,
> offsetof(CPUPPCState, fpscr), "fpscr");
> @@ -3979,78 +3983,66 @@ static void gen_lqarx(DisasContext *ctx)
> /* stqcx. */
> static void gen_stqcx_(DisasContext *ctx)
> {
> + TCGLabel *lab_fail, *lab_over;
> int rs = rS(ctx->opcode);
> - TCGv EA, hi, lo;
> + TCGv EA, t0, t1;
> + TCGv_i128 cmp, val;
>
> if (unlikely(rs & 1)) {
> gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
> return;
> }
>
> + lab_fail = gen_new_label();
> + lab_over = gen_new_label();
> +
> 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_temp_free(EA);
> +
> + cmp = tcg_temp_new_i128();
> + val = tcg_temp_new_i128();
> +
> + tcg_gen_concat_i64_i128(cmp, cpu_reserve_val2, cpu_reserve_val);
> +
> /* Note that the low part is always in RS+1, even in LE mode. */
> - lo = cpu_gpr[rs + 1];
> - hi = cpu_gpr[rs];
> + tcg_gen_concat_i64_i128(val, cpu_gpr[rs + 1], cpu_gpr[rs]);
>
> - if (tb_cflags(ctx->base.tb) & CF_PARALLEL) {
> - if (HAVE_CMPXCHG128) {
> - TCGv_i32 oi = tcg_const_i32(DEF_MEMOP(MO_128) | MO_ALIGN);
> - if (ctx->le_mode) {
> - gen_helper_stqcx_le_parallel(cpu_crf[0], cpu_env,
> - EA, lo, hi, oi);
> - } else {
> - gen_helper_stqcx_be_parallel(cpu_crf[0], cpu_env,
> - EA, lo, hi, oi);
> - }
> - tcg_temp_free_i32(oi);
> - } else {
> - /* Restart with exclusive lock. */
> - gen_helper_exit_atomic(cpu_env);
> - ctx->base.is_jmp = DISAS_NORETURN;
> - }
> - tcg_temp_free(EA);
> - } else {
> - TCGLabel *lab_fail = gen_new_label();
> - TCGLabel *lab_over = gen_new_label();
> - TCGv_i64 t0 = tcg_temp_new_i64();
> - TCGv_i64 t1 = tcg_temp_new_i64();
> + tcg_gen_atomic_cmpxchg_i128(val, cpu_reserve, cmp, val, ctx->mem_idx,
> + DEF_MEMOP(MO_128 | MO_ALIGN));
> + tcg_temp_free_i128(cmp);
>
> - tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lab_fail);
> - tcg_temp_free(EA);
> + t0 = tcg_temp_new();
> + t1 = tcg_temp_new();
> + tcg_gen_extr_i128_i64(t1, t0, val);
> + tcg_temp_free_i128(val);
>
> - gen_qemu_ld64_i64(ctx, t0, cpu_reserve);
> - tcg_gen_ld_i64(t1, cpu_env, (ctx->le_mode
> - ? offsetof(CPUPPCState, reserve_val2)
> - : offsetof(CPUPPCState, reserve_val)));
> - tcg_gen_brcond_i64(TCG_COND_NE, t0, t1, lab_fail);
> + tcg_gen_xor_tl(t1, t1, cpu_reserve_val2);
> + tcg_gen_xor_tl(t0, t0, cpu_reserve_val);
> + tcg_gen_or_tl(t0, t0, t1);
> + tcg_temp_free(t1);
>
> - tcg_gen_addi_i64(t0, cpu_reserve, 8);
> - gen_qemu_ld64_i64(ctx, t0, t0);
> - tcg_gen_ld_i64(t1, cpu_env, (ctx->le_mode
> - ? offsetof(CPUPPCState, reserve_val)
> - : offsetof(CPUPPCState, reserve_val2)));
> - tcg_gen_brcond_i64(TCG_COND_NE, t0, t1, lab_fail);
> + 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_temp_free(t0);
>
> - /* Success */
> - gen_qemu_st64_i64(ctx, ctx->le_mode ? lo : hi, cpu_reserve);
> - tcg_gen_addi_i64(t0, cpu_reserve, 8);
> - gen_qemu_st64_i64(ctx, ctx->le_mode ? hi : lo, t0);
> + tcg_gen_br(lab_over);
> + gen_set_label(lab_fail);
>
> - tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
> - tcg_gen_ori_i32(cpu_crf[0], cpu_crf[0], CRF_EQ);
> - tcg_gen_br(lab_over);
> + /*
> + * 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_fail);
> - tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
> -
> - gen_set_label(lab_over);
> - tcg_gen_movi_tl(cpu_reserve, -1);
> - tcg_temp_free_i64(t0);
> - tcg_temp_free_i64(t1);
> - }
> + gen_set_label(lab_over);
> + tcg_gen_movi_tl(cpu_reserve, -1);
> }
> #endif /* defined(TARGET_PPC64) */
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH for-8.0 1/1] target/ppc: Use tcg_gen_atomic_cmpxchg_i128 for STQCX
2022-11-21 22:37 ` Daniel Henrique Barboza
@ 2022-11-21 22:53 ` Richard Henderson
0 siblings, 0 replies; 7+ messages in thread
From: Richard Henderson @ 2022-11-21 22:53 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-devel; +Cc: david, qemu-ppc
On 11/21/22 14:37, Daniel Henrique Barboza wrote:
>
>
> On 11/12/22 03:11, Richard Henderson wrote:
>> Note that the previous direct reference to reserve_val,
>>
>> - tcg_gen_ld_i64(t1, cpu_env, (ctx->le_mode
>> - ? offsetof(CPUPPCState, reserve_val2)
>> - : offsetof(CPUPPCState, reserve_val)));
>>
>> was incorrect because all references should have gone through
>> cpu_reserve_val. Create a cpu_reserve_val2 tcg temp to fix this.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>
>
> Richard, since this depends on your int128 with helpers, do you want to
> queue this in the same queue?
I certainly can. Thanks,
r~
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-11-21 22:54 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-12 6:11 [PATCH for-8.0 0/1] target/ppc: Use tcg_gen_atomic_cmpxchg_i128 Richard Henderson
2022-11-12 6:11 ` [PATCH for-8.0 1/1] target/ppc: Use tcg_gen_atomic_cmpxchg_i128 for STQCX Richard Henderson
2022-11-16 13:38 ` Daniel Henrique Barboza
2022-11-17 2:48 ` Richard Henderson
2022-11-17 10:03 ` Daniel Henrique Barboza
2022-11-21 22:37 ` Daniel Henrique Barboza
2022-11-21 22:53 ` Richard Henderson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.