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

[prim, rom_ctrl] Remove S&P layer from data scrambling #20855

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

msfschaffner
Copy link
Contributor

@msfschaffner msfschaffner commented Jan 16, 2024

As elaborated on #20788, the S&P layer is removed from data scrambling in order to improve error detection guarantees, interactions with ECC and timing.

@msfschaffner msfschaffner self-assigned this Jan 16, 2024
@msfschaffner msfschaffner force-pushed the scrambling-sp-removal branch 2 times, most recently from 024a840 to 24da245 Compare January 17, 2024 00:26
@msfschaffner msfschaffner linked an issue Jan 17, 2024 that may be closed by this pull request
@msfschaffner msfschaffner force-pushed the scrambling-sp-removal branch 2 times, most recently from a491564 to fd24cda Compare January 17, 2024 00:59
@msfschaffner
Copy link
Contributor Author

For Ibex, this has to be disabled separately in a second PR, since we'll have to vendor the updated memory backdoor utils and the scrambling primitive into the ibex repository first.

@msfschaffner msfschaffner force-pushed the scrambling-sp-removal branch from fd24cda to 24d408d Compare January 17, 2024 01:06
@msfschaffner msfschaffner changed the title [prim, rom_ctrl] Remove S&P layer from data scrrambling [prim, rom_ctrl] Remove S&P layer from data scrambling Jan 17, 2024
@msfschaffner msfschaffner force-pushed the scrambling-sp-removal branch 5 times, most recently from 5c36f87 to db8b466 Compare January 17, 2024 01:58
Copy link
Contributor

@rswarbrick rswarbrick left a comment

Choose a reason for hiding this comment

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

Some nitty comments, but this looks sensible to me.

hw/ip/prim/dv/prim_ram_scr/cpp/scramble_model.h Outdated Show resolved Hide resolved
hw/ip/prim/dv/prim_ram_scr/cpp/scramble_model.h Outdated Show resolved Hide resolved
hw/ip/prim/rtl/prim_ram_1p_scr.sv Outdated Show resolved Hide resolved
@msfschaffner msfschaffner force-pushed the scrambling-sp-removal branch from db8b466 to 8e4bd19 Compare January 17, 2024 17:42
As elaborated on lowRISC#20788, the S&P layer is disabled in the SRAM scrambling
devices in order to improve error detection guarantees, interactions with
ECC and timing.

In order to minimize changes and keep the implementation around in case
it is needed for byte parity at some point, we just set the NumDiffRounds
parameter to zero for the modules that leverage prim_ram_1p_scr.

In case of rom_ctrl, the functionality is removed entirely.

Signed-off-by: Michael Schaffner <[email protected]>
@msfschaffner msfschaffner force-pushed the scrambling-sp-removal branch from 1f81bf2 to 3462e65 Compare January 17, 2024 19:21
@msfschaffner msfschaffner marked this pull request as ready for review January 17, 2024 20:08
@msfschaffner msfschaffner requested a review from a team as a code owner January 17, 2024 20:08
@msfschaffner
Copy link
Contributor Author

msfschaffner commented Jan 17, 2024

I am going ahead with this and merge it so that I can do the Ibex re-vendoring.
Please let me know if anything should be amended.

@msfschaffner msfschaffner merged commit 803d9ae into lowRISC:master Jan 17, 2024
32 checks passed
vogelpi added a commit to vogelpi/opentitan that referenced this pull request Apr 12, 2024
The S&P network on the data got removed in lowRISC#20855 as
it negatively impacts the end-to-end integrity scheme but the PR forgot
to update the documentation of the scrambling primitive (The SRAM_CTRL
documentation got updated though).

Since the additional diffusion might still be relevant for other
implementations, it's worth to keep the documentation but explain that
it is not enabled by default.

Signed-off-by: Pirmin Vogel <[email protected]>
vogelpi added a commit that referenced this pull request Apr 12, 2024
The S&P network on the data got removed in #20855 as
it negatively impacts the end-to-end integrity scheme but the PR forgot
to update the documentation of the scrambling primitive (The SRAM_CTRL
documentation got updated though).

Since the additional diffusion might still be relevant for other
implementations, it's worth to keep the documentation but explain that
it is not enabled by default.

Signed-off-by: Pirmin Vogel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants