Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for issue #589. #661

Merged
merged 2 commits into from
Sep 8, 2022
Merged

Fix for issue #589. #661

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a31e635
Fix for issue #589.
silabs-oysteink Sep 7, 2022
53948fd
Updated nextvalue of mscratchcsw/l as they were hooked up to mstatus_…
silabs-oysteink Sep 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 50 additions & 43 deletions rtl/cv32e40x_cs_registers.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,10 +202,10 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
logic [31:0] mintthresh_q, mintthresh_n, mintthresh_rdata;
logic mintthresh_we;

logic [31:0] mscratchcsw_q, mscratchcsw_n, mscratchcsw_rdata;
logic [31:0] mscratchcsw_n, mscratchcsw_rdata;
logic mscratchcsw_we;

logic [31:0] mscratchcswl_q, mscratchcswl_n, mscratchcswl_rdata;
logic [31:0] mscratchcswl_n, mscratchcswl_rdata;
logic mscratchcswl_we;

logic [31:0] mclicbase_q, mclicbase_n, mclicbase_rdata;
Expand Down Expand Up @@ -407,7 +407,18 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
// mscratchcsw: Scratch Swap for Multiple Privilege Modes
CSR_MSCRATCHCSW: begin
if (SMCLIC) begin
csr_rdata_int = mscratchcsw_rdata;
// CLIC spec 13.2
// 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).
Copy link
Contributor

@Silabs-ArjanB Silabs-ArjanB Sep 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (mcause_rdata.mpp != PRIV_LVL_M) begin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

