On 7/22/26 10:18, Yeoreum Yun wrote:
>>> @@ -1730,7 +1730,8 @@ static int populate_pud(struct cpa_data *cpa, 
>>> unsigned long start, p4d_t *p4d,
>>>     /*
>>>      * Map everything starting from the Gb boundary, possibly with 1G pages
>>>      */
>>> -   while (boot_cpu_has(X86_FEATURE_GBPAGES) && end - start >= PUD_SIZE) {
>>> +   while (CONFIG_PGTABLE_LEVELS > 3 && boot_cpu_has(X86_FEATURE_GBPAGES) &&
>>> +          end - start >= PUD_SIZE) {
>>>             set_pud(pud, pud_mkhuge(pfn_pud(cpa->pfn,
>>>                                canon_pgprot(pud_pgprot))));
>> This is an OK approach. But there's a way to fix this site *and*
>> optimize a non-zero amount of other code at the same time. Add this hunk
>> to arch/x86/Kconfig.cpufeatures:
>>
>> config X86_DISABLED_FEATURE_GBPAGES
>>         def_bool y
>>         depends on X86_32
>>
>> That will turn the boot_cpu_has() check in to something that can be
>> resolved at compile time. It has the added advantage of compiling out
>> all of the code under X86_FEATURE_GBPAGES everywhere else in the tree.
> Does it? when I glimpse check, this wouldn't be compiled since
> there is no bit for X86_DISABLED_FEATURE_GBPAGES and defining the
> DISABLED bit for FEATURE_GBPAGES seems odd since bit X86_FEATURE_GBPAGES
> is already defined.

x86 is a special snowflake here and all the similarly-named things glued
together with magic makes them hard to grok.

Our X86_FEATURE_* bits normally compile down to a bit in a bitmap in
memory. But, there are also some optimizations in the helpers that
access those bits. The optimizations turn bit checks like:

        if (bitmap[N] & bit)
                ...

into a compile-time check:

        if (__builtin_constant_p(bit) && DISABLED_MASK_BIT_SET(bit) ? 0:
            (bitmap[N] & bit))
                ...

The DISABLED_MASK_BIT_SET() macro magic check if (for instance)
X86_DISABLED_FEATURE_GBPAGES is around. Some processing of the Kconfig
variables produces a header with X86_DISABLED_FEATURE_GBPAGES defined.

> Instead of CONFIG_PGTABLE_LEVEL > 3, as above, would it be better to
> add check IS_ENABLED(CONFIG_X86_DIRECT_GBPAGES)?

That would definitely change the behavior. It honestly might be the
_right_ change, but I'm ignoring that for the moment because even if it
is best it is fodder for another patch, not this one.

Please just keep the check against X86_FEATURE_GBPAGES for now.

Reply via email to