Honour the GC's ring geometry (BP / RHBP) instead of hardcoding it
Compat TODO item 2, the last silent-corruption risk. RXFrameAssembler took the RX ring as a compile-time constant (pages 1-15), so a driver configuring a SMALLER ring was ignored and we kept writing pages it would never read, with nothing reporting an error. The bounds now come from bp/rhbp, wired in BBATop from the GC's BP (0x0A/0B) and RHBP (0x1A/1B) registers, and both the page wrap and the free-space calculation use them. Garbage is rejected rather than obeyed: the driver's values are used only when bp >= 1 (page 0 is the TX buffer), rhbp <= 0x0F and rhbp > bp. Otherwise the hardcoded layout stands and geom_honoured goes low, so a mismatch is observable. Without that check BP=0 or an RHBP below BP gives a zero or negative ring size and the assembler writes outside it. ⚠️ Recorded as a known limitation: a ring must be LARGER than _MAX_FRAME_PAGES (6) or nothing is ever stored, because a frame's length is unknown until EOF so the worst case (1518+4 = 6 pages) is reserved up front. Found the honest way -- the test was written with RHBP=4 first and passed VACUOUSLY, storing nothing at all. Making that work needs a length-aware reservation, which is a bigger change than this one. T7 (8-page ring honoured, frames only in pages 1-8, and at least one actually stored) and T7b (all three garbage cases rejected, fallback 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. Item 1 (RX filtering) remains the only open gap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -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),
|
||||
|
||||
+106
-12
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user