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
