From: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>

After CPU identification concludes, do a sanity check by comparing the
final x86_capability bitmask with the pre-defined required feature bits.

Because for_each_set_bit() expects 64-bit values and feature bitmasks
are 32-bit use DECLARE_BITMAP() to avoid unaligned memory accesses if
NCAPINTS is ever an odd number.

Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
---
Changelog v11:
- Use DECLARE_BITMAP() on missing[] and recast to a u32 pointer for
  iterating. Do it to avoid unaligned memory accesses when later using
  for_each_set_bit() if NCAPINTS is going to ever become an odd number.
  missing[] was previously an u32 array and for_each_set_bit() works on
  unsigned long chunks of memory.
- Add a paragraph about the above to the patch message.
- Remove Peter's acked-by due to more changes.

Changelog v10:
- Shorten the comment before the sanity check.
- cpu -> CPU in the warning.
- NCAPINTS << 5 -> NCAPINTS * 32

Changelog v9:
- REQUIRED_MASK_INITIALIZER -> REQUIRED_MASK_INIT
- Redo the comments.
- Fix reverse xmas order.
- Inside for_each_set_bit: (void *) -> (unsigned long *).
- 16 -> X86_CAP_BUF_SIZE.

Changelog v6:
- Add Peter's acked-by tag.
- Rename patch subject to imperative form.
- Add a char buffer to the x86_cap_name() call.

 arch/x86/kernel/cpu/common.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

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);
+}
+
 /*
  * This does the hard work of actually picking apart the CPU stuff...
  */
@@ -2134,6 +2166,8 @@ static void identify_cpu(struct cpuinfo_x86 *c)
 	mcheck_cpu_init(c);
 
 	numa_add_cpu(smp_processor_id());
+
+	verify_required_features(c);
 }
 
 /*
-- 
2.53.0

On Fri, Mar 20, 2026 at 12:50:25PM +0000, Maciej Wieczor-Retman wrote:
> From: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> 
> After CPU identification concludes, do a sanity check by comparing the
> final x86_capability bitmask with the pre-defined required feature bits.

The use being?

AFAICT, the required features are:

$ cat arch/x86/include/generated/asm/cpufeaturemasks.h

...

/*
 * REQUIRED features:
 *
 *    FPU MSR PAE CX8 CMOV FXSR XMM XMM2 LM NOPL ALWAYS CPUID
 */

AFAICT, if *any* of those features are not set, the machine will crash'n'burn
anyway.

So the required features will be "enforced" pretty early :-)

Otherwise they're not really required.

And besides, what's

arch/x86/kernel/verify_cpu.S

for if not for that?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

On 2026-03-23 09:31, Borislav Petkov wrote:
> 
> AFAICT, if *any* of those features are not set, the machine will crash'n'burn
> anyway.
> 
> So the required features will be "enforced" pretty early :-)
> 
> Otherwise they're not really required.
> 
> And besides, what's
> 
> arch/x86/kernel/verify_cpu.S
> 
> for if not for that?
> 

That is not necessarily true at all. As such, this may be a really cheap way
to get a message out in case we get that far without problems.

For one thing, this runs -- at least on the BSP -- before either alternatives
patching or running user space, so there is plenty of features that may not
have been used yet.

For another thing, there are some features -- such as PAE to mention one --
that are present in some CPUs but disabled in CPUID because for some reason or
another the manufacturer found during testing that it doesn't always work
right. However, it is likely that a PAE kernel will successfully boot, and it
might even work on any one particular CPU. This is *exactly* what
TAINT_CPU_OUT_OF_SPEC is supposed to represent.

Finally, as Maciej reported, the user might have tried to explicitly override
a required feature.

verify_cpu.S serves a different purpose: is to enable features that are
required to even set up the kernel execution environment and that may be
switched off through various mechanisms.

verify_cpu.S is unfortunately not able to issue messages (not to mention that
it is written in assembly, *AND* it doesn't have access to all the rules and
exceptions that are coded into arch/x86/kernel/cpu/*.) There *is* a messaging
function in the BIOS stub, but there is no equivalent in for code that bypass
this stage (which is of course standard these days.) verify_cpu.S just returns
a single bit, and that only represents checking for a few very basic features.

On Mon, Mar 23, 2026 at 11:43:08AM -0700, H. Peter Anvin wrote:
> That is not necessarily true at all.

What does that even mean?! :)

> As such, this may be a really cheap way to get a message out in case we get
> that far without problems.

Meh.

> For one thing, this runs -- at least on the BSP -- before either alternatives
> patching or running user space, so there is plenty of features that may not
> have been used yet.

We're talking about required features here. What's wrong with verify_cpu()
testing required features and stopping if some of them are not present?

It is already checking some of them.

> For another thing, there are some features -- such as PAE to mention one --
> that are present in some CPUs but disabled in CPUID because for some reason or
> another the manufacturer found during testing that it doesn't always work
> right. However, it is likely that a PAE kernel will successfully boot, and it
> might even work on any one particular CPU. This is *exactly* what
> TAINT_CPU_OUT_OF_SPEC is supposed to represent.

And?

We're supposed to support such a CPU or somewhat wobbly only?

You want to be able to boot up to the point of checking required features in
C code, find out that PAE is not supported, taint the CPU but still run?

What for?

How do you explain the user that her machine is actually fine but we'll taint
the kernel and that it maybe works but maybe not and there are no guarantees?

> Finally, as Maciej reported, the user might have tried to explicitly override
> a required feature.

You can still catch it in verify_cpu. Catch it such that you simply stop
there.

If the luser is overriding required features, then she gets to keep both
pieces.

> verify_cpu.S serves a different purpose: is to enable features that are
> required to even set up the kernel execution environment and that may be
> switched off through various mechanisms.

And because we run it at so many places, then it can do those checks for us
too.

> verify_cpu.S is unfortunately not able to issue messages (not to mention that
> it is written in assembly,

We can convert it to C. We've done this before with other crap. :)

> *AND* it doesn't have access to all the rules and exceptions that are coded
> into arch/x86/kernel/cpu/*.) There *is* a messaging function in the BIOS
> stub, but there is no equivalent in for code that bypass this stage (which
> is of course standard these days.) verify_cpu.S just returns a single bit,
> and that only represents checking for a few very basic features.

Yes, I have seen the error message about this CPU not being supported very
early.

But I don't see the point for adding another function to verify required
features which is somewhere else where we're pretty much doing that checking
early.

And what's the point of booting up to C code and kernel proper and say that
some of the required features are off?

I think we should extend verify_cpu or convert it to C or have it call
a C function or whatever and do the checking once and for all and not boot
into a wobbly and tainted kernel.

As to showing a proper error message, what is the real use case we're chasing
here?

The CPU has all the required features - which is probably 99.999% of the cases
out there or it doesn't and then it deserves to blow up.

So what are we really "fixing" here...?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

On 2026-03-23 12:19, Borislav Petkov wrote:
> On Mon, Mar 23, 2026 at 11:43:08AM -0700, H. Peter Anvin wrote:
>> That is not necessarily true at all.
> 
> What does that even mean?! :)
> 
>> As such, this may be a really cheap way to get a message out in case we get
>> that far without problems.
> 
> Meh.
> 
>> For one thing, this runs -- at least on the BSP -- before either alternatives
>> patching or running user space, so there is plenty of features that may not
>> have been used yet.
> 
> We're talking about required features here. What's wrong with verify_cpu()
> testing required features and stopping if some of them are not present?
> 
> It is already checking some of them.
> 

Well, there is the bits which need to be in assembly because they

>> For another thing, there are some features -- such as PAE to mention one --
>> that are present in some CPUs but disabled in CPUID because for some reason or
>> another the manufacturer found during testing that it doesn't always work
>> right. However, it is likely that a PAE kernel will successfully boot, and it
>> might even work on any one particular CPU. This is *exactly* what
>> TAINT_CPU_OUT_OF_SPEC is supposed to represent.
> 
> And?
> 
> We're supposed to support such a CPU or somewhat wobbly only?
>
> You want to be able to boot up to the point of checking required features in
> C code, find out that PAE is not supported, taint the CPU but still run?
> 
> What for?
> 
> How do you explain the user that her machine is actually fine but we'll taint
> the kernel and that it maybe works but maybe not and there are no guarantees?

That is EXACTLY what TAINT_CPU_OUT_OF_SPEC means.


>> Finally, as Maciej reported, the user might have tried to explicitly override
>> a required feature.
> 
> You can still catch it in verify_cpu. Catch it such that you simply stop
> there.
> 
> If the luser is overriding required features, then she gets to keep both
> pieces.

It doesn't mean we can't at least TRY to warn things.

>> verify_cpu.S serves a different purpose: is to enable features that are
>> required to even set up the kernel execution environment and that may be
>> switched off through various mechanisms.
> 
> And because we run it at so many places, then it can do those checks for us
> too.
> 
>> verify_cpu.S is unfortunately not able to issue messages (not to mention that
>> it is written in assembly,
> 
> We can convert it to C. We've done this before with other crap. :)

No, you can't, because YOU CAN'T GET FAR ENOUGH ALONG TO RUN C CODE WITHOUT
IT. That is what is unique about verify_cpu.S. It should probably really be
called "enable_cpu.S".

It doesn't mean you can't do an earlier check, but at that point you pretty
much need the entire machinery of arch/x86/kernel/cpu.

We COULD push a lot of that code much earlier, and make it sharable with the
boot/compressed prekernel, but that is a cleanup on a whole different scale.

> Yes, I have seen the error message about this CPU not being supported very
> early.
> 
> But I don't see the point for adding another function to verify required
> features which is somewhere else where we're pretty much doing that checking
> early.

Well, for one thing: it lets us avoid more ad hoc messages in central code.

> And what's the point of booting up to C code and kernel proper and say that
> some of the required features are off?
> 
> I think we should extend verify_cpu or convert it to C or have it call
> a C function or whatever and do the checking once and for all and not boot
> into a wobbly and tainted kernel.
> 
> As to showing a proper error message, what is the real use case we're chasing
> here?
> 
> The CPU has all the required features - which is probably 99.999% of the cases
> out there or it doesn't and then it deserves to blow up.
> 
> So what are we really "fixing" here...?
You can make the same argument about #MC for example: why bother trying to get
a message out when the CPU is literally telling you that your system just broke?

The answer is because it helps the user understand what is wrong. Certainly,
you have no guarantee that you will actually get there, but in practice, in
many (but definitely not all) cases you WILL be able to get far enough along
to get the message out so that when the user wonders why their machine crashed
they have a clue.

	-hpa

On Mon, Mar 23, 2026 at 01:24:25PM -0700, H. Peter Anvin wrote:
> > So what are we really "fixing" here...?
> You can make the same argument about #MC for example: why bother trying to get
> a message out when the CPU is literally telling you that your system just broke?

I don't think you're parsing me right: do you have actual, real-life use cases
for which you want this or is this something hypothetical?

Because from where I'm standing, this sounds like let's slap another checking
function somewhere, we won't hit it ever but just in case, let's be prepared.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

I did: the PAE case, which was not hypothetical. I'm sure I can dig up more.

On Mon, Mar 23, 2026 at 02:40:47PM -0700, H. Peter Anvin wrote:
> I did: the PAE case, which was not hypothetical. I'm sure I can dig up more.

So let's start our justification from this POV: we have use cases X and Y and they
would profit from getting these warning checks about required features not
being set.

I still am not persuaded that we want Linux to boot on those. Because those
CPUs are clearly "out of whack" and they would work by pure luck.

But I don't know anything about the particular snafus that happened there.

To take your example, the BIOS vendor disabled PAE. Do you really wanna deal
with fixing that? I mean, what else is b0rked there...?

So frankly, I'd turn that taint into a panic.

Because this CPU sounds to me like an all-bets-are-off thing.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

On March 23, 2026 2:50:49 PM PDT, Borislav Petkov <bp@alien8.de> wrote:
>On Mon, Mar 23, 2026 at 02:40:47PM -0700, H. Peter Anvin wrote:
>> I did: the PAE case, which was not hypothetical. I'm sure I can dig up more.
>
>So let's start our justification from this POV: we have use cases X and Y and they
>would profit from getting these warning checks about required features not
>being set.
>
>I still am not persuaded that we want Linux to boot on those. Because those
>CPUs are clearly "out of whack" and they would work by pure luck.
>
>But I don't know anything about the particular snafus that happened there.
>
>To take your example, the BIOS vendor disabled PAE. Do you really wanna deal
>with fixing that? I mean, what else is b0rked there...?
>
>So frankly, I'd turn that taint into a panic.
>
>Because this CPU sounds to me like an all-bets-are-off thing.
>

Yes, hence the tainting.

The PAE case is historic, but it was a concrete example I had in my head where this issue You also of course mention BIOS settings and there are VMs ...

On Mon, Mar 23, 2026 at 03:03:34PM -0700, H. Peter Anvin wrote:
> Yes, hence the tainting.
> 
> The PAE case is historic, but it was a concrete example I had in my head
> where this issue You also of course mention BIOS settings and there are VMs
> ...

I'm still not convinced. But it is just one function so whateva.

I'll buy you a beer the first time we hit this in conjunction with a real use
case!

(No, silly VMs don't count.)

:-)

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

On March 23, 2026 3:09:55 PM PDT, Borislav Petkov <bp@alien8.de> wrote:
>On Mon, Mar 23, 2026 at 03:03:34PM -0700, H. Peter Anvin wrote:
>> Yes, hence the tainting.
>> 
>> The PAE case is historic, but it was a concrete example I had in my head
>> where this issue You also of course mention BIOS settings and there are VMs
>> ...
>
>I'm still not convinced. But it is just one function so whateva.
>
>I'll buy you a beer the first time we hit this in conjunction with a real use
>case!
>
>(No, silly VMs don't count.)
>
>:-)
>

Heh :)

On Mon, Mar 23, 2026 at 10:50:49PM +0100, Borislav Petkov wrote:
> Because this CPU sounds to me like an all-bets-are-off thing.

AFAICT, if you cannot enable PAE, we can't enable long mode, right?

If so, this use case is for some 32-bit only broken gunk.

Why do we care?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

On 2026-03-23 at 17:31:45 +0100, Borislav Petkov wrote:
>On Fri, Mar 20, 2026 at 12:50:25PM +0000, Maciej Wieczor-Retman wrote:
>> From: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>>
>> After CPU identification concludes, do a sanity check by comparing the
>> final x86_capability bitmask with the pre-defined required feature bits.
>
>The use being?
>
>AFAICT, the required features are:
>
>$ cat arch/x86/include/generated/asm/cpufeaturemasks.h
>
>...
>
>/*
> * REQUIRED features:
> *
> *    FPU MSR PAE CX8 CMOV FXSR XMM XMM2 LM NOPL ALWAYS CPUID
> */
>
>AFAICT, if *any* of those features are not set, the machine will crash'n'burn
>anyway.
>
>So the required features will be "enforced" pretty early :-)
>
>Otherwise they're not really required.
>
>And besides, what's
>
>arch/x86/kernel/verify_cpu.S
>
>for if not for that?
>
>--
>Regards/Gruss,
>    Boris.
>
>https://people.kernel.org/tglx/notes-about-netiquette

