From 0a81509c5b094d4699384040826b322ac186ed2d Mon Sep 17 00:00:00 2001 From: Amaury Pouly Date: Fri, 3 Oct 2025 13:56:57 +0000 Subject: [PATCH 1/4] [silicon_creator] Split chip_info data from headers Having all in one is problematic because it can cause a non-ROM binary to include the chip info symbol which will then be placed somewhere else and override the actual ROM chip info. The ROM should be the only one including the chip info data whereas all other binaries only need the header. Signed-off-by: Amaury Pouly --- rules/autogen.bzl | 7 ++----- sw/device/silicon_creator/lib/BUILD | 13 ++++++++++++- sw/device/silicon_creator/rom/BUILD | 2 ++ 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/rules/autogen.bzl b/rules/autogen.bzl index b1fec9b2db970..1d5e49f418701 100644 --- a/rules/autogen.bzl +++ b/rules/autogen.bzl @@ -184,7 +184,7 @@ autogen_chip_info_src = rule( } | stamp_attr(-1, "//rules:stamp_flag"), ) -def autogen_chip_info(name): +def autogen_chip_info(name, deps = []): """Generates a cc_library named `name` that defines chip info.""" # Generate a C source file that defines the chip info struct. This is an @@ -198,10 +198,7 @@ def autogen_chip_info(name): native.cc_library( name = name, srcs = [chip_info_src_target], - hdrs = ["//sw/device/silicon_creator/lib:chip_info.h"], - deps = [ - "//sw/device/lib/base:macros", - ], + deps = deps, ) def _cryptotest_hjson_external(ctx): diff --git a/sw/device/silicon_creator/lib/BUILD b/sw/device/silicon_creator/lib/BUILD index 668013f44a460..223587928460e 100644 --- a/sw/device/silicon_creator/lib/BUILD +++ b/sw/device/silicon_creator/lib/BUILD @@ -352,7 +352,18 @@ filegroup( ], ) -autogen_chip_info(name = "chip_info") +cc_library( + name = "chip_info", + hdrs = ["chip_info.h"], +) + +autogen_chip_info( + name = "chip_info_data", + deps = [ + ":chip_info", + "//sw/device/lib/base:macros", + ], +) dual_cc_library( name = "shutdown", diff --git a/sw/device/silicon_creator/rom/BUILD b/sw/device/silicon_creator/rom/BUILD index cceb9a8cd4ccb..0cb5106f54042 100644 --- a/sw/device/silicon_creator/rom/BUILD +++ b/sw/device/silicon_creator/rom/BUILD @@ -123,6 +123,8 @@ cc_library( "//sw/device/lib/base:hardened", "//sw/device/lib/base:multibits", "//sw/device/silicon_creator/lib/base:chip", + # This embeds the chip info into the binary. + "//sw/device/silicon_creator/lib:chip_info_data", ], alwayslink = True, ) From 59dd03e6749ac2b6b7034dc75714def50d9661b2 Mon Sep 17 00:00:00 2001 From: Amaury Pouly Date: Fri, 3 Oct 2025 13:58:27 +0000 Subject: [PATCH 2/4] [rules] Mark chip_info as always-link and symbol as used Without this option, the chip_info symbol might get discarded if it is not used directly. Signed-off-by: Amaury Pouly --- rules/autogen.bzl | 3 +++ util/rom_chip_info.py | 1 + 2 files changed, 4 insertions(+) diff --git a/rules/autogen.bzl b/rules/autogen.bzl index 1d5e49f418701..c8daf6b5925ae 100644 --- a/rules/autogen.bzl +++ b/rules/autogen.bzl @@ -199,6 +199,9 @@ def autogen_chip_info(name, deps = []): name = name, srcs = [chip_info_src_target], deps = deps, + # Make sure to participate in linking so that the symbol is not discarded + # (since it is not meant to be directly used). + alwayslink = True, ) def _cryptotest_hjson_external(ctx): diff --git a/util/rom_chip_info.py b/util/rom_chip_info.py index b20394fd1b4a3..978ce6413fb12 100755 --- a/util/rom_chip_info.py +++ b/util/rom_chip_info.py @@ -34,6 +34,7 @@ def generate_chip_info_c_source(scm_revision: int) -> str: #include "sw/device/lib/base/macros.h" +OT_USED OT_SECTION(".chip_info") const chip_info_t kChipInfo = {{ .scm_revision = (chip_info_scm_revision_t){{ From 8b8d1719999177ba5e61b3c5b1e3d90de2fa867d Mon Sep 17 00:00:00 2001 From: Amaury Pouly Date: Fri, 3 Oct 2025 13:59:41 +0000 Subject: [PATCH 3/4] [silicon_creator] Use linker symbol to access the chip_info Some places were using a symbol provided by chip_info.h but this symbol might not be present in the binary. Instead, the expected way to access the symbol is to use the linker provided one which uses a fixed address. Signed-off-by: Amaury Pouly --- sw/device/lib/testing/test_rom/test_rom.c | 3 ++- sw/device/silicon_creator/lib/chip_info.h | 7 +++---- sw/device/silicon_creator/lib/shutdown.c | 3 ++- sw/device/silicon_creator/rom/rom.c | 3 ++- 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/sw/device/lib/testing/test_rom/test_rom.c b/sw/device/lib/testing/test_rom/test_rom.c index 02084b69b83e2..30f48da5ee0ca 100644 --- a/sw/device/lib/testing/test_rom/test_rom.c +++ b/sw/device/lib/testing/test_rom/test_rom.c @@ -155,7 +155,8 @@ bool rom_test_main(void) { } // Print the chip version information - LOG_INFO("kChipInfo: scm_revision=%x", kChipInfo.scm_revision); + chip_info_t *chip_info = (chip_info_t *)&_chip_info_start; + LOG_INFO("kChipInfo: scm_revision=%x", chip_info->scm_revision); // Skip sram_init for test_rom dif_rstmgr_reset_info_bitfield_t reset_reasons; diff --git a/sw/device/silicon_creator/lib/chip_info.h b/sw/device/silicon_creator/lib/chip_info.h index dc68389b60ea9..34712ac78af66 100644 --- a/sw/device/silicon_creator/lib/chip_info.h +++ b/sw/device/silicon_creator/lib/chip_info.h @@ -51,10 +51,9 @@ enum { }; /** - * Extern declaration for the `kChipInfo` instance placed at the end of ROM. - * - * The actual definition is in an auto-generated file. + * The chip information is placed at the end of ROM and is accessible using + * the following symbol. */ -extern const chip_info_t kChipInfo; +extern const char _chip_info_start[]; #endif // OPENTITAN_SW_DEVICE_SILICON_CREATOR_LIB_CHIP_INFO_H_ diff --git a/sw/device/silicon_creator/lib/shutdown.c b/sw/device/silicon_creator/lib/shutdown.c index 8e4000175e789..d6915439651ba 100644 --- a/sw/device/silicon_creator/lib/shutdown.c +++ b/sw/device/silicon_creator/lib/shutdown.c @@ -413,10 +413,11 @@ SHUTDOWN_FUNC(NO_MODIFIERS, shutdown_report_error(rom_error_t reason)) { // Print the error message and the raw life cycle state as reported by the // hardware. + const chip_info_t *rom_chip_info = (const chip_info_t *)_chip_info_start; shutdown_print(kShutdownLogPrefixBootFault, redacted_error); shutdown_print(kShutdownLogPrefixLifecycle, raw_state); shutdown_print(kShutdownLogPrefixVersion, - kChipInfo.scm_revision.scm_revision_high); + rom_chip_info->scm_revision.scm_revision_high); } SHUTDOWN_FUNC(NO_MODIFIERS, shutdown_software_escalate(void)) { diff --git a/sw/device/silicon_creator/rom/rom.c b/sw/device/silicon_creator/rom/rom.c index c1f5c209d38cd..e601c93b9ed0a 100644 --- a/sw/device/silicon_creator/rom/rom.c +++ b/sw/device/silicon_creator/rom/rom.c @@ -244,9 +244,10 @@ static rom_error_t rom_init(void) { // Initialize boot_log boot_log_t *boot_log = &retention_sram_get()->creator.boot_log; + const chip_info_t *rom_chip_info = (const chip_info_t *)_chip_info_start; memset(boot_log, 0, sizeof(*boot_log)); boot_log->identifier = kBootLogIdentifier; - boot_log->chip_version = kChipInfo.scm_revision; + boot_log->chip_version = rom_chip_info->scm_revision; boot_log->retention_ram_initialized = reset_reasons & reset_mask ? kHardenedBoolTrue : kHardenedBoolFalse; From 30d6790d6894eb76dbc20a2d6f2d08166d00a5cf Mon Sep 17 00:00:00 2001 From: Amaury Pouly Date: Fri, 3 Oct 2025 14:02:15 +0000 Subject: [PATCH 4/4] [rom,e2e] Add a test for the chip_info The chip_info became broken without anyone realizing because it was untested. This test ensures that at least the chip info is filled and has the expected version. Signed-off-by: Amaury Pouly --- sw/device/silicon_creator/rom/e2e/BUILD | 13 ++++++++++++ .../silicon_creator/rom/e2e/chip_info_test.c | 21 +++++++++++++++++++ 2 files changed, 34 insertions(+) create mode 100644 sw/device/silicon_creator/rom/e2e/chip_info_test.c diff --git a/sw/device/silicon_creator/rom/e2e/BUILD b/sw/device/silicon_creator/rom/e2e/BUILD index 13b80f7a6ca39..7d4e449d7965d 100644 --- a/sw/device/silicon_creator/rom/e2e/BUILD +++ b/sw/device/silicon_creator/rom/e2e/BUILD @@ -472,3 +472,16 @@ opentitan_test( "//sw/device/silicon_creator/lib/drivers:rstmgr", ], ) + +opentitan_test( + name = "rom_e2e_chip_info", + srcs = ["chip_info_test.c"], + exec_env = { + "//hw/top_earlgrey:fpga_cw310_rom_with_fake_keys": None, + "//hw/top_earlgrey:fpga_cw340_rom_with_fake_keys": None, + }, + deps = [ + "//sw/device/lib/testing/test_framework:ottf_main", + "//sw/device/silicon_creator/lib:chip_info", + ], +) diff --git a/sw/device/silicon_creator/rom/e2e/chip_info_test.c b/sw/device/silicon_creator/rom/e2e/chip_info_test.c new file mode 100644 index 0000000000000..2f5c9afa15d75 --- /dev/null +++ b/sw/device/silicon_creator/rom/e2e/chip_info_test.c @@ -0,0 +1,21 @@ +// Copyright lowRISC contributors (OpenTitan project). +// Licensed under the Apache License, Version 2.0, see LICENSE for details. +// SPDX-License-Identifier: Apache-2.0 + +#include "sw/device/silicon_creator/lib/chip_info.h" + +#include + +#include "sw/device/lib/testing/test_framework/ottf_main.h" + +OTTF_DEFINE_TEST_CONFIG(); + +bool test_main(void) { + chip_info_t *chip_info = (chip_info_t *)&_chip_info_start; + LOG_INFO("Chip Info"); + LOG_INFO(" Version: %x", chip_info->version); + LOG_INFO(" SCM lo: %x", chip_info->scm_revision.scm_revision_low); + LOG_INFO(" SCM hi: %x", chip_info->scm_revision.scm_revision_high); + + return chip_info->version == kChipInfoVersion1; +}