[PATCH 1/4] target/ppc: Fix lqarx to set cpu_reserve

[PATCH 1/4] target/ppc: Fix lqarx to set cpu_reserve

[Nicholas Piggin]

lqarx does not set cpu_reserve, which causes stqcx. to never succeed.
Fix this and slightly rearrange gen_load_locked so the two functions
match more closely.

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>
---
cpu_reserve got lost in the parallel part with the first patch, then
from serial part when it was merged with the parallel by the second
patch.

Thanks,
Nick

 target/ppc/translate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 3650d2985d..e129cdcb8f 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -3583,8 +3583,8 @@ static void gen_load_locked(DisasContext *ctx, MemOp memop)
 
     gen_set_access_type(ctx, ACCESS_RES);
     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_qemu_ld_tl(gpr, t0, ctx->mem_idx, memop | MO_ALIGN);
     tcg_gen_mov_tl(cpu_reserve_val, gpr);
     tcg_gen_mb(TCG_MO_ALL | TCG_BAR_LDAQ);
 }
@@ -3872,6 +3872,7 @@ static void gen_lqarx(DisasContext *ctx)
     gen_set_access_type(ctx, ACCESS_RES);
     EA = tcg_temp_new();
     gen_addr_reg_index(ctx, EA);
+    tcg_gen_mov_tl(cpu_reserve, EA);
 
     /* Note that the low part is always in RD+1, even in LE mode.  */
     lo = cpu_gpr[rd + 1];
-- 
2.40.1

[PATCH 2/4] target/ppc: Ensure stcx size matches larx

[Nicholas Piggin]

Differently-sized larx/stcx. pairs can succeed if the starting address
matches. Add a size check to require stcx. exactly match the larx that
established the reservation.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/cpu.h       | 1 +
 target/ppc/cpu_init.c  | 4 ++--
 target/ppc/translate.c | 8 ++++++++
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 7959bfed0a..1d71f325d8 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1124,6 +1124,7 @@ struct CPUArchState {
     target_ulong ca32;
 
     target_ulong reserve_addr; /* Reservation address */
+    target_ulong reserve_size; /* Reservation size */
     target_ulong reserve_val;  /* Reservation value */
     target_ulong reserve_val2;
 
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 944a74befe..082981b148 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_size, 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 e129cdcb8f..5195047146 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_size;
 static TCGv cpu_reserve_val;
 static TCGv cpu_reserve_val2;
 static TCGv cpu_fpscr;
@@ -141,6 +142,9 @@ void ppc_translate_init(void)
     cpu_reserve = tcg_global_mem_new(cpu_env,
                                      offsetof(CPUPPCState, reserve_addr),
                                      "reserve_addr");
+    cpu_reserve_size = tcg_global_mem_new(cpu_env,
+                                          offsetof(CPUPPCState, reserve_size),
+                                          "reserve_size");
     cpu_reserve_val = tcg_global_mem_new(cpu_env,
                                          offsetof(CPUPPCState, reserve_val),
                                          "reserve_val");
@@ -3584,6 +3588,7 @@ static void gen_load_locked(DisasContext *ctx, MemOp memop)
     gen_set_access_type(ctx, ACCESS_RES);
     gen_addr_reg_index(ctx, t0);
     tcg_gen_mov_tl(cpu_reserve, t0);
+    tcg_gen_movi_tl(cpu_reserve_size, memop_size(memop));
     tcg_gen_qemu_ld_tl(gpr, t0, ctx->mem_idx, memop | MO_ALIGN);
     tcg_gen_mov_tl(cpu_reserve_val, gpr);
     tcg_gen_mb(TCG_MO_ALL | TCG_BAR_LDAQ);
@@ -3816,6 +3821,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_size, memop_size(memop), l1);
 
     t0 = tcg_temp_new();
     tcg_gen_atomic_cmpxchg_tl(t0, cpu_reserve, cpu_reserve_val,
@@ -3873,6 +3879,7 @@ static void gen_lqarx(DisasContext *ctx)
     EA = tcg_temp_new();
     gen_addr_reg_index(ctx, EA);
     tcg_gen_mov_tl(cpu_reserve, EA);
+    tcg_gen_movi_tl(cpu_reserve_size, 128);
 
     /* Note that the low part is always in RD+1, even in LE mode.  */
     lo = cpu_gpr[rd + 1];
@@ -3907,6 +3914,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_size, 128, lab_fail);
 
     cmp = tcg_temp_new_i128();
     val = tcg_temp_new_i128();
