Philipp Deppenwiese has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
soc/intel/common/block/sgx: Fix crash in MP init
On Hyper-Threading enabled platforms the MSR_PRMRR_PHYS_MASK was written when already locked by the sibling thread. In addition it loads microcode updates on all threads.
To prevent such race conditions only call the code on one thread, such that the MSRs are only written once per core and the microcode is only loaded once for each core.
Also add comments that describe the scope of the MSR that is being written to and mention the Intel documents used for reference.
Fixes crash in SGX MP init. Tested on Supermicro X11SSH-TF.
Change-Id: I7102da028a449c60ca700b3f9ccda9017aa6d6b5 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35312 Reviewed-by: Christian Walter christian.walter@9elements.com Reviewed-by: Philipp Deppenwiese zaolin.daisuki@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M Documentation/mainboard/supermicro/x11-lga1151-series/x11-lga1151-series.md M src/mainboard/supermicro/x11-lga1151-series/devicetree.cb M src/soc/intel/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c 4 files changed, 52 insertions(+), 9 deletions(-)
Approvals: build bot (Jenkins): Verified Philipp Deppenwiese: Looks good to me, approved Christian Walter: Looks good to me, but someone else must approve
diff --git a/Documentation/mainboard/supermicro/x11-lga1151-series/x11-lga1151-series.md b/Documentation/mainboard/supermicro/x11-lga1151-series/x11-lga1151-series.md index 6a613e7..8c99527 100644 --- a/Documentation/mainboard/supermicro/x11-lga1151-series/x11-lga1151-series.md +++ b/Documentation/mainboard/supermicro/x11-lga1151-series/x11-lga1151-series.md @@ -29,7 +29,6 @@
These issues apply to all boards. Have a look at the board-specific issues, too.
-- Intel SGX causes secondary APs to crash (disabled for now) when HT is enabled (Fix is WIP CB:35312) - TianoCore doesn't work with Aspeed NGI, as it's text mode only (Fix is WIP CB:35726)
## ToDo diff --git a/src/mainboard/supermicro/x11-lga1151-series/devicetree.cb b/src/mainboard/supermicro/x11-lga1151-series/devicetree.cb index a5ff0c5..ee6aac7 100644 --- a/src/mainboard/supermicro/x11-lga1151-series/devicetree.cb +++ b/src/mainboard/supermicro/x11-lga1151-series/devicetree.cb @@ -17,8 +17,8 @@ register "Device4Enable" = "1" register "SaGv" = "SaGv_Disabled"
- # Disable SGX - register "sgx_enable" = "0" # SGX is broken in coreboot + # Enable SGX + register "sgx_enable" = "1" register "PrmrrSize" = "128 * MiB"
register "pirqa_routing" = "PCH_IRQ11" diff --git a/src/soc/intel/common/block/sgx/Kconfig b/src/soc/intel/common/block/sgx/Kconfig index 0852bfb..ffd501a 100644 --- a/src/soc/intel/common/block/sgx/Kconfig +++ b/src/soc/intel/common/block/sgx/Kconfig @@ -1,5 +1,7 @@ config SOC_INTEL_COMMON_BLOCK_SGX bool + select CPU_INTEL_COMMON + select CPU_INTEL_COMMON_HYPERTHREADING default n help Software Guard eXtension(SGX) Feature. Intel SGX is a set of new CPU diff --git a/src/soc/intel/common/block/sgx/sgx.c b/src/soc/intel/common/block/sgx/sgx.c index 0edf50f..377a719 100644 --- a/src/soc/intel/common/block/sgx/sgx.c +++ b/src/soc/intel/common/block/sgx/sgx.c @@ -17,6 +17,7 @@ #include <cpu/x86/msr.h> #include <cpu/x86/mtrr.h> #include <cpu/intel/microcode.h> +#include <cpu/intel/common/common.h> #include <intelblocks/mp_init.h> #include <intelblocks/msr.h> #include <intelblocks/sgx.h> @@ -54,9 +55,18 @@ } prmrr_base, prmrr_mask; msr_t msr;
- if (!is_sgx_supported()) + /* + * Software Developer's Manual Volume 4: + * Order Number: 335592-068US + * Chapter 2.16.1 + * MSR_PRMRR_PHYS_MASK is in scope "Core" + * MSR_PRMRR_PHYS_BASE is in scope "Core" + * Return if Hyper-Threading is enabled and not thread 0 + */ + if (!is_sgx_supported() || intel_ht_sibling()) return;
+ /* PRMRR_PHYS_MASK is in scope "Core" */ msr = rdmsr(MSR_PRMRR_PHYS_MASK); /* If it is locked don't attempt to write PRMRR MSRs. */ if (msr.lo & PRMRR_PHYS_MASK_LOCK) @@ -109,6 +119,12 @@ { msr_t msr;
+ /* + * Intel 64 and IA-32 ArchitecturesSoftware Developer's ManualVolume 3C + * Order Number: 326019-060US + * Chapter 35.10.2 "Additional MSRs Supported by Intel" + * IA32_FEATURE_CONTROL is in scope "Thread" + */ msr = rdmsr(IA32_FEATURE_CONTROL); /* Only enable it when it is not locked */ if ((msr.lo & FEATURE_CONTROL_LOCK_BIT) == 0) { @@ -121,6 +137,12 @@ { msr_t msr;
+ /* + * Intel 64 and IA-32 ArchitecturesSoftware Developer's ManualVolume 3C + * Order Number: 326019-060US + * Chapter 35.10.2 "Additional MSRs Supported by Intel" + * IA32_FEATURE_CONTROL is in scope "Thread" + */ msr = rdmsr(IA32_FEATURE_CONTROL); /* If it is locked don't attempt to lock it again. */ if ((msr.lo & 1) == 0) { @@ -135,6 +157,7 @@ * for PoC just write '0's to the MSRs. */ msr_t msr = {0, 0};
+ /* SGX_OWNEREPOCH is in scope "Package" */ wrmsr(MSR_SGX_OWNEREPOCH0, msr); wrmsr(MSR_SGX_OWNEREPOCH1, msr); return 0; @@ -175,16 +198,19 @@ return 0; }
+/* + * Configures SGX according to "Intel Software Guard Extensions Technology" + * Document Number: 565432 + */ void sgx_configure(void *unused) { - const void *microcode_patch = intel_mp_current_microcode();
if (!is_sgx_supported() || !is_prmrr_set()) { printk(BIOS_ERR, "SGX: pre-conditions not met\n"); return; }
- /* Enable the SGX feature */ + /* Enable the SGX feature on all threads. */ enable_sgx();
/* Update the owner epoch value */ @@ -194,10 +220,26 @@ /* Ensure to lock memory before reload microcode patch */ cpu_lock_sgx_memory();
- /* Reload the microcode patch */ - intel_microcode_load_unlocked(microcode_patch); + /* + * Update just on the first CPU in the core. Other siblings + * get the update automatically according to Document: 253668-060US + * Intel SDM Chapter 9.11.6.3 + * "Update in a System Supporting Intel Hyper-Threading Technology" + * Intel Hyper-Threading Technology has implications on the loading of the + * microcode update. The update must be loaded for each core in a physical + * processor. Thus, for a processor supporting Intel Hyper-Threading + * Technology, only one logical processor per core is required to load the + * microcode update. Each individual logical processor can independently + * load the update. However, MP initialization must provide some mechanism + * (e.g. a software semaphore) to force serialization of microcode update + * loads and to prevent simultaneous load attempts to the same core. + */ + if (!intel_ht_sibling()) { + const void *microcode_patch = intel_mp_current_microcode(); + intel_microcode_load_unlocked(microcode_patch); + }
- /* Lock the SGX feature */ + /* Lock the SGX feature on all threads. */ lock_sgx();
/* Activate the SGX feature, if PRMRR config was approved by MCHECK */