hpa requested to add this patch to the series [1]. As he explained the other modern
boot stubs don't include the same checks that the Linux kernel has.

[1] https://lore.kernel.org/all/5fd597a1-46fe-4f46-98ee-d346260c85b5@zytor.com/

-- 
Kind regards
Maciej Wieczór-Retman

On Mon, Mar 23, 2026 at 05:05:48PM +0000, Maciej Wieczor-Retman wrote:
> On 2026-03-23 at 17:31:45 +0100, Borislav Petkov wrote:
> >On Fri, Mar 20, 2026 at 12:50:25PM +0000, Maciej Wieczor-Retman wrote:
> >> From: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> >>
> >> After CPU identification concludes, do a sanity check by comparing the
> >> final x86_capability bitmask with the pre-defined required feature bits.
> >
> >The use being?
> >
> >AFAICT, the required features are:
> >
> >$ cat arch/x86/include/generated/asm/cpufeaturemasks.h
> >
> >...
> >
> >/*
> > * REQUIRED features:
> > *
> > *    FPU MSR PAE CX8 CMOV FXSR XMM XMM2 LM NOPL ALWAYS CPUID
> > */
> >
> >AFAICT, if *any* of those features are not set, the machine will crash'n'burn
> >anyway.
> >
> >So the required features will be "enforced" pretty early :-)
> >
> >Otherwise they're not really required.
> >
> >And besides, what's
> >
> >arch/x86/kernel/verify_cpu.S
> >
> >for if not for that?

> hpa requested to add this patch to the series [1]. As he explained the other modern
> boot stubs don't include the same checks that the Linux kernel has.
> 
> [1] https://lore.kernel.org/all/5fd597a1-46fe-4f46-98ee-d346260c85b5@zytor.com/

I don't understand.

We call verify_cpu() in startup_64 in kernel proper too.

So if anything's missing, it should be added there instead of adding yet
another let's verify features thing...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

On 2026-03-23 10:55, Borislav Petkov wrote:
> 
> I don't understand.
> 
> We call verify_cpu() in startup_64 in kernel proper too.
> 
> So if anything's missing, it should be added there instead of adding yet
> another let's verify features thing...
> 

See my response elsewhere in this thread.

	-hpa

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

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

On Thu, Mar 26, 2026 at 06:36:15PM +0000, Maciej Wieczor-Retman wrote:
> >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.

Isn't a partially initialized array always zeroed out for the uninitialized
part?

On 2026-03-26 at 12:04:30 -0700, Pawan Gupta wrote:
>On Thu, Mar 26, 2026 at 06:36:15PM +0000, Maciej Wieczor-Retman wrote:
>> >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.
>
>Isn't a partially initialized array always zeroed out for the uninitialized
>part?

Ah okay, my bad. Right, it should be okay then. Thanks!

