[Qemu-devel] qemu-arm: wrong execution of post-indexed loads when Rm and Rd are the same register

[Qemu-devel] qemu-arm: wrong execution of post-indexed loads when Rm and Rd are the same register

[Lauro Ramos Venancio]

Qemu-arm is wrongly executing post-indexed loads when Rm and Rd are
the same register. For example:

ldr r0, [r1], +r0

Current behavior:
r0 <- [r1]
r1 <- r1 + r0

Expected behavior:
addr <- r1
r1 <- r1 + r0
r0 <- [addr]

The attached patch fixes this bug. Patched by me and Rodrigo Vivi.
This patch was made based on qemu 0.9.


Lauro Venancio

[Qemu-devel] Re: qemu-arm: wrong execution of post-indexed loads when Rm and Rd are the same register

[Lauro Ramos Venancio]

[-- Attachment #1: Type: text/plain, Size: 486 bytes --]

Now sending the attachment. :)

Lauro 

On Thu, 2007-03-15 at 16:35 -0300, Lauro Ramos Venancio wrote:
> Qemu-arm is wrongly executing post-indexed loads when Rm and Rd are
> the same register. For example:
> 
> ldr r0, [r1], +r0
> 
> Current behavior:
> r0 <- [r1]
> r1 <- r1 + r0
> 
> Expected behavior:
> addr <- r1
> r1 <- r1 + r0
> r0 <- [addr]
> 
> The attached patch fixes this bug. Patched by me and Rodrigo Vivi.
> This patch was made based on qemu 0.9.
> 
> 
> Lauro Venancio

[-- Attachment #2: 00_ldr_writeback.patch --]
[-- Type: text/x-patch, Size: 3917 bytes --]

--- target-arm/op.c.orig	2007-03-09 18:40:02.000000000 -0300
+++ target-arm/op.c	2007-03-09 18:40:27.000000000 -0300
@@ -106,6 +106,11 @@ void OPPROTO op_movl_T0_T1(void)
     T0 = T1;
 }
 
+void OPPROTO op_movl_T1_T0(void)
+{
+    T1 = T0;
+}
+
 void OPPROTO op_movl_T1_im(void)
 {
     T1 = PARAM1;
--- target-arm/translate.c.orig	2007-03-09 18:40:02.000000000 -0300
+++ target-arm/translate.c	2007-03-09 18:40:32.000000000 -0300
@@ -383,23 +383,19 @@ static inline void gen_add_data_offset(D
     }
 }
 
