[PATCH 0/4] x86: cpuid: improve support for broken CPUID configurations

[PATCH 0/4] x86: cpuid: improve support for broken CPUID configurations

[Maxim Levitsky]

This patch series aims to harden the cpuid code against the case when
the hypervisor exposes a broken CPUID configuration to the guest,
in the form of having a feature disabled but not features that depend on it.

This is the more generic way to fix kernel panic in aes-ni kernel driver,
which was triggered by CPUID configuration in which AVX is disabled but
not AVX2.

https://lore.kernel.org/all/20211103145231.GA4485@gondor.apana.org.au/T/

This was tested by booting a guest with AVX disabled and not AVX2,
and observing that both a warning is now printed in dmesg, and
that avx2 is gone from /proc/cpuinfo.

Best regards,
	Maxim Levitsky

Maxim Levitsky (4):
  perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap
  x86/cpuid: refactor setup_clear_cpu_cap/clear_feature
  x86/cpuid: move filter_cpuid_features to cpuid-deps.c
  x86/cpuid: check for dependencies violations in CPUID and attempt to
    fix them

 arch/x86/events/intel/lbr.c       |  2 +-
 arch/x86/include/asm/cpufeature.h |  1 +
 arch/x86/kernel/cpu/common.c      | 50 +-----------------
 arch/x86/kernel/cpu/cpuid-deps.c  | 84 +++++++++++++++++++++++++++----
 4 files changed, 78 insertions(+), 59 deletions(-)

-- 
2.26.3

[PATCH 1/4] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

[Maxim Levitsky]

clear_cpu_cap(&boot_cpu_data) is very similar to setup_clear_cpu_cap
except that the latter also sets a bit in 'cpu_caps_cleared' which
later clears the same cap in secondary cpus, which is likely
what is meant here.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/events/intel/lbr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c
index 13179f31fe10fa..b08715172309a7 100644
--- a/arch/x86/events/intel/lbr.c
+++ b/arch/x86/events/intel/lbr.c
@@ -1860,7 +1860,7 @@ void __init intel_pmu_arch_lbr_init(void)
 	return;
 
 clear_arch_lbr:
-	clear_cpu_cap(&boot_cpu_data, X86_FEATURE_ARCH_LBR);
+	setup_clear_cpu_cap(X86_FEATURE_ARCH_LBR);
 }
 
 /**
-- 
2.26.3

Re: [PATCH 1/4] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

[Dave Hansen]

On 6/22/22 07:48, Maxim Levitsky wrote:
> clear_cpu_cap(&boot_cpu_data) is very similar to setup_clear_cpu_cap
> except that the latter also sets a bit in 'cpu_caps_cleared' which
> later clears the same cap in secondary cpus, which is likely
> what is meant here.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>

Seems like a:

Fixes: 47125db27e47 ("perf/x86/intel/lbr: Support Architectural LBR")

would be in order.

Kan, does this change look right to you?

>  arch/x86/events/intel/lbr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c
> index 13179f31fe10fa..b08715172309a7 100644
> --- a/arch/x86/events/intel/lbr.c
> +++ b/arch/x86/events/intel/lbr.c
> @@ -1860,7 +1860,7 @@ void __init intel_pmu_arch_lbr_init(void)
>  	return;
>  
>  clear_arch_lbr:
> -	clear_cpu_cap(&boot_cpu_data, X86_FEATURE_ARCH_LBR);
> +	setup_clear_cpu_cap(X86_FEATURE_ARCH_LBR);
>  }
>  
>  /**

Re: [PATCH 1/4] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

[Liang, Kan]

On 6/22/2022 10:58 AM, Dave Hansen wrote:
> On 6/22/22 07:48, Maxim Levitsky wrote:
>> clear_cpu_cap(&boot_cpu_data) is very similar to setup_clear_cpu_cap
>> except that the latter also sets a bit in 'cpu_caps_cleared' which
>> later clears the same cap in secondary cpus, which is likely
>> what is meant here.
>>
>> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> 
> Seems like a:
> 
> Fixes: 47125db27e47 ("perf/x86/intel/lbr: Support Architectural LBR")
> 
> would be in order.
> 
> Kan, does this change look right to you?

For the current implementation, the Arch LBR feature should be either 
supported by all the CPUs or disabled on all the CPUs. It cannot be only 
enabled for partial CPUs, even in a hybrid platform.
So the current code only check the boot CPU via static_cpu_has().

Ideally, Yes, I think it may be better to clear the bit for all CPUs, 
which makes the capability consistent among CPUs.

Thanks,
Kan
> 
>>   arch/x86/events/intel/lbr.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c
>> index 13179f31fe10fa..b08715172309a7 100644
>> --- a/arch/x86/events/intel/lbr.c
>> +++ b/arch/x86/events/intel/lbr.c
>> @@ -1860,7 +1860,7 @@ void __init intel_pmu_arch_lbr_init(void)
>>   	return;
>>   
>>   clear_arch_lbr:
>> -	clear_cpu_cap(&boot_cpu_data, X86_FEATURE_ARCH_LBR);
>> +	setup_clear_cpu_cap(X86_FEATURE_ARCH_LBR);
>>   }
>>   
>>   /**
> 

Re: [PATCH 1/4] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

[Liang, Kan]

On 6/22/2022 2:57 PM, Liang, Kan wrote:
> 
> 
> On 6/22/2022 10:58 AM, Dave Hansen wrote:
>> On 6/22/22 07:48, Maxim Levitsky wrote:
>>> clear_cpu_cap(&boot_cpu_data) is very similar to setup_clear_cpu_cap
>>> except that the latter also sets a bit in 'cpu_caps_cleared' which
>>> later clears the same cap in secondary cpus, which is likely
>>> what is meant here.
>>>
>>> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
>>
>> Seems like a:
>>
>> Fixes: 47125db27e47 ("perf/x86/intel/lbr: Support Architectural LBR")
>>
>> would be in order.
>>
>> Kan, does this change look right to you?
> 
> For the current implementation, the Arch LBR feature should be either 
> supported by all the CPUs or disabled on all the CPUs. It cannot be only 
> enabled for partial CPUs, even in a hybrid platform.
> So the current code only check the boot CPU via static_cpu_has().
> 
> Ideally, Yes, I think it may be better to clear the bit for all CPUs, 
> which makes the capability consistent among CPUs.
>

Reviewed-by: Kan Liang <kan.liang@linux.intel.com>

Thanks,
Kan


> Thanks,
> Kan
>>
>>>   arch/x86/events/intel/lbr.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c
>>> index 13179f31fe10fa..b08715172309a7 100644
>>> --- a/arch/x86/events/intel/lbr.c
>>> +++ b/arch/x86/events/intel/lbr.c
>>> @@ -1860,7 +1860,7 @@ void __init intel_pmu_arch_lbr_init(void)
>>>       return;
>>>   clear_arch_lbr:
>>> -    clear_cpu_cap(&boot_cpu_data, X86_FEATURE_ARCH_LBR);
>>> +    setup_clear_cpu_cap(X86_FEATURE_ARCH_LBR);
>>>   }
>>>   /**
>>

[PATCH 2/4] x86/cpuid: refactor setup_clear_cpu_cap/clear_feature

[Maxim Levitsky]

Simplify the code a bit by always passing &boot_cpu_data
in case the setup_clear_cpu_cap was called.

Also unify clear_cpu_cap and do_clear_cpu_cap.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kernel/cpu/cpuid-deps.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index c881bcafba7d70..d6777d07ba3302 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -88,18 +88,16 @@ static inline void clear_feature(struct cpuinfo_x86 *c, unsigned int feature)
 	 * rest of the cpufeature code uses atomics as well, so keep it for
 	 * consistency. Cleanup all of it separately.
 	 */
