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

["Brendan Jackman" <brendan.jackman@linux.dev>]

On Fri Jul 31, 2026 at 11:10 PM BST, Yosry Ahmed wrote:
...
>> +extern int preallocate_sub_pgd(struct mm_struct *mm, unsigned long addr);
>> +
>
> Do we need extern here?

Nope 

>> -		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");
>

Ack, thanks.

>>  	}
>> -
>> -	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";

This is just maintaining the old way preallocate_vmalloc_pages() was
written.

> 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?

Yeah I don't think this is very important, I'm just trying to avoid
changing log messages unnecessarily, keepign it aligned with the old
preallocate_vmalloc_pages().

For both questions, not strong feelings from me either way, happy to
change it or try to keep it the same.

> 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.

In the immediate usecase no, but once it's used for
mm_local_map_to_user() we do actually handle failure.