-- 
Kind regards
Maciej Wieczór-Retman

On 2026-03-26 12:11, Maciej Wieczor-Retman wrote:
> On 2026-03-26 at 12:04:30 -0700, Pawan Gupta wrote:
>> On Thu, Mar 26, 2026 at 06:36:15PM +0000, Maciej Wieczor-Retman wrote:
>>>> 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.
>>
>> Isn't a partially initialized array always zeroed out for the uninitialized
>> part?
> 
> Ah okay, my bad. Right, it should be okay then. Thanks!
> 

That being said, I would personally like to see an explicit assignment from
REQUIRED_MASK_INIT into an automatic variable replaced with a memcpy() from a
(possibly static) const array. It might be useful elsewhere, and it would
avoid compilers sometimes creating really ugly code.

One thing that matters here is that these bitmaps are *already* accessed using
bitop operations. Therefore, if this is a problem *here*, then it is a problem
*everywhere*. The simplest way to deal with it is probably to require NCAPINTS
and NBUGINTS to be even, even (pun intended) if that means a temporarily
unused word at the end of the array. That doesn't even require any code
changes, just a statement at the top of cpufeatures.h (see attached patch for
an untested example.)

A more bespoke variant would be to script-generate NCAPINTS and NBUGINTS, but
that might have other problems.

	-hpa
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index dbe104df339b..ca9ab8a7a4ee 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -3,18 +3,37 @@
 #define _ASM_X86_CPUFEATURES_H
 
 /*
- * Defines x86 CPU feature bits
+ * Defines x86 CPU feature bits.
+ */
+
+/*
+ * Number of words of features and bugs, respectively.
+ *
+ * These should be even, as these arrays can be accessed by bitmask operations that
+ * use "unsigned long", which is 64 bits on x86-64.
+ *
+ * These must be expressed as decimal constants as they are read
+ * by scripts.
  */
 #define NCAPINTS			22	   /* N 32-bit words worth of info */
 #define NBUGINTS			2	   /* N 32-bit bug flags */
 
+#if (NCAPINTS | NBUGINTS) & 1
+# error "NCAPINTS and NBUGINTS must be even, just increment any odd number by one"
+#endif
+
 /*
- * Note: If the comment begins with a quoted string, that string is used
- * in /proc/cpuinfo instead of the macro name.  Otherwise, this feature
- * bit is not displayed in /proc/cpuinfo at all.
+ * Note: If the comment begins with a quoted string, that string is
+ * displayed in /proc/cpuinfo. Otherwise, this feature bit is not
+ * displayed in /proc/cpuinfo at all.
+ *
+ * This string should be in lower case and match C identifier rules.
  *
  * When adding new features here that depend on other features,
  * please update the table in kernel/cpu/cpuid-deps.c as well.
+ *
+ * As this file is read by scripts, the format of each of these lines
+ * must be strictly followed.
  */
 
 /* Intel-defined CPU features, CPUID level 0x00000001 (EDX), word 0 */

