Skip to content

Mcontrol6 #702

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 6 commits into from
Nov 28, 2022
Merged

Mcontrol6 #702

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
17fe54f
Added the following features for trigger type 6 (mcontrol6):
silabs-oysteink Nov 23, 2022
b793ab9
Removed unused output from load_store_unit.
silabs-oysteink Nov 23, 2022
bc29151
Updated tdata1.match WARL resolve to fall back to the value 0 if writ…
silabs-oysteink Nov 24, 2022
e2bf78c
Reintroduced tdata*_rdata in debug_triggers.
silabs-oysteink Nov 25, 2022
4aa496f
Calculating LSU address match boundaries based on BE instead of addre…
silabs-oysteink Nov 28, 2022
3949629
LSU address trigger less-than was wrong. Now calculating the lowest a…
silabs-oysteink Nov 28, 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
4 changes: 3 additions & 1 deletion rtl/cv32e40x_controller_fsm.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,9 +442,11 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
// The cycle after fencei enters WB, the fencei handshake will be initiated. This must complete and the fencei instruction must retire before allowing debug.
// Any multi operation instruction (table jumps, push/pop and double moves) may not be interrupted once the first operation has completed its operation in WB.
// - This is guarded with using the sequence_interruptible, which tracks sequence progress through the WB stage.
// - Exception if a LSU trigger match happens during push or pop, then we must abort the sequence and enter debug mode.
// - If trigger_match_in_wb is caused by instruction address match, then no side effects will happen for a sequence, and debug mode is entered when the first operation is in WB.
// When a CLIC pointer is in the pipeline stages EX or WB, we must block debug.
// - Debug would otherwise kill the pointer and use the address of the pointer for dpc. A following dret would then return to the mtvt table, losing program progress.
assign debug_allowed = lsu_interruptible_i && !fencei_ongoing && !xif_in_wb && !clic_ptr_in_pipeline && sequence_interruptible;
assign debug_allowed = lsu_interruptible_i && !fencei_ongoing && !xif_in_wb && !clic_ptr_in_pipeline && (sequence_interruptible || trigger_match_in_wb);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change in itself SEC clean when ZC_EXT = 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SEC is still running after four days


// Debug pending for any other reason than single step
assign pending_debug = (trigger_match_in_wb) ||
Expand Down
29 changes: 26 additions & 3 deletions rtl/cv32e40x_core.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,11 @@ module cv32e40x_core import cv32e40x_pkg::*;
logic lsu_valid_wb;
logic lsu_ready_1;

// LSU signals to trigger module
logic [31:0] lsu_addr_ex;
logic lsu_we_ex;
logic [1:0] lsu_size_ex;

logic data_stall_wb;

// Stage ready signals
Expand All @@ -283,8 +288,10 @@ module cv32e40x_core import cv32e40x_pkg::*;
// From cs_registers
dcsr_t dcsr;

// trigger match detected in cs_registers (using ID timing)
// trigger match detected in trigger module (using IF timing)
logic trigger_match_if;
// trigger match detected in trigger module (using EX/LSU timing)
logic trigger_match_ex;

// Controller <-> decoder
logic alu_jmp_id;
Expand Down Expand Up @@ -573,6 +580,9 @@ module cv32e40x_core import cv32e40x_pkg::*;
.csr_illegal_i ( csr_illegal ),
.csr_mnxti_read_i ( csr_mnxti_read ),

// trigger input
.trigger_match_i ( trigger_match_ex ),

// Branch decision
.branch_decision_o ( branch_decision_ex ),
.branch_target_o ( branch_target_ex ),
Expand Down Expand Up @@ -634,11 +644,19 @@ module cv32e40x_core import cv32e40x_pkg::*;
.busy_o ( lsu_busy ),
.interruptible_o ( lsu_interruptible ),

