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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[rtl] Flush pipe on all CSR modifications #2214

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
37 changes: 7 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 @@ -509,36 +509,13 @@ 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 any 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
Collaborator 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
Collaborator 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
Collaborator 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
Collaborator 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.

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

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