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

[rtl] Flush pipe on all CSR modifications #2214

Merged
merged 2 commits into from
Feb 17, 2025
Merged

[rtl] Flush pipe on all CSR modifications #2214

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
1e4789a
[rtl] Read csr_addr direct from instruction
GregAC Feb 14, 2025
866572c
[rtl] Flush pipe on all CSR modifications
GregAC Sep 17, 2024
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
2 changes: 1 addition & 1 deletion rtl/ibex_core.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -618,6 +618,7 @@ module ibex_core import ibex_pkg::*; #(
// CSR ID/EX
.csr_access_o (csr_access),
.csr_op_o (csr_op),
.csr_addr_o (csr_addr),
.csr_op_en_o (csr_op_en),
.csr_save_if_o (csr_save_if), // control signal to save PC
.csr_save_id_o (csr_save_id), // control signal to save PC
Expand Down Expand Up @@ -1046,7 +1047,6 @@ module ibex_core import ibex_pkg::*; #(
/////////////////////////////////////////

assign csr_wdata = alu_operand_a_ex;
assign csr_addr = csr_num_e'(csr_access ? alu_operand_b_ex[11:0] : 12'b0);

ibex_cs_registers #(
.DbgTriggerEn (DbgTriggerEn),
Expand Down
8 changes: 6 additions & 2 deletions rtl/ibex_decoder.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ module ibex_decoder #(
// CSRs
output logic csr_access_o, // access to CSR
output ibex_pkg::csr_op_e csr_op_o, // operation to perform on CSR
output ibex_pkg::csr_num_e csr_addr_o, // CSR address

