Fix for issue #589. #661

silabs-oysteink · 2022-09-07T12:11:55Z

Removed instances for mscratchcsw and mscratchcswl CSRs. These CSRs now conditionally operate on mscratch according to the CLIC spec.

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

Removed instances for mscratchcsw and mscratchcswl CSRs. These CSRs now conditioinally operate on mscratch according to the CLIC spec. Signed-off-by: Oystein Knauserud <[email protected]>

silabs-oivind · 2022-09-07T12:44:23Z

sva/cv32e40x_cs_registers_sva.sv

+        |=> $stable(mscratch_rdata));
+    endproperty;
+
+    a_mscratchcswl_mscratch: assert property(p_mscratchcswl_mscratch)


Should we have a similar assertion for mscratchsw ?

I will add that to the E40S, on E40X such an assertions would not be reachable since mcause.mpp can only be of value PRIV_LVL_M.

silabs-oivind · 2022-09-07T12:45:31Z

rtl/cv32e40x_cs_registers.sv

@@ -646,10 +668,10 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
      mintthresh_n             = csr_wdata_int & CSR_MINTTHRESH_MASK;
      mintthresh_we            = 1'b0;

-      mscratchcsw_n            = csr_wdata_int; // todo: isssue 589
+      mscratchcsw_n            = mstatus_n; // mscratchcsw operates conditionally on mscratch


Should this be mscratch_n ? IF not, the comment is confusing.

Good catch, not the first time I type the wrong CSR name. Proves why we need reviews. I'll push a fix

silabs-oivind · 2022-09-07T12:45:42Z

rtl/cv32e40x_cs_registers.sv

      mscratchcsw_we           = 1'b0;

-      mscratchcswl_n           = csr_wdata_int; // todo: isssue 589
+      mscratchcswl_n           = mstatus_n; // mscratchcswl operates conditionally on mscratch


Same as above

Same answer as above

…n by mistake. Signed-off-by: Oystein Knauserud <[email protected]>

silabs-hfegran

From what I understand, and from your explanations, this looks good to me

Silabs-ArjanB · 2022-09-20T12:52:37Z

rtl/cv32e40x_cs_registers.sv

+          // Depending on MPP, we return either mscratch_rdata or rs1 to rd.
+          // Safe to use mcause_rdata here (EX timing), as there is a generic stall of the ID stage
+          // whenever a CSR instruction follows another CSR instruction. Alternative implementation using
+          // a local forward of mcause_rdata is identical (SEC).


Document this case in the controller_bypass module. The currently implemented stall has a todo to make it more specific and this code relies on a stall that therefore might not exist in the future. Note that the 'generic stall' in the bypass module needs to be fixed as we definitely do not want stalls for the common scenarios of back to back CSR reads and back to back CSR writes.

Also explain why this generic stall actually works as that stall is about a CSR instruction in ID and another one in EX/WB, whether the read/write conflict here is about EX vs. WB.

Silabs-ArjanB · 2022-09-22T04:39:11Z

rtl/cv32e40x_cs_registers.sv

+          // Safe to use mcause_rdata here (EX timing), as there is a generic stall of the ID stage
+          // whenever a CSR instruction follows another CSR instruction. Alternative implementation using
+          // a local forward of mcause_rdata is identical (SEC).
+          if (mcause_rdata.mpp != PRIV_LVL_M) begin


The github issue asked to take riscv/riscv-fast-interrupt#241 into account, which was not done. Please follow the pseudo code from https://github.com/riscv/riscv-fast-interrupt/blob/master/clic.adoc#optional-scratch-swap-csr-xscratchcsw-for-multiple-privilege-modes that already is based on that pull request. (This change should be SEC clean)

Silabs-ArjanB · 2022-09-22T04:52:18Z

rtl/cv32e40x_cs_registers.sv

-            mscratchcsw_we = 1'b1;
+            // mscratchcsw operates on mscratch
+            // Writing only when mcause.mpp != PRIV_LVL_M
+            if (mcause_rdata.mpp != PRIV_LVL_M) begin


Use mstatus, also in comment (should be SEC clean)

Silabs-ArjanB · 2022-09-22T04:53:23Z

rtl/cv32e40x_cs_registers.sv

