Re: [PATCH v2] arm64: mm: Use generic enum pgtable_level

[Anshuman Khandual]

On 18/03/26 2:55 PM, Kevin Brodsky wrote:
> enum pgtable_type was introduced for arm64 by commit c64f46ee1377
> ("arm64: mm: use enum to identify pgtable level instead of
> *_SHIFT"). In the meantime, the generic enum pgtable_level got
> introduced by commit b22cc9a9c7ff ("mm/rmap: convert "enum
> rmap_level" to "enum pgtable_level"").
> 
> Let's switch to the generic enum pgtable_level. The only difference
> is that it also includes PGD level; __pgd_pgtable_alloc() isn't
> expected to create PGD tables so we add a VM_WARN_ON() for that
> case.
> 
> Suggested-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
> Reviewed-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
> Reviewed-by: Ryan Roberts <ryan.roberts@xxxxxxx>
> Signed-off-by: Kevin Brodsky <kevin.brodsky@xxxxxxx>

Reviewed-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>

> ---
> v1..v2:
> - Add a VM_WARN_ON() for PGD in __pgd_pgtable_alloc() [Ryan, David]
> 
> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> Cc: Will Deacon <will@xxxxxxxxxx>
> ---
>  arch/arm64/include/asm/mmu.h |  7 -----
>  arch/arm64/mm/mmu.c          | 59 +++++++++++++++++++-----------------
>  2 files changed, 31 insertions(+), 35 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/mmu.h b/arch/arm64/include/asm/mmu.h
> index 137a173df1ff..f28e6e215da4 100644
> --- a/arch/arm64/include/asm/mmu.h
> +++ b/arch/arm64/include/asm/mmu.h
> @@ -17,13 +17,6 @@
>  #include <linux/refcount.h>
>  #include <asm/cpufeature.h>
>  
> -enum pgtable_type {
> -	TABLE_PTE,
> -	TABLE_PMD,
> -	TABLE_PUD,
> -	TABLE_P4D,
> -};
> -
>  typedef struct {
>  	atomic64_t	id;
>  #ifdef CONFIG_COMPAT
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index a6a00accf4f9..c45a6a47dad0 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -112,7 +112,7 @@ pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
>  }
>  EXPORT_SYMBOL(phys_mem_access_prot);
>  
> -static phys_addr_t __init early_pgtable_alloc(enum pgtable_type pgtable_type)
> +static phys_addr_t __init early_pgtable_alloc(enum pgtable_level pgtable_level)
>  {
>  	phys_addr_t phys;
>  
> @@ -197,7 +197,7 @@ static void init_pte(pte_t *ptep, unsigned long addr, unsigned long end,
>  static int alloc_init_cont_pte(pmd_t *pmdp, unsigned long addr,
>  			       unsigned long end, phys_addr_t phys,
>  			       pgprot_t prot,
> -			       phys_addr_t (*pgtable_alloc)(enum pgtable_type),
> +			       phys_addr_t (*pgtable_alloc)(enum pgtable_level),
>  			       int flags)
>  {
>  	unsigned long next;
> @@ -212,7 +212,7 @@ static int alloc_init_cont_pte(pmd_t *pmdp, unsigned long addr,
>  		if (flags & NO_EXEC_MAPPINGS)
>  			pmdval |= PMD_TABLE_PXN;
>  		BUG_ON(!pgtable_alloc);
> -		pte_phys = pgtable_alloc(TABLE_PTE);
> +		pte_phys = pgtable_alloc(PGTABLE_LEVEL_PTE);
>  		if (pte_phys == INVALID_PHYS_ADDR)
>  			return -ENOMEM;
>  		ptep = pte_set_fixmap(pte_phys);
> @@ -252,7 +252,7 @@ static int alloc_init_cont_pte(pmd_t *pmdp, unsigned long addr,
>  
>  static int init_pmd(pmd_t *pmdp, unsigned long addr, unsigned long end,
>  		    phys_addr_t phys, pgprot_t prot,
> -		    phys_addr_t (*pgtable_alloc)(enum pgtable_type), int flags)
> +		    phys_addr_t (*pgtable_alloc)(enum pgtable_level), int flags)
>  {
>  	unsigned long next;
>  
> @@ -292,7 +292,7 @@ static int init_pmd(pmd_t *pmdp, unsigned long addr, unsigned long end,
>  static int alloc_init_cont_pmd(pud_t *pudp, unsigned long addr,
>  			       unsigned long end, phys_addr_t phys,
>  			       pgprot_t prot,
> -			       phys_addr_t (*pgtable_alloc)(enum pgtable_type),
> +			       phys_addr_t (*pgtable_alloc)(enum pgtable_level),
>  			       int flags)
>  {
>  	int ret;
> @@ -311,7 +311,7 @@ static int alloc_init_cont_pmd(pud_t *pudp, unsigned long addr,
>  		if (flags & NO_EXEC_MAPPINGS)
>  			pudval |= PUD_TABLE_PXN;
>  		BUG_ON(!pgtable_alloc);
> -		pmd_phys = pgtable_alloc(TABLE_PMD);
> +		pmd_phys = pgtable_alloc(PGTABLE_LEVEL_PMD);
>  		if (pmd_phys == INVALID_PHYS_ADDR)
>  			return -ENOMEM;
>  		pmdp = pmd_set_fixmap(pmd_phys);
> @@ -349,7 +349,7 @@ static int alloc_init_cont_pmd(pud_t *pudp, unsigned long addr,
>  
>  static int alloc_init_pud(p4d_t *p4dp, unsigned long addr, unsigned long end,
>  			  phys_addr_t phys, pgprot_t prot,
> -			  phys_addr_t (*pgtable_alloc)(enum pgtable_type),
> +			  phys_addr_t (*pgtable_alloc)(enum pgtable_level),
>  			  int flags)
>  {
>  	int ret = 0;
> @@ -364,7 +364,7 @@ static int alloc_init_pud(p4d_t *p4dp, unsigned long addr, unsigned long end,
>  		if (flags & NO_EXEC_MAPPINGS)
>  			p4dval |= P4D_TABLE_PXN;
>  		BUG_ON(!pgtable_alloc);
> -		pud_phys = pgtable_alloc(TABLE_PUD);
> +		pud_phys = pgtable_alloc(PGTABLE_LEVEL_PUD);
>  		if (pud_phys == INVALID_PHYS_ADDR)
>  			return -ENOMEM;
>  		pudp = pud_set_fixmap(pud_phys);
> @@ -415,7 +415,7 @@ static int alloc_init_pud(p4d_t *p4dp, unsigned long addr, unsigned long end,
>  
>  static int alloc_init_p4d(pgd_t *pgdp, unsigned long addr, unsigned long end,
>  			  phys_addr_t phys, pgprot_t prot,
> -			  phys_addr_t (*pgtable_alloc)(enum pgtable_type),
> +			  phys_addr_t (*pgtable_alloc)(enum pgtable_level),
>  			  int flags)
>  {
>  	int ret;
> @@ -430,7 +430,7 @@ static int alloc_init_p4d(pgd_t *pgdp, unsigned long addr, unsigned long end,
>  		if (flags & NO_EXEC_MAPPINGS)
>  			pgdval |= PGD_TABLE_PXN;
>  		BUG_ON(!pgtable_alloc);
> -		p4d_phys = pgtable_alloc(TABLE_P4D);
> +		p4d_phys = pgtable_alloc(PGTABLE_LEVEL_P4D);
>  		if (p4d_phys == INVALID_PHYS_ADDR)
>  			return -ENOMEM;
>  		p4dp = p4d_set_fixmap(p4d_phys);
> @@ -467,7 +467,7 @@ static int alloc_init_p4d(pgd_t *pgdp, unsigned long addr, unsigned long end,
>  static int __create_pgd_mapping_locked(pgd_t *pgdir, phys_addr_t phys,
>  				       unsigned long virt, phys_addr_t size,
>  				       pgprot_t prot,
> -				       phys_addr_t (*pgtable_alloc)(enum pgtable_type),
> +				       phys_addr_t (*pgtable_alloc)(enum pgtable_level),
>  				       int flags)
>  {
>  	int ret;
> @@ -500,7 +500,7 @@ static int __create_pgd_mapping_locked(pgd_t *pgdir, phys_addr_t phys,
>  static int __create_pgd_mapping(pgd_t *pgdir, phys_addr_t phys,
>  				unsigned long virt, phys_addr_t size,
>  				pgprot_t prot,
> -				phys_addr_t (*pgtable_alloc)(enum pgtable_type),
> +				phys_addr_t (*pgtable_alloc)(enum pgtable_level),
>  				int flags)
>  {
>  	int ret;
> @@ -516,7 +516,7 @@ static int __create_pgd_mapping(pgd_t *pgdir, phys_addr_t phys,
>  static void early_create_pgd_mapping(pgd_t *pgdir, phys_addr_t phys,
>  				     unsigned long virt, phys_addr_t size,
>  				     pgprot_t prot,
> -				     phys_addr_t (*pgtable_alloc)(enum pgtable_type),
> +				     phys_addr_t (*pgtable_alloc)(enum pgtable_level),
>  				     int flags)
>  {
>  	int ret;
> @@ -528,7 +528,7 @@ static void early_create_pgd_mapping(pgd_t *pgdir, phys_addr_t phys,
>  }
>  
>  static phys_addr_t __pgd_pgtable_alloc(struct mm_struct *mm, gfp_t gfp,
> -				       enum pgtable_type pgtable_type)
> +				       enum pgtable_level pgtable_level)
>  {
>  	/* Page is zeroed by init_clear_pgtable() so don't duplicate effort. */
>  	struct ptdesc *ptdesc = pagetable_alloc(gfp & ~__GFP_ZERO, 0);
> @@ -539,40 +539,43 @@ static phys_addr_t __pgd_pgtable_alloc(struct mm_struct *mm, gfp_t gfp,
>  
>  	pa = page_to_phys(ptdesc_page(ptdesc));
>  
> -	switch (pgtable_type) {
> -	case TABLE_PTE:
> +	switch (pgtable_level) {
> +	case PGTABLE_LEVEL_PTE:
>  		BUG_ON(!pagetable_pte_ctor(mm, ptdesc));
>  		break;
> -	case TABLE_PMD:
> +	case PGTABLE_LEVEL_PMD:
>  		BUG_ON(!pagetable_pmd_ctor(mm, ptdesc));
>  		break;
> -	case TABLE_PUD:
> +	case PGTABLE_LEVEL_PUD:
>  		pagetable_pud_ctor(ptdesc);
>  		break;
> -	case TABLE_P4D:
> +	case PGTABLE_LEVEL_P4D:
>  		pagetable_p4d_ctor(ptdesc);
>  		break;
> +	case PGTABLE_LEVEL_PGD:
> +		VM_WARN_ON(1);
> +		break;
>  	}
>  
>  	return pa;
>  }
>  
>  static phys_addr_t
> -pgd_pgtable_alloc_init_mm_gfp(enum pgtable_type pgtable_type, gfp_t gfp)
> +pgd_pgtable_alloc_init_mm_gfp(enum pgtable_level pgtable_level, gfp_t gfp)
>  {
> -	return __pgd_pgtable_alloc(&init_mm, gfp, pgtable_type);
> +	return __pgd_pgtable_alloc(&init_mm, gfp, pgtable_level);
>  }
>  
>  static phys_addr_t __maybe_unused
> -pgd_pgtable_alloc_init_mm(enum pgtable_type pgtable_type)
> +pgd_pgtable_alloc_init_mm(enum pgtable_level pgtable_level)
>  {
> -	return pgd_pgtable_alloc_init_mm_gfp(pgtable_type, GFP_PGTABLE_KERNEL);
> +	return pgd_pgtable_alloc_init_mm_gfp(pgtable_level, GFP_PGTABLE_KERNEL);
>  }
>  
>  static phys_addr_t
> -pgd_pgtable_alloc_special_mm(enum pgtable_type pgtable_type)
> +pgd_pgtable_alloc_special_mm(enum pgtable_level pgtable_level)
>  {
> -	return  __pgd_pgtable_alloc(NULL, GFP_PGTABLE_KERNEL, pgtable_type);
> +	return  __pgd_pgtable_alloc(NULL, GFP_PGTABLE_KERNEL, pgtable_level);
>  }
>  
>  static void split_contpte(pte_t *ptep)
> @@ -593,7 +596,7 @@ static int split_pmd(pmd_t *pmdp, pmd_t pmd, gfp_t gfp, bool to_cont)
>  	pte_t *ptep;
>  	int i;
>  
> -	pte_phys = pgd_pgtable_alloc_init_mm_gfp(TABLE_PTE, gfp);
> +	pte_phys = pgd_pgtable_alloc_init_mm_gfp(PGTABLE_LEVEL_PTE, gfp);
>  	if (pte_phys == INVALID_PHYS_ADDR)
>  		return -ENOMEM;
>  	ptep = (pte_t *)phys_to_virt(pte_phys);
> @@ -638,7 +641,7 @@ static int split_pud(pud_t *pudp, pud_t pud, gfp_t gfp, bool to_cont)
>  	pmd_t *pmdp;
>  	int i;
>  
> -	pmd_phys = pgd_pgtable_alloc_init_mm_gfp(TABLE_PMD, gfp);
> +	pmd_phys = pgd_pgtable_alloc_init_mm_gfp(PGTABLE_LEVEL_PMD, gfp);
>  	if (pmd_phys == INVALID_PHYS_ADDR)
>  		return -ENOMEM;
>  	pmdp = (pmd_t *)phys_to_virt(pmd_phys);
> @@ -1226,7 +1229,7 @@ static void __init declare_vma(struct vm_struct *vma,
>  
>  static phys_addr_t kpti_ng_temp_alloc __initdata;
>  
> -static phys_addr_t __init kpti_ng_pgd_alloc(enum pgtable_type type)
> +static phys_addr_t __init kpti_ng_pgd_alloc(enum pgtable_level pgtable_level)
>  {
>  	kpti_ng_temp_alloc -= PAGE_SIZE;
>  	return kpti_ng_temp_alloc;