| Message ID | 20230604102858.148584-1-npiggin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] target/ppc: Fix lqarx to set cpu_reserve | expand |
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~
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
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];
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(-)