[sram_ctrl] Add readback feature #23212
Conversation
nasahlpa
requested review from
rswarbrick,
jon-flatley,
GregAC,
andreaskurth,
vogelpi,
cdgori,
moidx and
johannheyszl
and removed request for
a team,
mundaym,
rswarbrick and
jon-flatley
nasahlpa
force-pushed
the
sram_readback
branch
11 times, most recently
from
6270e58
to
72e0d3d
Compare
There was a problem hiding this comment.
Thanks @nasahlpa for porting this change over. I've done another pass and it looks mostly good as expected.
However, I spotted a few things that should be cleaned up before we merge this. It might be these things were introduced to pass lint checks or that I simply overlooked them during the previous review. Sorry for the inconvenience.
| name: sec_cm_mem_readback | ||
| desc: '''Verify the countermeasure(s) MEM.READBACK. | ||
|
|
||
| Test needs to be implemented, see lowRISC/opentitan-embargoed#97. |
There was a problem hiding this comment.
Please create an issue in the public repo for this and link it here.
There was a problem hiding this comment.
Thanks, I've now added the corresponding labels to take care of this for M5 hopefully.
| endtask | ||
|
|
||
| // Enable readback feature only for non-throughput and non-sec_cm tests. The |
There was a problem hiding this comment.
Enabling it for the throughput tests is probably also something that should be tracked in a public issue.
hw/ip/tlul/rtl/tlul_adapter_sram.sv
Outdated
| // permanently latch readback error until reset | ||
| logic readback_error_q; | ||
| always_ff @(posedge clk_i or negedge rst_ni) begin | ||
| if (!rst_ni) begin | ||
| readback_error_q <= '0; | ||
| end else if (readback_error) begin | ||
| readback_error_q <= 1'b1; | ||
| end | ||
| end |
There was a problem hiding this comment.
This should probably be moved up into the if (EnableReadback) begin statement. Because if EnableReadback is false, we have a flop that is never updated / holds a constant value. Some tools complain about this, some optimize the flop away but it's not a clean way of doing this. Sorry for not noting that in the previous review.
hw/ip/tlul/rtl/tlul_sram_byte.sv
Outdated
| module tlul_sram_byte import tlul_pkg::*; #( | ||
| parameter bit EnableIntg = 0, // Enable integrity handling at byte level | ||
| parameter int Outstanding = 1 | ||
| module tlul_sram_byte import tlul_pkg::*,prim_mubi_pkg::*; #( |
There was a problem hiding this comment.
Please don't do import prim_mubi_pkg::* as a whole. It's against our design style guide. Instead,
- Import only those constants you really need, and
- Import them inside the body of the module (not the parameter list).
I quickly checked the repo. Most cases where prim_mubi_pkg is imported as a whole using the wildcard import is in DV files (we do have different style guide rules for those). There are very few RTL files doing wildcard imports of packages from different IPs / prims but it's not good practice. We should clean this up after the tapeout.
There was a problem hiding this comment.
Yes, the majority of modules only import the needed constant but some modules (e.g., KMAC) do prim_mubi_pkg::*. Accidentally, I've used one of these modules as a template.
I've corrected the import based on your suggestion, thanks!
| logic [top_pkg::TL_DBW-1:0] a_mask; | ||
| logic [top_pkg::TL_DW-1:0] a_data; | ||
| tl_a_user_t a_user; | ||
| } tl_txn_data_t; |
There was a problem hiding this comment.
This is not your fault (you just moved this code) but according to line 618, some of these signals are flopped (in the FIFO) but not used:
logic unused_held_data;
assign unused_held_data = ^{held_data.a_address[AccessSize-1:0],
held_data.a_user.data_intg,
held_data.a_size};This PR would be a good opportunity to remove these unnecessary flops and save some area.
There was a problem hiding this comment.
Thanks for the comment. Actually, we need all the data stored inside held_data (a_address, a_user and a_size) in the readback mode. So unused_held_data is not needed, hence, I removed it.
There was a problem hiding this comment.
Thanks for taking a look at this. Removing the unused signals is then what should be done indeed :-)
hw/ip/tlul/rtl/tlul_sram_byte.sv
Outdated
| stall_host = 1'b1; | ||
|
|
||
| // Wait until there is a single ongoing transaction. | ||
| if (pending_txn_cnt == $bits(pending_txn_cnt)'(1)) begin |
There was a problem hiding this comment.
$bits(pending_txn_cnt)'(1)) may be problematic. We mostly use $bits in parameter definitions in our code base. Instead, I suggest to define a local parameter where you use this function and then you use that parameter in the code to perform the width extension, i.e.,
localparam int unsigned PendingTxnCntW = prim_util_pkg::vbits(Outstanding+1);
logic [PendingTxnCntW-1:0] pending_txn_cnt;
...
if (pending_txn_cnt == PendingTxnCntW'(1)) beginThere was a problem hiding this comment.
Thanks for the comment, I've changed the RTL based on your suggestion.
nasahlpa
force-pushed
the
sram_readback
branch
2 times, most recently
from
8c4c146
to
de3b69f
Compare
When enabled with the SRAM_CTRL.READBACK CSR, the SRAM readback mode checks each read and write to the SRAM. On a read, the readback mode issues a second read to the same address to check, whether the correct data was fetched from memory. On a write, the readback mode issues a read to check, whether the data was actually written into the memory. To avoid that the holding register is read, the readback is delayed by one cycle. On a mismatch, a fatal error is triggered and the STATUS.READBACK_ERROR register is set. To avoid RTL changes as much as possible, the readback mode is implemented in the tlul_sram_byte module. In this module, after the initial read or write was executed, the bus interface to the host is stalled and the readback is performed. Due to the bus stalling, a performance impact is expected. Signed-off-by: Pascal Nasahl <[email protected]> Co-authored-by: Greg Chadwick <[email protected]>
|
The one failing CI check (CW310 Test ROM) seems to be unrelated to this PR; all other checks passed --> merging |
This commit updates the RAM ports inside ibex_top to reflect recent changes introduced with lowRISC/opentitan#23212 (SRAM readback mode). Signed-off-by: Pascal Nasahl <[email protected]>
This commit adds the DIF functions for the readback mode (c.f. lowRISC#23212)and a basic test. Closes lowRISC#23365 Signed-off-by: Pascal Nasahl <[email protected]>
This commit adds the DIF functions for the readback mode (c.f. lowRISC#23212) and a basic test. Closes lowRISC#23365 Signed-off-by: Pascal Nasahl <[email protected]>
This commit adds the DIF functions for the readback mode (c.f. lowRISC#23212) and a basic test. Closes lowRISC#23365 Signed-off-by: Pascal Nasahl <[email protected]>
This commit updates the RAM ports inside ibex_top to reflect recent changes introduced with lowRISC/opentitan#23212 (SRAM readback mode). Signed-off-by: Pascal Nasahl <[email protected]>
This commit updates the RAM ports inside ibex_top to reflect recent changes introduced with lowRISC/opentitan#23212 (SRAM readback mode). Signed-off-by: Pascal Nasahl <[email protected]>
This commit adds the DIF functions for the readback mode (c.f. #23212) and a basic test. Closes #23365 Signed-off-by: Pascal Nasahl <[email protected]>
When enabled with the SRAM_CTRL.READBACK CSR, the SRAM readback mode checks each read and write to the SRAM.
On a read, the readback mode issues a second read to the same address to check, whether the correct data was fetched from memory. On a write, the readback mode issues a read to check, whether the data was actually written into the memory. To avoid that the holding register is read, the readback is delayed by one cycle.
On a mismatch, a fatal error is triggered and the STATUS.READBACK_ERROR register is set.
To avoid RTL changes as much as possible, the readback mode is implemented in the tlul_sram_byte module. In this module, after the initial read or write was executed, the bus interface to the host is stalled and the readback is performed. Due to the bus stalling, a performance impact is expected.