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

Add hello world imm rom ext #25274

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

lchiawei
Copy link
Contributor

  • Add minimized C code, startup asm and linker script for a standalone IM_EXT build target.
  • Add a bazel rule imm_rom_ext_section, which will transform binaries into object files and turn it to a target that can be directly used as deps of ROM_EXT's bazel targets.
  • Reuse the rule opentitan_binary to build IM_EXT and add a attribute to skip the signing stage to the rule. The manifest given as None will turn into the default manifest and keep the signing operation so we need to skip it explicitly.
  • Add a map for all IM_EXT target that can be used and make the ROM_EXT targets iterate the map to generate multiple targets.
  • Update the ROM_EXT to use in e2e verified boot test.

This PR partially addresses #24368.

The first commit is included in #25043. I will remove it after it is reviewed and merged.

@lchiawei lchiawei requested review from rswarbrick, cfrantz and a team as code owners November 20, 2024 17:58
@lchiawei lchiawei requested review from jadephilipoom and removed request for a team November 20, 2024 17:58
@pamaury
Copy link
Contributor

pamaury commented Nov 20, 2024

My memory might be fuzzy but I think that opentitan_binary exports not only the signed binary but also the unsigned one, just a different output group. Therefore, I think you might not need a new attribute to skip signing, you just need to teach imm_rom_ext_section to look for the unsigned binary (probably use use ctx.attr.src and look at src.output_group["<exec_env>_binary"].

Alternatively, you might want to use the kind attribute (in common_binary_attrs) to introduce a new type of binary? And/or: if you introduce a new exec_env (which might make sense here), you can probably create one with no default manifest so it doesn't sign by default?

@sasdf sasdf requested a review from timothytrippel November 21, 2024 03:25
@lchiawei lchiawei force-pushed the add-hello-world-imm-rom-ext branch from bfd47a2 to 1fc69c6 Compare November 25, 2024 06:49
@lchiawei
Copy link
Contributor Author

lchiawei commented Nov 25, 2024

@pamaury

My memory might be fuzzy but I think that opentitan_binary exports not only the signed binary but also the unsigned one, just a different output group. Therefore, I think you might not need a new attribute to skip signing, you just need to teach imm_rom_ext_section to look for the unsigned binary (probably use use ctx.attr.src and look at src.output_group["<exec_env>_binary"].

I did some experiments, if I reuse the exec env //hw/top_earlgrey:silicon_creator, then the signing will cause the following error:

ERROR: /usr/local/google/home/lchiawei/opentitan/sw/device/silicon_creator/imm_rom_ext/BUILD:40:17: PreSigningArtifacts sw/device/silicon_creator/imm_rom_ext/hello_world_binaries_silicon_creator.test_key_0.pre-signing failed: (Exit 1): opentitantool failed: error executing PreSigningArtifacts command (from target //sw/device/silicon_creator/imm_rom_ext:hello_world_binaries) bazel-out/k8-fastbuild-ST-13d3ddad9198/bin/sw/host/opentitantool/opentitantool '--rcfile=' --quiet image manifest update ... (remaining 4 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
Error: Image doesn't appear to contain a manifest, or the manifest is corrupted

Caused by:
    Condition failed: `manifest.signed_region_end <= len` (4294967295 vs 1048576)

Alternatively, you might want to use the kind attribute (in common_binary_attrs) to introduce a new type of binary? And/or: if you introduce a new exec_env (which might make sense here), you can probably create one with no default manifest so it doesn't sign by default?

Actually I did found that the issues only happen when we use "//hw/top_earlgrey:silicon_creator" as the exec env, and using an empty manifest in that exec env can solve the issue. As currently all exec envs will derive same binaries and I plan to match the exec env between IM_EXT and ROM_EXT in the future, I will simply use fpga_cw340 as the only exec env. The exec env for silicon creator with empty manifest will be add in the future. And I also remove the attribute to skip the signing process.

@pamaury
Copy link
Contributor

pamaury commented Nov 25, 2024

@pamaury

My memory might be fuzzy but I think that opentitan_binary exports not only the signed binary but also the unsigned one, just a different output group. Therefore, I think you might not need a new attribute to skip signing, you just need to teach imm_rom_ext_section to look for the unsigned binary (probably use use ctx.attr.src and look at src.output_group["<exec_env>_binary"].

I did some experiments, if I reuse the exec env //hw/top_earlgrey:silicon_creator, then the signing will cause the following error:

ERROR: /usr/local/google/home/lchiawei/opentitan/sw/device/silicon_creator/imm_rom_ext/BUILD:40:17: PreSigningArtifacts sw/device/silicon_creator/imm_rom_ext/hello_world_binaries_silicon_creator.test_key_0.pre-signing failed: (Exit 1): opentitantool failed: error executing PreSigningArtifacts command (from target //sw/device/silicon_creator/imm_rom_ext:hello_world_binaries) bazel-out/k8-fastbuild-ST-13d3ddad9198/bin/sw/host/opentitantool/opentitantool '--rcfile=' --quiet image manifest update ... (remaining 4 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
Error: Image doesn't appear to contain a manifest, or the manifest is corrupted

Caused by:
    Condition failed: `manifest.signed_region_end <= len` (4294967295 vs 1048576)

Ah yes, in retrospect this makes sense (although the error message is obscure and seems to point an underflow in the code).

Actually I did found that the issues only happen when we use "//hw/top_earlgrey:silicon_creator" as the exec env, and using an empty manifest in that exec env can solve the issue. As currently all exec envs will derive same binaries and I plan to match the exec env between IM_EXT and ROM_EXT in the future, I will simply use fpga_cw340 as the only exec env. The exec env for silicon creator with empty manifest will be add in the future. And I also remove the attribute to skip the signing process.

That solution seems acceptable.

@lchiawei lchiawei force-pushed the add-hello-world-imm-rom-ext branch 2 times, most recently from 296ea5b to e526c0c Compare November 29, 2024 06:41
@rswarbrick rswarbrick removed their request for review December 11, 2024 14:39
Copy link
Contributor

@timothytrippel timothytrippel left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! I think we just need to rebase this again and fix the CI failure, then we can merge.

opentitan_binary(
name = "rom_ext_with_{}_imm_slot_virtual".format(name),
testonly = True,
# TODO(#22780): Integrate real keys for A1 flows.
Copy link
Contributor

Choose a reason for hiding this comment

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

we can remove this todo, it is no longer relevant I think

@timothytrippel timothytrippel requested a review from sasdf December 14, 2024 13:46
@lchiawei lchiawei force-pushed the add-hello-world-imm-rom-ext branch from e526c0c to e4988d5 Compare December 15, 2024 19:30
* Add minimized C code, startup asm and linker script for a standalone
  IM_EXT build target.
* Add a bazel rule `imm_rom_ext_section`, which will transform binaries
  into object files and turn it to a target that can be directly used as
  deps of ROM_EXT's bazel targets.
* Add a map for all IM_EXT target that can be used and make the ROM_EXT
  targets iterate the map to generate multiple targets.
* Update the ROM_EXT to use in e2e verified boot test.

Test: String `Immutable` appears in the UART output of the test target
`verified_boot:position_romext_virtual_a_with_hello_world_imm_romext_enabled_fpga_cw340_rom_ext`:
```
OpenTitan:4001-0002-01
ROM:0a7a997f
bootstrap:1
OpenTitan:4001-0002-01
ROM:0a7a997f
Immutable
Starting ROM_EXT 0.1
```

Signed-off-by: Chia-Wei Liu <[email protected]>
@lchiawei lchiawei force-pushed the add-hello-world-imm-rom-ext branch from e4988d5 to d5327e5 Compare December 15, 2024 19:32
@lchiawei
Copy link
Contributor Author

Hi @timothytrippel ,

Thanks for your review. I've removed those TODOs and rebased my commit.

@timothytrippel timothytrippel merged commit ee4a828 into lowRISC:master Dec 16, 2024
38 checks passed
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.

3 participants