arch-riscv: fix unintentionally CSR bit overwritten in different mode

Some CSR register is physically shared between different privilige
level. Current implementation of CSR setting only considers to verify
the bits visable in current privilige level, and directly writes the
masked bits back to register. This leads to other bits invisable
to current mode is overwritten and wrong behavior across the modes.
Thus, CSR updating should always keep the bits value for other modes.
e.g. disabling interrupt in S mode with setting
SSTATUS SIE bit will lead to clear MIE bit as well (the interrupt
is disabled unintentionally).

All CSR register sharing same physical register in different mode
may have similar issue. I only fixed some important ones.

The fix is verified in FS.

Jira Issue: https://gem5.atlassian.net/browse/GEM5-860

Change-Id: I34d4766a4b483b5add2c3bbefd28b21b9abf37f6
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/39036
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Ayaz Akram <yazakram@ucdavis.edu>
Maintainer: Jason Lowe-Power <power.jg@gmail.com>

diff --git a/src/arch/riscv/isa/formats/standard.isa b/src/arch/riscv/isa/formats/standard.isa
index a32326f8f61b1d8e37a264394504157d7b458ce8..4ed35c907e30873e605f9d90293d02c6cb3a0de2 100644 (file)
--- a/src/arch/riscv/isa/formats/standard.isa
+++ b/src/arch/riscv/isa/formats/standard.isa
@@ -356,6 +356,7 @@ def template CSRExecute {{
             break;
         }
         auto mask = CSRMasks.find(csr);
+        auto olddata_all = olddata;
         if (mask != CSRMasks.end())
             olddata &= mask->second;
         DPRINTF(RiscvMisc, "Reading CSR %s: %#x\n", CSRData.at(csr).name,
@@ -374,23 +375,32 @@ def template CSRExecute {{
                         fault = std::make_shared<IllegalInstFault>(
                                 error, machInst);
                     } else {
-                        DPRINTF(RiscvMisc, "Writing %#x to CSR %s.\n", data,
+                        auto newdata_all = data;
+                        if (mask != CSRMasks.end()) {
+                            // we must keep those original bits not in mask
+                            // olddata and data only contain thebits visable
+                            // in current privilige level
+                            newdata_all = (olddata_all & (~mask->second))
+                                          | data;
+                        }
+                        DPRINTF(RiscvMisc, "Writing %#x to CSR %s.\n",
+                                newdata_all,
                                 CSRData.at(csr).name);
-                        INTERRUPT oldinterrupt = olddata;
-                        INTERRUPT newinterrupt = data;
                         switch (csr) {
                           case CSR_FCSR:
                             xc->setMiscReg(MISCREG_FFLAGS, bits(data, 4, 0));
                             xc->setMiscReg(MISCREG_FRM, bits(data, 7, 5));
                             break;
                           case CSR_MIP: case CSR_MIE:
-                            if (oldinterrupt.mei != newinterrupt.mei ||
-                                oldinterrupt.mti != newinterrupt.mti ||
-                                oldinterrupt.msi != newinterrupt.msi) {
-                                xc->setMiscReg(CSRData.at(csr).physIndex,data);
+                          case CSR_SIP: case CSR_SIE:
+                          case CSR_UIP: case CSR_UIE:
+                          case CSR_MSTATUS: case CSR_SSTATUS: case CSR_USTATUS:
+                            if (newdata_all != olddata_all) {
+                                xc->setMiscReg(CSRData.at(csr).physIndex,
+                                               newdata_all);
                             } else {
-                                std::string error = "Interrupt m bits are "
-                                                    "read-only\n";
+                                std::string error = "Only bits in mask are "
+                                                    "allowed to be set\n";
                                 fault = std::make_shared<IllegalInstFault>(
                                         error, machInst);
                             }