Re: [PATCH v2 06/16] x86/cpu: Clear VMX feature flag if VMX is not fully enabled

[Borislav Petkov]

On Mon, Oct 21, 2019 at 05:08:36PM -0700, 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.
> 
> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Cc: Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx>
> Cc: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> Cc: Wanpeng Li <wanpengli@xxxxxxxxxxx>
> Cc: Jim Mattson <jmattson@xxxxxxxxxx>
> Cc: kvm@xxxxxxxxxxxxxxx
> Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> ---
>  arch/x86/kernel/cpu/feature_control.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/feature_control.c b/arch/x86/kernel/cpu/feature_control.c
> index 57b928e64cf5..74c76159a046 100644
> --- a/arch/x86/kernel/cpu/feature_control.c
> +++ b/arch/x86/kernel/cpu/feature_control.c
> @@ -7,13 +7,19 @@
>  
>  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("x86/cpu: VMX disabled, IA32_FEATURE_CONTROL MSR unsupported\n");
				     ^^^^^^^^

pr_fmt

But, before that: do we really wanna know about this or there's nothing
the user can do? If she can reenable VMX in the BIOS, or otherwise do
something about it, maybe we should say that above... Otherwise, this
message is useless.

> +			clear_cpu_cap(c, X86_FEATURE_VMX);
> +		}
>  		return;
> +	}
>  
>  	if (msr & FEATURE_CONTROL_LOCKED)
> -		return;
> +		goto update_caps;
>  
>  	/*
>  	 * Ignore whatever value BIOS left in the MSR to avoid enabling random
> @@ -23,8 +29,19 @@ void init_feature_control_msr(struct cpuinfo_x86 *c)
>  
>  	if (cpu_has(c, X86_FEATURE_VMX)) {
>  		msr |= FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
> -		if (tboot_enabled())
> +		if (tboot)
>  			msr |= FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX;
>  	}
>  	wrmsrl(MSR_IA32_FEATURE_CONTROL, msr);
> +
> +update_caps:
> +	if (!cpu_has(c, X86_FEATURE_VMX))
> +		return;

If this test is just so we can save us the below code, I'd say remove it
for the sake of having less code in that function. The test is cheap and
not on a fast path so who cares if we clear an alrady cleared bit. But
maybe this evolves in the later patches...

> +
> +	if ((tboot && !(msr & FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX)) ||
> +	    (!tboot && !(msr & FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX))) {
> +		pr_err_once("x86/cpu: VMX disabled by BIOS (TXT %s)\n",
> +			    tboot ? "enabled" : "disabled");
> +		clear_cpu_cap(c, X86_FEATURE_VMX);
> +	}
>  }

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette