From b528901510e007154fef2a2d1f618de88ef9d4d2 Mon Sep 17 00:00:00 2001 From: Michele Bigi Date: Fri, 4 Sep 2026 14:41:44 +0200 Subject: [PATCH] test: certify mem_arbiter priority order (C.4) Priority B>C>A>D confirmed correct with distinguishable per-port data (not just "someone got served" but "the right requester got its own data back") across 4 contention scenarios. D-alone case confirms low priority does not mean never granted. Found and fixed a real race in the test harness itself: blocking assignments withdrew loser requests in the same clock edge meant to grant the winner, racing the DUT's own synchronous block -- dut.owner never left SEL_NONE, every wait() blocked forever. Fixed by switching request-signal drives to non-blocking assignments throughout. Documented (not filed as a bug) that D can starve indefinitely under sustained continuous B contention -- standard behavior for a fixed-priority arbiter with no aging, and explicitly outside the header's own stated operating assumption (B/C temporally disjoint in normal operation). Flagged the header's "never starves or corrupts A/B/C" wording as ambiguous about whether it promises D's own progress. Full regression: 40/40 real tests pass. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_013xXuuRUWZScuo1DeYJxs3v --- WORKLOG.md | 19 ++++ docs/validation/04-arbiter.md | 75 +++++++++++++ docs/validation/bugs.md | 5 + sim/int8_memory_access.vcd | 2 +- sim/int8_psram_integration.vcd | 2 +- sim/layer.vcd | 2 +- sim/layer_sequencer.vcd | 2 +- sim/mem_arbiter_priority_tb.v | 169 ++++++++++++++++++++++++++++++ sim/memory_interface.vcd | 2 +- sim/neuron_memory.vcd | 2 +- sim/neuron_memory_multi.vcd | 2 +- sim/neuron_parallel.vcd | 2 +- sim/parameter_sweep.vcd | 2 +- sim/parametric.vcd | 2 +- sim/psram_controller.vcd | 2 +- sim/psram_page_mode.vcd | 2 +- sim/spi_engine.vcd | 2 +- sim/spi_neuron_top.vcd | 2 +- sim/spi_neuron_top_graph.vcd | 2 +- sim/spi_neuron_top_runnetwork.vcd | 2 +- sim/spi_slave.vcd | 2 +- 21 files changed, 285 insertions(+), 17 deletions(-) create mode 100644 docs/validation/04-arbiter.md create mode 100644 sim/mem_arbiter_priority_tb.v diff --git a/WORKLOG.md b/WORKLOG.md index a730d8b..7ae561d 100644 --- a/WORKLOG.md +++ b/WORKLOG.md @@ -1293,3 +1293,22 @@ integrazione nel top level (F5), verifica consolidata e misure reali (F6). - **Regressione completa**: 39/39 test reali PASS (38 precedenti + 1 nuovo), 0 regressioni. - **Deliverable**: `docs/validation/03-memoria.md`. - **Prossimo passo**: C.4 (`mem_arbiter` — priorità B>C>A, starvation, contesa). + +## Campagna di ri-certificazione — C.4: Arbitro (2026-09-04) + +- **Priorità B>C>A>D certificata**: nuovo test (`sim/mem_arbiter_priority_tb.v`), 4 scenari + di contesa decrescente, dati distinguibili per confermare l'instradamento al richiedente + corretto (non solo che qualcuno venga servito). Tutti PASS. +- **Race trovata e corretta nella mia stessa testbench**: assegnazioni bloccanti per + ritirare le richieste dei "perdenti" nello stesso fronte di clock che concede la + richiesta — race reale con il blocco sincrono del DUT, `dut.owner` mai usciva da + `SEL_NONE`. Non un difetto RTL. Corretto con assegnazioni non bloccanti. +- **Finding su starvation di D**: sotto contesa continua e sostenuta da B (500 cicli), D + non viene mai concesso — non classificato come bug (arbitro a priorità fissa senza aging + è design standard, e la contesa continua testata è esplicitamente fuori dallo scenario + operativo previsto dall'header), ma segnalata un'ambiguità nella frase dell'header + ("never starves or corrupts A/B/C" può essere letta come garanzia su D stesso o solo su + A/B/C — il comportamento osservato è coerente solo con la seconda lettura). +- **Regressione completa**: 40/40 test reali PASS (39 precedenti + 1 nuovo), 0 regressioni. +- **Deliverable**: `docs/validation/04-arbiter.md`. +- **Prossimo passo**: C.5 (`layer_sequencer` — ping-pong, tabella descrittori, catena layer). diff --git a/docs/validation/04-arbiter.md b/docs/validation/04-arbiter.md new file mode 100644 index 0000000..82a7fb6 --- /dev/null +++ b/docs/validation/04-arbiter.md @@ -0,0 +1,75 @@ +# C.4 — Arbitro (`mem_arbiter.v`) + +Data: 2026-09-04. + +--- + +## 4.1 Priorità B>C>A>D — CERTIFICATO + +**Metodo**: nuovo test dedicato (`sim/mem_arbiter_priority_tb.v`). Quattro scenari, +combinazioni decrescenti di richiedenti simultanei, ciascuno con dati distinguibili +(`m_rdata` eco dell'indirizzo) per confermare che la risposta torni al **richiedente +corretto**, non solo che "qualcuno" venga servito: +1. A+B+C+D simultanei → B vince. +2. A+C+D (B assente) → C vince. +3. A+D (B,C assenti) → A vince. +4. D da solo → viene comunque servito (bassa priorità ≠ mai servito). + +``` +$ iverilog -g2012 -o /tmp/arb6.out rtl/mem_arbiter.v sim/mem_arbiter_priority_tb.v && vvp /tmp/arb6.out +ALL TESTS PASSED (priority order B>C>A>D confirmed; ...) +``` + +**Nota di processo — race trovata nella mia stessa testbench**: la prima versione usava +assegnazioni bloccanti per ritirare le richieste dei "perdenti" nello stesso +`@(posedge clk)` che doveva concedere la richiesta — una race reale con il blocco +sincrono del DUT sullo stesso fronte (l'ordine di esecuzione tra processi diversi +sensibili allo stesso evento non è garantito da Verilog). Diagnosticato con un +riferimento gerarchico a `dut.owner`, mai uscito da `SEL_NONE` nonostante le richieste +fossero pilotate — non un difetto dell'RTL. Corretto passando ad assegnazioni non +bloccanti per i segnali di richiesta in tutta la testbench, come farebbe un master reale +sincrono allo stesso clock. + +**Verdetto: CERTIFICATO.** L'ordine di priorità dichiarato nell'header è implementato +esattamente come descritto, dati instradati al richiedente corretto in ogni caso. + +--- + +## 4.2 Starvation di D sotto contesa continua — comportamento reale, ambiguità nella documentazione + +**Test**: `b_req` e `d_req` mantenuti entrambi asserti continuamente per 500 cicli +(B "ha sempre altro lavoro" nell'istante in cui si libera). + +**Risultato**: **D non viene MAI concesso in 500 cicli** di contesa continua da B. + +**Perché non lo classifico come bug**: l'header del modulo dichiara "flash operations +are ms-scale and never meant to compete with inference for memory bandwidth" e "In +normal operation B and C are temporally disjoint anyway" — la contesa continua e +sostenuta testata qui è esplicitamente fuori dallo scenario operativo previsto (un +`layer_sequencer`/`neuron_memory` che non lascia MAI un buco libero per centinaia di +cicli di fila non corrisponde a un'inferenza reale). Un arbitro a priorità fissa senza +invecchiamento (aging) che fa morire di fame il richiedente più basso sotto carico +sostenuto è un design standard e spesso intenzionale, non un difetto di per sé. + +**Cosa segnalo**: la frase dell'header "gets stretched out, never starves or corrupts +A/B/C" è **ambigua** — può essere letta sia come "[D] non affama mai [se stesso]" sia +come "[la contesa] non fa mai affamare o corrompere A/B/C" (una garanzia solo su A/B/C, +non su D). Il comportamento osservato è coerente con la SECONDA lettura, non con la +prima. Non è un bug funzionale, ma la frase andrebbe disambiguata nel commento sorgente +per evitare che un futuro lettore assuma erroneamente che D abbia una garanzia di +progresso che il codice non implementa. + +**Verdetto: CERTIFICATO come comportamento** (nessuna sorpresa rispetto a un arbitro a +priorità fissa senza aging), **riserva documentale** sulla frase ambigua dell'header. + +--- + +## 4.3 Verdetto complessivo C.4 + +| Sotto-aspetto | Verdetto | +|---|---| +| Priorità B>C>A>D, instradamento dati corretto | **CERTIFICATO** | +| Starvation di D sotto contesa sostenuta | **CERTIFICATO come comportamento**, riserva sulla chiarezza della documentazione (non un bug) | + +Nessun bug RTL trovato in questo aspetto. Un difetto di race trovato e corretto nella +testbench di verifica stessa (stesso schema del resto della campagna). diff --git a/docs/validation/bugs.md b/docs/validation/bugs.md index baaa853..ceae65b 100644 --- a/docs/validation/bugs.md +++ b/docs/validation/bugs.md @@ -142,3 +142,8 @@ perché siano difetti. precedente), e uno stub di memoria comportamentale che ignorava le byte-lane `mem_lb_n`/`mem_ub_n` durante la scrittura. Entrambi difetti della testbench, non dell'RTL — vedi `docs/validation/03-memoria.md` §3.1. +- **C.4 (`mem_arbiter` mai concedeva nulla nella mia prima testbench)**: assegnazioni + bloccanti per ritirare le richieste dei "perdenti" nello stesso fronte di clock che + doveva concedere la richiesta — race reale con il blocco sincrono del DUT. Diagnosticato + con `dut.owner` mai uscito da `SEL_NONE`. Corretto passando ad assegnazioni non bloccanti. + Vedi `docs/validation/04-arbiter.md` §4.1. diff --git a/sim/int8_memory_access.vcd b/sim/int8_memory_access.vcd index 875fce6..78ff492 100644 --- a/sim/int8_memory_access.vcd +++ b/sim/int8_memory_access.vcd @@ -1,5 +1,5 @@ $date - Fri Sep 4 14:31:15 2026 + Fri Sep 4 14:40:06 2026 $end $version Icarus Verilog diff --git a/sim/int8_psram_integration.vcd b/sim/int8_psram_integration.vcd index cd2d257..d6fdfcd 100644 --- a/sim/int8_psram_integration.vcd +++ b/sim/int8_psram_integration.vcd @@ -1,5 +1,5 @@ $date - Fri Sep 4 14:31:19 2026 + Fri Sep 4 14:40:10 2026 $end $version Icarus Verilog diff --git a/sim/layer.vcd b/sim/layer.vcd index c67c329..1246ecb 100644 --- a/sim/layer.vcd +++ b/sim/layer.vcd @@ -1,5 +1,5 @@ $date - Fri Sep 4 14:31:19 2026 + Fri Sep 4 14:40:11 2026 $end $version Icarus Verilog diff --git a/sim/layer_sequencer.vcd b/sim/layer_sequencer.vcd index d8c78a6..e4b635d 100644 --- a/sim/layer_sequencer.vcd +++ b/sim/layer_sequencer.vcd @@ -1,5 +1,5 @@ $date - Fri Sep 4 14:31:19 2026 + Fri Sep 4 14:40:11 2026 $end $version Icarus Verilog diff --git a/sim/mem_arbiter_priority_tb.v b/sim/mem_arbiter_priority_tb.v new file mode 100644 index 0000000..d632895 --- /dev/null +++ b/sim/mem_arbiter_priority_tb.v @@ -0,0 +1,169 @@ +`timescale 1ns/1ps + +// ================================================================ +// C.4 certification: rtl/mem_arbiter.v priority (B>C>A>D), grant +// correctness, and starvation behavior. +// +// Oracle: the priority order is a design DECISION stated in the +// module's own header (not derived from behavior) -- these tests +// check the RTL actually implements that stated order, and probe the +// starvation claim's exact wording ("D simply gets stretched out, +// never starves OR CORRUPTS A/B/C" -- a claim about protecting A/B/C, +// NOT a claim that D itself can never starve under sustained +// contention. This test checks both readings explicitly instead of +// assuming one). +// +// Testbench note: request signals are driven with NON-BLOCKING +// assignments throughout. An earlier version used blocking +// assignments to clear a request in the same `@(posedge clk)` step +// that was meant to grant it -- a real race with the DUT's own +// synchronous always block sampling the same edge (whichever process +// happens to run first in that Active-region step wins; Icarus +// resolved it in the DUT's disfavor here, silently losing every +// grant, `owner` never leaving SEL_NONE, every `wait(x_ready)` +// blocking forever). Caught via a hierarchical trace of `dut.owner` +// showing it never changed from 0 despite requests being driven -- +// not an RTL defect, a testbench race, same class as the one found in +// C.1/C.2 (see docs/validation/04-arbiter.md). +// ================================================================ + +module tb; + + localparam ADDR_WIDTH = 23; + + reg clk, rst; + reg a_req, b_req, c_req, d_req; + reg a_wr, b_wr, c_wr, d_wr; + reg [ADDR_WIDTH-1:0] a_addr, b_addr, c_addr, d_addr; + reg signed [7:0] a_wdata, b_wdata, c_wdata, d_wdata; + wire signed [7:0] a_rdata, b_rdata, c_rdata, d_rdata; + wire a_ready, b_ready, c_ready, d_ready; + + wire m_req, m_wr; + wire [ADDR_WIDTH-1:0] m_addr; + wire signed [7:0] m_wdata; + reg signed [7:0] m_rdata; + reg m_ready; + + mem_arbiter #(.ADDR_WIDTH(ADDR_WIDTH)) dut ( + .clk(clk), .rst(rst), + .a_req(a_req), .a_wr(a_wr), .a_addr(a_addr), .a_wdata(a_wdata), .a_rdata(a_rdata), .a_ready(a_ready), + .b_req(b_req), .b_wr(b_wr), .b_addr(b_addr), .b_wdata(b_wdata), .b_rdata(b_rdata), .b_ready(b_ready), + .c_req(c_req), .c_wr(c_wr), .c_addr(c_addr), .c_wdata(c_wdata), .c_rdata(c_rdata), .c_ready(c_ready), + .d_req(d_req), .d_wr(d_wr), .d_addr(d_addr), .d_wdata(d_wdata), .d_rdata(d_rdata), .d_ready(d_ready), + .m_req(m_req), .m_wr(m_wr), .m_addr(m_addr), .m_wdata(m_wdata), .m_rdata(m_rdata), .m_ready(m_ready) + ); + + initial begin clk = 0; forever #5 clk = ~clk; end + + initial begin + #200000; + $display("WATCHDOG TIMEOUT -- simulation did not finish in time"); + $finish; + end + + // Single-cycle-latency downstream memory stub: grants m_ready one + // cycle after m_req, echoes m_addr as the "data" (distinguishable + // per-port response, used to confirm rdata routes to the RIGHT + // requester and not a sibling). + always @(posedge clk) begin + m_ready <= m_req; + m_rdata <= m_addr[7:0]; + end + + integer errors; + + initial begin + errors = 0; + rst <= 1; + a_req <= 0; b_req <= 0; c_req <= 0; d_req <= 0; + a_wr <= 0; b_wr <= 0; c_wr <= 0; d_wr <= 0; + a_wdata <= 0; b_wdata <= 0; c_wdata <= 0; d_wdata <= 0; + a_addr <= 23'h001; b_addr <= 23'h002; c_addr <= 23'h003; d_addr <= 23'h004; + repeat(3) @(posedge clk); + rst <= 0; + @(posedge clk); + + // ------------------------------------------------------------ + // TEST 1: all four request simultaneously -- B must win first. + // ------------------------------------------------------------ + $display("--- TEST 1: simultaneous A+B+C+D request -- B must win ---"); + a_req <= 1; b_req <= 1; c_req <= 1; d_req <= 1; + @(posedge clk); // this edge: DUT samples all 4 (still their pre-edge values), grants B + a_req <= 0; b_req <= 0; c_req <= 0; d_req <= 0; // withdraw next cycle (one-cycle-pulse contract) + wait(b_ready); + if (b_rdata !== b_addr[7:0]) begin errors=errors+1; $display("FAIL: b_rdata=%0d expected=%0d", b_rdata, b_addr[7:0]); end + if (a_ready || c_ready || d_ready) begin errors=errors+1; $display("FAIL: a/c/d_ready asserted when B should have been the sole grantee"); end + else $display("PASS: B granted alone, correct data routed back"); + @(posedge clk); + + // ------------------------------------------------------------ + // TEST 2: A+C+D request (no B) -- C must win. + // ------------------------------------------------------------ + $display("--- TEST 2: A+C+D request (no B) -- C must win ---"); + a_req <= 1; c_req <= 1; d_req <= 1; + @(posedge clk); + a_req <= 0; c_req <= 0; d_req <= 0; + wait(c_ready); + if (c_rdata !== c_addr[7:0]) begin errors=errors+1; $display("FAIL: c_rdata=%0d expected=%0d", c_rdata, c_addr[7:0]); end + else $display("PASS: C granted (B absent), correct data"); + @(posedge clk); + + // ------------------------------------------------------------ + // TEST 3: A+D request (no B, no C) -- A must win. + // ------------------------------------------------------------ + $display("--- TEST 3: A+D request (no B, no C) -- A must win ---"); + a_req <= 1; d_req <= 1; + @(posedge clk); + a_req <= 0; d_req <= 0; + wait(a_ready); + if (a_rdata !== a_addr[7:0]) begin errors=errors+1; $display("FAIL: a_rdata=%0d expected=%0d", a_rdata, a_addr[7:0]); end + else $display("PASS: A granted (B,C absent), correct data"); + @(posedge clk); + + // ------------------------------------------------------------ + // TEST 4: D alone -- must be granted (lowest priority does not + // mean "never granted"). + // ------------------------------------------------------------ + $display("--- TEST 4: D alone -- must still be granted ---"); + d_req <= 1; + @(posedge clk); + d_req <= 0; + wait(d_ready); + if (d_rdata !== d_addr[7:0]) begin errors=errors+1; $display("FAIL: d_rdata=%0d expected=%0d", d_rdata, d_addr[7:0]); end + else $display("PASS: D granted alone, correct data"); + @(posedge clk); + + // ------------------------------------------------------------ + // TEST 5: sustained back-to-back B requests (held continuously) + // vs. continuous D requests -- does D ever get a turn? This + // checks the exact wording of the header's starvation claim, + // not an assumption. + // ------------------------------------------------------------ + $display("--- TEST 5: continuous B contention vs continuous D -- does D starve? ---"); + begin : starvation_check + integer cyc; + reg d_ever_granted; + d_ever_granted = 0; + d_req <= 1; + b_req <= 1; + for (cyc = 0; cyc < 500; cyc = cyc + 1) begin + @(posedge clk); + if (d_ready) d_ever_granted = 1; + end + if (d_ever_granted) + $display("OBSERVED: D was eventually granted within %0d cycles despite continuous B contention -- D does not starve under this exact pattern", cyc); + else + $display("OBSERVED: D was NEVER granted in %0d cycles of continuous B contention -- D CAN starve indefinitely under sustained higher-priority load (a literal 'gets stretched out' reading; does not contradict the header if read as only promising A/B/C's protection, not D's own)", cyc); + end + d_req <= 0; b_req <= 0; + @(posedge clk); + + if (errors == 0) + $display("ALL TESTS PASSED (priority order B>C>A>D confirmed; TEST 5 is an observation, not a pass/fail claim about starvation, since the header's wording is ambiguous about D's own guarantee)"); + else + $display("FAILED: %0d error(s)", errors); + $finish; + end + +endmodule diff --git a/sim/memory_interface.vcd b/sim/memory_interface.vcd index 85fce85..39bd9ed 100644 --- a/sim/memory_interface.vcd +++ b/sim/memory_interface.vcd @@ -1,5 +1,5 @@ $date - Fri Sep 4 14:31:19 2026 + Fri Sep 4 14:40:11 2026 $end $version Icarus Verilog diff --git a/sim/neuron_memory.vcd b/sim/neuron_memory.vcd index 9fbe32d..a97567d 100644 --- a/sim/neuron_memory.vcd +++ b/sim/neuron_memory.vcd @@ -1,5 +1,5 @@ $date - Fri Sep 4 14:31:28 2026 + Fri Sep 4 14:40:19 2026 $end $version Icarus Verilog diff --git a/sim/neuron_memory_multi.vcd b/sim/neuron_memory_multi.vcd index 2fa2279..c0c6eea 100644 --- a/sim/neuron_memory_multi.vcd +++ b/sim/neuron_memory_multi.vcd @@ -1,5 +1,5 @@ $date - Fri Sep 4 14:31:24 2026 + Fri Sep 4 14:40:15 2026 $end $version Icarus Verilog diff --git a/sim/neuron_parallel.vcd b/sim/neuron_parallel.vcd index 23dfad6..02c58f0 100644 --- a/sim/neuron_parallel.vcd +++ b/sim/neuron_parallel.vcd @@ -1,5 +1,5 @@ $date - Fri Sep 4 14:31:28 2026 + Fri Sep 4 14:40:20 2026 $end $version Icarus Verilog diff --git a/sim/parameter_sweep.vcd b/sim/parameter_sweep.vcd index 4c96f58..3b79715 100644 --- a/sim/parameter_sweep.vcd +++ b/sim/parameter_sweep.vcd @@ -1,5 +1,5 @@ $date - Fri Sep 4 14:31:28 2026 + Fri Sep 4 14:40:20 2026 $end $version Icarus Verilog diff --git a/sim/parametric.vcd b/sim/parametric.vcd index 4ca3d20..c298a47 100644 --- a/sim/parametric.vcd +++ b/sim/parametric.vcd @@ -1,5 +1,5 @@ $date - Fri Sep 4 14:31:28 2026 + Fri Sep 4 14:40:20 2026 $end $version Icarus Verilog diff --git a/sim/psram_controller.vcd b/sim/psram_controller.vcd index c903e92..99f3509 100644 --- a/sim/psram_controller.vcd +++ b/sim/psram_controller.vcd @@ -1,5 +1,5 @@ $date - Fri Sep 4 14:31:33 2026 + Fri Sep 4 14:40:24 2026 $end $version Icarus Verilog diff --git a/sim/psram_page_mode.vcd b/sim/psram_page_mode.vcd index 2371e15..83d521c 100644 --- a/sim/psram_page_mode.vcd +++ b/sim/psram_page_mode.vcd @@ -1,5 +1,5 @@ $date - Fri Sep 4 14:31:33 2026 + Fri Sep 4 14:40:24 2026 $end $version Icarus Verilog diff --git a/sim/spi_engine.vcd b/sim/spi_engine.vcd index 01931f1..891aaf5 100644 --- a/sim/spi_engine.vcd +++ b/sim/spi_engine.vcd @@ -1,5 +1,5 @@ $date - Fri Sep 4 14:31:33 2026 + Fri Sep 4 14:40:24 2026 $end $version Icarus Verilog diff --git a/sim/spi_neuron_top.vcd b/sim/spi_neuron_top.vcd index 271896b..77047b6 100644 --- a/sim/spi_neuron_top.vcd +++ b/sim/spi_neuron_top.vcd @@ -1,5 +1,5 @@ $date - Fri Sep 4 14:32:00 2026 + Fri Sep 4 14:40:50 2026 $end $version Icarus Verilog diff --git a/sim/spi_neuron_top_graph.vcd b/sim/spi_neuron_top_graph.vcd index 33c3238..9330efb 100644 --- a/sim/spi_neuron_top_graph.vcd +++ b/sim/spi_neuron_top_graph.vcd @@ -1,5 +1,5 @@ $date - Fri Sep 4 14:31:44 2026 + Fri Sep 4 14:40:35 2026 $end $version Icarus Verilog diff --git a/sim/spi_neuron_top_runnetwork.vcd b/sim/spi_neuron_top_runnetwork.vcd index b3868de..74f77ed 100644 --- a/sim/spi_neuron_top_runnetwork.vcd +++ b/sim/spi_neuron_top_runnetwork.vcd @@ -1,5 +1,5 @@ $date - Fri Sep 4 14:31:54 2026 + Fri Sep 4 14:40:45 2026 $end $version Icarus Verilog diff --git a/sim/spi_slave.vcd b/sim/spi_slave.vcd index 0ef9e45..bdc8009 100644 --- a/sim/spi_slave.vcd +++ b/sim/spi_slave.vcd @@ -1,5 +1,5 @@ $date - Fri Sep 4 14:32:01 2026 + Fri Sep 4 14:40:51 2026 $end $version Icarus Verilog