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.

Sign up for GitHub

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

[rom_ext_e2e] Update CI to run ROM_EXT tests on hyper310 #24430

Merged
merged 3 commits into from
Aug 29, 2024
Merged

[rom_ext_e2e] Update CI to run ROM_EXT tests on hyper310 #24430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6f7d85a
[rom_ext] Fix rescue-to-SlotB
cfrantz Aug 29, 2024
9a7e239
[rom_ext_e2e] Use the `fpga_hyper310_rom_ext` environment
cfrantz Aug 29, 2024
4b78f40
[rom_ext_e2e] Update CI to run ROM_EXT tests on hyper310
cfrantz Aug 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions azure-pipelines.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,23 +654,23 @@ jobs:
demands: BOARD -equals cw310
timeoutInMinutes: 60
dependsOn:
- chip_earlgrey_cw310
- chip_earlgrey_cw310_hyperdebug
- sw_build
condition: succeeded( 'chip_earlgrey_cw310', 'sw_build' )
condition: succeeded( 'chip_earlgrey_cw310_hyperdebug', 'sw_build' )
steps:
- template: ci/checkout-template.yml
- template: ci/install-package-dependencies.yml
- template: ci/download-artifacts-template.yml
parameters:
downloadPartialBuildBinFrom:
- chip_earlgrey_cw310
- chip_earlgrey_cw310_hyperdebug
- sw_build
- template: ci/load-bazel-cache-write-creds.yml
- bash: |
set -e
. util/build_consts.sh
module load "xilinx/vivado/$(VIVADO_VERSION)"
ci/scripts/run-fpga-tests.sh cw310 fpga_cw310_rom_ext_tests || { res=$?; echo "To reproduce failures locally, follow the instructions at https://opentitan.org/book/doc/getting_started/setup_fpga.html#reproducing-fpga-ci-failures-locally"; exit "${res}"; }
ci/scripts/run-fpga-tests.sh hyper310 fpga_hyper310_rom_ext_tests || { res=$?; echo "To reproduce failures locally, follow the instructions at https://opentitan.org/book/doc/getting_started/setup_fpga.html#reproducing-fpga-ci-failures-locally"; exit "${res}"; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, it's missing the artifacts from (and dependency on) the chip_earlgrey_cw310_hyperdebug job. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not a 100% sure I believe we have tests that are defined for fpga_cw310_rom_ext and not fpga_hyper310_rom_ext_tests (like the cryptotests). With this changes, those tests will not run anymore, but maybe they don't need to run on earlgrey_es?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are some tests defined that use fpga_cw310_rom_ext. They're located in //sw/device/silicon_owner/bare_metal and //sw/device/tests/crypto. However, in the run-fpga-tests script, it appears that an fpga_tags argument (arg 2) of fpga_{whatevs}_rom_ext_tests sets the pattern to //sw/device/silicon_creator/rom_ext/e2e/..., so this pipeline stage was never running those other tests in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAICT, the following tests are not currently being run in CI:

//sw/device/silicon_owner/bare_metal:rom_ext_virtual_bare_metal_virtual_boot_test
//sw/device/silicon_owner/bare_metal:rom_ext_virtual_ottf_bl0_virtual
//sw/device/tests/crypto:otcrypto_hash_test
//sw/device/tests/crypto:otcrypto_export_test

The change in this PR does not affect whether or not these tests run.

It would be nice to run them on earlgrey_es_sival. We should make sure they run on master:

  • The bare metal test is a post-ROM_EXT test that checks if we can run owner code that is mostly separate from our existing test framework. We could argue about whether the this test truly provides value now that there are ROM_EXT E2E tests in place.
  • The crypto tests check that the crypto library distributable build artifact is functional.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to run these jobs on the earlgrey_es_sival branch, I think a new FPGA job will be needed. I will double-check that they run on master (or at least will run once the CI orchestrator is merged)

displayName: Execute tests
- template: ci/publish-bazel-test-results.yml

Expand Down
4 changes: 2 additions & 2 deletions ci/scripts/run-fpga-tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,9 @@ then
> "${pattern_file}"
# We need to remove tests tagged as manual since we are not using a wildcard target.
test_args="${test_args} --test_tag_filters=cw310_sival,-broken,-skip_in_ci,-manual"
elif [ "${fpga_tags}" == "fpga_cw310_rom_ext_tests" ]
elif [ "${fpga_tags}" == "fpga_hyper310_rom_ext_tests" ]
then
test_args="${test_args} --test_tag_filters=cw310_rom_ext,-broken,-skip_in_ci"
test_args="${test_args} --test_tag_filters=hyper310_rom_ext,-broken,-skip_in_ci"
echo "//sw/device/silicon_creator/rom_ext/e2e/..." > "${pattern_file}"
else
test_args="${test_args} --test_tag_filters=${fpga_tags},-broken,-skip_in_ci"
Expand Down
4 changes: 3 additions & 1 deletion rules/opentitan/defs.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,9 @@ def _parameter_name(env, pname):
def _hacky_tags(env):
(_, suffix) = env.split(":")
tags = []
if suffix.startswith("fpga_cw310_") or suffix.startswith("fpga_cw340_"):
Copy link
Contributor

Choose a reason for hiding this comment

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

It is possible that master and es have diverged at this point but in master we have a slightly better way of doing hacky_tags (still hacky):

opentitan/rules/opentitan/defs.bzl

Line 142 in cf32d0f

def _hacky_tags(env):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They have diverged and from a first look, the function on master looks better. I don't think I want to dive down the rabbithole of resolving this divergence.

if (suffix.startswith("fpga_cw310_") or
suffix.startswith("fpga_cw340_") or
suffix.startswith("fpga_hyper310")):
# We have tags like "cw310_rom_with_real_keys" or "cw310_test_rom"
# applied to our tests. Since there is no way to adjust tags in a
# rule's implementation, we have to infer these tag names from the
Expand Down
23 changes: 12 additions & 11 deletions sw/device/silicon_creator/rom_ext/e2e/boot_svc/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ opentitan_test(
name = "boot_svc_empty_test",
srcs = ["boot_svc_empty_test.c"],
exec_env = {
"//hw/top_earlgrey:fpga_cw310_rom_ext": None,
"//hw/top_earlgrey:fpga_hyper310_rom_ext": None,
},
fpga = fpga_params(
assemble = "{rom_ext}@0 {firmware}@0x10000 {firmware}@0x90000",
Expand All @@ -57,7 +57,7 @@ opentitan_test(
name = "boot_svc_wakeup_test",
srcs = ["boot_svc_wakeup_test.c"],
exec_env = {
"//hw/top_earlgrey:fpga_cw310_rom_ext": None,
"//hw/top_earlgrey:fpga_hyper310_rom_ext": None,
},
fpga = fpga_params(
assemble = "{rom_ext}@0 {firmware}@0x10000",
Expand Down Expand Up @@ -85,7 +85,7 @@ opentitan_test(
name = "boot_svc_next_test",
srcs = ["boot_svc_next_test.c"],
exec_env = {
"//hw/top_earlgrey:fpga_cw310_rom_ext": None,
"//hw/top_earlgrey:fpga_hyper310_rom_ext": None,
},
fpga = fpga_params(
assemble = "{rom_ext}@0 {firmware}@0x10000 {firmware}@0x90000",
Expand All @@ -111,7 +111,7 @@ opentitan_test(
name = "boot_svc_primary_test",
srcs = ["boot_svc_primary_test.c"],
exec_env = {
"//hw/top_earlgrey:fpga_cw310_rom_ext": None,
"//hw/top_earlgrey:fpga_hyper310_rom_ext": None,
},
fpga = fpga_params(
assemble = "{rom_ext}@0 {firmware}@0x10000 {firmware}@0x90000",
Expand Down Expand Up @@ -145,7 +145,7 @@ opentitan_test(
name = "boot_svc_bad_next_test",
srcs = ["boot_svc_bad_next_test.c"],
exec_env = {
"//hw/top_earlgrey:fpga_cw310_rom_ext": None,
"//hw/top_earlgrey:fpga_hyper310_rom_ext": None,
},
fpga = fpga_params(
assemble = "{rom_ext}@0 {firmware}@0x10000 {firmware}@0x90000",
Expand Down Expand Up @@ -173,26 +173,27 @@ opentitan_test(
"//sw/device/silicon_creator/rom_ext/e2e/verified_boot:boot_test",
],
exec_env = {
"//hw/top_earlgrey:fpga_cw310_rom_ext": None,
"//hw/top_earlgrey:fpga_hyper310_rom_ext": None,
},
fpga = fpga_params(
data = [
"//sw/device/silicon_creator/lib/ownership/keys/fake:no_owner_recovery_key",
],
exit_failure = "(PASS|FAIL|FAULT).*\n",
# This test requires serial break support which is not available in CI yet.
tags = ["broken"],
test_cmd = """
--exec="transport init"
--exec="fpga clear-bitstream"
--exec="fpga load-bitstream {bitstream}"
--exec="bootstrap --clear-uart=true {firmware}"
--exec="console --non-interactive --exit-success='ownership_state: \\x00\\x00\\x00\\x00\r\n' --exit-failure='{exit_failure}'"
--exec="console --non-interactive --exit-success='ownership_state = OWND\r\n' --exit-failure='{exit_failure}'"
--exec="rescue boot-svc ownership-unlock \
--mode Any \
--nonce 0 \
--sign $(location //sw/device/silicon_creator/lib/ownership/keys/fake:no_owner_recovery_key)"
--exec="console --non-interactive --exit-success='ownership_state = UANY\r\n' --exit-failure='{exit_failure}'"

# Since we've altered the ownership state, clear the bitstream to not affect later tests.
--exec="fpga clear-bitstream"
no-op
""",
),
Expand All @@ -215,7 +216,7 @@ opentitan_binary(
testonly = True,
srcs = ["boot_svc_min_sec_ver_test.c"],
exec_env = [
"//hw/top_earlgrey:fpga_cw310_rom_ext",
"//hw/top_earlgrey:fpga_hyper310_rom_ext",
],
linker_script = "//sw/device/lib/testing/test_framework:ottf_ld_silicon_owner_slot_virtual",
manifest = ":manifest_version_4",
Expand All @@ -237,7 +238,7 @@ opentitan_test(
name = "boot_svc_min_sec_ver_test",
srcs = ["boot_svc_min_sec_ver_test.c"],
exec_env = {
"//hw/top_earlgrey:fpga_cw310_rom_ext": None,
"//hw/top_earlgrey:fpga_hyper310_rom_ext": None,
},
fpga = fpga_params(
assemble = "{rom_ext}@0 {firmware}@0x10000 {min_sec_ver_4:signed_bin}@0x90000",
Expand Down
2 changes: 1 addition & 1 deletion sw/device/silicon_creator/rom_ext/e2e/handoff/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ _FAULT_TEST_CASES = {
],
defines = test_data["defines"],
exec_env = {
"//hw/top_earlgrey:fpga_cw310_rom_ext": None,
"//hw/top_earlgrey:fpga_hyper310_rom_ext": None,
},
fpga = fpga_params(
exit_success = test_data["exit_success"],
Expand Down
8 changes: 4 additions & 4 deletions sw/device/silicon_creator/rom_ext/e2e/lockdown/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ opentitan_test(
name = "otp_creator_lockdown",
srcs = ["otp_creator_lockdown.c"],
exec_env = {
"//hw/top_earlgrey:fpga_cw310_rom_ext": None,
"//hw/top_earlgrey:fpga_hyper310_rom_ext": None,
},
fpga = fpga_params(
exit_failure = "PASS|FAIL|BFV:.*\r\n",
Expand All @@ -36,7 +36,7 @@ opentitan_test(
name = "otp_dai_lockdown",
srcs = ["otp_dai_lockdown.c"],
exec_env = {
"//hw/top_earlgrey:fpga_cw310_rom_ext": None,
"//hw/top_earlgrey:fpga_hyper310_rom_ext": None,
},
fpga = fpga_params(
exit_failure = "PASS|FAIL|BFV:.*\r\n",
Expand All @@ -60,7 +60,7 @@ opentitan_test(
name = "sram_lockdown",
srcs = ["sram_lockdown.c"],
exec_env = {
"//hw/top_earlgrey:fpga_cw310_rom_ext": None,
"//hw/top_earlgrey:fpga_hyper310_rom_ext": None,
},
deps = [
"//hw/ip/sram_ctrl/data:sram_ctrl_regs",
Expand All @@ -74,7 +74,7 @@ opentitan_test(
name = "epmp_rlb_lockdown",
srcs = ["epmp_rlb_lockdown.c"],
exec_env = {
"//hw/top_earlgrey:fpga_cw310_rom_ext": None,
"//hw/top_earlgrey:fpga_hyper310_rom_ext": None,
},
deps = [
"//hw/top_earlgrey/sw/autogen:top_earlgrey",
Expand Down
13 changes: 4 additions & 9 deletions sw/device/silicon_creator/rom_ext/e2e/rescue/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ _POSITIONS = {
"//sw/device/silicon_creator/rom_ext/e2e/verified_boot:boot_test",
],
exec_env = [
"//hw/top_earlgrey:fpga_cw310_rom_ext",
"//hw/top_earlgrey:fpga_hyper310_rom_ext",
],
linker_script = position["linker_script"],
deps = [
Expand All @@ -52,15 +52,14 @@ _POSITIONS = {
opentitan_test(
name = "rescue_firmware_{}".format(name),
exec_env = {
"//hw/top_earlgrey:fpga_cw310_rom_ext": None,
"//hw/top_earlgrey:fpga_hyper310_rom_ext": None,
},
fpga = fpga_params(
assemble = "",
binaries = {
":boot_test_{}".format(name): "payload",
},
slot = position["slot"],
tags = ["broken"],
test_cmd = """
--exec="transport init"
--exec="fpga load-bitstream {bitstream}"
Expand All @@ -81,16 +80,14 @@ _POSITIONS = {
opentitan_test(
name = "next_slot",
exec_env = {
"//hw/top_earlgrey:fpga_cw310_rom_ext": None,
"//hw/top_earlgrey:fpga_hyper310_rom_ext": None,
},
fpga = fpga_params(
assemble = "{rom_ext}@0 {slot_a:signed_bin}@0x10000 {slot_b:signed_bin}@0x90000",
binaries = {
":boot_test_slot_a": "slot_a",
":boot_test_slot_b": "slot_b",
},
# This test requires serial break support which is not available in CI yet.
tags = ["broken"],
test_cmd = """
--exec="transport init"
--exec="fpga load-bitstream {bitstream}"
Expand All @@ -111,16 +108,14 @@ opentitan_test(
opentitan_test(
name = "primary_slot",
exec_env = {
"//hw/top_earlgrey:fpga_cw310_rom_ext": None,
"//hw/top_earlgrey:fpga_hyper310_rom_ext": None,
},
fpga = fpga_params(
assemble = "{rom_ext}@0 {slot_a:signed_bin}@0x10000 {slot_b:signed_bin}@0x90000",
binaries = {
":boot_test_slot_a": "slot_a",
":boot_test_slot_b": "slot_b",
},
# This test requires serial break support which is not available in CI yet.
tags = ["broken"],
test_cmd = """
--exec="transport init"
--exec="fpga load-bitstream {bitstream}"
Expand Down
6 changes: 3 additions & 3 deletions sw/device/silicon_creator/rom_ext/e2e/verified_boot/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ _POSITIONS = {
name = "position_{}".format(name),
srcs = [":boot_test"],
exec_env = {
"//hw/top_earlgrey:fpga_cw310_rom_ext": None,
"//hw/top_earlgrey:fpga_hyper310_rom_ext": None,
},
fpga = fpga_params(
assemble = "{romext}@{romext_offset} {firmware}@{owner_offset}",
Expand Down Expand Up @@ -124,7 +124,7 @@ opentitan_test(
name = "bad_manifest_test",
srcs = [":boot_test"],
exec_env = {
"//hw/top_earlgrey:fpga_cw310_rom_ext": None,
"//hw/top_earlgrey:fpga_hyper310_rom_ext": None,
},
fpga = fpga_params(
exit_failure = "PASS|FAIL|FAULT|BFV:.{8}",
Expand Down Expand Up @@ -169,7 +169,7 @@ _KEYS = {
name = "key_{}".format(name),
srcs = [":boot_test"],
exec_env = {
"//hw/top_earlgrey:fpga_cw310_rom_ext": None,
"//hw/top_earlgrey:fpga_hyper310_rom_ext": None,
},
fpga = fpga_params(
exit_failure = keyinfo["exit_failure"],
Expand Down
11 changes: 8 additions & 3 deletions sw/device/silicon_creator/rom_ext/rescue.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,11 @@ const uint32_t kFlashBankSize =
static rescue_state_t rescue_state;

rom_error_t flash_firmware_block(rescue_state_t *state) {
uint32_t bank_offset =
state->mode == kRescueModeFirmwareSlotB ? kFlashBankSize : 0;
if (state->flash_offset == 0) {
// TODO(#24428): Make sure we interact correctly with owner flash region
// configuration.
flash_ctrl_data_default_perms_set((flash_ctrl_perms_t){
.read = kMultiBitBool4True,
.write = kMultiBitBool4True,
Expand All @@ -38,14 +42,14 @@ rom_error_t flash_firmware_block(rescue_state_t *state) {
for (uint32_t addr = state->flash_start; addr < state->flash_limit;
addr += kFlashPageSize) {
HARDENED_RETURN_IF_ERROR(
flash_ctrl_data_erase(addr, kFlashCtrlEraseTypePage));
flash_ctrl_data_erase(bank_offset + addr, kFlashCtrlEraseTypePage));
}
state->flash_offset = state->flash_start;
}
if (state->flash_offset < state->flash_limit) {
HARDENED_RETURN_IF_ERROR(flash_ctrl_data_write(
state->flash_offset, sizeof(state->data) / sizeof(uint32_t),
state->data));
bank_offset + state->flash_offset,
sizeof(state->data) / sizeof(uint32_t), state->data));
state->flash_offset += sizeof(state->data);
} else {
xmodem_cancel(iohandle);
Expand Down Expand Up @@ -134,6 +138,7 @@ static void validate_mode(uint32_t mode, rescue_state_t *state,
}
break;
case kRescueModeFirmware:
case kRescueModeFirmwareSlotB:
dbg_printf("ok: send firmware via xmodem-crc\r\n");
break;
case kRescueModeReboot:
Expand Down
Loading