Re: [PATCH] x86/mm: Introduce helper for checking direct map 1G page support

[Sohil Mehta]

On 9/2/2026 12:47 PM, Dave Hansen wrote:
> 
> From: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> 
> There is an existing variable (direct_gbpages) that says whether the
> kernel can and should use 1G pages in the direct map. It is driven
> by a bunch of other machinery. At least:
> 
>  1. Hardware support for 1G pages
>  2. Kconfig support for 1G direct mappings
>  3. Kernel command line overrides
> 
> Most code just checks the 'direct_gbpages' variable itself.  But there
> are cases where 1G mappings are compile-time disabled (via
> X86_DIRECT_GBPAGES) and 'direct_gbpages' is always 0. Unfortunately,
> that constraint is invisible to the compiler.
> 
> This opacity has been historically functionally harmless; it only
> leaves a bit of dead code. But, there are plans to tigthen up the

s/tigthen/tighten


> compile-time checks around folded page table levels. The build will
> break if that dead code appears reachable to the compiler. Making
> the compile-time config visible to the compiler fixes the build.
> 
> Add a helper to replace 'direct_gbpages' checks. Check the Kconfig
> option and base CPU support before looking at the variable.
> 

It is a bit confusing when this variable is finalized and when the
helper can be called. I was assuming that early __init code modifies the
variable and later code consumes the helper.

That is mostly true except for probe_page_size_mask() which is an __init
consumer as well as a modifier. I don't see any functional issue. But it
may be worthy of a code comment to avoid future issues.

> This lets the compiler optimize things better, especially
> collapse_pud_page() where most of the function can now be optimized
> out when the PUD level is folded.
> 

The Link: tag below mentions [1]. Did you mean to reference it here?

> Notes:
> 
> Use boot_cpu_has() instead of cpu_feature_enabled(). There's no
> required/disabled features for 1G pages themselves
> (X86_DIRECT_GBPAGES is for kernel mappings only) and the
> static_cpu_has() infrastructure is just gets in the compiler's way.
> 

s/is//

> This makes 64-bit build marginally larger (20 bytes in one compile)
> and 32-bit builds less marginally _smaller_ (~700 bytes).
> 
> Signed-off-by: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> Reviewed-by: Yeoreum Yun <yeoreum.yun@xxxxxxx>
> Tested-by: Yeoreum Yun <yeoreum.yun@xxxxxxx>
> Link: https://lore.kernel.org/all/20260902-dummy_ptxp3-v3-15-5d8f5b17c25c@xxxxxxx/ [1]
> ---
> 
>  b/arch/x86/include/asm/pgtable.h     |   14 ++++++++++++++
>  b/arch/x86/kernel/cpu/common.c       |    2 +-
>  b/arch/x86/kernel/machine_kexec_64.c |    2 +-
>  b/arch/x86/mm/init.c                 |    2 +-
>  b/arch/x86/mm/pat/set_memory.c       |    4 ++--
>  5 files changed, 19 insertions(+), 5 deletions(-)
> 

Overall, the changes look good to me.

Reviewed-by: Sohil Mehta <sohil.mehta@xxxxxxxxx>


> diff -puN arch/x86/include/asm/pgtable.h~direct_gbpages-compiletime arch/x86/include/asm/pgtable.h
> --- a/arch/x86/include/asm/pgtable.h~direct_gbpages-compiletime	2026-09-02 10:08:59.999169372 -0700
> +++ b/arch/x86/include/asm/pgtable.h	2026-09-02 10:09:00.011170394 -0700
> @@ -1163,6 +1163,20 @@ static inline int pgd_none(pgd_t pgd)
>  #ifndef __ASSEMBLER__
>  
>  extern int direct_gbpages;
> +static inline bool direct_gbpages_enabled(void)
> +{
> +	/* Check the direct map config option: */
> +	if (!IS_ENABLED(CONFIG_X86_DIRECT_GBPAGES))
> +		return false;
> +
> +	/* Check the CPU feature: */
> +	if (!boot_cpu_has(X86_FEATURE_GBPAGES))
> +		return false;
> +

The first two comments don't add much beyond the code. Would it be
useful to say why boot_cpu_has() instead of static_cpu_has() over here
(mainly to avoid accidental cleanup)?

> +	/* Check the command-line and early setup variable: */
> +	return direct_gbpages;
> +}
> +
>  void init_mem_mapping(void);
>  void early_alloc_pgt_buf(void);
>  void __init poking_init(void);
> diff -puN arch/x86/kernel/machine_kexec_64.c~direct_gbpages-compiletime arch/x86/kernel/machine_kexec_64.c
> --- a/arch/x86/kernel/machine_kexec_64.c~direct_gbpages-compiletime	2026-09-02 10:09:00.004169798 -0700
> +++ b/arch/x86/kernel/machine_kexec_64.c	2026-09-02 10:09:00.012170479 -0700
> @@ -257,7 +257,7 @@ static int init_pgtable(struct kimage *i
>  		info.kernpg_flag |= _PAGE_ENC;
>  	}
>  
> -	if (direct_gbpages)
> +	if (direct_gbpages_enabled())
>  		info.direct_gbpages = true;
>  

How about:

	info.direct_gbpages = direct_gbpages_enabled()


>  	for (i = 0; i < nr_pfn_mapped; i++) {