[icache] Disable S&P diffusion layer in memory scrambling #2130
Conversation
msfschaffner
requested review from
rswarbrick,
GregAC,
vogelpi,
a-will and
johannheyszl
There was a problem hiding this comment.
Thanks for taking care of this, @msfschaffner. LGTM, just added two comments.
| @@ -40,13 +40,16 @@ std::vector<uint8_t> scramble_addr(const std::vector<uint8_t> &addr_in, | |||
| * @param repeat_keystream Repeat the keystream of one single PRINCE instance if | |||
| * set to true. Otherwise multiple PRINCE instances are | |||
| * used. | |||
| * @param use_sp_layer Use the S&P layer for data diffusion. In HW this is | |||
| * disabled by default since it interacts adversely with | |||
There was a problem hiding this comment.
Maybe we can link issue #20788 or the corresponding RFC to highlight why this is the case.
There was a problem hiding this comment.
The issue is actually mentioned already in the description of the parameter in the scrambling primitive (prim_ram_1p_scr). I can add it here, but it'll require another round of vendoring.
There was a problem hiding this comment.
If we're waiting for another change to vendor this in, we can add a link to scramble_model.h as well.
There was a problem hiding this comment.
Coming in lowRISC/opentitan#20882. I can repeat the vendoring step once @vogelpi has merged his Trivium PR.
There was a problem hiding this comment.
I've now factored out the prim change into a separate PR here lowRISC/opentitan#20885. The AES change itself might take more time until it's fully reviewed and approved.
There was a problem hiding this comment.
This looks sensible to me, but don't we need to pass 2 for NumDiffRounds as part of the first commit?
If it's possible, it would be really nice if the first commit was "use newer vendored code, but no change in behaviour" and the second commit was "now switch to the new behaviour".
(It's possible I've missed something! Say if so!)
We actually do that already ;). |
|
Unrelated question, is the readthedocs CI error an intermittent one? |
There was a problem hiding this comment.
LGTM, thanks for the PR @msfschaffner !
The CI error on the documentation step is unrelated to this PR. I'll file an issue.
Update code from upstream repository https://github.com/lowRISC/opentitan to revision 4cf2479b8e6c9b68b9fe1adba202443d3dbe3ff3 * [prim_trivium] Allow dynamically disabling the lockup protection (Pirmin Vogel) * [scrambling] Add reference to RFC issue (Michael Schaffner) * [edn] Move prim_edn_req out of prim (Rupert Swarbrick) * [reggen] Remove the devmode input (Michael Schaffner) * [prim, rom_ctrl] Remove S&P layer from data scrambling (Michael Schaffner) * [prim] Fix typo in Trivium/Bivium stream cipher primitives (Pirmin Vogel) * [prim] Add scratch Verilator testbench for Trivium/Bivium primitives (Pirmin Vogel) * [prim] Add Trivium/Bivium stream cipher primitives (Pirmin Vogel) * [chip,dv] update makefile for real_key rom test (Jaedon Kim) * [dvsim] cast self.seed to 'int' (Jaedon Kim) * [dvsim] Change systemverilog seed to 32 bits (Hakim Filali) * [dv] Specialize dv_spinwait_* documentation comments (Rupert Swarbrick) Signed-off-by: Michael Schaffner <msf@opentitan.org>
Signed-off-by: Michael Schaffner <msf@opentitan.org>
msfschaffner
force-pushed
the
remove-sp-layer
branch
from
8a8f05f
to
3247307
Compare
This re-vendors updated primitives into the Ibex repository and disables the S&P diffusion layer in data scrambling as per lowRISC/opentitan#20788