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