// Trigger match
.trigger_match_i ( trigger_match_ex ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Stick to naming conventions:

.trigger_match_0_i ( trigger_match_ex ),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed


// Stage 0 outputs (EX)
.lsu_split_0_o ( lsu_split_ex ),
.lsu_first_op_0_o ( lsu_first_op_ex ),
.lsu_last_op_0_o ( lsu_last_op_ex ),

// Outputs to trigger module
.lsu_addr_o ( lsu_addr_ex ),
.lsu_we_o ( lsu_we_ex ),
.lsu_size_o ( lsu_size_ex ),

// Stage 1 outputs (WB)
.lsu_err_1_o ( lsu_err_wb ), // To controller (has WB timing, but does not pass through WB stage)
.lsu_rdata_1_o ( lsu_rdata_wb ),
Expand Down Expand Up @@ -792,9 +810,14 @@ module cv32e40x_core import cv32e40x_pkg::*;
.csr_wr_in_wb_flush_o ( csr_wr_in_wb_flush ),

// Debug
.trigger_match_o ( trigger_match_if ),
.trigger_match_if_o ( trigger_match_if ),
.trigger_match_ex_o ( trigger_match_ex ),
.pc_if_i ( pc_if ),
.ptr_in_if_i ( ptr_in_if )
.ptr_in_if_i ( ptr_in_if ),
.lsu_valid_ex_i ( lsu_valid_ex ),
.lsu_addr_ex_i ( lsu_addr_ex ),
.lsu_we_ex_i ( lsu_we_ex ),
.lsu_size_ex_i ( lsu_size_ex )
);

////////////////////////////////////////////////////////////////////
Expand Down
20 changes: 17 additions & 3 deletions rtl/cv32e40x_cs_registers.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,12 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
// Debug
input logic [31:0] pc_if_i,
input logic ptr_in_if_i,
output logic trigger_match_o
output logic trigger_match_if_o,
output logic trigger_match_ex_o,
input logic lsu_valid_ex_i,
input logic [31:0] lsu_addr_ex_i,
input logic lsu_we_ex_i,
input logic [1:0] lsu_size_ex_i
);

localparam logic [31:0] CORE_MISA =
Expand Down Expand Up @@ -1516,12 +1521,21 @@ module cv32e40x_cs_registers import cv32e40x_pkg::*;
// IF stage inputs
.pc_if_i ( pc_if_i ),
.ptr_in_if_i ( ptr_in_if_i ),
.priv_lvl_if_i ( PRIV_LVL_M ),

// LSU inputs
.lsu_valid_ex_i ( lsu_valid_ex_i),
.lsu_addr_ex_i ( lsu_addr_ex_i ),
.lsu_we_ex_i ( lsu_we_ex_i ),
.lsu_size_ex_i ( lsu_size_ex_i ),
.priv_lvl_ex_i ( PRIV_LVL_M ),

// Controller inputs
.ctrl_fsm_i ( ctrl_fsm_i ),

// Trigger match output
.trigger_match_o ( trigger_match_o )
// Trigger match outputs
.trigger_match_if_o ( trigger_match_if_o ),
.trigger_match_ex_o ( trigger_match_ex_o )
);


Expand Down
103 changes: 89 additions & 14 deletions rtl/cv32e40x_debug_triggers.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,21 @@ import cv32e40x_pkg::*;
// IF stage inputs
input logic [31:0] pc_if_i,
input logic ptr_in_if_i,
input privlvl_t priv_lvl_if_i,

// EX stage / LSU inputs
input logic lsu_valid_ex_i,
input logic [31:0] lsu_addr_ex_i,
input logic lsu_we_ex_i,
input logic [1:0] lsu_size_ex_i,
input privlvl_t priv_lvl_ex_i,

// Controller inputs
input ctrl_fsm_t ctrl_fsm_i,

// Trigger match output
output logic trigger_match_o
// Trigger match outputs
output logic trigger_match_if_o, // Instruction address match
output logic trigger_match_ex_o // Load/Store address match
);

// CSR write data
Expand All @@ -88,8 +97,23 @@ import cv32e40x_pkg::*;
logic [31:0] tdata2_q[DBG_NUM_TRIGGERS];
logic [31:0] tselect_q;

// Fetch stage trigger match
// IF and EX stages trigger match
logic [DBG_NUM_TRIGGERS-1 : 0] trigger_match_if;
logic [DBG_NUM_TRIGGERS-1 : 0] trigger_match_ex;

// Instruction address match
logic [DBG_NUM_TRIGGERS-1 : 0] if_addr_match;

// LSU address match signals
logic [DBG_NUM_TRIGGERS-1 : 0] lsu_addr_match_en;
logic [DBG_NUM_TRIGGERS-1 : 0] lsu_addr_match;

// Enable matching based on privilege level per trigger
logic [DBG_NUM_TRIGGERS-1 : 0] priv_lvl_match_en_if;
logic [DBG_NUM_TRIGGERS-1 : 0] priv_lvl_match_en_ex;

