Roflin/rebbarb
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix two dropped-write bugs in RXFrameAssembler's SPRAM handshake

Browse Source 
Both from one mistake: eth_wr_valid is scheduled with m.d.sync and so is LOW
for the whole first cycle in a state, while eth_wr_ready was tested
immediately -- and `ready` is usually already high on entry.  Both
valid.eq(1) and valid.eq(0) then fired in the same cycle, the later won, the
write was never issued, and it was dropped SILENTLY.

1. WRITE_LEN_HI dropped the descriptor's HIGH length byte, leaving it at its
   placeholder 0x00.  Hardware read a descriptor of 00 00 00 01 -- a 1-byte
   frame length, impossible for ethernet -- and rejected the ring.

2. WRITE_PLACEHOLDER counted a phantom first write, advancing wr_addr without
   issuing one.  Only THREE placeholder bytes were written and wr_addr ended
   at desc_base+3, so the frame body started at descriptor byte 3 and EVERY
   RECEIVED FRAME WAS SHIFTED ONE BYTE EARLY.  The GC's ethertype test then
   read the wrong offset and silently dropped every frame: the host saw "no
   ACK" while the ring filled and RWP advanced normally.

Both now advance on `valid & ready`, and the body offset is set explicitly
(desc_base + 4) rather than relying on an incremented pointer, so it cannot
drift with the handshake again.

WHY NO TEST CAUGHT THIS, which is the more important half:

  - The testbench drove the RX stream OPEN-LOOP -- one byte per tick, never
    checking rx_ready, which is low through IDLE and the whole placeholder
    write.  It silently lost bytes, including the SOF byte.  A real master
    must hold each byte until accepted.
  - It only ever asserted on rwp and rx_irq.  It declared a spram_writes list
    and never filled it, so NOTHING had ever checked what was written.
  - Every frame it sent was short, and a short frame's high length byte is
    legitimately zero -- so a dropped high byte was indistinguishable from a
    correct one.

The testbench now respects back-pressure, records every write, and checks the
descriptor bytes and the body offset.  T2 sends a 300-byte frame specifically
so the high length byte is non-zero.  Mutation-tested: restoring either bug
fails T2 with got [0,0,0,48] want [0,0,1,48] -- the same shape as the
hardware's 00 00 00 01 -- while T1 (short frame) still passes, which is
exactly the blind spot that let this ship.

Bring-up's 342-byte DHCP frame read a correct 00 00 01 5A only because the
arbiter happened to hold `ready` low on that entry cycle; the domain merge
and the SPRAM response-FIFO removal changed that timing.