-	if (!c) {
-		clear_cpu_cap(&boot_cpu_data, feature);
+	clear_bit(feature, (unsigned long *)c->x86_capability);
+
+	if (c == &boot_cpu_data)
 		set_bit(feature, (unsigned long *)cpu_caps_cleared);
-	} else {
-		clear_bit(feature, (unsigned long *)c->x86_capability);
-	}
 }
 
 /* Take the capabilities and the BUG bits into account */
 #define MAX_FEATURE_BITS ((NCAPINTS + NBUGINTS) * sizeof(u32) * 8)
 
-static void do_clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int feature)
+void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int feature)
 {
 	DECLARE_BITMAP(disable, MAX_FEATURE_BITS);
 	const struct cpuid_dep *d;
@@ -129,12 +127,7 @@ static void do_clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int feature)
 	} while (changed);
 }
 
-void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int feature)
-{
-	do_clear_cpu_cap(c, feature);
-}
-
 void setup_clear_cpu_cap(unsigned int feature)
 {
-	do_clear_cpu_cap(NULL, feature);
+	clear_cpu_cap(&boot_cpu_data, feature);
 }
-- 
2.26.3

Re: [PATCH 2/4] x86/cpuid: refactor setup_clear_cpu_cap/clear_feature

[Dave Hansen]

On 6/22/22 07:48, Maxim Levitsky wrote:
> Simplify the code a bit by always passing &boot_cpu_data
> in case the setup_clear_cpu_cap was called.
> 
> Also unify clear_cpu_cap and do_clear_cpu_cap.

Please always add a "()" suffix to functions.  "foo" is a variable, but
"foo()" is a function.

I also really like when a changelog has a clear problem statement.  I
_think_ the problem here is something along the lines of the 'c'
argument to clear_feature() having different behavior when it is NULL
versus '&boot_cpu_data'.

Basically, there's no reason to support clearing a bit in
'&boot_cpu_data' without also setting that bit in 'cpu_caps_cleared'.
>  {
> -	do_clear_cpu_cap(NULL, feature);
> +	clear_cpu_cap(&boot_cpu_data, feature);
>  }

Re: [PATCH 2/4] x86/cpuid: refactor setup_clear_cpu_cap/clear_feature

[Maxim Levitsky]

On Wed, 2022-06-22 at 08:07 -0700, Dave Hansen wrote:
> On 6/22/22 07:48, Maxim Levitsky wrote:
> > Simplify the code a bit by always passing &boot_cpu_data
> > in case the setup_clear_cpu_cap was called.
> > 
> > Also unify clear_cpu_cap and do_clear_cpu_cap.
> 
> Please always add a "()" suffix to functions.  "foo" is a variable, but
> "foo()" is a function.
Will do next time!


