axi_demux: enforce exactly MaxTrans in-flight transactions (fixes #249) - #440
axi_demux: enforce exactly MaxTrans in-flight transactions (fixes #249)#440arjunsv98 wants to merge 5 commits into
Conversation
08dbeae to
88939f5
Compare
imchenwu
left a comment
imchenwu
left a comment
There was a problem hiding this comment.
Hi, thx for working on this. :) As this PR requires exposing a new port, we'll have to merge it in the next major release, so I modified the base branch to devel.
|
|
||
| `include "axi/typedef.svh" | ||
|
|
||
| module tb_axi_demux_maxtrans #( |
There was a problem hiding this comment.
Is this tb necessary? I suggest to add assertions inside the axi_demux_id_counters module instead.
There was a problem hiding this comment.
Replied in the main thread with more detail, but short version: the existing tb configs all use non-power-of-2 MaxTrans, so module assertions alone would never fire in CI — this tb is what actually builds a triggering config. Can add assertions on top or drop the tb, whichever you prefer.
|
Thanks for taking a look! Applied your About the tb — I considered moving the check into assertions in Happy to add the assertions on top if you'd like both, or drop the tb if you feel strongly — just wanted to flag why I kept it for now. |
88939f5 to
dc46d0c
Compare
The per-ID in-flight counters were sized with idx_width(MaxTrans) bits, which can only represent values 0 to MaxTrans-1. As a result: - For power-of-two MaxTrans (including the module default of 8), the counters reported full at MaxTrans-1, silently delivering one less outstanding transaction than configured on both the AR and AW paths as well as the open-W-burst counter. - For other values, the all-ones full condition allowed more than MaxTrans outstanding transactions (e.g. 15 for MaxTrans = 10). Widen the counters to idx_width(MaxTrans + 1) bits and compare against MaxTrans directly instead of the counter's saturation value, so the configured limit is enforced exactly. The comparison uses >= rather than == because a simultaneous push and ATOP injection advances a counter by two. Add a directed regression, tb_axi_demux_maxtrans, which issues back-to-back same-ID reads into a demux whose selected master port withholds all R responses: the number of accepted AR handshakes is exactly the counter limit. Before this change it stalls at 7 of a configured 8; after, at exactly MaxTrans for both power-of-two and other values. Fixes pulp-platform#249
Review feedback: with 2**CounterWidth outstanding transactions the counter wraps and any_outstanding_trx_o would report no outstanding transactions. Include the overflow flag in the occupied computation.
dc46d0c to
5d78989
Compare
|
Hi, I checked the current CI. Actually |
Review feedback: replace the specialized testbench with an elaboration-time assertion. The assertion fails as soon as any instantiation configures a counter width that cannot hold MaxTrans, which covers configurations reached through other DUTs (e.g. tb_axi_dw_upsizer instantiates axi_demux with MaxTrans=4 through axi_dw_upsizer) without a dedicated test.
|
Fair enough, you're right — I'd only checked the top-level tb parameters and missed configs reaching the demux through other DUTs. Swapped the tb for an elaboration-time assertion in |
| // inclusive), so they are sized for `MaxTrans + 1` states. Sizing them with | ||
| // `idx_width(MaxTrans)` lets them saturate at `MaxTrans - 1` when `MaxTrans` is a power of | ||
| // two, silently reducing the configured concurrency by one transaction (issue #249). | ||
| localparam int unsigned IdCounterWidth = cc_pkg::idx_width(MaxTrans + 1); |
There was a problem hiding this comment.
| localparam int unsigned IdCounterWidth = cc_pkg::idx_width(MaxTrans + 1); | |
| localparam int unsigned IdCounterWidth = cc_pkg::cnt_width(MaxTrans); |
There was a problem hiding this comment.
TIL cnt_width exists — that's exactly the right helper here (and kind of poetic that the bug was idx_width being used where cnt_width belonged). Applied.
| // pragma translate_off | ||
| initial begin : validate_params | ||
| counter_holds_maxtrans : assert (2**CounterWidth > MaxTrans) else | ||
| $fatal(1, "CounterWidth (%0d bits, maximum value %0d) cannot represent MaxTrans (%0d): \ | ||
| the counters would report full at %0d in-flight transactions.", | ||
| CounterWidth, 2**CounterWidth - 1, MaxTrans, 2**CounterWidth - 1); | ||
| end | ||
| // pragma translate_on | ||
|
|
There was a problem hiding this comment.
Maybe replace by runtime assertions, see the other comment.
Review feedback: size the counters with cc_pkg::cnt_width, which exists for exactly this purpose, and watch the in-flight limit with the suggested runtime assertion properties (full only at or above MaxTrans, no push at or beyond MaxTrans) alongside the existing underflow check.
2 participants
Summary
The demux's per-ID in-flight counters are sized with
idx_width(MaxTrans)bits — enough to indexMaxTransvalues (0..MaxTrans-1), not enough to count toMaxTrans. Consequently:MaxTrans— including the module default of 8 — the counters report full atMaxTrans - 1: a demux configured for 8 outstanding transactions accepts only 7, on the AR path, the AW path, and the open-W-burst (w_open) ceiling.MaxTrans = 10).This is exactly the behavior diagnosed by the reporter of #249 in 2022; this PR implements the fix (with exact-limit semantics) and adds the regression that was missing.
Fix
axi_demux_simple: widenIdCounterWidthtoidx_width(MaxTrans + 1); gatew_openwith< MaxTransinstead of!= all-ones; passMaxTransto the ID counters.axi_demux_id_counters: newMaxTransparameter (defaults to the legacy saturation value, so standalone instantiations are unaffected); full condition comparesin_flight >= MaxTransinstead of&in_flight.>=rather than==because a simultaneous push + ATOP injection advances a counter by 2.Note on semantics: this makes
MaxTransan exact limit. Non-power-of-two configurations previously enjoyed slack up to the counter's all-ones value (e.g. 15 forMaxTrans=10); they are now limited to exactlyMaxTrans. This matches the parameter's documentation (see also #336).Verification
New directed test
tb_axi_demux_maxtrans: back-to-back same-ID reads into a demux whose selected master port accepts every AR but withholds all R responses, so the in-flight count can only grow; the number of accepted AR handshakes equals the true counter limit.MaxTrans = 8MaxTrans = 10Full
tb_axi_xbarregression (Verilator): 8072 checks, 0 failures, in both the default (10/6) and an all-power-of-two (8/8)MaxMstTrans/MaxSlvTransconfiguration.No existing test could observe this bug: the shipped testbenches all use non-power-of-two limits, and none measures achieved concurrency — the new directed test closes that gap.
Fixes #249