-- 
2.40.1

Re: [PATCH 2/4] target/ppc: Ensure stcx size matches larx

[Richard Henderson]

> @@ -3584,6 +3588,7 @@ static void gen_load_locked(DisasContext *ctx, MemOp memop)
>       gen_set_access_type(ctx, ACCESS_RES);
>       gen_addr_reg_index(ctx, t0);
>       tcg_gen_mov_tl(cpu_reserve, t0);
> +    tcg_gen_movi_tl(cpu_reserve_size, memop_size(memop));

Not that it really matters, this produces a byte value...

> @@ -3873,6 +3879,7 @@ static void gen_lqarx(DisasContext *ctx)
>       EA = tcg_temp_new();
>       gen_addr_reg_index(ctx, EA);
>       tcg_gen_mov_tl(cpu_reserve, EA);
> +    tcg_gen_movi_tl(cpu_reserve_size, 128);

... so perhaps ideally this would be 16.

Perhaps name it reserve_length to exactly match the manual.
Otherwise,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 2/4] target/ppc: Ensure stcx size matches larx

[Nicholas Piggin]

On Sun Jun 4, 2023 at 8:28 PM AEST, Nicholas Piggin wrote:
> Differently-sized larx/stcx. pairs can succeed if the starting address
> matches. Add a size check to require stcx. exactly match the larx that
> established the reservation.

Hmm, question: reserve_addr is a VMSTATE field, but reserve_val is not
(nor reserve_size after this patch).