> 
> I also really like when a changelog has a clear problem statement.  I
> _think_ the problem here is something along the lines of the 'c'
> argument to clear_feature() having different behavior when it is NULL
> versus '&boot_cpu_data'.

To be honest I didn't try to fix any problem here, I just wanted to simplify
clear_feature() a bit by avoiding a recursive call to clear_cpu_cap.

> 
> Basically, there's no reason to support clearing a bit in
> '&boot_cpu_data' without also setting that bit in 'cpu_caps_cleared'.

> >  {
> > -	do_clear_cpu_cap(NULL, feature);
> > +	clear_cpu_cap(&boot_cpu_data, feature);
> >  }

I'll try to think about a better changelog message for this.

Thank you!
Best regards,
	Maxim Levitsky

[PATCH 3/4] x86/cpuid: move filter_cpuid_features to cpuid-deps.c

[Maxim Levitsky]

No functional change intended.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/include/asm/cpufeature.h |  1 +
 arch/x86/kernel/cpu/common.c      | 46 -----------------------------
 arch/x86/kernel/cpu/cpuid-deps.c  | 48 +++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+), 46 deletions(-)

diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index ea34cc31b0474f..3eb5fe0d654e63 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -147,6 +147,7 @@ extern const char * const x86_bug_flags[NBUGINTS*32];
 
 extern void setup_clear_cpu_cap(unsigned int bit);
 extern void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int bit);
+extern void filter_cpuid_features(struct cpuinfo_x86 *c, bool warn);
 
 #define setup_force_cpu_cap(bit) do { \
 	set_cpu_cap(&boot_cpu_data, bit);	\
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 4730b0a58f24a5..4cc79971d2d847 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -620,52 +620,6 @@ __noendbr void cet_disable(void)
 		wrmsrl(MSR_IA32_S_CET, 0);
 }
 
-/*
- * Some CPU features depend on higher CPUID levels, which may not always
- * be available due to CPUID level capping or broken virtualization
- * software.  Add those features to this table to auto-disable them.
- */
-struct cpuid_dependent_feature {
-	u32 feature;
-	u32 level;
-};
-
-static const struct cpuid_dependent_feature
-cpuid_dependent_features[] = {
-	{ X86_FEATURE_MWAIT,		0x00000005 },
-	{ X86_FEATURE_DCA,		0x00000009 },
-	{ X86_FEATURE_XSAVE,		0x0000000d },
-	{ 0, 0 }
-};
-
-static void filter_cpuid_features(struct cpuinfo_x86 *c, bool warn)
-{
-	const struct cpuid_dependent_feature *df;
-
-	for (df = cpuid_dependent_features; df->feature; df++) {
-
-		if (!cpu_has(c, df->feature))
-			continue;
-		/*
-		 * Note: cpuid_level is set to -1 if unavailable, but
-		 * extended_extended_level is set to 0 if unavailable
-		 * and the legitimate extended levels are all negative
-		 * when signed; hence the weird messing around with
-		 * signs here...
-		 */
-		if (!((s32)df->level < 0 ?
-		     (u32)df->level > (u32)c->extended_cpuid_level :
-		     (s32)df->level > (s32)c->cpuid_level))
-			continue;
-
-		clear_cpu_cap(c, df->feature);
-		if (!warn)
-			continue;
-
-		pr_warn("CPU: CPU feature " X86_CAP_FMT " disabled, no CPUID level 0x%x\n",
-			x86_cap_flag(df->feature), df->level);
-	}
-}
 
 /*
  * Naming convention should be: <Name> [(<Codename>)]
diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index d6777d07ba3302..bcb091d02a754b 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -131,3 +131,51 @@ void setup_clear_cpu_cap(unsigned int feature)
 {
 	clear_cpu_cap(&boot_cpu_data, feature);
 }
+
+
+/*
+ * Some CPU features depend on higher CPUID levels, which may not always
+ * be available due to CPUID level capping or broken virtualization
+ * software.  Add those features to this table to auto-disable them.
+ */
+struct cpuid_dependent_feature {
+	u32 feature;
+	u32 level;
+};
+
+static const struct cpuid_dependent_feature
+cpuid_dependent_features[] = {
+	{ X86_FEATURE_MWAIT,		0x00000005 },
+	{ X86_FEATURE_DCA,		0x00000009 },
+	{ X86_FEATURE_XSAVE,		0x0000000d },
+	{ 0, 0 }
+};
+
+void filter_cpuid_features(struct cpuinfo_x86 *c, bool warn)
+{
+	const struct cpuid_dependent_feature *df;
+
+	for (df = cpuid_dependent_features; df->feature; df++) {
+
+		if (!cpu_has(c, df->feature))
+			continue;
+		/*
+		 * Note: cpuid_level is set to -1 if unavailable, but
+		 * extended_extended_level is set to 0 if unavailable
+		 * and the legitimate extended levels are all negative
+		 * when signed; hence the weird messing around with
+		 * signs here...
+		 */
+		if (!((s32)df->level < 0 ?
+		     (u32)df->level > (u32)c->extended_cpuid_level :
+		     (s32)df->level > (s32)c->cpuid_level))
+			continue;
+
+		clear_cpu_cap(c, df->feature);
+		if (!warn)
+			continue;
+
+		pr_warn("CPU: CPU feature " X86_CAP_FMT " disabled, no CPUID level 0x%x\n",
+			x86_cap_flag(df->feature), df->level);
+	}
+}
-- 
2.26.3

