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

[sw/silicon_creator] Implement ROM_EXT bootstrap library #18929

Merged
merged 2 commits into from Jul 31, 2023

Conversation

dmcardle
Copy link
Contributor

No description provided.

@dmcardle dmcardle force-pushed the dmcardle/rom-ext-recovery branch 3 times, most recently from 0efa623 to 9c9e807 Compare June 16, 2023 21:47
@dmcardle dmcardle force-pushed the dmcardle/rom-ext-recovery branch 3 times, most recently from 65971c4 to c62ba6a Compare July 6, 2023 17:53
@dmcardle dmcardle force-pushed the dmcardle/rom-ext-recovery branch 5 times, most recently from f198b7c to 6315069 Compare July 13, 2023 18:27
@dmcardle dmcardle changed the title WIP ROM_EXT Recovery [sw/silicon_creator] Implement ROM_EXT bootstrap library Jul 13, 2023
@dmcardle dmcardle requested a review from alphan July 13, 2023 18:28
@dmcardle dmcardle marked this pull request as ready for review July 13, 2023 18:29
@dmcardle dmcardle requested a review from a team as a code owner July 13, 2023 18:29
@dmcardle dmcardle force-pushed the dmcardle/rom-ext-recovery branch 3 times, most recently from 296ff41 to a276bc9 Compare July 17, 2023 17:25
@moidx moidx removed the request for review from a team July 18, 2023 14:04
Copy link
Contributor

@alphan alphan left a comment

Choose a reason for hiding this comment

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

Thanks a lot for putting this together @dmcardle ! I'm just not sure about the following points:

  • Can we try splitting this into two implementations like rom and rom_ext so that we don't have to cache and pass protect_rom_ext to the functions?
  • I think we should enhance the flash_ctrl.c driver and add a function for setting mp_region_cfg registers for enabling erase over an arbitrary region instead of enabling it for a bank -- this would handle protecting rom_ext as well.
  • We should double-check with rom_ext is at a page boundary. If not, we will need to read-modify-write since we can only erase at page boundaries.

I'll send an invite to chat about this tomorrow.

sw/device/silicon_creator/lib/error_test_util.h Outdated Show resolved Hide resolved
sw/device/silicon_creator/lib/error_test_util.h Outdated Show resolved Hide resolved
sw/device/silicon_creator/lib/BUILD Outdated Show resolved Hide resolved
sw/device/silicon_creator/lib/BUILD Outdated Show resolved Hide resolved
sw/device/silicon_creator/lib/BUILD Outdated Show resolved Hide resolved
// If the boot failed, check whether we should enter bootstrap mode.
if (error != kErrorOk && rom_ext_bootstrap_enabled() == kHardenedBoolTrue) {
HARDENED_CHECK_EQ(rom_ext_bootstrap_enabled(), kHardenedBoolTrue);
OT_DISCARD(rom_printf("Entering ROM_EXT bootstrap\r\n"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
OT_DISCARD(rom_printf("Entering ROM_EXT bootstrap\r\n"));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this printf is not strictly necessary, but I figured it simplifies testing rom_ext_bootstrap_enabled() via a functest, since we can set the exit_success to "Entering ROM_EXT bootstrap\r\n".

Deleting is fine as well — e2e tests can just test the observable behavior, such as whether the requested pages were written to flash.

Copy link
Contributor

Choose a reason for hiding this comment

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

So this printf is not strictly necessary, but I figured it simplifies testing rom_ext_bootstrap_enabled() via a functest, since we can set the exit_success to "Entering ROM_EXT bootstrap\r\n".

I see.

Deleting is fine as well — e2e tests can just test the observable behavior, such as whether the requested pages were written to flash.

I think we can also send jedec_id or read_sfdp commands and check the response.

If you prefer to include this please feel free to add a fpga-specific function implementation in device_fpga_cw310.c. You might also want to convert the existing function to a generic fpga_rom_printf(). But please note that if you use it in tests those tests would pass only on FPGA.

sw/device/silicon_creator/rom_ext/rom_ext.c Show resolved Hide resolved
sw/device/silicon_creator/rom_ext/bootstrap_unittest.cc Outdated Show resolved Hide resolved
sw/device/silicon_creator/lib/bootstrap_test_util.cc Outdated Show resolved Hide resolved
sw/device/silicon_creator/lib/bootstrap.c Outdated Show resolved Hide resolved
@dmcardle dmcardle force-pushed the dmcardle/rom-ext-recovery branch 2 times, most recently from 0eb14ee to 503a09a Compare July 25, 2023 20:42
@dmcardle
Copy link
Contributor Author

Hey @alphan, this is ready to review whenever you get a chance!

  • Can we try splitting this into two implementations like rom and rom_ext so that we don't have to cache and pass protect_rom_ext to the functions?

Done.

  • I think we should enhance the flash_ctrl.c driver and add a function for setting mp_region_cfg registers for enabling erase over an arbitrary region instead of enabling it for a bank -- this would handle protecting rom_ext as well.

Per our chat, I'll enhance the flash_ctrl driver in a followup PR. This PR has unit tests that verify/document the non-bounds-checking behavior. I've left TODO comments (linking to #19151) about making use of the flash_ctrl memory protection features. I'll sweep through and clean out those TODOs in the followup PR.

  • We should double-check with rom_ext is at a page boundary. If not, we will need to read-modify-write since we can only erase at page boundaries.

Good point. The ROM_EXT regions in slot A and slot B definitely lie on page boundaries. I'll plan on adding a static_assert in the followup PR.

$ python
>>> 524288 % 2048
0
>>> 0x10000 % 2048
0
>>> (524288 + 0x10000) % 2048
0

Copy link
Contributor

@alphan alphan left a comment

Choose a reason for hiding this comment

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

Thanks @dmcardle ! LGTM mod some minor nits.

sw/device/silicon_creator/rom_ext/rom_ext.c Outdated Show resolved Hide resolved
sw/device/silicon_creator/lib/bootstrap.h Outdated Show resolved Hide resolved
sw/device/silicon_creator/lib/bootstrap.h Outdated Show resolved Hide resolved
sw/device/silicon_creator/lib/bootstrap.h Outdated Show resolved Hide resolved
sw/device/silicon_creator/rom/bootstrap.c Outdated Show resolved Hide resolved
sw/device/silicon_creator/rom_ext/bootstrap.c Outdated Show resolved Hide resolved
sw/device/silicon_creator/rom_ext/bootstrap.c Outdated Show resolved Hide resolved
sw/device/silicon_creator/rom_ext/bootstrap.c Outdated Show resolved Hide resolved
sw/device/silicon_creator/rom_ext/bootstrap.c Outdated Show resolved Hide resolved
sw/device/silicon_creator/rom_ext/bootstrap.c Outdated Show resolved Hide resolved
Signed-off-by: Dan McArdle <dmcardle@opentitan.org>
@dmcardle dmcardle force-pushed the dmcardle/rom-ext-recovery branch 3 times, most recently from 0b864cd to 81e5a3c Compare July 31, 2023 20:31
Issue lowRISC#19151

Signed-off-by: Dan McArdle <dmcardle@opentitan.org>
@dmcardle dmcardle merged commit 934678a into lowRISC:master Jul 31, 2023
24 checks passed
@dmcardle dmcardle deleted the dmcardle/rom-ext-recovery branch August 2, 2023 14:32
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.

None yet

2 participants