On 2026-03-27 at 18:52:30 -0700, H. Peter Anvin wrote:
>On 2026-03-26 12:11, Maciej Wieczor-Retman wrote:
>> On 2026-03-26 at 12:04:30 -0700, Pawan Gupta wrote:
>>> On Thu, Mar 26, 2026 at 06:36:15PM +0000, Maciej Wieczor-Retman wrote:
>>>>> 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.
>>>
>>> Isn't a partially initialized array always zeroed out for the uninitialized
>>> part?
>> 
>> Ah okay, my bad. Right, it should be okay then. Thanks!
>> 
>
>That being said, I would personally like to see an explicit assignment from
>REQUIRED_MASK_INIT into an automatic variable replaced with a memcpy() from a
>(possibly static) const array. It might be useful elsewhere, and it would
>avoid compilers sometimes creating really ugly code.

Did you have something like the following in mind?

I also noticed the alignment issue is resolved in cpu_caps_cleared[] by adding
the __aligned(sizeof(unsigned long)) to their declaration. With this I assume
the cpu_caps_required[] I added would also be fine to get casted as unsigned
long?

 arch/x86/kernel/cpu/common.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 8d024e0ecaea..68de2d5f8a73 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -739,6 +739,8 @@ static const char *table_lookup_model(struct cpuinfo_x86 *c)
 __u32 cpu_caps_cleared[NCAPINTS + NBUGINTS] __aligned(sizeof(unsigned long)) =
        DISABLED_MASK_INIT;
 __u32 cpu_caps_set[NCAPINTS + NBUGINTS] __aligned(sizeof(unsigned long));
+static const __u32 cpu_caps_required[NCAPINTS] __aligned(sizeof(unsigned long)) =
+       REQUIRED_MASK_INIT;

 #ifdef CONFIG_X86_32
 /* The 32-bit entry code needs to find cpu_entry_area. */
