[PATCH v11 0/4] x86: Capability bits fix and required bits sanity check

[PATCH v11 0/4] x86: Capability bits fix and required bits sanity check

[Maciej Wieczor-Retman]

Series aims to fix the inconsistency between the cpuinfo behavior and
the documentation. Specifically the features that are not compiled are
still present in the cpuinfo bitmasks as enabled. This is not in line
with the documentation which specifies that not-compiled features are
not present in /proc/cpuinfo.

Along adding the disabled feature bitmask initializer array, the
complementary required bitmask initializer is also added. It can be used
to provide a sanity check, after the cpu identification is finished, to
make sure every required bit is set in the final bitmask. A warning with
the cpu number and all required bits that were not set is emitted in
case of the sanity check failure.

Before adding the sanity check a small cleanup can be done. Three places
open code an operation that retrieves either a feature string or, if the
string is not present, the feature number in word:bit format. One of
these places also doesn't check whether the string is actually there or
not. The cleanup patch fixes that and simplifies the other two
instances.

Patches are based on v7.0-rc4

Previous patchset versions:
v10: https://lore.kernel.org/all/cover.1773771353.git.m.wieczorretman@pm.me/
v9:  https://lore.kernel.org/all/cover.1773165421.git.m.wieczorretman@pm.me/
v8:  https://lore.kernel.org/all/cover.1772453012.git.m.wieczorretman@pm.me/
v7:  https://lore.kernel.org/all/cover.1771936214.git.m.wieczorretman@pm.me/
v6:  https://lore.kernel.org/all/cover.1771590895.git.m.wieczorretman@pm.me/
v5:  https://lore.kernel.org/all/cover.1770908783.git.m.wieczorretman@pm.me/
v4:  https://lore.kernel.org/all/20250724125346.2792543-1-maciej.wieczor-retman@intel.com/
v3:  https://lore.kernel.org/all/20250724094554.2153919-1-maciej.wieczor-retman@intel.com/
v2:  https://lore.kernel.org/all/20250723092250.3411923-1-maciej.wieczor-retman@intel.com/
v1:  https://lore.kernel.org/all/20250722074439.4069992-1-maciej.wieczor-retman@intel.com/

Maciej Wieczor-Retman (4):
  x86/cpu: Clear feature bits disabled at compile-time
  x86/cpu: Check if feature string is non-zero
  x86/cpu: Do a sanity check on required feature bits
  x86/cpu: Clear feature bits whose dependencies were cleared

 arch/x86/include/asm/cpufeature.h  |  4 ++
 arch/x86/kernel/cpu/common.c       | 69 +++++++++++++++++++++++++++---
 arch/x86/kernel/cpu/cpuid-deps.c   | 31 ++++++--------
 arch/x86/tools/cpufeaturemasks.awk |  6 +++
 4 files changed, 86 insertions(+), 24 deletions(-)

-- 
2.53.0

[PATCH v11 1/4] x86/cpu: Clear feature bits disabled at compile-time

[Maciej Wieczor-Retman]

From: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>

If some config options are disabled during compile time, they still are
enumerated in macros that use the x86_capability bitmask - cpu_has() or
this_cpu_has().

The features are also visible in /proc/cpuinfo even though they are not
enabled - which is contrary to what the documentation states about the
file. Examples of such feature flags are lam, fred, sgx, user_shstk and
enqcmd.

Initialize cpu_caps_cleared[] with an autogenerated disabled bitmask.
During CPU init, apply_forced_caps() will clear the corresponding bits
in struct cpuinfo_x86 for each CPU. Thus features disabled at compile
time won't show up in /proc/cpuinfo.

No BUGS are defined to be cleared at compile time, therefore only the
NCAPINTS part of cpu_caps_cleared[] is initialized using the macro. The
NBUGINTS part is set to zero.

Reported-by: Farrah Chen <farrah.chen@intel.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=220348
Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
---
Changelog v10:
- Remove examples of feature flags that came from stable kernels.
- Redo the patch message a bit with Sohil's suggestions.
- Add Sohil's Reviewed-by tag.

Changelog v9:
- *_MASK_INITIALIZER -> *_MASK_INIT
- Remove Cc stable.
- Note that the BUGS part of cpu_caps_cleared[] is zeroed.

Changelog v6:
- Remove patch message portions that are not just describing the diff.

 arch/x86/kernel/cpu/common.c       | 3 ++-
 arch/x86/tools/cpufeaturemasks.awk | 6 ++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index a8ff4376c286..76339e988304 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -735,7 +735,8 @@ static const char *table_lookup_model(struct cpuinfo_x86 *c)
 }
 
 /* Aligned to unsigned long to avoid split lock in atomic bitmap ops */
-__u32 cpu_caps_cleared[NCAPINTS + NBUGINTS] __aligned(sizeof(unsigned long));
+__u32 cpu_caps_cleared[NCAPINTS + NBUGINTS] __aligned(sizeof(unsigned long)) =
+	DISABLED_MASK_INIT;
 __u32 cpu_caps_set[NCAPINTS + NBUGINTS] __aligned(sizeof(unsigned long));
 
 #ifdef CONFIG_X86_32
diff --git a/arch/x86/tools/cpufeaturemasks.awk b/arch/x86/tools/cpufeaturemasks.awk
index 173d5bf2d999..9382bd15279a 100755
--- a/arch/x86/tools/cpufeaturemasks.awk
+++ b/arch/x86/tools/cpufeaturemasks.awk
@@ -82,6 +82,12 @@ END {
 		}
 		printf " 0\t\\\n";
 		printf "\t) & (1U << ((x) & 31)))\n\n";
+
+		printf "\n#define %s_MASK_INIT\t\t\t\\", s;
+		printf "\n\t{\t\t\t\t\t\t\\";
+		for (i = 0; i < ncapints; i++)
+			printf "\n\t\t%s_MASK%d,\t\t\t\\", s, i;
+		printf "\n\t}\n\n";
 	}
 
 	printf "#endif /* _ASM_X86_CPUFEATUREMASKS_H */\n";
-- 
2.53.0

[PATCH v11 2/4] x86/cpu: Check if feature string is non-zero

[Maciej Wieczor-Retman]

From: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>

In filter_cpuid_features(), x86_cap_flags[] is read, but it's not verified
whether the string is non-zero which could lead to unwanted output.

In two more places there are open coded paths that try to retrieve a
feature string, and if there isn't one, the feature word and bit are
returned instead.

Add a common helper to fix filter_cpuid_features() as well as clean up
the open coded cases.

Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Reviewed-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
---
Changelog v11:
- Remove one extra tab after X86_CAP_BUF_SIZE.
- Add Reviewed-by tags from Sohil and Pawan.

Changelog v10:
- Reword the patch message a bit.
- Move x86_cap_name() declaration and X86_CAP_BUF_SIZE define to
  asm/cpufeature.h.
- Don't include asm/cpu.h in cpuid-deps.c
- Extend the comment above x86_cap_name to include information on buffer
  size that needs to be prepared before calling the function.
- Remove the likely(), unlikely() calls since this is not a hot path.

Changelog v9:
- 16 -> X86_CAP_BUF_SIZE.
- Add comment to the x86_cap_name().

Changelog v8:
- Move x86_cap_name() declaration from linux/cpu.h to the arch/cpu.h.
  Include arch/cpu.h in the cpuid-deps.c file instead of linux/cpu.h.

Changelog v7:
- sizeof(buf) -> 16
- Rebase onto 7.01-rc1.

Changelog v6:
- Remove parts of the patch message that are redundant and just copy
  what's visible in the diff.
- Redo the helper to use an external char buffer instead of a local
  static string.

 arch/x86/include/asm/cpufeature.h |  4 ++++
 arch/x86/kernel/cpu/common.c      | 32 ++++++++++++++++++++++++++-----
 arch/x86/kernel/cpu/cpuid-deps.c  | 21 +++-----------------
 3 files changed, 34 insertions(+), 23 deletions(-)

diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index 3ddc1d33399b..0698a6638463 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -138,5 +138,9 @@ static __always_inline bool _static_cpu_has(u16 bit)
 #define CPU_FEATURE_TYPEVAL		boot_cpu_data.x86_vendor, boot_cpu_data.x86, \
 					boot_cpu_data.x86_model
 
+#define X86_CAP_BUF_SIZE		16
+
+const char *x86_cap_name(unsigned int bit, char *buf);
+
 #endif /* defined(__KERNEL__) && !defined(__ASSEMBLER__) */
 #endif /* _ASM_X86_CPUFEATURE_H */
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 76339e988304..0e318f3d56cb 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -678,6 +678,7 @@ cpuid_dependent_features[] = {
 static void filter_cpuid_features(struct cpuinfo_x86 *c, bool warn)
 {
 	const struct cpuid_dependent_feature *df;
+	char feature_buf[X86_CAP_BUF_SIZE];
 
 	for (df = cpuid_dependent_features; df->feature; df++) {
 
@@ -700,7 +701,7 @@ static void filter_cpuid_features(struct cpuinfo_x86 *c, bool warn)
 			continue;
 
 		pr_warn("CPU: CPU feature %s disabled, no CPUID level 0x%x\n",
-			x86_cap_flags[df->feature], df->level);
+			x86_cap_name(df->feature, feature_buf), df->level);
 	}
 }
 
@@ -1637,6 +1638,7 @@ static inline bool parse_set_clear_cpuid(char *arg, bool set)
 
 	while (arg) {
 		bool found __maybe_unused = false;
+		char name_buf[X86_CAP_BUF_SIZE];
 		unsigned int bit;
 
 		opt = strsep(&arg, ",");
@@ -1657,10 +1659,7 @@ static inline bool parse_set_clear_cpuid(char *arg, bool set)
 					setup_clear_cpu_cap(bit);
 				}
 				/* empty-string, i.e., ""-defined feature flags */
-				if (!x86_cap_flags[bit])
-					pr_cont(" %d:%d\n", bit >> 5, bit & 31);
-				else
-					pr_cont(" %s\n", x86_cap_flags[bit]);
+				pr_cont(" %s\n", x86_cap_name(bit, name_buf));
 
 				taint++;
 			}
@@ -1983,6 +1982,29 @@ static void generic_identify(struct cpuinfo_x86 *c)
 #endif
 }
 
+/*
+ * Return the feature "name" if available, otherwise return the
+ * X86_FEATURE_* numerals to make it easier to identify the feature.
+ * Callers of this function need to pass a char * buffer of size
+ * X86_CAP_BUF_SIZE.
+ */
+const char *x86_cap_name(unsigned int bit, char *buf)
+{
+	unsigned int word = bit >> 5;
+	const char *name = NULL;
+
+	if (word < NCAPINTS)
+		name = x86_cap_flags[bit];
+	else if (word < NCAPINTS + NBUGINTS)
+		name = x86_bug_flags[bit - 32 * NCAPINTS];
+
+	if (name)
+		return name;
+
+	snprintf(buf, X86_CAP_BUF_SIZE, "%u:%u", word, bit & 31);
+	return buf;
+}
+
 /*
  * This does the hard work of actually picking apart the CPU stuff...
  */
diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index 146f6f8b0650..5002f496d095 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -156,24 +156,9 @@ 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, 16, "%d*32+%2d", feature / 32, feature % 32);
-
-	return buf;
-}
-
 void check_cpufeature_deps(struct cpuinfo_x86 *c)
 {
-	char feature_buf[16], depends_buf[16];
+	char feature_buf[X86_CAP_BUF_SIZE], depends_buf[X86_CAP_BUF_SIZE];
 	const struct cpuid_dep *d;
 
 	for (d = cpuid_deps; d->feature; d++) {
@@ -185,8 +170,8 @@ void check_cpufeature_deps(struct cpuinfo_x86 *c)
 			 */
 			pr_warn_once("x86 CPU feature dependency check failure: CPU%d has '%s' enabled but '%s' disabled. Kernel might be fine, but no guarantees.\n",
 				     smp_processor_id(),
-				     x86_feature_name(d->feature, feature_buf),
-				     x86_feature_name(d->depends, depends_buf));
+				     x86_cap_name(d->feature, feature_buf),
+				     x86_cap_name(d->depends, depends_buf));
 		}
 	}
 }
