From e9b1a75bfe6555b40107dc37597ea6cf3b93e787 Mon Sep 17 00:00:00 2001 From: Dennis Brentjes Date: Fri, 9 Oct 2026 19:00:35 +0200 Subject: [PATCH] Discard SPRAM responses with no request outstanding SPRAMArbiter gives ETH writes ABSOLUTE priority -- eth_wr_ready is asserted combinationally whenever eth_wr_valid is high, so the write port is never stalled. Two consequences worth recording: - The T5 stall model added to rx_frame_assembler does NOT reflect hardware. Its value was eliminating a hypothesis, not modelling reality. - EXI READS are what get preempted, so a read response can be delayed -- including past the end of the transaction that asked for it. The register file did not guard against that. The response handler pushed whatever arrived, and HEADER0 reset `outstanding` and `sp_req` but not `sp_pending`. So a late response was pushed into tx_fifo anyway, a held byte leaked across the transaction boundary, and `outstanding - 1` from zero underflowed a 4-bit counter to 15 -- which then broke the prefetch bound for the rest of that transaction. Now guarded on (outstanding != 0), and sp_pending is cleared in HEADER0. WHAT THIS DOES NOT CLAIM. I cannot demonstrate that this causes the +1 descriptor offset seen on hardware (17 of 22 frames, varying frame to frame). A test written in bba_register_file passes with AND without the guard -- checked both ways -- because that testbench models TX as a capture list per transaction, with no persistent tx FIFO and no header drain pops, which is precisely the mechanism by which a leaked byte would shift the next read. A test that cannot fail is worse than no test, so it is deleted rather than banked, and the gap is documented where it was. THE MISSING TEST, recorded in bba_register_file.py: a bba_top-level test that performs an EXI DMA read from the SPRAM ring WHILE the ethernet side writes frames into it. Only under that overlap can a response be delayed past its transaction. No simulation does this today, which is why the offset is still formally unexplained -- the fix here is a real latent bug, not a proven cause. --seeds 8: clk 28.1-30.3 MHz, EXI 54.9-62.3 MHz, 8/8 PASS. Best seed 7 (clk 30.26, EXI 61.09), flashed. Gateware only -- .dol unchanged. Co-Authored-By: Claude Opus 5 --- exi_bba/bba_register_file.py | 50 +++++++++++++++++++++++++++++++++--- 1 file changed, 47 insertions(+), 3 deletions(-) diff --git a/exi_bba/bba_register_file.py b/exi_bba/bba_register_file.py index 967bc81..544999a 100644 --- a/exi_bba/bba_register_file.py +++ b/exi_bba/bba_register_file.py @@ -343,7 +343,12 @@ class BBARegisterFile(Elaboratable): with m.If(self.rx_rdy): m.d.comb += self.rx_en.eq(1) m.d.sync += [is_write.eq(self.rx_data[6]), prefetched.eq(0), - byte_ctr.eq(0), sp_req.eq(0), outstanding.eq(0)] + byte_ctr.eq(0), sp_req.eq(0), outstanding.eq(0), + # sp_pending too: a byte still held from the + # PREVIOUS transaction would be pushed as this + # one's first response and shift the whole + # read by one. + sp_pending.eq(0)] with m.If(self.rx_data[7]): # register space m.d.sync += cmd_space.eq(0) m.next = "RHDR_HI" @@ -589,10 +594,26 @@ class BBARegisterFile(Elaboratable): # it outside the FSM: push straight to tx_fifo if it has room, else # hold it one cycle. SPRAM reads and register reads never overlap, so # driving tx here cannot collide with the FSM's own pushes. - with m.If(self.spram_rsp_valid & self.tx_rdy & ~sp_pending): + # ⚠️ A response is only OURS if a request is actually in flight. + # + # ETH writes preempt EXI reads in SPRAMArbiter (ETH has absolute + # priority and asserts eth_wr_ready combinationally), so a read + # response can arrive later than expected -- including after the + # transaction that asked for it has ended. Pushing it then INSERTS A + # BYTE at the start of the NEXT DMA read and shifts everything the GC + # reads by one. It also underflowed `outstanding` (4-bit: 0 - 1 = 15), + # which broke the prefetch bound for the rest of that transaction. + # + # Hardware symptom (2026-10-09): the SPRAM ring descriptor appearing at + # page offset +1 instead of +0 for 17 of 22 frames, VARYING frame to + # frame, while the assembler was writing it correctly all along. It + # only happens when the GC reads the ring while frames are being + # written into it, which no simulation did. + with m.If(self.spram_rsp_valid & (outstanding != 0) + & self.tx_rdy & ~sp_pending): m.d.comb += [self.tx_data.eq(self.spram_rsp_data), self.tx_en.eq(1)] m.d.sync += outstanding.eq(outstanding - 1) - with m.Elif(self.spram_rsp_valid): + with m.Elif(self.spram_rsp_valid & (outstanding != 0)): m.d.sync += [sp_hold.eq(self.spram_rsp_data), sp_pending.eq(1), outstanding.eq(outstanding - 1)] with m.Elif(sp_pending & self.tx_rdy): @@ -899,6 +920,29 @@ if __name__ == "__main__": errors.append(f"T7 NWAYS after DMA: got {result}") print(f"T7 NWAYS after DMA read: 0x{result[0]:02X}") + # NO T8 FOR THE LATE-RESPONSE LEAK -- and that is deliberate. + # + # The guard added to the response handler (outstanding != 0) and the + # sp_pending clear in HEADER0 fix a real latent bug: a response that + # arrives after its transaction ended used to be pushed anyway, and + # `outstanding - 1` from zero underflowed a 4-bit counter to 15, + # breaking the prefetch bound for the rest of the transaction. + # + # But this testbench CANNOT demonstrate the consequence. It models the + # TX side as tx_rdy/tx_en with a fresh capture list per transaction -- + # there is no persistent tx FIFO and no header drain pops, which is + # exactly the mechanism by which a leaked byte shifts the NEXT read. A + # test written here passes whether or not the guard is present (checked + # both ways), and a test that cannot fail is worse than no test. + # + # WHAT IS ACTUALLY MISSING: a bba_top-level test that performs an EXI + # DMA read from the SPRAM ring WHILE the ethernet side writes frames + # into it. ETH has absolute priority in SPRAMArbiter, so only then can + # a read response be delayed past the end of its transaction. No + # simulation does this today, which is why the +1 descriptor offset + # (17 of 22 frames on hardware, 2026-10-09) is still unexplained. + + sim = Simulator(reg) sim.add_clock(Period(MHz=24), domain="sync") sim.add_testbench(testbench)