Re: [PATCH 3/4] x86/cpuid: move filter_cpuid_features to cpuid-deps.c

[Dave Hansen]

On 6/22/22 07:48, Maxim Levitsky wrote:
> No functional change intended.

It would be really nice to at least write a "why" sentence.  You wrote
the "what" (move code), but I have no idea why you are moving it.

>  arch/x86/kernel/cpu/common.c      | 46 -----------------------------
>  arch/x86/kernel/cpu/cpuid-deps.c  | 48 +++++++++++++++++++++++++++++++

That looks a wee bit odd for a code move.  Where did the 2 lines go?

> diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
> index ea34cc31b0474f..3eb5fe0d654e63 100644
> --- a/arch/x86/include/asm/cpufeature.h
> +++ b/arch/x86/include/asm/cpufeature.h
> @@ -147,6 +147,7 @@ extern const char * const x86_bug_flags[NBUGINTS*32];
>  
>  extern void setup_clear_cpu_cap(unsigned int bit);
>  extern void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int bit);
> +extern void filter_cpuid_features(struct cpuinfo_x86 *c, bool warn);
>  
>  #define setup_force_cpu_cap(bit) do { \
>  	set_cpu_cap(&boot_cpu_data, bit);	\
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 4730b0a58f24a5..4cc79971d2d847 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -620,52 +620,6 @@ __noendbr void cet_disable(void)
>  		wrmsrl(MSR_IA32_S_CET, 0);
>  }
>  
> -/*
...
> -}
>  
>  /*
>   * Naming convention should be: <Name> [(<Codename>)]

One, by leaving extra whitespace.

> diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
> index d6777d07ba3302..bcb091d02a754b 100644
> --- a/arch/x86/kernel/cpu/cpuid-deps.c
> +++ b/arch/x86/kernel/cpu/cpuid-deps.c
> @@ -131,3 +131,51 @@ void setup_clear_cpu_cap(unsigned int feature)
>  {
>  	clear_cpu_cap(&boot_cpu_data, feature);
>  }
> +
> +
> +/*

Two, by adding extra whitespace.

Re: [PATCH 3/4] x86/cpuid: move filter_cpuid_features to cpuid-deps.c

[Maxim Levitsky]

On Wed, 2022-06-22 at 08:07 -0700, Dave Hansen wrote:
> On 6/22/22 07:48, Maxim Levitsky wrote:
> > No functional change intended.
> 
> It would be really nice to at least write a "why" sentence.  You wrote
> the "what" (move code), but I have no idea why you are moving it.

This is a good point, I will add a justification in V2.

> 
> >  arch/x86/kernel/cpu/common.c      | 46 -----------------------------
> >  arch/x86/kernel/cpu/cpuid-deps.c  | 48 +++++++++++++++++++++++++++++++
> 
> That looks a wee bit odd for a code move.  Where did the 2 lines go?
> 
> > diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
> > index ea34cc31b0474f..3eb5fe0d654e63 100644
> > --- a/arch/x86/include/asm/cpufeature.h
> > +++ b/arch/x86/include/asm/cpufeature.h
> > @@ -147,6 +147,7 @@ extern const char * const x86_bug_flags[NBUGINTS*32];
> >  
> >  extern void setup_clear_cpu_cap(unsigned int bit);
> >  extern void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int bit);
> > +extern void filter_cpuid_features(struct cpuinfo_x86 *c, bool warn);
> >  
> >  #define setup_force_cpu_cap(bit) do { \
> >  	set_cpu_cap(&boot_cpu_data, bit);	\
> > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> > index 4730b0a58f24a5..4cc79971d2d847 100644
> > --- a/arch/x86/kernel/cpu/common.c
> > +++ b/arch/x86/kernel/cpu/common.c
> > @@ -620,52 +620,6 @@ __noendbr void cet_disable(void)
> >  		wrmsrl(MSR_IA32_S_CET, 0);
> >  }
> >  
> > -/*
> ...
> > -}
> >  
> >  /*
> >   * Naming convention should be: <Name> [(<Codename>)]
> 
> One, by leaving extra whitespace.
> 
> > diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
> > index d6777d07ba3302..bcb091d02a754b 100644
> > --- a/arch/x86/kernel/cpu/cpuid-deps.c
> > +++ b/arch/x86/kernel/cpu/cpuid-deps.c
> > @@ -131,3 +131,51 @@ void setup_clear_cpu_cap(unsigned int feature)
> >  {
> >  	clear_cpu_cap(&boot_cpu_data, feature);
> >  }
> > +
> > +
> > +/*
> 
> Two, by adding extra whitespace.
> 

I will fix this, I didn't notice that I added new lines.
Next time I move code around, I'll check that the number count matches.

Thanks!
Best regards,
	Maxim Levitsky

[PATCH 4/4] x86/cpuid: check for dependencies violations in CPUID and attempt to fix them

[Maxim Levitsky]

Due to configuration bugs, sometimes a CPU feature is disabled in CPUID,
but not features that depend on it.

While the above is not supported, the kernel should try to not crash,
and clearing the dependent cpu caps is the best way to do it.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kernel/cpu/common.c     |  4 ++--
 arch/x86/kernel/cpu/cpuid-deps.c | 27 +++++++++++++++++++++++++--
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 4cc79971d2d847..c83a8f447d6aed 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1469,7 +1469,7 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 			this_cpu->c_early_init(c);
 
 		c->cpu_index = 0;
-		filter_cpuid_features(c, false);
+		filter_cpuid_features(c, true);
 
 		if (this_cpu->c_bsp_init)
 			this_cpu->c_bsp_init(c);
@@ -1757,7 +1757,7 @@ static void identify_cpu(struct cpuinfo_x86 *c)
 	 */
 
 	/* Filter out anything that depends on CPUID levels we don't have */