-- 
2.53.0

[PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[Maciej Wieczor-Retman]

From: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>

After CPU identification concludes, do a sanity check by comparing the
final x86_capability bitmask with the pre-defined required feature bits.

Because for_each_set_bit() expects 64-bit values and feature bitmasks
are 32-bit use DECLARE_BITMAP() to avoid unaligned memory accesses if
NCAPINTS is ever an odd number.

Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
---
Changelog v11:
- Use DECLARE_BITMAP() on missing[] and recast to a u32 pointer for
  iterating. Do it to avoid unaligned memory accesses when later using
  for_each_set_bit() if NCAPINTS is going to ever become an odd number.
  missing[] was previously an u32 array and for_each_set_bit() works on
  unsigned long chunks of memory.
- Add a paragraph about the above to the patch message.
- Remove Peter's acked-by due to more changes.

Changelog v10:
- Shorten the comment before the sanity check.
- cpu -> CPU in the warning.
- NCAPINTS << 5 -> NCAPINTS * 32

Changelog v9:
- REQUIRED_MASK_INITIALIZER -> REQUIRED_MASK_INIT
- Redo the comments.
- Fix reverse xmas order.
- Inside for_each_set_bit: (void *) -> (unsigned long *).
- 16 -> X86_CAP_BUF_SIZE.

Changelog v6:
- Add Peter's acked-by tag.
- Rename patch subject to imperative form.
- Add a char buffer to the x86_cap_name() call.

 arch/x86/kernel/cpu/common.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 0e318f3d56cb..92159a0963c8 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2005,6 +2005,38 @@ const char *x86_cap_name(unsigned int bit, char *buf)
 	return buf;
 }
 
+/*
+ * As a sanity check compare the final x86_capability bitmask with the initial
+ * predefined required feature bits.
+ */
+static void verify_required_features(const struct cpuinfo_x86 *c)
+{
+	u32 required_features[NCAPINTS] = REQUIRED_MASK_INIT;
+	DECLARE_BITMAP(missing, NCAPINTS * 32);
+	char cap_buf[X86_CAP_BUF_SIZE];
+	u32 *missing_u32;
+	unsigned int i;
+	u32 error = 0;
+
+	memset(missing, 0, sizeof(missing));
+	missing_u32 = (u32 *)missing;
+
+	for (i = 0; i < NCAPINTS; i++) {
+		missing_u32[i] = ~c->x86_capability[i] & required_features[i];
+		error |= missing_u32[i];
+	}
+
+	if (!error)
+		return;
+
+	/* At least one required feature is missing */
+	pr_warn("CPU %d: missing required feature(s):", c->cpu_index);
+	for_each_set_bit(i, missing, NCAPINTS * 32)
+		pr_cont(" %s", x86_cap_name(i, cap_buf));
+	pr_cont("\n");
+	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
+}
+
 /*
  * This does the hard work of actually picking apart the CPU stuff...
  */
@@ -2134,6 +2166,8 @@ static void identify_cpu(struct cpuinfo_x86 *c)
 	mcheck_cpu_init(c);
 
 	numa_add_cpu(smp_processor_id());
+
+	verify_required_features(c);
 }
 
 /*
-- 
2.53.0

[PATCH v11 4/4] x86/cpu: Clear feature bits whose dependencies were cleared

[Maciej Wieczor-Retman]

From: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>

After cpu_caps_cleared[] is initialized with DISABLED_MASK_INIT,
features present in disabled bitmasks are cleared from x86_capability[].
However features that depend on them and are not part of any disabled
mask are not cleared by anything. They can trigger the warning in
check_cpufeature_deps(), as before both features would show up as
enabled even though they weren't. The uncleared features can also still
falsely show up in /proc/cpuinfo.

Running setup_clear_cpu_cap() on such cases inside
check_cpufeature_deps() should guarantee that the cleanup of the
leftover bits has to run only once. Afterwards apply_forced_caps()
clears the leftover bits from x86_capability[] and the warning inside
check_cpufeature_deps() doesn't trigger anymore.

Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>

---
Changelog v11:
- Add this patch to the series.

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

diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index 5002f496d095..b0f5d3fe6655 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -163,6 +163,16 @@ void check_cpufeature_deps(struct cpuinfo_x86 *c)
 
 	for (d = cpuid_deps; d->feature; d++) {
 		if (cpu_has(c, d->feature) && !cpu_has(c, d->depends)) {
+			/*
+			 * If the dependency was cleared through the disabled
+			 * bitmasks while the feature wasn't it also needs to be
+			 * cleared.
+			 */
+			if (!DISABLED_MASK_BIT_SET(d->feature) && DISABLED_MASK_BIT_SET(d->depends)) {
+				setup_clear_cpu_cap(d->feature);
+				continue;
+			}
+
 			/*
 			 * Only warn about the first unmet dependency on the
 			 * first CPU where it is encountered to avoid spamming
-- 
2.53.0

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[Pawan Gupta]

On Fri, Mar 20, 2026 at 12:50:25PM +0000, Maciej Wieczor-Retman wrote:
...
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 0e318f3d56cb..92159a0963c8 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -2005,6 +2005,38 @@ const char *x86_cap_name(unsigned int bit, char *buf)
>  	return buf;
>  }
>  
> +/*
> + * As a sanity check compare the final x86_capability bitmask with the initial
> + * predefined required feature bits.
> + */
> +static void verify_required_features(const struct cpuinfo_x86 *c)
> +{
> +	u32 required_features[NCAPINTS] = REQUIRED_MASK_INIT;
> +	DECLARE_BITMAP(missing, NCAPINTS * 32);
> +	char cap_buf[X86_CAP_BUF_SIZE];
> +	u32 *missing_u32;
> +	unsigned int i;
> +	u32 error = 0;
> +
> +	memset(missing, 0, sizeof(missing));
> +	missing_u32 = (u32 *)missing;
> +
> +	for (i = 0; i < NCAPINTS; i++) {
> +		missing_u32[i] = ~c->x86_capability[i] & required_features[i];
> +		error |= missing_u32[i];
> +	}
> +
> +	if (!error)
> +		return;
> +
> +	/* At least one required feature is missing */
> +	pr_warn("CPU %d: missing required feature(s):", c->cpu_index);
> +	for_each_set_bit(i, missing, NCAPINTS * 32)
> +		pr_cont(" %s", x86_cap_name(i, cap_buf));
> +	pr_cont("\n");
> +	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
> +}

Do we need 2 loops? Can this be simplified as below:

static void verify_required_features(const struct cpuinfo_x86 *c)
{
	u32 required_features[NCAPINTS + 1] = REQUIRED_MASK_INIT;
	char cap_buf[X86_CAP_BUF_SIZE];
	int i, error = 0;

	for_each_set_bit(i, (unsigned long *)required_features, NCAPINTS * 32) {
		if (test_bit(i, (unsigned long *)c->x86_capability))
			continue;
		if (!error)
			pr_warn("CPU %d: missing required feature(s):", c->cpu_index);
		pr_cont(" %s", x86_cap_name(i, cap_buf));
		error = 1;
	}

	if (!error)
		return;

	pr_cont("\n");
	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
}

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[Maciej Wieczór-Retman]

On 2026-03-20 at 17:31:27 -0700, Pawan Gupta wrote:
>On Fri, Mar 20, 2026 at 12:50:25PM +0000, Maciej Wieczor-Retman wrote:
>...
>> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
>> index 0e318f3d56cb..92159a0963c8 100644
>> --- a/arch/x86/kernel/cpu/common.c
>> +++ b/arch/x86/kernel/cpu/common.c
>> @@ -2005,6 +2005,38 @@ const char *x86_cap_name(unsigned int bit, char *buf)
>>  	return buf;
>>  }
>>
>> +/*
>> + * As a sanity check compare the final x86_capability bitmask with the initial
>> + * predefined required feature bits.
>> + */
>> +static void verify_required_features(const struct cpuinfo_x86 *c)
>> +{
>> +	u32 required_features[NCAPINTS] = REQUIRED_MASK_INIT;
>> +	DECLARE_BITMAP(missing, NCAPINTS * 32);
>> +	char cap_buf[X86_CAP_BUF_SIZE];
>> +	u32 *missing_u32;
>> +	unsigned int i;
>> +	u32 error = 0;
>> +
>> +	memset(missing, 0, sizeof(missing));
>> +	missing_u32 = (u32 *)missing;
>> +
>> +	for (i = 0; i < NCAPINTS; i++) {
>> +		missing_u32[i] = ~c->x86_capability[i] & required_features[i];
>> +		error |= missing_u32[i];
>> +	}
>> +
>> +	if (!error)
>> +		return;
>> +
>> +	/* At least one required feature is missing */
>> +	pr_warn("CPU %d: missing required feature(s):", c->cpu_index);
>> +	for_each_set_bit(i, missing, NCAPINTS * 32)
>> +		pr_cont(" %s", x86_cap_name(i, cap_buf));
>> +	pr_cont("\n");
>> +	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
>> +}
>
>Do we need 2 loops? Can this be simplified as below:
>
>static void verify_required_features(const struct cpuinfo_x86 *c)
>{
>	u32 required_features[NCAPINTS + 1] = REQUIRED_MASK_INIT;
>	char cap_buf[X86_CAP_BUF_SIZE];
>	int i, error = 0;
>
>	for_each_set_bit(i, (unsigned long *)required_features, NCAPINTS * 32) {
>		if (test_bit(i, (unsigned long *)c->x86_capability))
>			continue;
>		if (!error)
>			pr_warn("CPU %d: missing required feature(s):", c->cpu_index);
>		pr_cont(" %s", x86_cap_name(i, cap_buf));
>		error = 1;
>	}
>
>	if (!error)
>		return;
>
>	pr_cont("\n");
>	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
>}

I'll have to test it but one concern I'd have is the pr_cont() in this
context? Since it can technically have asynchronous problems I would
think putting more code between subsequent calls to pr_cont() can
increase the chance of some race condition. But perhaps these two if
checks are not nearly enough for that to happen.

Otherwise I liked in the previous approach the steps of setting up a
bitmask with simple bitwise logic operations, then checking the results
later. But the above code also works and I think it is easier to read.
So if there is no opposition I'll test it and switch to it for the next
version, thanks :)

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH v11 2/4] x86/cpu: Check if feature string is non-zero

[Borislav Petkov]

On Fri, Mar 20, 2026 at 12:50:19PM +0000, Maciej Wieczor-Retman wrote:
> From: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> 
> In filter_cpuid_features(), x86_cap_flags[] is read, but it's not verified
> whether the string is non-zero which could lead to unwanted output.

How can this happen?

That loop in there iterates over cpuid_dependent_features[] and all *three*
features there have non-zero strings.

What am I missing?

> In two more places there are open coded paths that try to retrieve a
> feature string, and if there isn't one, the feature word and bit are
> returned instead.

Yes, as it should work.

> Add a common helper to fix filter_cpuid_features() as well as clean up
> the open coded cases.

Fix what?

> diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
> index 3ddc1d33399b..0698a6638463 100644
> --- a/arch/x86/include/asm/cpufeature.h
> +++ b/arch/x86/include/asm/cpufeature.h
> @@ -138,5 +138,9 @@ static __always_inline bool _static_cpu_has(u16 bit)
>  #define CPU_FEATURE_TYPEVAL		boot_cpu_data.x86_vendor, boot_cpu_data.x86, \
>  					boot_cpu_data.x86_model
>  
> +#define X86_CAP_BUF_SIZE		16

