From 05bad430b65ca626c8e819cdeda4ffe2a9b6feb3 Mon Sep 17 00:00:00 2001 From: Patrick Rudolph Date: Thu, 26 Sep 2019 10:30:22 +0200 Subject: 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 Reviewed-on: https://review.coreboot.org/c/coreboot/+/35312 Reviewed-by: Christian Walter Reviewed-by: Philipp Deppenwiese Tested-by: build bot (Jenkins) --- .../x11-lga1151-series/x11-lga1151-series.md | 1 - .../supermicro/x11-lga1151-series/devicetree.cb | 4 +- src/soc/intel/common/block/sgx/Kconfig | 2 + src/soc/intel/common/block/sgx/sgx.c | 54 +++++++++++++++++++--- 4 files changed, 52 insertions(+), 9 deletions(-) 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 6a613e77d1..8c99527319 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 @@ Look at the [flashing tutorial] and the board-specific section. 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 a5ff0c5df0..ee6aac7e17 100644 --- a/src/mainboard/supermicro/x11-lga1151-series/devicetree.cb +++ b/src/mainboard/supermicro/x11-lga1151-series/devicetree.cb @@ -17,8 +17,8 @@ chip soc/intel/skylake 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 0852bfbf49..ffd501a7e2 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 0edf50f09b..377a71994f 100644 --- a/src/soc/intel/common/block/sgx/sgx.c +++ b/src/soc/intel/common/block/sgx/sgx.c @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -54,9 +55,18 @@ void prmrr_core_configure(void) } 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 @@ static void enable_sgx(void) { 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 @@ static void lock_sgx(void) { 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 @@ static int owner_epoch_update(void) * 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 @@ static int is_prmrr_approved(void) 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 @@ void sgx_configure(void *unused) /* 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 */ -- cgit v1.2.3