Skip to content

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

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

Merged
merged 2 commits into from
Dec 20, 2022
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
46d08cd
Updated aliasing of mpp and mpie between mcause and mstatus.
silabs-oysteink Dec 20, 2022
aad8cc7
Updates after review.
silabs-oysteink Dec 20, 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
130 changes: 96 additions & 34 deletions rtl/cv32e40x_cs_registers.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,15 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
logic csr_wr_in_wb;
logic jvt_wr_in_wb;

// Special we signal for aliased writes between mstatus and mcause.
// Only used for explicit writes to either mcause or mstatus, to signal that
// mpp and mpie of the aliased CSR should be written.
// All implicit writes (upon taking exceptions etc) handle the aliasing by also
// writing mcause.mpp/mpie to mstatus and vice versa.
logic mcause_alias_we;
logic mstatus_alias_we;


// Performance Counter Signals
logic [31:0] [63:0] mhpmcounter_q; // Performance counters
logic [31:0] [63:0] mhpmcounter_n; // Performance counters next value
Expand Down Expand Up @@ -681,6 +690,7 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;

mstatush_n = mstatush_rdata; // Read only
mstatush_we = 1'b0;
mstatus_alias_we = 1'b0;

misa_n = misa_rdata; // Read only
misa_we = 1'b0;
Expand Down Expand Up @@ -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]),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, make a separate mcause_mpp_resolve

Copy link
Contributor

Choose a reason for hiding this comment

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

and use mcause_rdata.mpp,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

mpie: csr_wdata_int[MCAUSE_MPIE_BIT],
mpil: csr_wdata_int[23:16],
exception_code: csr_wdata_int[10:0],
default: 'b0
};
mcause_we = 1'b0;
mcause_alias_we = 1'b0;
end else begin // !SMCLIC
mtvec_n.mode = csr_mtvec_init_i ? mtvec_rdata.mode : mtvec_mode_clint_resolve(mtvec_rdata.mode, csr_wdata_int[MTVEC_MODE_BIT_HIGH:MTVEC_MODE_BIT_LOW]);

Expand Down Expand Up @@ -765,6 +778,7 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
default: 'b0
};
mcause_we = 1'b0;
mcause_alias_we = 1'b0;
end

mtval_n = mtval_rdata; // Read-only
Expand Down Expand Up @@ -799,6 +813,12 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
// mstatus
CSR_MSTATUS: begin
mstatus_we = 1'b1;
// CLIC mode is assumed when SMCLIC = 1
// For CLIC, a write to mstatus.mpp or mstatus.mpie will write to the
// corresponding bits in mstatus as well.
if (SMCLIC) begin
mcause_alias_we = 1'b1;
end
end

CSR_MISA: begin
Expand Down Expand Up @@ -843,7 +863,7 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
// For CLIC, a write to mcause.mpp or mcause.mpie will write to the
// corresponding bits in mstatus as well.
if (SMCLIC) begin
mstatus_we = 1'b1;
mstatus_alias_we = 1'b1;
end
end

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

Choose a reason for hiding this comment

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

Fix above comment

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


// Writes to mintstatus.mil and mcause depend on the current state of
// clic interrupts AND the type of CSR instruction used.
// Side effects occur when there is an actual write to the mstatus CSR
// This is already coded into the csr_we_int/mnxti_we
if (mnxti_irq_pending_i) begin
mintstatus_we = 1'b1;
mcause_we = 1'b1;
end
end
end
Expand Down Expand Up @@ -963,30 +983,44 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
end

// CSR side effects from other CSRs
// All write enables are already calculated at this point

// CLIC mode is assumed when SMCLIC = 1
if (SMCLIC) begin
if (mnxti_we) begin
// Mstatus is written as part of an mnxti access
// Make sure we alias the mpp/mpie to mcause
mcause_n = mcause_rdata;
mcause_n.mpie = mstatus_n.mpie;
mcause_n.mpp = mstatus_n.mpp;

// mintstatus and mcause are updated if an actual mstatus write happens and
// a higher level non-shv interrupt is pending.
// This is already decoded into the respective _we signals below.
if (mintstatus_we) begin
mintstatus_n.mil = mnxti_irq_level_i;
end
if (mcause_we) begin
mcause_n = mcause_rdata;
if (mnxti_irq_pending_i) begin
mcause_n.irq = 1'b1;
mcause_n.exception_code = {1'b0, 10'(mnxti_irq_id_i)};
end
end else if (mcause_we) begin
end else if (mstatus_alias_we) begin
// In CLIC mode, writes to mcause.mpp/mpie is aliased to mstatus.mpp/mpie
// All other mstatus bits are preserved
mstatus_n = mstatus_rdata; // Preserve all fields

// Write mpie and mpp as aliased through mcause
mstatus_n.mpie = csr_wdata_int[MCAUSE_MPIE_BIT];
mstatus_n.mpp = mstatus_mpp_resolve(mstatus_rdata.mpp, csr_wdata_int[MCAUSE_MPP_BIT_HIGH:MCAUSE_MPP_BIT_LOW]);
mstatus_n.mpie = mcause_n.mpie;
mstatus_n.mpp = mcause_n.mpp;

mstatus_we = 1'b1;
end else if (mcause_alias_we) begin
// In CLIC mode, writes to mstatus.mpp/mpie is aliased to mcause.mpp/mpie
// All other mcause bits are preserved
mcause_n = mcause_rdata;
mcause_n.mpie = mstatus_n.mpie;
mcause_n.mpp = mstatus_n.mpp;

mcause_we = 1'b1;
end
// The CLIC pointer address should always be output for an access to MNXTI,
// but will only contain a nonzero value if a CLIC interrupt is actually pending
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Only for SMCLIC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved relevant fields into "if (SMCLIC)"

mcause_n.irq = ctrl_fsm_i.csr_cause.irq;
mcause_n.exception_code = ctrl_fsm_i.csr_cause.exception_code;
mcause_n.minhv = ctrl_fsm_i.csr_cause.minhv;
// Save aliased values for mpp and mpie
mcause_n.mpp = mstatus_n.mpp;
mcause_n.mpie = mstatus_n.mpie;

mcause_we = 1'b1;


if (SMCLIC) begin
Expand All @@ -1059,8 +1101,6 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
end else begin
mcause_n.mpil = '0;
end
mcause_we = 1'b1;

end
end //ctrl_fsm_i.csr_save_cause

Expand All @@ -1079,11 +1119,15 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
mintstatus_n.mil = mcause_rdata.mpil;
mintstatus_we = 1'b1;

// Save aliased values for mpp and mpie
mcause_n = mcause_rdata;
mcause_n.mpp = mstatus_n.mpp;
mcause_n.mpie = mstatus_n.mpie;
mcause_we = 1'b1;

if (ctrl_fsm_i.csr_restore_mret_ptr) begin
// Clear mcause.minhv if the mret also caused a successful CLIC pointer fetch
mcause_n = mcause_rdata;
mcause_n.minhv = 1'b0;
mcause_we = 1'b1;
end
end
end //ctrl_fsm_i.csr_restore_mret
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

This section should only be done for SMCLIC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

mcause_n = mcause_rdata;
mcause_we = 1'b1;

end //ctrl_fsm_i.csr_restore_dret

// Clear mcause.minhv on successful CLIC pointer fetches
Expand All @@ -1110,6 +1158,10 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
mcause_n = mcause_rdata;
mcause_n.minhv = 1'b0;
mcause_we = 1'b1;

// Not really needed, but allows for asserting mstatus_we == mcause_we to check aliasing formally
mstatus_n = mstatus_rdata;
mstatus_we = 1'b1;
end
end
default:;
Expand Down Expand Up @@ -1260,24 +1312,26 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
.rd_data_o ( mstatus_q )
);

cv32e40x_csr
#(
.WIDTH (32),
.RESETVALUE (32'd0)
)
mcause_csr_i (
.clk ( clk ),
.rst_n ( rst_n ),
.wr_data_i ( mcause_n ),
.wr_en_i ( mcause_we ),
.rd_data_o ( mcause_q )
);


generate
if (SMCLIC) begin : smclic_csrs

assign mie_q = 32'h0; // CLIC mode is assumed when SMCLIC = 1

cv32e40x_csr
#(
.WIDTH (32),
.RESETVALUE (MCAUSE_RESET_VAL)
)
mcause_csr_i (
.clk ( clk ),
.rst_n ( rst_n ),
.wr_data_i ( mcause_n ),
.wr_en_i ( mcause_we ),
.rd_data_o ( mcause_q )
);

cv32e40x_csr
#(
.WIDTH (32),
Expand Down Expand Up @@ -1337,6 +1391,19 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;

end else begin : basic_mode_csrs

cv32e40x_csr
#(
.WIDTH (32),
.RESETVALUE (32'd0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Introduce MCAUSE_BASIC_RESET_VAL and MCAUSE_CLIC_RESET_VAL (just like for MTVEC)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

)
mcause_csr_i (
.clk ( clk ),
.rst_n ( rst_n ),
.wr_data_i ( mcause_n ),
.wr_en_i ( mcause_we ),
.rd_data_o ( mcause_q )
);

cv32e40x_csr
#(
.WIDTH (32),
Expand Down Expand Up @@ -1432,14 +1499,9 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
// dcsr_rdata factors in the flop outputs and the nmip bit from the controller
assign dcsr_rdata = {dcsr_q[31:4], ctrl_fsm_i.pending_nmi, dcsr_q[2:0]};

generate
if (SMCLIC) begin : smclic_rdata
// mcause.mpp is alias of mstatus.mpp, mcause.mpie is alias of mstatus.mpie
assign mcause_rdata = {mcause_q[31:30], mstatus_q.mpp, mstatus_q.mpie, mcause_q[26:0]};
end else begin : basic_mode_rdata
assign mcause_rdata = mcause_q;
end
endgenerate

assign mcause_rdata = mcause_q;


assign csr_rdata_o = csr_rdata_int;

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

end else begin : basic_irq_en
assign csr_irq_enable_write_o = mie_we || mstatus_we || priv_lvl_we;
end
Expand Down
4 changes: 4 additions & 0 deletions rtl/include/cv32e40x_pkg.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,10 @@ parameter mstatus_t MSTATUS_RESET_VAL = '{
zero0 : 'b0,
default : 'b0};

parameter mcause_t MCAUSE_RESET_VAL = '{
mpp : PRIV_LVL_M,
default: 'b0};

parameter logic [31:0] TDATA1_RST_VAL = {
TTYPE_MCONTROL6, // type : address/data match
1'b1, // dmode : access from D mode only
Expand Down
37 changes: 28 additions & 9 deletions sva/cv32e40x_cs_registers_sva.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ module cv32e40x_cs_registers_sva
input privlvl_t priv_lvl_rdata,
input logic [31:0] mscratch_rdata,
input mcause_t mcause_rdata,
input mstatus_t mstatus_rdata,
csr_num_e csr_waddr,
input logic mscratch_we,
input logic instr_valid,
Expand All @@ -59,7 +60,10 @@ module cv32e40x_cs_registers_sva
input logic [31:0] mepc_n,
input logic [24:0] mtvec_addr_o,
input logic [31:0] dpc_n,
input logic dpc_we
input logic dpc_we,
input mstatus_t mstatus_n,
input mcause_t mcause_n,
input logic mstatus_we

);

Expand Down Expand Up @@ -135,6 +139,29 @@ module cv32e40x_cs_registers_sva
a_mscratchcswl_mscratch_we: assert property(p_mscratchcswl_mscratch_we)
else `uvm_error("cs_registers", "Mscratch not written by mscratchcswl");

property p_mstatus_mcause_we;
@(posedge clk) disable iff (!rst_n)
(
1'b1
|->
mstatus_we == mcause_we
);
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")
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix error message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


property p_mcause_mstatus_alias;
@(posedge clk) disable iff (!rst_n)
(
1'b1
|->
(mstatus_rdata.mpp == mcause_rdata.mpp) &&
(mstatus_rdata.mpie == mcause_rdata.mpie)
);
endproperty;
a_mcause_mstatus_alias: assert property(p_mcause_mstatus_alias)
else `uvm_error("cs_registers", "mcause.mpp and mcause.mpie not aliased correctly")

end

// Check that no csr instruction can be in WB during sleep when ctrl_fsm.halt_limited_wb is set
Expand All @@ -146,14 +173,6 @@ module cv32e40x_cs_registers_sva
a_halt_limited_wb: assert property(p_halt_limited_wb)
else `uvm_error("cs_registers", "CSR in WB while halt_limited_wb is set");

// Check that mcause is not written when an mret or dret is in WB
property p_mret_dret_wb_mcause_write;
@(posedge clk) disable iff (!rst_n)
( (ctrl_fsm_i.csr_restore_mret || ctrl_fsm_i.csr_restore_dret) && !ctrl_fsm_i.csr_clear_minhv |-> !mcause_we);
endproperty;

a_mret_dret_wb_mcause_write: assert property(p_mret_dret_wb_mcause_write)
else `uvm_error("cs_registers", "mcause written when MRET or DRET is in WB.");

// Check csr_clear_minhv cannot happen at the same time as csr_save_cause or csr_restore_dret (would cause mcause_we conflict)
property p_minhv_unique;
Expand Down