// Lower boundary of LSU address match checks, calculated for each trigger
logic [31:0] tdata2_q_low[DBG_NUM_TRIGGERS];


// Write data
Expand All @@ -108,7 +132,7 @@ import cv32e40x_pkg::*;
4'b0000, // zero, size (match any size) 19:16
4'b0001, // action, WARL(1), enter debug 15:12
1'b0, // zero, chain 11
4'b0000, // match, WARL(0,2,3) 10:7 todo: resolve WARL
mcontrol6_match_resolve(tdata1_rdata_o[MCONTROL6_MATCH_H:MCONTROL6_MATCH_L], csr_wdata_i[MCONTROL6_MATCH_H:7]), // match, WARL(0,2,3) 10:7
Copy link
Contributor

Choose a reason for hiding this comment

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

Stick to _HIGH, _LOW naming convention as used for other bitfields

Copy link
Contributor

Choose a reason for hiding this comment

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

Replace '7'

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

csr_wdata_i[6], // M 6
1'b0, // zero 5
1'b0, // zero, S 4
Expand All @@ -125,18 +149,65 @@ import cv32e40x_pkg::*;

// Generate DBG_NUM_TRIGGERS instances of tdata1, tdata2 and match checks
for (genvar idx=0; idx<DBG_NUM_TRIGGERS; idx++) begin : tmatch_csr
// Breakpoint matching
// We match against the next address, as the breakpoint must be taken before execution
// Matching is disabled when ctrl_fsm_i.debug_mode == 1'b1

////////////////////////////////////
// Instruction address match (IF)
////////////////////////////////////

// With timing=0 we enter debug before executing the instruction at the match address. We use the IF stage PC
// for comparison, and any trigger match will cause the instruction to act as a NOP with no side effects until it
// reaches WB where debug mode is entered.
//
// Trigger match is disabled while in debug mode.
//
// Trigger CSRs can only be written from debug mode, writes from any other privilege level are ignored.
// Thus we do not have an issue where a write to the tdata2 CSR immediately before the matched instruction
// could be missed since we must write in debug mode, then dret to machine mode (kills pipeline) before
// returning to dpc.
// Todo: There is no CLIC spec for trigger matches for pointers.
// todo: use struct or parameters for indexing to make code more readable.
// todo: Check tdata1[6] vs actual priv_lvl and add check for tdata1[3] (PRIV_LVL_U)
assign trigger_match_if[idx] = tdata1_q[idx][2] && tdata1_q[idx][6] && !ctrl_fsm_i.debug_mode && !ptr_in_if_i &&
(pc_if_i[31:0] == tdata2_q[idx][31:0]);
// No instruction address match on any pointer type (CLIC and Zc tablejumps).