Except that you have feature strings which are longer than 16 - look at
arch/x86/kernel/cpu/capflags.c.

But this macro name is generically saying, this is the cap string size. Which
makes it misleading.

> +
> +const char *x86_cap_name(unsigned int bit, char *buf);

No, certainly NOT exported through a common header which others can use.

arch/x86/kernel/cpu/cpu.h probably which is an internal enough.

>  #endif /* defined(__KERNEL__) && !defined(__ASSEMBLER__) */
>  #endif /* _ASM_X86_CPUFEATURE_H */

...

> diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
> index 146f6f8b0650..5002f496d095 100644
> --- a/arch/x86/kernel/cpu/cpuid-deps.c
> +++ b/arch/x86/kernel/cpu/cpuid-deps.c
> @@ -156,24 +156,9 @@ 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)

What's wrong with keeping "x86_feature_name" - it is a perfectly fine function
name - and calling it x86_cap_name? Why don't you just fix that function to
DTRT as needed?

> -{
> -	if (x86_cap_flags[feature])
> -		return x86_cap_flags[feature];
> -
> -	snprintf(buf, 16, "%d*32+%2d", feature / 32, feature % 32);
> -
> -	return buf;
> -}

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v11 2/4] x86/cpu: Check if feature string is non-zero

[Maciej Wieczor-Retman]

On 2026-03-23 at 15:24:07 +0100, Borislav Petkov wrote:
>On Fri, Mar 20, 2026 at 12:50:19PM +0000, Maciej Wieczor-Retman wrote:
>> From: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>>
>> In filter_cpuid_features(), x86_cap_flags[] is read, but it's not verified
>> whether the string is non-zero which could lead to unwanted output.
>
>How can this happen?
>
>That loop in there iterates over cpuid_dependent_features[] and all *three*
>features there have non-zero strings.
>
>What am I missing?

It could be a problem if another entry is added that doesn't have the string
specified? Other spots that return either the string or the word:bit format do
check before if (!x86_cap_name[x]) isn't true. Is it not checked here on
purpose? Or is cpuid_dependent_features[] not expected to grow in the future?

>> In two more places there are open coded paths that try to retrieve a
>> feature string, and if there isn't one, the feature word and bit are
>> returned instead.
>
>Yes, as it should work.

With the above paragraph I was trying to establish that there are more places
that can get unified into one single purpose helper.

>> Add a common helper to fix filter_cpuid_features() as well as clean up
>> the open coded cases.
>
>Fix what?

I assumed the lack of null check on x86_cap_name[x] was not intentional.

>> diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
>> index 3ddc1d33399b..0698a6638463 100644
>> --- a/arch/x86/include/asm/cpufeature.h
>> +++ b/arch/x86/include/asm/cpufeature.h
>> @@ -138,5 +138,9 @@ static __always_inline bool _static_cpu_has(u16 bit)
>>  #define CPU_FEATURE_TYPEVAL		boot_cpu_data.x86_vendor, boot_cpu_data.x86, \
>>  					boot_cpu_data.x86_model
>>
>> +#define X86_CAP_BUF_SIZE		16
>
>Except that you have feature strings which are longer than 16 - look at
>arch/x86/kernel/cpu/capflags.c.
>
>But this macro name is generically saying, this is the cap string size. Which
>makes it misleading.

Right, sorry. Perhaps setting it to 24 would make sense? I think the longest
right now is 19, So there'd be some extra space in case a longer string is added
later?

>> +
>> +const char *x86_cap_name(unsigned int bit, char *buf);
>
>No, certainly NOT exported through a common header which others can use.
>
>arch/x86/kernel/cpu/cpu.h probably which is an internal enough.

Thanks, I'll switch it to that, I wasn't sure which one would be most
appropriate.

>>  #endif /* defined(__KERNEL__) && !defined(__ASSEMBLER__) */
>>  #endif /* _ASM_X86_CPUFEATURE_H */
>
>...
>
>> diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
>> index 146f6f8b0650..5002f496d095 100644
>> --- a/arch/x86/kernel/cpu/cpuid-deps.c
>> +++ b/arch/x86/kernel/cpu/cpuid-deps.c
>> @@ -156,24 +156,9 @@ 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)
>
>What's wrong with keeping "x86_feature_name" - it is a perfectly fine function
>name - and calling it x86_cap_name? Why don't you just fix that function to
>DTRT as needed?

Sure, I can switch the name to x86_feature_name(). But I assume keeping it in
common.c makes sense? Since it could be used there mostly and at that point it's
more generic and not related to cpuid-deps specifically.

>> -{
>> -	if (x86_cap_flags[feature])
>> -		return x86_cap_flags[feature];
>> -
>> -	snprintf(buf, 16, "%d*32+%2d", feature / 32, feature % 32);
>> -
>> -	return buf;
>> -}
>
>--
>Regards/Gruss,
>    Boris.
>
>https://people.kernel.org/tglx/notes-about-netiquette

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH v11 2/4] x86/cpu: Check if feature string is non-zero

[Borislav Petkov]

On Mon, Mar 23, 2026 at 03:52:04PM +0000, Maciej Wieczor-Retman wrote:
> It could be a problem if another entry is added that doesn't have the string
> specified? Other spots that return either the string or the word:bit format do
> check before if (!x86_cap_name[x]) isn't true. Is it not checked here on
> purpose? Or is cpuid_dependent_features[] not expected to grow in the future?

Yes, maybe, no...

The point is, when you write commit messages, you should *precisely* explain
what the issue or the non-issue is. What you have now is misleading - it
should say what you just wrote above - that this could *potentially* be
a problem but it isn't a problem now.

> Right, sorry. Perhaps setting it to 24 would make sense? I think the longest
> right now is 19, So there'd be some extra space in case a longer string is added
> later?

The use being?

Nothing's stopping someone from slapping a longer name in "" in cpufeatures.h

As long as you select a size and you enforce it somewhere and scream loudly
when someone overflows it, then that's good. Otherwise what's the point of
calling it anything if it is not being enforced?

> Sure, I can switch the name to x86_feature_name(). But I assume keeping it in
> common.c makes sense? Since it could be used there mostly and at that point it's
> more generic and not related to cpuid-deps specifically.

Right, makes sense.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[Borislav Petkov]

On Fri, Mar 20, 2026 at 12:50:25PM +0000, Maciej Wieczor-Retman wrote:
> From: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> 
> After CPU identification concludes, do a sanity check by comparing the
> final x86_capability bitmask with the pre-defined required feature bits.

The use being?

AFAICT, the required features are:

$ cat arch/x86/include/generated/asm/cpufeaturemasks.h

...

/*
 * REQUIRED features:
 *
 *    FPU MSR PAE CX8 CMOV FXSR XMM XMM2 LM NOPL ALWAYS CPUID
 */

AFAICT, if *any* of those features are not set, the machine will crash'n'burn
anyway.

So the required features will be "enforced" pretty early :-)

Otherwise they're not really required.

And besides, what's

arch/x86/kernel/verify_cpu.S

for if not for that?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v11 4/4] x86/cpu: Clear feature bits whose dependencies were cleared

[Borislav Petkov]

On Fri, Mar 20, 2026 at 12:50:29PM +0000, Maciej Wieczor-Retman wrote:
> From: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> 
> After cpu_caps_cleared[] is initialized with DISABLED_MASK_INIT,
> features present in disabled bitmasks are cleared from x86_capability[].
> However features that depend on them and are not part of any disabled
> mask are not cleared by anything. They can trigger the warning in
> check_cpufeature_deps(), as before both features would show up as
> enabled even though they weren't. The uncleared features can also still
> falsely show up in /proc/cpuinfo.

Well, why aren't we clearing those apply_forced_caps() too instead of clearing
them at check time?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v11 2/4] x86/cpu: Check if feature string is non-zero

[Maciej Wieczor-Retman]

On 2026-03-23 at 17:23:24 +0100, Borislav Petkov wrote:
>On Mon, Mar 23, 2026 at 03:52:04PM +0000, Maciej Wieczor-Retman wrote:
>> It could be a problem if another entry is added that doesn't have the string
>> specified? Other spots that return either the string or the word:bit format do
>> check before if (!x86_cap_name[x]) isn't true. Is it not checked here on
>> purpose? Or is cpuid_dependent_features[] not expected to grow in the future?
>
>Yes, maybe, no...
>
>The point is, when you write commit messages, you should *precisely* explain
>what the issue or the non-issue is. What you have now is misleading - it
>should say what you just wrote above - that this could *potentially* be
>a problem but it isn't a problem now.

Okay, you're right, that could've been misleading, I'll rewrite that paragraph.

>> Right, sorry. Perhaps setting it to 24 would make sense? I think the longest
>> right now is 19, So there'd be some extra space in case a longer string is added
>> later?
>
>The use being?
>
>Nothing's stopping someone from slapping a longer name in "" in cpufeatures.h
>
>As long as you select a size and you enforce it somewhere and scream loudly
>when someone overflows it, then that's good. Otherwise what's the point of
>calling it anything if it is not being enforced?

Ah sorry, it's a bit of a misunderstanding, the X86_CAP_BUF_SIZE refers only to
the value that's needed for a buffer when the word:bit format is returned.
Otherwise the const char * is returned from the x86_cap_flags[] array.

So I guess the constant name should be different? Maybe X86_CAP_NUM_BUF_SIZE?


-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[Maciej Wieczor-Retman]

On 2026-03-23 at 17:31:45 +0100, Borislav Petkov wrote:
>On Fri, Mar 20, 2026 at 12:50:25PM +0000, Maciej Wieczor-Retman wrote:
>> From: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>>
>> After CPU identification concludes, do a sanity check by comparing the
>> final x86_capability bitmask with the pre-defined required feature bits.
>
>The use being?
>
>AFAICT, the required features are:
>
>$ cat arch/x86/include/generated/asm/cpufeaturemasks.h
>
>...
>
>/*
> * REQUIRED features:
> *
> *    FPU MSR PAE CX8 CMOV FXSR XMM XMM2 LM NOPL ALWAYS CPUID
> */
>
>AFAICT, if *any* of those features are not set, the machine will crash'n'burn
>anyway.
>
>So the required features will be "enforced" pretty early :-)
>
>Otherwise they're not really required.
>
>And besides, what's
>
>arch/x86/kernel/verify_cpu.S
>
>for if not for that?
>
>--
>Regards/Gruss,
>    Boris.
>
>https://people.kernel.org/tglx/notes-about-netiquette

hpa requested to add this patch to the series [1]. As he explained the other modern
boot stubs don't include the same checks that the Linux kernel has.

[1] https://lore.kernel.org/all/5fd597a1-46fe-4f46-98ee-d346260c85b5@zytor.com/

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH v11 4/4] x86/cpu: Clear feature bits whose dependencies were cleared

[Maciej Wieczor-Retman]

On 2026-03-23 at 17:35:57 +0100, Borislav Petkov wrote:
>On Fri, Mar 20, 2026 at 12:50:29PM +0000, Maciej Wieczor-Retman wrote:
>> From: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>>
>> After cpu_caps_cleared[] is initialized with DISABLED_MASK_INIT,
>> features present in disabled bitmasks are cleared from x86_capability[].
>> However features that depend on them and are not part of any disabled
>> mask are not cleared by anything. They can trigger the warning in
>> check_cpufeature_deps(), as before both features would show up as
>> enabled even though they weren't. The uncleared features can also still
>> falsely show up in /proc/cpuinfo.
>
>Well, why aren't we clearing those apply_forced_caps() too instead of clearing
>them at check time?

At the moment only the SGX + SGX_LC pair can cause this problem (case of SGX is
not compiled and it has X86_DISABLED_FEATURE_SGX while SGX_LC depends on SGX but
doesn't have X86_DISABLED_FEATURE_SGX_LC defined therefore it is not
automatically cleared).