-	filter_cpuid_features(c, true);
+	filter_cpuid_features(c, false);
 
 	/* If the model name is still unset, do table lookup. */
 	if (!c->x86_model_id[0]) {
diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index bcb091d02a754b..6d9c0e39851805 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -94,6 +94,11 @@ static inline void clear_feature(struct cpuinfo_x86 *c, unsigned int feature)
 		set_bit(feature, (unsigned long *)cpu_caps_cleared);
 }
 
+static inline bool test_feature(struct cpuinfo_x86 *c, unsigned int feature)
+{
+	return test_bit(feature, (unsigned long *)c->x86_capability);
+}
+
 /* Take the capabilities and the BUG bits into account */
 #define MAX_FEATURE_BITS ((NCAPINTS + NBUGINTS) * sizeof(u32) * 8)
 
@@ -127,6 +132,7 @@ void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int feature)
 	} while (changed);
 }
 
+
 void setup_clear_cpu_cap(unsigned int feature)
 {
 	clear_cpu_cap(&boot_cpu_data, feature);
@@ -137,6 +143,10 @@ void setup_clear_cpu_cap(unsigned int feature)
  * Some CPU features depend on higher CPUID levels, which may not always
  * be available due to CPUID level capping or broken virtualization
  * software.  Add those features to this table to auto-disable them.
+ *
+ * Also due to configuration bugs, some CPUID features might be present
+ * while CPUID features that they depend on are not present,
+ * e.g a AVX2 present but AVX is not present.
  */
 struct cpuid_dependent_feature {
 	u32 feature;
@@ -151,9 +161,10 @@ cpuid_dependent_features[] = {
 	{ 0, 0 }
 };
 
-void filter_cpuid_features(struct cpuinfo_x86 *c, bool warn)
+void filter_cpuid_features(struct cpuinfo_x86 *c, bool early)
 {
 	const struct cpuid_dependent_feature *df;
+	const struct cpuid_dep *d;
 
 	for (df = cpuid_dependent_features; df->feature; df++) {
 
@@ -172,10 +183,22 @@ void filter_cpuid_features(struct cpuinfo_x86 *c, bool warn)
 			continue;
 
 		clear_cpu_cap(c, df->feature);
-		if (!warn)
+		if (early)
 			continue;
 
 		pr_warn("CPU: CPU feature " X86_CAP_FMT " disabled, no CPUID level 0x%x\n",
 			x86_cap_flag(df->feature), df->level);
 	}
+
+	for (d = cpuid_deps; d->feature; d++) {
+
+		if (!test_feature(c, d->feature) || test_feature(c, d->depends))
+			continue;
+
+		clear_feature(c, d->feature);
+
+		pr_warn("CPU: CPU feature " X86_CAP_FMT " disabled, because it depends on "
+			X86_CAP_FMT " which is not supported in CPUID\n",
+			x86_cap_flag(d->feature), x86_cap_flag(d->depends));
+	}
 }
-- 
2.26.3

Re: [PATCH 4/4] x86/cpuid: check for dependencies violations in CPUID and attempt to fix them

[Dave Hansen]

On 6/22/22 07:48, Maxim Levitsky wrote:
> Due to configuration bugs, sometimes a CPU feature is disabled in CPUID,
> but not features that depend on it.
> 
> While the above is not supported, the kernel should try to not crash,
> and clearing the dependent cpu caps is the best way to do it.

That's a rather paltry changelog.

If I remember correctly, there's a crystal clear problem:

	If a CPU enumerates support for AVX2 but AVX via CPUID, the
	kernel crashes.

There's also a follow-on problem.  The kernel has all the data it needs
to fix this, but just doesn't consult it:

	To make matters worse, the kernel _knows_ that this is an ill-
	advised situation: The kernel prevents itself from clearing the
	software representation of the AVX CPUID bit without also
	clearing AVX2.

	But, the kernel only consults this knowledge when it is clearing
	cpu_cap bits.  It does not consult this information when it is
	populating those cpu_cap bits.

> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 4cc79971d2d847..c83a8f447d6aed 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -1469,7 +1469,7 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
>  			this_cpu->c_early_init(c);
>  
>  		c->cpu_index = 0;
> -		filter_cpuid_features(c, false);
> +		filter_cpuid_features(c, true);
>  
>  		if (this_cpu->c_bsp_init)
>  			this_cpu->c_bsp_init(c);
> @@ -1757,7 +1757,7 @@ static void identify_cpu(struct cpuinfo_x86 *c)
>  	 */
>  
>  	/* Filter out anything that depends on CPUID levels we don't have */
> -	filter_cpuid_features(c, true);
> +	filter_cpuid_features(c, false);
>  
>  	/* If the model name is still unset, do table lookup. */
>  	if (!c->x86_model_id[0]) {

While we're at it, could we please rid ourselves of this unreadable
mystery true/false gunk?

> diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
> index bcb091d02a754b..6d9c0e39851805 100644
> --- a/arch/x86/kernel/cpu/cpuid-deps.c
> +++ b/arch/x86/kernel/cpu/cpuid-deps.c
> @@ -94,6 +94,11 @@ static inline void clear_feature(struct cpuinfo_x86 *c, unsigned int feature)
>  		set_bit(feature, (unsigned long *)cpu_caps_cleared);
>  }
>  
> +static inline bool test_feature(struct cpuinfo_x86 *c, unsigned int feature)
> +{
> +	return test_bit(feature, (unsigned long *)c->x86_capability);
> +}
> +
>  /* Take the capabilities and the BUG bits into account */
>  #define MAX_FEATURE_BITS ((NCAPINTS + NBUGINTS) * sizeof(u32) * 8)
>  
> @@ -127,6 +132,7 @@ void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int feature)
>  	} while (changed);
>  }
>  
> +
>  void setup_clear_cpu_cap(unsigned int feature)

More superfluous whitespace.

>  {
>  	clear_cpu_cap(&boot_cpu_data, feature);
> @@ -137,6 +143,10 @@ void setup_clear_cpu_cap(unsigned int feature)
>   * Some CPU features depend on higher CPUID levels, which may not always
>   * be available due to CPUID level capping or broken virtualization
>   * software.  Add those features to this table to auto-disable them.
> + *
> + * Also due to configuration bugs, some CPUID features might be present
> + * while CPUID features that they depend on are not present,
> + * e.g a AVX2 present but AVX is not present.
>   */
>  struct cpuid_dependent_feature {
>  	u32 feature;
> @@ -151,9 +161,10 @@ cpuid_dependent_features[] = {
>  	{ 0, 0 }
>  };
>  
> -void filter_cpuid_features(struct cpuinfo_x86 *c, bool warn)
> +void filter_cpuid_features(struct cpuinfo_x86 *c, bool early)
>  {

I have at least an inkling what 'warn' could mean.  But, 'early'?  One
man's 'early' is another one's 'late'.

>  	const struct cpuid_dependent_feature *df;
> +	const struct cpuid_dep *d;
>  
>  	for (df = cpuid_dependent_features; df->feature; df++) {
>  
> @@ -172,10 +183,22 @@ void filter_cpuid_features(struct cpuinfo_x86 *c, bool warn)
>  			continue;
>  
>  		clear_cpu_cap(c, df->feature);
> -		if (!warn)
> +		if (early)
>  			continue;

Why is it that 'early' calls don't want warnings?

>  		pr_warn("CPU: CPU feature " X86_CAP_FMT " disabled, no CPUID level 0x%x\n",
>  			x86_cap_flag(df->feature), df->level);
>  	}
> +
> +	for (d = cpuid_deps; d->feature; d++) {
> +
> +		if (!test_feature(c, d->feature) || test_feature(c, d->depends))
> +			continue;
> +
> +		clear_feature(c, d->feature);
> +
> +		pr_warn("CPU: CPU feature " X86_CAP_FMT " disabled, because it depends on "
> +			X86_CAP_FMT " which is not supported in CPUID\n",
> +			x86_cap_flag(d->feature), x86_cap_flag(d->depends));
> +	}
>  }

The do_clear_cpu_cap() does this with a loop, presumably because a later
(higher index in the array) feature in cpuid_deps[] could theoretically
clear an earlier (lower index) feature.

Also, is that message strictly correct?  There might have been a
clearcpuid= argument or even another dependency that ended up clearing a
bit.  It might have nothing to do with CPUID itself.

Re: [PATCH 4/4] x86/cpuid: check for dependencies violations in CPUID and attempt to fix them

[Maxim Levitsky]

On Wed, 2022-06-22 at 08:32 -0700, Dave Hansen wrote:
> On 6/22/22 07:48, Maxim Levitsky wrote:
> > Due to configuration bugs, sometimes a CPU feature is disabled in CPUID,
> > but not features that depend on it.
> > 
> > While the above is not supported, the kernel should try to not crash,
> > and clearing the dependent cpu caps is the best way to do it.
> 
> That's a rather paltry changelog.
> 
> If I remember correctly, there's a crystal clear problem:
> 
> 	If a CPU enumerates support for AVX2 but AVX via CPUID, the
> 	kernel crashes.
> 
> There's also a follow-on problem.  The kernel has all the data it needs
> to fix this, but just doesn't consult it:
> 
> 	To make matters worse, the kernel _knows_ that this is an ill-
> 	advised situation: The kernel prevents itself from clearing the
> 	software representation of the AVX CPUID bit without also
> 	clearing AVX2.
> 
> 	But, the kernel only consults this knowledge when it is clearing
> 	cpu_cap bits.  It does not consult this information when it is
> 	populating those cpu_cap bits.

Yes, I agree. I'll update the changelog with something more in depth.

> 
> > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> > index 4cc79971d2d847..c83a8f447d6aed 100644
> > --- a/arch/x86/kernel/cpu/common.c
> > +++ b/arch/x86/kernel/cpu/common.c
> > @@ -1469,7 +1469,7 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
> >  			this_cpu->c_early_init(c);
> >  
> >  		c->cpu_index = 0;
> > -		filter_cpuid_features(c, false);
> > +		filter_cpuid_features(c, true);
> >  
> >  		if (this_cpu->c_bsp_init)
> >  			this_cpu->c_bsp_init(c);
> > @@ -1757,7 +1757,7 @@ static void identify_cpu(struct cpuinfo_x86 *c)
> >  	 */
> >  
> >  	/* Filter out anything that depends on CPUID levels we don't have */
> > -	filter_cpuid_features(c, true);
> > +	filter_cpuid_features(c, false);
> >  
> >  	/* If the model name is still unset, do table lookup. */
> >  	if (!c->x86_model_id[0]) {
> 
> While we're at it, could we please rid ourselves of this unreadable
> mystery true/false gunk?

It is present if I understand the code correctly to avoid printing a warning twice.
It used to be 'warn' parameter, and I changed it to 'early' parameter, 
inverting its boolean value, because I have seen that warning is not printed at all, 
and I assumed that it is because the first early call already clears the cpuid cap
and the second call doesn't get the warning.

Now however, looking at that I think that the same will
happen with the cpuid level fitering as well, and thus we can just remove that
'warn' parameter.

> 
> > diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
> > index bcb091d02a754b..6d9c0e39851805 100644
> > --- a/arch/x86/kernel/cpu/cpuid-deps.c
> > +++ b/arch/x86/kernel/cpu/cpuid-deps.c
> > @@ -94,6 +94,11 @@ static inline void clear_feature(struct cpuinfo_x86 *c, unsigned int feature)
> >  		set_bit(feature, (unsigned long *)cpu_caps_cleared);
> >  }
> >  
> > +static inline bool test_feature(struct cpuinfo_x86 *c, unsigned int feature)
> > +{
> > +	return test_bit(feature, (unsigned long *)c->x86_capability);
> > +}
> > +
> >  /* Take the capabilities and the BUG bits into account */
> >  #define MAX_FEATURE_BITS ((NCAPINTS + NBUGINTS) * sizeof(u32) * 8)
> >  
> > @@ -127,6 +132,7 @@ void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int feature)
> >  	} while (changed);
> >  }
> >  
> > +
> >  void setup_clear_cpu_cap(unsigned int feature)
> 
> More superfluous whitespace.