@@ -2011,10 +2013,13 @@ const char *x86_feature_name(unsigned int bit, char *buf)
  */
 static void verify_required_features(const struct cpuinfo_x86 *c)
 {
-       u32 required_features[NCAPINTS + 1] = REQUIRED_MASK_INIT;
+       DECLARE_BITMAP(required_features, NCAPINTS * 32);
        char cap_buf[X86_NAMELESS_FEAT_BUFLEN];
        int i, error = 0;

+       memset(required_features, 0, sizeof(required_features));
+       memcpy(required_features, cpu_caps_required, NCAPINTS * sizeof(u32));
+
        for_each_set_bit(i, (unsigned long *)required_features, NCAPINTS * 32) {
                if (test_bit(i, (unsigned long *)c->x86_capability))
                        continue;
--
2.53.0

-- 
Kind regards
Maciej Wieczór-Retman

On 2026-03-27 at 18:52:30 -0700, H. Peter Anvin wrote:
>On 2026-03-26 12:11, Maciej Wieczor-Retman wrote:
>> On 2026-03-26 at 12:04:30 -0700, Pawan Gupta wrote:
>>> On Thu, Mar 26, 2026 at 06:36:15PM +0000, Maciej Wieczor-Retman wrote:
>>>>> 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.
>>>
>>> Isn't a partially initialized array always zeroed out for the uninitialized
>>> part?
>> 
>> Ah okay, my bad. Right, it should be okay then. Thanks!
>> 
>
>That being said, I would personally like to see an explicit assignment from
>REQUIRED_MASK_INIT into an automatic variable replaced with a memcpy() from a
>(possibly static) const array. It might be useful elsewhere, and it would
>avoid compilers sometimes creating really ugly code.

So setting up something similar to cpu_caps_cleared[] that's initialized with
DISABLED_MASK_INIT - only do that with the required one, and then copy that to a
64-bit aligned local bitmap-array?

>One thing that matters here is that these bitmaps are *already* accessed using
>bitop operations. Therefore, if this is a problem *here*, then it is a problem
>*everywhere*. 

I think for example the set_bit()/clear_bit() bitops are not problematic while
for_each_set_bit() is, specfically used in this context. Most operations seem to
not affect or not be affected by the potential unaligned 32-bit. And while
briefly looking for other such cases I didn't find anything related to features,
ncapints etc.

But I agree, a systemic solution like trying to keep NCAPINTS even, would be
better than adding band aids to the issue.

>The simplest way to deal with it is probably to require NCAPINTS
>and NBUGINTS to be even, even (pun intended) if that means a temporarily
>unused word at the end of the array. That doesn't even require any code
>changes, just a statement at the top of cpufeatures.h (see attached patch for
>an untested example.)
>
>A more bespoke variant would be to script-generate NCAPINTS and NBUGINTS, but
>that might have other problems.
>
>	-hpa

>diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
>index dbe104df339b..ca9ab8a7a4ee 100644
>--- a/arch/x86/include/asm/cpufeatures.h
>+++ b/arch/x86/include/asm/cpufeatures.h
>@@ -3,18 +3,37 @@
> #define _ASM_X86_CPUFEATURES_H
> 
> /*
>- * Defines x86 CPU feature bits
>+ * Defines x86 CPU feature bits.
>+ */
>+
>+/*
>+ * Number of words of features and bugs, respectively.
>+ *
>+ * These should be even, as these arrays can be accessed by bitmask operations that
>+ * use "unsigned long", which is 64 bits on x86-64.
>+ *
>+ * These must be expressed as decimal constants as they are read
>+ * by scripts.
>  */
> #define NCAPINTS			22	   /* N 32-bit words worth of info */
> #define NBUGINTS			2	   /* N 32-bit bug flags */
> 
>+#if (NCAPINTS | NBUGINTS) & 1
>+# error "NCAPINTS and NBUGINTS must be even, just increment any odd number by one"
>+#endif
>+
> /*
>- * Note: If the comment begins with a quoted string, that string is used
>- * in /proc/cpuinfo instead of the macro name.  Otherwise, this feature
>- * bit is not displayed in /proc/cpuinfo at all.
>+ * Note: If the comment begins with a quoted string, that string is
>+ * displayed in /proc/cpuinfo. Otherwise, this feature bit is not
>+ * displayed in /proc/cpuinfo at all.
>+ *
>+ * This string should be in lower case and match C identifier rules.
>  *
>  * When adding new features here that depend on other features,
>  * please update the table in kernel/cpu/cpuid-deps.c as well.
>+ *
>+ * As this file is read by scripts, the format of each of these lines
>+ * must be strictly followed.
>  */
> 
> /* Intel-defined CPU features, CPUID level 0x00000001 (EDX), word 0 */


-- 
Kind regards
Maciej Wieczór-Retman

On Mon, 30 Mar 2026 10:09:47 +0000
Maciej Wieczor-Retman <m.wieczorretman@pm.me> wrote:

> On 2026-03-27 at 18:52:30 -0700, H. Peter Anvin wrote:
...
> >One thing that matters here is that these bitmaps are *already* accessed using
> >bitop operations. Therefore, if this is a problem *here*, then it is a problem
> >*everywhere*.   
> 
> I think for example the set_bit()/clear_bit() bitops are not problematic while
> for_each_set_bit() is, specfically used in this context. Most operations seem to
> not affect or not be affected by the potential unaligned 32-bit.

Oh they are...
Look up 'split lock'.

You must not cast int[] to long[] for the 'bit' functions.

Basically if the 'long' gets split over a cache-line boundary the cpu
has to do an 'old fashioned' lock of the inter-cpu bus in order to
perform the locked memory accesses.
That is both slow and kills the rest of the machine.

	David

On 2026-03-30 at 22:24:24 +0100, David Laight wrote:
>On Mon, 30 Mar 2026 10:09:47 +0000
>Maciej Wieczor-Retman <m.wieczorretman@pm.me> wrote:
>
>> On 2026-03-27 at 18:52:30 -0700, H. Peter Anvin wrote:
>...
>> >One thing that matters here is that these bitmaps are *already* accessed using
>> >bitop operations. Therefore, if this is a problem *here*, then it is a problem
>> >*everywhere*.
>>
>> I think for example the set_bit()/clear_bit() bitops are not problematic while
>> for_each_set_bit() is, specfically used in this context. Most operations seem to
>> not affect or not be affected by the potential unaligned 32-bit.
>
>Oh they are...
>Look up 'split lock'.
>
>You must not cast int[] to long[] for the 'bit' functions.
>
>Basically if the 'long' gets split over a cache-line boundary the cpu
>has to do an 'old fashioned' lock of the inter-cpu bus in order to
>perform the locked memory accesses.
>That is both slow and kills the rest of the machine.
>
>	David

Oh, right, that is a problem. So the approach of declaring the long bitmap and
then copying over data there makes the most sense. Thanks for explaining!

-- 
Kind regards
Maciej Wieczór-Retman

On Mon, Mar 30, 2026 at 10:09:47AM +0000, Maciej Wieczor-Retman wrote:
> On 2026-03-27 at 18:52:30 -0700, H. Peter Anvin wrote:
> >On 2026-03-26 12:11, Maciej Wieczor-Retman wrote:
> >> On 2026-03-26 at 12:04:30 -0700, Pawan Gupta wrote:
> >>> On Thu, Mar 26, 2026 at 06:36:15PM +0000, Maciej Wieczor-Retman wrote:
> >>>>> 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.
> >>>
> >>> Isn't a partially initialized array always zeroed out for the uninitialized
> >>> part?
> >> 
> >> Ah okay, my bad. Right, it should be okay then. Thanks!
> >> 
> >
> >That being said, I would personally like to see an explicit assignment from
> >REQUIRED_MASK_INIT into an automatic variable replaced with a memcpy() from a
> >(possibly static) const array. It might be useful elsewhere, and it would
> >avoid compilers sometimes creating really ugly code.
> 
> So setting up something similar to cpu_caps_cleared[] that's initialized with
> DISABLED_MASK_INIT - only do that with the required one, and then copy that to a
> 64-bit aligned local bitmap-array?
> 
> >One thing that matters here is that these bitmaps are *already* accessed using
> >bitop operations. Therefore, if this is a problem *here*, then it is a problem
> >*everywhere*. 
> 
> I think for example the set_bit()/clear_bit() bitops are not problematic while
> for_each_set_bit() is, specfically used in this context. Most operations seem to
> not affect or not be affected by the potential unaligned 32-bit. And while
> briefly looking for other such cases I didn't find anything related to features,
> ncapints etc.
> 
> But I agree, a systemic solution like trying to keep NCAPINTS even, would be
> better than adding band aids to the issue.

Maybe use the below alignment trick:

struct cpuinfo_x86 {
	...
        /*
         * Align to size of unsigned long because the x86_capability array
         * is passed to bitops which require the alignment. Use unnamed
         * union to enforce the array is aligned to size of unsigned long.
         */
         union {
                 __u32           x86_capability[NCAPINTS + NBUGINTS];
                 unsigned long   x86_capability_alignment;
         };

On 2026-03-27 18:52, H. Peter Anvin wrote:
> 
> One thing that matters here is that these bitmaps are *already* accessed using
> bitop operations. Therefore, if this is a problem *here*, then it is a problem
> *everywhere*. The simplest way to deal with it is probably to require NCAPINTS
> and NBUGINTS to be even, even (pun intended) if that means a temporarily
> unused word at the end of the array. That doesn't even require any code
> changes, just a statement at the top of cpufeatures.h (see attached patch for
> an untested example.)
> 

Untested indeed. I just realized this breaks cpufeaturemasks.awk:

# Note: this blithely assumes that each word has at least one
# feature defined in it; if not, something else is wrong!

	-hpa

On 2026-03-27 at 19:01:01 -0700, H. Peter Anvin wrote:
>On 2026-03-27 18:52, H. Peter Anvin wrote:
>> 
>> One thing that matters here is that these bitmaps are *already* accessed using
>> bitop operations. Therefore, if this is a problem *here*, then it is a problem
>> *everywhere*. The simplest way to deal with it is probably to require NCAPINTS
>> and NBUGINTS to be even, even (pun intended) if that means a temporarily
>> unused word at the end of the array. That doesn't even require any code
>> changes, just a statement at the top of cpufeatures.h (see attached patch for
>> an untested example.)
>> 
>
>Untested indeed. I just realized this breaks cpufeaturemasks.awk:
>
># Note: this blithely assumes that each word has at least one
># feature defined in it; if not, something else is wrong!
>
>	-hpa
>

I think you'd also need to modify enum cpuid_leafs. There is a check at some
point that verifies if there is enough of these enums to match NCAPINTS.

-- 
Kind regards
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

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.

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