Clearing the compile time disabled bits is fairly low effort since it's just
initializing the cpu_caps_cleared[]. To validate the whole cpuid_deps[] array
and add relevant bits to cpu_caps_cleared[] would probably require adding
another loop that would check all of the entries. Since now only SGX has this
problem I didn't think it warranted adding more than this check in
check_cpufeature_deps().

>
>--
>Regards/Gruss,
>    Boris.
>
>https://people.kernel.org/tglx/notes-about-netiquette

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH v11 2/4] x86/cpu: Check if feature string is non-zero

[Borislav Petkov]

On Mon, Mar 23, 2026 at 04:58:20PM +0000, Maciej Wieczor-Retman wrote:
> Ah sorry, it's a bit of a misunderstanding, the X86_CAP_BUF_SIZE refers only to
> the value that's needed for a buffer when the word:bit format is returned.
> Otherwise the const char * is returned from the x86_cap_flags[] array.

Yes, I know that. That's why I said:

"But this macro name is generically saying, this is the cap string size. Which
makes it misleading."

> So I guess the constant name should be different? Maybe X86_CAP_NUM_BUF_SIZE?

#define X86_NAMELESS_FEAT_STRLEN

or so.

Your naming must be explicit and denote for what this is used.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[Borislav Petkov]

On Mon, Mar 23, 2026 at 05:05:48PM +0000, Maciej Wieczor-Retman wrote:
> On 2026-03-23 at 17:31:45 +0100, Borislav Petkov wrote:
> >On Fri, Mar 20, 2026 at 12:50:25PM +0000, Maciej Wieczor-Retman wrote:
> >> From: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> >>
> >> After CPU identification concludes, do a sanity check by comparing the
> >> final x86_capability bitmask with the pre-defined required feature bits.
> >
> >The use being?
> >
> >AFAICT, the required features are:
> >
> >$ cat arch/x86/include/generated/asm/cpufeaturemasks.h
> >
> >...
> >
> >/*
> > * REQUIRED features:
> > *
> > *    FPU MSR PAE CX8 CMOV FXSR XMM XMM2 LM NOPL ALWAYS CPUID
> > */
> >
> >AFAICT, if *any* of those features are not set, the machine will crash'n'burn
> >anyway.
> >
> >So the required features will be "enforced" pretty early :-)
> >
> >Otherwise they're not really required.
> >
> >And besides, what's
> >
> >arch/x86/kernel/verify_cpu.S
> >
> >for if not for that?

> hpa requested to add this patch to the series [1]. As he explained the other modern
> boot stubs don't include the same checks that the Linux kernel has.
> 
> [1] https://lore.kernel.org/all/5fd597a1-46fe-4f46-98ee-d346260c85b5@zytor.com/

I don't understand.

We call verify_cpu() in startup_64 in kernel proper too.

So if anything's missing, it should be added there instead of adding yet
another let's verify features thing...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v11 4/4] x86/cpu: Clear feature bits whose dependencies were cleared

[Borislav Petkov]

On Mon, Mar 23, 2026 at 05:23:19PM +0000, Maciej Wieczor-Retman wrote:
> Clearing the compile time disabled bits is fairly low effort since it's just
> initializing the cpu_caps_cleared[].

and cpu_caps_cleared[] is consulted in apply_forced_caps().

> To validate the whole cpuid_deps[] array and add relevant bits to
> cpu_caps_cleared[] would probably require adding another loop that would
> check all of the entries. Since now only SGX has this problem I didn't think
> it warranted adding more than this check in check_cpufeature_deps().

So we're going to do half-baked solution again and it'll come to bite us in
the ass later?

How about we hammer it out properly once and for all?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v11 2/4] x86/cpu: Check if feature string is non-zero

[Maciej Wieczor-Retman]

On 2026-03-23 at 18:51:51 +0100, Borislav Petkov wrote:
>On Mon, Mar 23, 2026 at 04:58:20PM +0000, Maciej Wieczor-Retman wrote:
>> Ah sorry, it's a bit of a misunderstanding, the X86_CAP_BUF_SIZE refers only to
>> the value that's needed for a buffer when the word:bit format is returned.
>> Otherwise the const char * is returned from the x86_cap_flags[] array.
>
>Yes, I know that. That's why I said:
>
>"But this macro name is generically saying, this is the cap string size. Which
>makes it misleading."
>
>> So I guess the constant name should be different? Maybe X86_CAP_NUM_BUF_SIZE?
>
>#define X86_NAMELESS_FEAT_STRLEN
>
>or so.
>
>Your naming must be explicit and denote for what this is used.
>
>--
>Regards/Gruss,
>    Boris.
>
>https://people.kernel.org/tglx/notes-about-netiquette

Thanks! I'll correct it

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH v11 2/4] x86/cpu: Check if feature string is non-zero

[H. Peter Anvin]

On March 23, 2026 11:11:33 AM PDT, Maciej Wieczor-Retman <m.wieczorretman@pm.me> wrote:
>On 2026-03-23 at 18:51:51 +0100, Borislav Petkov wrote:
>>On Mon, Mar 23, 2026 at 04:58:20PM +0000, Maciej Wieczor-Retman wrote:
>>> Ah sorry, it's a bit of a misunderstanding, the X86_CAP_BUF_SIZE refers only to
>>> the value that's needed for a buffer when the word:bit format is returned.
>>> Otherwise the const char * is returned from the x86_cap_flags[] array.
>>
>>Yes, I know that. That's why I said:
>>
>>"But this macro name is generically saying, this is the cap string size. Which
>>makes it misleading."
>>
>>> So I guess the constant name should be different? Maybe X86_CAP_NUM_BUF_SIZE?
>>
>>#define X86_NAMELESS_FEAT_STRLEN
>>
>>or so.
>>
>>Your naming must be explicit and denote for what this is used.
>>
>>--
>>Regards/Gruss,
>>    Boris.
>>
>>https://people.kernel.org/tglx/notes-about-netiquette
>
>Thanks! I'll correct it
>

Call it "BUF" or "BUFLEN". It's not the length of the string, but the length of the buffer for the caller to allocate.

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[Pawan Gupta]

On Sat, Mar 21, 2026 at 05:58:18AM +0000, Maciej Wieczór-Retman wrote:
> On 2026-03-20 at 17:31:27 -0700, Pawan Gupta wrote:
> >On Fri, Mar 20, 2026 at 12:50:25PM +0000, Maciej Wieczor-Retman wrote:
> >...
> >> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> >> index 0e318f3d56cb..92159a0963c8 100644
> >> --- a/arch/x86/kernel/cpu/common.c
> >> +++ b/arch/x86/kernel/cpu/common.c
> >> @@ -2005,6 +2005,38 @@ const char *x86_cap_name(unsigned int bit, char *buf)
> >>  	return buf;
> >>  }
> >>
> >> +/*
> >> + * As a sanity check compare the final x86_capability bitmask with the initial
> >> + * predefined required feature bits.
> >> + */
> >> +static void verify_required_features(const struct cpuinfo_x86 *c)
> >> +{
> >> +	u32 required_features[NCAPINTS] = REQUIRED_MASK_INIT;
> >> +	DECLARE_BITMAP(missing, NCAPINTS * 32);
> >> +	char cap_buf[X86_CAP_BUF_SIZE];
> >> +	u32 *missing_u32;
> >> +	unsigned int i;
> >> +	u32 error = 0;
> >> +
> >> +	memset(missing, 0, sizeof(missing));
> >> +	missing_u32 = (u32 *)missing;
> >> +
> >> +	for (i = 0; i < NCAPINTS; i++) {
> >> +		missing_u32[i] = ~c->x86_capability[i] & required_features[i];
> >> +		error |= missing_u32[i];
> >> +	}
> >> +
> >> +	if (!error)
> >> +		return;
> >> +
> >> +	/* At least one required feature is missing */
> >> +	pr_warn("CPU %d: missing required feature(s):", c->cpu_index);
> >> +	for_each_set_bit(i, missing, NCAPINTS * 32)
> >> +		pr_cont(" %s", x86_cap_name(i, cap_buf));
> >> +	pr_cont("\n");
> >> +	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
> >> +}
> >
> >Do we need 2 loops? Can this be simplified as below:
> >
> >static void verify_required_features(const struct cpuinfo_x86 *c)
> >{
> >	u32 required_features[NCAPINTS + 1] = REQUIRED_MASK_INIT;
> >	char cap_buf[X86_CAP_BUF_SIZE];
> >	int i, error = 0;
> >
> >	for_each_set_bit(i, (unsigned long *)required_features, NCAPINTS * 32) {
> >		if (test_bit(i, (unsigned long *)c->x86_capability))
> >			continue;
> >		if (!error)
> >			pr_warn("CPU %d: missing required feature(s):", c->cpu_index);
> >		pr_cont(" %s", x86_cap_name(i, cap_buf));
> >		error = 1;
> >	}
> >
> >	if (!error)
> >		return;
> >
> >	pr_cont("\n");
> >	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
> >}
> 
> I'll have to test it but one concern I'd have is the pr_cont() in this
> context? Since it can technically have asynchronous problems I would
> think putting more code between subsequent calls to pr_cont() can
> increase the chance of some race condition. But perhaps these two if
> checks are not nearly enough for that to happen.

You may be right, but relying on number of instructions in the loop for
print syncronization seems flawed to begin with. What probably saves from
output being garbled is the printk machinery caching the string until a
'\n'. I am not fully sure.

> Otherwise I liked in the previous approach the steps of setting up a
> bitmask with simple bitwise logic operations, then checking the results
> later.

My main motivation for suggesting the change was to try and use the
existing infrastructure for bit operations much as possible. Even in my
suggestion test_bit(cap) can be replaced with test_cpu_cap().

> But the above code also works and I think it is easier to read.
> So if there is no opposition I'll test it and switch to it for the next
> version, thanks :)

As I looked at it again, I see that cpu_has() helpers unconditionally
returns true for all the required features.

#define cpu_has(c, bit)                                                 \
        (__builtin_constant_p(bit) && REQUIRED_MASK_BIT_SET(bit) ? 1 :  \
         test_cpu_cap(c, bit))

So this seems inline with Boris's comment, system should crash and burn if
any of the required features is missing.

Re: [PATCH v11 4/4] x86/cpu: Clear feature bits whose dependencies were cleared

[Maciej Wieczor-Retman]

On 2026-03-23 at 18:59:02 +0100, Borislav Petkov wrote:
>On Mon, Mar 23, 2026 at 05:23:19PM +0000, Maciej Wieczor-Retman wrote:
>> Clearing the compile time disabled bits is fairly low effort since it's just
>> initializing the cpu_caps_cleared[].
>
>and cpu_caps_cleared[] is consulted in apply_forced_caps().
>
>> To validate the whole cpuid_deps[] array and add relevant bits to
>> cpu_caps_cleared[] would probably require adding another loop that would
>> check all of the entries. Since now only SGX has this problem I didn't think
>> it warranted adding more than this check in check_cpufeature_deps().
>
>So we're going to do half-baked solution again and it'll come to bite us in
>the ass later?
>
>How about we hammer it out properly once and for all?
>
>--
>Regards/Gruss,
>    Boris.
>
>https://people.kernel.org/tglx/notes-about-netiquette

Okay, I'll work on a helper/loop to check the whole array and clear such cases
somewhat early so the warning in check_cpufeature_deps() doesn't get triggered.

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[Maciej Wieczor-Retman]

On 2026-03-23 at 11:16:43 -0700, Pawan Gupta wrote:
>On Sat, Mar 21, 2026 at 05:58:18AM +0000, Maciej Wieczór-Retman wrote:
>> On 2026-03-20 at 17:31:27 -0700, Pawan Gupta wrote:
>> >On Fri, Mar 20, 2026 at 12:50:25PM +0000, Maciej Wieczor-Retman wrote:
>> >...
>> >> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
>> >> index 0e318f3d56cb..92159a0963c8 100644
>> >> --- a/arch/x86/kernel/cpu/common.c
>> >> +++ b/arch/x86/kernel/cpu/common.c
>> >> @@ -2005,6 +2005,38 @@ const char *x86_cap_name(unsigned int bit, char *buf)
>> >>  	return buf;
>> >>  }
>> >>
>> >> +/*
>> >> + * As a sanity check compare the final x86_capability bitmask with the initial
>> >> + * predefined required feature bits.
>> >> + */
>> >> +static void verify_required_features(const struct cpuinfo_x86 *c)
>> >> +{
>> >> +	u32 required_features[NCAPINTS] = REQUIRED_MASK_INIT;
>> >> +	DECLARE_BITMAP(missing, NCAPINTS * 32);
>> >> +	char cap_buf[X86_CAP_BUF_SIZE];
>> >> +	u32 *missing_u32;
>> >> +	unsigned int i;
>> >> +	u32 error = 0;
>> >> +
>> >> +	memset(missing, 0, sizeof(missing));
>> >> +	missing_u32 = (u32 *)missing;
>> >> +
>> >> +	for (i = 0; i < NCAPINTS; i++) {
>> >> +		missing_u32[i] = ~c->x86_capability[i] & required_features[i];
>> >> +		error |= missing_u32[i];
>> >> +	}
>> >> +
>> >> +	if (!error)
>> >> +		return;
>> >> +
>> >> +	/* At least one required feature is missing */
>> >> +	pr_warn("CPU %d: missing required feature(s):", c->cpu_index);
>> >> +	for_each_set_bit(i, missing, NCAPINTS * 32)
>> >> +		pr_cont(" %s", x86_cap_name(i, cap_buf));
>> >> +	pr_cont("\n");
>> >> +	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
>> >> +}
>> >
>> >Do we need 2 loops? Can this be simplified as below:
>> >
>> >static void verify_required_features(const struct cpuinfo_x86 *c)
>> >{
>> >	u32 required_features[NCAPINTS + 1] = REQUIRED_MASK_INIT;
>> >	char cap_buf[X86_CAP_BUF_SIZE];
>> >	int i, error = 0;
>> >
>> >	for_each_set_bit(i, (unsigned long *)required_features, NCAPINTS * 32) {
>> >		if (test_bit(i, (unsigned long *)c->x86_capability))
>> >			continue;
>> >		if (!error)
>> >			pr_warn("CPU %d: missing required feature(s):", c->cpu_index);
>> >		pr_cont(" %s", x86_cap_name(i, cap_buf));
>> >		error = 1;
>> >	}
>> >
>> >	if (!error)
>> >		return;
>> >
>> >	pr_cont("\n");
>> >	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
>> >}
>>
>> I'll have to test it but one concern I'd have is the pr_cont() in this
>> context? Since it can technically have asynchronous problems I would
>> think putting more code between subsequent calls to pr_cont() can
>> increase the chance of some race condition. But perhaps these two if
>> checks are not nearly enough for that to happen.
>
>You may be right, but relying on number of instructions in the loop for
>print syncronization seems flawed to begin with. What probably saves from
>output being garbled is the printk machinery caching the string until a
>'\n'. I am not fully sure.
>
>> Otherwise I liked in the previous approach the steps of setting up a
>> bitmask with simple bitwise logic operations, then checking the results
>> later.
>
>My main motivation for suggesting the change was to try and use the
>existing infrastructure for bit operations much as possible. Even in my
>suggestion test_bit(cap) can be replaced with test_cpu_cap().

Okay, yes, I suppose that is a good idea.

>> But the above code also works and I think it is easier to read.
>> So if there is no opposition I'll test it and switch to it for the next
>> version, thanks :)
>
>As I looked at it again, I see that cpu_has() helpers unconditionally
>returns true for all the required features.
>
>#define cpu_has(c, bit)                                                 \
>        (__builtin_constant_p(bit) && REQUIRED_MASK_BIT_SET(bit) ? 1 :  \
>         test_cpu_cap(c, bit))
>
>So this seems inline with Boris's comment, system should crash and burn if
>any of the required features is missing.

Yeah, that would make sense too. What I checked is that this sanity check can
catch if any kernel calls cleared any of the required bits from the
x86_capability[] array. I might have to figure out a test to actually disable
something required that doesn't completely crash everything at the same time.

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[H. Peter Anvin]

On 2026-03-23 09:31, Borislav Petkov wrote:
> 
> AFAICT, if *any* of those features are not set, the machine will crash'n'burn
> anyway.
> 
> So the required features will be "enforced" pretty early :-)
> 
> Otherwise they're not really required.
> 
> And besides, what's
> 
> arch/x86/kernel/verify_cpu.S
> 
> for if not for that?
> 

That is not necessarily true at all. As such, this may be a really cheap way
to get a message out in case we get that far without problems.

For one thing, this runs -- at least on the BSP -- before either alternatives
patching or running user space, so there is plenty of features that may not
have been used yet.

For another thing, there are some features -- such as PAE to mention one --
that are present in some CPUs but disabled in CPUID because for some reason or
another the manufacturer found during testing that it doesn't always work
right. However, it is likely that a PAE kernel will successfully boot, and it
might even work on any one particular CPU. This is *exactly* what
TAINT_CPU_OUT_OF_SPEC is supposed to represent.

Finally, as Maciej reported, the user might have tried to explicitly override
a required feature.

verify_cpu.S serves a different purpose: is to enable features that are
required to even set up the kernel execution environment and that may be
switched off through various mechanisms.

