target/i386: cmpxchg should not touch accumulator

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

yqwfh posted 1 patch 2 years, 9 months ago

Download mbox

Test checkpatch passed

Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210731190507.9007-1-amdeniulari@protonmail.com

Maintainers: Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <ehabkost@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>

target/i386/tcg/translate.c | 9 ---------
1 file changed, 9 deletions(-)

Expand all Fold all

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

Posted by yqwfh 2 years, 9 months ago

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);
                 }
             }
             tcg_gen_mov_tl(cpu_cc_src, oldv);
-- 
2.26.3

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

Posted by Richard Henderson 2 years, 9 months ago

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~