ARM: Fix subtle bug in LDM.

If the instruction faults mid-op the base register shouldn't be written back.

3 files changed, 68 insertions, 31 deletions

diff --git a/src/arch/arm/insts/macromem.cc b/src/arch/arm/insts/macromem.cc
index c03b7ccc1..8a82bd319 100644
--- a/src/arch/arm/insts/macromem.cc
+++ b/src/arch/arm/insts/macromem.cc
@@ -58,8 +58,13 @@ MacroMemOp::MacroMemOp(const char *mnem, ExtMachInst machInst,
 {
     uint32_t regs = reglist;
     uint32_t ones = number_of_ones(reglist);
-    // Remember that writeback adds a uop
-    numMicroops = ones + (writeback ? 1 : 0) + 1;
+    // Remember that writeback adds a uop or two and the temp register adds one
+    numMicroops = ones + (writeback ? (load ? 2 : 1) : 0) + 1;
+
+    // It's technically legal to do a lot of nothing
+    if (!ones)
+        numMicroops = 1;
+
     microOps = new StaticInstPtr[numMicroops];
     uint32_t addr = 0;
 
@@ -70,28 +75,13 @@ MacroMemOp::MacroMemOp(const char *mnem, ExtMachInst machInst,
         addr += 4;
 
     StaticInstPtr *uop = microOps;
-    StaticInstPtr wbUop;
-    if (writeback) {
-        if (up) {
-            wbUop = new MicroAddiUop(machInst, rn, rn, ones * 4);
-        } else {
-            wbUop = new MicroSubiUop(machInst, rn, rn, ones * 4);
-        }
-    }
 
     // Add 0 to Rn and stick it in ureg0.
     // This is equivalent to a move.
     *uop = new MicroAddiUop(machInst, INTREG_UREG0, rn, 0);
 
-    // Write back at the start for loads. This covers the ldm exception return
-    // case where the base needs to be written in the old mode. Stores may need
-    // the original value of the base, but they don't change mode and can
-    // write back at the end like before.
-    if (load && writeback) {
-        *++uop = wbUop;
-    }
-
     unsigned reg = 0;
+    unsigned regIdx = 0;
     bool force_user = user & !bits(reglist, 15);
     bool exception_ret = user & bits(reglist, 15);
 
@@ -101,19 +91,28 @@ MacroMemOp::MacroMemOp(const char *mnem, ExtMachInst machInst,
             reg++;
         replaceBits(regs, reg, 0);
 
-        unsigned regIdx = reg;
+        regIdx = reg;
         if (force_user) {
             regIdx = intRegInMode(MODE_USER, regIdx);
         }
 
         if (load) {
-            if (reg == INTREG_PC && exception_ret) {
-                // This must be the exception return form of ldm.
-                *++uop = new MicroLdrRetUop(machInst, regIdx,
-                                           INTREG_UREG0, up, addr);
+            if (writeback && i == ones - 1) {
+                // If it's a writeback and this is the last register
+                // do the load into a temporary register which we'll move
+                // into the final one later
+                *++uop = new MicroLdrUop(machInst, INTREG_UREG1, INTREG_UREG0,
+                        up, addr);
             } else {
-                *++uop = new MicroLdrUop(machInst, regIdx,
-                                        INTREG_UREG0, up, addr);
+                // Otherwise just do it normally
+                if (reg == INTREG_PC && exception_ret) {
+                    // This must be the exception return form of ldm.
+                    *++uop = new MicroLdrRetUop(machInst, regIdx,
+                                               INTREG_UREG0, up, addr);
+                } else {
+                    *++uop = new MicroLdrUop(machInst, regIdx,
+                                            INTREG_UREG0, up, addr);
+                }
             }
         } else {
             *++uop = new MicroStrUop(machInst, regIdx, INTREG_UREG0, up, addr);
@@ -125,8 +124,32 @@ MacroMemOp::MacroMemOp(const char *mnem, ExtMachInst machInst,
             addr -= 4;
     }
 
-    if (!load && writeback) {
-        *++uop = wbUop;
+    if (writeback && ones) {
+        // put the register update after we're done all loading
+        if (up)
+            *++uop = new MicroAddiUop(machInst, rn, rn, ones * 4);
+        else
+            *++uop = new MicroSubiUop(machInst, rn, rn, ones * 4);
+
+        // If this was a load move the last temporary value into place
+        // this way we can't take an exception after we update the base
+        // register.
+        if (load && reg == INTREG_PC && exception_ret) {
+            *++uop = new MicroUopRegMovRet(machInst, 0, INTREG_UREG1);
+            warn("creating instruction with exception return at curTick:%d\n",
+                    curTick());
+        } else if (load) {
+            *++uop = new MicroUopRegMov(machInst, regIdx, INTREG_UREG1);
+            if (reg == INTREG_PC) {
+                (*uop)->setFlag(StaticInstBase::IsControl);
+                (*uop)->setFlag(StaticInstBase::IsCondControl);
+                (*uop)->setFlag(StaticInstBase::IsIndirectControl);
+                // This is created as a RAS POP
+                if (rn == INTREG_SP)
+                    (*uop)->setFlag(StaticInstBase::IsReturn);
+
+            }
+        }
     }
 
     (*uop)->setLastMicroop();
diff --git a/src/arch/arm/isa/insts/macromem.isa b/src/arch/arm/isa/insts/macromem.isa
index d6c929353..15879e0e3 100644
--- a/src/arch/arm/isa/insts/macromem.isa
+++ b/src/arch/arm/isa/insts/macromem.isa
@@ -86,24 +86,27 @@ let {{
                                        'predicate_test': predicateTest},
                                       ['IsMicroop'])
 
-    microLdrRetUopCode = '''
+    microRetUopCode = '''
         CPSR cpsr = Cpsr;
         SCTLR sctlr = Sctlr;
         uint32_t newCpsr =
             cpsrWriteByInstr(cpsr | CondCodes, Spsr, 0xF, true, sctlr.nmfi);
         Cpsr = ~CondCodesMask & newCpsr;
         CondCodes = CondCodesMask & newCpsr;
-        IWNPC = cSwap(Mem.uw, cpsr.e) | ((Spsr & 0x20) ? 1 : 0);
+        IWNPC = cSwap(%s, cpsr.e) | ((Spsr & 0x20) ? 1 : 0);
         ForcedItState = ((((CPSR)Spsr).it2 << 2) & 0xFC)
                 | (((CPSR)Spsr).it1 & 0x3);
     '''
+
     microLdrRetUopIop = InstObjParams('ldr_ret_uop', 'MicroLdrRetUop',
                                       'MicroMemOp',
-                                      {'memacc_code': microLdrRetUopCode,
+                                      {'memacc_code':
+                                          microRetUopCode % 'Mem.uw',
                                        'ea_code':
                                           'EA = URb + (up ? imm : -imm);',
                                        'predicate_test': condPredicateTest},
-                                      ['IsMicroop','IsNonSpeculative','IsSerializeAfter'])
+                                      ['IsMicroop','IsNonSpeculative',
+                                       'IsSerializeAfter'])
 
     microStrUopCode = "Mem = cSwap(URa.uw, ((CPSR)Cpsr).e);"
     microStrUopIop = InstObjParams('str_uop', 'MicroStrUop',
@@ -608,6 +611,13 @@ let {{
                                     'predicate_test': predicateTest},
                                    ['IsMicroop'])
 
+    microUopRegMovRetIop = InstObjParams('movret_uop', 'MicroUopRegMovRet',
+                                      'MicroIntMov',
+                                     {'code': microRetUopCode % 'URb',
+                                      'predicate_test': predicateTest},
+                                     ['IsMicroop', 'IsNonSpeculative',
+                                      'IsSerializeAfter'])
+
     setPCCPSRDecl = '''
                     CPSR cpsrOrCondCodes = URc;
                     SCTLR sctlr = Sctlr;
@@ -634,6 +644,7 @@ let {{
                     MicroIntRegDeclare.subst(microAddUopIop) + \
                     MicroIntRegDeclare.subst(microSubUopIop) + \
                     MicroIntMovDeclare.subst(microUopRegMovIop) + \
+                    MicroIntMovDeclare.subst(microUopRegMovRetIop) + \
                     MicroSetPCCPSRDeclare.subst(microUopSetPCCPSRIop)
 
     decoder_output = MicroIntImmConstructor.subst(microAddiUopIop) + \
@@ -641,6 +652,7 @@ let {{
                      MicroIntRegConstructor.subst(microAddUopIop) + \
                      MicroIntRegConstructor.subst(microSubUopIop) + \
                      MicroIntMovConstructor.subst(microUopRegMovIop) + \
+                     MicroIntMovConstructor.subst(microUopRegMovRetIop) + \
                      MicroSetPCCPSRConstructor.subst(microUopSetPCCPSRIop)
 
     exec_output = PredOpExecute.subst(microAddiUopIop) + \
@@ -648,6 +660,7 @@ let {{
                   PredOpExecute.subst(microAddUopIop) + \
                   PredOpExecute.subst(microSubUopIop) + \
                   PredOpExecute.subst(microUopRegMovIop) + \
+                  PredOpExecute.subst(microUopRegMovRetIop) + \
                   PredOpExecute.subst(microUopSetPCCPSRIop)
 
 }};
diff --git a/src/cpu/static_inst.hh b/src/cpu/static_inst.hh
index 933496864..d07b322df 100644
--- a/src/cpu/static_inst.hh
+++ b/src/cpu/static_inst.hh
@@ -267,6 +267,7 @@ class StaticInstBase : public RefCounted
 
     void setLastMicroop() { flags[IsLastMicroop] = true; }
     void setDelayedCommit() { flags[IsDelayedCommit] = true; }
+    void setFlag(Flags f) { flags[f] = true; }
 
     /// Operation class.  Used to select appropriate function unit in issue.
     OpClass opClass()     const { return _opClass; }