diff --git a/CLAUDE.md b/CLAUDE.md index 654c794..e83301b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1340,14 +1340,19 @@ Known differences between this implementation and what real GC software expects, with the evidence for each, are tracked in **`docs/bba_compat_todo.md`**. Summary of what is still OPEN: -1. **No hardware RX filtering** — we take every frame on the segment; the real - BBA delivers own-MAC + broadcast. Causes the ring pressure already seen on - hardware. Needs a decision, not just code. -2. **Ring geometry hardcoded** — `RHBP`/`BP`/`TLBP` writes ignored; a game - using a smaller RX ring corrupts silently. -3. **`NCRB` (0x01) not decoded** — filtering and interrupt-coalescing bits. -4. **Registers reading `0x00` that have non-zero hardware defaults** — - `NWAYC` `0x84`, `MISC` `0x3C`, and the rest of the map. +1. **No hardware RX filtering** — ⚠️ STILL OPEN. We take every frame on the + segment; the real BBA delivers own-MAC + broadcast. Causes the ring + pressure already seen on hardware. Needs a decision, not just code: the + W5100's MF bit filters against `SHAR`, which we write only at init, so a + driver that writes PAR0–5 would leave it stale and MF would drop + everything. +2. ✅ **Ring geometry** — `BP`/`RHBP` are honoured, with garbage rejected and + `geom_honoured` exposed. A ring must still be larger than + `_MAX_FRAME_PAGES` (6) or the worst-case reservation drops every frame. +3. ✅ **`NCRB` (0x01)** — decoded, stored and exported as `reg.ncrb`. +4. ✅ **Hardware defaults** — every register libogc2 writes now stores and + reads back (`NWAYC` `0x84`, `MISC` `0x3C`, …). Status registers + (`LTPS`/`LRPS`/`TWP`/`TRP`/`TXFIFOCNT`) still read `0x00` deliberately. Keep that file updated in the same commit whenever one is closed. diff --git a/docs/bba_compat_todo.md b/docs/bba_compat_todo.md index 22a5542..0a4fe13 100644 --- a/docs/bba_compat_todo.md +++ b/docs/bba_compat_todo.md @@ -59,17 +59,37 @@ nothing is ever lost but the ring fills with other hosts' traffic. ## 2. Ring geometry is hardcoded — silent corruption risk -**Status: OPEN.** +**Status: ✅ DONE 2026-10-10.** `RXFrameAssembler` takes the ring bounds from +`bp`/`rhbp`, wired from the GC's `BP` (0x0A/0B) and `RHBP` (0x1A/1B) registers +in `BBATop`. The wrap and the free-space calculation both use them, so a +driver's smaller ring is respected instead of ignored. -We fix TX = page 0 and RX = pages 1–15 (`_RX_PAGE_FIRST`, `_RX_PAGE_LAST`, -`_PAGES_TOTAL`). libogc2 writes `TLBP = 0x0000`, `BP = 0x0001`, -`RHBP = 0x000F`, which matches us exactly — but **we ignore those writes.** +**Garbage is rejected, not obeyed.** The bounds are used only when +`bp >= 1` (page 0 is the TX buffer), `rhbp <= 0x0F` and `rhbp > bp`; +otherwise the hardcoded pages 1–15 stand and `geom_honoured` goes low, so a +mismatch is observable rather than silent. Without that check, `BP = 0` or an +`RHBP` below `BP` gives a zero or negative ring size and the assembler writes +outside it. -A game choosing a smaller RX ring (`RHBP < 0x0F`) would read pages the -assembler is still writing, with nothing reporting an error. The minimum -honest fix is to honour `RHBP`/`BP`, or failing that to detect a value that -disagrees with our layout and surface it (a status bit, or at least the UART -console) rather than corrupt silently. +⚠️ **A ring must be LARGER than `_MAX_FRAME_PAGES` (6) or nothing is ever +stored.** A frame's length is unknown until EOF, so the assembler reserves the +worst case (1518 + 4 bytes = 6 pages) before accepting one. With a 4-page ring +`free` can never reach 6, so RBFI drops every frame — correct, conservative, +and useless. This was found by writing the test with `RHBP = 4` first, where it +passed **vacuously** because nothing was stored at all. If a real driver ever +configures a ring that small, the reservation has to become length-aware +(accept a frame whose actual length fits), which is a bigger change than this +one. + +Tests: `rx_frame_assembler` **T7** (an 8-page ring is honoured, frames land +only in pages 1–8) and **T7b** (all three garbage cases rejected, and the +fallback layout still stores). Mutation-tested: wrapping at the hardcoded page +15 writes pages 9–11 and fails T7; removing the sanity check fails all four +T7b assertions. + +For the record, libogc2 writes `TLBP = 0x0000`, `BP = 0x0001`, +`RHBP = 0x000F` — exactly the old hardcoded layout, which is why this never +showed up in practice. ## 3. `NCRB` (0x01) is not decoded at all diff --git a/exi_bba/bba_top.py b/exi_bba/bba_top.py index bf98755..cb41d58 100644 --- a/exi_bba/bba_top.py +++ b/exi_bba/bba_top.py @@ -195,6 +195,13 @@ class BBATop(Elaboratable): # pointer to know what it must not overwrite, and reports a # dropped frame back as RBFI (IR[7]). asm.rrp.eq(reg.rrp), + # The GC's own ring geometry (BP / RHBP). Previously hardcoded in + # the assembler, so a driver configuring a smaller RX ring was + # ignored and we kept writing pages it would never read. The + # assembler sanity-checks these and falls back to pages 1-15 if + # they are unusable -- see docs/bba_compat_todo.md item 2. + asm.bp .eq(reg.bp), + asm.rhbp.eq(reg.rhbp), asm.ncra_rst.eq(reg.ncra_rst_o), 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 740afee..6a2e4c9 100644 --- a/exi_bba/rx_frame_assembler.py +++ b/exi_bba/rx_frame_assembler.py @@ -119,9 +119,21 @@ class RXFrameAssembler(Elaboratable): # 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) + # ── Ring geometry, from the GC's own BP / RHBP registers ────────── + # compat TODO item 2. These were hardcoded, so a driver configuring a + # SMALLER RX ring was ignored and we kept writing pages it would never + # read -- silent corruption with nothing reporting an error. libogc2 + # writes BP = 0x0001 and RHBP = 0x000F, which happens to match the old + # hardcoded layout exactly, which is why it never showed up. + self.bp = Signal(12, init=_RX_PAGE_FIRST) # first RX page + self.rhbp = Signal(12, init=_RX_PAGE_LAST) # last RX page # 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() + # High when the driver's BP/RHBP were usable and are being honoured; + # low means they were rejected and the hardcoded layout is in use, so + # a mismatch is observable rather than silent. + self.geom_honoured = Signal() # The GC's NCRA reset. MUST reset `rwp` here as well as the register # file's copy: they are two copies of the same pointer, and resetting @@ -164,21 +176,41 @@ class RXFrameAssembler(Elaboratable): m.d.sync += self.rx_wptr_w_en.eq(0) m.d.sync += self.rx_overflow.eq(0) + # ── Sanitised ring bounds ───────────────────────────────────────── + # A driver's BP/RHBP are used only when they describe a usable ring + # inside SPRAM; otherwise the hardcoded layout stands. Without this a + # garbage write (BP = 0, or RHBP below BP) would give a zero or + # negative ring size and the assembler would write outside it. Page 0 + # is reserved for the TX buffer, so BP must be at least 1. + geom_ok = Signal() + pg_first = Signal(8) + pg_last = Signal(8) + pg_total = Signal(5) + m.d.comb += [ + geom_ok.eq((self.bp >= _RX_PAGE_FIRST) + & (self.rhbp <= _RX_PAGE_LAST) + & (self.rhbp > self.bp)), + pg_first.eq(Mux(geom_ok, self.bp[0:8], _RX_PAGE_FIRST)), + pg_last .eq(Mux(geom_ok, self.rhbp[0:8], _RX_PAGE_LAST)), + pg_total.eq(pg_last - pg_first + 1), + ] + m.d.comb += self.geom_honoured.eq(geom_ok) + # ── 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)) + # rwp == rrp means EMPTY, so writing the whole ring would wrap back to + # "empty" and lose the lot. Usable = (rrp - rwp - 1) mod pg_total, + # which is pg_total-1 when the ring is empty. + rrp_s = Signal(8) + m.d.comb += rrp_s.eq(Mux((self.rrp >= pg_first) + & (self.rrp <= pg_last), + self.rrp, pg_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)) + m.d.comb += raw.eq(rrp_s + pg_total - rwp) + m.d.comb += free.eq(Mux(raw == 0, pg_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 @@ -308,8 +340,8 @@ class RXFrameAssembler(Elaboratable): # next_rwp = ((rwp - 1 + pages_used) % 15) + 1 next_rwp_raw = Signal(8) m.d.comb += next_rwp_raw.eq(rwp + pages_used) - with m.If(next_rwp_raw > _RX_PAGE_LAST): - m.d.sync += next_page.eq(next_rwp_raw - _PAGES_TOTAL) + with m.If(next_rwp_raw > pg_last): + m.d.sync += next_page.eq(next_rwp_raw - pg_total) with m.Else(): m.d.sync += next_page.eq(next_rwp_raw) m.next = "WRITE_D0" @@ -393,7 +425,7 @@ class RXFrameAssembler(Elaboratable): # this also clears the SR bit, so rx_enabled is low and the FSM is in # IDLE. with m.If(self.ncra_rst): - m.d.sync += rwp.eq(_RX_PAGE_FIRST) + m.d.sync += rwp.eq(pg_first) return m @@ -707,6 +739,68 @@ if __name__ == "__main__": ctx.set(dut.rx_sof, 0) check("T3 no writes while rx disabled", len(mem), before) + # ── T7: the GC's BP/RHBP are HONOURED (compat TODO item 2) ─────── + # These were hardcoded to pages 1-15, so a driver configuring a + # SMALLER ring was ignored and we kept writing pages it would never + # read -- silent corruption. libogc2 writes BP=1/RHBP=15, exactly the + # old hardcode, which is why it never showed up. + ctx.set(dut.rx_enabled, 1) + ctx.set(dut.rrp, 1) + ctx.set(dut.bp, 1) + ctx.set(dut.rhbp, 8) # an 8-page ring: pages 1..8 + await tick(4) + check("T7 small ring honoured", ctx.get(dut.geom_honoured), 1) + + # ⚠️ The ring must be LARGER than _MAX_FRAME_PAGES or nothing is ever + # stored: `free` can never reach the worst-case reservation, so RBFI + # drops every frame. A 4-page ring was tried here first and stored + # NOTHING -- the test passed vacuously. 8 pages is the smallest that + # both exercises the wrap and leaves room. + pages_seen = set() + for n in range(10): + mem.clear() + # Keep the reader moving so the ring never fills. + ctx.set(dut.rrp, (n % 8) + 1) + await tick(4) + await send_frame([0x90 + n] * 32) + if await wait_done() == "stored": + pages_seen |= {a >> 8 for a in mem} + await tick(4) + print(f" T7 pages written with RHBP=8: {sorted(pages_seen)}") + check("T7 stored at least one frame in the small ring", + len(pages_seen) > 0, True) + outside = {p for p in pages_seen if p < 1 or p > 8} + check("T7 never writes outside pages 1-8", sorted(outside), []) + + # ── T7b: garbage geometry is REJECTED, not obeyed ──────────────── + # Without the sanity check, BP=0 (page 0 is the TX buffer) or an RHBP + # below BP gives a zero or negative ring size and the assembler writes + # outside it. + for bp, rhbp, why in ((0, 15, "BP=0 is the TX buffer"), + (8, 4, "RHBP below BP"), + (1, 0x20, "RHBP beyond SPRAM ring")): + ctx.set(dut.bp, bp) + ctx.set(dut.rhbp, rhbp) + await tick(4) + if ctx.get(dut.geom_honoured): + check(f"T7b reject {why}", "honoured", "rejected") + check("T7b garbage geometry rejected", ctx.get(dut.geom_honoured), 0) + + # ... and with it rejected, the hardcoded layout still works. + ctx.set(dut.rrp, 1) + await tick(4) + mem.clear() + await send_frame([0x5A] * 32) + check("T7b still stores with fallback layout", await wait_done(), + "stored") + fb = {a >> 8 for a in mem} + print(f" T7b fallback pages: {sorted(fb)}") + check("T7b fallback wrote something", len(fb) > 0, True) + check("T7b fallback writes inside pages 1-15", + sorted(p for p in fb if p < 1 or p > 15), []) + ctx.set(dut.bp, 1) + ctx.set(dut.rhbp, 15) + sim = Simulator(dut) sim.add_clock(Period(MHz=24), domain="sync") sim.add_testbench(testbench)