| Message ID | 20191119031240.7779-10-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/cpu: Clean up handling of VMX features | expand |
On Mon, Nov 18, 2019 at 07:12:30PM -0800, Sean Christopherson wrote: > Now that the IA32_FEATURE_CONTROL MSR is guaranteed to be configured and > locked, clear the VMX capability flag if the IA32_FEATURE_CONTROL MSR is > not supported or if BIOS disabled VMX, i.e. locked IA32_FEATURE_CONTROL > and did not set the appropriate VMX enable bit. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/kernel/cpu/feature_control.c | 28 ++++++++++++++++++++++++--- > 1 file changed, 25 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kernel/cpu/feature_control.c b/arch/x86/kernel/cpu/feature_control.c > index 33c9444dda52..2bd1a9e6021a 100644 > --- a/arch/x86/kernel/cpu/feature_control.c > +++ b/arch/x86/kernel/cpu/feature_control.c > @@ -5,15 +5,26 @@ > #include <asm/msr-index.h> > #include <asm/processor.h> > > +#undef pr_fmt > +#define pr_fmt(fmt) "x86/cpu: " fmt > + > +#define FEAT_CTL_UNSUPPORTED_MSG "IA32_FEATURE_CONTROL MSR unsupported on VMX capable CPU, suspected hardware or hypervisor issue.\n" > + > void init_feature_control_msr(struct cpuinfo_x86 *c) > { > + bool tboot = tboot_enabled(); > u64 msr; > > - if (rdmsrl_safe(MSR_IA32_FEATURE_CONTROL, &msr)) > + if (rdmsrl_safe(MSR_IA32_FEATURE_CONTROL, &msr)) { > + if (cpu_has(c, X86_FEATURE_VMX)) { > + pr_err_once(FEAT_CTL_UNSUPPORTED_MSG); > + clear_cpu_cap(c, X86_FEATURE_VMX); > + } > return; > + } Right, so this test: is this something that could happen on some configurations - i.e., the MSR is not there but VMX bit is set - or are you being too cautious here? IOW, do you have any concrete use cases in mind (cloud provider can f*ck it up this way) or? My angle is that if this is never going to happen, why even bother to print anything... > if (msr & FEAT_CTL_LOCKED) > - return; > + goto update_caps; > > /* > * Ignore whatever value BIOS left in the MSR to avoid enabling random > @@ -28,8 +39,19 @@ void init_feature_control_msr(struct cpuinfo_x86 *c) > */ > if (cpu_has(c, X86_FEATURE_VMX) && IS_ENABLED(CONFIG_KVM)) { > msr |= FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX; > - if (tboot_enabled()) > + if (tboot) > msr |= FEAT_CTL_VMX_ENABLED_INSIDE_SMX; > } > wrmsrl(MSR_IA32_FEATURE_CONTROL, msr); > + > +update_caps: > + if (!cpu_has(c, X86_FEATURE_VMX)) > + return; > + > + if ((tboot && !(msr & FEAT_CTL_VMX_ENABLED_INSIDE_SMX)) || > + (!tboot && !(msr & FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX))) { Align those vertically like this so that the check is grokkable at a quick glance: if ( (tboot && !(msr & FEAT_CTL_VMX_ENABLED_INSIDE_SMX)) || (!tboot && !(msr & FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX))) { Thx.
On Thu, Nov 21, 2019 at 05:24:52PM +0100, Borislav Petkov wrote: > On Mon, Nov 18, 2019 at 07:12:30PM -0800, Sean Christopherson wrote: > > Now that the IA32_FEATURE_CONTROL MSR is guaranteed to be configured and > > locked, clear the VMX capability flag if the IA32_FEATURE_CONTROL MSR is > > not supported or if BIOS disabled VMX, i.e. locked IA32_FEATURE_CONTROL > > and did not set the appropriate VMX enable bit. > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > --- > > arch/x86/kernel/cpu/feature_control.c | 28 ++++++++++++++++++++++++--- > > 1 file changed, 25 insertions(+), 3 deletions(-) > > > > diff --git a/arch/x86/kernel/cpu/feature_control.c b/arch/x86/kernel/cpu/feature_control.c > > index 33c9444dda52..2bd1a9e6021a 100644 > > --- a/arch/x86/kernel/cpu/feature_control.c > > +++ b/arch/x86/kernel/cpu/feature_control.c > > @@ -5,15 +5,26 @@ > > #include <asm/msr-index.h> > > #include <asm/processor.h> > > > > +#undef pr_fmt > > +#define pr_fmt(fmt) "x86/cpu: " fmt > > + > > +#define FEAT_CTL_UNSUPPORTED_MSG "IA32_FEATURE_CONTROL MSR unsupported on VMX capable CPU, suspected hardware or hypervisor issue.\n" > > + > > void init_feature_control_msr(struct cpuinfo_x86 *c) > > { > > + bool tboot = tboot_enabled(); > > u64 msr; > > > > - if (rdmsrl_safe(MSR_IA32_FEATURE_CONTROL, &msr)) > > + if (rdmsrl_safe(MSR_IA32_FEATURE_CONTROL, &msr)) { > > + if (cpu_has(c, X86_FEATURE_VMX)) { > > + pr_err_once(FEAT_CTL_UNSUPPORTED_MSG); > > + clear_cpu_cap(c, X86_FEATURE_VMX); > > + } > > return; > > + } > > Right, so this test: is this something that could happen on some > configurations - i.e., the MSR is not there but VMX bit is set - or are > you being too cautious here? Probably being overly cautious. > IOW, do you have any concrete use cases in mind (cloud provider can f*ck > it up this way) or? Yes, VMM somehow managing to break things. Admittedly extremely unlikely given how long IA32_FEATURE_CONTROL has been around. > My angle is that if this is never going to happen, why even bother to > print anything... My thought was to add an equivalent of the WARN that fires when an MSR access unexpectedly faults. That's effectively what'd be happening, except I used the safe variant to reduce the maintenance cost, e.g. so that the RDMSR doesn't have to be conditioned on every possible feature. What about a WARN_ON cpu_has? That'd be more aligned with the unexpected #GP on RDMSR behavior. if (rdmsrl_safe(...)) { if (WARN_ON_ONCE(cpu_has(c, X86_FEATURE_VMX))) clear_cpu_cap(c, X86_FEATURE_VMX); return; } I'm also ok dropping it altogether, though from a KVM developer perspective I wouldn't mind the extra sanity check :-)
On Thu, Nov 21, 2019 at 01:07:14PM -0800, Sean Christopherson wrote: > I'm also ok dropping it altogether, though from a KVM developer > perspective I wouldn't mind the extra sanity check :-) Right, I think we should keep it in the bag and only take it out when it really happens. Because if we really wanna be overly cautious in every possible case where stuff may fail, we won't see the actual code from all the error handling. :-) Thx.
diff --git a/arch/x86/kernel/cpu/feature_control.c b/arch/x86/kernel/cpu/feature_control.c index 33c9444dda52..2bd1a9e6021a 100644 --- a/arch/x86/kernel/cpu/feature_control.c +++ b/arch/x86/kernel/cpu/feature_control.c @@ -5,15 +5,26 @@ #include <asm/msr-index.h> #include <asm/processor.h> +#undef pr_fmt +#define pr_fmt(fmt) "x86/cpu: " fmt + +#define FEAT_CTL_UNSUPPORTED_MSG "IA32_FEATURE_CONTROL MSR unsupported on VMX capable CPU, suspected hardware or hypervisor issue.\n" + void init_feature_control_msr(struct cpuinfo_x86 *c) { + bool tboot = tboot_enabled(); u64 msr; - if (rdmsrl_safe(MSR_IA32_FEATURE_CONTROL, &msr)) + if (rdmsrl_safe(MSR_IA32_FEATURE_CONTROL, &msr)) { + if (cpu_has(c, X86_FEATURE_VMX)) { + pr_err_once(FEAT_CTL_UNSUPPORTED_MSG); + clear_cpu_cap(c, X86_FEATURE_VMX); + } return; + } if (msr & FEAT_CTL_LOCKED) - return; + goto update_caps; /* * Ignore whatever value BIOS left in the MSR to avoid enabling random @@ -28,8 +39,19 @@ void init_feature_control_msr(struct cpuinfo_x86 *c) */ if (cpu_has(c, X86_FEATURE_VMX) && IS_ENABLED(CONFIG_KVM)) { msr |= FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX; - if (tboot_enabled()) + if (tboot) msr |= FEAT_CTL_VMX_ENABLED_INSIDE_SMX; } wrmsrl(MSR_IA32_FEATURE_CONTROL, msr); + +update_caps: + if (!cpu_has(c, X86_FEATURE_VMX)) + return; + + if ((tboot && !(msr & FEAT_CTL_VMX_ENABLED_INSIDE_SMX)) || + (!tboot && !(msr & FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX))) { + pr_err_once("VMX (%s TXT) disabled by BIOS\n", + tboot ? "inside" : "outside"); + clear_cpu_cap(c, X86_FEATURE_VMX); + } }
Now that the IA32_FEATURE_CONTROL MSR is guaranteed to be configured and locked, clear the VMX capability flag if the IA32_FEATURE_CONTROL MSR is not supported or if BIOS disabled VMX, i.e. locked IA32_FEATURE_CONTROL and did not set the appropriate VMX enable bit. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kernel/cpu/feature_control.c | 28 ++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-)