[PATCH v3 1/2] x86/cpufeature: Add feature dependency checks

[PATCH v3 1/2] x86/cpufeature: Add feature dependency checks

[Sohil Mehta]

Currently, the cpuid_deps[] table is only exercised when a particular
feature gets explicitly disabled and clear_cpu_cap() is called. However,
some of these listed dependencies might already be missing during boot.

These types of errors shouldn't generally happen in production
environments but they could sometimes sneak through, especially when VMs
and Kconfigs are in the mix. Also, the kernel might introduce artificial
dependencies between unrelated features such as making LAM depend on
LASS.

Unexpected failures can occur when the kernel tries to use such a
feature. Rather than debug such scenarios, it would be better to disable
the feature upfront.

Add a simple boot time scan of the cpuid_deps[] table and disable any
feature with unmet dependencies. do_clear_cpu_cap() makes sure that the
dependency tree reaches a stable state in the end.

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
---
v3: Picked up the review tag.
    Reworded the commit message.

v2: Use cpu_has() instead of boot_cpu_has() (Sean)
    https://lore.kernel.org/lkml/20241030233118.615493-1-sohil.mehta@intel.com/

RFC-v1: New patch.
---
 arch/x86/include/asm/cpufeature.h |  1 +
 arch/x86/kernel/cpu/common.c      |  4 ++++
 arch/x86/kernel/cpu/cpuid-deps.c  | 10 ++++++++++
 3 files changed, 15 insertions(+)

diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index 0b9611da6c53..8821336a6c73 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -148,6 +148,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);
+void filter_feature_dependencies(struct cpuinfo_x86 *c);
 
 #define setup_force_cpu_cap(bit) do {			\
 							\
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index a5c28975c608..bd27cf5974d4 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1608,6 +1608,7 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 
 		c->cpu_index = 0;
 		filter_cpuid_features(c, false);
+		filter_feature_dependencies(c);
 
 		if (this_cpu->c_bsp_init)
 			this_cpu->c_bsp_init(c);
@@ -1862,6 +1863,9 @@ 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 out features that don't have their dependencies met */
+	filter_feature_dependencies(c);
+
 	/* If the model name is still unset, do table lookup. */
 	if (!c->x86_model_id[0]) {
 		const char *p;
diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index 8bd84114c2d9..8bea5c5e4fd2 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -146,3 +146,13 @@ void setup_clear_cpu_cap(unsigned int feature)
 {
 	do_clear_cpu_cap(NULL, feature);
 }
+
+void filter_feature_dependencies(struct cpuinfo_x86 *c)
+{
+	const struct cpuid_dep *d;
+
+	for (d = cpuid_deps; d->feature; d++) {
+		if (cpu_has(c, d->feature) && !cpu_has(c, d->depends))
+			do_clear_cpu_cap(c, d->feature);
+	}
+}
-- 
2.43.0

[PATCH v3 2/2] x86/cpufeature: Add a debug print for unmet dependencies

[Sohil Mehta]

Instead of silently disabling features, add a print which might be
useful to users if their favorite feature gets unexpectedly disabled.

Features are typically represented through unsigned integers though some
of them have user friendly names if they are exposed via cpuinfo.  Show
the friendlier name if available otherwise display the X86_FEATURE_*
numerals to make it easier to identify the feature.

Use pr_debug() to avoid spamming the kernel log and generating false
alarms. Note that the print will occur once per disabled feature on
every CPU. Show this information only when someone is really looking for
it.

Suggested-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
---
Sent as a separate patch to make it easier to review and drop if it
feels unnecessary.

I can see both sides of the argument. The pr_debug() serves a
compromise between the two.

v3: New patch.

---
 arch/x86/kernel/cpu/cpuid-deps.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index 8bea5c5e4fd2..c72f2dd77d72 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -147,12 +147,32 @@ void setup_clear_cpu_cap(unsigned int feature)
 	do_clear_cpu_cap(NULL, feature);
 }
 
