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

[Maciej Wieczor-Retman]

On 2026-03-23 at 11:16:43 -0700, Pawan Gupta wrote:
>On Sat, Mar 21, 2026 at 05:58:18AM +0000, Maciej Wieczór-Retman wrote:
>> 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.
>
>You may be right, but relying on number of instructions in the loop for
>print syncronization seems flawed to begin with. What probably saves from
>output being garbled is the printk machinery caching the string until a
>'\n'. I am not fully sure.
>
>> Otherwise I liked in the previous approach the steps of setting up a
>> bitmask with simple bitwise logic operations, then checking the results
>> later.
>
>My main motivation for suggesting the change was to try and use the
>existing infrastructure for bit operations much as possible. Even in my
>suggestion test_bit(cap) can be replaced with test_cpu_cap().

Okay, yes, I suppose that is a good idea.

>> 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 :)
>
>As I looked at it again, I see that cpu_has() helpers unconditionally
>returns true for all the required features.
>
>#define cpu_has(c, bit)                                                 \
>        (__builtin_constant_p(bit) && REQUIRED_MASK_BIT_SET(bit) ? 1 :  \
>         test_cpu_cap(c, bit))
>
>So this seems inline with Boris's comment, system should crash and burn if
>any of the required features is missing.

Yeah, that would make sense too. What I checked is that this sanity check can
catch if any kernel calls cleared any of the required bits from the
x86_capability[] array. I might have to figure out a test to actually disable
something required that doesn't completely crash everything at the same time.

-- 
Kind regards
Maciej Wieczór-Retman