[v3,09/19] x86/cpu: Clear VMX feature flag if VMX is not fully enabled
diff mbox series

Message ID 20191119031240.7779-10-sean.j.christopherson@intel.com
State New
Headers show
Series
  • x86/cpu: Clean up handling of VMX features
Related show

Commit Message

Sean Christopherson Nov. 19, 2019, 3:12 a.m. UTC
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(-)

Comments

Borislav Petkov Nov. 21, 2019, 4:24 p.m. UTC | #1
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.
Sean Christopherson Nov. 21, 2019, 9:07 p.m. UTC | #2
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 :-)
Borislav Petkov Nov. 22, 2019, 4:02 p.m. UTC | #3
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.

Patch
diff mbox series

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);
+	}
 }