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

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

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