On Wed, Jul 22, 2026 at 09:33:55AM -0700, Dave Hansen wrote:
> On 7/22/26 08:30, Yeoreum Yun wrote:
> > We want to rework how set_pXd() behaves for generic compile-time folded
>
> Please move this all to imperative voice. I think I asked for this
> before, but perhaps I didn't. Either way, please fix the whole series.
Oh. Sorry, I've forgotten to edit this commit message.
I'll fix in next around.
>
> > page tables by disallowing its use and triggering a compile-time error
> > when it is used improperly, ensuring that the actual first-level set_pXd()
> > function is used instead.
> >
> > The behaviour of pXd_page() will change with generic compile-time folded
> > page tables by disallowing its use and triggering a compile-time error
> > when it's used improperly, ensuring that the actual.
> >
> > To prepare fot that, skip collapse_pud_page() and populate_pud() according
> > to CONFIG_PGTABLE_LEVELS.
> >
> > There should be no functional change.
>
> If this wasn't done, what would happen? There would be a compile error,
> right?
>
> Shouldn't that be said out loud somewhere?
Yes compile error for using pud_page(). I'll shout out detail in the
commit message.
>
> > diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> > index 078689aa7206..91b33a265a43 100644
> > --- a/arch/x86/mm/pat/set_memory.c
> > +++ b/arch/x86/mm/pat/set_memory.c
> > @@ -1346,7 +1346,7 @@ static int collapse_pud_page(pud_t *pud, unsigned
> > long addr,
> > pmd_t *pmd, first;
> > int i;
> >
> > - if (!direct_gbpages)
> > + if (CONFIG_PGTABLE_LEVELS == 2 || !direct_gbpages)
> > return 0;
>
> This is subtly wrong. It probably shuts the compiler up, but it's subtly
> wrong.
>
> x86 has 4 paging modes. I'll use the SDM/Intel terminology:
>
> 32-bit:
> * 32-bit paging: 2-level
> * PAE: 3-level
> 64-bit:
> * 4-level paging
> * 5-level paging
>
> But "gbpages" are not available in PAE paging. It's a hardware
> limitation. This doesn't cause a functional problem because the pud
> level is not folded. But it is confusing and not really an accurate way
> to write this check.
>
> The *right* way to do this is probably something like this in a header:
>
> static inline bool direct_gbpages(void)
> {
> if (!IS_ENABLED(CONFIG_X86_DIRECT_GBPAGES))
> return false;
>
> return __direct_gbpages;
> }
>
> Which just happens to point readers over to this Kconfig nugget:
>
> config X86_DIRECT_GBPAGES
> def_bool y
> depends on X86_64
>
> which, combined with the knowledge that 32-bit has a 3-level mode, would
> make the proposed check obviously wrong.
>
> So I'll take this as a suitable alternative:
>
> /* Avoid compiling the below code if PUD-level mappings are impossible:
> */
> if (IS_ENABLED(CONFIG_X86_DIRECT_GBPAGES))
> return 0;
>
> if (!direct_gbpages)
> return 0;
>
> That will compile-time optimize the code away in the exact right
> conditions *and* fix the (assumed by me) compile error that you were
> chasing.
I thought the same. but It seemed odd at the time since direct_gbpages is
always 0 when !CONFIG_X86_DIRECT_GBPAGES.
Howver, I've missed the X86_DIRECT_GBPAGES is for X86_64 only and It was
wrong totally.
I'll follow your suggestion.
> > @@ -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.
In stead of CONFIG_PGTABLE_LEVEL > 3, as above, would it be better to
add check IS_ENABLED(CONFIG_X86_DIRECT_GBPAGES)?
Thanks.
--
Sincerely,
Yeoreum Yun
