<p>Patrick Georgi <strong>merged</strong> this change.</p><p><a href="https://review.coreboot.org/c/coreboot/+/30229">View Change</a></p><div style="white-space:pre-wrap">Approvals:
build bot (Jenkins): Verified
Nico Huber: Looks good to me, approved
David Guckian: Looks good to me, but someone else must approve
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">cpu/intel/common: decouple IA32_FEATURE_CONTROL lock from set_vmx()<br><br>Newer CPUs/SoCs need to configure other features via the<br>IA32_FEATURE_CONTROL msr, such as SGX, which cannot be done if the<br>msr is already locked. Create separate functions for setting the<br>vmx flag and lock bit, and rename existing function to indicate that<br>the lock bit will be set in addition to vmx flag (per Kconfig).<br><br>This will allow Skylake/Kabylake (and others?) to use the common<br>VMX code without breaking SGX, while ensuring no change in functionality<br>to existing platforms which current set both together.<br><br>Test: build/boot each affected platform, ensure no change in functionality<br><br>Change-Id: Iee772fe87306b4729ca012cef8640d3858e2cb06<br>Signed-off-by: Matt DeVillier <matt.devillier@gmail.com><br>Reviewed-on: https://review.coreboot.org/c/30229<br>Reviewed-by: Nico Huber <nico.h@gmx.de><br>Reviewed-by: David Guckian<br>Tested-by: build bot (Jenkins) <no-reply@coreboot.org><br>---<br>M src/cpu/intel/common/Kconfig<br>M src/cpu/intel/common/common.h<br>M src/cpu/intel/common/common_init.c<br>M src/cpu/intel/fsp_model_406dx/model_406dx_init.c<br>M src/cpu/intel/haswell/haswell_init.c<br>M src/cpu/intel/model_1067x/model_1067x_init.c<br>M src/cpu/intel/model_106cx/model_106cx_init.c<br>M src/cpu/intel/model_2065x/model_2065x_init.c<br>M src/cpu/intel/model_206ax/model_206ax_init.c<br>M src/cpu/intel/model_6ex/model_6ex_init.c<br>M src/cpu/intel/model_6fx/model_6fx_init.c<br>M src/soc/intel/baytrail/cpu.c<br>M src/soc/intel/braswell/cpu.c<br>M src/soc/intel/broadwell/cpu.c<br>14 files changed, 46 insertions(+), 22 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/cpu/intel/common/Kconfig b/src/cpu/intel/common/Kconfig</span><br><span>index 739333e..56bed22 100644</span><br><span>--- a/src/cpu/intel/common/Kconfig</span><br><span>+++ b/src/cpu/intel/common/Kconfig</span><br><span>@@ -7,9 +7,8 @@</span><br><span> bool "Enable VMX for virtualization"</span><br><span> default y</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-config SET_VMX_LOCK_BIT</span><br><span style="color: hsl(0, 100%, 40%);">- bool "Set lock bit after configuring VMX"</span><br><span style="color: hsl(0, 100%, 40%);">- depends on ENABLE_VMX</span><br><span style="color: hsl(120, 100%, 40%);">+config SET_IA32_FC_LOCK_BIT</span><br><span style="color: hsl(120, 100%, 40%);">+ bool "Set IA32_FEATURE_CONTROL lock bit"</span><br><span> default y</span><br><span> help</span><br><span> Although the Intel manual says you must set the lock bit in addition</span><br><span>diff --git a/src/cpu/intel/common/common.h b/src/cpu/intel/common/common.h</span><br><span>index 81c9f16..b9ac056 100644</span><br><span>--- a/src/cpu/intel/common/common.h</span><br><span>+++ b/src/cpu/intel/common/common.h</span><br><span>@@ -15,7 +15,9 @@</span><br><span> #ifndef _CPU_INTEL_COMMON_H</span><br><span> #define _CPU_INTEL_COMMON_H</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-void set_vmx(void);</span><br><span style="color: hsl(120, 100%, 40%);">+void set_vmx_and_lock(void);</span><br><span style="color: hsl(120, 100%, 40%);">+void set_feature_ctrl_vmx(void);</span><br><span style="color: hsl(120, 100%, 40%);">+void set_feature_ctrl_lock(void);</span><br><span> </span><br><span> /*</span><br><span> * Init CPPC block with MSRs for Intel Enhanced Speed Step Technology.</span><br><span>diff --git a/src/cpu/intel/common/common_init.c b/src/cpu/intel/common/common_init.c</span><br><span>index 7dbbfda..9c0fcbb 100644</span><br><span>--- a/src/cpu/intel/common/common_init.c</span><br><span>+++ b/src/cpu/intel/common/common_init.c</span><br><span>@@ -21,12 +21,17 @@</span><br><span> #include <cpu/x86/msr.h></span><br><span> #include "common.h"</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-void set_vmx(void)</span><br><span style="color: hsl(120, 100%, 40%);">+void set_vmx_and_lock(void)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ set_feature_ctrl_vmx();</span><br><span style="color: hsl(120, 100%, 40%);">+ set_feature_ctrl_lock();</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+void set_feature_ctrl_vmx(void)</span><br><span> {</span><br><span> msr_t msr;</span><br><span> uint32_t feature_flag;</span><br><span> int enable = IS_ENABLED(CONFIG_ENABLE_VMX);</span><br><span style="color: hsl(0, 100%, 40%);">- int lock = IS_ENABLED(CONFIG_SET_VMX_LOCK_BIT);</span><br><span> </span><br><span> feature_flag = cpu_get_feature_flags_ecx();</span><br><span> /* Check that the VMX is supported before reading or writing the MSR. */</span><br><span>@@ -38,10 +43,10 @@</span><br><span> msr = rdmsr(IA32_FEATURE_CONTROL);</span><br><span> </span><br><span> if (msr.lo & (1 << 0)) {</span><br><span style="color: hsl(0, 100%, 40%);">- printk(BIOS_ERR, "VMX is locked, so %s will do nothing\n",</span><br><span style="color: hsl(120, 100%, 40%);">+ printk(BIOS_ERR, "IA32_FEATURE_CONTROL is locked, so %s will do nothing\n",</span><br><span> __func__);</span><br><span style="color: hsl(0, 100%, 40%);">- /* VMX locked. If we set it again we get an illegal</span><br><span style="color: hsl(0, 100%, 40%);">- * instruction</span><br><span style="color: hsl(120, 100%, 40%);">+ /* IA32_FEATURE_CONTROL locked. If we set it again we get an</span><br><span style="color: hsl(120, 100%, 40%);">+ * illegal instruction</span><br><span> */</span><br><span> return;</span><br><span> }</span><br><span>@@ -59,14 +64,32 @@</span><br><span> </span><br><span> wrmsr(IA32_FEATURE_CONTROL, msr);</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+ printk(BIOS_DEBUG, "VMX status: %s\n",</span><br><span style="color: hsl(120, 100%, 40%);">+ enable ? "enabled" : "disabled");</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+void set_feature_ctrl_lock(void)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ msr_t msr;</span><br><span style="color: hsl(120, 100%, 40%);">+ int lock = IS_ENABLED(CONFIG_SET_IA32_FC_LOCK_BIT);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ msr = rdmsr(IA32_FEATURE_CONTROL);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ if (msr.lo & (1 << 0)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ printk(BIOS_ERR, "IA32_FEATURE_CONTROL is locked, so %s will do nothing\n",</span><br><span style="color: hsl(120, 100%, 40%);">+ __func__);</span><br><span style="color: hsl(120, 100%, 40%);">+ /* IA32_FEATURE_CONTROL locked. If we set it again we get an</span><br><span style="color: hsl(120, 100%, 40%);">+ * illegal instruction</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span style="color: hsl(120, 100%, 40%);">+ return;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> if (lock) {</span><br><span> /* Set lock bit */</span><br><span> msr.lo |= (1 << 0);</span><br><span> wrmsr(IA32_FEATURE_CONTROL, msr);</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- printk(BIOS_DEBUG, "VMX status: %s, %s\n",</span><br><span style="color: hsl(0, 100%, 40%);">- enable ? "enabled" : "disabled",</span><br><span style="color: hsl(120, 100%, 40%);">+ printk(BIOS_DEBUG, "IA32_FEATURE_CONTROL status: %s\n",</span><br><span> lock ? "locked" : "unlocked");</span><br><span> }</span><br><span> </span><br><span>diff --git a/src/cpu/intel/fsp_model_406dx/model_406dx_init.c b/src/cpu/intel/fsp_model_406dx/model_406dx_init.c</span><br><span>index 289305f..7994f0b 100644</span><br><span>--- a/src/cpu/intel/fsp_model_406dx/model_406dx_init.c</span><br><span>+++ b/src/cpu/intel/fsp_model_406dx/model_406dx_init.c</span><br><span>@@ -148,7 +148,7 @@</span><br><span> setup_lapic();</span><br><span> </span><br><span> /* Set virtualization based on Kconfig option */</span><br><span style="color: hsl(0, 100%, 40%);">- set_vmx();</span><br><span style="color: hsl(120, 100%, 40%);">+ set_vmx_and_lock();</span><br><span> </span><br><span> /* Configure Enhanced SpeedStep and Thermal Sensors */</span><br><span> configure_misc();</span><br><span>diff --git a/src/cpu/intel/haswell/haswell_init.c b/src/cpu/intel/haswell/haswell_init.c</span><br><span>index 9d85960..aa77964 100644</span><br><span>--- a/src/cpu/intel/haswell/haswell_init.c</span><br><span>+++ b/src/cpu/intel/haswell/haswell_init.c</span><br><span>@@ -686,7 +686,7 @@</span><br><span> setup_lapic();</span><br><span> </span><br><span> /* Set virtualization based on Kconfig option */</span><br><span style="color: hsl(0, 100%, 40%);">- set_vmx();</span><br><span style="color: hsl(120, 100%, 40%);">+ set_vmx_and_lock();</span><br><span> </span><br><span> /* Configure C States */</span><br><span> configure_c_states();</span><br><span>diff --git a/src/cpu/intel/model_1067x/model_1067x_init.c b/src/cpu/intel/model_1067x/model_1067x_init.c</span><br><span>index 7eb121e..dbb9631 100644</span><br><span>--- a/src/cpu/intel/model_1067x/model_1067x_init.c</span><br><span>+++ b/src/cpu/intel/model_1067x/model_1067x_init.c</span><br><span>@@ -296,7 +296,7 @@</span><br><span> init_timer();</span><br><span> </span><br><span> /* Set virtualization based on Kconfig option */</span><br><span style="color: hsl(0, 100%, 40%);">- set_vmx();</span><br><span style="color: hsl(120, 100%, 40%);">+ set_vmx_and_lock();</span><br><span> </span><br><span> /* Configure C States */</span><br><span> configure_c_states(quad);</span><br><span>diff --git a/src/cpu/intel/model_106cx/model_106cx_init.c b/src/cpu/intel/model_106cx/model_106cx_init.c</span><br><span>index 56598fa..a609aed 100644</span><br><span>--- a/src/cpu/intel/model_106cx/model_106cx_init.c</span><br><span>+++ b/src/cpu/intel/model_106cx/model_106cx_init.c</span><br><span>@@ -99,7 +99,7 @@</span><br><span> setup_lapic();</span><br><span> </span><br><span> /* Set virtualization based on Kconfig option */</span><br><span style="color: hsl(0, 100%, 40%);">- set_vmx();</span><br><span style="color: hsl(120, 100%, 40%);">+ set_vmx_and_lock();</span><br><span> </span><br><span> /* Configure C States */</span><br><span> configure_c_states();</span><br><span>diff --git a/src/cpu/intel/model_2065x/model_2065x_init.c b/src/cpu/intel/model_2065x/model_2065x_init.c</span><br><span>index 7ce516b..240bf50 100644</span><br><span>--- a/src/cpu/intel/model_2065x/model_2065x_init.c</span><br><span>+++ b/src/cpu/intel/model_2065x/model_2065x_init.c</span><br><span>@@ -333,7 +333,7 @@</span><br><span> setup_lapic();</span><br><span> </span><br><span> /* Set virtualization based on Kconfig option */</span><br><span style="color: hsl(0, 100%, 40%);">- set_vmx();</span><br><span style="color: hsl(120, 100%, 40%);">+ set_vmx_and_lock();</span><br><span> </span><br><span> /* Configure Enhanced SpeedStep and Thermal Sensors */</span><br><span> configure_misc();</span><br><span>diff --git a/src/cpu/intel/model_206ax/model_206ax_init.c b/src/cpu/intel/model_206ax/model_206ax_init.c</span><br><span>index 27f56be..58aabdb 100644</span><br><span>--- a/src/cpu/intel/model_206ax/model_206ax_init.c</span><br><span>+++ b/src/cpu/intel/model_206ax/model_206ax_init.c</span><br><span>@@ -556,7 +556,7 @@</span><br><span> setup_lapic();</span><br><span> </span><br><span> /* Set virtualization based on Kconfig option */</span><br><span style="color: hsl(0, 100%, 40%);">- set_vmx();</span><br><span style="color: hsl(120, 100%, 40%);">+ set_vmx_and_lock();</span><br><span> </span><br><span> /* Configure C States */</span><br><span> configure_c_states();</span><br><span>diff --git a/src/cpu/intel/model_6ex/model_6ex_init.c b/src/cpu/intel/model_6ex/model_6ex_init.c</span><br><span>index 5041dcd..78ece74 100644</span><br><span>--- a/src/cpu/intel/model_6ex/model_6ex_init.c</span><br><span>+++ b/src/cpu/intel/model_6ex/model_6ex_init.c</span><br><span>@@ -136,7 +136,7 @@</span><br><span> setup_lapic();</span><br><span> </span><br><span> /* Set virtualization based on Kconfig option */</span><br><span style="color: hsl(0, 100%, 40%);">- set_vmx();</span><br><span style="color: hsl(120, 100%, 40%);">+ set_vmx_and_lock();</span><br><span> </span><br><span> /* Configure C States */</span><br><span> configure_c_states();</span><br><span>diff --git a/src/cpu/intel/model_6fx/model_6fx_init.c b/src/cpu/intel/model_6fx/model_6fx_init.c</span><br><span>index c5659f3..9d11478 100644</span><br><span>--- a/src/cpu/intel/model_6fx/model_6fx_init.c</span><br><span>+++ b/src/cpu/intel/model_6fx/model_6fx_init.c</span><br><span>@@ -150,7 +150,7 @@</span><br><span> setup_lapic();</span><br><span> </span><br><span> /* Set virtualization based on Kconfig option */</span><br><span style="color: hsl(0, 100%, 40%);">- set_vmx();</span><br><span style="color: hsl(120, 100%, 40%);">+ set_vmx_and_lock();</span><br><span> </span><br><span> /* Configure C States */</span><br><span> configure_c_states();</span><br><span>diff --git a/src/soc/intel/baytrail/cpu.c b/src/soc/intel/baytrail/cpu.c</span><br><span>index 6c37aa86..9526932 100644</span><br><span>--- a/src/soc/intel/baytrail/cpu.c</span><br><span>+++ b/src/soc/intel/baytrail/cpu.c</span><br><span>@@ -57,7 +57,7 @@</span><br><span> enable_turbo();</span><br><span> </span><br><span> /* Set virtualization based on Kconfig option */</span><br><span style="color: hsl(0, 100%, 40%);">- set_vmx();</span><br><span style="color: hsl(120, 100%, 40%);">+ set_vmx_and_lock();</span><br><span> </span><br><span> /* Set core MSRs */</span><br><span> reg_script_run(core_msr_script);</span><br><span>diff --git a/src/soc/intel/braswell/cpu.c b/src/soc/intel/braswell/cpu.c</span><br><span>index 9063c2a..12f34fb 100644</span><br><span>--- a/src/soc/intel/braswell/cpu.c</span><br><span>+++ b/src/soc/intel/braswell/cpu.c</span><br><span>@@ -62,7 +62,7 @@</span><br><span> enable_turbo();</span><br><span> </span><br><span> /* Set virtualization based on Kconfig option */</span><br><span style="color: hsl(0, 100%, 40%);">- set_vmx();</span><br><span style="color: hsl(120, 100%, 40%);">+ set_vmx_and_lock();</span><br><span> </span><br><span> /* Set core MSRs */</span><br><span> reg_script_run(core_msr_script);</span><br><span>diff --git a/src/soc/intel/broadwell/cpu.c b/src/soc/intel/broadwell/cpu.c</span><br><span>index 66ec345..eab6b00 100644</span><br><span>--- a/src/soc/intel/broadwell/cpu.c</span><br><span>+++ b/src/soc/intel/broadwell/cpu.c</span><br><span>@@ -581,7 +581,7 @@</span><br><span> setup_lapic();</span><br><span> </span><br><span> /* Set virtualization based on Kconfig option */</span><br><span style="color: hsl(0, 100%, 40%);">- set_vmx();</span><br><span style="color: hsl(120, 100%, 40%);">+ set_vmx_and_lock();</span><br><span> </span><br><span> /* Configure C States */</span><br><span> configure_c_states();</span><br><span></span><br></pre><p>To view, visit <a href="https://review.coreboot.org/c/coreboot/+/30229">change 30229</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://review.coreboot.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://review.coreboot.org/c/coreboot/+/30229"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: coreboot </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Iee772fe87306b4729ca012cef8640d3858e2cb06 </div>
<div style="display:none"> Gerrit-Change-Number: 30229 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: Matt DeVillier <matt.devillier@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: David Guckian </div>
<div style="display:none"> Gerrit-Reviewer: David Guckian <david.guckian@intel.com> </div>
<div style="display:none"> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> </div>
<div style="display:none"> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> </div>
<div style="display:none"> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> </div>
<div style="display:none"> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> </div>
<div style="display:none"> Gerrit-MessageType: merged </div>