Skip to content

Commit 64dde79

Browse files
author
Marcos
committed
fix(axi4_mem_model): address self-review findings on PR #8
Self-review by code-reviewer agent flagged 3 HIGH findings: HIGH 1. Write-write conflict between loader and AXI4 FSM was undefined behaviour per SV NBA semantics across separate always blocks. Verilator happens to make the loader win but other simulators may differ. Fix: collapsed the standalone loader always block into the AXI4 write FSM's always block, sequenced as the LAST NBA in the procedural step. SV guarantees the last NBA in a single procedural block wins on conflicts — deterministic loader- wins-on-conflict semantics across all SV-compliant tools. HIGH 2. Misaligned loader_addr silently corrupted adjacent 32-bit slots (or wrote past bit 255 of the cache line at offsets 29-31). Fix: added 'assert (loader_addr[1:0] == 2'b00) else $error' inside the loader branch. Misaligned addresses now produce a clear simulator error instead of silent corruption. Synthesis ignores the assertion. HIGH 3. Loader always block had no reset arm — inconsistent with every other always block in the file. Fix: subsumed by HIGH 1 fix. The merged always block inherits the AXI4 FSM's '@(posedge clk or negedge rst_n)' sensitivity list and reset semantics. Also addressed LOW finding: LOW. README.md in src/popsolutions/axi4/ did not document the loader port contract, semantics, conflict resolution rule, alignment requirement, or synthesis tie-off convention. Fix: appended a 'Memory loader back-door' section covering all of the above. Test status (verified locally, all green): axi4_mem_model: 6/6 pass (no regression) axi4_master_simple: 5/5 pass (no regression) core_axi4_adapter: 5/5 pass (no regression) inner_jib_top (IJ7EA): 2/2 pass (no regression — full sum_ints still runs end-to-end) 18/18 across both repos. Deferred MEDIUM findings (not blocking): - Test coverage gap for slots 3-7 within a cache line and explicit misalignment negative test. Will land as a follow-up PR with test_loader_all_slots and test_loader_misalign_errors. - Bit-width arithmetic note already covered by the new alignment assertion (the unsafe path now errors at runtime). Signed-off-by: Marcos <m@pop.coop>
1 parent c12a6a5 commit 64dde79

2 files changed

Lines changed: 61 additions & 5 deletions

File tree

src/popsolutions/axi4/README.md

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,3 +99,40 @@ Signal naming convention:
9999
- [ADR-006 — Internal bus: AXI4](../../../docs/popsolutions/ADRS.md)
100100
- [ADR-002 — Project topology: MAST trunk + N Sails](../../../docs/popsolutions/ADRS.md)
101101
- [`docs/popsolutions/architecture/PARAMETER_TAXONOMY.md`](../../../docs/popsolutions/architecture/PARAMETER_TAXONOMY.md)
102+
103+
## Memory loader back-door (since v0.2)
104+
105+
`axi4_mem_model` carries three additional input ports for testbench /
106+
boot-ROM-style program loading:
107+
108+
| Port | Width | Meaning |
109+
|---|---|---|
110+
| `loader_en` | 1 | Pulse high for one cycle to commit a write |
111+
| `loader_addr` | `phys_addr_width` | **Byte address**, must be 4-byte aligned |
112+
| `loader_data` | `data_width` (32 bits) | Datum to land in the addressed slot |
113+
114+
**Semantics.** When `loader_en` is high on a rising clock edge, the
115+
32-bit `loader_data` is written into the correct slot of the targeted
116+
256-bit cache line. The slot is selected by `loader_addr[4:2]` (which
117+
of the eight 32-bit slots within the 32-byte line). The cache line
118+
index is `loader_addr[OFF_WIDTH +: IDX_WIDTH]`.
119+
120+
**Conflict resolution.** The loader write is sequenced as the LAST
121+
non-blocking assignment in the AXI4 write FSM's `always` block, so on
122+
simultaneous loader-and-AXI4 writes to the same cache line the loader
123+
wins by SystemVerilog NBA semantics — not by simulator-specific cross-
124+
block ordering.
125+
126+
**Alignment enforcement.** A simulation `assert` fires if `loader_addr`
127+
is not 4-byte aligned (would silently corrupt adjacent slots otherwise).
128+
129+
**Synthesis.** Production builds tie `loader_en` to `1'b0`. The `if
130+
(loader_en) ...` branch becomes constant-false and standard synthesis
131+
tools (Yosys, Synopsys, Cadence) prune the entire loader path — no
132+
backing logic, no extra registers. The unused `loader_addr` and
133+
`loader_data` input ports may leave port-buffer cells in the netlist
134+
under hierarchical synthesis flows; this is negligible for our use.
135+
136+
Test wrappers that don't need the back-door tie all three signals to
137+
constants in their instantiation — see `verif/axi4_master_simple/` and
138+
`verif/core_axi4_adapter/`.

src/popsolutions/axi4/axi4_mem_model.sv

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -96,11 +96,13 @@ module axi4_mem_model #(
9696
// bit position within the cache line where the 32-bit datum lands
9797
wire [OFF_WIDTH+2:0] loader_bit_off = {loader_byte_off, 3'b000};
9898

99-
always @(posedge clk) begin
100-
if (loader_en) begin
101-
mem[loader_idx][loader_bit_off +: data_width] <= loader_data;
102-
end
103-
end
99+
// The actual loader write is sequenced AT THE END of the AXI4 write
100+
// FSM's always block (see write side FSM below). This guarantees that
101+
// when the loader and the AXI4 write fire in the same cycle on the
102+
// same cache line, the loader's NBA is the LAST one in the procedural
103+
// step and therefore wins by SystemVerilog NBA semantics — not by
104+
// simulator-specific cross-block ordering. Production synthesis with
105+
// loader_en tied to 0 prunes the loader branch entirely.
104106

105107
// ====================================================================
106108
// Write side FSM
@@ -192,6 +194,23 @@ module axi4_mem_model #(
192194

193195
default: wr_state <= WR_IDLE;
194196
endcase
197+
198+
// ============================================================
199+
// Loader back-door write — placed AT THE END of this block so
200+
// its NBA fires after the AXI4 FSM's NBAs in the same step.
201+
// Loader-wins-on-conflict is therefore deterministic.
202+
// ============================================================
203+
if (loader_en) begin
204+
// Misaligned loader_addr would silently corrupt adjacent
205+
// 32-bit slots (or write past bit 255 of the cache line).
206+
// Catch this in simulation before it propagates.
207+
assert (loader_addr[1:0] == 2'b00)
208+
else $error(
209+
"axi4_mem_model: loader_addr 0x%0h not 4-byte aligned",
210+
loader_addr
211+
);
212+
mem[loader_idx][loader_bit_off +: data_width] <= loader_data;
213+
end
195214
end
196215
end
197216

0 commit comments

Comments
 (0)