// Check for address match using tdata2.match for checking rule
assign if_addr_match[idx] = (tdata1_q[idx][MCONTROL6_MATCH_H:MCONTROL6_MATCH_L] == 4'h0) ? (pc_if_i == tdata2_q[idx]) :
(tdata1_q[idx][MCONTROL6_MATCH_H:MCONTROL6_MATCH_L] == 4'h2) ? (pc_if_i >= tdata2_q[idx]) : (pc_if_i < tdata2_q[idx]);

// Check if matching is enabled for the current privilege level from IF
assign priv_lvl_match_en_if[idx] = (tdata1_q[idx][MCONTROL6_M] && (priv_lvl_if_i == PRIV_LVL_M)) ||
(tdata1_q[idx][MCONTROL6_U] && (priv_lvl_if_i == PRIV_LVL_U));

// Check for trigger match from IF
assign trigger_match_if[idx] = tdata1_q[idx][MCONTROL6_EXECUTE] && priv_lvl_match_en_if[idx] && !ctrl_fsm_i.debug_mode && !ptr_in_if_i &&
if_addr_match[idx];

///////////////////////////////////////
// Load/Store address match (EX)
///////////////////////////////////////

// As for instruction address match, the load/store address match happens before the instruction is executed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this complexity caused by how misaligned loads/stores are handled? Do you generate all compare values for a misaligned load/store during its first operation? If so, why, and would it help to treat each opeartion separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The adder to calculate boundaries came (partly) from checking all values, even for misaligned load/stores. I rewrote the code to check per-transaction, so two checks will be performed for misaligned. I also replaced the adder by logic to generate boundaries base on the byte enables instead.

// Matching is detected from the EX stage, and upon a trigger match the corresponding bus request and
// register file write (for loads) are suppressed.

// Calculate lower boundary of address checks from tdata2
// Needed for LSU address matching, where halfwords check {A, A+1}
// and words check {A, A+1, A+2, A+3}.
// For timing reasons we check A vs (tdata_q - N) instead of (A+N) vs tdata_q. This avoids two adders (one in the LSU and one here) in the critical path.
// - This comes at the cost of DBG_NUM_TRIGGERS adders instead of one common for all triggers.
assign tdata2_q_low[idx] = (lsu_size_ex_i == 2'b10) ? tdata2_q[idx] - 32'h3 :
(lsu_size_ex_i == 2'b01) ? tdata2_q[idx] - 32'h1 : tdata2_q[idx];


// Check if matching is enabled for the current privilege level from EX
assign priv_lvl_match_en_ex[idx] = (tdata1_q[idx][MCONTROL6_M] && (priv_lvl_ex_i == PRIV_LVL_M)) ||
(tdata1_q[idx][MCONTROL6_U] && (priv_lvl_ex_i == PRIV_LVL_U));

// Enable LSU address matching
assign lsu_addr_match_en[idx] = lsu_valid_ex_i && ((tdata1_q[idx][MCONTROL6_LOAD] && !lsu_we_ex_i) || (tdata1_q[idx][MCONTROL6_STORE] && lsu_we_ex_i));
Copy link
Contributor

Choose a reason for hiding this comment

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

Add todo to revisit this code when supporting Atomics

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


// LSU address matching
assign lsu_addr_match[idx] = (tdata1_q[idx][MCONTROL6_MATCH_H:MCONTROL6_MATCH_L] == 4'h2) ? (lsu_addr_ex_i >= tdata2_q[idx]) :
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the _llow value should be used for match == 2 instead.

Also, order match values 0, 2, 3 just like for the I-side comparisons

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

(tdata1_q[idx][MCONTROL6_MATCH_H:MCONTROL6_MATCH_L] == 4'h3) ? (lsu_addr_ex_i < tdata2_q_low[idx]) :
(lsu_addr_ex_i >= tdata2_q_low[idx]) && (lsu_addr_ex_i <= tdata2_q[idx]);

assign trigger_match_ex[idx] = priv_lvl_match_en_ex[idx] && lsu_addr_match_en[idx] && lsu_addr_match[idx] && !ctrl_fsm_i.debug_mode;


cv32e40x_csr
Expand Down Expand Up @@ -207,7 +278,10 @@ import cv32e40x_pkg::*;
assign tcontrol_rdata_o = 32'h00000000;

// Set trigger match for IF
assign trigger_match_o = |trigger_match_if;
assign trigger_match_if_o = |trigger_match_if;

// Set trigger match for EX
assign trigger_match_ex_o = |trigger_match_ex;

assign unused_signals = tinfo_we_i | tcontrol_we_i | tdata3_we_i | (|tinfo_n) | (|tdata3_n) | (|tcontrol_n);

Expand All @@ -219,7 +293,8 @@ import cv32e40x_pkg::*;
assign tselect_rdata_o = '0;
assign tinfo_rdata_o = '0;
assign tcontrol_rdata_o = '0;
assign trigger_match_o = '0;
assign trigger_match_if_o = '0;
assign trigger_match_ex_o = '0;
assign tdata1_n = '0;
assign tdata2_n = '0;
assign tdata3_n = '0;
Expand Down
35 changes: 20 additions & 15 deletions rtl/cv32e40x_ex_stage.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ module cv32e40x_ex_stage import cv32e40x_pkg::*;
input logic csr_illegal_i,
input logic csr_mnxti_read_i,

input logic trigger_match_i,

// EX/WB pipeline
output ex_wb_pipe_t ex_wb_pipe_o,

Expand Down Expand Up @@ -364,12 +366,12 @@ module cv32e40x_ex_stage import cv32e40x_pkg::*;

ex_wb_pipe_o.last_op <= last_op_o;
ex_wb_pipe_o.first_op <= first_op_o;
ex_wb_pipe_o.abort_op <= id_ex_pipe_i.abort_op; // MPU exceptions have WB timing and will not impact ex_wb_pipe.abort_op
// Deassert rf_we in case of illegal csr instruction or
// when the first half of a misaligned/split LSU goes to WB.
ex_wb_pipe_o.abort_op <= id_ex_pipe_i.abort_op || trigger_match_i; // MPU exceptions have WB timing and will not impact ex_wb_pipe.abort_op
// Deassert rf_we in case of illegal csr instruction,
// when the first half of a misaligned/split LSU goes to WB or when there is a trigger match on the LSU address.
// Also deassert if CSR was accepted both by eXtension if and pipeline
ex_wb_pipe_o.rf_we <= (csr_is_illegal || lsu_split_i) ? 1'b0 : id_ex_pipe_i.rf_we;
ex_wb_pipe_o.lsu_en <= id_ex_pipe_i.lsu_en;
ex_wb_pipe_o.rf_we <= (csr_is_illegal || lsu_split_i || trigger_match_i) ? 1'b0 : id_ex_pipe_i.rf_we;
ex_wb_pipe_o.lsu_en <= trigger_match_i ? 1'b0 : id_ex_pipe_i.lsu_en;

if (id_ex_pipe_i.rf_we) begin
ex_wb_pipe_o.rf_waddr <= id_ex_pipe_i.rf_waddr;
Expand Down Expand Up @@ -411,7 +413,7 @@ module cv32e40x_ex_stage import cv32e40x_pkg::*;

// CSR illegal instruction detected in this stage, OR'ing in the status
ex_wb_pipe_o.illegal_insn <= id_ex_pipe_i.illegal_insn || csr_is_illegal;
ex_wb_pipe_o.trigger_match <= id_ex_pipe_i.trigger_match;
ex_wb_pipe_o.trigger_match <= id_ex_pipe_i.trigger_match || trigger_match_i;

// eXtension interface
ex_wb_pipe_o.xif_en <= ctrl_fsm_i.kill_xif ? 1'b0 : id_ex_pipe_i.xif_en;
Expand All @@ -425,6 +427,8 @@ module cv32e40x_ex_stage import cv32e40x_pkg::*;
end

// LSU inputs are valid when LSU is enabled; LSU outputs need to remain valid until downstream stage is ready
// If a trigger matches LSU address, this valid will be gated off inside the load_store_unit to prevent the instruction
// from accessing the bus.
assign lsu_valid_o = lsu_en_gated;
assign lsu_ready_o = wb_ready_i;

Expand All @@ -447,15 +451,16 @@ module cv32e40x_ex_stage import cv32e40x_pkg::*;
assign ex_ready_o = ctrl_fsm_i.kill_ex || (alu_ready && csr_ready && sys_ready && mul_ready && div_ready && lsu_ready_i && xif_ready && !ctrl_fsm_i.halt_ex);

// TODO:ab Reconsider setting alu_en for exception/trigger instead of using 'previous_exception'
assign ex_valid_o = ((id_ex_pipe_i.alu_en && alu_valid) ||
(id_ex_pipe_i.csr_en && csr_valid) ||
(id_ex_pipe_i.sys_en && sys_valid) ||
(id_ex_pipe_i.mul_en && mul_valid) ||
(id_ex_pipe_i.div_en && div_valid) ||
(id_ex_pipe_i.lsu_en && lsu_valid_i) ||
(id_ex_pipe_i.xif_en && xif_valid) ||
(id_ex_pipe_i.instr_meta.clic_ptr) || // todo: Should this instead have it's own _valid?
(id_ex_pipe_i.instr_meta.mret_ptr) || // todo: Should this instead have it's own _valid?
assign ex_valid_o = ((id_ex_pipe_i.alu_en && alu_valid) ||
(id_ex_pipe_i.csr_en && csr_valid) ||
(id_ex_pipe_i.sys_en && sys_valid) ||
(id_ex_pipe_i.mul_en && mul_valid) ||
(id_ex_pipe_i.div_en && div_valid) ||
(id_ex_pipe_i.lsu_en && lsu_valid_i) ||
(id_ex_pipe_i.lsu_en && trigger_match_i) || // LSU trigger match causes no side effects in EX, raise ex_valid
(id_ex_pipe_i.xif_en && xif_valid) ||
(id_ex_pipe_i.instr_meta.clic_ptr) || // todo: Should this instead have it's own _valid?
(id_ex_pipe_i.instr_meta.mret_ptr) || // todo: Should this instead have it's own _valid?
previous_exception // todo:ab:remove
) && instr_valid;

Expand Down
Loading