Patchwork i386 target bug with cmpxchg instruction handling

login
register
mail settings
Submitter Brad
Date Dec. 25, 2010, 9:16 p.m.
Message ID <20101225211637.GA13596@rox.home.comstyle.com>
Download mbox | patch
Permalink /patch/76702/
State New
Headers show

Comments

Brad - Dec. 25, 2010, 9:16 p.m.
The following diff fixes a bug with the i386 targets cmpxhg instruction
handling. I'm making an attempt to submit this upstream since OpenBSD
currently has this in our port but it was originally from NetBSD's
pkgsrc tree with QEMU.

http://www.netbsd.org/cgi-bin/query-pr-single.pl?number=42158

https://bugs.launchpad.net/qemu/+bug/569760
malc - Dec. 25, 2010, 9:42 p.m.
On Sat, 25 Dec 2010, Brad wrote:

> The following diff fixes a bug with the i386 targets cmpxhg instruction
> handling. I'm making an attempt to submit this upstream since OpenBSD
> currently has this in our port but it was originally from NetBSD's
> pkgsrc tree with QEMU.
> 
> http://www.netbsd.org/cgi-bin/query-pr-single.pl?number=42158
> 
> https://bugs.launchpad.net/qemu/+bug/569760
> 

Please see
http://www.mail-archive.com/qemu-devel@nongnu.org/msg41500.html
Brad - Dec. 25, 2010, 9:55 p.m.
On Saturday 25 December 2010 16:42:14 malc wrote:
> On Sat, 25 Dec 2010, Brad wrote:
> > The following diff fixes a bug with the i386 targets cmpxhg instruction
> > handling. I'm making an attempt to submit this upstream since OpenBSD
> > currently has this in our port but it was originally from NetBSD's
> > pkgsrc tree with QEMU.
> >
> > http://www.netbsd.org/cgi-bin/query-pr-single.pl?number=42158
> >
> > https://bugs.launchpad.net/qemu/+bug/569760
>
> Please see
> http://www.mail-archive.com/qemu-devel@nongnu.org/msg41500.html

Ok. I see. Oh well, I guess we'll keep this as a local patch for now. It's 
better than the situation with the current upstream code.

Patch

diff --git a/target-i386/translate.c b/target-i386/translate.c
index 7b6e3c2..b5d6c97 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -4879,20 +4879,24 @@  static target_ulong disas_insn(DisasContext *s, target_ulong pc_start)
             tcg_gen_sub_tl(t2, cpu_regs[R_EAX], t0);
             gen_extu(ot, t2);
             tcg_gen_brcondi_tl(TCG_COND_EQ, t2, 0, label1);
+            label2 = gen_new_label();
             if (mod == 3) {
-                label2 = gen_new_label();
                 gen_op_mov_reg_v(ot, R_EAX, t0);
                 tcg_gen_br(label2);
                 gen_set_label(label1);
                 gen_op_mov_reg_v(ot, rm, t1);
-                gen_set_label(label2);
             } else {
-                tcg_gen_mov_tl(t1, t0);
+                /* perform no-op 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(ot + s->mem_index, t0, a0);
                 gen_op_mov_reg_v(ot, R_EAX, t0);
+                tcg_gen_br(label2);
                 gen_set_label(label1);
-                /* always store */
                 gen_op_st_v(ot + s->mem_index, t1, a0);
             }
+            gen_set_label(label2);
             tcg_gen_mov_tl(cpu_cc_src, t0);
             tcg_gen_mov_tl(cpu_cc_dst, t2);
             s->cc_op = CC_OP_SUBB + ot;