Skip to content

Conversation

@rivos-eblot
Copy link

No description provided.

@rivos-eblot rivos-eblot force-pushed the dev/ebl/otp_secret_scramble branch from d7559b3 to b4888f0 Compare October 24, 2025 13:27
Copy link

@AlexJones0 AlexJones0 left a comment

Choose a reason for hiding this comment

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

Thanks for looking at this, it looks great to me! I'll base my other OTP PR (#258) on the changes in this PR.

You can ignore the CI failure in //sw/device/tests/crypto:aes_kwp_kat_functest_sim_qemu_sival_rom_ext, I need to find some time to look into why all the AES tests are recently flaky on Earlgrey regression CI - no QEMU change was made so some of the OpenTitanLib crypto refactoring must have started doing something differently. The errors are quite strange.

@AlexJones0 AlexJones0 self-requested a review October 24, 2025 14:48
Copy link

@AlexJones0 AlexJones0 left a comment

Choose a reason for hiding this comment

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

Actually I've just checked and this is causing the otp_ctrl_descrambling_test to fail again, let me see if I can figure out what's going wrong.

@rivos-eblot rivos-eblot force-pushed the dev/ebl/otp_secret_scramble branch from b4888f0 to de7b785 Compare October 24, 2025 15:13
@AlexJones0
Copy link

I have something resembling a fix in my last commit here. At least for Earlgrey it gets the descrambling test passing. The commit message goes into more detail but this ensures that DAI reads from buffered partitions read from the buffer and do not erroneously calculate the ECC on bufferized data (which, if descrambled, will not have the same ECC as the scrambled data).

This is probably not entirely correct, as I haven't cross-referenced the RTL nor properly considered the implications on digests at all, but it is sufficient to at least get the descrambling datapath exercised by otp_ctrl_descrambling_test passing again after switching to descrambling during bufferization. Hopefully it is at least a useful starting point.

@rivos-eblot rivos-eblot force-pushed the dev/ebl/otp_secret_scramble branch 3 times, most recently from 2e770f4 to a04e37b Compare October 27, 2025 09:39
Copy link

@AlexJones0 AlexJones0 left a comment

Choose a reason for hiding this comment

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

Thanks, this LGTM (apart from the CI lint failure).

Also on the commit for fixing buffered DAI reads (542d0c0), this should be renamed to ot_otp instead of ot_otp_eg?

The CI regression run reports that the otp_ctrl_descrambling_test is passing, so this should be correct for secret partition descrambling.

@rivos-eblot rivos-eblot force-pushed the dev/ebl/otp_secret_scramble branch from a04e37b to 94fd1b3 Compare October 27, 2025 13:01
@rivos-eblot
Copy link
Author

Should be ok, waiting for CI

Copy link

@AlexJones0 AlexJones0 left a comment

Choose a reason for hiding this comment

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

Thanks for all the work on this, it looks really nice to me.

If you use all the suggestions/fixes from the comments I've left and rebase my OTP PR (#258) on top, then I can see the the Earlgrey OTP tests that we have passing.

Function naming was a bit confusing, rename it to match similar feature.
Remove condition known to be always true.

Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>
Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>
…IZE} macros

with ot_otp_eg_part_data_byte_{offset,size} static inline function.

This aligns code with Darjeeling version.

Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>
@rivos-eblot rivos-eblot force-pushed the dev/ebl/otp_secret_scramble branch from 598ac19 to aafab66 Compare October 28, 2025 07:40
@rivos-eblot rivos-eblot force-pushed the dev/ebl/otp_secret_scramble branch from aafab66 to 174c3a5 Compare October 28, 2025 10:31
…ementation

- Partially generated with autoreg.py and otptool.py

Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>
Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>
…igest management.

- unscramble secret partitions at OTP load time
- content of buffered partitions is stored, unscrambled, in partition buffers
- all partition digest are loaded, ECC-corrected and stored in dedicated partition
  controller field, whether they are buffered or not
- DAI read always access the OTP back-end data, never the buffered copy

Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>
@rivos-eblot rivos-eblot force-pushed the dev/ebl/otp_secret_scramble branch from 174c3a5 to cb32e1c Compare October 28, 2025 11:23
Replace this legacy implementation based on partition "type",
with the `integrity` partition descriptor to perform ECC.

Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>
Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>
Signed-off-by: Emmanuel Blot <eblot@rivosinc.com>
@rivos-eblot rivos-eblot force-pushed the dev/ebl/otp_secret_scramble branch from cb32e1c to 473089b Compare October 28, 2025 11:32
Copy link

@AlexJones0 AlexJones0 left a comment

Choose a reason for hiding this comment

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

Thanks, this is much more closely aligned to the RTL.

With my PR rebased on top, all the OTP tests that I have locally are passing (when the QEMU support is merged I will try and enable them upstream so we get the checks in CI regressions).

@rivos-eblot rivos-eblot marked this pull request as ready for review October 28, 2025 12:57
@rivos-eblot rivos-eblot merged commit f35d78f into lowRISC:ot-9.2.0 Oct 28, 2025
10 checks passed
@rivos-eblot
Copy link
Author

Thanks, this is much more closely aligned to the RTL.

With my PR rebased on top, all the OTP tests that I have locally are passing (when the QEMU support is merged I will try and enable them upstream so we get the checks in CI regressions).

Thanks a lot for your help + review + diagnosis.

@rivos-eblot rivos-eblot deleted the dev/ebl/otp_secret_scramble branch October 28, 2025 13:01
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