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

[Maciej Wieczor-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:
>...
>> +/*
>> + * 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;

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.

But what do you think about this?

	/*
	 * As REQUIRED_MASK_INIT is NCAPINTS long clear the last word of
	 * required_features in case NCAPINTS is odd so it can be parsed in
	 * 64 bit chunks by for_each_set_bit().
	 */
	required_features[NCAPINTS] = 0;

it feels less confusing than what I was trying before with the differently sized
pointers. And it explains the + 1 for anyone that wouldn't get it straight
away.

>
>	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);
>}

-- 
Kind regards
Maciej Wieczór-Retman