Re: [PATCH v2 01/16] x86/intel: Initialize IA32_FEATURE_CONTROL MSR at boot

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Fri, Oct 25, 2019 at 04:09:27PM +0200, Borislav Petkov wrote:
> On Mon, Oct 21, 2019 at 04:54:23PM -0700, Sean Christopherson wrote:
> > --- a/arch/x86/kernel/cpu/Makefile
> > +++ b/arch/x86/kernel/cpu/Makefile
> > @@ -29,6 +29,7 @@ obj-y			+= umwait.o
> >  obj-$(CONFIG_PROC_FS)	+= proc.o
> >  obj-$(CONFIG_X86_FEATURE_NAMES) += capflags.o powerflags.o
> >  
> > +obj-$(CONFIG_X86_FEATURE_CONTROL_MSR) += feature_control.o
> >  ifdef CONFIG_CPU_SUP_INTEL
> >  obj-y			+= intel.o intel_pconfig.o
> >  obj-$(CONFIG_PM)	+= intel_epb.o
> > diff --git a/arch/x86/kernel/cpu/cpu.h b/arch/x86/kernel/cpu/cpu.h
> > index c0e2407abdd6..d2750f53a0cb 100644
> > --- a/arch/x86/kernel/cpu/cpu.h
> > +++ b/arch/x86/kernel/cpu/cpu.h
> > @@ -62,4 +62,8 @@ unsigned int aperfmperf_get_khz(int cpu);
> >  
> >  extern void x86_spec_ctrl_setup_ap(void);
> >  
> > +#ifdef CONFIG_X86_FEATURE_CONTROL_MSR
> > +void init_feature_control_msr(struct cpuinfo_x86 *c);
> > +#endif
> > +
> >  #endif /* ARCH_X86_CPU_H */
> > diff --git a/arch/x86/kernel/cpu/feature_control.c b/arch/x86/kernel/cpu/feature_control.c
> > new file mode 100644
> > index 000000000000..57b928e64cf5
> > --- /dev/null
> > +++ b/arch/x86/kernel/cpu/feature_control.c
> 
> Why the separate compilation unit and the Kconfig variable? This can
> live in ...cpu/intel.c just fine, right?

Patches 03/14 and 04/14 enable CONFIG_X86_FEATURE_CONTROL_MSR for Centaur
and Zhaoxin CPUs, putting this in intel.c would make those CPUs depend on
CONFIG_CPU_SUP_INTEL.

The common code and Kconfig is used in patch 10/16 to consolidate the VMX
feature flag code that is copy-pasted from Intel -> Centaur/Zhaoxin.

CONFIG_X86_FEATURE_CONTROL_MSR is also used by KVM in patch 16/16 to
gatekeep CONFIG_KVM_INTEL, i.e. VMX support, instead of requiring
CONFIG_CPU_SUP_INTEL.  In other words, allow building KVM for Cenatur or
Zhaoxin without having to build in support for Intel.

> > @@ -0,0 +1,30 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <linux/tboot.h>
> > +
> > +#include <asm/cpufeature.h>
> > +#include <asm/msr-index.h>
> > +#include <asm/processor.h>
> > +
> > +void init_feature_control_msr(struct cpuinfo_x86 *c)
> > +{
> > +	u64 msr;
> > +
> > +	if (rdmsrl_safe(MSR_IA32_FEATURE_CONTROL, &msr))
> > +		return;
> > +
> > +	if (msr & FEATURE_CONTROL_LOCKED)
> > +		return;
> > +
> > +	/*
> > +	 * Ignore whatever value BIOS left in the MSR to avoid enabling random
> > +	 * features or faulting on the WRMSR.
> > +	 */
> > +	msr = FEATURE_CONTROL_LOCKED;
> > +
> > +	if (cpu_has(c, X86_FEATURE_VMX)) {
> > +		msr |= FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
> > +		if (tboot_enabled())
> > +			msr |= FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX;
> 
> Any chance you can do s/FEATURE_CONTROL_/FT_CTL_/ or FEAT_CTL or so, to
> those bit defines and maybe the MSR define too? They're a mouthful now.

FEAT_CTL Works for me.  I'd also like to do s/VMXON/VMX to match the SDM.
My vote is to leave the name of the MSR itself as is.

Paolo, any opinion on tweaking the MSR bits/name?