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

["H. Peter Anvin" <hpa@zytor.com>]

[-- 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 */