Blue Swirl added that with commit a456d59c20f ("VM load/save support for
PPC CPU"), and when reserve_val was added in commit 18b21a2f83a
("target-ppc: retain l{w,d}arx loaded value") it did not get migrated.

Could we end up with reserve_addr != -1, but with a bogus reserve_val,
which could then permit a stcx. incorrectly? Not entirely outlandish if
reserve_val starts out initialised to zero.

Could we just clear the reserve in cpu_post_load? It is permitted to be
lost for an implementation-specific reason. Doesn't seem necessary to
try keep it alive over a migration.

Thanks,
Nick

Re: [PATCH 2/4] target/ppc: Ensure stcx size matches larx

[Richard Henderson]

On 6/5/23 08:27, Nicholas Piggin wrote:
> On Sun Jun 4, 2023 at 8:28 PM AEST, Nicholas Piggin wrote:
>> Differently-sized larx/stcx. pairs can succeed if the starting address
>> matches. Add a size check to require stcx. exactly match the larx that
>> established the reservation.
> 
> Hmm, question: reserve_addr is a VMSTATE field, but reserve_val is not
> (nor reserve_size after this patch).
> 
> Blue Swirl added that with commit a456d59c20f ("VM load/save support for
> PPC CPU"), and when reserve_val was added in commit 18b21a2f83a
> ("target-ppc: retain l{w,d}arx loaded value") it did not get migrated.
> 
> Could we end up with reserve_addr != -1, but with a bogus reserve_val,
> which could then permit a stcx. incorrectly? Not entirely outlandish if
> reserve_val starts out initialised to zero.
> 
> Could we just clear the reserve in cpu_post_load? It is permitted to be
> lost for an implementation-specific reason. Doesn't seem necessary to
> try keep it alive over a migration.

It's not a bad idea to flush the reservation over migrate.
You can do

-       VMSTATE_UINTTL(env.reserve_addr, PowerPCCPU),
+       VMSTATE_UNUSED(sizeof(target_long))

when making that change.

Peter, any thoughts on this?  If we're going to do one guest, we might ought to do the 
same for all load-lock/store-conditional guests.


r~

Re: [PATCH 2/4] target/ppc: Ensure stcx size matches larx

[Peter Maydell]

On Mon, 19 Jun 2023 at 16:49, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 6/5/23 08:27, Nicholas Piggin wrote:
> > On Sun Jun 4, 2023 at 8:28 PM AEST, Nicholas Piggin wrote:
> >> Differently-sized larx/stcx. pairs can succeed if the starting address
> >> matches. Add a size check to require stcx. exactly match the larx that
> >> established the reservation.
> >
> > Hmm, question: reserve_addr is a VMSTATE field, but reserve_val is not
> > (nor reserve_size after this patch).
> >
> > Blue Swirl added that with commit a456d59c20f ("VM load/save support for
> > PPC CPU"), and when reserve_val was added in commit 18b21a2f83a
> > ("target-ppc: retain l{w,d}arx loaded value") it did not get migrated.
> >
> > Could we end up with reserve_addr != -1, but with a bogus reserve_val,
> > which could then permit a stcx. incorrectly? Not entirely outlandish if
> > reserve_val starts out initialised to zero.
> >
> > Could we just clear the reserve in cpu_post_load? It is permitted to be
> > lost for an implementation-specific reason. Doesn't seem necessary to
> > try keep it alive over a migration.
>
> It's not a bad idea to flush the reservation over migrate.

Is there any particular reason to do so? The default simple
thing is "if this is state that persists across instructions
then migrate it"; we usually reserve "do something special in
post-load" for oddball cases where "just copy the data" doesn't
work.

target/arm migrates both the exclusive addr and value.

target/mips migrates lladdr but has forgotten llval
(and perhaps llval_wp and llnewval_wp, depending on what
those fields do).

thanks
-- PMM

Re: [PATCH 2/4] target/ppc: Ensure stcx size matches larx

[Richard Henderson]

On 6/19/23 17:55, Peter Maydell wrote:
> On Mon, 19 Jun 2023 at 16:49, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 6/5/23 08:27, Nicholas Piggin wrote:
>>> On Sun Jun 4, 2023 at 8:28 PM AEST, Nicholas Piggin wrote:
>>>> Differently-sized larx/stcx. pairs can succeed if the starting address
>>>> matches. Add a size check to require stcx. exactly match the larx that
>>>> established the reservation.
>>>
>>> Hmm, question: reserve_addr is a VMSTATE field, but reserve_val is not
>>> (nor reserve_size after this patch).
>>>
>>> Blue Swirl added that with commit a456d59c20f ("VM load/save support for
>>> PPC CPU"), and when reserve_val was added in commit 18b21a2f83a
>>> ("target-ppc: retain l{w,d}arx loaded value") it did not get migrated.
>>>
>>> Could we end up with reserve_addr != -1, but with a bogus reserve_val,
>>> which could then permit a stcx. incorrectly? Not entirely outlandish if
>>> reserve_val starts out initialised to zero.
>>>
>>> Could we just clear the reserve in cpu_post_load? It is permitted to be
>>> lost for an implementation-specific reason. Doesn't seem necessary to
>>> try keep it alive over a migration.
>>
>> It's not a bad idea to flush the reservation over migrate.
> 
> Is there any particular reason to do so? The default simple
> thing is "if this is state that persists across instructions
> then migrate it"; we usually reserve "do something special in
> post-load" for oddball cases where "just copy the data" doesn't
> work.
> 
> target/arm migrates both the exclusive addr and value.

ppc is adding "size", which arm technically should have as well.

> target/mips migrates lladdr but has forgotten llval
> (and perhaps llval_wp and llnewval_wp, depending on what
> those fields do).

So, similarly, would need to handle migration for which all of the required data is not 
present.

The thought is, rather than migrate this new data also, and handle compatibility, simply 
discard all reservations.


r~

Re: [PATCH 2/4] target/ppc: Ensure stcx size matches larx

[Peter Maydell]

On Mon, 19 Jun 2023 at 18:03, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 6/19/23 17:55, Peter Maydell wrote:
> > On Mon, 19 Jun 2023 at 16:49, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> On 6/5/23 08:27, Nicholas Piggin wrote:
> >>> On Sun Jun 4, 2023 at 8:28 PM AEST, Nicholas Piggin wrote:
> >>>> Differently-sized larx/stcx. pairs can succeed if the starting address
> >>>> matches. Add a size check to require stcx. exactly match the larx that
> >>>> established the reservation.
> >>>
> >>> Hmm, question: reserve_addr is a VMSTATE field, but reserve_val is not
> >>> (nor reserve_size after this patch).
> >>>
> >>> Blue Swirl added that with commit a456d59c20f ("VM load/save support for
> >>> PPC CPU"), and when reserve_val was added in commit 18b21a2f83a
> >>> ("target-ppc: retain l{w,d}arx loaded value") it did not get migrated.
> >>>
> >>> Could we end up with reserve_addr != -1, but with a bogus reserve_val,
> >>> which could then permit a stcx. incorrectly? Not entirely outlandish if
> >>> reserve_val starts out initialised to zero.
> >>>
> >>> Could we just clear the reserve in cpu_post_load? It is permitted to be
> >>> lost for an implementation-specific reason. Doesn't seem necessary to
> >>> try keep it alive over a migration.
> >>
> >> It's not a bad idea to flush the reservation over migrate.
> >
> > Is there any particular reason to do so? The default simple
> > thing is "if this is state that persists across instructions
> > then migrate it"; we usually reserve "do something special in
> > post-load" for oddball cases where "just copy the data" doesn't
> > work.
> >
> > target/arm migrates both the exclusive addr and value.
>
> ppc is adding "size", which arm technically should have as well.

Arm allows an implementation to require the transaction size
to match on loadexcl and storexcl, but doesn't mandate it, fwiw.
(Also, our implementation is miles away from the architectural
requirements anyway because we operate on virtual addresses,
not physical addresses.)

> > target/mips migrates lladdr but has forgotten llval
> > (and perhaps llval_wp and llnewval_wp, depending on what
> > those fields do).
>
> So, similarly, would need to handle migration for which all of the required data is not
> present.
>
> The thought is, rather than migrate this new data also, and handle compatibility, simply
> discard all reservations.

I don't see a problem for normal migration and snapshotting.
I do wonder whether this would have a bad interaction
with record-and-replay's use of snapshots. Does that
expect "execution from the loaded snapshot" to match
"execution continues from point of snapshot save" ?

-- PMM

Re: [PATCH 2/4] target/ppc: Ensure stcx size matches larx

[Nicholas Piggin]

On Tue Jun 20, 2023 at 3:14 AM AEST, Peter Maydell wrote:
> On Mon, 19 Jun 2023 at 18:03, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> >
> > On 6/19/23 17:55, Peter Maydell wrote:
> > > On Mon, 19 Jun 2023 at 16:49, Richard Henderson
> > > <richard.henderson@linaro.org> wrote:
> > >>
> > >> On 6/5/23 08:27, Nicholas Piggin wrote:
> > >>> On Sun Jun 4, 2023 at 8:28 PM AEST, Nicholas Piggin wrote:
> > >>>> Differently-sized larx/stcx. pairs can succeed if the starting address
> > >>>> matches. Add a size check to require stcx. exactly match the larx that
> > >>>> established the reservation.
> > >>>
> > >>> Hmm, question: reserve_addr is a VMSTATE field, but reserve_val is not
> > >>> (nor reserve_size after this patch).
> > >>>
> > >>> Blue Swirl added that with commit a456d59c20f ("VM load/save support for
> > >>> PPC CPU"), and when reserve_val was added in commit 18b21a2f83a
> > >>> ("target-ppc: retain l{w,d}arx loaded value") it did not get migrated.
> > >>>
> > >>> Could we end up with reserve_addr != -1, but with a bogus reserve_val,
> > >>> which could then permit a stcx. incorrectly? Not entirely outlandish if
> > >>> reserve_val starts out initialised to zero.
> > >>>
> > >>> Could we just clear the reserve in cpu_post_load? It is permitted to be
> > >>> lost for an implementation-specific reason. Doesn't seem necessary to
> > >>> try keep it alive over a migration.
> > >>
> > >> It's not a bad idea to flush the reservation over migrate.
> > >
> > > Is there any particular reason to do so? The default simple
> > > thing is "if this is state that persists across instructions
> > > then migrate it"; we usually reserve "do something special in
> > > post-load" for oddball cases where "just copy the data" doesn't
> > > work.
> > >
> > > target/arm migrates both the exclusive addr and value.
> >
> > ppc is adding "size", which arm technically should have as well.
>
> Arm allows an implementation to require the transaction size
> to match on loadexcl and storexcl, but doesn't mandate it, fwiw.
> (Also, our implementation is miles away from the architectural
> requirements anyway because we operate on virtual addresses,
> not physical addresses.)

The same as powerpc. Size *and* address within reserve granule
does not have to match the larx which established the reserve,
but the latter we always enforced and in practice no open source
software seems to hit it (or AIX).

My thinking is that it is good to tighten it because very likely
software that gets it wrong is deviating from ISA unintentionally.
Linux provides no HWCAP bit to allow code to test such
implementation details, for example.

> > > target/mips migrates lladdr but has forgotten llval
> > > (and perhaps llval_wp and llnewval_wp, depending on what
> > > those fields do).
> >
> > So, similarly, would need to handle migration for which all of the required data is not
> > present.
> >
> > The thought is, rather than migrate this new data also, and handle compatibility, simply
> > discard all reservations.
>
> I don't see a problem for normal migration and snapshotting.
> I do wonder whether this would have a bad interaction
> with record-and-replay's use of snapshots. Does that
> expect "execution from the loaded snapshot" to match
> "execution continues from point of snapshot save" ?

I don't mind the idea of moving the new state across, I wondered
if clearing the reserve would be easier for compatibility and
backporting.

I don't know the rr code but if the snapshots use this vmstate
and the replay from that is expected to match exactly the
recording, then I think you must be right.

Thanks,
Nick

[PATCH 3/4] target/ppc: Remove larx/stcx. memory barrier semantics

[Nicholas Piggin]

larx and stcx. are not defined to order any memory operations.
Remove the barriers.

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 5195047146..77e1c5abb6 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -3591,7 +3591,6 @@ static void gen_load_locked(DisasContext *ctx, MemOp memop)
     tcg_gen_movi_tl(cpu_reserve_size, memop_size(memop));
     tcg_gen_qemu_ld_tl(gpr, t0, ctx->mem_idx, memop | MO_ALIGN);
     tcg_gen_mov_tl(cpu_reserve_val, gpr);
-    tcg_gen_mb(TCG_MO_ALL | TCG_BAR_LDAQ);
 }
 
 #define LARX(name, memop)                  \
@@ -3835,11 +3834,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);
@@ -3943,11 +3937,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

Re: [PATCH 3/4] target/ppc: Remove larx/stcx. memory barrier semantics

[Richard Henderson]

On 6/4/23 03:28, Nicholas Piggin wrote:
> larx and stcx. are not defined to order any memory operations.
> Remove the barriers.
> 
> Signed-off-by: Nicholas Piggin<npiggin@gmail.com>
> ---
>   target/ppc/translate.c | 11 -----------
>   1 file changed, 11 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 4/4] target/ppc: Rework store conditional to avoid branch

[Nicholas Piggin]

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_.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/translate.c | 65 ++++++++++++++++++++----------------------
 1 file changed, 31 insertions(+), 34 deletions(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 77e1c5abb6..cf99e961f7 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -3812,31 +3812,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_size, 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_size, 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,
-                              DEF_MEMOP(memop) | MO_ALIGN);
+                              cpu_gpr[rs], ctx->mem_idx,
+                              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);
 }
 
@@ -3890,25 +3891,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_size, 128, lab_fail);
+    tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lfail);
+    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_size, 128, lfail);
 
     cmp = tcg_temp_new_i128();
     val = tcg_temp_new_i128();
