Change in coreboot[master]: intel/stm: Add platform opt-in Kconfig

[Nico Huber (Code Review)]

Hello Kyösti Mälkki, Aaron Durbin, Arthur Heymans, [e-mail],

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/38764

to review the following change.

Change subject: intel/stm: Add platform opt-in Kconfig
......................................................................

intel/stm: Add platform opt-in Kconfig

Selecting STM on an arbitrary platform would likely result in a brick,
so let's hide the prompt by default.

Change-Id: I50f2106ac05c3efb7f92fccb1e6edfbf961b68b8
Signed-off-by: Nico Huber 
---
M src/security/intel/stm/Kconfig
M src/soc/intel/skylake/Kconfig
2 files changed, 5 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/38764/1

diff --git a/src/security/intel/stm/Kconfig b/src/security/intel/stm/Kconfig
index a74eba8..144deed 100644
--- a/src/security/intel/stm/Kconfig
+++ b/src/security/intel/stm/Kconfig
@@ -1,9 +1,12 @@

+config PLATFORM_SUPPORTS_STM
+	bool
+	depends on SMM_TSEG

config STM
    bool "Enable STM"
    default n
-	depends on SMM_TSEG
+	depends on PLATFORM_SUPPORTS_STM
    select USE_BLOBS

help
diff --git a/src/soc/intel/skylake/Kconfig b/src/soc/intel/skylake/Kconfig
index 6277cea..ae60a63 100644
--- a/src/soc/intel/skylake/Kconfig
+++ b/src/soc/intel/skylake/Kconfig
@@ -44,6 +44,7 @@
    select NO_FIXED_XIP_ROM_SIZE
    select PARALLEL_MP
    select PARALLEL_MP_AP_WORK
+	select PLATFORM_SUPPORTS_STM
    select PLATFORM_USES_FSP2_0
    select REG_SCRIPT
    select SA_ENABLE_DPR

[Name of user not set (Code Review)]

[e-mail] has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38764 )

Change subject: intel/stm: Add platform opt-in Kconfig
......................................................................

Patch Set 1: Code-Review+1

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38764 )

Change subject: intel/stm: Add platform opt-in Kconfig
......................................................................

Patch Set 1: Code-Review+2

Platform... Why don't we use that concept more often?

[Name of user not set (Code Review)]

[e-mail] has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38764 )

Change subject: intel/stm: Add platform opt-in Kconfig
......................................................................

Patch Set 1:

Need to also add this to the other Intel processor families Kconfig's as well.  Generally, if the processor supports VTX it supports the STM. My oldest platform running an STM is a Sandybridge.  And we were working on the family proceeding Sandybridge as well.

Also, I will also all an additional feature MSR (in stm_setup) check that indicates if that particular processor supports an STM.  In any case, if the STM setup is not successful, IA32_SMM_MONITOR_VALID does not get set in the IA32_SMM_MONITOR_CTL_MSR.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38764 )

Change subject: intel/stm: Add platform opt-in Kconfig
......................................................................

Patch Set 1:
> Need to also add this to the other Intel processor families Kconfig's as well.  Generally, if the processor supports VTX it supports the STM. My oldest platform running an STM is a Sandybridge.  And we were working on the family proceeding Sandybridge as well.

Right, maybe we should just figure out a good `default y` line for this
maybe `if PARALLEL_MP && INTEL` (we don't have `INTEL`, though).

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38764 )

Change subject: intel/stm: Add platform opt-in Kconfig
......................................................................

intel/stm: Add platform opt-in Kconfig

Selecting STM on an arbitrary platform would likely result in a brick,
so let's hide the prompt by default.

Change-Id: I50f2106ac05c3efb7f92fccb1e6edfbf961b68b8
Signed-off-by: Nico Huber 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/38764
Tested-by: build bot (Jenkins) 
Reviewed-by: 
Reviewed-by: Angel Pons 
---
M src/security/intel/stm/Kconfig
M src/soc/intel/skylake/Kconfig
2 files changed, 5 insertions(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved
  [e-mail]: Looks good to me, but someone else must approve

diff --git a/src/security/intel/stm/Kconfig b/src/security/intel/stm/Kconfig
index a74eba8..144deed 100644
--- a/src/security/intel/stm/Kconfig
+++ b/src/security/intel/stm/Kconfig
@@ -1,9 +1,12 @@

+config PLATFORM_SUPPORTS_STM
+	bool
+	depends on SMM_TSEG

config STM
    bool "Enable STM"
    default n
-	depends on SMM_TSEG
+	depends on PLATFORM_SUPPORTS_STM
    select USE_BLOBS

help
diff --git a/src/soc/intel/skylake/Kconfig b/src/soc/intel/skylake/Kconfig
index 6277cea..ae60a63 100644
--- a/src/soc/intel/skylake/Kconfig
+++ b/src/soc/intel/skylake/Kconfig
@@ -44,6 +44,7 @@
    select NO_FIXED_XIP_ROM_SIZE
    select PARALLEL_MP
    select PARALLEL_MP_AP_WORK
+	select PLATFORM_SUPPORTS_STM
    select PLATFORM_USES_FSP2_0
    select REG_SCRIPT
    select SA_ENABLE_DPR

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38764 )

Change subject: intel/stm: Add platform opt-in Kconfig
......................................................................

Patch Set 2:

Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3
Emulation targets:
EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/526
EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/525
EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/524

Please note: This test is under development and might not be accurate at all!