// LSU
output logic data_req_o, // start transaction to data memory
Expand Down Expand Up @@ -138,6 +139,8 @@ module ibex_decoder #(
assign imm_u_type_o = { instr[31:12], 12'b0 };
assign imm_j_type_o = { {12{instr[31]}}, instr[19:12], instr[20], instr[30:21], 1'b0 };

assign csr_addr_o = csr_num_e'(instr[31:20]);

// immediate for CSR manipulation (zero extended)
assign zimm_rs1_type_o = { 27'b0, instr_rs1 }; // rs1

Expand Down Expand Up @@ -1168,9 +1171,10 @@ module ibex_decoder #(
alu_op_b_mux_sel_o = OP_B_IMM;
end else begin
// instruction to read/modify CSR
alu_op_b_mux_sel_o = OP_B_IMM;
imm_a_mux_sel_o = IMM_A_Z;
imm_b_mux_sel_o = IMM_B_I; // CSR address is encoded in I imm

// No need for operand/immediate B mux selection. The CSR address is fed out as csr_addr_o
// as the CSR address always comes from the same field in the instruction.

if (instr_alu[14]) begin
// rs1 field is used as immediate
Expand Down
47 changes: 17 additions & 30 deletions rtl/ibex_id_stage.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ module ibex_id_stage #(
// CSR
output logic csr_access_o,
output ibex_pkg::csr_op_e csr_op_o,
output ibex_pkg::csr_num_e csr_addr_o,
output logic csr_op_en_o,
output logic csr_save_if_o,
output logic csr_save_id_o,
Expand Down Expand Up @@ -290,6 +291,7 @@ module ibex_id_stage #(
logic data_req_allowed;

// CSR control
logic no_flush_csr_addr;
logic csr_pipe_flush;

logic [31:0] alu_operand_a;
Expand Down Expand Up @@ -497,6 +499,7 @@ module ibex_id_stage #(
// CSRs
.csr_access_o(csr_access_o),
.csr_op_o (csr_op_o),
.csr_addr_o (csr_addr_o),

// LSU
.data_req_o (lsu_req_dec),
Expand All @@ -509,36 +512,20 @@ module ibex_id_stage #(
.branch_in_dec_o(branch_in_dec)
);

/////////////////////////////////
// CSR-related pipeline flushes //
/////////////////////////////////
always_comb begin : csr_pipeline_flushes
csr_pipe_flush = 1'b0;

// A pipeline flush is needed to let the controller react after modifying certain CSRs:
// - When enabling interrupts, pending IRQs become visible to the controller only during
// the next cycle. If during that cycle the core disables interrupts again, it does not
// see any pending IRQs and consequently does not start to handle interrupts.
// - When modifying any PMP CSR, PMP check of the next instruction might get invalidated.
// Hence, a pipeline flush is needed to instantiate another PMP check with the updated CSRs.
// - When modifying debug CSRs.
if (csr_op_en_o == 1'b1 && (csr_op_o == CSR_OP_WRITE || csr_op_o == CSR_OP_SET)) begin
if (csr_num_e'(instr_rdata_i[31:20]) == CSR_MSTATUS ||
csr_num_e'(instr_rdata_i[31:20]) == CSR_MIE ||
csr_num_e'(instr_rdata_i[31:20]) == CSR_MSECCFG ||
// To catch all PMPCFG/PMPADDR registers, get the shared top most 7 bits.
instr_rdata_i[31:25] == 7'h1D) begin
csr_pipe_flush = 1'b1;
end
end else if (csr_op_en_o == 1'b1 && csr_op_o != CSR_OP_READ) begin
if (csr_num_e'(instr_rdata_i[31:20]) == CSR_DCSR ||
csr_num_e'(instr_rdata_i[31:20]) == CSR_DPC ||
csr_num_e'(instr_rdata_i[31:20]) == CSR_DSCRATCH0 ||
csr_num_e'(instr_rdata_i[31:20]) == CSR_DSCRATCH1) begin
csr_pipe_flush = 1'b1;
end
end
end
// Flush pipe on most CSR modification. Some CSR modifications alter how instructions execute
// (e.g. the PMP CSRs) so this ensures all instructions always see the latest architectural state
// when entering the fetch stage. This causes some needless flushes but performance impact is
// limited. We have a single fetch stage to flush not many stages of a deep pipeline and CSR
Copy link
Contributor

Choose a reason for hiding this comment

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

Grammar question: Do you mean something like "We only have a single fetch stage and the pipeline is not deep. Also, CSR instructions are normally rare and aren't part of performance critical parts of the code."

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 I do, I shall rewrite it to make it clearer

// instructions are in general rare and not part of performance critical parts of the code.
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that it's a common pattern to do a csrrw sp, mscratch, sp to switch to a dedicated stack for exception handling.

For exception handling it also performs quite a few other CSR reads, so I wouldn't call CSR instructions rare and not part of performance critical code.

Copy link
Contributor Author

@GregAC GregAC Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

These are good points.

Though do note we don't flush on read only access. Whilst strictly all CSR instructions do perform modifications CSRRS and CSRRC with an x0 rs1 (the register that specifies which bits to set/clear) and the immediate versions with a 0 immediate do not alter the CSRs and will have csr_op_o in this RTL set to CSR_OP_READ so won't trigger the flush.

Do we expect any CSR writes other than to mscratch during normal exception handling code?

I'm wonder if we should operate a white-list system here where modifications to specific CSRs do not trigger a flush but everything else does. mscratch is a good candidate for that list, anything else?

Copy link
Contributor

Choose a reason for hiding this comment

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

mepc would also be updated in case the exception needs to skip over the current instruction. mstatus may be updated but that actually require flushes.

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 suspect mepc updates would be less common, not something you'd do in an interrupt handling routine for instance, maybe used in error handling cases but it's reasonable to expect those to be rare. Also used where you're doing software emulation of an instruction where speed would be desired but uncertain how much of that would practically be done on Ibex (the typical use case would be unaligned memory access which we already have hardware handling for).

Still putting mepc in the whitelist seems pretty safe, it's basically a special jump target that gets read out in the ID/EX stage and we'd probably need some radical change to make it have relevance from IF (and hence require flushing on modification).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just realised use of ecall will require mepc modification (otherwise you get an infinite loop of ecall). Performance degradation from a pipe flush is modest (a single cycle in cases where you hit in icache or have single cycle access to instruction memory I believe) and there'd only be one mepc write per ecall so I don't think it'd be a huge issue but as above it seems a safe one to whitelist so we can do that.

Copy link
Contributor

@nbdd0121 nbdd0121 Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For exception handling I think it's not uncommon to save mepc & all registers to memory, call some routine, and unconditionally restore mepc. Also, for things like ecall, it's also needed to unconditionally increment by 4. But you're right they're not touched for interrupt handling, which matters more for than synchronous exceptions.

//
// No flush is triggered for a small number of specific CSRs. These are ones that have been
// specifically identified to be a) likely to be modifed in exception handlers and b) safe to
// alter without a flush.
assign no_flush_csr_addr = csr_addr_o inside {CSR_MSCRATCH, CSR_MEPC};

assign csr_pipe_flush = (csr_op_en_o == 1) &&
(csr_op_o inside {CSR_OP_WRITE, CSR_OP_SET, CSR_OP_CLEAR}) &&
!no_flush_csr_addr;

////////////////
// Controller //
Expand Down
Loading