Hello Eugene Myers,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/44686
to review the following change.
Change subject: security/intel/stm: Add options for STM build ......................................................................
security/intel/stm: Add options for STM build
This patch adds options that support building the STM as a part of the coreboot build. The option defaults are set so that the STM can be built for a majority of coreboot platforms.
Change-Id: I80ed7cbcb93468c5ff93d089d77742ce7b671a37 Signed-off-by: Eugene D Myers cedarhouse@comcast.net --- M src/security/intel/stm/Kconfig 1 file changed, 77 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/44686/1
diff --git a/src/security/intel/stm/Kconfig b/src/security/intel/stm/Kconfig index f7dd363..4d963aa 100644 --- a/src/security/intel/stm/Kconfig +++ b/src/security/intel/stm/Kconfig @@ -1,3 +1,4 @@ + config STM bool "Enable STM" default n @@ -27,20 +28,93 @@
config MSEG_SIZE hex "mseg size" - default 0x400000 + default 0x100000 help - STM only - 0x100000 + The MSEG_SIZE of 0x100000 assumes that: + IED_REGION_SIZE = 0x400000 + SMM_RESERVED_SIZE = 0x200000 + SMM_TSEG_SIZE = 0x800000 + To use STM/PE, a larger MSEG_SIZE is necessary. This can be + done by either increasing SMM_TSEG_SIZE or reducing the + IED_REGION_SIZE and/or SMM_RESERVED_SIZE or some combination + of the three. + NOTE: The authors experience is that these configuration + parameters have to be changed at the soc Konfig for them to + be applied. + Minimum sizes: + STM only - 0x100000 - Supports up to 38 processor threads + - 0x200000 - Supports up to 102 processor threads STM/PE - 0x300000+ depending on the amount of memory needed for the protected execution virtual machine (VM/PE)
+config STM_STMPE_ENABLED + bool "STM/PE Enabled" + default n + help + STM/PE provides for additional virtual machines in SMRAM + that provides a protected execution environment for + applications such as introspection, which need to be + protected from malicious code. More information can be + found on the stmpe branch of + ssh://review.coreboot.org/STM + + config BIOS_RESOURCE_LIST_SIZE hex "bios_resource_list_size" default 0x1000 + help + The BIOS resource list defines the resources that the + SMI Handler needs. This list is created during the + coreboot bootup. Unless there has been a lot of elements + added to this list, this value should not change.
config STM_BINARY_FILE string "STM binary file" - default "3rdparty/blobs/cpu/intel/stm/stm.bin" + default "payloads/external/stm/STM/Stm/build/StmPkg/Core/stm.bin" + help + Location of the STM binary file. The default location is + where the file will be located when coreboot builds + the STM. + Old location is "3rdparty/blobs/cpu/intel/stm/stm.bin" + +config STM_HEAPSIZE + hex "stm_heapsize" + default 0x46000 + help + The STM_HEAPSIZE defines the heap space that is available + to the STM. The default size assumes a MSEG_SIZE of 0x100000. + For STM/PE this size should be a minimum of 0x246000. + +config STM_UART + hex "stm_uart" + default 0x3F8 + help + Defines the serial port for the STM to send its console + output to. + +config STM_CBMEM_CONSOLE + bool "STM cbmem console" + default n + depends on CONSOLE_CBMEM + help + Places the STM console output into the cbmem. + + +config STM_BUILD + string "stm_build" + default "debug" + help + The default (debug) build will generate runtime console output. + "release" will deactivate the console output. + +config STM_GIT_REPO + string "stm_git_repo" + default "https://review.coreboot.org/STM" + +config STM_GIT_BRANCH + string "stm_git_branch" + default "stmpe"
endmenu #STM
Hello build bot (Jenkins), ron minnich, Eugene Myers,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44686
to look at the new patch set (#3).
Change subject: security/intel/stm: Add options for STM build ......................................................................
security/intel/stm: Add options for STM build
This patch adds options that support building the STM as a part of the coreboot build. The option defaults are set so that the STM can be built for a majority of coreboot platforms.
Change-Id: I80ed7cbcb93468c5ff93d089d77742ce7b671a37 Signed-off-by: Eugene D Myers cedarhouse@comcast.net --- M src/security/intel/stm/Kconfig 1 file changed, 77 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/44686/3
Paul Menzel 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 3:
(5 comments)
I thought Kconfig help text is indented with two spaces.
https://review.coreboot.org/c/coreboot/+/44686/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44686/3//COMMIT_MSG@12 PS3, Line 12: Tested how?
https://review.coreboot.org/c/coreboot/+/44686/3/src/security/intel/stm/Kcon... File src/security/intel/stm/Kconfig:
https://review.coreboot.org/c/coreboot/+/44686/3/src/security/intel/stm/Kcon... PS3, Line 1: Unrelated change.
https://review.coreboot.org/c/coreboot/+/44686/3/src/security/intel/stm/Kcon... PS3, Line 33: Isn’t it indented by two spaces?
https://review.coreboot.org/c/coreboot/+/44686/3/src/security/intel/stm/Kcon... PS3, Line 37: To use STM/PE, a larger MSEG_SIZE is necessary. This can be Please add a blank line above.
https://review.coreboot.org/c/coreboot/+/44686/3/src/security/intel/stm/Kcon... PS3, Line 68: Handler handler
Hello build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi, Paul Menzel, Angel Pons, Patrick Rudolph, ron minnich, Eugene Myers,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44686
to look at the new patch set (#4).
Change subject: security/intel/stm: Add options for STM build ......................................................................
security/intel/stm: Add options for STM build
This patch adds options that support building the STM as a part of the coreboot build. The option defaults assume that these configuration options are set as follows:
IED_REGION_SIZE = 0x400000 SMM_RESERVED_SIZE = 0x200000 SMM_TSEG_SIZE = 0x800000
Change-Id: I80ed7cbcb93468c5ff93d089d77742ce7b671a37 Signed-off-by: Eugene D Myers cedarhouse@comcast.net --- M src/security/intel/stm/Kconfig A src/security/intel/stm/Makefile M src/security/intel/stm/Makefile.inc 3 files changed, 131 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/44686/4
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 4:
(5 comments)
Updates are in patch set 4
https://review.coreboot.org/c/coreboot/+/44686/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44686/3//COMMIT_MSG@12 PS3, Line 12:
Tested how?
Text changed to indicate the conditions that the default settings assume.
https://review.coreboot.org/c/coreboot/+/44686/3/src/security/intel/stm/Kcon... File src/security/intel/stm/Kconfig:
https://review.coreboot.org/c/coreboot/+/44686/3/src/security/intel/stm/Kcon... PS3, Line 1:
Unrelated change.
Done
https://review.coreboot.org/c/coreboot/+/44686/3/src/security/intel/stm/Kcon... PS3, Line 33:
Isn’t it indented by two spaces?
Done
https://review.coreboot.org/c/coreboot/+/44686/3/src/security/intel/stm/Kcon... PS3, Line 37: To use STM/PE, a larger MSEG_SIZE is necessary. This can be
Please add a blank line above.
Done
https://review.coreboot.org/c/coreboot/+/44686/3/src/security/intel/stm/Kcon... PS3, Line 68: Handler
handler
Done
Hello build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi, Paul Menzel, Angel Pons, Patrick Rudolph, ron minnich, Eugene Myers,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44686
to look at the new patch set (#5).
Change subject: security/intel/stm: Add options for STM build ......................................................................
security/intel/stm: Add options for STM build
This patch adds options that support building the STM as a part of the coreboot build. The option defaults assume that these configuration options are set as follows:
IED_REGION_SIZE = 0x400000 SMM_RESERVED_SIZE = 0x200000 SMM_TSEG_SIZE = 0x800000
Change-Id: I80ed7cbcb93468c5ff93d089d77742ce7b671a37 Signed-off-by: Eugene D Myers cedarhouse@comcast.net --- M src/security/intel/stm/Kconfig A src/security/intel/stm/Makefile M src/security/intel/stm/Makefile.inc 3 files changed, 130 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/44686/5
Hello build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi, Paul Menzel, Angel Pons, Patrick Rudolph, ron minnich, Eugene Myers,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44686
to look at the new patch set (#6).
Change subject: security/intel/stm: Add options for STM build ......................................................................
security/intel/stm: Add options for STM build
This patch adds options that support building the STM as a part of the coreboot build. The option defaults assume that these configuration options are set as follows:
IED_REGION_SIZE = 0x400000 SMM_RESERVED_SIZE = 0x200000 SMM_TSEG_SIZE = 0x800000
Change-Id: I80ed7cbcb93468c5ff93d089d77742ce7b671a37 Signed-off-by: Eugene D Myers cedarhouse@comcast.net --- M src/security/intel/stm/Kconfig A src/security/intel/stm/Makefile M src/security/intel/stm/Makefile.inc 3 files changed, 128 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/44686/6
Hello build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi, Paul Menzel, Angel Pons, Patrick Rudolph, ron minnich, Eugene Myers,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44686
to look at the new patch set (#7).
Change subject: security/intel/stm: Add options for STM build ......................................................................
security/intel/stm: Add options for STM build
This patch adds options that support building the STM as a part of the coreboot build. The option defaults assume that these configuration options are set as follows:
IED_REGION_SIZE = 0x400000 SMM_RESERVED_SIZE = 0x200000 SMM_TSEG_SIZE = 0x800000
Change-Id: I80ed7cbcb93468c5ff93d089d77742ce7b671a37 Signed-off-by: Eugene D Myers cedarhouse@comcast.net --- M src/security/intel/stm/Kconfig A src/security/intel/stm/Makefile M src/security/intel/stm/Makefile.inc 3 files changed, 127 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/44686/7
Hello build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi, Paul Menzel, Angel Pons, Patrick Rudolph, ron minnich, Eugene Myers,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44686
to look at the new patch set (#8).
Change subject: security/intel/stm: Add options for STM build ......................................................................
security/intel/stm: Add options for STM build
This patch adds options that support building the STM as a part of the coreboot build. The option defaults assume that these configuration options are set as follows:
IED_REGION_SIZE = 0x400000 SMM_RESERVED_SIZE = 0x200000 SMM_TSEG_SIZE = 0x800000
Change-Id: I80ed7cbcb93468c5ff93d089d77742ce7b671a37 Signed-off-by: Eugene D Myers cedarhouse@comcast.net --- M src/security/intel/stm/Kconfig A src/security/intel/stm/Makefile M src/security/intel/stm/Makefile.inc 3 files changed, 117 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/44686/8
Hello build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi, Paul Menzel, Angel Pons, Patrick Rudolph, ron minnich, Eugene Myers,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44686
to look at the new patch set (#9).
Change subject: security/intel/stm: Add options for STM build ......................................................................
security/intel/stm: Add options for STM build
This patch adds options that support building the STM as a part of the coreboot build. The option defaults assume that these configuration options are set as follows:
IED_REGION_SIZE = 0x400000 SMM_RESERVED_SIZE = 0x200000 SMM_TSEG_SIZE = 0x800000
Change-Id: I80ed7cbcb93468c5ff93d089d77742ce7b671a37 Signed-off-by: Eugene D Myers cedarhouse@comcast.net
NOTE: The "Build Unstable" result from Jenkins is the result of a newer version of the loader complaining about the overlapping of tables in the nonstandard image. Currently, working the issue. Change-Id: I80ed7cbcb93468c5ff93d089d77742ce7b671a37 --- M src/security/intel/stm/Kconfig A src/security/intel/stm/Makefile M src/security/intel/stm/Makefile.inc 3 files changed, 117 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/44686/9
Hello build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi, Paul Menzel, Angel Pons, Patrick Rudolph, ron minnich, Eugene Myers,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44686
to look at the new patch set (#10).
Change subject: security/intel/stm: Add options for STM build ......................................................................
security/intel/stm: Add options for STM build
This patch adds options that support building the STM as a part of the coreboot build. The option defaults assume that these configuration options are set as follows:
IED_REGION_SIZE = 0x400000 SMM_RESERVED_SIZE = 0x200000 SMM_TSEG_SIZE = 0x800000
Change-Id: I80ed7cbcb93468c5ff93d089d77742ce7b671a37 Signed-off-by: Eugene D Myers cedarhouse@comcast.net --- M src/security/intel/stm/Kconfig A src/security/intel/stm/Makefile M src/security/intel/stm/Makefile.inc 3 files changed, 117 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/44686/10
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
Hello build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi, Paul Menzel, Angel Pons, Patrick Rudolph, ron minnich, Eugene Myers,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44686
to look at the new patch set (#11).
Change subject: security/intel/stm: Add options for STM build ......................................................................
security/intel/stm: Add options for STM build
This patch adds options that support building the STM as a part of the coreboot build. The option defaults assume that these configuration options are set as follows:
IED_REGION_SIZE = 0x400000 SMM_RESERVED_SIZE = 0x200000 SMM_TSEG_SIZE = 0x800000
Change-Id: I80ed7cbcb93468c5ff93d089d77742ce7b671a37 Signed-off-by: Eugene D Myers cedarhouse@comcast.net --- M src/security/intel/stm/Kconfig A src/security/intel/stm/Makefile M src/security/intel/stm/Makefile.inc 3 files changed, 130 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/44686/11
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.
ron minnich 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 12: Code-Review+2
With the proviso that I have no way to test, this looks good.
I do wish we did not rely on cmake, is that absolutely necessary?
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 12: Code-Review+1
(4 comments)
https://review.coreboot.org/c/coreboot/+/44686/12/src/security/intel/stm/Kco... File src/security/intel/stm/Kconfig:
https://review.coreboot.org/c/coreboot/+/44686/12/src/security/intel/stm/Kco... PS12, Line 93: indicatea indicate*s*
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; \
Not quite sure what you mean here. I did remove the 'rm -rf build' as the cmake causes a rebuild. […]
See comment in Makefile.inc
https://review.coreboot.org/c/coreboot/+/44686/12/src/security/intel/stm/Mak... File src/security/intel/stm/Makefile:
https://review.coreboot.org/c/coreboot/+/44686/12/src/security/intel/stm/Mak... PS12, Line 36: #rm -rf $(project_dir) This won't work for a submodule. Best approach would be to delegate clean/distclean to the STM Makefiles (and mask failures if already cleaned)
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 21: 3rdparty/stm/Stm/build/StmPkg/Core/stm.bin
The issue here is that if the developer changes any of the STM parameters, then the build will not g […]
Making `3rdparty/stm/Stm/build/StmPkg/Core/stm.bin` depend on `$(objutil)/kconfig/conf` should be good enough. This is the same dependency that `$(obj)/config.h` uses.
Hello build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi, Paul Menzel, Angel Pons, Patrick Rudolph, ron minnich, Eugene Myers,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44686
to look at the new patch set (#13).
Change subject: security/intel/stm: Add options for STM build ......................................................................
security/intel/stm: Add options for STM build
This patch adds options that support building the STM as a part of the coreboot build. The option defaults assume that these configuration options are set as follows:
IED_REGION_SIZE = 0x400000 SMM_RESERVED_SIZE = 0x200000 SMM_TSEG_SIZE = 0x800000
Change-Id: I80ed7cbcb93468c5ff93d089d77742ce7b671a37 Signed-off-by: Eugene Myers cedarhouse@comcast.net --- M src/security/intel/stm/Kconfig A src/security/intel/stm/Makefile M src/security/intel/stm/Makefile.inc 3 files changed, 130 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/44686/13
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 14:
This change is ready for review.
ron minnich 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 14: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44686 )
Change subject: security/intel/stm: Add options for STM build ......................................................................
security/intel/stm: Add options for STM build
This patch adds options that support building the STM as a part of the coreboot build. The option defaults assume that these configuration options are set as follows:
IED_REGION_SIZE = 0x400000 SMM_RESERVED_SIZE = 0x200000 SMM_TSEG_SIZE = 0x800000
Change-Id: I80ed7cbcb93468c5ff93d089d77742ce7b671a37 Signed-off-by: Eugene Myers cedarhouse@comcast.net Reviewed-on: https://review.coreboot.org/c/coreboot/+/44686 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: ron minnich rminnich@gmail.com --- M src/security/intel/stm/Kconfig A src/security/intel/stm/Makefile M src/security/intel/stm/Makefile.inc 3 files changed, 123 insertions(+), 7 deletions(-)
Approvals: build bot (Jenkins): Verified ron minnich: Looks good to me, approved
diff --git a/src/security/intel/stm/Kconfig b/src/security/intel/stm/Kconfig index f7dd363..5286354 100644 --- a/src/security/intel/stm/Kconfig +++ b/src/security/intel/stm/Kconfig @@ -27,20 +27,93 @@
config MSEG_SIZE hex "mseg size" - default 0x400000 + default 0x100000 help - STM only - 0x100000 - STM/PE - 0x300000+ depending on the amount of memory needed - for the protected execution virtual - machine (VM/PE) + The MSEG_SIZE of 0x100000 assumes that: + IED_REGION_SIZE = 0x400000 + SMM_RESERVED_SIZE = 0x200000 + SMM_TSEG_SIZE = 0x800000 + + To use STM/PE, a larger MSEG_SIZE is necessary. This can be + done by either increasing SMM_TSEG_SIZE or reducing the + IED_REGION_SIZE and/or SMM_RESERVED_SIZE or some combination + of the three. + NOTE: The authors experience is that these configuration + parameters have to be changed at the soc Konfig for them to + be applied. + Minimum sizes: + STM only - 0x100000 - Supports up to 38 processor threads + - 0x200000 - Supports up to 102 processor threads + STM/PE - 0x300000+ depending on the amount of memory needed + for the protected execution virtual + machine (VM/PE) + +config STM_STMPE_ENABLED + bool "STM/PE Enabled" + default n + help + STM/PE provides for additional virtual machines in SMRAM + that provides a protected execution environment for + applications such as introspection, which need to be + protected from malicious code. More information can be + found on the stmpe branch of + https://review.coreboot.org/STM +
config BIOS_RESOURCE_LIST_SIZE - hex "bios_resource_list_size" + hex "bios resource list size" default 0x1000 + help + The BIOS resource list defines the resources that the + SMI handler needs. This list is created during the + coreboot bootup. Unless there has been a lot of elements + added to this list, this value should not change.
config STM_BINARY_FILE string "STM binary file" - default "3rdparty/blobs/cpu/intel/stm/stm.bin" + default "3rdparty/stm/Stm/build/StmPkg/Core/stm.bin" + help + Location of the STM binary file. The default location is + where the file will be located when coreboot builds + the STM. + +config STM_HEAPSIZE + hex "stm heapsize" + default 0x46000 + help + The STM_HEAPSIZE defines the heap space that is available + to the STM. The default size assumes a MSEG_SIZE of 0x100000. + For STM/PE this size should be a minimum of 0x246000. + +config STM_TTYS0_BASE + hex "stm uart" + default TTYS0_BASE if TTYS0_BASE + default 0x000 + help + Defines the serial port for STM console output. 0x000 indicates + no serial port. + +config STM_CBMEM_CONSOLE + bool "STM cbmem console" + default n + depends on CONSOLE_CBMEM + help + Places the STM console output into the cbmem. + +choice + prompt "Select STM console output" + +config STM_CONSOLE_DEBUG + bool "Debug output" + depends on STM_CBMEM_CONSOLE || STM_TTYS0_BASE + help + "Produces all STM console output" + +config STM_CONSOLE_RELEASE + bool "Deactivate console output" + help + "No console output is produced" +endchoice
endmenu #STM
diff --git a/src/security/intel/stm/Makefile b/src/security/intel/stm/Makefile new file mode 100644 index 0000000..1493869 --- /dev/null +++ b/src/security/intel/stm/Makefile @@ -0,0 +1,33 @@ +# SPDX-License-Identifier: BSD-2-Clause + +project_name=STM +project_dir=../../../../3rdparty/stm/ +build_dir=$(project_dir)/Stm/build +project_git_branch=$(CONFIG_STM_GIT_BRANCH) + +ifeq ($(CONFIG_STM_CONSOLE_DEBUG),y) +STM_BUILD="debug" +endif + +ifeq ($(CONFIG_STM_CONSOLE_RELEASE),y) +STM_BUILD="release" +endif + + +all: build + +build: + echo "STM - Build" + cd $(project_dir)/Stm; \ + mkdir -p build; \ + cd build; \ + cmake .. -DBIOS=coreboot \ + -DUART=$(CONFIG_STM_TTYS0_BASE) \ + -DHEAPSIZE=$(CONFIG_STM_HEAPSIZE) \ + -DCBMEM_ENABLE=$(CONFIG_STM_CBMEM_CONSOLE) \ + -DSTMPE_ENABLED=$(CONFIG_STM_STMPE_ENABLED) \ + -DBUILD=$(STM_BUILD); \ + $(MAKE); + + +.PHONY: build diff --git a/src/security/intel/stm/Makefile.inc b/src/security/intel/stm/Makefile.inc index 1a23fe9..3f5b9ee 100644 --- a/src/security/intel/stm/Makefile.inc +++ b/src/security/intel/stm/Makefile.inc @@ -8,3 +8,13 @@ ramstage-$(CONFIG_STM) += SmmStm.c ramstage-$(CONFIG_STM) += StmPlatformSmm.c ramstage-$(CONFIG_STM) += StmPlatformResource.c + +3rdparty/stm/Stm/build/StmPkg/Core/stm.bin: $(obj)/config.h + $(MAKE) -C src/security/intel/stm \ + CONFIG_STM_TTYSO_BASE=$(CONFIG_STM_TTYSO_BASE) \ + CONFIG_STM_HEAPSIZE=$(CONFIG_STM_HEAPSIZE) \ + CONFIG_STM_CONSOLE_DEBUG=$(CONFIG_STM_CONSOLE_DEBUG) \ + CONFIG_STM_CONSOLE_RELEASE=$(CONFIG_STM_CONSOLE_RELEASE) \ + CONFIG_STM_GIT_BRANCH=$(CONFIG_STM_GIT_BRANCH) \ + CONFIG_STM_STMPE_ENABLED=$(CONFIG_STM_STMPE_ENABLED) \ + CONFIG_STM_CBMEM_CONSOLE=$(CONFIG_STM_CBMEM_CONSOLE)
-
Angel Pons (Code Review) -
Eugene Myers (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
ron minnich (Code Review)