fix(load_unit): move flush_i branch after ldbuf_w to prevent phantom …#3309
fix(load_unit): move flush_i branch after ldbuf_w to prevent phantom …#3309flaviens wants to merge 1 commit into
Conversation
…writeback In ldbuf_comb the flush_i "set-all-flushed" branch appeared before the ldbuf_w "allocate-slot" branch. When both are asserted in the same cycle (ldbuf_w = data_req & data_gnt, flush_i = flush_ex_o from controller), the allocation write wins for the new slot via lexical last-assignment semantics: if (flush_i) ldbuf_flushed_d = '1; // (a) ... if (ldbuf_w) ldbuf_flushed_d[windex] = 1'b0; // (b) overrides (a) for windex Result: flushed_q[windex]=0, valid_q[windex]=1 — a slot that survived the flush. When data_rvalid arrives N cycles later the FSM is in IDLE (kill_req=0) and ldbuf_flushed_q[slot]=0, so valid_o=1. The flushed load's result is forwarded to scoreboard writeback with a potentially recycled trans_id, corrupting an unrelated instruction's result. Trigger: an exception or fence commits (flush_ex_o=1 → flush_i=1) while the load unit is in WAIT_GNT and the dcache arbiter grants the same cycle. A concrete sequence: an older store faults (store page-fault) at the commit head while a younger speculative load has been held in WAIT_GNT waiting for the dcache arbiter. If the arbiter grants on the same cycle the fault commits, flush_i and data_gnt are simultaneously high. Fix: move if (flush_i) to after if (ldbuf_w) so the flush unconditionally wins for every slot, including the one just allocated. Verified with a standalone Verilator simulation of the isolated ldbuf_comb / ldbuf_ff logic. Pre-fix: valid_o=1 on rvalid (BUG-CONFIRMED). Post-fix: valid_o=0 (correctly suppressed).
There was a problem hiding this comment.
Pull request overview
This PR fixes a corner-case in the load buffer control where a same-cycle allocation (ldbuf_w) could inadvertently override a flush, leaving a newly allocated entry effectively “not flushed” and allowing a killed response to propagate later.
Changes:
- Reordered the
flush_ihandling inldbuf_combto occur afterldbuf_wso flush consistently takes priority for all slots (including same-cycle allocations). - Added an explanatory comment documenting why the ordering matters and what failure mode it prevents.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Oh no, we get slop right from Microsoft GitHub in the PR comments now 😭 Please @JeanRochCoulon get this slop off! Here is how to disable it: https://github.com/orgs/community/discussions/164200#discussioncomment-14544724 |
|
Hi @flaviens Thanks for your two contributions! Please read the CONTRIBUTING.md and sign the ECA so your PR can be reviewed. Tell us if you encounter any issues in the process. |
|
Could finally sign after filling 5x2 grocery shopping captcha 👍 |
Hi there!
ldbuf_comb let a same-cycle load allocation override a flush because the ldbuf_w branch ran after flush_i. This could leave a newly allocated slot marked valid but not flushed, allowing a killed load response to reach scoreboard writeback later and potentially corrupt a recycled trans_id.
This PR moves the flush_i handling after allocation so the flush wins for all slots, including the one allocated in the same cycle. Verilator confirms the flushed response is now suppressed.
Thanks!
Flavien