Re: [PATCH RFC 02/11] x86/mm: Factor out phys_pgd_init()

[Yosry Ahmed <yosry.ahmed@linux.dev>]

On Thu, Mar 13, 2025 at 06:11:21PM +0000, Brendan Jackman wrote:
> __kernel_physical_mapping_init() will soon need to work on multiple
> PGDs, so factor out something similar to phys_p4d_init() and friends,
> which takes the base of the PGD as an argument.
> 
> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> ---
>  arch/x86/mm/init_64.c | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
> index 01ea7c6df3036bd185cdb3f54ddf244b79cbce8c..8f75274fddd96b8285aff48493ebad93e30daebe 100644
> --- a/arch/x86/mm/init_64.c
> +++ b/arch/x86/mm/init_64.c
> @@ -731,21 +731,20 @@ phys_p4d_init(p4d_t *p4d_page, unsigned long paddr, unsigned long paddr_end,
>  }
>  
>  static unsigned long __meminit
> -__kernel_physical_mapping_init(unsigned long paddr_start,
> -			       unsigned long paddr_end,
> -			       unsigned long page_size_mask,
> -			       pgprot_t prot, bool init)
> +phys_pgd_init(pgd_t *pgd_page, unsigned long paddr_start, unsigned long paddr_end,
> +	      unsigned long page_size_mask, pgprot_t prot, bool init, bool *pgd_changed)
>  {
> -	bool pgd_changed = false;
>  	unsigned long vaddr, vaddr_start, vaddr_end, vaddr_next, paddr_last;
>  
> +	*pgd_changed = false;
> +
>  	paddr_last = paddr_end;
>  	vaddr = (unsigned long)__va(paddr_start);
>  	vaddr_end = (unsigned long)__va(paddr_end);
>  	vaddr_start = vaddr;
>  
>  	for (; vaddr < vaddr_end; vaddr = vaddr_next) {
> -		pgd_t *pgd = pgd_offset_k(vaddr);
> +		pgd_t *pgd = pgd_offset_pgd(pgd_page, vaddr);
>  		p4d_t *p4d;
>  
>  		vaddr_next = (vaddr & PGDIR_MASK) + PGDIR_SIZE;
> @@ -771,15 +770,29 @@ __kernel_physical_mapping_init(unsigned long paddr_start,
>  					  (pud_t *) p4d, init);
>  
>  		spin_unlock(&init_mm.page_table_lock);
> -		pgd_changed = true;
> +		*pgd_changed = true;
>  	}
>  
> -	if (pgd_changed)
> -		sync_global_pgds(vaddr_start, vaddr_end - 1);
> -
>  	return paddr_last;
>  }
>  
> +static unsigned long __meminit
> +__kernel_physical_mapping_init(unsigned long paddr_start,
> +			       unsigned long paddr_end,
> +			       unsigned long page_size_mask,
> +			       pgprot_t prot, bool init)
> +{
> +	bool pgd_changed;
> +	unsigned long paddr_last;
> +
> +	paddr_last = phys_pgd_init(init_mm.pgd, paddr_start, paddr_end, page_size_mask,
> +				   prot, init, &pgd_changed);
> +	if (pgd_changed)
> +		sync_global_pgds((unsigned long)__va(paddr_start),
> +				 (unsigned long)__va(paddr_end) - 1);

This patch keeps the sync_global_pgds() in
__kernel_physical_mapping_init(), then a following patch adds it back in
phys_pgd_init() (but still leaves it here).

Should we just leave sync_global_pgds() in phys_pgd_init() and eliminate
the pgd_changed argument?

> +
> +	return paddr_last;
> +}
>  
>  /*
>   * Create page table mapping for the physical memory for specific physical
> 
> -- 
> 2.49.0.rc1.451.g8f38331e32-goog
>