Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[top_earlgrey] Reduce number of PRINCE half rounds for main SRAM scrambling #24447

Merged
merged 3 commits into from
Aug 30, 2024

Conversation

vogelpi
Copy link
Contributor

@vogelpi vogelpi commented Aug 30, 2024

This PR contains multiple commits to reduce the number of PRINCE half rounds for the main SRAM scrambling from 3 back to 2 as used for Earlgrey ES. This is done to reduce timing pressure.

The scrambling parameters of the other scrambled memory primitives (retention SRAM, ROM, OTBN IMEM & DMEM) are not touched by this PR.

This PR supersedes #24376 as it also includes the required tooling changes. #24376 is intentionally left untouched to allow comparing the RTL changes as #24376 has already been absorbed by the PD team last week, i.e., we must not change the RTL beyond #24376.

For reference, the PRs where we increased the number of scrambling rounds in the past are:

Previously, all scrambled memory primitives had to use the same number
of PRINCE half rounds which was set to 3. This commit adds a new and
optional argument to the mem_bkdr_util constructor, to allow configuring
different values. If the argument is not provided, the default of 3
PRINCE half rounds is used.

Signed-off-by: Pirmin Vogel <[email protected]>
This commit reduces the number of PRINCE half rounds for the scrambling
of the main SRAM to 2 half rounds as used in the past. This is done
to reduce timing pressure.

Signed-off-by: Pirmin Vogel <[email protected]>
@vogelpi vogelpi added the ECO Accepted as an ECO label Aug 30, 2024
@vogelpi vogelpi requested a review from a team as a code owner August 30, 2024 10:07
@vogelpi vogelpi requested review from eshapira and removed request for a team August 30, 2024 10:07
@vogelpi
Copy link
Contributor Author

vogelpi commented Aug 30, 2024

CHANGE AUTHORIZED: hw/ip/sram_ctrl/data/sram_ctrl.hjson
CHANGE AUTHORIZED: hw/ip/sram_ctrl/rtl/sram_ctrl.sv
CHANGE AUTHORIZED: hw/top_earlgrey/data/top_earlgrey.hjson
CHANGE AUTHORIZED: hw/top_earlgrey/rtl/autogen/top_earlgrey.sv

This is an ECO PR confirmed by the PD team.

Copy link
Contributor

@andreaskurth andreaskurth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for this well-designed and -structured PR, @vogelpi! The changes LGTM and are aligned with discussions/decisions.

Just one question: What evidence on the DV side (top- and block-level tests) do we have that this works as intended?

@andreaskurth
Copy link
Contributor

andreaskurth commented Aug 30, 2024

To ensure there are no inadvertent changes compared to what the PD team is working with, I git diff'ed hw/ip/sram_ctrl/rtl/sram_ctrl.sv and hw/top_earlgrey/rtl/autogen/top_earlgrey.sv (which are the only RTL files changed by this PR) w.r.t. PR #24376: that diff is empty. ✔️

@andreaskurth
Copy link
Contributor

CHANGE AUTHORIZED: hw/ip/sram_ctrl/data/sram_ctrl.hjson
CHANGE AUTHORIZED: hw/ip/sram_ctrl/rtl/sram_ctrl.sv
CHANGE AUTHORIZED: hw/top_earlgrey/data/top_earlgrey.hjson
CHANGE AUTHORIZED: hw/top_earlgrey/rtl/autogen/top_earlgrey.sv

This is an ECO PR confirmed by the PD team.

@vogelpi
Copy link
Contributor Author

vogelpi commented Aug 30, 2024

Many thanks for this well-designed and -structured PR, @vogelpi! The changes LGTM and are aligned with discussions/decisions.

Just one question: What evidence on the DV side (top- and block-level tests) do we have that this works as intended?

Thanks for your review @andreaskurth . We have the following tests running in CI:

  • top-level:
    • chip_sw_power_virus: does some back door accesses into the main SRAM to communicate with the C side
    • chip_sw_sram_scrambled_access: does back door accesses into all scrambled SRAMs to test the scrambling, including error generation
  • block-level:
    • SRAM_CTRL_MAIN smoke test: tests the SRAM_CTRL configuration (2 half rounds) used for the main SRAM, including backdoor accesses
    • SRAM_CTRL_RET smoke test: tests the SRAM_CTRL congfiguration (3 half rounds) used for the retention SRAM, including backdoor accesses

There is also a C++ model of the scrambler hw/ip/prim/dv/prim_ram_scr/cpp/scramble_model.cc that is not touched by this PR. My understanding is that we only use this C++ model for OTBN block-level Verilator simulation. OTBN uses the default setting of 3 half rounds. This model is also run in CI.

@vogelpi vogelpi requested a review from johannheyszl August 30, 2024 12:39
@vogelpi
Copy link
Contributor Author

vogelpi commented Aug 30, 2024

From a security side, this has been approved already by @johannheyszl .

@vogelpi vogelpi merged commit 0ff694a into lowRISC:master Aug 30, 2024
38 checks passed
@vogelpi vogelpi deleted the sram-scrambling-incl-tooling branch September 17, 2024 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ECO Accepted as an ECO
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants