Re: [PATCH v3 04/26] x86/mm: split out preallocate_sub_pgd()

[Yosry Ahmed <yosry@kernel.org>]

On Sun, Jul 26, 2026 at 10:22:37PM +0000, Brendan Jackman wrote:
> This code will be needed elsewhere in a following patch. Split out the
> trivial code move for easy review.
> 
> As a side effect, change the logging slightly: instead of directly
> reporting the level of the failure in panic(), show a generic panic
> message, will be preceded by a separate warn that reports the level of
> the failure. This is a simple way to have this helper suit the needs of
> its new user as well as the existing one.
> 
> Other than logging, no functional change intended.
> 
> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> ---
>  arch/x86/include/asm/pgalloc.h |  3 +++
>  arch/x86/mm/init_64.c          | 44 +++++++-----------------------------------
>  arch/x86/mm/pgtable.c          | 38 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 48 insertions(+), 37 deletions(-)
> 
> diff --git a/arch/x86/include/asm/pgalloc.h b/arch/x86/include/asm/pgalloc.h
> index c88691b15f3c6..2aba6cfabf495 100644
> --- a/arch/x86/include/asm/pgalloc.h
> +++ b/arch/x86/include/asm/pgalloc.h
> @@ -2,6 +2,7 @@
>  #ifndef _ASM_X86_PGALLOC_H
>  #define _ASM_X86_PGALLOC_H
>  
> +#include <linux/printk.h>
>  #include <linux/threads.h>
>  #include <linux/mm.h>		/* for struct page */
>  #include <linux/pagemap.h>
> @@ -128,6 +129,8 @@ static inline void __pud_free_tlb(struct mmu_gather *tlb, pud_t *pud,
>  	___pud_free_tlb(tlb, pud);
>  }
>  
> +extern int preallocate_sub_pgd(struct mm_struct *mm, unsigned long addr);
> +

Do we need extern here?

>  #if CONFIG_PGTABLE_LEVELS > 4
>  static inline void pgd_populate(struct mm_struct *mm, pgd_t *pgd, p4d_t *p4d)
>  {
> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
> index ab4c5a02326f7..ac6688c70872e 100644
> --- a/arch/x86/mm/init_64.c
> +++ b/arch/x86/mm/init_64.c
> @@ -1293,46 +1293,16 @@ static struct kcore_list kcore_vsyscall;
>  static void __init preallocate_vmalloc_pages(void)
>  {
>  	unsigned long addr;
> -	const char *lvl;
>  
>  	for (addr = VMALLOC_START; addr <= VMEMORY_END; addr = ALIGN(addr + 1, PGDIR_SIZE)) {
> -		pgd_t *pgd = pgd_offset_k(addr);
> -		p4d_t *p4d;
> -		pud_t *pud;
> -
> -		lvl = "p4d";
> -		p4d = p4d_alloc(&init_mm, pgd, addr);
> -		if (!p4d)
> -			goto failed;
> -
> -		if (pgtable_l5_enabled())
> -			continue;
> -
> -		/*
> -		 * The goal here is to allocate all possibly required
> -		 * hardware page tables pointed to by the top hardware
> -		 * level.
> -		 *
> -		 * On 4-level systems, the P4D layer is folded away and
> -		 * the above code does no preallocation.  Below, go down
> -		 * to the pud _software_ level to ensure the second
> -		 * hardware level is allocated on 4-level systems too.
> -		 */
> -		lvl = "pud";
> -		pud = pud_alloc(&init_mm, p4d, addr);
> -		if (!pud)
> -			goto failed;
> +		if (preallocate_sub_pgd(&init_mm, addr)) {
> +			/*
> +			 * The pages have to be there now or they will be
> +			 * missing in process page-tables later.
> +			 */
> +			panic("Failed to pre-allocate pagetables for vmalloc area\n");
> +		}

Nit: We can probably move this comment above the if block, and drop the
curly braces:

		/*
		 * The pages have to be there now or they will be missing in
		 * process page-tables later.
		 */
		 if (preallocate_sub_pgd(&init_mm, addr))
		 	panic("Failed to pre-allocate pagetables for vmalloc area\n");

>  	}
> -
> -	return;
> -
> -failed:
> -
> -	/*
> -	 * The pages have to be there now or they will be missing in
> -	 * process page-tables later.
> -	 */
> -	panic("Failed to pre-allocate %s pages for vmalloc area\n", lvl);
>  }
>  
>  void __init arch_mm_preinit(void)
> diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
> index f32facdb30354..fdd3709509946 100644
> --- a/arch/x86/mm/pgtable.c
> +++ b/arch/x86/mm/pgtable.c
> @@ -833,3 +833,41 @@ void arch_check_zapped_pud(struct vm_area_struct *vma, pud_t pud)
>  	/* See note in arch_check_zapped_pte() */
>  	VM_WARN_ON_ONCE(!(vma->vm_flags & VM_SHADOW_STACK) && pud_shstk(pud));
>  }
> +
> +#if CONFIG_PGTABLE_LEVELS > 3
> +/*
> + * Allocate all possibly required hardware page tables pointed to ths

								     ^the

> + * top hardware level. In other words, allocate a p4d on 5-level or a

   					  allocate p4ds?
> + * pud on 4-level.

      puds?

> + */
> +int preallocate_sub_pgd(struct mm_struct *mm, unsigned long addr)
> +{
> +	const char *lvl;

Nit:

	const char *lvl = "p4d";

or:

	const char *lvl = pgtable_l5_enabled() ? "p4d" : "pud";

But I am wondering how important this information is here?

We should be able to tell whether 5-level paging is enabled based on
kernel config and command line. If the information is generally not easy
to get, maybe logging it during boot would generally be useful?

Anyway, if we drop lvl here we can drop the gotos, which would be nice.

> +	p4d_t *p4d;
> +	pud_t *pud;
> +
> +	lvl = "p4d";
> +	p4d = p4d_alloc(mm, pgd_offset_pgd(mm->pgd, addr), addr);
> +	if (!p4d)
> +		goto failed;
> +
> +	if (pgtable_l5_enabled())
> +		return 0;
> +
> +	/*
> +	 * On 4-level systems, the P4D layer is folded away and
> +	 * the above code does no preallocation.  Below, go down
> +	 * to the pud _software_ level to ensure the second
> +	 * hardware level is allocated on 4-level systems too.
> +	 */
> +	lvl = "pud";
> +	pud = pud_alloc(mm, p4d, addr);
> +	if (!pud)
> +		goto failed;
> +	return 0;
> +
> +failed:
> +	pr_warn_ratelimited("Failed to preallocate %s\n", lvl);

Can this possibly fire more than once? IIUC we will panic right after
returning.

> +	return -ENOMEM;
> +}
> +#endif
> 
> -- 
> 2.54.0
>