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~