-static inline void gen_add_datah_offset(DisasContext *s, unsigned int insn,
-                                        int extra)
+static inline void gen_add_datah_offset(DisasContext *s, unsigned int insn)
 {
     int val, rm;
     
     if (insn & (1 << 22)) {
         /* immediate */
         val = (insn & 0xf) | ((insn >> 4) & 0xf0);
-        val += extra;
         if (!(insn & (1 << 23)))
             val = -val;
         if (val != 0)
             gen_op_addl_T1_im(val);
     } else {
         /* register */
-        if (extra)
-            gen_op_addl_T1_im(extra);
         rm = (insn) & 0xf;
         gen_movl_T2_reg(s, rm);
         if (!(insn & (1 << 23)))
@@ -1534,14 +1530,17 @@ static void disas_arm_insn(CPUState * en
                     }
                 }
             } else {
-                int address_offset;
                 /* Misc load/store */
                 rn = (insn >> 16) & 0xf;
                 rd = (insn >> 12) & 0xf;
                 gen_movl_T1_reg(s, rn);
-                if (insn & (1 << 24))
-                    gen_add_datah_offset(s, insn, 0);
-                address_offset = 0;
+                gen_movl_T0_reg(s, rn);
+                gen_add_datah_offset(s, insn);
+                /* writeback */
+                if (!(insn & (1 << 24))||(insn & (1 << 21)))
+                  gen_movl_reg_T1(s, rn);
+                if (!(insn & (1 << 24))) /* pos-indexed */
+                  gen_op_movl_T1_T0();
                 if (insn & (1 << 20)) {
                     /* load */
                     switch(sh) {
@@ -1574,20 +1573,11 @@ static void disas_arm_insn(CPUState * en
                         gen_ldst(ldl, s);
                         gen_movl_reg_T0(s, rd + 1);
                     }
-                    address_offset = -4;
                 } else {
                     /* store */
                     gen_movl_T0_reg(s, rd);
                     gen_ldst(stw, s);
                 }
-                if (!(insn & (1 << 24))) {
-                    gen_add_datah_offset(s, insn, address_offset);
-                    gen_movl_reg_T1(s, rn);
-                } else if (insn & (1 << 21)) {
-                    if (address_offset)
-                        gen_op_addl_T1_im(address_offset);
-                    gen_movl_reg_T1(s, rn);
-                }
             }
             break;
         case 0x4:
@@ -1607,9 +1597,14 @@ static void disas_arm_insn(CPUState * en
             rn = (insn >> 16) & 0xf;
             rd = (insn >> 12) & 0xf;
             gen_movl_T1_reg(s, rn);
+	    gen_movl_T0_reg(s, rn);
             i = (IS_USER(s) || (insn & 0x01200000) == 0x00200000);
-            if (insn & (1 << 24))
                 gen_add_data_offset(s, insn);
+	    /* writeback */
+	    if (!(insn & (1 << 24))||(insn & (1 << 21)))
+	      gen_movl_reg_T1(s, rn);
+	    if (!(insn & (1 << 24))) /* pos-indexed */
+	      gen_op_movl_T1_T0();
             if (insn & (1 << 20)) {
                 /* load */
 #if defined(CONFIG_USER_ONLY)
@@ -1656,12 +1651,6 @@ static void disas_arm_insn(CPUState * en
                 }
 #endif
             }
-            if (!(insn & (1 << 24))) {
-                gen_add_data_offset(s, insn);
-                gen_movl_reg_T1(s, rn);
-            } else if (insn & (1 << 21))
-                gen_movl_reg_T1(s, rn); {
-            }
             break;
         case 0x08:
         case 0x09:

Re: [Qemu-devel] qemu-arm: wrong execution of post-indexed loads when Rm and Rd are the same register

[Paul Brook]

On Thursday 15 March 2007 19:35, Lauro Ramos Venancio wrote:
> Qemu-arm is wrongly executing post-indexed loads when Rm and Rd are
> the same register. For example:
>
> ldr r0, [r1], +r0
>
> Current behavior:
> r0 <- [r1]
> r1 <- r1 + r0
>
> Expected behavior:
> addr <- r1
> r1 <- r1 + r0
> r0 <- [addr]

This is still wrong. The writeback must happen after the load. Your 
implementation will give incorrect results if the load faults.

Paul

Re: [Qemu-devel] qemu-arm: wrong execution of post-indexed loads when Rm and Rd are the same register

[Rodrigo Vivi]

Hi Paul,

On 3/15/07, Paul Brook <paul@codesourcery.com> wrote:
> On Thursday 15 March 2007 19:35, Lauro Ramos Venancio wrote:
> > Qemu-arm is wrongly executing post-indexed loads when Rm and Rd are
> > the same register. For example:
> >
> > ldr r0, [r1], +r0
> >
> > Current behavior:
> > r0 <- [r1]
> > r1 <- r1 + r0
> >
> > Expected behavior:
> > addr <- r1
> > r1 <- r1 + r0
> > r0 <- [addr]
>
> This is still wrong.
So, is this a known bug?

> The writeback must happen after the load.
We code like this because
- we didn't find this restriction in arm reference manual
- the LLVM uses this instruction expecting a result like this
- That was the result that we got running these instructions in an OMAP1710

> Your
> implementation will give incorrect results if the load faults.

Another thing, is that in this manual (at page A2-18) there are 2
rules for data abort:
- Base Restored Abort Model and Base Updated Abort Model.

So, we can not understand why it will give incorect results if a load faults...

>
> Paul

thanks
Rodrigo Vivi
vivijim at freenode

>
>
> _______________________________________________
> Qemu-devel mailing list
> Qemu-devel@nongnu.org
> http://lists.nongnu.org/mailman/listinfo/qemu-devel
>

Re: [Qemu-devel] qemu-arm: wrong execution of post-indexed loads when Rm and Rd are the same register

[Paul Brook]

> > This is still wrong.
>
> So, is this a known bug?

Still wrong implies it's a bug, and your patch does not fix it properly.

> > The writeback must happen after the load.
>
> We code like this because
> - we didn't find this restriction in arm reference manual

It's the Abort model section you mention below.

> - the LLVM uses this instruction expecting a result like this

The compiler knows nothing about the abort behavior. The difference is only 
visible if the load faults.

> - That was the result that we got running these instructions in an OMAP1710

I suggest you check again. I'm fairly sure the arm926 implements the Base 
Restored abort model.

Paul

Re: [Qemu-devel] qemu-arm: wrong execution of post-indexed loads when Rm and Rd are the same register

[Rodrigo Vivi]

On 3/15/07, Paul Brook <paul@codesourcery.com> wrote:
> > > This is still wrong.
> >
> > So, is this a known bug?
>
> Still wrong implies it's a bug, and your patch does not fix it properly.

I know that...
I was not clear.. sorry...
what I mean is: do you agree that there was a bug in these instructions?

>
> > > The writeback must happen after the load.
> >
> > We code like this because
> > - we didn't find this restriction in arm reference manual
>
> It's the Abort model section you mention below.
>
> > - the LLVM uses this instruction expecting a result like this
>
> The compiler knows nothing about the abort behavior. The difference is only
> visible if the load faults.
>
> > - That was the result that we got running these instructions in an OMAP1710
>
> I suggest you check again. I'm fairly sure the arm926 implements the Base
> Restored abort model.

Actually we did not test the abort model...

So, Base Restored abort model is the model that qemu implements, isn't it?
then we will try to use that and recode the patch...

thanks for your help

>
> Paul
>
vivijim

Re: [Qemu-devel] qemu-arm: wrong execution of post-indexed loads when Rm and Rd are the same register

[Laurent Desnogues]

Paul Brook a écrit :
> 
> I suggest you check again. I'm fairly sure the arm926 implements the Base 
> Restored abort model.

Yes, but arm7 is Based Updated IIRC.  What particular implementation
does Qemu target?

There are so many IMPLEMENTATION DEFINED and UNPREDICTABLE in the
architecture (that are indeed used by even Linux kernel) that targeting
multiple implementations in the same is almost impossible.


			Laurent

Re: [Qemu-devel] qemu-arm: wrong execution of post-indexed loads when Rm and Rd are the same register

[Paul Brook]

On Thursday 15 March 2007 21:55, Laurent Desnogues wrote:
> Paul Brook a écrit :
> > I suggest you check again. I'm fairly sure the arm926 implements the Base
> > Restored abort model.
>
> Yes, but arm7 is Based Updated IIRC.  What particular implementation
> does Qemu target?

Qemu currently emulates arm926 or arm1026.
For userspace emulation we have to use the Base Restored model because that's 
what linux implements, even on Base Updated hardware.
In recent revisions of the ARM Architecture (v6+) Base Restored is the only 
model allowed.

Paul

Re: [Qemu-devel] qemu-arm: wrong execution of post-indexed loads when Rm and Rd are the same register

[Lauro Ramos Venancio]

[-- Attachment #1: Type: text/plain, Size: 88 bytes --]

I'm sending a new version of the patch that uses Base Restored data
abort model.

Lauro

[-- Attachment #2: 00_ldr_writeback.patch --]
[-- Type: text/x-patch, Size: 2010 bytes --]

diff -ru qemu-0.9.0.orig/target-arm/translate.c qemu-0.9.0/target-arm/translate.c
--- qemu-0.9.0.orig/target-arm/translate.c	2007-03-16 11:41:28.000000000 -0300
+++ qemu-0.9.0/target-arm/translate.c	2007-03-16 14:59:40.000000000 -0300
@@ -1556,7 +1556,6 @@
                         gen_ldst(ldsw, s);
                         break;
                     }
-                    gen_movl_reg_T0(s, rd);
                 } else if (sh & 2) {
                     /* doubleword */
                     if (sh & 1) {
@@ -1572,7 +1571,7 @@
                         gen_movl_reg_T0(s, rd);
                         gen_op_addl_T1_im(4);
                         gen_ldst(ldl, s);
-                        gen_movl_reg_T0(s, rd + 1);
+                        ++rd;
                     }
                     address_offset = -4;
                 } else {
@@ -1588,6 +1587,12 @@
                         gen_op_addl_T1_im(address_offset);
                     gen_movl_reg_T1(s, rn);
                 }
+
+                if ((insn & (1 << 20)) ||
+                    ((!(insn & (1 << 20)))&&((sh & 3) == 2))) {
+                    /* load */
+                    gen_movl_reg_T0(s, rd);
+                }
             }
             break;
         case 0x4:
@@ -1630,10 +1635,6 @@
                         gen_op_ldl_kernel();
                 }
 #endif
-                if (rd == 15)
-                    gen_bx(s);
-                else
-                    gen_movl_reg_T0(s, rd);
             } else {
                 /* store */
                 gen_movl_T0_reg(s, rd);
@@ -1662,6 +1663,13 @@
             } else if (insn & (1 << 21))
                 gen_movl_reg_T1(s, rn); {
             }
+            if (insn & (1 << 20)) {
+                /* load */
+                if (rd == 15)
+                    gen_bx(s);
+                else
+                    gen_movl_reg_T0(s, rd);
+            }
             break;
         case 0x08:
         case 0x09:
Only in qemu-0.9.0/target-arm: translate.c~