--seeds 8: clk 27.8-32.2 MHz, EXI 54.9-64.5 MHz, 8/8 PASS.  Best seed 8
(clk 32.22, EXI 62.25), flashed.  Gateware only -- the .dol is unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Dennis Brentjes
2026-10-09 12:04:00 +02:00
parent 77cd00a63c
commit 86cd5d1abd
1 changed files with 156 additions and 82 deletions
Download Patch File Download Diff File Expand all files Collapse all files
exi_bba/rx_frame_assembler.py
+155 -81
View File
@@ -122,11 +122,34 @@ class RXFrameAssembler(Elaboratable):
m.next = "WRITE_PLACEHOLDER"
with m.State("WRITE_PLACEHOLDER"):
# Write 4 bytes of 0x00 as placeholder descriptor
# Write 4 bytes of 0x00 as placeholder descriptor.
#
# ⚠️ Advance on `valid & ready`, NOT on `ready` alone.
# `eth_wr_valid` is scheduled with m.d.sync and so is LOW for
# the whole first cycle in this state, while `ready` is tested
# immediately -- and `ready` is usually already high here.
# Testing `ready` alone therefore counted a write that was
# never issued: only THREE placeholder bytes went out and
# wr_addr ended at desc_base+3, so the frame body started at
# descriptor byte 3 and EVERY RECEIVED FRAME WAS SHIFTED ONE
# BYTE EARLY.
#
# Hardware symptom (2026-10-09): the ring filled normally and
# RWP advanced, but the GC's ethertype test read the wrong
# offset and silently dropped every frame -- "no ACK" from the
# host with no other sign of trouble. The matching
# descriptor bug (same cause, in WRITE_LEN_HI) showed up as a
# descriptor of 00 00 00 01.
#
# The first byte is written twice as a consequence of
# eth_wr_addr trailing wr_addr by a cycle, and desc_base+3 is
# left unwritten -- both harmless, because every descriptor
# byte is either zero anyway or overwritten by WRITE_LEN_*.
# What matters is that wr_addr ends at desc_base+4.
m.d.sync += self.eth_wr_addr .eq(wr_addr)
m.d.sync += self.eth_wr_data .eq(0x00)
m.d.sync += self.eth_wr_valid.eq(1)
with m.If(self.eth_wr_ready):
with m.If(self.eth_wr_valid & self.eth_wr_ready):
m.d.sync += wr_addr.eq(wr_addr + 1)
with m.If(desc_ctr == 3):
m.d.sync += self.eth_wr_valid.eq(0)
@@ -157,12 +180,27 @@ class RXFrameAssembler(Elaboratable):
m.d.sync += pages_used.eq(total_len[8:12] + (total_len[:8] != 0))
m.next = "WRITE_LEN_HI"
# ⚠️ Both length writes must advance on `valid & ready`, NOT on
# `ready` alone. `valid` is scheduled with m.d.sync and so is low
# for the whole first cycle in the state, while `ready` is tested
# immediately -- and coming out of WAIT_LAST_WRITE `ready` is
# usually already high. Testing `ready` alone therefore fired both
# valid.eq(1) and valid.eq(0) in the same cycle, the later won, the
# write was never issued, and the length byte was SILENTLY DROPPED.
#
# Hardware symptom (2026-10-09): a descriptor of 00 00 00 01 --
# byte 2 still at its placeholder 0x00 because the HI write was
# dropped -- so the GC read a 1-byte frame length and rejected the
# ring. Intermittent by nature: it depends on whether the arbiter
# happens to hold `ready` high on the entry cycle, which is why
# bring-up's 342-byte DHCP frame read a correct 00 00 01 5A and
# this went unnoticed.
with m.State("WRITE_LEN_HI"):
# Overwrite descriptor byte 2 with total_len[15:8]
m.d.sync += self.eth_wr_addr.eq(desc_base + 2)
m.d.sync += self.eth_wr_data.eq(total_len[8:12])
m.d.sync += self.eth_wr_valid.eq(1)
with m.If(self.eth_wr_ready):
with m.If(self.eth_wr_valid & self.eth_wr_ready):
m.d.sync += self.eth_wr_valid.eq(0)
m.next = "WRITE_LEN_LO"
@@ -171,7 +209,7 @@ class RXFrameAssembler(Elaboratable):
m.d.sync += self.eth_wr_addr.eq(desc_base + 3)
m.d.sync += self.eth_wr_data.eq(total_len[:8])
m.d.sync += self.eth_wr_valid.eq(1)
with m.If(self.eth_wr_ready):
with m.If(self.eth_wr_valid & self.eth_wr_ready):
m.d.sync += self.eth_wr_valid.eq(0)
m.next = "ADVANCE_RWP"
@@ -204,97 +242,134 @@ if __name__ == "__main__":
dut = RXFrameAssembler()
errors = []
# Track all SPRAM writes issued by the DUT
spram_writes = []
def brief(v):
"""Long byte lists are unreadable in a pass/fail line; show the ends
and the length, and keep the full value only for a failure."""
if isinstance(v, list) and len(v) > 8:
head = ", ".join(str(x) for x in v[:4])
tail = ", ".join(str(x) for x in v[-2:])
return f"[{head}, ... {tail}] ({len(v)} bytes)"
return str(v)
def check(name, got, want):
ok = got == want
print(f" {'ok ' if ok else 'FAIL'} {name}: "
f"got {brief(got)}, want {brief(want)}")
if not ok:
if isinstance(got, list) and isinstance(want, list):
first = next((i for i, (g, w) in enumerate(zip(got, want))
if g != w), None)
if first is not None:
print(f" first difference at index {first}: "
f"got {got[first]}, want {want[first]}")
errors.append(name)
async def testbench(ctx):
# Setup: acknowledge all SPRAM writes immediately
# Final contents of every SPRAM address the DUT wrote.
#
# The old testbench declared a writes list and never filled it, and
# drove the RX stream OPEN-LOOP -- one byte per tick, never checking
# rx_ready. Both of the 2026-10-09 handshake bugs were therefore
# invisible to it: it only ever asserted on rwp and rx_irq, so a
# dropped descriptor byte or a frame body written one byte early
# changed nothing it looked at.
mem = {}
async def tick(n=1):
for _ in range(n):
# A write is issued on a cycle where valid AND ready are both
# high, so sample before the edge that completes it.
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")
async def send_frame(frame):
"""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
simply loses those bytes -- which is what the old testbench did,
and why it could not see the frame body land at the wrong
offset."""
for i, b in enumerate(frame):
ctx.set(dut.rx_data, b)
ctx.set(dut.rx_valid, 1)
ctx.set(dut.rx_sof, 1 if i == 0 else 0)
ctx.set(dut.rx_eof, 1 if i == len(frame) - 1 else 0)
guard = 0
while not ctx.get(dut.rx_ready):
await tick()
guard += 1
if guard > 400:
errors.append("send_frame: rx_ready never asserted")
return
await tick()
ctx.set(dut.rx_valid, 0)
ctx.set(dut.rx_sof, 0)
ctx.set(dut.rx_eof, 0)
async def wait_irq(limit=200):
for _ in range(limit):
await tick()
if ctx.get(dut.rx_irq):
return ctx.get(dut.rx_wptr_w_data)
return None
def descriptor(page):
base = page << 8
return [mem.get(base + i) for i in range(4)]
def body(page, n):
base = (page << 8) + 4
return [mem.get(base + i) for i in range(n)]
ctx.set(dut.eth_wr_ready, 1)
ctx.set(dut.rx_wptr_w_rdy, 1)
ctx.set(dut.rx_enabled, 1)
await ctx.tick("sync").repeat(2)
await tick(2)
# ── T1: 10-byte frame → pages_used=1, rwp advances 1→2 ──────────────
# Send SOF + first byte
frame = [0xAA, 0xBB, 0xCC, 0xDD, 0x08, 0x00, 0x45, 0x00, 0x00, 0x01]
# ── T1: short frame at page 1 ────────────────────────────────────
f1 = [0xAA, 0xBB, 0xCC, 0xDD, 0x08, 0x00, 0x45, 0x00, 0x00, 0x01]
await send_frame(f1)
check("T1 rwp advance", await wait_irq(), 2)
total1 = len(f1) + 4
check("T1 descriptor", descriptor(1),
[0x00, 0x00, (total1 >> 8) & 0xFF, total1 & 0xFF])
# The body MUST start at base+4. This is the check that catches a
# placeholder write being dropped: with only three placeholder bytes
# written the body landed at base+3 and every frame was shifted one
# byte early, which on hardware made the GC drop every frame.
check("T1 body at base+4", body(1, len(f1)), f1)
ctx.set(dut.rx_data, frame[0])
ctx.set(dut.rx_valid, 1)
ctx.set(dut.rx_sof, 1)
await ctx.tick("sync").repeat(1)
ctx.set(dut.rx_sof, 0)
await tick(4)
for i, b in enumerate(frame[1:], start=1):
ctx.set(dut.rx_data, b)
ctx.set(dut.rx_eof, 1 if i == len(frame) - 1 else 0)
await ctx.tick("sync").repeat(1)
# ── T2: frame LONGER THAN 255 BYTES, so the descriptor's HIGH length
# byte is non-zero. This is the case that catches a dropped
# WRITE_LEN_HI: with the high byte left at its placeholder 0x00 the GC
# read an impossible length (hardware saw 00 00 00 01) and rejected
# the ring. Every earlier test used a short frame, where a dropped
# high byte is indistinguishable from a correct zero. ───────────────
f2 = [(i * 7 + 3) & 0xFF for i in range(300)]
await send_frame(f2)
rwp2 = await wait_irq()
total2 = len(f2) + 4
check("T2 descriptor (len > 255)", descriptor(2),
[0x00, 0x00, (total2 >> 8) & 0xFF, total2 & 0xFF])
check("T2 body at base+4", body(2, len(f2)), f2)
# 304 bytes spans two pages, so page 2 -> page 4.
check("T2 rwp advance (2 pages)", rwp2, 4)
ctx.set(dut.rx_valid, 0)
ctx.set(dut.rx_eof, 0)
await tick(4)
# Poll for up to 30 ticks until rx_irq pulses (1-cycle pulse)
t1_irq_seen = False
t1_wptr_d = 0
for _ in range(30):
await ctx.tick("sync").repeat(1)
if ctx.get(dut.rx_irq):
t1_irq_seen = True
t1_wptr_d = ctx.get(dut.rx_wptr_w_data)
break
print(f"T1 rx_irq_seen={t1_irq_seen} wptr_data=0x{t1_wptr_d:02X}")
if not t1_irq_seen:
errors.append("T1: rx_irq never pulsed")
if t1_wptr_d != 2:
errors.append(f"T1: rwp should be 2 (page 1→2), got {t1_wptr_d}")
await ctx.tick("sync").repeat(4)
# ── T2: Send a second frame; verify rwp advances further ────────────
frame2 = [0x11, 0x22, 0x33, 0x44, 0x55, 0x66]
ctx.set(dut.rx_data, frame2[0])
ctx.set(dut.rx_valid, 1)
ctx.set(dut.rx_sof, 1)
await ctx.tick("sync").repeat(1)
ctx.set(dut.rx_sof, 0)
for i, b in enumerate(frame2[1:], start=1):
ctx.set(dut.rx_data, b)
ctx.set(dut.rx_eof, 1 if i == len(frame2) - 1 else 0)
await ctx.tick("sync").repeat(1)
ctx.set(dut.rx_valid, 0)
ctx.set(dut.rx_eof, 0)
t2_irq_seen = False
t2_wptr_d = 0
for _ in range(30):
await ctx.tick("sync").repeat(1)
if ctx.get(dut.rx_irq):
t2_irq_seen = True
t2_wptr_d = ctx.get(dut.rx_wptr_w_data)
break
print(f"T2 rx_irq_seen={t2_irq_seen} wptr_data=0x{t2_wptr_d:02X}")
if not t2_irq_seen:
errors.append("T2: rx_irq never pulsed after second frame")
if t2_wptr_d != 3:
errors.append(f"T2: rwp should be 3 (page 2→3), got {t2_wptr_d}")
# ── T3: RX disabled — SOF must be ignored ──────────────────────────
# ── T3: RX disabled — SOF must be ignored ────────────────────────
before = len(mem)
ctx.set(dut.rx_enabled, 0)
ctx.set(dut.rx_data, 0xDE)
ctx.set(dut.rx_valid, 1)
ctx.set(dut.rx_sof, 1)
await ctx.tick("sync").repeat(4)
await tick(8)
ctx.set(dut.rx_valid, 0)
ctx.set(dut.rx_sof, 0)
# No SPRAM write should have been issued
wr_valid = ctx.get(dut.eth_wr_valid)
if wr_valid:
errors.append("T3: SPRAM write issued while rx_enabled=0")
print(f"T3 rx disabled: eth_wr_valid={wr_valid} (expected 0)")
check("T3 no writes while rx disabled", len(mem), before)
sim = Simulator(dut)
sim.add_clock(Period(MHz=24), domain="sync")
@@ -306,7 +381,6 @@ if __name__ == "__main__":
if errors:
print("\nFAILURES:")
for e in errors:
print(" ", e)
print(" -", e)
sys.exit(1)
else:
print("\nAll tests passed.")