Re: [PATCH 1/3] x86/cpu: Process all CPUID dependencies after identifying CPU info

[Borislav Petkov]

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