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