Refactor illegal instruction warning handling - #328
MikeOpenHWGroup merged 4 commits into
Conversation
The controller has a bit of behavioural code that prints a warning if an illegal instruction is decoded. This PR will terminate the simulation if the PC does not advance (such circumstances will result in an infinite loop that spews "Illegal instruction..." messages every clock cycle).
There was a problem hiding this comment.
Pull request overview
This PR updates the simulation-only illegal-instruction warning logic in cve2_controller to detect when the core is repeatedly decoding an illegal instruction without making forward progress, and to terminate the simulation to avoid infinite warning spam.
Changes:
- Adds a “last PC” tracker in the controller’s simulation-only block.
- Attempts to terminate the simulation when the same illegal instruction is seen at an unchanged PC.
- Minor formatting updates to
$displaytime formatting.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| logic [31:0] pc_last = 0; | ||
| always_ff @(negedge clk_i) begin | ||
| // print warning in case of decoding errors | ||
| // print warning in case of decoding errors (terminate if the PC doesn't advance). | ||
| if ((ctrl_fsm_cs == DECODE) && instr_valid_i && !instr_fetch_err_i && illegal_insn_d) begin | ||
| $display("%m @ %t: Illegal instruction (hart %0x) at PC 0x%h: 0x%h", $time, cve2_core.hart_id_i, | ||
| cve2_id_stage.pc_id_i, cve2_id_stage.instr_rdata_i); | ||
| $display("%m @ %0t: Illegal instruction (hart %0x) at PC 0x%h: 0x%h", |
There was a problem hiding this comment.
Hi @MikeOpenHWGroup,
On top of what Copilot said, I think that the premise that two illegal instructions decoded in a row on the same PC value does not necessarily mean an issue deserving termination. Consider that between two illegal instructions the PC may change to other values on legal instructions.
An indication of a problem where the termination of the test is necessary is when PC is stuck with the same upon many illegal instructions, i.e. when it seems to indicate that there is a indefinite loop.
On this case, maybe we don't need such kind of changes on the code, but rather rely on the timeout for the execution of a test.
There was a problem hiding this comment.
The purpose of this code is to catch back-to-back illegal instructions at the same address. In this case, the core is stuck and the current code will create a very large (and useless) logfile. It is important to terminate such simulations. I have pushed in a version that addresses Copilot's comments.
cairo-caplan
left a comment
cairo-caplan
left a comment
There was a problem hiding this comment.
Hello @MikeOpenHWGroup ,
I approve the changes proposed by this PR. I would like to add two points though:
- Signal
prev_illegalcan effectively be removed, as it is not used anymore: it is implied that if the current instruction is illegal and it has the same PC as the previous one, the previous instruction was also illegal. - This code may change again the future to acknowledge instructions routed to a coprocessor through the CV-X-IF, since they are illegal to the CVE2 itself. But that does not block this to be merged now
|
Good feedback @cairo-caplan. I removed |
The controller has a bit of behavioural code that prints a warning if an illegal instruction is decoded. This PR will terminate the simulation if the PC does not advance (such circumstances will result in an infinite loop that spews "Illegal instruction..." messages every clock cycle).