-
Notifications
You must be signed in to change notification settings - Fork 52
Trigger types 0x5 and 0xF #706
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.
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -96,6 +96,9 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*; | |
input dcsr_t dcsr_i, | ||
input mcause_t mcause_i, | ||
|
||
// Trigger module | ||
input logic etrigger_wb_i, // Trigger module detected match in WB (etrigger) | ||
|
||
// Toplevel input | ||
input logic debug_req_i, // External debug request | ||
|
||
|
@@ -173,7 +176,8 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*; | |
logic mret_ptr_in_wb; // CLIC pointer caused by mret is in WB | ||
logic dret_in_wb; | ||
logic ebreak_in_wb; | ||
logic trigger_match_in_wb; | ||
logic trigger_match_in_wb; // mcontrol6 trigger in WB | ||
logic etrigger_in_wb; // exception trigger in WB | ||
logic xif_in_wb; | ||
logic clic_ptr_in_wb; // CLIC pointer caused by directly acking an SHV is in WB (no mret) | ||
|
||
|
@@ -183,6 +187,7 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*; | |
logic pending_single_step; | ||
logic pending_interrupt; | ||
|
||
|
||
// Flags for allowing interrupt and debug | ||
logic exception_allowed; | ||
logic interrupt_allowed; | ||
|
@@ -330,6 +335,8 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*; | |
(lsu_mpu_status_wb_i == MPU_WR_FAULT) ? EXC_CAUSE_STORE_FAULT : | ||
EXC_CAUSE_LOAD_FAULT; // (lsu_mpu_status_wb_i == MPU_RE_FAULT) | ||
|
||
assign ctrl_fsm_o.exception_cause_wb = exception_cause_wb; | ||
|
||
// For now we are always allowed to take exceptions once they arrive in WB. | ||
// For a misaligned load/store with MPU error on the first half, the second half | ||
// will arrive in EX when the first half (with error) arrives in WB. The exception will | ||
|
@@ -365,6 +372,11 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*; | |
// Trigger_match during debug mode is masked in the trigger logic inside cs_registers.sv | ||
assign trigger_match_in_wb = (ex_wb_pipe_i.trigger_match && ex_wb_pipe_i.instr_valid); | ||
|
||
// Only set the etrigger_in_wb flag when wb_valid is true (WB is not halted or killed). | ||
// If a higher priority event than taking an exception (NMI, external debug or interrupts) are present, wb_valid_i will be | ||
// suppressed by either halt_wb followed by kill_wb (debug), or kill_wb (NMI/interrupt). | ||
assign etrigger_in_wb = etrigger_wb_i && wb_valid_i; | ||
|
||
// An offloaded instruction is in WB | ||
assign xif_in_wb = (ex_wb_pipe_i.xif_en && ex_wb_pipe_i.instr_valid); | ||
|
||
|
@@ -458,8 +470,8 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*; | |
// pending_single_step may only happen if no other causes for debug are true. | ||
// The flopped version of this is checked during DEBUG_TAKEN state (one cycle delay) | ||
// todo: update priority according to updated debug spec | ||
assign debug_cause_n = pending_single_step ? DBG_CAUSE_STEP : | ||
trigger_match_in_wb ? DBG_CAUSE_TRIGGER : | ||
assign debug_cause_n = pending_single_step ? DBG_CAUSE_STEP : | ||
(trigger_match_in_wb || etrigger_wb_i) ? DBG_CAUSE_TRIGGER : | ||
(ebreak_in_wb && dcsr_i.ebreakm && !debug_mode_q) ? DBG_CAUSE_EBREAK : | ||
DBG_CAUSE_HALTREQ; | ||
|
||
|
@@ -481,7 +493,6 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*; | |
// - This is guarded with using the sequence_interruptible, which tracks sequence progress through the WB stage. | ||
// When a CLIC pointer is in the pipeline stages EX or WB, we must block interrupts. | ||
// - Interrupt would otherwise kill the pointer and use the address of the pointer for mepc. A following mret would then return to the mtvt table, losing program progress. | ||
|
||
assign interrupt_allowed = lsu_interruptible_i && debug_interruptible && !fencei_ongoing && !xif_in_wb && !clic_ptr_in_pipeline && sequence_interruptible && !interrupt_blanking_q; | ||
|
||
// Allowing NMI's follow the same rule as regular interrupts, except we don't need to regard blanking of NMIs after a load/store. | ||
|
@@ -930,11 +941,11 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*; | |
end | ||
end // !debug or interrupts | ||
|
||
// Single step debug entry | ||
// Single step debug entry or etrigger debug entry | ||
// Need to be after (in parallell with) exception/interrupt handling | ||
// to ensure mepc and if_pc set correctly for use in dpc, | ||
// to ensure mepc and if_pc are set correctly for use in dpc, | ||
// and to ensure only one instruction can retire during single step | ||
if (pending_single_step) begin | ||
if (pending_single_step || etrigger_in_wb) begin | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add an assertion that etrigger_in_wb implies exception_in_wb There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added assertion |
||
if (single_step_allowed) begin | ||
ctrl_fsm_ns = DEBUG_TAKEN; | ||
end | ||
|
@@ -988,6 +999,7 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*; | |
ctrl_fsm_o.kill_ex = 1'b1; | ||
// Ebreak that causes debug entry should not be killed, otherwise RVFI will skip it | ||
// Trigger match should also be signalled as not killed (all write enables are suppressed in ID), otherwise RVFI/ISS will not attempt to execute and detect trigger | ||
// Exception trigger match should have nothing in WB, excepted instruction finished the previous cycle and set mepc and mcause due to the exception. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As always when claiming that something 'should be' the case, please check it with an assertion There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is checked by the a_etrigger_dpc_write in cs_registers_sva. It check for !wb_valid the cycle after an etrigger has been taken. I added a check for !ex_wb_pipe.instr_valid just to make it more clear. |
||
// Ebreak during debug_mode restarts from dm_halt_addr, without CSR updates. Not killing ebreak due to the same RVFI/ISS reasons. | ||
// Neither ebreak nor trigger match have any state updates in WB. For trigger match, all write enables are suppressed in the ID stage. | ||
// Thus this change is not visible to core state, only for RVFI use. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -291,6 +291,8 @@ module cv32e40x_core import cv32e40x_pkg::*; | |
logic trigger_match_if; | ||
// trigger match detected in trigger module (using EX/LSU timing) | ||
logic trigger_match_ex; | ||
// trigger match detected in trigger module (using WB timing, etrigger) | ||
logic etrigger_wb; | ||
|
||
// Controller <-> decoder | ||
logic alu_jmp_id; | ||
|
@@ -811,6 +813,7 @@ module cv32e40x_core import cv32e40x_pkg::*; | |
// Debug | ||
.trigger_match_if_o ( trigger_match_if ), | ||
.trigger_match_ex_o ( trigger_match_ex ), | ||
.etrigger_wb_o ( etrigger_wb ), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Align brackets There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
.pc_if_i ( pc_if ), | ||
.ptr_in_if_i ( ptr_in_if ), | ||
.lsu_valid_ex_i ( lsu_valid_ex ), | ||
|
@@ -903,6 +906,9 @@ module cv32e40x_core import cv32e40x_pkg::*; | |
.mtvec_mode_i ( mtvec_mode ), | ||
.mcause_i ( mcause ), | ||
|
||
// Trigger module | ||
.etrigger_wb_i ( etrigger_wb ), | ||
|
||
// CSR write strobes | ||
.csr_wr_in_wb_flush_i ( csr_wr_in_wb_flush ), | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,12 +67,16 @@ import cv32e40x_pkg::*; | |
input logic [3:0] lsu_be_ex_i, | ||
input privlvl_t priv_lvl_ex_i, | ||
|
||
// WB stage inputs | ||
input privlvl_t priv_lvl_wb_i, | ||
|
||
// Controller inputs | ||
input ctrl_fsm_t ctrl_fsm_i, | ||
|
||
// Trigger match outputs | ||
output logic trigger_match_if_o, // Instruction address match | ||
output logic trigger_match_ex_o // Load/Store address match | ||
output logic trigger_match_ex_o, // Load/Store address match | ||
output logic etrigger_wb_o // Exception trigger match | ||
); | ||
|
||
// CSR write data | ||
|
@@ -103,9 +107,10 @@ import cv32e40x_pkg::*; | |
logic [31:0] tdata1_rdata[DBG_NUM_TRIGGERS]; | ||
logic [31:0] tdata2_rdata[DBG_NUM_TRIGGERS]; | ||
|
||
// IF and EX stages trigger match | ||
// IF, EX and WB stages trigger match | ||
logic [DBG_NUM_TRIGGERS-1 : 0] trigger_match_if; | ||
logic [DBG_NUM_TRIGGERS-1 : 0] trigger_match_ex; | ||
logic [DBG_NUM_TRIGGERS-1 : 0] etrigger_wb; | ||
|
||
// Instruction address match | ||
logic [DBG_NUM_TRIGGERS-1 : 0] if_addr_match; | ||
|
@@ -118,38 +123,86 @@ import cv32e40x_pkg::*; | |
// 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; | ||
logic [DBG_NUM_TRIGGERS-1 : 0] priv_lvl_match_en_wb; | ||
|
||
logic [1:0] lsu_addr_low_lsb; // Lower two bits of the lowest accessed address | ||
logic [1:0] lsu_addr_high_lsb; // Lower two bits of the highest accessed address | ||
logic [31:0] lsu_addr_low; // The lowest accessed address of an LSU transaction | ||
logic [31:0] lsu_addr_high; // The highest accessed address of an LSU transaction | ||
|
||
// Exception trigger code match | ||
logic [31:0] exception_match[DBG_NUM_TRIGGERS]; | ||
|
||
// Write data | ||
always_comb begin | ||
// Tselect is WARL (0 -> DBG_NUM_TRIGGERS-1) | ||
tselect_n = (csr_wdata_i < DBG_NUM_TRIGGERS) ? csr_wdata_i : tselect_rdata_o; | ||
|
||
// todo: handle WARL based on trigger type | ||
tdata1_n = { | ||
TTYPE_MCONTROL6, // type : address/data match | ||
1'b1, // dmode : access from D mode only | ||
2'b00, // zero 26:25 | ||
3'b000, // zero, vs, vu, hit 24:22 | ||
1'b0, // zero, select 21 | ||
1'b0, // zero, timing 20 | ||
4'b0000, // zero, size (match any size) 19:16 | ||
4'b0001, // action, WARL(1), enter debug 15:12 | ||
1'b0, // zero, chain 11 | ||
mcontrol6_match_resolve(csr_wdata_i[MCONTROL6_MATCH_HIGH:MCONTROL6_MATCH_LOW]), // match, WARL(0,2,3) 10:7 | ||
csr_wdata_i[6], // M 6 | ||
1'b0, // zero 5 | ||
1'b0, // zero, S 4 | ||
1'b0, // zero, U 3 | ||
csr_wdata_i[2], // EXECUTE 2 | ||
csr_wdata_i[1], // STORE 1 | ||
csr_wdata_i[0]}; // LOAD 0 | ||
|
||
tdata2_n = csr_wdata_i; | ||
tselect_n = (csr_wdata_i < DBG_NUM_TRIGGERS) ? csr_wdata_i : tselect_rdata_o; | ||
tdata1_n = tdata1_rdata_o; | ||
tdata2_n = tdata2_rdata_o; | ||
|
||
if (csr_wdata_i[TDATA1_TTYPE_HIGH:TDATA1_TTYPE_LOW] == TTYPE_MCONTROL6) begin | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn''t such code only be done when there is an actual write to tdata1? Same question with respect to code related to tdata_2. No need maybe to change the code, but do add a remark that the _n data is not used without the worresponding _we signals There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, added qualifiers for tdata1_we_i and tdata2_we_i |
||
// Mcontrol6 supports any value in tdata2, no need to check tdata2 before writing tdata1 | ||
tdata1_n = { | ||
TTYPE_MCONTROL6, // type : address/data match | ||
1'b1, // dmode : access from D mode only | ||
2'b00, // zero 26:25 | ||
3'b000, // zero, vs, vu, hit 24:22 | ||
1'b0, // zero, select 21 | ||
1'b0, // zero, timing 20 | ||
4'b0000, // zero, size (match any size) 19:16 | ||
4'b0001, // action, WARL(1), enter debug 15:12 | ||
1'b0, // zero, chain 11 | ||
mcontrol6_match_resolve(csr_wdata_i[MCONTROL6_MATCH_HIGH:MCONTROL6_MATCH_LOW]), // match, WARL(0,2,3) 10:7 | ||
csr_wdata_i[6], // M 6 | ||
1'b0, // zero 5 | ||
1'b0, // zero, S 4 | ||
1'b0, // zero, U 3 | ||
csr_wdata_i[2], // EXECUTE 2 | ||
csr_wdata_i[1], // STORE 1 | ||
csr_wdata_i[0] // LOAD 0 | ||
}; | ||
end else if (csr_wdata_i[TDATA1_TTYPE_HIGH:TDATA1_TTYPE_LOW] == TTYPE_ETRIGGER) begin | ||
// Etrigger can only support a subset of possible values in tdata2, only update tdata1 if | ||
// tdata2 contains a legal value for etrigger. | ||
|
||
// Detect if any cleared bits in ETRIGGER_TDATA2_MASK are set in tdata2 | ||
if (0) begin //|(tdata2_rdata_o & (~ETRIGGER_TDATA2_MASK))) begin | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if (0) ??? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will fixing this also fix the case we discussed (i.e. keeping type DISABLED when trying to switch to a type that is incomptabile with values in tdata1 and tdata2? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, this should definitely not have propagated here, this came from some debugging and slipped through. |
||
// Unsupported exception codes enabled, keep value of tdata1 | ||
tdata1_n = tdata1_rdata_o; | ||
end else begin | ||
tdata1_n = { | ||
TTYPE_ETRIGGER, // type : exception trigger | ||
1'b1, // dmode : access from D mode only 27 | ||
1'b0, // hit : WARL(0) 26 | ||
13'h0, // zero : tied to zero 25:13 | ||
1'b0, // vs : WARL(0) 12 | ||
1'b0, // vu : WARL(0) 11 | ||
1'b0, // zero : tied to zero 10 | ||
csr_wdata_i[9], // m : Match in machine mode 9 | ||
1'b0, // zero : tied to zero 8 | ||
1'b0, // s : WARL(0) 7 | ||
1'b0, // u : Match in user mode 7 | ||
6'b000001 // action : WARL(1), enter debug on match | ||
}; | ||
end | ||
end else if (csr_wdata_i[TDATA1_TTYPE_HIGH:TDATA1_TTYPE_LOW] == TTYPE_DISABLED) begin | ||
// All tdata2 values are legal for a disabled trigger, no WARL on tdata1. | ||
tdata1_n = {TTYPE_DISABLED, 1'b1, {27{1'b0}}}; | ||
end else begin | ||
// No legal trigger type, keep currently selected value | ||
tdata1_n = tdata1_rdata_o; | ||
end | ||
|
||
// tdata2 | ||
if ((tdata1_rdata_o[TDATA1_TTYPE_HIGH:TDATA1_TTYPE_LOW] == TTYPE_DISABLED) || | ||
(tdata1_rdata_o[TDATA1_TTYPE_HIGH:TDATA1_TTYPE_LOW] == TTYPE_MCONTROL6)) begin | ||
// Disabled trigger and mcontrol6 can have any value in tdata2 | ||
tdata2_n = csr_wdata_i; | ||
end else begin | ||
// Exception trigger, only allow implemented exception codes to be used | ||
tdata2_n = csr_wdata_i & ETRIGGER_TDATA2_MASK; | ||
end | ||
|
||
tdata3_n = tdata3_rdata_o; // Read only | ||
tinfo_n = tinfo_rdata_o; // Read only | ||
tcontrol_n = tcontrol_rdata_o; // Read only | ||
|
@@ -205,7 +258,8 @@ import cv32e40x_pkg::*; | |
(tdata1_rdata[idx][MCONTROL6_U] && (priv_lvl_if_i == PRIV_LVL_U)); | ||
|
||
// Check for trigger match from IF | ||
assign trigger_match_if[idx] = tdata1_rdata[idx][MCONTROL6_EXECUTE] && priv_lvl_match_en_if[idx] && !ctrl_fsm_i.debug_mode && !ptr_in_if_i && | ||
assign trigger_match_if[idx] = (tdata1_rdata[idx][TDATA1_TTYPE_HIGH:TDATA1_TTYPE_LOW] == TTYPE_MCONTROL6) && | ||
tdata1_rdata[idx][MCONTROL6_EXECUTE] && priv_lvl_match_en_if[idx] && !ctrl_fsm_i.debug_mode && !ptr_in_if_i && | ||
if_addr_match[idx]; | ||
|
||
/////////////////////////////////////// | ||
|
@@ -243,11 +297,30 @@ import cv32e40x_pkg::*; | |
(tdata1_rdata[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_rdata[idx][MCONTROL6_LOAD] && !lsu_we_ex_i) || (tdata1_rdata[idx][MCONTROL6_STORE] && lsu_we_ex_i)); | ||
assign lsu_addr_match_en[idx] = lsu_valid_ex_i && (tdata1_rdata[idx][TDATA1_TTYPE_HIGH:TDATA1_TTYPE_LOW] == TTYPE_MCONTROL6) && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not consistent with IF trigger match handling. Make sure that the (tdata1_rdata[idx][TDATA1_TTYPE_HIGH:TDATA1_TTYPE_LOW] == TTYPE_MCONTROL6) classification is done on the same type of signal, so for example on trigger_match_* signals. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved classification to align with IF and WB |
||
((tdata1_rdata[idx][MCONTROL6_LOAD] && !lsu_we_ex_i) || (tdata1_rdata[idx][MCONTROL6_STORE] && lsu_we_ex_i)); | ||
|
||
// Signal trigger match for LSU address | ||
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; | ||
|
||
///////////////////////////// | ||
// Exception trigger | ||
///////////////////////////// | ||
|
||
always_comb begin | ||
exception_match[idx] = 32'd0; | ||
for (int i=0; i<32; i++) begin | ||
exception_match[idx][i] = tdata2_rdata[idx][i] && ctrl_fsm_i.exception_in_wb && (ctrl_fsm_i.exception_cause_wb == 11'(i)); | ||
end | ||
end | ||
|
||
assign priv_lvl_match_en_wb[idx] = (tdata1_rdata[idx][ETRIGGER_M] && (priv_lvl_wb_i == PRIV_LVL_M)) || | ||
(tdata1_rdata[idx][ETRIGGER_U] && (priv_lvl_wb_i == PRIV_LVL_U)); | ||
|
||
assign etrigger_wb[idx] = (tdata1_rdata[idx][TDATA1_TTYPE_HIGH:TDATA1_TTYPE_LOW] == TTYPE_ETRIGGER) && priv_lvl_match_en_wb[idx] && | ||
(|exception_match[idx]) && !ctrl_fsm_i.debug_mode; | ||
|
||
|
||
|
||
cv32e40x_csr | ||
#( | ||
|
@@ -282,7 +355,6 @@ import cv32e40x_pkg::*; | |
assign tdata2_we_int[idx] = tdata2_we_i && (tselect_rdata_o == idx); | ||
|
||
// Assign read data | ||
// todo: WARL | ||
assign tdata1_rdata[idx] = tdata1_q[idx]; | ||
assign tdata2_rdata[idx] = tdata2_q[idx]; | ||
end // for | ||
|
@@ -318,7 +390,7 @@ import cv32e40x_pkg::*; | |
|
||
assign tdata3_rdata_o = 32'h00000000; | ||
assign tselect_rdata_o = tselect_q; | ||
assign tinfo_rdata_o = 32'h4; // todo: update | ||
assign tinfo_rdata_o = 32'h00008060; // Supported types 0x5, 0x6 and 0xF | ||
assign tcontrol_rdata_o = 32'h00000000; | ||
|
||
// Set trigger match for IF | ||
|
@@ -327,6 +399,9 @@ import cv32e40x_pkg::*; | |
// Set trigger match for EX | ||
assign trigger_match_ex_o = |trigger_match_ex; | ||
|
||
// Set trigger match for WB | ||
assign etrigger_wb_o = |etrigger_wb; | ||
|
||
assign unused_signals = tinfo_we_i | tcontrol_we_i | tdata3_we_i | (|tinfo_n) | (|tdata3_n) | (|tcontrol_n); | ||
|
||
end else begin : gen_no_triggers | ||
|
@@ -339,6 +414,7 @@ import cv32e40x_pkg::*; | |
assign tcontrol_rdata_o = '0; | ||
assign trigger_match_if_o = '0; | ||
assign trigger_match_ex_o = '0; | ||
assign etrigger_wb_o = '0; | ||
assign tdata1_n = '0; | ||
assign tdata2_n = '0; | ||
assign tdata3_n = '0; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this expression also contain ex_wb_pipe_i.instr_valid like all other *_in_wb signals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ex_wb_pipe_i.instr_valid is already implied by wb_valid_i as that signal is only set when (ex_wb_pipe_i.instr_valid && !halt_wb && !kill_wb).