@@ -1356,6 +1353,15 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
  // if no interrupt is pending.
  assign mnxti_rdata        = mnxti_irq_pending_i ? {mtvt_addr_o, mnxti_irq_id_i, 2'b00} : 32'h00000000;

+  // mscratchcsw_rdata breaks the regular convension for CSrs. Read data depend on mcause.mpp


CSrs -> CSRs
depend -> depends
mcause -> mstatus (update in all related comments and RTL code)

Silabs-ArjanB · 2022-09-22T04:59:45Z

rtl/cv32e40x_cs_registers.sv

+  // mscratchcsw_rdata breaks the regular convension for CSrs. Read data depend on mcause.mpp
+  // mscratch_rdata is returned if mcause.mpp differs from PRIV_LVL_M, otherwise rs1 is returned.
+  // This signal is only used by RVFI, and has WB timing (rs1 comes from ex_wb_pipe_i.csr_wdata, flopped version of id_ex_pipe.alu_operand_a)
+  assign mscratchcsw_rdata  = (mcause_rdata.mpp != PRIV_LVL_M) ? mscratch_rdata : ex_wb_pipe_i.csr_wdata;


This will not return the correct value for machine mode at other cycles than while just handling a related CSR instruction because ex_wb_pipe_i.csr_wdata might be completely unrelated to this CSR. Therefore this expression cannot be used for checking this CSR against the ISS in each cycle.

Silabs-ArjanB · 2022-09-22T04:59:52Z

rtl/cv32e40x_cs_registers.sv

+  // mscratchcsw_rdata breaks the regular convension for CSrs. Read data depend on mcause.mpp
+  // mscratch_rdata is returned if mcause.mpp differs from PRIV_LVL_M, otherwise rs1 is returned.
+  // This signal is only used by RVFI, and has WB timing (rs1 comes from ex_wb_pipe_i.csr_wdata, flopped version of id_ex_pipe.alu_operand_a)
+  assign mscratchcsw_rdata  = (mcause_rdata.mpp != PRIV_LVL_M) ? mscratch_rdata : ex_wb_pipe_i.csr_wdata;


Use mstatus

Silabs-ArjanB · 2022-09-22T05:00:17Z

rtl/cv32e40x_cs_registers.sv

+
+  // mscratchcswl_rdata breaks the regular convension for CSrs. Read data depend on mcause.pil and mintstatus.mil.
+  // This signal is only used by RVFI, and has WB timing (rs1 comes from ex_wb_pipe_i.csr_wdata, flopped version of id_ex_pipe.alu_operand_a)
+  assign mscratchcswl_rdata = ((mcause_rdata.mpil == '0) != (mintstatus_rdata.mil == 0)) ? mscratch_rdata : ex_wb_pipe_i.csr_wdata;


Not accurate because of the csr_wdata usage

Silabs-ArjanB · 2022-09-22T05:01:57Z

sva/cv32e40x_cs_registers_sva.sv

-   input privlvl_t    priv_lvl_rdata
+   input privlvl_t    priv_lvl_rdata,
+   input logic [31:0] mscratch_rdata,
+   input mcause_t     mcause_rdata,


use mstatus instead of mcause everywhere related to mscratchcsw

Silabs-ArjanB · 2022-09-22T05:07:21Z

sva/cv32e40x_cs_registers_sva.sv

@@ -90,6 +96,38 @@ module cv32e40x_cs_registers_sva

    a_htrap_interrupt_level: assert property(p_htrap_interrupt_level)
      else `uvm_error("cs_registers", "Horizontal trap taken caused interrupt level to change");
+
+    // Check that mscratch do not update due to mscratchcsw if the conditions are not right
+    property p_mscratchcsw_mscratch;


These assertions are self fullfilling as they basically just match the RTL. Could an alternative implementation that is closer to the actual CLIC specification be used here instead? I.e. use the for this one use a mstatus bypass instead of the mcause/mstatus rdata which has the dangerous timing.

Same remark for all assertions; use the 'bypass' (i.e. _q value of the CSR or to be written value of the CSR)

Fix for issue openhwgroup#589.

a31e635

Removed instances for mscratchcsw and mscratchcswl CSRs. These CSRs now conditioinally operate on mscratch according to the CLIC spec. Signed-off-by: Oystein Knauserud <[email protected]>

silabs-oysteink requested review from silabs-oivind and silabs-hfegran September 7, 2022 12:12

silabs-oivind reviewed Sep 7, 2022

View reviewed changes

Updated nextvalue of mscratchcsw/l as they were hooked up to mstatus_…

53948fd

…n by mistake. Signed-off-by: Oystein Knauserud <[email protected]>

silabs-oivind approved these changes Sep 7, 2022

View reviewed changes

silabs-hfegran reviewed Sep 7, 2022

View reviewed changes

silabs-oysteink merged commit 118a05c into openhwgroup:master Sep 8, 2022

Silabs-ArjanB reviewed Sep 21, 2022

View reviewed changes

Silabs-ArjanB reviewed Sep 22, 2022

View reviewed changes

Silabs-ArjanB mentioned this pull request Sep 22, 2022

Additional feedback on https://github.com/openhwgroup/cv32e40x/pull/661 #668

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for issue #589. #661

Fix for issue #589. #661

silabs-oysteink commented Sep 7, 2022 •

edited

Loading

silabs-oivind Sep 7, 2022

silabs-oysteink Sep 7, 2022

silabs-oivind Sep 7, 2022

silabs-oysteink Sep 7, 2022

silabs-oivind Sep 7, 2022

silabs-oysteink Sep 7, 2022

silabs-hfegran left a comment

Silabs-ArjanB Sep 20, 2022 •

edited

Loading

Silabs-ArjanB Sep 22, 2022

Silabs-ArjanB Sep 22, 2022

Silabs-ArjanB Sep 22, 2022

Silabs-ArjanB Sep 22, 2022

Silabs-ArjanB Sep 22, 2022

Silabs-ArjanB Sep 22, 2022

Silabs-ArjanB Sep 22, 2022

Silabs-ArjanB Sep 22, 2022

Silabs-ArjanB Sep 22, 2022

Fix for issue #589. #661

Fix for issue #589. #661

Conversation

silabs-oysteink commented Sep 7, 2022 • edited Loading

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

silabs-hfegran left a comment

Choose a reason for hiding this comment

Silabs-ArjanB Sep 20, 2022 • edited Loading

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

silabs-oysteink commented Sep 7, 2022 •

edited

Loading

Silabs-ArjanB Sep 20, 2022 •

edited

Loading