Re: [PATCH RFC v2 14/20] x86: mm: skip pud setup when using generic compile-time folded pagetable

[Yeoreum Yun <yeoreum.yun@arm.com>]

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