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

[Daniel Henrique Barboza]

On 6/4/23 23:54, Nicholas Piggin wrote:
> Differently-sized larx/stcx. pairs can succeed if the starting address
> matches. Add a check to require the size of stcx. exactly match the larx
> that established the reservation. Use the term "reserve_length" for this
> state, which matches the terminology used in the ISA.
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---

Queued. Thanks,


Daniel

> v2:
> - Changed lqarx/stqcx. reservation size to 16 [Richard]
> - Changed name to reserve_length [Richard]
>
>   target/ppc/cpu.h       | 5 +++--
>   target/ppc/cpu_init.c  | 4 ++--
>   target/ppc/translate.c | 9 +++++++++
>   3 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 7959bfed0a..45d84ce06a 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1123,8 +1123,9 @@ struct CPUArchState {
>       target_ulong ov32;
>       target_ulong ca32;
>   
> -    target_ulong reserve_addr; /* Reservation address */
> -    target_ulong reserve_val;  /* Reservation value */
> +    target_ulong reserve_addr;   /* Reservation address */
> +    target_ulong reserve_length; /* Reservation larx op size (bytes) */
> +    target_ulong reserve_val;    /* Reservation value */
>       target_ulong reserve_val2;
>   
>       /* These are used in supervisor mode only */
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 944a74befe..c3dd7052a3 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -7421,8 +7421,8 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
>           }
>           qemu_fprintf(f, " %c%c", a, env->crf[i] & 0x01 ? 'O' : ' ');
>       }
> -    qemu_fprintf(f, " ]             RES " TARGET_FMT_lx "\n",
> -                 env->reserve_addr);
> +    qemu_fprintf(f, " ]     RES %03x@" TARGET_FMT_lx "\n",
> +                 (int)env->reserve_length, env->reserve_addr);
>   
>       if (flags & CPU_DUMP_FPU) {
>           for (i = 0; i < 32; i++) {
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 7a5bf1d820..538f757dec 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -71,6 +71,7 @@ static TCGv cpu_cfar;
>   #endif
>   static TCGv cpu_xer, cpu_so, cpu_ov, cpu_ca, cpu_ov32, cpu_ca32;
>   static TCGv cpu_reserve;
> +static TCGv cpu_reserve_length;
>   static TCGv cpu_reserve_val;
>   static TCGv cpu_reserve_val2;
>   static TCGv cpu_fpscr;
> @@ -141,6 +142,10 @@ void ppc_translate_init(void)
>       cpu_reserve = tcg_global_mem_new(cpu_env,
>                                        offsetof(CPUPPCState, reserve_addr),
>                                        "reserve_addr");
> +    cpu_reserve_length = tcg_global_mem_new(cpu_env,
> +                                            offsetof(CPUPPCState,
> +                                                     reserve_length),
> +                                            "reserve_length");
>       cpu_reserve_val = tcg_global_mem_new(cpu_env,
>                                            offsetof(CPUPPCState, reserve_val),
>                                            "reserve_val");
> @@ -3585,6 +3590,7 @@ static void gen_load_locked(DisasContext *ctx, MemOp 
> memop)
>       gen_addr_reg_index(ctx, t0);
>       tcg_gen_qemu_ld_tl(gpr, t0, ctx->mem_idx, memop | MO_ALIGN);
>       tcg_gen_mov_tl(cpu_reserve, t0);
> +    tcg_gen_movi_tl(cpu_reserve_length, memop_size(memop));
>       tcg_gen_mov_tl(cpu_reserve_val, gpr);
>       tcg_gen_mb(TCG_MO_ALL | TCG_BAR_LDAQ);
>   }
> @@ -3816,6 +3822,7 @@ static void gen_conditional_store(DisasContext *ctx, 
> MemOp memop)
>       gen_set_access_type(ctx, ACCESS_RES);
>       gen_addr_reg_index(ctx, t0);
>       tcg_gen_brcond_tl(TCG_COND_NE, t0, cpu_reserve, l1);
> +    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, memop_size(memop), l1);
>   
>       t0 = tcg_temp_new();
>       tcg_gen_atomic_cmpxchg_tl(t0, cpu_reserve, cpu_reserve_val,
> @@ -3882,6 +3889,7 @@ static void gen_lqarx(DisasContext *ctx)
>       tcg_gen_extr_i128_i64(lo, hi, t16);
>   
>       tcg_gen_mov_tl(cpu_reserve, EA);
> +    tcg_gen_movi_tl(cpu_reserve_length, 16);
>       tcg_gen_st_tl(hi, cpu_env, offsetof(CPUPPCState, reserve_val));
>       tcg_gen_st_tl(lo, cpu_env, offsetof(CPUPPCState, reserve_val2));
>   }
> @@ -3907,6 +3915,7 @@ static void gen_stqcx_(DisasContext *ctx)
>       gen_addr_reg_index(ctx, EA);
>   
>       tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, lab_fail);
> +    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_reserve_length, 16, lab_fail);
>   
>       cmp = tcg_temp_new_i128();
>       val = tcg_temp_new_i128();