On Sat, Dec 03, 2022 at 12:37:43AM +0000, Sean Christopherson wrote: > Process all CPUID dependencies to ensure that a dependent is disabled if > one or more of its parent features is unsupported. Just out of curiosity: this is some weird guest configuration, right? Not addressing a real hw issue... > As is, cpuid_deps is > processed if an only if a feature is explicitly disabled via > clear_cpu_cap(), which makes it annoying/dangerous to use cpuid_deps for > features whose parent(s) do not always have explicit processing. > > E.g. VMX and SGX depend on the synthetic X86_FEATURE_MSR_IA32_FEAT_CTL, > but there is no common location to clear MSR_IA32_FEAT_CTL, and so > consumers of VMX and SGX are forced to check MSR_IA32_FEAT_CTL on top > of the dependent feature. > > Manually clearing X86_FEATURE_MSR_IA32_FEAT_CTL is the obvious > alternative, but it's subtly more difficult that updating > init_ia32_feat_ctl(). CONFIG_IA32_FEAT_CTL depends on any of > CONFIG_CPU_SUP_{INTEL,CENATUR,ZHAOXIN}, but init_ia32_feat_ctl() is > invoked if and only if the actual CPU type matches one of the > aforementioned CPU_SUP_* types. E.g. running a kernel built with > > CONFIG_CPU_SUP_INTEL=y > CONFIG_CPU_SUP_AMD=y > # CONFIG_CPU_SUP_HYGON is not set > # CONFIG_CPU_SUP_CENTAUR is not set > # CONFIG_CPU_SUP_ZHAOXIN is not set > > on a Cenatur or Zhaoxin CPU will leave X86_FEATURE_VMX set but not set Typo fix for the committer: Centaur > X86_FEATURE_MSR_IA32_FEAT_CTL, and will never call init_ia32_feat_ctl() > to give the kernel a convenient opportunity to clear > X86_FEATURE_MSR_IA32_FEAT_CTL. > > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx> > --- > arch/x86/include/asm/cpufeature.h | 1 + > arch/x86/kernel/cpu/common.c | 6 ++++++ > arch/x86/kernel/cpu/cpuid-deps.c | 10 ++++++++++ > 3 files changed, 17 insertions(+) ... > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c > index bf4ac1cb93d7..094fc69dba63 100644 > --- a/arch/x86/kernel/cpu/common.c > +++ b/arch/x86/kernel/cpu/common.c > @@ -1887,6 +1887,12 @@ static void identify_cpu(struct cpuinfo_x86 *c) > > ppin_init(c); > > + /* > + * Apply CPUID dependencies to ensure dependent features are disabled > + * if a parent feature is unsupported but wasn't explicitly disabled. > + */ > + apply_cpuid_deps(c); I'd probably call that resolve_cpuid_deps()... But yeah, that init path would need cleaning up at some point - all kinds of init detection functions have been hastily slapped there over the years... -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette