Re: [PATCH] target/i386: cmpxchg should not touch accumulator

[Richard Henderson]

On 7/31/21 9:06 AM, yqwfh wrote:
> Signed-off-by: Daniele Ahmed <ahmeddan@amazon.com>
> ---
>   target/i386/tcg/translate.c | 9 ---------
>   1 file changed, 9 deletions(-)
>
> diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
> index aacb605..41386dd 100644
> --- a/target/i386/tcg/translate.c
> +++ b/target/i386/tcg/translate.c
> @@ -5366,7 +5366,6 @@ static target_ulong disas_insn(DisasContext *s, CPUState 
> *cpu)
>                   gen_lea_modrm(env, s, modrm);
>                   tcg_gen_atomic_cmpxchg_tl(oldv, s->A0, cmpv, newv,
>                                             s->mem_index, ot | MO_LE);
> -                gen_op_mov_reg_v(s, ot, R_EAX, oldv);
>               } else {
>                   if (mod == 3) {
>                       rm = (modrm & 7) | REX_B(s);
> @@ -5381,15 +5380,7 @@ static target_ulong disas_insn(DisasContext *s, CPUState 
> *cpu)
>                   /* store value = (old == cmp ? new : old);  */
>                   tcg_gen_movcond_tl(TCG_COND_EQ, newv, oldv, cmpv, newv, oldv);
>                   if (mod == 3) {
> -                    gen_op_mov_reg_v(s, ot, R_EAX, oldv);
>                       gen_op_mov_reg_v(s, ot, rm, newv);
> -                } else {
> -                    /* Perform an unconditional store cycle like physical cpu;
> -                       must be before changing accumulator to ensure
> -                       idempotency if the store faults and the instruction
> -                       is restarted */
> -                    gen_op_st_v(s, ot, newv, s->A0);
> -                    gen_op_mov_reg_v(s, ot, R_EAX, oldv);
>                   }

Obviously incorrect.  You can't *not* store to EAX.  You must conditionally 
store to EAX.


r~