Re: [PATCH 1/2] target/i386: fix cmpxchg with 32-bit register destination

[Richard Henderson]

On 9/11/22 23:26, Paolo Bonzini wrote:
> +/* Compute the result of writing t0 to the OT-sized register REG.
> + *
> + * If DEST is NULL, store the result into the register and return the
> + * register's TCGv.
> + *
> + * If DEST is not NULL, store the result into DEST and return the
> + * register's TCGv.
> + */

/*
 * Compute...

Why bother passing NULL, and fixing it up at each use...

> +static void gen_op_mov_reg_v(DisasContext *s, MemOp ot, int reg, TCGv t0)
> +{
> +    gen_op_deposit_reg_v(s, ot, reg, NULL, t0);
>   }

... when you can just as easily pass in the register here?

>   
>   static inline
> @@ -5495,26 +5514,36 @@ static target_ulong disas_insn(DisasContext *s, 
> CPUState *cpu)
>                                             s->mem_index, ot | MO_LE);
>                   gen_op_mov_reg_v(s, ot, R_EAX, oldv);
>               } else {
> +                gen_extu(ot, cmpv);
>                   if (mod == 3) {
> +                    TCGv dest;
>                       rm = (modrm & 7) | REX_B(s);
>                       gen_op_mov_v_reg(s, ot, oldv, rm);
> +                    gen_extu(ot, oldv);
> +
> +                    /*
> +                     * Unlike the memory case, where "the destination operand 
> receives
> +                     * a write cycle without regard to the result of the 
> comparison",
> +                     * rm must not be touched altogether if the write fails, 
> including
> +                     * not zero-extending it on 64-bit processors.  So, 
> precompute
> +                     * the result of a successful writeback and perform the 
> movcond
> +                     * directly on cpu_regs.  Also need to write accumulator 
> first, in
> +                     * case rm is part of RAX too.
> +                     */
> +                    gen_op_mov_reg_v(s, ot, R_EAX, oldv);
> +                    dest = gen_op_deposit_reg_v(s, ot, rm, newv, newv);
> +                    tcg_gen_movcond_tl(TCG_COND_EQ, dest, oldv, cmpv, newv, 
> dest);
>                   } else {
>                       gen_lea_modrm(env, s, modrm);
>                       gen_op_ld_v(s, ot, oldv, s->A0);
> -                    rm = 0; /* avoid warning */
> -                }
> -                gen_extu(ot, oldv);
> -                gen_extu(ot, cmpv);
> -                /* 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 */
> +
> +                    /*
> +                     * 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
> +                     */
> +                    tcg_gen_movcond_tl(TCG_COND_EQ, newv, oldv, cmpv, newv, 
> oldv);
>                       gen_op_st_v(s, ot, newv, s->A0);
>                       gen_op_mov_reg_v(s, ot, R_EAX, oldv);
>                   }

We have an outstanding bug report that suggests that the move to eax must use the deposit 
in both cases:

https://gitlab.com/qemu-project/qemu/-/issues/508


r~