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

[bazel] Pass copt and features through the OT transition #20388

Merged
merged 1 commit into from
Jan 13, 2024
Merged

[bazel] Pass copt and features through the OT transition #20388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 7 additions & 2 deletions azure-pipelines.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,12 +339,17 @@ jobs:
displayName: Build & execute tests
- template: ci/publish-bazel-test-results.yml
# TODO: build and cache the verilator model to avoid building twice (#12574)
# NOTE: The new build/test rules reference verilator as a dependency, but under the
# platforms transition of that rule. Therefore, verilator is built under a
# configuration like 'k8-opt-exec-$host-FOR-$target'. In order to get the
# verilator binary, we query the output of one of the verilated tests and ask
# for the verilator binary, which is in a subdir named 'build.verilator_<stuff>'.
- bash: |
. util/build_consts.sh
mkdir -p "$BIN_DIR/hw/top_earlgrey/"
cp $(ci/scripts/target-location.sh //hw:verilator) \
cp $(./bazelisk.sh outquery-build.verilator //sw/device/tests:uart_smoketest_sim_verilator) \
"$BIN_DIR/hw/top_earlgrey/Vchip_earlgrey_verilator"
displayName: Copy //hw:verilator to $BIN_DIR
displayName: Copy verilated binary to $BIN_DIR
- template: ci/upload-artifacts-template.yml
parameters:
includePatterns:
Expand Down
6 changes: 3 additions & 3 deletions bazelisk.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,16 +79,16 @@ function outquery_starlark_expr() {
echo "target.files.to_list()[0].path"
;;
-all)
echo "\"\\n\".join([f.path for f in target.files.to_list()])"
echo "\"\\n\".join([f.path for f in depset(transitive=[target.files, target.default_runfiles.files]).to_list()])"
;;
-providers)
echo "providers(target)"
;;
-*)
echo "\"\\n\".join([f.path for f in target.files.to_list() if \"$q\"[1:] in f.path])"
echo "\"\\n\".join([f.path for f in depset(transitive=[target.files, target.default_runfiles.files]).to_list() if \"$q\"[1:] in f.path])"
;;
.*)
echo "\"\\n\".join([f.path for f in target.files.to_list() if f.path.endswith(\"$q\")])"
echo "\"\\n\".join([f.path for f in depset(transitive=[target.files, target.default_runfiles.files]).to_list() if f.path.endswith(\"$q\")])"
;;
esac
}
Expand Down
7 changes: 5 additions & 2 deletions ci/scripts/run-english-breakfast-verilator-tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,16 @@ ci/bazelisk.sh build \

# Run the one test.
# This needs to be run outside the bazel sandbox, so we do not use `bazel run`
#
# NOTE: we specify `-type f` in the find commands to avoid finding any
# additional symlinks bazel might have prepared when building the test targets.
bazel-bin/sw/host/opentitantool/opentitantool \
--rcfile="" \
--logging=info \
--interface=verilator \
--verilator-bin=$BIN_DIR/hw/top_englishbreakfast/Vchip_englishbreakfast_verilator \
--verilator-rom="$(find bazel-out/* -name 'test_rom_sim_verilator.32.vmem')" \
--verilator-flash="$(find bazel-out/* -name 'aes_smoketest_prog_sim_verilator.64.scr.vmem')" \
--verilator-rom="$(find bazel-out/* -type f -name 'test_rom_sim_verilator.32.vmem')" \
--verilator-flash="$(find bazel-out/* -type f -name 'aes_smoketest_sim_verilator.64.vmem')" \
console \
--exit-failure="(FAIL|FAULT).*\n" \
--exit-success="PASS.*\n" \
Expand Down
8 changes: 4 additions & 4 deletions hw/top_earlgrey/dv/chip_sim_cfg.hjson
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1220,14 +1220,14 @@
{
name: chip_sw_aes_enc
uvm_test_seq: chip_sw_base_vseq
sw_images: ["//sw/device/tests:aes_smoketest:1"]
sw_images: ["//sw/device/tests:aes_smoketest:1:new_rules"]
Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, it seems there are some other "sw_images" missing the new_rules suffix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you point out where I've missed them? I can't seem to find any with grep.

en_run_modes: ["sw_test_mode_test_rom"]
run_opts: ["+sw_test_timeout_ns=22_000_000"]
}
{
name: chip_sw_aes_enc_jitter_en
uvm_test_seq: chip_sw_base_vseq
sw_images: ["//sw/device/tests:aes_smoketest:1"]
sw_images: ["//sw/device/tests:aes_smoketest:1:new_rules"]
en_run_modes: ["sw_test_mode_test_rom"]
run_opts: ["+sw_test_timeout_ns=26_000_000", "+en_jitter=1"]
}
Expand Down Expand Up @@ -1912,7 +1912,7 @@
{
name: chip_sw_rv_core_ibex_lockstep_glitch
uvm_test_seq: chip_sw_rv_core_ibex_lockstep_glitch_vseq
sw_images: ["sw/device/tests:aes_smoketest:1"]
sw_images: ["sw/device/tests:aes_smoketest:1:new_rules"]
en_run_modes: ["sw_test_mode_test_rom"]
// This test currently stops without completing all transactions, so we
// have to disable the final assertions.
Expand Down Expand Up @@ -1980,7 +1980,7 @@
{
name: chip_sw_aes_enc_jitter_en_reduced_freq
uvm_test_seq: chip_sw_base_vseq
sw_images: ["//sw/device/tests:aes_smoketest:1"]
sw_images: ["//sw/device/tests:aes_smoketest:1:new_rules"]
en_run_modes: ["sw_test_mode_test_rom"]
run_opts: ["+sw_test_timeout_ns=26_000_000", "+en_jitter=1", "+cal_sys_clk_70mhz=1"]
}
Expand Down
2 changes: 1 addition & 1 deletion hw/top_earlgrey/dv/chip_smoketests.hjson
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
{
name: chip_sw_aes_smoketest
uvm_test_seq: chip_sw_base_vseq
sw_images: ["//sw/device/tests:aes_smoketest:1"]
sw_images: ["//sw/device/tests:aes_smoketest:1:new_rules"]
en_run_modes: ["sw_test_mode_test_rom"]
}
{
Expand Down
3 changes: 2 additions & 1 deletion hw/top_englishbreakfast/util/prepare_sw.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
'//sw/device/lib/testing/test_rom',
'//sw/device/sca:aes_serial',
'//sw/device/examples/hello_world',
'//sw/device/tests:aes_smoketest_prog',
'//sw/device/tests:aes_smoketest_sim_verilator',
]


Expand Down Expand Up @@ -182,6 +182,7 @@ def main():
'build',
'--features=-rv32_bitmanip',
'--copt=-DOT_IS_ENGLISH_BREAKFAST_REDUCED_SUPPORT_FOR_INTERNAL_USE_ONLY_',
Comment on lines 183 to 184
Copy link
Contributor

Choose a reason for hiding this comment

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

And these are the --features and --copt you intended to pass for the risc V code generation. Along the line of @a-will's concern, is it safe to assume no other --copt flags will sneak in via some other kind of command? For example, the regular test rules will build both harness and device code, so if --copt was given in the bazel test invocation intended for the harness it may sneak through.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The --copt and --features serve two purposes:

  • The copts identify the platform as englishbreakfast and cause the preprocessor symbol OT_IS_ENGLISH_BREAKFAST to get defined (in macros.h).
  • The features identify CPU code generation options for the specific rv32imc implementation in englishbreakfast. Most notably, it disables the bitmanip and crc32 extensions.

Unfortunately, the copts currently have to be passed through to host code generation to make sure the correct enumerations/defines are available for DIFs (e.g. rstmgr reasons) and for bindgen'ing the constants in to Rust opentitanlib/tool.

The feature flags matter less, but they also have no meaning to the host compiler - the bazel compiler configuration for the host doesn't have a feature called rv32_bitmanip and ignores it.

I don't particularly like this situation with copt leaking into other configurations, but until we change the overall hacky way englishbreakfast is handled in the codebase, we need it. Eliminating all legacy usage of opentitan_functest in favor of the new rules will help us get to a configuration where englishbreakfast is a true separate top supported by exec_envs rather than by the current tree-dirtying operation.

'--define=DISABLE_VERILATOR_BUILD=true',
] + BAZEL_BINARIES)


Expand Down
14 changes: 13 additions & 1 deletion rules/rv.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,28 @@ PER_DEVICE_DEPS = {
def _opentitan_transition_impl(settings, attr):
return {
"//command_line_option:platforms": attr.platform,
"//command_line_option:copt": settings["//command_line_option:copt"],
"//command_line_option:features": settings["//command_line_option:features"],
"//hw/bitstream/universal:rom": "//hw/bitstream/universal:none",
"//hw/bitstream/universal:otp": "//hw/bitstream/universal:none",
"//hw/bitstream/universal:env": "//hw/bitstream/universal:none",
}

opentitan_transition = transition(
implementation = _opentitan_transition_impl,
inputs = [],
# In order to build the englishbreakfast binaries, we need to pass through
# the `--copt` and `--features` flags:
# - The copt flag defines a preprocessor symbol indicating englishbreakfast.
# - The features flags turn off compiler support for CPU extensions not
# present in the englishbreakfast rv32i implementation.
inputs = [
"//command_line_option:copt",
"//command_line_option:features",
],
outputs = [
"//command_line_option:platforms",
"//command_line_option:copt",
"//command_line_option:features",
"//hw/bitstream/universal:rom",
"//hw/bitstream/universal:otp",
"//hw/bitstream/universal:env",
Expand Down
7 changes: 2 additions & 5 deletions sw/device/tests/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,13 +140,10 @@ alias(
visibility = ["//visibility:private"],
)

# TODO(#19926): The `opentitan_test` rule doesn't propagate --copt accross all
# dependencies, causing issues when compiling targets against
# --copt=-DOT_IS_ENGLISH_BREAKFAST_REDUCED_SUPPORT_FOR_INTERNAL_USE_ONLY_.
# Using `opentitan_functest` as a workaround.
opentitan_functest(
opentitan_test(
name = "aes_smoketest",
srcs = ["aes_smoketest.c"],
exec_env = EARLGREY_TEST_ENVS,
deps = [
"//hw/ip/aes:model",
"//hw/top_earlgrey/sw/autogen:top_earlgrey",
Expand Down
Loading