verify_cpu.S is unfortunately not able to issue messages (not to mention that
it is written in assembly, *AND* it doesn't have access to all the rules and
exceptions that are coded into arch/x86/kernel/cpu/*.) There *is* a messaging
function in the BIOS stub, but there is no equivalent in for code that bypass
this stage (which is of course standard these days.) verify_cpu.S just returns
a single bit, and that only represents checking for a few very basic features.

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[H. Peter Anvin]

On 2026-03-23 10:55, Borislav Petkov wrote:
> 
> I don't understand.
> 
> We call verify_cpu() in startup_64 in kernel proper too.
> 
> So if anything's missing, it should be added there instead of adding yet
> another let's verify features thing...
> 

See my response elsewhere in this thread.

	-hpa

Re: [PATCH v11 4/4] x86/cpu: Clear feature bits whose dependencies were cleared

[H. Peter Anvin]

On 2026-03-23 10:59, Borislav Petkov wrote:
> On Mon, Mar 23, 2026 at 05:23:19PM +0000, Maciej Wieczor-Retman wrote:
>> Clearing the compile time disabled bits is fairly low effort since it's just
>> initializing the cpu_caps_cleared[].
> 
> and cpu_caps_cleared[] is consulted in apply_forced_caps().
> 
>> To validate the whole cpuid_deps[] array and add relevant bits to
>> cpu_caps_cleared[] would probably require adding another loop that would
>> check all of the entries. Since now only SGX has this problem I didn't think
>> it warranted adding more than this check in check_cpufeature_deps().
> 
> So we're going to do half-baked solution again and it'll come to bite us in
> the ass later?
> 
> How about we hammer it out properly once and for all?
> 

One could argue that the Right Thing[TM] to do would be to encode the
dependencies in a file that can be processed at compile time, and have those
features explicitly added to the enabled and disabled masks, basically
augmenting Kconfig.cpufeatures.

The sanest way to do that would probably be to move cpuid_deps[] from
cpuid-deps.c into a standalone file which can be processed by
cpufeaturesmask.awk or equivalent.

I do have to say that using awk for these things, especially being restricted
to POSIX awk, is really awful as it really is pushing the limits of that tool.
String processing in C is most definitely no fun either -- the single biggest
shortcoming of what is otherwise one of my favorite languages -- but it
*would* also be possible to refactor cpuid-deps.c so that it can be compiled
in user space and linked with a C tool that would replace cpufeaturesmask.awk,
mkcapflags.sh, and boot/mkcpustr.c.

This seemed to me to be a bit excessive in terms of churn and complexity, but
perhaps this is what you are looking for?

	-hpa

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[Borislav Petkov]

On Mon, Mar 23, 2026 at 11:43:08AM -0700, H. Peter Anvin wrote:
> That is not necessarily true at all.

What does that even mean?! :)

> As such, this may be a really cheap way to get a message out in case we get
> that far without problems.

Meh.

> For one thing, this runs -- at least on the BSP -- before either alternatives
> patching or running user space, so there is plenty of features that may not
> have been used yet.

We're talking about required features here. What's wrong with verify_cpu()
testing required features and stopping if some of them are not present?

It is already checking some of them.

> For another thing, there are some features -- such as PAE to mention one --
> that are present in some CPUs but disabled in CPUID because for some reason or
> another the manufacturer found during testing that it doesn't always work
> right. However, it is likely that a PAE kernel will successfully boot, and it
> might even work on any one particular CPU. This is *exactly* what
> TAINT_CPU_OUT_OF_SPEC is supposed to represent.

And?

We're supposed to support such a CPU or somewhat wobbly only?

You want to be able to boot up to the point of checking required features in
C code, find out that PAE is not supported, taint the CPU but still run?

What for?

How do you explain the user that her machine is actually fine but we'll taint
the kernel and that it maybe works but maybe not and there are no guarantees?

> Finally, as Maciej reported, the user might have tried to explicitly override
> a required feature.

You can still catch it in verify_cpu. Catch it such that you simply stop
there.

If the luser is overriding required features, then she gets to keep both
pieces.

> verify_cpu.S serves a different purpose: is to enable features that are
> required to even set up the kernel execution environment and that may be
> switched off through various mechanisms.

And because we run it at so many places, then it can do those checks for us
too.

> verify_cpu.S is unfortunately not able to issue messages (not to mention that
> it is written in assembly,

We can convert it to C. We've done this before with other crap. :)

> *AND* it doesn't have access to all the rules and exceptions that are coded
> into arch/x86/kernel/cpu/*.) There *is* a messaging function in the BIOS
> stub, but there is no equivalent in for code that bypass this stage (which
> is of course standard these days.) verify_cpu.S just returns a single bit,
> and that only represents checking for a few very basic features.

Yes, I have seen the error message about this CPU not being supported very
early.

But I don't see the point for adding another function to verify required
features which is somewhere else where we're pretty much doing that checking
early.

And what's the point of booting up to C code and kernel proper and say that
some of the required features are off?

I think we should extend verify_cpu or convert it to C or have it call
a C function or whatever and do the checking once and for all and not boot
into a wobbly and tainted kernel.

As to showing a proper error message, what is the real use case we're chasing
here?

The CPU has all the required features - which is probably 99.999% of the cases
out there or it doesn't and then it deserves to blow up.

So what are we really "fixing" here...?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v11 4/4] x86/cpu: Clear feature bits whose dependencies were cleared

[Borislav Petkov]

On Mon, Mar 23, 2026 at 11:57:45AM -0700, H. Peter Anvin wrote:
> I do have to say that using awk for these things, especially being restricted
> to POSIX awk, is really awful as it really is pushing the limits of that tool.
> String processing in C is most definitely no fun either -- the single biggest
> shortcoming of what is otherwise one of my favorite languages -- but it
> *would* also be possible to refactor cpuid-deps.c so that it can be compiled
> in user space and linked with a C tool that would replace cpufeaturesmask.awk,
> mkcapflags.sh, and boot/mkcpustr.c.
> 
> This seemed to me to be a bit excessive in terms of churn and complexity, but
> perhaps this is what you are looking for?

I was actually suggesting we handle the dependent features in
apply_forced_caps() and keep that all in one place.

As to the whole glue handling deps, feature flags, names, yadda yadda, yap,
I agree, it ain't easy and I need to grep each time where the generated files
are and go stare at them.

So I really like the idea of replacing it all with a C tool which you can
massage in luserspace and replace all that other gunk and we won't depend on
the awk flavor or shell script or whatnot but keep everything in a single
C program and a header.

I don't know whether that would work and how ugly it would become - we need to
try it I guess - but the idea ure sounds enticing...

Maybe something for a future todo.

:-)

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v11 4/4] x86/cpu: Clear feature bits whose dependencies were cleared

[Ahmed S. Darwish]

On Mon, 23 Mar 2026, H. Peter Anvin wrote:
>
> On 2026-03-23 10:59, Borislav Petkov wrote:
> > On Mon, Mar 23, 2026 at 05:23:19PM +0000, Maciej Wieczor-Retman wrote:
> >> Clearing the compile time disabled bits is fairly low effort since it's just
> >> initializing the cpu_caps_cleared[].
> >
> > and cpu_caps_cleared[] is consulted in apply_forced_caps().
> >
> >> To validate the whole cpuid_deps[] array and add relevant bits to
> >> cpu_caps_cleared[] would probably require adding another loop that would
> >> check all of the entries. Since now only SGX has this problem I didn't think
> >> it warranted adding more than this check in check_cpufeature_deps().
> >
> > So we're going to do half-baked solution again and it'll come to bite us in
> > the ass later?
> >
> > How about we hammer it out properly once and for all?
> >
>
> One could argue that the Right Thing[TM] to do would be to encode the
> dependencies in a file that can be processed at compile time, and have those
> features explicitly added to the enabled and disabled masks, basically
> augmenting Kconfig.cpufeatures.
>
> The sanest way to do that would probably be to move cpuid_deps[] from
> cpuid-deps.c into a standalone file which can be processed by
> cpufeaturesmask.awk or equivalent.
>

I'm posting the CPUID patch queue tomorrow.  It has all the X86_FEATURE
words (synthetic and hardware-backed) modeled and mapped by x86-cpuid-db.

These XMLs are definitely the right place to encode the dependencies, as
there is already different types of generators in the project.

The series cover letter tomorrow will cover a lot of details on this.

Thanks!
Ahmed

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[H. Peter Anvin]

On 2026-03-23 12:19, Borislav Petkov wrote:
> On Mon, Mar 23, 2026 at 11:43:08AM -0700, H. Peter Anvin wrote:
>> That is not necessarily true at all.
> 
> What does that even mean?! :)
> 
>> As such, this may be a really cheap way to get a message out in case we get
>> that far without problems.
> 
> Meh.
> 
>> For one thing, this runs -- at least on the BSP -- before either alternatives
>> patching or running user space, so there is plenty of features that may not
>> have been used yet.
> 
> We're talking about required features here. What's wrong with verify_cpu()
> testing required features and stopping if some of them are not present?
> 
> It is already checking some of them.
> 

Well, there is the bits which need to be in assembly because they

>> For another thing, there are some features -- such as PAE to mention one --
>> that are present in some CPUs but disabled in CPUID because for some reason or
>> another the manufacturer found during testing that it doesn't always work
>> right. However, it is likely that a PAE kernel will successfully boot, and it
>> might even work on any one particular CPU. This is *exactly* what
>> TAINT_CPU_OUT_OF_SPEC is supposed to represent.
> 
> And?
> 
> We're supposed to support such a CPU or somewhat wobbly only?
>
> You want to be able to boot up to the point of checking required features in
> C code, find out that PAE is not supported, taint the CPU but still run?
> 
> What for?
> 
> How do you explain the user that her machine is actually fine but we'll taint
> the kernel and that it maybe works but maybe not and there are no guarantees?

That is EXACTLY what TAINT_CPU_OUT_OF_SPEC means.


>> Finally, as Maciej reported, the user might have tried to explicitly override
>> a required feature.
> 
> You can still catch it in verify_cpu. Catch it such that you simply stop
> there.
> 
> If the luser is overriding required features, then she gets to keep both
> pieces.

It doesn't mean we can't at least TRY to warn things.

>> verify_cpu.S serves a different purpose: is to enable features that are
>> required to even set up the kernel execution environment and that may be
>> switched off through various mechanisms.
> 
> And because we run it at so many places, then it can do those checks for us
> too.
> 
>> verify_cpu.S is unfortunately not able to issue messages (not to mention that
>> it is written in assembly,
> 
> We can convert it to C. We've done this before with other crap. :)

No, you can't, because YOU CAN'T GET FAR ENOUGH ALONG TO RUN C CODE WITHOUT
IT. That is what is unique about verify_cpu.S. It should probably really be
called "enable_cpu.S".

It doesn't mean you can't do an earlier check, but at that point you pretty
much need the entire machinery of arch/x86/kernel/cpu.

We COULD push a lot of that code much earlier, and make it sharable with the
boot/compressed prekernel, but that is a cleanup on a whole different scale.

> Yes, I have seen the error message about this CPU not being supported very
> early.
> 
> But I don't see the point for adding another function to verify required
> features which is somewhere else where we're pretty much doing that checking
> early.

Well, for one thing: it lets us avoid more ad hoc messages in central code.

> And what's the point of booting up to C code and kernel proper and say that
> some of the required features are off?
> 
> I think we should extend verify_cpu or convert it to C or have it call
> a C function or whatever and do the checking once and for all and not boot
> into a wobbly and tainted kernel.
> 
> As to showing a proper error message, what is the real use case we're chasing
> here?
> 
> The CPU has all the required features - which is probably 99.999% of the cases
> out there or it doesn't and then it deserves to blow up.
> 
> So what are we really "fixing" here...?
You can make the same argument about #MC for example: why bother trying to get
a message out when the CPU is literally telling you that your system just broke?

The answer is because it helps the user understand what is wrong. Certainly,
you have no guarantee that you will actually get there, but in practice, in
many (but definitely not all) cases you WILL be able to get far enough along
to get the message out so that when the user wonders why their machine crashed
they have a clue.

	-hpa

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[Borislav Petkov]

On Mon, Mar 23, 2026 at 01:24:25PM -0700, H. Peter Anvin wrote:
> > So what are we really "fixing" here...?
> You can make the same argument about #MC for example: why bother trying to get
> a message out when the CPU is literally telling you that your system just broke?

I don't think you're parsing me right: do you have actual, real-life use cases
for which you want this or is this something hypothetical?

Because from where I'm standing, this sounds like let's slap another checking
function somewhere, we won't hit it ever but just in case, let's be prepared.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[H. Peter Anvin]

I did: the PAE case, which was not hypothetical. I'm sure I can dig up more.

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[Borislav Petkov]

On Mon, Mar 23, 2026 at 02:40:47PM -0700, H. Peter Anvin wrote:
> I did: the PAE case, which was not hypothetical. I'm sure I can dig up more.

So let's start our justification from this POV: we have use cases X and Y and they
would profit from getting these warning checks about required features not
being set.

I still am not persuaded that we want Linux to boot on those. Because those
CPUs are clearly "out of whack" and they would work by pure luck.

But I don't know anything about the particular snafus that happened there.

To take your example, the BIOS vendor disabled PAE. Do you really wanna deal
with fixing that? I mean, what else is b0rked there...?

So frankly, I'd turn that taint into a panic.

Because this CPU sounds to me like an all-bets-are-off thing.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[Borislav Petkov]

On Mon, Mar 23, 2026 at 10:50:49PM +0100, Borislav Petkov wrote:
> Because this CPU sounds to me like an all-bets-are-off thing.

AFAICT, if you cannot enable PAE, we can't enable long mode, right?

If so, this use case is for some 32-bit only broken gunk.

Why do we care?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[H. Peter Anvin]

On March 23, 2026 2:50:49 PM PDT, Borislav Petkov <bp@alien8.de> wrote:
>On Mon, Mar 23, 2026 at 02:40:47PM -0700, H. Peter Anvin wrote:
>> I did: the PAE case, which was not hypothetical. I'm sure I can dig up more.
>
>So let's start our justification from this POV: we have use cases X and Y and they
>would profit from getting these warning checks about required features not
>being set.
>
>I still am not persuaded that we want Linux to boot on those. Because those
>CPUs are clearly "out of whack" and they would work by pure luck.
>
>But I don't know anything about the particular snafus that happened there.
>
>To take your example, the BIOS vendor disabled PAE. Do you really wanna deal
>with fixing that? I mean, what else is b0rked there...?
>
>So frankly, I'd turn that taint into a panic.
>
>Because this CPU sounds to me like an all-bets-are-off thing.
>

Yes, hence the tainting.

The PAE case is historic, but it was a concrete example I had in my head where this issue You also of course mention BIOS settings and there are VMs ...

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[Borislav Petkov]

On Mon, Mar 23, 2026 at 03:03:34PM -0700, H. Peter Anvin wrote:
> Yes, hence the tainting.
> 
> The PAE case is historic, but it was a concrete example I had in my head
> where this issue You also of course mention BIOS settings and there are VMs
> ...

I'm still not convinced. But it is just one function so whateva.

I'll buy you a beer the first time we hit this in conjunction with a real use
case!

(No, silly VMs don't count.)

:-)

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[H. Peter Anvin]

On March 23, 2026 3:09:55 PM PDT, Borislav Petkov <bp@alien8.de> wrote:
>On Mon, Mar 23, 2026 at 03:03:34PM -0700, H. Peter Anvin wrote:
>> Yes, hence the tainting.
>> 
>> The PAE case is historic, but it was a concrete example I had in my head
>> where this issue You also of course mention BIOS settings and there are VMs
>> ...
>
>I'm still not convinced. But it is just one function so whateva.
>
>I'll buy you a beer the first time we hit this in conjunction with a real use
>case!
>
>(No, silly VMs don't count.)
>
>:-)
>

Heh :)

Re: [PATCH v11 4/4] x86/cpu: Clear feature bits whose dependencies were cleared

[Maciej Wieczor-Retman]

On 2026-03-23 at 20:30:33 +0100, Borislav Petkov wrote:
>On Mon, Mar 23, 2026 at 11:57:45AM -0700, H. Peter Anvin wrote:
>> I do have to say that using awk for these things, especially being restricted
>> to POSIX awk, is really awful as it really is pushing the limits of that tool.
>> String processing in C is most definitely no fun either -- the single biggest
>> shortcoming of what is otherwise one of my favorite languages -- but it
>> *would* also be possible to refactor cpuid-deps.c so that it can be compiled
>> in user space and linked with a C tool that would replace cpufeaturesmask.awk,
>> mkcapflags.sh, and boot/mkcpustr.c.
>> 
>> This seemed to me to be a bit excessive in terms of churn and complexity, but
>> perhaps this is what you are looking for?
>
>I was actually suggesting we handle the dependent features in
>apply_forced_caps() and keep that all in one place.
>
>As to the whole glue handling deps, feature flags, names, yadda yadda, yap,
>I agree, it ain't easy and I need to grep each time where the generated files
>are and go stare at them.
>
>So I really like the idea of replacing it all with a C tool which you can
>massage in luserspace and replace all that other gunk and we won't depend on
>the awk flavor or shell script or whatnot but keep everything in a single
>C program and a header.
>
>I don't know whether that would work and how ugly it would become - we need to
>try it I guess - but the idea ure sounds enticing...
>
>Maybe something for a future todo.
>
>:-)
>
>-- 
>Regards/Gruss,
>    Boris.
>
>https://people.kernel.org/tglx/notes-about-netiquette

I'll move the cpuid_deps[] checking to apply_forced_caps() for now then.

I can try giving the C tool a shot too, we were previously thinking that merging
x86_cap_flags[] and x86_bug_flags[] could be a nice cleanup. But using the
current scripts that turned out to not have any real benefits, maybe that's
worth looking into as well when rewriting the scripts into C?

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[Maciej Wieczor-Retman]

On 2026-03-20 at 17:31:27 -0700, Pawan Gupta wrote:
>On Fri, Mar 20, 2026 at 12:50:25PM +0000, Maciej Wieczor-Retman wrote:
>...
>> +/*
>> + * As a sanity check compare the final x86_capability bitmask with the initial
>> + * predefined required feature bits.
>> + */
>> +static void verify_required_features(const struct cpuinfo_x86 *c)
>> +{
>> +	u32 required_features[NCAPINTS] = REQUIRED_MASK_INIT;
>> +	DECLARE_BITMAP(missing, NCAPINTS * 32);
>> +	char cap_buf[X86_CAP_BUF_SIZE];
>> +	u32 *missing_u32;
>> +	unsigned int i;
>> +	u32 error = 0;
>> +
>> +	memset(missing, 0, sizeof(missing));
>> +	missing_u32 = (u32 *)missing;
>> +
>> +	for (i = 0; i < NCAPINTS; i++) {
>> +		missing_u32[i] = ~c->x86_capability[i] & required_features[i];
>> +		error |= missing_u32[i];
>> +	}
>> +
>> +	if (!error)
>> +		return;
>> +
>> +	/* At least one required feature is missing */
>> +	pr_warn("CPU %d: missing required feature(s):", c->cpu_index);
>> +	for_each_set_bit(i, missing, NCAPINTS * 32)
>> +		pr_cont(" %s", x86_cap_name(i, cap_buf));
>> +	pr_cont("\n");
>> +	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
>> +}
>
>Do we need 2 loops? Can this be simplified as below:
>
>static void verify_required_features(const struct cpuinfo_x86 *c)
>{
>	u32 required_features[NCAPINTS + 1] = REQUIRED_MASK_INIT;
>	char cap_buf[X86_CAP_BUF_SIZE];
>	int i, error = 0;

Isn't this [NCAPINTS + 1] still a problem because for_each_set_bit() works in 64
bit chunks? If NCAPINTS becomes an odd number in the future, the
required_features[] last 32 bits will be uninitialized - REQUIRED_MASK_INIT is
of (NCAPINTS * sizeof(u32)) size. So they might have some bits set and trigger
the pr_warn() below.

But what do you think about this?

	/*
	 * As REQUIRED_MASK_INIT is NCAPINTS long clear the last word of
	 * required_features in case NCAPINTS is odd so it can be parsed in
	 * 64 bit chunks by for_each_set_bit().
	 */
	required_features[NCAPINTS] = 0;

it feels less confusing than what I was trying before with the differently sized
pointers. And it explains the + 1 for anyone that wouldn't get it straight
away.

>
>	for_each_set_bit(i, (unsigned long *)required_features, NCAPINTS * 32) {
>		if (test_bit(i, (unsigned long *)c->x86_capability))
>			continue;
>		if (!error)
>			pr_warn("CPU %d: missing required feature(s):", c->cpu_index);
>		pr_cont(" %s", x86_cap_name(i, cap_buf));
>		error = 1;
>	}
>
>	if (!error)
>		return;
>
>	pr_cont("\n");
>	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
>}

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[Pawan Gupta]

On Thu, Mar 26, 2026 at 06:36:15PM +0000, Maciej Wieczor-Retman wrote:
> >Do we need 2 loops? Can this be simplified as below:
> >
> >static void verify_required_features(const struct cpuinfo_x86 *c)
> >{
> >	u32 required_features[NCAPINTS + 1] = REQUIRED_MASK_INIT;
> >	char cap_buf[X86_CAP_BUF_SIZE];
> >	int i, error = 0;
> 
> Isn't this [NCAPINTS + 1] still a problem because for_each_set_bit() works in 64
> bit chunks? If NCAPINTS becomes an odd number in the future, the
> required_features[] last 32 bits will be uninitialized - REQUIRED_MASK_INIT is
> of (NCAPINTS * sizeof(u32)) size. So they might have some bits set and trigger
> the pr_warn() below.

Isn't a partially initialized array always zeroed out for the uninitialized
part?

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[Maciej Wieczor-Retman]

On 2026-03-26 at 12:04:30 -0700, Pawan Gupta wrote:
>On Thu, Mar 26, 2026 at 06:36:15PM +0000, Maciej Wieczor-Retman wrote:
>> >Do we need 2 loops? Can this be simplified as below:
>> >
>> >static void verify_required_features(const struct cpuinfo_x86 *c)
>> >{
>> >	u32 required_features[NCAPINTS + 1] = REQUIRED_MASK_INIT;
>> >	char cap_buf[X86_CAP_BUF_SIZE];
>> >	int i, error = 0;
>>
>> Isn't this [NCAPINTS + 1] still a problem because for_each_set_bit() works in 64
>> bit chunks? If NCAPINTS becomes an odd number in the future, the
>> required_features[] last 32 bits will be uninitialized - REQUIRED_MASK_INIT is
>> of (NCAPINTS * sizeof(u32)) size. So they might have some bits set and trigger
>> the pr_warn() below.
>
>Isn't a partially initialized array always zeroed out for the uninitialized
>part?

Ah okay, my bad. Right, it should be okay then. Thanks!

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[H. Peter Anvin]

[-- Attachment #1: Type: text/plain, Size: 1832 bytes --]

On 2026-03-26 12:11, Maciej Wieczor-Retman wrote:
> On 2026-03-26 at 12:04:30 -0700, Pawan Gupta wrote:
>> On Thu, Mar 26, 2026 at 06:36:15PM +0000, Maciej Wieczor-Retman wrote:
>>>> Do we need 2 loops? Can this be simplified as below:
>>>>
>>>> static void verify_required_features(const struct cpuinfo_x86 *c)
>>>> {
>>>> 	u32 required_features[NCAPINTS + 1] = REQUIRED_MASK_INIT;
>>>> 	char cap_buf[X86_CAP_BUF_SIZE];
>>>> 	int i, error = 0;
>>>
>>> Isn't this [NCAPINTS + 1] still a problem because for_each_set_bit() works in 64
>>> bit chunks? If NCAPINTS becomes an odd number in the future, the
>>> required_features[] last 32 bits will be uninitialized - REQUIRED_MASK_INIT is
>>> of (NCAPINTS * sizeof(u32)) size. So they might have some bits set and trigger
>>> the pr_warn() below.
>>
>> Isn't a partially initialized array always zeroed out for the uninitialized
>> part?
> 
> Ah okay, my bad. Right, it should be okay then. Thanks!
> 

That being said, I would personally like to see an explicit assignment from
REQUIRED_MASK_INIT into an automatic variable replaced with a memcpy() from a
(possibly static) const array. It might be useful elsewhere, and it would
avoid compilers sometimes creating really ugly code.

One thing that matters here is that these bitmaps are *already* accessed using
bitop operations. Therefore, if this is a problem *here*, then it is a problem
*everywhere*. The simplest way to deal with it is probably to require NCAPINTS
and NBUGINTS to be even, even (pun intended) if that means a temporarily
unused word at the end of the array. That doesn't even require any code
changes, just a statement at the top of cpufeatures.h (see attached patch for
an untested example.)

A more bespoke variant would be to script-generate NCAPINTS and NBUGINTS, but
that might have other problems.

	-hpa

[-- Attachment #2: diff --]
[-- Type: text/plain, Size: 1636 bytes --]

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index dbe104df339b..ca9ab8a7a4ee 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -3,18 +3,37 @@
 #define _ASM_X86_CPUFEATURES_H
 
 /*
- * Defines x86 CPU feature bits
+ * Defines x86 CPU feature bits.
+ */
+
+/*
+ * Number of words of features and bugs, respectively.
+ *
+ * These should be even, as these arrays can be accessed by bitmask operations that
+ * use "unsigned long", which is 64 bits on x86-64.
+ *
+ * These must be expressed as decimal constants as they are read
+ * by scripts.
  */
 #define NCAPINTS			22	   /* N 32-bit words worth of info */
 #define NBUGINTS			2	   /* N 32-bit bug flags */
 
+#if (NCAPINTS | NBUGINTS) & 1
+# error "NCAPINTS and NBUGINTS must be even, just increment any odd number by one"
+#endif
+
 /*
- * Note: If the comment begins with a quoted string, that string is used
- * in /proc/cpuinfo instead of the macro name.  Otherwise, this feature
- * bit is not displayed in /proc/cpuinfo at all.
+ * Note: If the comment begins with a quoted string, that string is
+ * displayed in /proc/cpuinfo. Otherwise, this feature bit is not
+ * displayed in /proc/cpuinfo at all.
+ *
+ * This string should be in lower case and match C identifier rules.
  *
  * When adding new features here that depend on other features,
  * please update the table in kernel/cpu/cpuid-deps.c as well.
+ *
+ * As this file is read by scripts, the format of each of these lines
+ * must be strictly followed.
  */
 
 /* Intel-defined CPU features, CPUID level 0x00000001 (EDX), word 0 */

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[H. Peter Anvin]

On 2026-03-27 18:52, H. Peter Anvin wrote:
> 
> One thing that matters here is that these bitmaps are *already* accessed using
> bitop operations. Therefore, if this is a problem *here*, then it is a problem
> *everywhere*. The simplest way to deal with it is probably to require NCAPINTS
> and NBUGINTS to be even, even (pun intended) if that means a temporarily
> unused word at the end of the array. That doesn't even require any code
> changes, just a statement at the top of cpufeatures.h (see attached patch for
> an untested example.)
> 

Untested indeed. I just realized this breaks cpufeaturemasks.awk:

# Note: this blithely assumes that each word has at least one
# feature defined in it; if not, something else is wrong!

	-hpa

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[Maciej Wieczor-Retman]

On 2026-03-27 at 19:01:01 -0700, H. Peter Anvin wrote:
>On 2026-03-27 18:52, H. Peter Anvin wrote:
>> 
>> One thing that matters here is that these bitmaps are *already* accessed using
>> bitop operations. Therefore, if this is a problem *here*, then it is a problem
>> *everywhere*. The simplest way to deal with it is probably to require NCAPINTS
>> and NBUGINTS to be even, even (pun intended) if that means a temporarily
>> unused word at the end of the array. That doesn't even require any code
>> changes, just a statement at the top of cpufeatures.h (see attached patch for
>> an untested example.)
>> 
>
>Untested indeed. I just realized this breaks cpufeaturemasks.awk:
>
># Note: this blithely assumes that each word has at least one
># feature defined in it; if not, something else is wrong!
>
>	-hpa
>

I think you'd also need to modify enum cpuid_leafs. There is a check at some
point that verifies if there is enough of these enums to match NCAPINTS.

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[Maciej Wieczor-Retman]

On 2026-03-27 at 18:52:30 -0700, H. Peter Anvin wrote:
>On 2026-03-26 12:11, Maciej Wieczor-Retman wrote:
>> On 2026-03-26 at 12:04:30 -0700, Pawan Gupta wrote:
>>> On Thu, Mar 26, 2026 at 06:36:15PM +0000, Maciej Wieczor-Retman wrote:
>>>>> Do we need 2 loops? Can this be simplified as below:
>>>>>
>>>>> static void verify_required_features(const struct cpuinfo_x86 *c)
>>>>> {
>>>>> 	u32 required_features[NCAPINTS + 1] = REQUIRED_MASK_INIT;
>>>>> 	char cap_buf[X86_CAP_BUF_SIZE];
>>>>> 	int i, error = 0;
>>>>
>>>> Isn't this [NCAPINTS + 1] still a problem because for_each_set_bit() works in 64
>>>> bit chunks? If NCAPINTS becomes an odd number in the future, the
>>>> required_features[] last 32 bits will be uninitialized - REQUIRED_MASK_INIT is
>>>> of (NCAPINTS * sizeof(u32)) size. So they might have some bits set and trigger
>>>> the pr_warn() below.
>>>
>>> Isn't a partially initialized array always zeroed out for the uninitialized
>>> part?
>> 
>> Ah okay, my bad. Right, it should be okay then. Thanks!
>> 
>
>That being said, I would personally like to see an explicit assignment from
>REQUIRED_MASK_INIT into an automatic variable replaced with a memcpy() from a
>(possibly static) const array. It might be useful elsewhere, and it would
>avoid compilers sometimes creating really ugly code.

So setting up something similar to cpu_caps_cleared[] that's initialized with
DISABLED_MASK_INIT - only do that with the required one, and then copy that to a
64-bit aligned local bitmap-array?

>One thing that matters here is that these bitmaps are *already* accessed using
>bitop operations. Therefore, if this is a problem *here*, then it is a problem
>*everywhere*. 

I think for example the set_bit()/clear_bit() bitops are not problematic while
for_each_set_bit() is, specfically used in this context. Most operations seem to
not affect or not be affected by the potential unaligned 32-bit. And while
briefly looking for other such cases I didn't find anything related to features,
ncapints etc.

But I agree, a systemic solution like trying to keep NCAPINTS even, would be
better than adding band aids to the issue.

>The simplest way to deal with it is probably to require NCAPINTS
>and NBUGINTS to be even, even (pun intended) if that means a temporarily
>unused word at the end of the array. That doesn't even require any code
>changes, just a statement at the top of cpufeatures.h (see attached patch for
>an untested example.)
>
>A more bespoke variant would be to script-generate NCAPINTS and NBUGINTS, but
>that might have other problems.
>
>	-hpa

>diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
>index dbe104df339b..ca9ab8a7a4ee 100644
>--- a/arch/x86/include/asm/cpufeatures.h
>+++ b/arch/x86/include/asm/cpufeatures.h
>@@ -3,18 +3,37 @@
> #define _ASM_X86_CPUFEATURES_H
> 
> /*
>- * Defines x86 CPU feature bits
>+ * Defines x86 CPU feature bits.
>+ */
>+
>+/*
>+ * Number of words of features and bugs, respectively.
>+ *
>+ * These should be even, as these arrays can be accessed by bitmask operations that
>+ * use "unsigned long", which is 64 bits on x86-64.
>+ *
>+ * These must be expressed as decimal constants as they are read
>+ * by scripts.
>  */
> #define NCAPINTS			22	   /* N 32-bit words worth of info */
> #define NBUGINTS			2	   /* N 32-bit bug flags */
> 
>+#if (NCAPINTS | NBUGINTS) & 1
>+# error "NCAPINTS and NBUGINTS must be even, just increment any odd number by one"
>+#endif
>+
> /*
>- * Note: If the comment begins with a quoted string, that string is used
>- * in /proc/cpuinfo instead of the macro name.  Otherwise, this feature
>- * bit is not displayed in /proc/cpuinfo at all.
>+ * Note: If the comment begins with a quoted string, that string is
>+ * displayed in /proc/cpuinfo. Otherwise, this feature bit is not
>+ * displayed in /proc/cpuinfo at all.
>+ *
>+ * This string should be in lower case and match C identifier rules.
>  *
>  * When adding new features here that depend on other features,
>  * please update the table in kernel/cpu/cpuid-deps.c as well.
>+ *
>+ * As this file is read by scripts, the format of each of these lines
>+ * must be strictly followed.
>  */
> 
> /* Intel-defined CPU features, CPUID level 0x00000001 (EDX), word 0 */


-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[Pawan Gupta]

On Mon, Mar 30, 2026 at 10:09:47AM +0000, Maciej Wieczor-Retman wrote:
> On 2026-03-27 at 18:52:30 -0700, H. Peter Anvin wrote:
> >On 2026-03-26 12:11, Maciej Wieczor-Retman wrote:
> >> On 2026-03-26 at 12:04:30 -0700, Pawan Gupta wrote:
> >>> On Thu, Mar 26, 2026 at 06:36:15PM +0000, Maciej Wieczor-Retman wrote:
> >>>>> Do we need 2 loops? Can this be simplified as below:
> >>>>>
> >>>>> static void verify_required_features(const struct cpuinfo_x86 *c)
> >>>>> {
> >>>>> 	u32 required_features[NCAPINTS + 1] = REQUIRED_MASK_INIT;
> >>>>> 	char cap_buf[X86_CAP_BUF_SIZE];
> >>>>> 	int i, error = 0;
> >>>>
> >>>> Isn't this [NCAPINTS + 1] still a problem because for_each_set_bit() works in 64
> >>>> bit chunks? If NCAPINTS becomes an odd number in the future, the
> >>>> required_features[] last 32 bits will be uninitialized - REQUIRED_MASK_INIT is
> >>>> of (NCAPINTS * sizeof(u32)) size. So they might have some bits set and trigger
> >>>> the pr_warn() below.
> >>>
> >>> Isn't a partially initialized array always zeroed out for the uninitialized
> >>> part?
> >> 
> >> Ah okay, my bad. Right, it should be okay then. Thanks!
> >> 
> >
> >That being said, I would personally like to see an explicit assignment from
> >REQUIRED_MASK_INIT into an automatic variable replaced with a memcpy() from a
> >(possibly static) const array. It might be useful elsewhere, and it would
> >avoid compilers sometimes creating really ugly code.
> 
> So setting up something similar to cpu_caps_cleared[] that's initialized with
> DISABLED_MASK_INIT - only do that with the required one, and then copy that to a
> 64-bit aligned local bitmap-array?
> 
> >One thing that matters here is that these bitmaps are *already* accessed using
> >bitop operations. Therefore, if this is a problem *here*, then it is a problem
> >*everywhere*. 
> 
> I think for example the set_bit()/clear_bit() bitops are not problematic while
> for_each_set_bit() is, specfically used in this context. Most operations seem to
> not affect or not be affected by the potential unaligned 32-bit. And while
> briefly looking for other such cases I didn't find anything related to features,
> ncapints etc.
> 
> But I agree, a systemic solution like trying to keep NCAPINTS even, would be
> better than adding band aids to the issue.

Maybe use the below alignment trick:

struct cpuinfo_x86 {
	...
        /*
         * Align to size of unsigned long because the x86_capability array
         * is passed to bitops which require the alignment. Use unnamed
         * union to enforce the array is aligned to size of unsigned long.
         */
         union {
                 __u32           x86_capability[NCAPINTS + NBUGINTS];
                 unsigned long   x86_capability_alignment;
         };

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[David Laight]

On Mon, 30 Mar 2026 10:09:47 +0000
Maciej Wieczor-Retman <m.wieczorretman@pm.me> wrote:

> On 2026-03-27 at 18:52:30 -0700, H. Peter Anvin wrote:
...
> >One thing that matters here is that these bitmaps are *already* accessed using
> >bitop operations. Therefore, if this is a problem *here*, then it is a problem
> >*everywhere*.   
> 
> I think for example the set_bit()/clear_bit() bitops are not problematic while
> for_each_set_bit() is, specfically used in this context. Most operations seem to
> not affect or not be affected by the potential unaligned 32-bit.

Oh they are...
Look up 'split lock'.

You must not cast int[] to long[] for the 'bit' functions.

Basically if the 'long' gets split over a cache-line boundary the cpu
has to do an 'old fashioned' lock of the inter-cpu bus in order to
perform the locked memory accesses.
That is both slow and kills the rest of the machine.

	David

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[Maciej Wieczor-Retman]

On 2026-03-30 at 22:24:24 +0100, David Laight wrote:
>On Mon, 30 Mar 2026 10:09:47 +0000
>Maciej Wieczor-Retman <m.wieczorretman@pm.me> wrote:
>
>> On 2026-03-27 at 18:52:30 -0700, H. Peter Anvin wrote:
>...
>> >One thing that matters here is that these bitmaps are *already* accessed using
>> >bitop operations. Therefore, if this is a problem *here*, then it is a problem
>> >*everywhere*.
>>
>> I think for example the set_bit()/clear_bit() bitops are not problematic while
>> for_each_set_bit() is, specfically used in this context. Most operations seem to
>> not affect or not be affected by the potential unaligned 32-bit.
>
>Oh they are...
>Look up 'split lock'.
>
>You must not cast int[] to long[] for the 'bit' functions.
>
>Basically if the 'long' gets split over a cache-line boundary the cpu
>has to do an 'old fashioned' lock of the inter-cpu bus in order to
>perform the locked memory accesses.
>That is both slow and kills the rest of the machine.
>
>	David

Oh, right, that is a problem. So the approach of declaring the long bitmap and
then copying over data there makes the most sense. Thanks for explaining!

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH v11 3/4] x86/cpu: Do a sanity check on required feature bits

[Maciej Wieczor-Retman]

On 2026-03-27 at 18:52:30 -0700, H. Peter Anvin wrote:
>On 2026-03-26 12:11, Maciej Wieczor-Retman wrote:
>> On 2026-03-26 at 12:04:30 -0700, Pawan Gupta wrote:
>>> On Thu, Mar 26, 2026 at 06:36:15PM +0000, Maciej Wieczor-Retman wrote:
>>>>> Do we need 2 loops? Can this be simplified as below:
>>>>>
>>>>> static void verify_required_features(const struct cpuinfo_x86 *c)
>>>>> {
>>>>> 	u32 required_features[NCAPINTS + 1] = REQUIRED_MASK_INIT;
>>>>> 	char cap_buf[X86_CAP_BUF_SIZE];
>>>>> 	int i, error = 0;
>>>>
>>>> Isn't this [NCAPINTS + 1] still a problem because for_each_set_bit() works in 64
>>>> bit chunks? If NCAPINTS becomes an odd number in the future, the
>>>> required_features[] last 32 bits will be uninitialized - REQUIRED_MASK_INIT is
>>>> of (NCAPINTS * sizeof(u32)) size. So they might have some bits set and trigger
>>>> the pr_warn() below.
>>>
>>> Isn't a partially initialized array always zeroed out for the uninitialized
>>> part?
>> 
>> Ah okay, my bad. Right, it should be okay then. Thanks!
>> 
>
>That being said, I would personally like to see an explicit assignment from
>REQUIRED_MASK_INIT into an automatic variable replaced with a memcpy() from a
>(possibly static) const array. It might be useful elsewhere, and it would
>avoid compilers sometimes creating really ugly code.

Did you have something like the following in mind?

I also noticed the alignment issue is resolved in cpu_caps_cleared[] by adding
the __aligned(sizeof(unsigned long)) to their declaration. With this I assume
the cpu_caps_required[] I added would also be fine to get casted as unsigned
long?

 arch/x86/kernel/cpu/common.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 8d024e0ecaea..68de2d5f8a73 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -739,6 +739,8 @@ static const char *table_lookup_model(struct cpuinfo_x86 *c)
 __u32 cpu_caps_cleared[NCAPINTS + NBUGINTS] __aligned(sizeof(unsigned long)) =
        DISABLED_MASK_INIT;
 __u32 cpu_caps_set[NCAPINTS + NBUGINTS] __aligned(sizeof(unsigned long));
+static const __u32 cpu_caps_required[NCAPINTS] __aligned(sizeof(unsigned long)) =
+       REQUIRED_MASK_INIT;

 #ifdef CONFIG_X86_32
 /* The 32-bit entry code needs to find cpu_entry_area. */
@@ -2011,10 +2013,13 @@ const char *x86_feature_name(unsigned int bit, char *buf)
  */
 static void verify_required_features(const struct cpuinfo_x86 *c)
 {
-       u32 required_features[NCAPINTS + 1] = REQUIRED_MASK_INIT;
+       DECLARE_BITMAP(required_features, NCAPINTS * 32);
        char cap_buf[X86_NAMELESS_FEAT_BUFLEN];
        int i, error = 0;

+       memset(required_features, 0, sizeof(required_features));
+       memcpy(required_features, cpu_caps_required, NCAPINTS * sizeof(u32));
+
        for_each_set_bit(i, (unsigned long *)required_features, NCAPINTS * 32) {
                if (test_bit(i, (unsigned long *)c->x86_capability))
                        continue;
--
2.53.0

-- 
Kind regards
Maciej Wieczór-Retman