Re: [PATCH v5 13/19] x86/cpufeatures: Add flag to track whether MSR IA32_FEAT_CTL is configured
From: Jacob Keller
Date: Tue Feb 25 2020 - 19:41:51 EST
On 2/25/2020 3:54 PM, Jacob Keller wrote:
>
> I reverted the suggested commit and added some prints:
>
> [ 26.056398] X86_FEATURE_MSR_IA32_FEAT_CTL is enabled
> [ 26.062426] X86_FEATURE_VMX is enabled
> [ 26.066923] kvm: disabled by bios
>
> So the old code flow is finding KVM to be disabled, but both features
> are set...
>
> The code that sets this is run first:
>
>> Feb 25 15:46:05 jbrandeb-saw1 kernel: x86/cpu: FEAT_CTL_LOCKED is set
>> Feb 25 15:46:05 jbrandeb-saw1 kernel: x86/cpu: FEAT_CTL_VMX_ENABLED_INSIDE_SMX is unset
>> Feb 25 15:46:05 jbrandeb-saw1 kernel: x86/cpu: FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX is unset
>> Feb 25 15:46:05 jbrandeb-saw1 kernel: x86/cpu: MSR locked by bios
>> Feb 25 15:46:05 jbrandeb-saw1 kernel: x86/cpu: VMX (outside TXT) disabled by BIOS
>> Feb 25 15:46:05 jbrandeb-saw1 kernel: x86/cpu: disabling X86_FEATURE_VMX
>
> But somehow... it is still set later...
>
> So there's something weird going on. Maybe "boot_cpu_has" in the
> vmx_disabled_by_bios is wrong? Hmm.
>
I added even more pr_warns, giving me the following diff after reverting
the suggested commit:
>
>
> diff --git a/arch/x86/kernel/cpu/feat_ctl.c b/arch/x86/kernel/cpu/feat_ctl.c
> index 0268185bef94..a86619acab80 100644
> --- a/arch/x86/kernel/cpu/feat_ctl.c
> +++ b/arch/x86/kernel/cpu/feat_ctl.c
> @@ -97,13 +97,27 @@ void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
> bool tboot = tboot_enabled();
> u64 msr;
>
> + pr_warn("before X86_FEATURE_MSR_IA32_FEAT_CTL is %s\n",
> + cpu_has(c, X86_FEATURE_MSR_IA32_FEAT_CTL) ? "enabled" : "disabled");
> + pr_warn("before X86_FEATURE_VMX is %s\n",
> + cpu_has(c, X86_FEATURE_VMX) ? "enabled" : "disabled");
> +
> if (rdmsrl_safe(MSR_IA32_FEAT_CTL, &msr)) {
> clear_cpu_cap(c, X86_FEATURE_VMX);
> return;
> }
>
> - if (msr & FEAT_CTL_LOCKED)
> + pr_warn("FEAT_CTL_LOCKED is %s\n",
> + msr & FEAT_CTL_LOCKED ? "set" : "unset");
> + pr_warn("FEAT_CTL_VMX_ENABLED_INSIDE_SMX is %s\n",
> + msr & FEAT_CTL_VMX_ENABLED_INSIDE_SMX ? "set" : "unset");
> + pr_warn("FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX is %s\n",
> + msr & FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX ? "set" : "unset");
> +
> + if (msr & FEAT_CTL_LOCKED) {
> + pr_warn("MSR locked by bios\n");
> goto update_caps;
> + }
>
> /*
> * Ignore whatever value BIOS left in the MSR to avoid enabling random
> @@ -136,10 +150,16 @@ void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
> if (IS_ENABLED(CONFIG_KVM_INTEL))
> pr_err_once("VMX (%s TXT) disabled by BIOS\n",
> tboot ? "inside" : "outside");
> + pr_warn("disabling X86_FEATURE_VMX\n");
> clear_cpu_cap(c, X86_FEATURE_VMX);
> } else {
> #ifdef CONFIG_X86_VMX_FEATURE_NAMES
> init_vmx_capabilities(c);
> #endif
> }
> +
> + pr_warn("after X86_FEATURE_MSR_IA32_FEAT_CTL is %s\n",
> + cpu_has(c, X86_FEATURE_MSR_IA32_FEAT_CTL) ? "enabled" : "disabled");
> + pr_warn("after X86_FEATURE_VMX is %s\n",
> + cpu_has(c, X86_FEATURE_VMX) ? "enabled" : "disabled");
> }
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index a2e18e60c2db..550f8d556251 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -2222,6 +2222,16 @@ static __init int vmx_disabled_by_bios(void)
> {
> u64 msr;
>
> + pr_warn("boot X86_FEATURE_MSR_IA32_FEAT_CTL is %s\n",
> + boot_cpu_has(X86_FEATURE_MSR_IA32_FEAT_CTL) ? "enabled" : "disabled");
> + pr_warn("boot X86_FEATURE_VMX is %s\n",
> + boot_cpu_has(X86_FEATURE_VMX) ? "enabled" : "disabled");
> +
> + pr_warn("this_cpu X86_FEATURE_MSR_IA32_FEAT_CTL is %s\n",
> + this_cpu_has(X86_FEATURE_MSR_IA32_FEAT_CTL) ? "enabled" : "disabled");
> + pr_warn("this_cpu X86_FEATURE_VMX is %s\n",
> + this_cpu_has(X86_FEATURE_VMX) ? "enabled" : "disabled");
> +
> rdmsrl(MSR_IA32_FEAT_CTL, msr);
>
> if (unlikely(!(msr & FEAT_CTL_LOCKED)))
With this, I see the following output for each CPU, starting with boot CPU:
> Feb 25 16:35:59 jbrandeb-saw1 kernel: x86/cpu: before X86_FEATURE_MSR_IA32_FEAT_CTL is disabled
> Feb 25 16:35:59 jbrandeb-saw1 kernel: x86/cpu: before X86_FEATURE_VMX is enabled
> Feb 25 16:35:59 jbrandeb-saw1 kernel: x86/cpu: FEAT_CTL_LOCKED is set
> Feb 25 16:35:59 jbrandeb-saw1 kernel: x86/cpu: FEAT_CTL_VMX_ENABLED_INSIDE_SMX is unset
> Feb 25 16:35:59 jbrandeb-saw1 kernel: x86/cpu: FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX is unset
> Feb 25 16:35:59 jbrandeb-saw1 kernel: x86/cpu: MSR locked by bios
> Feb 25 16:35:59 jbrandeb-saw1 kernel: x86/cpu: VMX (outside TXT) disabled by BIOS
> Feb 25 16:35:59 jbrandeb-saw1 kernel: x86/cpu: disabling X86_FEATURE_VMX
> Feb 25 16:35:59 jbrandeb-saw1 kernel: x86/cpu: after X86_FEATURE_MSR_IA32_FEAT_CTL is enabled
> Feb 25 16:35:59 jbrandeb-saw1 kernel: x86/cpu: after X86_FEATURE_VMX is disabled
And for each of the SMP CPUs:
> Feb 25 16:35:59 jbrandeb-saw1 kernel: x86/cpu: before X86_FEATURE_MSR_IA32_FEAT_CTL is disabled
> Feb 25 16:35:59 jbrandeb-saw1 kernel: x86/cpu: before X86_FEATURE_VMX is enabled
> Feb 25 16:35:59 jbrandeb-saw1 kernel: x86/cpu: FEAT_CTL_LOCKED is set
> Feb 25 16:35:59 jbrandeb-saw1 kernel: x86/cpu: FEAT_CTL_VMX_ENABLED_INSIDE_SMX is unset
> Feb 25 16:35:59 jbrandeb-saw1 kernel: x86/cpu: FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX is unset
> Feb 25 16:35:59 jbrandeb-saw1 kernel: x86/cpu: MSR locked by bios
> Feb 25 16:35:59 jbrandeb-saw1 kernel: x86/cpu: disabling X86_FEATURE_VMX
> Feb 25 16:35:59 jbrandeb-saw1 kernel: x86/cpu: after X86_FEATURE_MSR_IA32_FEAT_CTL is enabled
> Feb 25 16:35:59 jbrandeb-saw1 kernel: x86/cpu: after X86_FEATURE_VMX is disabled
But when we finally go to check kvm:
> Feb 25 16:36:06 jbrandeb-saw1 kernel: boot X86_FEATURE_MSR_IA32_FEAT_CTL is enabled
> Feb 25 16:36:06 jbrandeb-saw1 kernel: boot X86_FEATURE_VMX is enabled
> Feb 25 16:36:06 jbrandeb-saw1 kernel: this_cpu X86_FEATURE_MSR_IA32_FEAT_CTL is enabled
> Feb 25 16:36:06 jbrandeb-saw1 kernel: this_cpu X86_FEATURE_VMX is enabled
I tried checking both boot and this_cpu, just in case.
Somehow the things are being restored/re-enabled. I can't figure out
where this even happens. At a glance it's not even obvious to me where
the original features get set, and nothing seems to obviously set these
flags....
Thanks,
Jake