Sorry about that, will check better next time.
> 
> >  {
> >  	clear_cpu_cap(&boot_cpu_data, feature);
> > @@ -137,6 +143,10 @@ void setup_clear_cpu_cap(unsigned int feature)
> >   * Some CPU features depend on higher CPUID levels, which may not always
> >   * be available due to CPUID level capping or broken virtualization
> >   * software.  Add those features to this table to auto-disable them.
> > + *
> > + * Also due to configuration bugs, some CPUID features might be present
> > + * while CPUID features that they depend on are not present,
> > + * e.g a AVX2 present but AVX is not present.
> >   */
> >  struct cpuid_dependent_feature {
> >  	u32 feature;
> > @@ -151,9 +161,10 @@ cpuid_dependent_features[] = {
> >  	{ 0, 0 }
> >  };
> >  
> > -void filter_cpuid_features(struct cpuinfo_x86 *c, bool warn)
> > +void filter_cpuid_features(struct cpuinfo_x86 *c, bool early)
> >  {
> 
> I have at least an inkling what 'warn' could mean.  But, 'early'?  One
> man's 'early' is another one's 'late'.

I understand what you mean, as I said above, I will try to reproduce
the original issue of cpuid level mismatch and see if I can remove
the warn parameter at all.


> 
> >  	const struct cpuid_dependent_feature *df;
> > +	const struct cpuid_dep *d;
> >  
> >  	for (df = cpuid_dependent_features; df->feature; df++) {
> >  
> > @@ -172,10 +183,22 @@ void filter_cpuid_features(struct cpuinfo_x86 *c, bool warn)
> >  			continue;
> >  
> >  		clear_cpu_cap(c, df->feature);
> > -		if (!warn)
> > +		if (early)
> >  			continue;
> 
> Why is it that 'early' calls don't want warnings?

I don't know to be honest, except that I assumed that this
allows to not print the warning twice, but as I said above,
I might be able to just remove that code.


> 
> >  		pr_warn("CPU: CPU feature " X86_CAP_FMT " disabled, no CPUID level 0x%x\n",
> >  			x86_cap_flag(df->feature), df->level);
> >  	}
> > +
> > +	for (d = cpuid_deps; d->feature; d++) {
> > +
> > +		if (!test_feature(c, d->feature) || test_feature(c, d->depends))
> > +			continue;
> > +
> > +		clear_feature(c, d->feature);
> > +
> > +		pr_warn("CPU: CPU feature " X86_CAP_FMT " disabled, because it depends on "
> > +			X86_CAP_FMT " which is not supported in CPUID\n",
> > +			x86_cap_flag(d->feature), x86_cap_flag(d->depends));
> > +	}
> >  }
> 
> The do_clear_cpu_cap() does this with a loop, presumably because a later
> (higher index in the array) feature in cpuid_deps[] could theoretically
> clear an earlier (lower index) feature.

Sorry this is my silly mistake. I intended to call clear_cpu_cap here,
which will if needed disable all the depedencies, so a loop doesn't
seem to be needed here. 

It's not very efficient but this is only done once per vCPU so shouldn't matter.


> 
> Also, is that message strictly correct?  There might have been a
> clearcpuid= argument or even another dependency that ended up clearing a
> bit.  It might have nothing to do with CPUID itself.

I think it should work, because clearcpuid= will end up calling clear_cpu_cap
which will disable both the requested feature and everything that depends on
it, thus filter_cpuid_features should not notice any inconsistencies.

Other way around, if the clear_cpu_cap is called before filter_cpuid_features,
it might 'fix' the inconsistency, and silence the warning but that isn't an
issue IMHO.


Thanks a lot for the review,
Best regards,
	Maxim Levitsky

> 

Re: [PATCH 4/4] x86/cpuid: check for dependencies violations in CPUID and attempt to fix them

[Dave Hansen]

On 6/22/22 10:09, Maxim Levitsky wrote:
> On Wed, 2022-06-22 at 08:32 -0700, Dave Hansen wrote:
>> On 6/22/22 07:48, Maxim Levitsky wrote:
>>> Due to configuration bugs, sometimes a CPU feature is disabled in CPUID,
>>> but not features that depend on it.
>>>
>>> While the above is not supported, the kernel should try to not crash,
>>> and clearing the dependent cpu caps is the best way to do it.
>>
>> That's a rather paltry changelog.
>>
>> If I remember correctly, there's a crystal clear problem:
>>
>> 	If a CPU enumerates support for AVX2 but AVX via CPUID, the
>> 	kernel crashes.
>>
>> There's also a follow-on problem.  The kernel has all the data it needs
>> to fix this, but just doesn't consult it:
>>
>> 	To make matters worse, the kernel _knows_ that this is an ill-
>> 	advised situation: The kernel prevents itself from clearing the
>> 	software representation of the AVX CPUID bit without also
>> 	clearing AVX2.
>>
>> 	But, the kernel only consults this knowledge when it is clearing
>> 	cpu_cap bits.  It does not consult this information when it is
>> 	populating those cpu_cap bits.
> 
> Yes, I agree. I'll update the changelog with something more in depth.
> 
>>
>>> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
>>> index 4cc79971d2d847..c83a8f447d6aed 100644
>>> --- a/arch/x86/kernel/cpu/common.c
>>> +++ b/arch/x86/kernel/cpu/common.c
>>> @@ -1469,7 +1469,7 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
>>>  			this_cpu->c_early_init(c);
>>>  
>>>  		c->cpu_index = 0;
>>> -		filter_cpuid_features(c, false);
>>> +		filter_cpuid_features(c, true);
>>>  
>>>  		if (this_cpu->c_bsp_init)
>>>  			this_cpu->c_bsp_init(c);
>>> @@ -1757,7 +1757,7 @@ static void identify_cpu(struct cpuinfo_x86 *c)
>>>  	 */
>>>  
>>>  	/* Filter out anything that depends on CPUID levels we don't have */
>>> -	filter_cpuid_features(c, true);
>>> +	filter_cpuid_features(c, false);
>>>  
>>>  	/* If the model name is still unset, do table lookup. */
>>>  	if (!c->x86_model_id[0]) {
>>
>> While we're at it, could we please rid ourselves of this unreadable
>> mystery true/false gunk?
> 
> It is present if I understand the code correctly to avoid printing a warning twice.
> It used to be 'warn' parameter, and I changed it to 'early' parameter, 
> inverting its boolean value, because I have seen that warning is not printed at all, 
> and I assumed that it is because the first early call already clears the cpuid cap
> and the second call doesn't get the warning.

If the goal is truly to suppress the warning once per dependency, it
would be trivial to do:

	struct cpuid_dep {
	        unsigned int    feature;
        	unsigned int    depends;
+		bool warned;
	};

Then:

	...
	clear_feature(c, d->feature);
+	if (!d->warned)
+		pr_warn(...);
+	d->warned = 1;

You could even have two bits if you feel that we need separate warnings
for the hardware (true CPUID) and software (setup_clear_cpu_cap()) checks.

>>>  		pr_warn("CPU: CPU feature " X86_CAP_FMT " disabled, no CPUID level 0x%x\n",
>>>  			x86_cap_flag(df->feature), df->level);
>>>  	}
>>> +
>>> +	for (d = cpuid_deps; d->feature; d++) {
>>> +
>>> +		if (!test_feature(c, d->feature) || test_feature(c, d->depends))
>>> +			continue;
>>> +
>>> +		clear_feature(c, d->feature);
>>> +
>>> +		pr_warn("CPU: CPU feature " X86_CAP_FMT " disabled, because it depends on "
>>> +			X86_CAP_FMT " which is not supported in CPUID\n",
>>> +			x86_cap_flag(d->feature), x86_cap_flag(d->depends));
>>> +	}
>>>  }
>>
>> The do_clear_cpu_cap() does this with a loop, presumably because a later
>> (higher index in the array) feature in cpuid_deps[] could theoretically
>> clear an earlier (lower index) feature.
> 
> Sorry this is my silly mistake. I intended to call clear_cpu_cap here,
> which will if needed disable all the depedencies, so a loop doesn't
> seem to be needed here. 
> 
> It's not very efficient but this is only done once per vCPU so shouldn't matter.

Right, and it only loops if it is actually clearing features, which is
arguably the result of a broken CPU or hypervisor, which is rare too.
It's not inefficient in any case that matters.