Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44686 )
Change subject: security/intel/stm: Add options for STM build ......................................................................
Patch Set 10:
(11 comments)
I'm looking forward to testing STM on the Asrock B85M Pro4.
https://review.coreboot.org/c/coreboot/+/44686/10/src/security/intel/stm/Kco... File src/security/intel/stm/Kconfig:
https://review.coreboot.org/c/coreboot/+/44686/10/src/security/intel/stm/Kco... PS10, Line 35: SMM_TSEG_SIZE = 0x800000 It would be better to calculate the MSEG size (and all other relevant values) automatically using the values set in Kconfig options. This needs to be done in a header or similar, because Kconfig can't do math.
https://review.coreboot.org/c/coreboot/+/44686/10/src/security/intel/stm/Kco... PS10, Line 60: ssh I'd expect HTTPS
https://review.coreboot.org/c/coreboot/+/44686/10/src/security/intel/stm/Kco... PS10, Line 64: bios_resource_list_size This is a user-visible prompt. It shouldn't be snake_case
https://review.coreboot.org/c/coreboot/+/44686/10/src/security/intel/stm/Kco... PS10, Line 90: 0x3F8 There's a Kconfig option to control the serial port I/O base address for regular coreboot messages: `TTYS0_BASE`. I'd default to it instead of a hardcoded value.
Plus, I'd rename this symbol to `STM_TTYS0_BASE` for consistency
https://review.coreboot.org/c/coreboot/+/44686/10/src/security/intel/stm/Kco... PS10, Line 106: help : The default (debug) build will generate runtime console output. : "release" will deactivate the console output. I would wrap this in a choice (like the one we use for mainboard vendors and models or the one for ROM size, for instance)
https://review.coreboot.org/c/coreboot/+/44686/10/src/security/intel/stm/Kco... PS10, Line 110: STM_GIT_BRANCH I'm afraid I have bad news for you... With submodules, one can't select which branch to use with a Kconfig option. 😞
Since submodule pointers refer to a git commit hash, it's important to preserve the history (no force-pushing things, please), but it doesn't matter which branch contains the commit hash (so you can always make a `coreboot-stable_YYYY-MM-DD` branch every time you update the submodule pointer in coreboot).
We had a similar problem with the FSP repo, and the way to solve it was this commit: https://github.com/intel/FSP/commit/667eb3edc5d5cd562c33d9fbe89b9bee6a8a80ae
For development, the best approach when using submodules would be to locally disable updates for 3rdparty/STM (setting update=none and/or ignore=all should work), and then use force-add if you want to commit an update to coreboot's submodule pointer. I'm not a Git expert, though.
https://review.coreboot.org/c/coreboot/+/44686/10/src/security/intel/stm/Mak... File src/security/intel/stm/Makefile:
https://review.coreboot.org/c/coreboot/+/44686/10/src/security/intel/stm/Mak... PS10, Line 14: rm -rf build; \ : mkdir -p build; \ : cd build; \ This looks like there's a missing dependency somewhere, and STM doesn't get rebuilt when it should
https://review.coreboot.org/c/coreboot/+/44686/10/src/security/intel/stm/Mak... PS10, Line 31: checkout I don't see that target anywhere
https://review.coreboot.org/c/coreboot/+/44686/10/src/security/intel/stm/Mak... PS10, Line 31: fetch Same here
https://review.coreboot.org/c/coreboot/+/44686/10/src/security/intel/stm/Mak... File src/security/intel/stm/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/44686/10/src/security/intel/stm/Mak... PS10, Line 12: 3rdparty/stm/Stm/build/StmPkg/Core/stm.bin stm: This will create two identical recipes, one for the `3rdparty/stm/Stm/build/StmPkg/Core/stm.bin` target, and another for the `stm` target. If the `stm` target is ever used, this will randomly break parallel builds.
I had to make CB:39188 to fix a race condition in SeaBIOS when using multiple threads.
https://review.coreboot.org/c/coreboot/+/44686/10/src/security/intel/stm/Mak... PS10, Line 21: 3rdparty/stm/Stm/build/StmPkg/Core/stm.bin This target is a valid file name. It is not a phony: https://www.gnu.org/software/make/manual/html_node/Phony-Targets.html