soc_event_generator: preserve r_err set on concurrent APB read race#103
Open
flaviens wants to merge 1 commit into
Open
soc_event_generator: preserve r_err set on concurrent APB read race#103flaviens wants to merge 1 commit into
flaviens wants to merge 1 commit into
Conversation
The single always_ff at line 197 mixes blocking (=) and non-blocking
(<=) assignments and writes r_err on two paths:
- the per-source set loop:
for (int i=0;i<EVNT_NUM;i++)
if(s_err[i])
r_err[i] = 1'b1; // blocking
- the APB read-clear branch (PSEL && PENABLE && ~PWRITE):
case (s_apb_addr)
REG_ERR_0: r_err[31:0] = 'h0; // blocking
...
Both use blocking assignments and execute in lexical order. The set
loop runs first, then the read-clear runs and overwrites the bits
just set. If a queue overflow at index i in [0..31] fires (s_err[i]
high) on the same HCLK edge as the data phase of a SW read of
REG_ERR_0, the cycle plays out as:
step 1: r_err[i] = 1'b1;
step 2: r_err[31:0] = 'h0; <-- nukes step 1 for i in [0,31]
The new error is silently dropped. Software does not see it via
PRDATA either, because the read multiplexer captures the *pre-cycle*
value of r_err[31:0] in a separate always_comb. The overflow is
invisible to software both in the read result and in subsequent
reads.
The race window is the heavily-loaded scenario in which SW would
*want* to drain errors: soc_event_queue.err_o is high precisely when
its 2-bit counter is at max and a new event_i arrives.
Reproducer (Verilator 5.046):
- APB-write REG_FC_MASK_0 = 0 so per_events_i[0] gets routed and
the testbench's fc_event_ready_i = 0 prevents the arbiter from
draining the queue.
- Pulse per_events_i[0] until soc_event_queue's counter saturates
and r_err[0] sticky-sets to 1.
- APB-read REG_ERR_0 while pulsing per_events_i[0] in the data
phase. Pre-read r_err[0] = 1; post-read r_err[0] = 0 (the
in-flight overflow event was lost).
- With this commit applied, post-read r_err[0] = 1 (sticky).
Fix: move the per-source set loop to *after* the APB read-clear
branch within the else block. Same lexical ordering rule, but with
set after clear so the sticky-set wins on a same-cycle race. Other
behaviour (mask register writes, r_apb_events default) is untouched.
There was a problem hiding this comment.
Pull request overview
This PR fixes a same-cycle race in soc_event_generator where a newly asserted per-source error (s_err) could be lost if it coincided with an APB read-to-clear of an error register (REG_ERR_*). The change ensures the error “set” operation has priority over the read-clear in the same clock edge, preserving sticky error visibility to software.
Changes:
- Reordered sequential logic so per-source
r_errsticky-set occurs after the APB read-clear path. - Added an inline comment clarifying the intended set-after-clear priority for same-cycle races.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi there!
soc_event_generator could drop a newly raised r_err bit when a queue overflow occurs on the same HCLK edge as an APB read-clear of REG_ERR_0. The set loop runs first, but the later blocking read-clear assignment overwrites it, and software never sees the new error in PRDATA or later reads.
Move the per-source r_err set loop after the APB read-clear branch so same-cycle set-after-clear preserves sticky error bits
Thanks!
Flavien