@@ -3931,15 +3933,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

Re: [PATCH 4/4] target/ppc: Rework store conditional to avoid branch

[Richard Henderson]

On 6/4/23 03:28, 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_.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   target/ppc/translate.c | 65 ++++++++++++++++++++----------------------
>   1 file changed, 31 insertions(+), 34 deletions(-)
> 
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 77e1c5abb6..cf99e961f7 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -3812,31 +3812,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_size, 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_size, 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,
> -                              DEF_MEMOP(memop) | MO_ALIGN);
> +                              cpu_gpr[rs], ctx->mem_idx,
> +                              memop | MO_ALIGN);

Lost DEF_MEMOP here.  Otherwise,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 1/4] target/ppc: Fix lqarx to set cpu_reserve

[Richard Henderson]

On 6/4/23 03:28, Nicholas Piggin wrote:
> lqarx does not set cpu_reserve, which causes stqcx. to never succeed.
> Fix this and slightly rearrange gen_load_locked so the two functions
> match more closely.
> 
> 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>
> ---
> cpu_reserve got lost in the parallel part with the first patch, then
> from serial part when it was merged with the parallel by the second
> patch.

Oops, sorry about that.

> 
> Thanks,
> Nick
> 
>   target/ppc/translate.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 3650d2985d..e129cdcb8f 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -3583,8 +3583,8 @@ static void gen_load_locked(DisasContext *ctx, MemOp memop)
>   
>       gen_set_access_type(ctx, ACCESS_RES);
>       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_qemu_ld_tl(gpr, t0, ctx->mem_idx, memop | MO_ALIGN);
>       tcg_gen_mov_tl(cpu_reserve_val, gpr);