+/*
+ * Return the feature "name" if available otherwise return
+ * the X86_FEATURE_* numerals to make it easier to identify
+ * the feature.
+ */
+static const char *x86_feature_name(unsigned int feature, char *buf)
+{
+	if (x86_cap_flags[feature])
+		return x86_cap_flags[feature];
+
+	snprintf(buf, 12, "%d*32+%2d", feature / 32, feature % 32);
+
+	return buf;
+}
+
 void filter_feature_dependencies(struct cpuinfo_x86 *c)
 {
+	char feature_buf[12], depends_buf[12];
 	const struct cpuid_dep *d;
 
 	for (d = cpuid_deps; d->feature; d++) {
-		if (cpu_has(c, d->feature) && !cpu_has(c, d->depends))
+		if (cpu_has(c, d->feature) && !cpu_has(c, d->depends)) {
+			pr_debug("x86/cpu: Disabling feature %s since feature %s is missing\n",
+				 x86_feature_name(d->feature, feature_buf),
+				 x86_feature_name(d->depends, depends_buf));
 			do_clear_cpu_cap(c, d->feature);
+		}
 	}
 }
-- 
2.43.0

Re: [PATCH v3 2/2] x86/cpufeature: Add a debug print for unmet dependencies

[Ingo Molnar]

* Sohil Mehta <sohil.mehta@intel.com> wrote:

