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

[Pawan Gupta <pawan.kumar.gupta@linux.intel.com>]

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().

> 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.