// Return mscratch for writing to GPR
csr_rdata_int = mscratch_rdata;
end else begin
// return rs1 for writing to GPR
csr_rdata_int = id_ex_pipe_i.alu_operand_a;
end
end else begin
csr_rdata_int = '0;
illegal_csr_read = 1'b1;
Expand All @@ -417,7 +428,18 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
// mscratchcswl: Scratch Swap for Interrupt Levels
CSR_MSCRATCHCSWL: begin
if (SMCLIC) begin
csr_rdata_int = mscratchcswl_rdata;
// CLIC spec 14.1
// Depending on mcause.pil and mintstatus.mil, either mscratch or rs1 is returned to rd.
// Safe to use mcause_rdata and mintstatus_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 and mintstatus_rdata is identical (SEC).
if ((mcause_rdata.mpil == '0) != (mintstatus_rdata.mil == 0)) begin
// Return mscratch for writing to GPR
csr_rdata_int = mscratch_rdata;
end else begin
// return rs1 for writing to GPR
csr_rdata_int = id_ex_pipe_i.alu_operand_a;
end
end else begin
csr_rdata_int = '0;
illegal_csr_read = 1'b1;
Expand Down Expand Up @@ -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 = mscratch_n; // mscratchcsw operates conditionally on mscratch
mscratchcsw_we = 1'b0;

mscratchcswl_n = csr_wdata_int; // todo: isssue 589
mscratchcswl_n = mscratch_n; // mscratchcswl operates conditionally on mscratch
mscratchcswl_we = 1'b0;

mie_n = '0;
Expand Down Expand Up @@ -827,13 +849,22 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;

CSR_MSCRATCHCSW: begin
if (SMCLIC) begin
mscratchcsw_we = 1'b1;
// mscratchcsw operates on mscratch
// Writing only when mcause.mpp != PRIV_LVL_M
if (mcause_rdata.mpp != PRIV_LVL_M) begin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

mscratchcsw_we = 1'b1;
mscratch_we = 1'b1;
end
end
end

CSR_MSCRATCHCSWL: begin
if (SMCLIC) begin
mscratchcswl_we = 1'b1;
// mscratchcswl operates on mscratch
if ((mcause_rdata.mpil == '0) != (mintstatus_rdata.mil == '0)) begin
mscratchcswl_we = 1'b1;
mscratch_we = 1'b1;
end
end
end

Expand Down Expand Up @@ -1244,34 +1275,6 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
.rd_data_o ( mintthresh_q )
);

cv32e40x_csr
#(
.WIDTH (32),
.RESETVALUE (32'h0)
)
mscratchcsw_csr_i
(
.clk ( clk ),
.rst_n ( rst_n ),
.wr_data_i ( mscratchcsw_n ),
.wr_en_i ( mscratchcsw_we ),
.rd_data_o ( mscratchcsw_q )
);

cv32e40x_csr
#(
.WIDTH (32),
.RESETVALUE (32'h0)
)
mscratchcswl_csr_i
(
.clk ( clk ),
.rst_n ( rst_n ),
.wr_data_i ( mscratchcswl_n ),
.wr_en_i ( mscratchcswl_we ),
.rd_data_o ( mscratchcswl_q )
);

cv32e40x_csr
#(
.WIDTH (32),
Expand Down Expand Up @@ -1322,10 +1325,6 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;

assign mintthresh_q = 32'h0;

assign mscratchcsw_q = 32'h0;

assign mscratchcswl_q = 32'h0;

assign mclicbase_q = 32'h0;

end
Expand All @@ -1346,8 +1345,6 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
assign mtvt_rdata = mtvt_q;
assign mintstatus_rdata = mintstatus_q;
assign mintthresh_rdata = mintthresh_q;
assign mscratchcsw_rdata = mscratchcsw_q;
assign mscratchcswl_rdata = mscratchcswl_q;
assign mclicbase_rdata = mclicbase_q;
assign mie_rdata = mie_q;

Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use mstatus


// 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not accurate because of the csr_wdata usage


assign mip_rdata = mip_i;
assign misa_rdata = MISA_VALUE;
assign mstatush_rdata = 32'h0;
Expand Down Expand Up @@ -1749,6 +1755,7 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
// Some signals are unused on purpose (typically they are used by RVFI code). Use them here for easier LINT waiving.

assign unused_signals = tselect_we | tinfo_we | tcontrol_we | mstatush_we | misa_we | mip_we | mvendorid_we |
marchid_we | mimpid_we | mhartid_we | mconfigptr_we | mtval_we | (|mnxti_n);
marchid_we | mimpid_we | mhartid_we | mconfigptr_we | mtval_we | (|mnxti_n) | mscratchcsw_we | mscratchcswl_we |
(|mscratchcsw_rdata) | (|mscratchcswl_rdata) | (|mscratchcsw_n) | (|mscratchcswl_n);

endmodule
40 changes: 39 additions & 1 deletion sva/cv32e40x_cs_registers_sva.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,13 @@ module cv32e40x_cs_registers_sva
input logic clic_pa_valid_o,
input mintstatus_t mintstatus_rdata,
input privlvl_t priv_lvl_n,
input privlvl_t priv_lvl_rdata
input privlvl_t priv_lvl_rdata,
input logic [31:0] mscratch_rdata,
input mcause_t mcause_rdata,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use mstatus instead of mcause everywhere related to mscratchcsw

csr_num_e csr_waddr,
input logic mscratch_we,
input logic instr_valid,
input csr_opcode_e csr_op

);

Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@(posedge clk) disable iff (!rst_n)
( ex_wb_pipe_i.csr_en && (csr_waddr == CSR_MSCRATCHCSW) && (mcause_rdata.mpp == PRIV_LVL_M)
|=> $stable(mscratch_rdata));
endproperty;

a_mscratchcsw_mscratch: assert property(p_mscratchcsw_mscratch)
else `uvm_error("cs_registers", "Mscratch not stable after mscratwchsw with mpp=M");

// Check that mscratch do not update due to mscratchcswl if the conditions are not right
property p_mscratchcswl_mscratch;
@(posedge clk) disable iff (!rst_n)
( ex_wb_pipe_i.csr_en && (csr_waddr == CSR_MSCRATCHCSWL) && !((mcause_rdata.mpil == '0) != (mintstatus_rdata.mil == '0))
|=> $stable(mscratch_rdata));
endproperty;

a_mscratchcswl_mscratch: assert property(p_mscratchcswl_mscratch)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a similar assertion for mscratchsw ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else `uvm_error("cs_registers", "Mscratch not stable after mscratwchswl");

// Check that mscratch is written by mscratchcswl when the conditions are right
property p_mscratchcswl_mscratch_we;
@(posedge clk) disable iff (!rst_n)
( ex_wb_pipe_i.csr_en && (csr_waddr == CSR_MSCRATCHCSWL) && ((mcause_rdata.mpil == '0) != (mintstatus_rdata.mil == '0))
&& (csr_op != CSR_OP_READ) && instr_valid
|-> mscratch_we);
endproperty;

a_mscratchcswl_mscratch_we: assert property(p_mscratchcswl_mscratch_we)
else `uvm_error("cs_registers", "Mscratch not written by mscratchcswl");

end
endmodule