This change is wrong.  Reserve should not be set if the load faults.

>       tcg_gen_mb(TCG_MO_ALL | TCG_BAR_LDAQ);
>   }
> @@ -3872,6 +3872,7 @@ static void gen_lqarx(DisasContext *ctx)
>       gen_set_access_type(ctx, ACCESS_RES);
>       EA = tcg_temp_new();
>       gen_addr_reg_index(ctx, EA);
> +    tcg_gen_mov_tl(cpu_reserve, EA);
>   
>       /* Note that the low part is always in RD+1, even in LE mode.  */
>       lo = cpu_gpr[rd + 1];

This needs to go lower with the sets of reserve_val*.


r~

Re: [PATCH 1/4] target/ppc: Fix lqarx to set cpu_reserve

[Nicholas Piggin]

On Mon Jun 5, 2023 at 2:05 AM AEST, Richard Henderson wrote:
> On 6/4/23 03:28, Nicholas Piggin wrote:
> > lqarx does not set cpu_reserve, which causes stqcx. to never succeed.
> > Fix this and slightly rearrange gen_load_locked so the two functions
> > match more closely.
> > 
> > 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>
> > ---
> > cpu_reserve got lost in the parallel part with the first patch, then
> > from serial part when it was merged with the parallel by the second
> > patch.
>
> Oops, sorry about that.

No problem, I really appreciate your work on ppc, ppc just should have
more unit tests particularly for non-trivial instructions like lqarx
which would have caught it. That's the real problem.

>
> > 
> > Thanks,
> > Nick
> > 
> >   target/ppc/translate.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > index 3650d2985d..e129cdcb8f 100644
> > --- a/target/ppc/translate.c
> > +++ b/target/ppc/translate.c
> > @@ -3583,8 +3583,8 @@ static void gen_load_locked(DisasContext *ctx, MemOp memop)
> >   
> >       gen_set_access_type(ctx, ACCESS_RES);
> >       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_qemu_ld_tl(gpr, t0, ctx->mem_idx, memop | MO_ALIGN);
> >       tcg_gen_mov_tl(cpu_reserve_val, gpr);
>
> This change is wrong.  Reserve should not be set if the load faults.

Oh yeah, good catch.

Thanks
Nick