riscv: Fix bugs with RISC-V decoder and detailed CPUs

This patch fixes some bugs that were missed with the changes to the
decoder that enabled compatibility with compressed instructions. In
order to accommodate speculation with variable instruction widths, a few
assertions in decoder had to be changed to returning faults as the
specification describes should normally happen.  The rest of these
assertions will be changed in a later patch.

[Remove commented-out debugging line and add clarifying comment to
registerName in utility.hh.]

Change-Id: I3f333008430d4a905cb59547a3513f5149b43b95
Reviewed-on: https://gem5-review.googlesource.com/4041
Reviewed-by: Jason Lowe-Power <jason@lowepower.com>
Maintainer: Alec Roelke <ar4jc@virginia.edu>

diff --git a/src/arch/riscv/decoder.cc b/src/arch/riscv/decoder.cc
index 36504f4f8956387cf669b4604f715568b2201d00..020c5e34ef9af2bff7d0a336ae74e95f1f04006f 100644 (file)
--- a/src/arch/riscv/decoder.cc
+++ b/src/arch/riscv/decoder.cc
@@ -37,29 +37,45 @@
 namespace RiscvISA
 {
 
+static const MachInst LowerBitMask = (1 << sizeof(MachInst) * 4) - 1;
+static const MachInst UpperBitMask = LowerBitMask << sizeof(MachInst) * 4;
+
+void Decoder::reset()
+{
+    aligned = true;
+    mid = false;
+    more = true;
+    emi = NoopMachInst;
+    instDone = false;
+}
+
 void
 Decoder::moreBytes(const PCState &pc, Addr fetchPC, MachInst inst)
 {
-    DPRINTF(Decode, "Getting bytes 0x%08x from address %#x\n",
-            inst, pc.pc());
+    DPRINTF(Decode, "Requesting bytes 0x%08x from address %#x\n", inst,
+            fetchPC);
 
     bool aligned = pc.pc() % sizeof(MachInst) == 0;
-    if (mid) {
-        assert(!aligned);
-        emi |= (inst & 0xFFFF) << 16;
+    if (aligned) {
+        emi = inst;
+        if (compressed(emi))
+            emi &= LowerBitMask;
+        more = !compressed(emi);
         instDone = true;
     } else {
-        MachInst instChunk = aligned ? inst & 0xFFFF :
-                                      (inst & 0xFFFF0000) >> 16;
-        if (aligned) {
-            emi = (inst & 0x3) < 0x3 ? instChunk : inst;
+        if (mid) {
+            assert((emi & UpperBitMask) == 0);
+            emi |= (inst & LowerBitMask) << sizeof(MachInst)*4;
+            mid = false;
+            more = false;
             instDone = true;
         } else {
-            emi = instChunk;
-            instDone = (instChunk & 0x3) < 0x3;
+            emi = (inst & UpperBitMask) >> sizeof(MachInst)*4;
+            mid = !compressed(emi);
+            more = true;
+            instDone = compressed(emi);
         }
     }
-    mid = !instDone;
 }
 
 StaticInstPtr
@@ -83,12 +99,10 @@ Decoder::decode(RiscvISA::PCState &nextPC)
         return nullptr;
     instDone = false;
 
-    if ((emi & 0x3) < 0x3) {
-        nextPC.compressed(true);
-        nextPC.npc(nextPC.pc() + sizeof(MachInst)/2);
+    if (compressed(emi)) {
+        nextPC.npc(nextPC.instAddr() + sizeof(MachInst) / 2);
     } else {
-        nextPC.compressed(false);
-        nextPC.npc(nextPC.pc() + sizeof(MachInst));
+        nextPC.npc(nextPC.instAddr() + sizeof(MachInst));
     }
 
     return decode(emi, nextPC.instAddr());

diff --git a/src/arch/riscv/decoder.hh b/src/arch/riscv/decoder.hh
index ef644fa13a962d27396ccb91bd954901919b35ff..c1d68bf06032f561ca39b7f508560db54c5aa0bc 100644 (file)
--- a/src/arch/riscv/decoder.hh
+++ b/src/arch/riscv/decoder.hh
@@ -49,7 +49,9 @@ class Decoder
 {
   private:
     DecodeCache::InstMap instMap;
+    bool aligned;
     bool mid;
+    bool more;
 
   protected:
     //The extended machine instruction being generated
@@ -57,18 +59,18 @@ class Decoder
     bool instDone;
 
   public:
-    Decoder(ISA* isa=nullptr)
-        : mid(false), emi(NoopMachInst), instDone(false)
-    {}
+    Decoder(ISA* isa=nullptr) { reset(); }
 
     void process() {}
-    void reset() { instDone = false; }
+    void reset();
+
+    inline bool compressed(ExtMachInst inst) { return (inst & 0x3) < 0x3; }
 
     //Use this to give data to the decoder. This should be used
     //when there is control flow.
     void moreBytes(const PCState &pc, Addr fetchPC, MachInst inst);
 
-    bool needMoreBytes() { return true; }
+    bool needMoreBytes() { return more; }
     bool instReady() { return instDone; }
     void takeOverFrom(Decoder *old) {}
 

diff --git a/src/arch/riscv/faults.cc b/src/arch/riscv/faults.cc
index 58baa4e32de338e408ee51d163e33223118c25aa..4e44d43f074201ea87037a42bb474ddf4ca56687 100644 (file)
--- a/src/arch/riscv/faults.cc
+++ b/src/arch/riscv/faults.cc
@@ -63,6 +63,13 @@ UnknownInstFault::invoke_se(ThreadContext *tc, const StaticInstPtr &inst)
         tc->pcState().pc());
 }
 
+void
+IllegalInstFault::invoke_se(ThreadContext *tc, const StaticInstPtr &inst)
+{
+    panic("Illegal instruction 0x%08x at pc 0x%016llx: %s", inst->machInst,
+        tc->pcState().pc(), reason.c_str());
+}
+
 void
 UnimplementedFault::invoke_se(ThreadContext *tc,
         const StaticInstPtr &inst)

diff --git a/src/arch/riscv/faults.hh b/src/arch/riscv/faults.hh
index d0d7988c591498a54829722dce7be3412a874927..1e33b648fee03aa7be284615c64ee7ffb2772119 100644 (file)
--- a/src/arch/riscv/faults.hh
+++ b/src/arch/riscv/faults.hh
@@ -116,6 +116,19 @@ class UnknownInstFault : public RiscvFault
     invoke_se(ThreadContext *tc, const StaticInstPtr &inst);
 };
 
+class IllegalInstFault : public RiscvFault
+{
+  private:
+    const std::string reason;
+  public:
+    IllegalInstFault(std::string r)
+        : RiscvFault("Illegal instruction", INST_ILLEGAL, SOFTWARE),
+          reason(r)
+    {}
+
+    void invoke_se(ThreadContext *tc, const StaticInstPtr &inst);
+};
+
 class UnimplementedFault : public RiscvFault
 {
   private:

diff --git a/src/arch/riscv/isa/decoder.isa b/src/arch/riscv/isa/decoder.isa
index 4f4ef7636034e79bcb080bb227a2cc1995938970..0e5567ac3bbf9729c1eaeb985f3a5baf42678ca7 100644 (file)
--- a/src/arch/riscv/isa/decoder.isa
+++ b/src/arch/riscv/isa/decoder.isa
@@ -42,7 +42,8 @@ decode QUADRANT default Unknown::unknown() {
                   CIMM8<7:6> << 4 |
                   CIMM8<5:2> << 6;
         }}, {{
-            assert(imm != 0);
+            if (machInst == 0)
+                fault = make_shared<IllegalInstFault>("zero instruction");
             Rp2 = sp + imm;
         }});
         format CompressedLoad {
@@ -103,7 +104,12 @@ decode QUADRANT default Unknown::unknown() {
                 if (CIMM1 > 0)
                     imm |= ~((uint64_t)0x1F);
             }}, {{
-                assert((RC1 == 0) == (imm == 0));
+                if ((RC1 == 0) != (imm == 0)) {
+                    if (RC1 == 0) {
+                        fault = make_shared<IllegalInstFault>("source reg x0");
+                    } else // imm == 0
+                        fault = make_shared<IllegalInstFault>("immediate = 0");
+                }
                 Rc1_sd = Rc1_sd + imm;
             }});
             0x1: c_addiw({{

diff --git a/src/arch/riscv/isa/formats/fp.isa b/src/arch/riscv/isa/formats/fp.isa
index 1f60b9b70f7403975b2d0380622aac42a4dbf23a..3de0bb2ff1c0ee9a89dab50385c969c3e18701cd 100644 (file)
--- a/src/arch/riscv/isa/formats/fp.isa
+++ b/src/arch/riscv/isa/formats/fp.isa
@@ -56,8 +56,8 @@ def template FloatExecute {{
                 std::fesetround(FE_UPWARD);
                 break;
             case 0x4:
-                panic("Round to nearest, "
-                    "ties to max magnitude not implemented.");
+                // Round to nearest, ties to max magnitude not implemented
+                fault = make_shared<IllegalFrmFault>(ROUND_MODE);
                 break;
             case 0x7: {
                 uint8_t frm = xc->readMiscReg(MISCREG_FRM);
@@ -75,8 +75,8 @@ def template FloatExecute {{
                     std::fesetround(FE_UPWARD);
                     break;
                 case 0x4:
-                    panic("Round to nearest,"
-                        " ties to max magnitude not implemented.");
+                    // Round to nearest, ties to max magnitude not implemented
+                    fault = make_shared<IllegalFrmFault>(ROUND_MODE);
                     break;
                 default:
                     fault = std::make_shared<IllegalFrmFault>(frm);

diff --git a/src/arch/riscv/utility.hh b/src/arch/riscv/utility.hh
index 38109a2087857537a9a4180b1692a5533df385d5..78e9b91a981564b651bd047c091df510dd6654fb 100644 (file)
--- a/src/arch/riscv/utility.hh
+++ b/src/arch/riscv/utility.hh
@@ -48,6 +48,7 @@
 
 #include <cmath>
 #include <cstdint>
+#include <sstream>
 #include <string>
 
 #include "arch/riscv/registers.hh"
@@ -133,8 +134,27 @@ inline std::string
 registerName(RegId reg)
 {
     if (reg.isIntReg()) {
+        if (reg.index() >= NumIntArchRegs) {
+            /*
+             * This should only happen if a instruction is being speculatively
+             * executed along a not-taken branch, and if that instruction's
+             * width was incorrectly predecoded (i.e., it was predecoded as a
+             * full instruction rather than a compressed one or vice versa).
+             * It also should only happen if a debug flag is on that prints
+             * disassembly information, so rather than panic the incorrect
+             * value is printed for debugging help.
+             */
+            std::stringstream str;
+            str << "?? (x" << reg.index() << ')';
+            return str.str();
+        }
         return IntRegNames[reg.index()];
     } else {
+        if (reg.index() >= NumFloatRegs) {
+            std::stringstream str;
+            str << "?? (f" << reg.index() << ')';
+            return str.str();
+        }
         return FloatRegNames[reg.index()];
     }
 }