Fix chip info in the ROM/ROM_EXT #28418
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 <[email protected]>
Without this option, the chip_info symbol might get discarded if it is not used directly. Signed-off-by: Amaury Pouly <[email protected]>
pamaury
force-pushed
the
fix_chip_info
branch
2 times, most recently
from
4d77037 to
cb4e92a
Compare
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 <[email protected]>
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 <[email protected]>
engdoreis
reviewed
Oct 6, 2025
| ], | ||
| ) | ||
|
|
||
| opentitan_test( |
There was a problem hiding this comment.
Can you please add an entry to the tesplan?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While looking at the chip info, I realized that there were several problems. Originally, the issue comes from the fact that we have a single
cc_librarythat contains both thechip_info.hheader and thechip_info.cdata. This means that any binary using this dependency ends up including thekChipInfodata even if they didn't intend to. Coupled with the fact that several pieces of code directly refer tokChipInfoinstead of the linker symbol_chip_info_startmeans that for example part of the ROM_EXT doesn't actually read from the ROM's chip_info but from a ROM_EXT-embedded chip info!I decided to fix that by splitting the library into the header (which is what everyone but the ROM should use) and the data. I also eliminated the
kChipInfosymbol from the header entirely and replaced all users with_chip_info_start. While doing so, I stumbled on a subtle linker problem which is that sincekChipInfobecomes unused, it won't participate in linking (even if the section is marked as NODISARD) unless--whole-archiveis used which fortunaltely bazel has a setting for (alwayslink=True).I also decided to create a new ROM end-to-end test for the chip info to make sure that the data is present there. Maybe it might be worth also create a ROM_EXT end-to-end test to make sure that it uses the ROM chip info and not some other chip info but I leave that to a future PR.