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 <noreply@anthropic.com>

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
 

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),
         ]
 

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)

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,