From f9aed6578565593ff2b5d9e90f8e6e80e5d9831d Mon Sep 17 00:00:00 2001 From: Matt DeVillier Date: Sat, 15 Dec 2018 15:57:33 -0600 Subject: [PATCH] cpu/intel/common: decouple IA32_FEATURE_CONTROL lock from set_vmx() Newer CPUs/SoCs need to configure other features via the IA32_FEATURE_CONTROL msr, such as SGX, which cannot be done if the msr is already locked. Create separate functions for setting the vmx flag and lock bit, and rename existing function to indicate that the lock bit will be set in addition to vmx flag (per Kconfig). This will allow Skylake/Kabylake (and others?) to use the common VMX code without breaking SGX, while ensuring no change in functionality to existing platforms which current set both together. Test: build/boot each affected platform, ensure no change in functionality Change-Id: Iee772fe87306b4729ca012cef8640d3858e2cb06 Signed-off-by: Matt DeVillier Reviewed-on: https://review.coreboot.org/c/30229 Reviewed-by: Nico Huber Reviewed-by: David Guckian Tested-by: build bot (Jenkins) --- src/cpu/intel/common/Kconfig | 5 +-- src/cpu/intel/common/common.h | 4 +- src/cpu/intel/common/common_init.c | 37 +++++++++++++++---- .../intel/fsp_model_406dx/model_406dx_init.c | 2 +- src/cpu/intel/haswell/haswell_init.c | 2 +- src/cpu/intel/model_1067x/model_1067x_init.c | 2 +- src/cpu/intel/model_106cx/model_106cx_init.c | 2 +- src/cpu/intel/model_2065x/model_2065x_init.c | 2 +- src/cpu/intel/model_206ax/model_206ax_init.c | 2 +- src/cpu/intel/model_6ex/model_6ex_init.c | 2 +- src/cpu/intel/model_6fx/model_6fx_init.c | 2 +- src/soc/intel/baytrail/cpu.c | 2 +- src/soc/intel/braswell/cpu.c | 2 +- src/soc/intel/broadwell/cpu.c | 2 +- 14 files changed, 46 insertions(+), 22 deletions(-) diff --git a/src/cpu/intel/common/Kconfig b/src/cpu/intel/common/Kconfig index 739333e4aa..56bed22a1a 100644 --- a/src/cpu/intel/common/Kconfig +++ b/src/cpu/intel/common/Kconfig @@ -7,9 +7,8 @@ config ENABLE_VMX bool "Enable VMX for virtualization" default y -config SET_VMX_LOCK_BIT - bool "Set lock bit after configuring VMX" - depends on ENABLE_VMX +config SET_IA32_FC_LOCK_BIT + bool "Set IA32_FEATURE_CONTROL lock bit" default y help Although the Intel manual says you must set the lock bit in addition diff --git a/src/cpu/intel/common/common.h b/src/cpu/intel/common/common.h index 81c9f16d19..b9ac0566c6 100644 --- a/src/cpu/intel/common/common.h +++ b/src/cpu/intel/common/common.h @@ -15,7 +15,9 @@ #ifndef _CPU_INTEL_COMMON_H #define _CPU_INTEL_COMMON_H -void set_vmx(void); +void set_vmx_and_lock(void); +void set_feature_ctrl_vmx(void); +void set_feature_ctrl_lock(void); /* * Init CPPC block with MSRs for Intel Enhanced Speed Step Technology. diff --git a/src/cpu/intel/common/common_init.c b/src/cpu/intel/common/common_init.c index 7dbbfda2e8..9c0fcbb122 100644 --- a/src/cpu/intel/common/common_init.c +++ b/src/cpu/intel/common/common_init.c @@ -21,12 +21,17 @@ #include #include "common.h" -void set_vmx(void) +void set_vmx_and_lock(void) +{ + set_feature_ctrl_vmx(); + set_feature_ctrl_lock(); +} + +void set_feature_ctrl_vmx(void) { msr_t msr; uint32_t feature_flag; int enable = IS_ENABLED(CONFIG_ENABLE_VMX); - int lock = IS_ENABLED(CONFIG_SET_VMX_LOCK_BIT); feature_flag = cpu_get_feature_flags_ecx(); /* Check that the VMX is supported before reading or writing the MSR. */ @@ -38,10 +43,10 @@ void set_vmx(void) msr = rdmsr(IA32_FEATURE_CONTROL); if (msr.lo & (1 << 0)) { - printk(BIOS_ERR, "VMX is locked, so %s will do nothing\n", + printk(BIOS_ERR, "IA32_FEATURE_CONTROL is locked, so %s will do nothing\n", __func__); - /* VMX locked. If we set it again we get an illegal - * instruction + /* IA32_FEATURE_CONTROL locked. If we set it again we get an + * illegal instruction */ return; } @@ -59,14 +64,32 @@ void set_vmx(void) wrmsr(IA32_FEATURE_CONTROL, msr); + printk(BIOS_DEBUG, "VMX status: %s\n", + enable ? "enabled" : "disabled"); +} +void set_feature_ctrl_lock(void) +{ + msr_t msr; + int lock = IS_ENABLED(CONFIG_SET_IA32_FC_LOCK_BIT); + + msr = rdmsr(IA32_FEATURE_CONTROL); + + if (msr.lo & (1 << 0)) { + printk(BIOS_ERR, "IA32_FEATURE_CONTROL is locked, so %s will do nothing\n", + __func__); + /* IA32_FEATURE_CONTROL locked. If we set it again we get an + * illegal instruction + */ + return; + } + if (lock) { /* Set lock bit */ msr.lo |= (1 << 0); wrmsr(IA32_FEATURE_CONTROL, msr); } - printk(BIOS_DEBUG, "VMX status: %s, %s\n", - enable ? "enabled" : "disabled", + printk(BIOS_DEBUG, "IA32_FEATURE_CONTROL status: %s\n", lock ? "locked" : "unlocked"); } diff --git a/src/cpu/intel/fsp_model_406dx/model_406dx_init.c b/src/cpu/intel/fsp_model_406dx/model_406dx_init.c index 289305fcd3..7994f0bb2c 100644 --- a/src/cpu/intel/fsp_model_406dx/model_406dx_init.c +++ b/src/cpu/intel/fsp_model_406dx/model_406dx_init.c @@ -148,7 +148,7 @@ static void model_406dx_init(struct device *cpu) setup_lapic(); /* Set virtualization based on Kconfig option */ - set_vmx(); + set_vmx_and_lock(); /* Configure Enhanced SpeedStep and Thermal Sensors */ configure_misc(); diff --git a/src/cpu/intel/haswell/haswell_init.c b/src/cpu/intel/haswell/haswell_init.c index 9d859605a3..aa77964f68 100644 --- a/src/cpu/intel/haswell/haswell_init.c +++ b/src/cpu/intel/haswell/haswell_init.c @@ -686,7 +686,7 @@ static void haswell_init(struct device *cpu) setup_lapic(); /* Set virtualization based on Kconfig option */ - set_vmx(); + set_vmx_and_lock(); /* Configure C States */ configure_c_states(); diff --git a/src/cpu/intel/model_1067x/model_1067x_init.c b/src/cpu/intel/model_1067x/model_1067x_init.c index 7eb121effd..dbb9631ad8 100644 --- a/src/cpu/intel/model_1067x/model_1067x_init.c +++ b/src/cpu/intel/model_1067x/model_1067x_init.c @@ -296,7 +296,7 @@ static void model_1067x_init(struct device *cpu) init_timer(); /* Set virtualization based on Kconfig option */ - set_vmx(); + set_vmx_and_lock(); /* Configure C States */ configure_c_states(quad); diff --git a/src/cpu/intel/model_106cx/model_106cx_init.c b/src/cpu/intel/model_106cx/model_106cx_init.c index 56598fae82..a609aed550 100644 --- a/src/cpu/intel/model_106cx/model_106cx_init.c +++ b/src/cpu/intel/model_106cx/model_106cx_init.c @@ -99,7 +99,7 @@ static void model_106cx_init(struct device *cpu) setup_lapic(); /* Set virtualization based on Kconfig option */ - set_vmx(); + set_vmx_and_lock(); /* Configure C States */ configure_c_states(); diff --git a/src/cpu/intel/model_2065x/model_2065x_init.c b/src/cpu/intel/model_2065x/model_2065x_init.c index 7ce516bd19..240bf50737 100644 --- a/src/cpu/intel/model_2065x/model_2065x_init.c +++ b/src/cpu/intel/model_2065x/model_2065x_init.c @@ -333,7 +333,7 @@ static void model_2065x_init(struct device *cpu) setup_lapic(); /* Set virtualization based on Kconfig option */ - set_vmx(); + set_vmx_and_lock(); /* Configure Enhanced SpeedStep and Thermal Sensors */ configure_misc(); diff --git a/src/cpu/intel/model_206ax/model_206ax_init.c b/src/cpu/intel/model_206ax/model_206ax_init.c index 27f56be67d..58aabdb198 100644 --- a/src/cpu/intel/model_206ax/model_206ax_init.c +++ b/src/cpu/intel/model_206ax/model_206ax_init.c @@ -556,7 +556,7 @@ static void model_206ax_init(struct device *cpu) setup_lapic(); /* Set virtualization based on Kconfig option */ - set_vmx(); + set_vmx_and_lock(); /* Configure C States */ configure_c_states(); diff --git a/src/cpu/intel/model_6ex/model_6ex_init.c b/src/cpu/intel/model_6ex/model_6ex_init.c index 5041dcd51d..78ece74d66 100644 --- a/src/cpu/intel/model_6ex/model_6ex_init.c +++ b/src/cpu/intel/model_6ex/model_6ex_init.c @@ -136,7 +136,7 @@ static void model_6ex_init(struct device *cpu) setup_lapic(); /* Set virtualization based on Kconfig option */ - set_vmx(); + set_vmx_and_lock(); /* Configure C States */ configure_c_states(); diff --git a/src/cpu/intel/model_6fx/model_6fx_init.c b/src/cpu/intel/model_6fx/model_6fx_init.c index c5659f381f..9d11478dee 100644 --- a/src/cpu/intel/model_6fx/model_6fx_init.c +++ b/src/cpu/intel/model_6fx/model_6fx_init.c @@ -150,7 +150,7 @@ static void model_6fx_init(struct device *cpu) setup_lapic(); /* Set virtualization based on Kconfig option */ - set_vmx(); + set_vmx_and_lock(); /* Configure C States */ configure_c_states(); diff --git a/src/soc/intel/baytrail/cpu.c b/src/soc/intel/baytrail/cpu.c index 6c37aa86b9..952693217c 100644 --- a/src/soc/intel/baytrail/cpu.c +++ b/src/soc/intel/baytrail/cpu.c @@ -57,7 +57,7 @@ static void baytrail_core_init(struct device *cpu) enable_turbo(); /* Set virtualization based on Kconfig option */ - set_vmx(); + set_vmx_and_lock(); /* Set core MSRs */ reg_script_run(core_msr_script); diff --git a/src/soc/intel/braswell/cpu.c b/src/soc/intel/braswell/cpu.c index 9063c2a6ac..12f34fb2fe 100644 --- a/src/soc/intel/braswell/cpu.c +++ b/src/soc/intel/braswell/cpu.c @@ -62,7 +62,7 @@ static void soc_core_init(struct device *cpu) enable_turbo(); /* Set virtualization based on Kconfig option */ - set_vmx(); + set_vmx_and_lock(); /* Set core MSRs */ reg_script_run(core_msr_script); diff --git a/src/soc/intel/broadwell/cpu.c b/src/soc/intel/broadwell/cpu.c index 66ec345615..eab6b00734 100644 --- a/src/soc/intel/broadwell/cpu.c +++ b/src/soc/intel/broadwell/cpu.c @@ -581,7 +581,7 @@ static void cpu_core_init(struct device *cpu) setup_lapic(); /* Set virtualization based on Kconfig option */ - set_vmx(); + set_vmx_and_lock(); /* Configure C States */ configure_c_states();