[tlul] tlul_fifo_sync zeroes d_data of non-AccessAckData responses but forwards the original data_intg
- Dominant language
- SystemVerilog
- Stars
- 3.6k
- Forks
- 1.1k
- Avg merge
- 2d 22h
- Merged PRs (30d)
- 141
Description
## Description
`tlul_fifo_sync` rewrites the response payload — `d_data` is forced to zero for
every response whose opcode is not `AccessAckData` — while the parallel
integrity FIFO forwards `d_user` (including `data_intg`) unchanged. A response
that enters the FIFO as a well-formed `(d_data, data_intg)` pair therefore
leaves it as `(0, ECC(original_d_data))` whenever `d_opcode != AccessAckData`.
The pair is only still valid because of a coincidence in the code: for the
inverted 39/32 Hsiao code, `ECC(0) == ECC(~0) == 7'h2A`
(`prim_secded_pkg::SecdedInv3932ZeroEcc`), and every in-tree producer happens to
emit exactly `0` or `'1` on non-`AccessAckData` beats. Any producer that emits
any other value produces a response that fails the host-side data-integrity
check. Nothing in the tree documents or enforces the 0/~0 convention.
This appears to be the mechanism that #14611 assumed existed. In that
discussion the outcome was "fixed value with integrity good" for write
responses, and it was noted that it would be good to positively identify where
in the RTL the blanking happens. The zeroing mux below is the only such
mechanism, and it does not adjust `data_intg` — it is correct only by the
coincidence above, not by construction.
## Current behaviour
`hw/ip/tlul/rtl/tlul_fifo_sync.sv:127-135`, response FIFO write data:
```systemverilog
.wdata_i ({tl_d_i.d_opcode,
tl_d_i.d_param ,
tl_d_i.d_size ,
tl_d_i.d_source,
tl_d_i.d_sink ,
(tl_d_i.d_opcode == tlul_pkg::AccessAckData) ? tl_d_i.d_data :
{top_pkg::TL_DW{1'b0}} ,
tl_d_i.d_error ,
spare_rsp_i}),
```
`rspfifo_intg` (added in 67490a97, "[tlul,rtl] Separate integrity bits into an
additional FIFO", 2026-03) carries `tl_d_i.d_user` unchanged, so the
`data_intg` that was computed by the producer over the *original* `d_data` is
delivered alongside the zeroed data.
The mux is unconditional: it applies for every `RspPass`/`RspDepth`
configuration, including `RspDepth == 0`, where `prim_fifo_sync`'s
`gen_passthru_fifo` assigns `rdata_o = wdata_i`. So this is not limited to
buffered configurations.
## Why the pair contract covers every opcode
* `tlul_rsp_intg_gen` (`hw/ip/tlul/rtl/tlul_rsp_intg_gen.sv:53-58`) computes
`data_intg` from `d_data` with no opcode qualification.
* `tlul_adapter_sram` (`hw/ip/tlul/rtl/tlul_adapter_sram.sv:363-369`) explicitly
maintains the pair for write responses and replaces `data_intg` with
`SecdedInv3932ZeroEcc` when it blanks the data, with the comment: *"If this a
write response with data fields set to 0, we have to set all ECC bits
correctly since we are using an inverted Hsiao code."*
* `tlul_adapter_reg` (`:208` and `:221`) returns `'1` for error and write
responses, and `tlul_err_resp` returns `DataWhenInstrError = '0` /
`DataWhenError = '1` (`tlul_pkg.sv:41,44`) — again opcode-independent.
* `tlul_rsp_intg_chk` (`hw/ip/tlul/rtl/tlul_rsp_intg_chk.sv:33-40`) checks the
data pair on every `d_valid` beat when `EnableRspDataIntgCheck` is set, with
no opcode qualification.
* `tlul_fifo_async` (`hw/ip/tlul/rtl/tlul_fifo_async.sv:81,91`) forwards
`d_data` unmodified. The sync FIFO's mux is the odd one out in the transport
layer; it predates the end-to-end scheme and was carried through the
integrity-split refactor unadjusted.
## Where this matters
* `rv_core_ibex` places `tlul_fifo_sync` (`fifo_i`, `fifo_d`, `fifo_revbm`)
directly on the core's instruction and data response paths, and the core
checks response-data integrity (`ibus_intg_err`/`dbus_intg_err` feed
`fatal_intg_event`). Every non-`AccessAckData` response the CPU receives has
crossed this mux.
* `tlul_socket_1n` and `tlul_socket_m1` instantiate `tlul_fifo_sync` for their
host and device FIFOs, so the mux is in the response path of the generated
crossbars as well (in earlgrey these are configured with depth 0, which still
routes through the mux as noted above).
* Hosts with `EnableRspDataIntgCheck` set in-tree include the DMA and MBX host
adapters, the `rv_dm` SBA host adapter, and `tlul_adapter_vh` (default 1).
In all of these paths the check passes today only because in-tree producers
emit `0` or `'1` on non-`AccessAckData` beats.
## Counterexample
With `RspDepth = 1, RspPass = 0`: a device returns
`(AccessAck, d_data = 0x42032080, data_intg = 0x45)`. The host receives
`(AccessAck, d_data = 0x00000000, data_intg = 0x45)`.
`ECC_inv_39_32(0x42032080) == 0x45` and `ECC_inv_39_32(0) == 0x2A`, so the
delivered pair is not a valid codeword. I verified both ECC values against
`prim_secded_inv_39_32_enc.sv`, including that `ECC_inv(0) == ECC_inv(~0) ==
0x2A` (every parity mask in the encoder has even popcount over the data bits).
I also ran a bounded model check on the module (SymbiYosys, `smtbmc`/z3, free
host/device inputs, assuming pair validity on the device side and asserting it
on the host side). It fails at step 3 for both `RspDepth=1/RspPass=0` and
`RspDepth=2/RspPass=1`; with the fix below it passes — `RspDepth=1/RspPass=0`
to BMC depth 20, `RspDepth=2/RspPass=1` to BMC depth 8, and the prove-mode
basecase (reset-reachable states) is clean for both. Harness published here:
https://gist.github.com/john-kearney/51f7f3da8d6c14625bf00203b938a2d7
## Suggested fix
Forward `d_data` unmodified, matching `tlul_fifo_async`:
```diff
- (tl_d_i.d_opcode == tlul_pkg::AccessAckData) ? tl_d_i.d_data :
- {top_pkg::TL_DW{1'b0}} ,
+ tl_d_i.d_data ,
```
Rationale:
* Under the end-to-end scheme the transport layer should be transparent:
producers own pair formation, hosts own checking. Blanking write-response
data, where wanted, belongs at the producer with a matching ECC — which is
what `tlul_adapter_sram` and `tlul_adapter_reg` already do.
* Regenerating `data_intg` inside the FIFO instead would re-sign whatever the
data FIFO storage delivers and would hide storage faults from the host-side
check that the integrity-split refactor is there to catch.
* TileLink leaves `d_data` of `AccessAck` undefined, and `tlul_assert`'s
`dDataKnown` only constrains `AccessAckData`, so forwarding is spec-legal.
Hosts do not consume write-response data.
A matching DV update is needed in `xbar_scoreboard.sv` (`process_src_packet`
currently masks `AccessAck` `d_data` to 0, and models the unmapped-address
response as `0` for non-`Get` opcodes, whereas `tlul_err_resp` selects
`DataWhenInstrError`/`DataWhenError` on `instr_type` alone). I have that change
ready and will send it with the RTL fix.
If the intended behaviour is instead to blank write-response data in the
fabric, that needs an explicit decision and a matching `data_intg` — happy to
prepare that variant. Either way, the current state is correct only by
coincidence.
Contributor guide
Research direction
Start with hw/ip/tlul/rtl/tlul_fifo_sync.sv:127-135 and compare its response path with hw/ip/tlul/rtl/tlul_fifo_async.sv and the integrity handling in tlul_adapter_sram. Review xbar_scoreboard.sv, then run the relevant TLUL DV or bounded-model checks. Done means the chosen response-data behavior and its data_intg pairing are consistent through the FIFO and scoreboard.
Written by the indexing model from the issue text.
Assessment
- Domain
- embedded-iot, testing-qa
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 52/100