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