From 92f7d716d910fe49692f57911711471eb7ec3c13 Mon Sep 17 00:00:00 2001 From: Dennis Brentjes Date: Fri, 9 Oct 2026 12:36:07 +0200 Subject: [PATCH] Add RBFI ring-overflow protection: drop frames instead of overwriting RXFrameAssembler ignored RRP entirely, so it lapped the reader: a traffic burst overwrote pages the GC had not read, and the "descriptor" the driver then parsed was frame data from whatever had replaced it. That is where the hardware's 00 00 00 01 came from -- not a dropped write. This is a product bug, not just a netboot one. Until now ANY burst could corrupt a frame Swiss was midway through reading, with nothing to indicate it had happened. The assembler now takes the GC's read pointer and computes free space as (rrp - rwp - 1) mod 15. On a frame start with fewer than 6 free pages -- the worst case for 1518 bytes plus a 4-byte descriptor -- it enters DROP_FRAME, discards to EOF, and pulses rx_overflow, which sets IR[7] (RBFI). That is the bit the GC's IMR = 0x86 already enables, so a driver that loses a frame is told so instead of reading corruption. Deliberate details: - rx_ready stays HIGH while dropping. Stalling the ethernet master instead would back-pressure into the W5100's own buffer and turn a dropped frame into a wedged receiver. - Six pages are reserved up front because a frame's length is not known until EOF. Starting with less risks running into unread data partway through, which is worse than refusing cleanly. - have_room is REGISTERED, not combinational. Feeding a mod-15 subtract and comparator into the FSM branch cost real depth in `sync`, the domain with the least margin: clk fell 27.8-32.2 -> 27.0-28.0 MHz. Registering it restored 28.4-31.4. One cycle of staleness is harmless -- rwp moves only at end-of-frame and rrp only when the GC writes it. T4 proves the property that matters, and the negative control REPRODUCES THE HARDWARE FAULT IN SIMULATION: with the check removed the unread page reads back as [245, 248, 251, 254, ...] -- frame data where a descriptor belongs. First time one of today's RX faults has been reproducible on this side rather than inferred from the console. T4 also checks the receiver un-latches once RRP advances, so a full ring cannot kill RX permanently. Fixed a test artifact in the same pass: rx_overflow fires on the very tick that consumes the EOF byte, inside send_frame, so polling for a 1-cycle pulse afterwards missed it and a correctly-dropped frame looked like a stall. Both pulses are now latched in the tick helper. netboot also clears IR (including RBFI) at init so a stale bit is not inherited from an earlier run. --seeds 8: clk 28.4-31.4 MHz, EXI 55.2-62.7 MHz, 8/8 PASS. Best seed 4 (clk 31.39, EXI 61.85), flashed. Co-Authored-By: Claude Opus 5 --- exi_bba/bba_register_file.py | 18 +++++ exi_bba/bba_top.py | 5 ++ exi_bba/rx_frame_assembler.py | 126 +++++++++++++++++++++++++++++++++- gc_bba_test/source/netboot.c | 4 ++ 4 files changed, 150 insertions(+), 3 deletions(-) diff --git a/exi_bba/bba_register_file.py b/exi_bba/bba_register_file.py index c08e443..967bc81 100644 --- a/exi_bba/bba_register_file.py +++ b/exi_bba/bba_register_file.py @@ -81,6 +81,16 @@ class BBARegisterFile(Elaboratable): # through both byte FIFOs. This output is what arms that path. self.id_phase = Signal(init=1) + # ── Ring-overflow protection (RBFI) ─────────────────────────────── + # RRP out to RXFrameAssembler so it can tell how much of the ring is + # still unread and DROP frames instead of overwriting them. + self.rrp = Signal(8, init=_RX_PAGE_FIRST) + # Pulse in: a frame was dropped because the ring was full. Sets + # IR[7] (RBFI), which the GC's IMR = 0x86 enables alongside RI and TI, + # so the driver learns it lost a frame instead of silently reading + # corrupted data. + self.rx_overflow = Signal() + # ── Interrupt (exi domain) ──────────────────────────────────────── self.exi_int_n = Signal(init=1) @@ -597,6 +607,7 @@ class BBARegisterFile(Elaboratable): with m.Case(0x03): m.d.sync += r_cmd_ir.eq(r_cmd_ir & ~cmd_wdata) # ── Interrupt output ───────────────────────────────────────────── + m.d.comb += self.rrp.eq(r_rrp_lo) m.d.sync += self.exi_int_n.eq(~(r_ir & r_imr).any()) # ── Consume RWP updates from sync domain ────────────────────────── @@ -618,6 +629,13 @@ class BBARegisterFile(Elaboratable): with m.If(self.tx_irq_i): m.d.sync += r_ir[2].eq(1) # TI bit + + # RBFI: the ring was full and a frame was discarded. Reported rather + # than hidden -- the previous behaviour was to overwrite unread pages, + # so the driver got corrupted frames with no signal that anything had + # gone wrong. + with m.If(self.rx_overflow): + m.d.sync += r_ir[7].eq(1) m.d.sync += r_ncra[1:3].eq(0) # clear ST bits m.d.sync += tx_queued.eq(0) # ready for the next frame diff --git a/exi_bba/bba_top.py b/exi_bba/bba_top.py index f91a564..f7e543f 100644 --- a/exi_bba/bba_top.py +++ b/exi_bba/bba_top.py @@ -191,6 +191,11 @@ class BBATop(Elaboratable): self.exi_miso.eq(cap.spi_miso), self.exi_miso_next.eq(cap.miso_next), cap.id_phase.eq(reg.id_phase), + # Ring-overflow protection: the assembler needs the GC's read + # pointer to know what it must not overwrite, and reports a + # dropped frame back as RBFI (IR[7]). + asm.rrp.eq(reg.rrp), + reg.rx_overflow.eq(asm.rx_overflow), self.int_n .eq(reg.exi_int_n), ] diff --git a/exi_bba/rx_frame_assembler.py b/exi_bba/rx_frame_assembler.py index fd70847..8e2181f 100644 --- a/exi_bba/rx_frame_assembler.py +++ b/exi_bba/rx_frame_assembler.py @@ -32,6 +32,8 @@ __all__ = ["RXFrameAssembler"] _RX_PAGE_FIRST = 0x01 _RX_PAGE_LAST = 0x0F _PAGES_TOTAL = _RX_PAGE_LAST - _RX_PAGE_FIRST + 1 # 15 +# Worst-case pages one frame can occupy: 1518 + 4 descriptor bytes -> 6 pages. +_MAX_FRAME_PAGES = 6 class RXFrameAssembler(Elaboratable): @@ -78,6 +80,16 @@ class RXFrameAssembler(Elaboratable): # RX gate from NCRA SR bit self.rx_enabled = Signal() + # ── Ring-overflow protection (RBFI) ─────────────────────────────── + # The GC's read pointer, so we can tell how much of the ring is still + # unread. Without it this module overwrote pages the GC had not read + # yet: a traffic burst corrupted whatever frame the driver was midway + # through, with no indication that it had happened. + self.rrp = Signal(8, init=_RX_PAGE_FIRST) + # Pulses when a frame had to be DROPPED because the ring was full. + # Drives IR[7] (RBFI), which the GC's IMR enables (0x86). + self.rx_overflow = Signal() + def elaborate(self, platform): m = Module() @@ -103,6 +115,37 @@ class RXFrameAssembler(Elaboratable): # Default: no pulses m.d.sync += self.rx_irq.eq(0) m.d.sync += self.rx_wptr_w_en.eq(0) + m.d.sync += self.rx_overflow.eq(0) + + # ── How many pages may we still consume? ────────────────────────── + # rwp == rrp means EMPTY, so writing all 15 pages would wrap back to + # "empty" and lose the lot. Usable = (rrp - rwp - 1) mod 15, which is + # 14 when the ring is empty. + rrp_s = Signal(4) + m.d.comb += rrp_s.eq(Mux((self.rrp >= _RX_PAGE_FIRST) + & (self.rrp <= _RX_PAGE_LAST), + self.rrp, _RX_PAGE_FIRST)) + raw = Signal(5) + free = Signal(5) + with m.If(rrp_s >= rwp): + m.d.comb += raw.eq(rrp_s - rwp) + with m.Else(): + m.d.comb += raw.eq(rrp_s + _PAGES_TOTAL - rwp) + m.d.comb += free.eq(Mux(raw == 0, _PAGES_TOTAL - 1, raw - 1)) + + # A frame's length is not known until EOF, so reserve the worst case: + # 1518 bytes + a 4-byte descriptor is 6 pages. Starting a frame with + # less room than that risks running into unread data partway through, + # which is worse than refusing it outright. + # REGISTERED, not combinational. The free-space calculation is a + # mod-15 subtract plus a comparator, and feeding that into the FSM's + # branch condition cost real depth in `sync` -- the domain with the + # least margin in this design (clk fell 27.8-32.2 -> 27.0-28.0 MHz + # when this was comb). One cycle of staleness is harmless: rwp only + # moves at end-of-frame and rrp only when the GC writes it, so both + # are many cycles old by the time the next SOF arrives. + have_room = Signal() + m.d.sync += have_room.eq(free >= _MAX_FRAME_PAGES) # Combinatorial outputs m.d.comb += total_len.eq(data_ctr + 4) @@ -112,7 +155,13 @@ class RXFrameAssembler(Elaboratable): with m.State("IDLE"): m.d.comb += self.rx_ready.eq(0) m.d.sync += self.eth_wr_valid.eq(0) - with m.If(self.rx_valid & self.rx_sof & self.rx_enabled): + with m.If(self.rx_valid & self.rx_sof & self.rx_enabled + & ~have_room): + # Ring full: discard this frame rather than overwrite data + # the GC has not read. This is what the real BBA's RBFI + # reports, and what this module previously lacked. + m.next = "DROP_FRAME" + with m.Elif(self.rx_valid & self.rx_sof & self.rx_enabled): frame_base = Signal(16) m.d.comb += frame_base.eq(Cat(Const(0, 8), rwp)) m.d.sync += desc_base.eq(frame_base) @@ -121,6 +170,16 @@ class RXFrameAssembler(Elaboratable): m.d.sync += desc_ctr.eq(0) m.next = "WRITE_PLACEHOLDER" + with m.State("DROP_FRAME"): + # Consume and discard to EOF. rx_ready MUST stay high here: + # stalling the ethernet master instead would back pressure into + # the W5100's own buffer and turn a dropped frame into a wedged + # receiver. + m.d.comb += self.rx_ready.eq(1) + with m.If(self.rx_valid & self.rx_eof): + m.d.sync += self.rx_overflow.eq(1) + m.next = "IDLE" + with m.State("WRITE_PLACEHOLDER"): # Write 4 bytes of 0x00 as placeholder descriptor. # @@ -275,6 +334,12 @@ if __name__ == "__main__": # changed nothing it looked at. mem = {} + # rx_irq and rx_overflow are 1-CYCLE pulses, and rx_overflow fires on + # the very tick that consumes the EOF byte -- i.e. inside send_frame. + # Polling for it afterwards misses it entirely, which first showed up + # as a dropped frame looking like a stall. Latch both in tick(). + pulse = {"irq": None, "ovf": False} + async def tick(n=1): for _ in range(n): # A write is issued on a cycle where valid AND ready are both @@ -282,8 +347,14 @@ if __name__ == "__main__": if ctx.get(dut.eth_wr_valid) and ctx.get(dut.eth_wr_ready): mem[ctx.get(dut.eth_wr_addr)] = ctx.get(dut.eth_wr_data) await ctx.tick("sync") + if ctx.get(dut.rx_overflow): + pulse["ovf"] = True + if ctx.get(dut.rx_irq): + pulse["irq"] = ctx.get(dut.rx_wptr_w_data) async def send_frame(frame): + pulse["irq"] = None + pulse["ovf"] = False """Push a frame RESPECTING BACK-PRESSURE, the way the real ethernet master does. rx_ready is LOW through IDLE and the whole placeholder-descriptor write, so a master that advances anyway @@ -309,11 +380,23 @@ if __name__ == "__main__": async def wait_irq(limit=200): for _ in range(limit): + if pulse["irq"] is not None: + return pulse["irq"] await tick() - if ctx.get(dut.rx_irq): - return ctx.get(dut.rx_wptr_w_data) return None + async def wait_done(limit=400): + """Wait for a frame to be either stored (rx_irq) or discarded + (rx_overflow), reading the latches so a pulse that fired during + send_frame is not lost.""" + for _ in range(limit): + if pulse["ovf"]: + return "dropped" + if pulse["irq"] is not None: + return "stored" + await tick() + return "timeout" + def descriptor(page): base = page << 8 return [mem.get(base + i) for i in range(4)] @@ -360,6 +443,43 @@ if __name__ == "__main__": await tick(4) + # ── T4: ring overflow must DROP, not overwrite ─────────────────── + # rrp is held still, as if the GC had stopped reading. The writer + # must refuse new frames once the ring is nearly full and leave the + # unread ones intact. Before this existed the writer simply lapped + # the reader, so a traffic burst corrupted whatever frame the driver + # was midway through -- with no indication it had happened. That is + # what produced a "descriptor" of 00 00 00 01 on hardware: frame data + # from the frame that had replaced the one being read. + ctx.set(dut.rrp, 4) # GC has consumed up to page 4 + await tick(2) + f3 = [(i * 3 + 1) & 0xFF for i in range(300)] # 2 pages each + await send_frame(f3) + check("T4 first frame stored", await wait_done(), "stored") + page4 = descriptor(4) + body(4, 32) + + outcomes = [] + for _ in range(12): + await send_frame(f3) + outcomes.append(await wait_done()) + if outcomes[-1] == "dropped": + break + check("T4 ring eventually refuses a frame", + outcomes[-1] if outcomes else "none", "dropped") + # THE property: the frame the GC has not read yet is untouched. + check("T4 unread page preserved", descriptor(4) + body(4, 32), page4) + + # Once the GC catches up, frames must be accepted again -- a full ring + # must not latch the receiver off. The writer is at page 14 here, so + # rrp must move well past it to free the 6 pages a worst-case frame + # needs: free = (rrp - rwp - 1) mod 15, so rrp = 8 gives 8 free. + ctx.set(dut.rrp, 8) + await tick(4) + await send_frame(f3) + check("T4 accepts again after RRP advances", await wait_done(), "stored") + + await tick(4) + # ── T3: RX disabled — SOF must be ignored ──────────────────────── before = len(mem) ctx.set(dut.rx_enabled, 0) diff --git a/gc_bba_test/source/netboot.c b/gc_bba_test/source/netboot.c index 287ffdc..f59c223 100644 --- a/gc_bba_test/source/netboot.c +++ b/gc_bba_test/source/netboot.c @@ -174,6 +174,10 @@ int netboot_receive(void *dst, u32 maxlen, void (*status)(const char *msg)) /* Start receive. Without the SR bit the assembler never writes the * ring, so frames arrive at the W5100 and go nowhere. */ if (bba_w8(REG_NCRA, NCRA_SR) < 0) return -1; + /* Clear any stale interrupt flags, RBFI (bit 7) included: the gateware + * now reports a ring-full drop there, and we would rather see a fresh + * one than inherit a set bit from an earlier run. */ + bba_w8(REG_IR, 0xFF); /* Establish a KNOWN ring state instead of trusting what we inherited. * Swiss initialises the BBA before launching us and writes RRP itself,