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

[Maciej Wieczor-Retman]

On 2026-03-10 at 16:13:25 -0700, Sohil Mehta wrote:
>On 3/10/2026 11:03 AM, Maciej Wieczor-Retman wrote:
>> From: Maciej Wieczor-Retman <maciej.wieczor-retman@xxxxxxxxx>
>> 
>> After CPU identification concludes, do a sanity check by comparing the
>> final x86_capability bitmask with the pre-defined required feature bits.
>> 
>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@xxxxxxxxx>
>> Acked-by: H. Peter Anvin (Intel) <hpa@xxxxxxxxx>
>> ---
>
>> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
>> index cfffbbda3d95..43ee2d55a708 100644
>> --- a/arch/x86/kernel/cpu/common.c
>> +++ b/arch/x86/kernel/cpu/common.c
>> @@ -2003,6 +2003,34 @@ 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. In case of a mismatch emit a warning with
>> + * all the missing feature names or X86_FEATURE_* numerals.
>> + */
>
>I feel the second sentence is more appropriate as part of the commit
>message instead of a code comment.

Okay, yeah, I can move it there.

>
>> +static void verify_required_features(const struct cpuinfo_x86 *c)
>> +{
>> +	u32 missing[NCAPINTS] = REQUIRED_MASK_INIT;
>> +	char cap_buf[X86_CAP_BUF_SIZE];
>> +	unsigned int i;
>> +	u32 error = 0;
>> +
>> +	for (i = 0; i < NCAPINTS; i++) {
>> +		missing[i] &= ~c->x86_capability[i];
>> +		error |= missing[i];
>> +	}
>> +
>> +	if (!error)
>> +		return;
>> +
>> +	/* At least one required feature is missing */
>> +	pr_warn("cpu %d: missing required feature(s):", c->cpu_index);
>
>s/cpu/CPU

Sure

>
>I haven't seen cpu_index being used commonly. I don't know the nuance
>between that and smp_processor_id(). It's probably the same in this case.

cpu_index gets set to 0 on the boot CPU before identify_cpu() is called and
is set to the cpu variable in identify_secondary_cpu(). So I think it should
work okay in this case.

>Also, it seems too noisy to print this for every CPU in a large core
>count system. But, pr_warn_once() won't work well with the pr_cont()
>implementation that you have.
>
>I don't have a better suggestion except for a static variable. Maybe
>someone else has a better idea..
>
>My assumption is that typically we won't run into this on a production
>system. But, VMs could encounter this. So, I am not sure.

As it is a sanity check the assumption is this should not happen unless
something is quite wrong on the system. At that point more warning logs seem
helpful rather than noisy. I only managed to trigger this warning by modifying
the boot code to clear some capability bits just before this function. Not sure
if there are any other trivial ways to trigger this by accident.

>
>> +	for_each_set_bit(i, (unsigned long *)missing, NCAPINTS << 5)
>
>NCAPINTS * 32 is the common usage across arch/x86.

Oh, indeed, I'll change it.

>
>> +		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...
>>   */
>> @@ -2132,6 +2160,8 @@ static void identify_cpu(struct cpuinfo_x86 *c)
>>  	mcheck_cpu_init(c);
>>  
>>  	numa_add_cpu(smp_processor_id());
>> +
>> +	verify_required_features(c);
>>  }
>>  
>>  /*
>

-- 
Kind regards
Maciej Wieczór-Retman