> Instead of silently disabling features, add a print which might be
> useful to users if their favorite feature gets unexpectedly disabled.
> 
> Features are typically represented through unsigned integers though some
> of them have user friendly names if they are exposed via cpuinfo.  Show
> the friendlier name if available otherwise display the X86_FEATURE_*
> numerals to make it easier to identify the feature.
> 
> Use pr_debug() to avoid spamming the kernel log and generating false
> alarms. Note that the print will occur once per disabled feature on
> every CPU. Show this information only when someone is really looking for
> it.
> 
> Suggested-by: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
> ---
> Sent as a separate patch to make it easier to review and drop if it
> feels unnecessary.
> 
> I can see both sides of the argument. The pr_debug() serves a
> compromise between the two.
> 
> v3: New patch.
> 
> ---
>  arch/x86/kernel/cpu/cpuid-deps.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
> index 8bea5c5e4fd2..c72f2dd77d72 100644
> --- a/arch/x86/kernel/cpu/cpuid-deps.c
> +++ b/arch/x86/kernel/cpu/cpuid-deps.c
> @@ -147,12 +147,32 @@ void setup_clear_cpu_cap(unsigned int feature)
>  	do_clear_cpu_cap(NULL, feature);
>  }
>  
> +/*
> + * Return the feature "name" if available otherwise return
> + * the X86_FEATURE_* numerals to make it easier to identify
> + * the feature.
> + */
> +static const char *x86_feature_name(unsigned int feature, char *buf)
> +{
> +	if (x86_cap_flags[feature])
> +		return x86_cap_flags[feature];
> +
> +	snprintf(buf, 12, "%d*32+%2d", feature / 32, feature % 32);
> +
> +	return buf;
> +}
> +
>  void filter_feature_dependencies(struct cpuinfo_x86 *c)
>  {
> +	char feature_buf[12], depends_buf[12];
>  	const struct cpuid_dep *d;
>  
>  	for (d = cpuid_deps; d->feature; d++) {
> -		if (cpu_has(c, d->feature) && !cpu_has(c, d->depends))
> +		if (cpu_has(c, d->feature) && !cpu_has(c, d->depends)) {
> +			pr_debug("x86/cpu: Disabling feature %s since feature %s is missing\n",
> +				 x86_feature_name(d->feature, feature_buf),
> +				 x86_feature_name(d->depends, depends_buf));
>  			do_clear_cpu_cap(c, d->feature);

So why not make this a pr_info() at minimum?

Since this new logic will disable certain feature bits on random 
machines, I'm sure there will be some surprises. I'd sure like to see 
this printed in the system log of my machine if it happens.

It might also inform firmware & distro testers that something wasn't 
set up properly on the firmware (or virtualization) side.

Thanks,

	Ingo

Re: [PATCH v3 2/2] x86/cpufeature: Add a debug print for unmet dependencies

[Sohil Mehta]

On 12/7/2024 12:35 AM, Ingo Molnar wrote:
>>  void filter_feature_dependencies(struct cpuinfo_x86 *c)
>>  {
>> +	char feature_buf[12], depends_buf[12];
>>  	const struct cpuid_dep *d;
>>  
>>  	for (d = cpuid_deps; d->feature; d++) {
>> -		if (cpu_has(c, d->feature) && !cpu_has(c, d->depends))
>> +		if (cpu_has(c, d->feature) && !cpu_has(c, d->depends)) {
>> +			pr_debug("x86/cpu: Disabling feature %s since feature %s is missing\n",
>> +				 x86_feature_name(d->feature, feature_buf),
>> +				 x86_feature_name(d->depends, depends_buf));
>>  			do_clear_cpu_cap(c, d->feature);
> 
> So why not make this a pr_info() at minimum?
> 

I was hesitant because the feature disabling may be inconsequential to
the end user. Also, the printed numbers would not make sense unless they
have the kernel source handy. But maybe it's better to inform and let
the user decide.

> Since this new logic will disable certain feature bits on random 
> machines, I'm sure there will be some surprises. I'd sure like to see 
> this printed in the system log of my machine if it happens.
> 

Sure, will update the print as follows in the next (hopefully final)
version.

pr_info("CPU%d: Disabling feature %s due to missing feature %s\n",
	smp_processor_id(),
	x86_feature_name(d->feature, feature_buf),
	x86_feature_name(d->depends, depends_buf));

Since it seems almost certain that we are going in this direction will
merge this patch into the previous one and send a combined one next time.

> It might also inform firmware & distro testers that something wasn't 
> set up properly on the firmware (or virtualization) side.
> 

Sohil

Re: [PATCH v3 2/2] x86/cpufeature: Add a debug print for unmet dependencies

[H. Peter Anvin]

On December 6, 2024 4:41:26 PM PST, Sohil Mehta <sohil.mehta@intel.com> wrote:
>Instead of silently disabling features, add a print which might be
>useful to users if their favorite feature gets unexpectedly disabled.
>
>Features are typically represented through unsigned integers though some
>of them have user friendly names if they are exposed via cpuinfo.  Show
>the friendlier name if available otherwise display the X86_FEATURE_*
>numerals to make it easier to identify the feature.
>
>Use pr_debug() to avoid spamming the kernel log and generating false
>alarms. Note that the print will occur once per disabled feature on
>every CPU. Show this information only when someone is really looking for
>it.
>
>Suggested-by: Tony Luck <tony.luck@intel.com>
>Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
>---
>Sent as a separate patch to make it easier to review and drop if it
>feels unnecessary.
>
>I can see both sides of the argument. The pr_debug() serves a
>compromise between the two.
>
>v3: New patch.
>
>---
> arch/x86/kernel/cpu/cpuid-deps.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
>diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
>index 8bea5c5e4fd2..c72f2dd77d72 100644
>--- a/arch/x86/kernel/cpu/cpuid-deps.c
>+++ b/arch/x86/kernel/cpu/cpuid-deps.c
>@@ -147,12 +147,32 @@ void setup_clear_cpu_cap(unsigned int feature)
> 	do_clear_cpu_cap(NULL, feature);
> }
> 
>+/*
>+ * Return the feature "name" if available otherwise return
>+ * the X86_FEATURE_* numerals to make it easier to identify
>+ * the feature.
>+ */
>+static const char *x86_feature_name(unsigned int feature, char *buf)
>+{
>+	if (x86_cap_flags[feature])
>+		return x86_cap_flags[feature];
>+
>+	snprintf(buf, 12, "%d*32+%2d", feature / 32, feature % 32);
>+
>+	return buf;
>+}
>+
> void filter_feature_dependencies(struct cpuinfo_x86 *c)
> {
>+	char feature_buf[12], depends_buf[12];
> 	const struct cpuid_dep *d;
> 
> 	for (d = cpuid_deps; d->feature; d++) {
>-		if (cpu_has(c, d->feature) && !cpu_has(c, d->depends))
>+		if (cpu_has(c, d->feature) && !cpu_has(c, d->depends)) {
>+			pr_debug("x86/cpu: Disabling feature %s since feature %s is missing\n",
>+				 x86_feature_name(d->feature, feature_buf),
>+				 x86_feature_name(d->depends, depends_buf));
> 			do_clear_cpu_cap(c, d->feature);
>+		}
> 	}
> }

Ok, I realize that the x86 maintainers **very legitimately** don't want more crap in /proc/cpuinfo, but perhaps we could include the strings for printing debug messages in cleartext? Add a bitmap for which entries should go into /proc/cpuinfo.

Re: [PATCH v3 2/2] x86/cpufeature: Add a debug print for unmet dependencies

[Sohil Mehta]

On 12/7/2024 1:11 PM, H. Peter Anvin wrote:
> 
> Ok, I realize that the x86 maintainers **very legitimately** don't
> want more crap in /proc/cpuinfo, but perhaps we could include the
> strings for printing debug messages in cleartext? Add a bitmap for
> which entries should go into /proc/cpuinfo.
Could the cpuid-db project be helpful at some point in this regard? It
seems to have a name ("id") for each feature and a tracking of whether
each feature is exposed via cpuinfo and with what name.

> <bit20 len="1"  id="cet_ibt"                 desc="CET indirect branch tracking">
>   <linux        feature="true"               proc="true"            altid="ibt" />
> </bit20>


Though I am not sure if it only tracks the hardware originating features
represented in cpuid or the linux defined features as well?

Re: [PATCH v3 2/2] x86/cpufeature: Add a debug print for unmet dependencies

[kernel test robot]

Hi Sohil,

kernel test robot noticed the following build warnings:

[auto build test WARNING on tip/master]
[also build test WARNING on tip/x86/core tip/auto-latest bp/for-next linus/master v6.13-rc1 next-20241206]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sohil-Mehta/x86-cpufeature-Add-a-debug-print-for-unmet-dependencies/20241207-084543
base:   tip/master
patch link:    https://lore.kernel.org/r/20241207004126.2054658-2-sohil.mehta%40intel.com
patch subject: [PATCH v3 2/2] x86/cpufeature: Add a debug print for unmet dependencies
config: x86_64-randconfig-103-20241207 (https://download.01.org/0day-ci/archive/20241207/202412071926.MHYBbSb1-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241207/202412071926.MHYBbSb1-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412071926.MHYBbSb1-lkp@intel.com/

All warnings (new ones prefixed by >>):

   arch/x86/kernel/cpu/cpuid-deps.c: In function 'filter_feature_dependencies':
>> arch/x86/kernel/cpu/cpuid-deps.c:160:30: warning: '*32+' directive output may be truncated writing 4 bytes into a region of size between 3 and 11 [-Wformat-truncation=]
     160 |         snprintf(buf, 12, "%d*32+%2d", feature / 32, feature % 32);
         |                              ^~~~
   In function 'x86_feature_name',
       inlined from 'x86_feature_name' at arch/x86/kernel/cpu/cpuid-deps.c:155:20,
       inlined from 'filter_feature_dependencies' at arch/x86/kernel/cpu/cpuid-deps.c:172:4:
   arch/x86/kernel/cpu/cpuid-deps.c:160:27: note: directive argument in the range [0, 31]
     160 |         snprintf(buf, 12, "%d*32+%2d", feature / 32, feature % 32);
         |                           ^~~~~~~~~~~
   arch/x86/kernel/cpu/cpuid-deps.c:160:9: note: 'snprintf' output between 8 and 16 bytes into a destination of size 12
     160 |         snprintf(buf, 12, "%d*32+%2d", feature / 32, feature % 32);
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/x86/kernel/cpu/cpuid-deps.c: In function 'filter_feature_dependencies':
>> arch/x86/kernel/cpu/cpuid-deps.c:160:30: warning: '*32+' directive output may be truncated writing 4 bytes into a region of size between 3 and 11 [-Wformat-truncation=]
     160 |         snprintf(buf, 12, "%d*32+%2d", feature / 32, feature % 32);
         |                              ^~~~
   In function 'x86_feature_name',
       inlined from 'x86_feature_name' at arch/x86/kernel/cpu/cpuid-deps.c:155:20,
       inlined from 'filter_feature_dependencies' at arch/x86/kernel/cpu/cpuid-deps.c:172:4:
   arch/x86/kernel/cpu/cpuid-deps.c:160:27: note: directive argument in the range [0, 31]
     160 |         snprintf(buf, 12, "%d*32+%2d", feature / 32, feature % 32);
         |                           ^~~~~~~~~~~
   arch/x86/kernel/cpu/cpuid-deps.c:160:9: note: 'snprintf' output between 8 and 16 bytes into a destination of size 12
     160 |         snprintf(buf, 12, "%d*32+%2d", feature / 32, feature % 32);
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +160 arch/x86/kernel/cpu/cpuid-deps.c

   149	
   150	/*
   151	 * Return the feature "name" if available otherwise return
   152	 * the X86_FEATURE_* numerals to make it easier to identify
   153	 * the feature.
   154	 */
   155	static const char *x86_feature_name(unsigned int feature, char *buf)
   156	{
   157		if (x86_cap_flags[feature])
   158			return x86_cap_flags[feature];
   159	
 > 160		snprintf(buf, 12, "%d*32+%2d", feature / 32, feature % 32);
   161	
   162		return buf;
   163	}
   164	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki