Updated aliasing of mpp and mpie between mcause and mstatus. #738

silabs-oysteink · 2022-12-20T05:42:52Z

RVFI didn't always get the correct write values, and the aliasing was only working from mcause to mstatus and not the other way.

The three bits are now duplicated in both mcause and mstatus, and are always written simultaneously. Assertions are added to check that mcause and mstatus always write at the same time, and that the *_rdata.mpp/mpie fields are always the same.

SEC clean

Signed-off-by: Oystein Knauserud [email protected]

RVFI didn't always get the correct write values, and the aliasing was only working from mcause to mstatus and not the other way. The three bits are now duplicated in both mcause and mstatus, and are always written simultaneously. Assertions are added to check that mcause and mstatus always write at the same time, and that the *_rdata.mpp/mpie fields are always the same. SEC clean Signed-off-by: Oystein Knauserud <[email protected]>

silabs-oysteink · 2022-12-20T05:44:23Z

rtl/cv32e40x_cs_registers.sv

@@ -725,11 +735,14 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
      mcause_n                 = '{
                                    irq:            csr_wdata_int[31],
                                    minhv:          csr_wdata_int[30],
+                                    mpp:            mstatus_mpp_resolve(mstatus_rdata.mpp, csr_wdata_int[MCAUSE_MPP_BIT_HIGH:MCAUSE_MPP_BIT_LOW]),


Could create a separate mcause_mpp_resolve, or rename the existing one to just 'mpp_resolve'.

Yes, make a separate mcause_mpp_resolve

and use mcause_rdata.mpp,

silabs-oysteink · 2022-12-20T05:48:09Z

rtl/cv32e40x_cs_registers.sv

@@ -1459,7 +1521,7 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
  // Signal when an interrupt may become enabled due to a CSR write
  generate
    if (SMCLIC) begin : smclic_irq_en
-      assign csr_irq_enable_write_o = mstatus_we || priv_lvl_we || mintthresh_we || mintstatus_we;
+      assign csr_irq_enable_write_o = (mstatus_we && !ctrl_fsm_i.csr_clear_minhv) || priv_lvl_we || mintthresh_we || mintstatus_we;


Needed to be SEC clean. Not needed functionally, as a clearing of minhv will only touch that bit and never update any interrupt enables. Could keep it as it was, or modify to not raise the signal on write to mstatus where we know that mie will not be written?

I prefer the non-SEC clean variant. The only negative effect could be an unneeded stall, right?

Yes, it would cause an extra stall, but I believe the scenario creating this isn't very common. I will remove the non-SEC code in my final push.

Removed check for clear_minhv, resulting PR is not SEC clean.

Silabs-ArjanB · 2022-12-20T06:37:47Z

rtl/cv32e40x_cs_registers.sv

@@ -725,11 +735,14 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
      mcause_n                 = '{
                                    irq:            csr_wdata_int[31],
                                    minhv:          csr_wdata_int[30],
+                                    mpp:            mstatus_mpp_resolve(mstatus_rdata.mpp, csr_wdata_int[MCAUSE_MPP_BIT_HIGH:MCAUSE_MPP_BIT_LOW]),


Yes, make a separate mcause_mpp_resolve

Silabs-ArjanB · 2022-12-20T06:45:14Z

rtl/cv32e40x_cs_registers.sv

@@ -725,11 +735,14 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
      mcause_n                 = '{
                                    irq:            csr_wdata_int[31],
                                    minhv:          csr_wdata_int[30],
+                                    mpp:            mstatus_mpp_resolve(mstatus_rdata.mpp, csr_wdata_int[MCAUSE_MPP_BIT_HIGH:MCAUSE_MPP_BIT_LOW]),


and use mcause_rdata.mpp,

Silabs-ArjanB · 2022-12-20T06:47:40Z

rtl/cv32e40x_cs_registers.sv

@@ -862,14 +882,14 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;

            // Writes to mnxti also writes to mstatus
            mstatus_we = 1'b1;
+            mcause_we  = 1'b1;


Fix above comment

Also fix comment on line 887 and explain whyit is now okay to write mcause unconditionally.

Silabs-ArjanB · 2022-12-20T06:56:55Z

rtl/cv32e40x_cs_registers.sv

@@ -1098,6 +1142,10 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
        mstatus_n.mprv = (privlvl_t'(dcsr_rdata.prv) == PRIV_LVL_M) ? mstatus_rdata.mprv : 1'b0;
        mstatus_we     = 1'b1;

+        // Not really needed, but allows for asserting mstatus_we == mcause_we to check aliasing formally


This section should only be done for SMCLIC

Silabs-ArjanB · 2022-12-20T06:58:53Z

rtl/cv32e40x_cs_registers.sv

@@ -1037,7 +1071,15 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
          mepc_n         = ctrl_fsm_i.pipe_pc;
          mepc_we        = 1'b1;

-          mcause_n       = ctrl_fsm_i.csr_cause;
+          // Save relevant fields from controller to mcause


Only for SMCLIC

Moved relevant fields into "if (SMCLIC)"

Silabs-ArjanB · 2022-12-20T07:02:00Z

rtl/cv32e40x_cs_registers.sv

+      cv32e40x_csr
+      #(
+        .WIDTH      (32),
+        .RESETVALUE (32'd0)


Introduce MCAUSE_BASIC_RESET_VAL and MCAUSE_CLIC_RESET_VAL (just like for MTVEC)

Silabs-ArjanB · 2022-12-20T07:06:43Z

rtl/cv32e40x_cs_registers.sv

@@ -1459,7 +1521,7 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
  // Signal when an interrupt may become enabled due to a CSR write
  generate
    if (SMCLIC) begin : smclic_irq_en
-      assign csr_irq_enable_write_o = mstatus_we || priv_lvl_we || mintthresh_we || mintstatus_we;
+      assign csr_irq_enable_write_o = (mstatus_we && !ctrl_fsm_i.csr_clear_minhv) || priv_lvl_we || mintthresh_we || mintstatus_we;


I prefer the non-SEC clean variant. The only negative effect could be an unneeded stall, right?

Silabs-ArjanB · 2022-12-20T07:10:06Z

sva/cv32e40x_cs_registers_sva.sv

+      );
+    endproperty;
+    a_mstatus_mcause_we: assert property(p_mstatus_mcause_we)
+      else `uvm_error("cs_registers", "Mstatus.mpp and mstatus.mpie not written at the same time")


Fix error message

Signed-off-by: Oystein Knauserud <[email protected]>

silabs-oysteink marked this pull request as draft December 20, 2022 05:43

silabs-oysteink requested a review from Silabs-ArjanB December 20, 2022 05:43

silabs-oysteink commented Dec 20, 2022

View reviewed changes

Silabs-ArjanB requested changes Dec 20, 2022

View reviewed changes

Silabs-ArjanB added the Component:RTL For issues in the RTL (e.g. for files in the rtl directory) label Dec 20, 2022

Updates after review.

aad8cc7

Signed-off-by: Oystein Knauserud <[email protected]>

silabs-oysteink marked this pull request as ready for review December 20, 2022 09:27

silabs-oysteink requested a review from Silabs-ArjanB December 20, 2022 09:27

Silabs-ArjanB approved these changes Dec 20, 2022

View reviewed changes

Silabs-ArjanB merged commit 1af01ad into openhwgroup:master Dec 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated aliasing of mpp and mpie between mcause and mstatus. #738

Updated aliasing of mpp and mpie between mcause and mstatus. #738

silabs-oysteink commented Dec 20, 2022

silabs-oysteink Dec 20, 2022

Silabs-ArjanB Dec 20, 2022

Silabs-ArjanB Dec 20, 2022

silabs-oysteink Dec 20, 2022

silabs-oysteink Dec 20, 2022

Silabs-ArjanB Dec 20, 2022

silabs-oysteink Dec 20, 2022

silabs-oysteink Dec 20, 2022

Silabs-ArjanB Dec 20, 2022

Silabs-ArjanB Dec 20, 2022

Silabs-ArjanB Dec 20, 2022

Silabs-ArjanB Dec 20, 2022

silabs-oysteink Dec 20, 2022

Silabs-ArjanB Dec 20, 2022

silabs-oysteink Dec 20, 2022

Silabs-ArjanB Dec 20, 2022

silabs-oysteink Dec 20, 2022

Silabs-ArjanB Dec 20, 2022

silabs-oysteink Dec 20, 2022

Silabs-ArjanB Dec 20, 2022

Silabs-ArjanB Dec 20, 2022

silabs-oysteink Dec 20, 2022

Updated aliasing of mpp and mpie between mcause and mstatus. #738

Updated aliasing of mpp and mpie between mcause and mstatus. #738

Conversation

silabs-oysteink commented Dec 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment