Eugene Myers 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 11:
(11 comments)
Patch Set 10:
(11 comments)
I'm looking forward to testing STM on the Asrock B85M Pro4.
I appreciate any feedback you can provide
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 th […]
Agreed, It seems that this would be best done in cpu/x86/smm/tseg_region.c. I would suggest adding the MSEG size as part of the calculation and including the subregion names (MSEG, IED, etc) in the 'SMM Memory Map' output to make it clear.
I'll make a stab at this and submit a patch to do the above.
https://review.coreboot.org/c/coreboot/+/44686/10/src/security/intel/stm/Kco... PS10, Line 60: ssh
I'd expect HTTPS
Done
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. […]
Done
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: […]
Done
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 R […]
Done
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... […]
Removed it.
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
Not quite sure what you mean here. I did remove the 'rm -rf build' as the cmake causes a rebuild. This has to happen to ensure that any changes in the parameter setting are propagated through the code.
Any suggestions otherwise would be appreciated.
https://review.coreboot.org/c/coreboot/+/44686/10/src/security/intel/stm/Mak... PS10, Line 31: checkout
I don't see that target anywhere
Done
https://review.coreboot.org/c/coreboot/+/44686/10/src/security/intel/stm/Mak... PS10, Line 31: fetch
Same here
Done
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. […]
fixed
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. […]
The issue here is that if the developer changes any of the STM parameters, then the build will not get triggered.
Any suggestions on how to trigger this otherwise.