On Wed, Mar 23, 2022 at 9:37 AM Wei Li <lw945lw...@yahoo.com> wrote:
> This patch fixes a bug reported on issues #508.
> The problem is cmpxchg and lock cmpxchg would touch accumulator when
> the comparison is equal.
>
> Signed-off-by: Wei Li <lw945lw...@yahoo.com>
> ---
> target/i386/tcg/translate.c | 41 +++++++++++++++++--------------------
> 1 file changed, 19 insertions(+), 22 deletions(-)
>
> diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
> index 2a94d33742..9677f9576b 100644
> --- a/target/i386/tcg/translate.c
> +++ b/target/i386/tcg/translate.c
> @@ -5339,7 +5339,7 @@ static target_ulong disas_insn(DisasContext *s,
> CPUState *cpu)
> case 0x1b0:
> case 0x1b1: /* cmpxchg Ev, Gv */
> {
> - TCGv oldv, newv, cmpv;
> + TCGv oldv, newv, cmpv, temp;
>
> ot = mo_b_d(b, dflag);
> modrm = x86_ldub_code(env, s);
> @@ -5348,42 +5348,38 @@ static target_ulong disas_insn(DisasContext *s,
> CPUState *cpu)
> oldv = tcg_temp_new();
> newv = tcg_temp_new();
> cmpv = tcg_temp_new();
> + temp = tcg_temp_new();
> gen_op_mov_v_reg(s, ot, newv, reg);
> tcg_gen_mov_tl(cmpv, cpu_regs[R_EAX]);
> + tcg_gen_mov_tl(temp, cpu_regs[R_EAX]);
>
> - if (s->prefix & PREFIX_LOCK) {
> + if ((s->prefix & PREFIX_LOCK) ||
> + (mod != 3)) {
> + /* Use the tcg_gen_atomic_cmpxchg_tl path whenever mod !=
> 3.
> + While an unlocked cmpxchg need not be atomic, it is not
> + required to be non-atomic either. */
> if (mod == 3) {
> goto illegal_op;
> }
> 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);
> + gen_extu(ot, oldv);
> + gen_extu(ot, cmpv);
> } else {
> - if (mod == 3) {
> - rm = (modrm & 7) | REX_B(s);
> - gen_op_mov_v_reg(s, ot, oldv, rm);
> - } else {
> - gen_lea_modrm(env, s, modrm);
> - gen_op_ld_v(s, ot, oldv, s->A0);
> - rm = 0; /* avoid warning */
> - }
> + rm = (modrm & 7) | REX_B(s);
> + gen_op_mov_v_reg(s, ot, oldv, rm);
> 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 */
> - gen_op_st_v(s, ot, newv, s->A0);
> - gen_op_mov_reg_v(s, ot, R_EAX, oldv);
> - }
> + gen_op_mov_reg_v(s, ot, rm, newv);
> }
> + /* Perform the merge into %al or %ax as required by ot. */
> + gen_op_mov_reg_v(s, ot, R_EAX, oldv);
> + /* Undo the entire modification to %rax if comparison equal.
> */
> + tcg_gen_movcond_tl(TCG_COND_EQ, cpu_regs[R_EAX], oldv, cmpv,
> + temp, cpu_regs[R_EAX]);
> tcg_gen_mov_tl(cpu_cc_src, oldv);
> tcg_gen_mov_tl(s->cc_srcT, cmpv);
> tcg_gen_sub_tl(cpu_cc_dst, cmpv, oldv);
> @@ -5391,6 +5387,7 @@ static target_ulong disas_insn(DisasContext *s,
> CPUState *cpu)
> tcg_temp_free(oldv);
> tcg_temp_free(newv);
> tcg_temp_free(cmpv);
> + tcg_temp_free(temp);
> }
> break;
> case 0x1c7: /* cmpxchg8b */
> --
> 2.30.2
>
>
>
Is this patch OK? or I forgot to add a reviewed-by tags by my hand?
This is my first time participating in this process.
Please let me know if anything is wrong.
Thanks a lot.
Wei Li
