Reimplement huge pages without hugepd on powerpc (8xx, e500, book3s/64)

Unlike many architectures, powerpc 8xx hardware tablewalk requires a two level process for all page sizes, allthough second level only has one entry when pagesize is 8M.

To fit with Linux page table topology and without requiring special page directory layout like hugepd, the page entry will be replicated 1024 times in the standard page table. However for large pages it is

necessary to set bits in the level-1 (PMD) entry. At the time being, for 512k pages the flag is kept in the PTE and inserted in the PMD entry at TLB miss exception, that is necessary because we can have

pages of different sizes in a page table. However the 12 PTE bits are fully used and there is no room for an additional bit for page size.

For 8M pages, there will be only one page per PMD entry, it is therefore possible to flag the pagesize in the PMD entry, with the

advantage that the information will already be at the right place for the hardware.

To do so, add a new helper called pmd_populate_size() which takes the page size as an additional argument, and modify __pte_alloc() to also

take that argument. pte_alloc() is left unmodified in order to reduce churn on callers, and a pte_alloc_size() is added for use by pte_alloc_huge().

When an architecture doesn't provide pmd_populate_size(), pmd_populate() is used as a fallback.

Signed-off-by Christophe Leroy <[email protected]>

---
 include/linux/mm.h | 12 +++++++-----
 mm/filemap.c       |  2 +-
 mm/internal.h      |  2 +-
 mm/memory.c        | 19 ++++++++++++-------
 mm/pgalloc-track.h |  2 +-
 mm/userfaultfd.c   |  4 ++--
 6 files changed, 24 insertions(+), 17 deletions(-)

include/linux/mm.h +7 -5

index b6bdaa18b9e9..158cb87bc604 100644
@@ -2803,8 +2803,8 @@ static inline void mm_inc_nr_ptes(struct mm_struct *mm) {}
 static inline void mm_dec_nr_ptes(struct mm_struct *mm) {}
 #endif
 
-int __pte_alloc(struct mm_struct *mm, pmd_t *pmd);
-int __pte_alloc_kernel(pmd_t *pmd);
+int __pte_alloc(struct mm_struct *mm, pmd_t *pmd, unsigned long sz);
+int __pte_alloc_kernel(pmd_t *pmd, unsigned long sz);
 
 #if defined(CONFIG_MMU)
 

@@ -2989,7 +2989,8 @@ pte_t *pte_offset_map_nolock(struct mm_struct *mm, pmd_t *pmd,
 	pte_unmap(pte);					\
 } while (0)
 
-#define pte_alloc(mm, pmd) (unlikely(pmd_none(*(pmd))) && __pte_alloc(mm, pmd))
+#define pte_alloc_size(mm, pmd, sz) (unlikely(pmd_none(*(pmd))) && __pte_alloc(mm, pmd, sz))
+#define pte_alloc(mm, pmd) pte_alloc_size(mm, pmd, PAGE_SIZE)
 
 #define pte_alloc_map(mm, pmd, address)			\
 	(pte_alloc(mm, pmd) ? NULL : pte_offset_map(pmd, address))

@@ -2998,9 +2999,10 @@ pte_t *pte_offset_map_nolock(struct mm_struct *mm, pmd_t *pmd,
 	(pte_alloc(mm, pmd) ?			\
 		 NULL : pte_offset_map_lock(mm, pmd, address, ptlp))
 
-#define pte_alloc_kernel(pmd, address)			\
-	((unlikely(pmd_none(*(pmd))) && __pte_alloc_kernel(pmd))? \
+#define pte_alloc_kernel_size(pmd, address, sz)			\
+	((unlikely(pmd_none(*(pmd))) && __pte_alloc_kernel(pmd, sz))? \
 		NULL: pte_offset_kernel(pmd, address))
+#define pte_alloc_kernel(pmd, address)	pte_alloc_kernel_size(pmd, address, PAGE_SIZE)
 
 #if USE_SPLIT_PMD_PTLOCKS
 

mm/filemap.c +1 -1

index 30de18c4fd28..5a783063d1f6 100644
@@ -3428,7 +3428,7 @@ static bool filemap_map_pmd(struct vm_fault *vmf, struct folio *folio,
 	}
 
 	if (pmd_none(*vmf->pmd) && vmf->prealloc_pte)
-		pmd_install(mm, vmf->pmd, &vmf->prealloc_pte);
+		pmd_install(mm, vmf->pmd, &vmf->prealloc_pte, PAGE_SIZE);
 
 	return false;
 }

mm/internal.h +1 -1

index 07ad2675a88b..4a01bbf55264 100644
@@ -206,7 +206,7 @@ void folio_activate(struct folio *folio);
 void free_pgtables(struct mmu_gather *tlb, struct ma_state *mas,
 		   struct vm_area_struct *start_vma, unsigned long floor,
 		   unsigned long ceiling, bool mm_wr_locked);
-void pmd_install(struct mm_struct *mm, pmd_t *pmd, pgtable_t *pte);
+void pmd_install(struct mm_struct *mm, pmd_t *pmd, pgtable_t *pte, unsigned long sz);
 
 struct zap_details;
 void unmap_page_range(struct mmu_gather *tlb,

mm/memory.c +12 -7

index d2155ced45f8..2a9eba13a95f 100644
@@ -409,7 +409,12 @@ void free_pgtables(struct mmu_gather *tlb, struct ma_state *mas,
 	} while (vma);
 }
 
-void pmd_install(struct mm_struct *mm, pmd_t *pmd, pgtable_t *pte)
+#ifndef pmd_populate_size
+#define pmd_populate_size(mm, pmdp, pte, sz) pmd_populate(mm, pmdp, pte)
+#define pmd_populate_kernel_size(mm, pmdp, pte, sz) pmd_populate_kernel(mm, pmdp, pte)
+#endif
+
+void pmd_install(struct mm_struct *mm, pmd_t *pmd, pgtable_t *pte, unsigned long sz)
 {
 	spinlock_t *ptl = pmd_lock(mm, pmd);
 

@@ -429,25 +434,25 @@ void pmd_install(struct mm_struct *mm, pmd_t *pmd, pgtable_t *pte)
 		 * smp_rmb() barriers in page table walking code.
 		 */
 		smp_wmb(); /* Could be smp_wmb__xxx(before|after)_spin_lock */
-		pmd_populate(mm, pmd, *pte);
+		pmd_populate_size(mm, pmd, *pte, sz);
 		*pte = NULL;
 	}
 	spin_unlock(ptl);
 }
 
-int __pte_alloc(struct mm_struct *mm, pmd_t *pmd)
+int __pte_alloc(struct mm_struct *mm, pmd_t *pmd, unsigned long sz)
 {
 	pgtable_t new = pte_alloc_one(mm);
 	if (!new)
 		return -ENOMEM;
 
-	pmd_install(mm, pmd, &new);
+	pmd_install(mm, pmd, &new, sz);
 	if (new)
 		pte_free(mm, new);
 	return 0;
 }
 
-int __pte_alloc_kernel(pmd_t *pmd)
+int __pte_alloc_kernel(pmd_t *pmd, unsigned long sz)
 {
 	pte_t *new = pte_alloc_one_kernel(&init_mm);
 	if (!new)

@@ -456,7 +461,7 @@ int __pte_alloc_kernel(pmd_t *pmd)
 	spin_lock(&init_mm.page_table_lock);
 	if (likely(pmd_none(*pmd))) {	/* Has another populated it ? */
 		smp_wmb(); /* See comment in pmd_install() */
-		pmd_populate_kernel(&init_mm, pmd, new);
+		pmd_populate_kernel_size(&init_mm, pmd, new, sz);
 		new = NULL;
 	}
 	spin_unlock(&init_mm.page_table_lock);

@@ -4740,7 +4745,7 @@ vm_fault_t finish_fault(struct vm_fault *vmf)
 		}
 
 		if (vmf->prealloc_pte)
-			pmd_install(vma->vm_mm, vmf->pmd, &vmf->prealloc_pte);
+			pmd_install(vma->vm_mm, vmf->pmd, &vmf->prealloc_pte, PAGE_SIZE);
 		else if (unlikely(pte_alloc(vma->vm_mm, vmf->pmd)))
 			return VM_FAULT_OOM;
 	}

mm/pgalloc-track.h +1 -1

index e9e879de8649..90e37de7ab77 100644
@@ -45,7 +45,7 @@ static inline pmd_t *pmd_alloc_track(struct mm_struct *mm, pud_t *pud,
 
 #define pte_alloc_kernel_track(pmd, address, mask)			\
 	((unlikely(pmd_none(*(pmd))) &&					\
-	  (__pte_alloc_kernel(pmd) || ({*(mask)|=PGTBL_PMD_MODIFIED;0;})))?\
+	  (__pte_alloc_kernel(pmd, PAGE_SIZE) || ({*(mask)|=PGTBL_PMD_MODIFIED;0;})))?\
 		NULL: pte_offset_kernel(pmd, address))
 
 #endif /* _LINUX_PGALLOC_TRACK_H */

mm/userfaultfd.c +2 -2

index 3c3539c573e7..0f129d5c5aa2 100644
@@ -764,7 +764,7 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
 			break;
 		}
 		if (unlikely(pmd_none(dst_pmdval)) &&
-		    unlikely(__pte_alloc(dst_mm, dst_pmd))) {
+		    unlikely(__pte_alloc(dst_mm, dst_pmd, PAGE_SIZE))) {
 			err = -ENOMEM;
 			break;
 		}

@@ -1687,7 +1687,7 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
 					err = -ENOENT;
 					break;
 				}
-				if (unlikely(__pte_alloc(mm, src_pmd))) {
+				if (unlikely(__pte_alloc(mm, src_pmd, PAGE_SIZE))) {
 					err = -ENOMEM;
 					break;
 				}

Signature · 1 line

2.44.0

Quoted 3 lines

Unlike many architectures, powerpc 8xx hardware tablewalk requires a two level process for all page sizes, allthough second level only has one entry when pagesize is 8M.

So, I went on a quick reading on

https://www.nxp.com/docs/en/application-note-software/AN3066.pdf

to get more insight, and I realized that some of the questions I made in v1 were quite dump.

Quoted 5 lines

To fit with Linux page table topology and without requiring special page directory layout like hugepd, the page entry will be replicated 1024 times in the standard page table. However for large pages it is

You only have to replicate 1024 times in case the page size is 4KB, and you will have to replicate that twice and have 2 PMDs pointing to it, right?

For 16KB, you will have the PMD containing 512 entries of 16KB.

Quoted 4 lines

necessary to set bits in the level-1 (PMD) entry. At the time being, for 512k pages the flag is kept in the PTE and inserted in the PMD entry at TLB miss exception, that is necessary because we can have

rlwimi r11, r10, 32 - 9, _PMD_PAGE_512K

mtspr SPRN_MI_TWC, r11

So we shift the value and compare it to _PMD_PAGE_512K to see if the PTE is a 512K page, and then we set it to SPRN_MI_TWC which I guess is some CPU special register?

Quoted 3 lines

pages of different sizes in a page table. However the 12 PTE bits are fully used and there is no room for an additional bit for page size.

You are referring to the bits in
arch/powerpc/include/asm/nohash/32/pte-8xx.h ?

Quoted 3 lines

For 8M pages, there will be only one page per PMD entry, it is therefore possible to flag the pagesize in the PMD entry, with the

I am confused, and it might be just terminology, or I am getting wrong the design.
You say that for 8MB pages, there will one page per PMD entry, but based on the above, you will have 1024 entries (replicated)?
So, maybe this wanted to be read as "there will be only one page size per PMD entry".

Quoted 6 lines

advantage that the information will already be at the right place for the hardware.

To do so, add a new helper called pmd_populate_size() which takes the page size as an additional argument, and modify __pte_alloc() to also

"page size" makes me thing of the standart page size the kernel is operating on (aka PAGE_SIZE), but it is actually the size of the huge page, so I think we should clarify it.

Quoted 7 lines

take that argument. pte_alloc() is left unmodified in order to reduce churn on callers, and a pte_alloc_size() is added for use by pte_alloc_huge().

When an architecture doesn't provide pmd_populate_size(), pmd_populate() is used as a fallback.

It is a bit unfortunate that we have to touch the code for other architectures (in patch#2)

Quoted 2 lines

Signed-off-by Christophe Leroy <[email protected]>

So far I only looked at this patch and patch#2, and code-wise looks good and makes sense, but I find it a bit unfortunate that we have to touch general definitons and arch code (done in patch#2 and patch#3), and I hoped we could somehow isolate this, but I could not think of a way.

I will give it some more though.

Quoted 177 lines

---
  include/linux/mm.h | 12 +++++++-----
  mm/filemap.c       |  2 +-
  mm/internal.h      |  2 +-
  mm/memory.c        | 19 ++++++++++++-------
  mm/pgalloc-track.h |  2 +-
  mm/userfaultfd.c   |  4 ++--
  6 files changed, 24 insertions(+), 17 deletions(-)

include/linux/mm.h +7 -5

index b6bdaa18b9e9..158cb87bc604 100644
@@ -2803,8 +2803,8 @@ static inline void mm_inc_nr_ptes(struct mm_struct *mm) {}
  static inline void mm_dec_nr_ptes(struct mm_struct *mm) {}
  #endif
 
-int __pte_alloc(struct mm_struct *mm, pmd_t *pmd);
-int __pte_alloc_kernel(pmd_t *pmd);
+int __pte_alloc(struct mm_struct *mm, pmd_t *pmd, unsigned long sz);
+int __pte_alloc_kernel(pmd_t *pmd, unsigned long sz);
 
  #if defined(CONFIG_MMU)
 

@@ -2989,7 +2989,8 @@ pte_t *pte_offset_map_nolock(struct mm_struct *mm, pmd_t *pmd,
  	pte_unmap(pte);					\
  } while (0)
 
-#define pte_alloc(mm, pmd) (unlikely(pmd_none(*(pmd))) && __pte_alloc(mm, pmd))
+#define pte_alloc_size(mm, pmd, sz) (unlikely(pmd_none(*(pmd))) && __pte_alloc(mm, pmd, sz))
+#define pte_alloc(mm, pmd) pte_alloc_size(mm, pmd, PAGE_SIZE)
 
  #define pte_alloc_map(mm, pmd, address)			\
  	(pte_alloc(mm, pmd) ? NULL : pte_offset_map(pmd, address))

@@ -2998,9 +2999,10 @@ pte_t *pte_offset_map_nolock(struct mm_struct *mm, pmd_t *pmd,
  	(pte_alloc(mm, pmd) ?			\
  		 NULL : pte_offset_map_lock(mm, pmd, address, ptlp))
 
-#define pte_alloc_kernel(pmd, address)			\
-	((unlikely(pmd_none(*(pmd))) && __pte_alloc_kernel(pmd))? \
+#define pte_alloc_kernel_size(pmd, address, sz)			\
+	((unlikely(pmd_none(*(pmd))) && __pte_alloc_kernel(pmd, sz))? \
  		NULL: pte_offset_kernel(pmd, address))
+#define pte_alloc_kernel(pmd, address)	pte_alloc_kernel_size(pmd, address, PAGE_SIZE)
 
  #if USE_SPLIT_PMD_PTLOCKS
 

mm/filemap.c +1 -1

index 30de18c4fd28..5a783063d1f6 100644
@@ -3428,7 +3428,7 @@ static bool filemap_map_pmd(struct vm_fault *vmf, struct folio *folio,
  	}
 
  	if (pmd_none(*vmf->pmd) && vmf->prealloc_pte)
-		pmd_install(mm, vmf->pmd, &vmf->prealloc_pte);
+		pmd_install(mm, vmf->pmd, &vmf->prealloc_pte, PAGE_SIZE);
 
  	return false;
  }

mm/internal.h +1 -1

index 07ad2675a88b..4a01bbf55264 100644
@@ -206,7 +206,7 @@ void folio_activate(struct folio *folio);
  void free_pgtables(struct mmu_gather *tlb, struct ma_state *mas,
  		   struct vm_area_struct *start_vma, unsigned long floor,
  		   unsigned long ceiling, bool mm_wr_locked);
-void pmd_install(struct mm_struct *mm, pmd_t *pmd, pgtable_t *pte);
+void pmd_install(struct mm_struct *mm, pmd_t *pmd, pgtable_t *pte, unsigned long sz);
 
  struct zap_details;
  void unmap_page_range(struct mmu_gather *tlb,

mm/memory.c +12 -7

index d2155ced45f8..2a9eba13a95f 100644
@@ -409,7 +409,12 @@ void free_pgtables(struct mmu_gather *tlb, struct ma_state *mas,
  	} while (vma);
  }
 
-void pmd_install(struct mm_struct *mm, pmd_t *pmd, pgtable_t *pte)
+#ifndef pmd_populate_size
+#define pmd_populate_size(mm, pmdp, pte, sz) pmd_populate(mm, pmdp, pte)
+#define pmd_populate_kernel_size(mm, pmdp, pte, sz) pmd_populate_kernel(mm, pmdp, pte)
+#endif
+
+void pmd_install(struct mm_struct *mm, pmd_t *pmd, pgtable_t *pte, unsigned long sz)
  {
  	spinlock_t *ptl = pmd_lock(mm, pmd);
 

@@ -429,25 +434,25 @@ void pmd_install(struct mm_struct *mm, pmd_t *pmd, pgtable_t *pte)
  		 * smp_rmb() barriers in page table walking code.
  		 */
  		smp_wmb(); /* Could be smp_wmb__xxx(before|after)_spin_lock */
-		pmd_populate(mm, pmd, *pte);
+		pmd_populate_size(mm, pmd, *pte, sz);
  		*pte = NULL;
  	}
  	spin_unlock(ptl);
  }
 
-int __pte_alloc(struct mm_struct *mm, pmd_t *pmd)
+int __pte_alloc(struct mm_struct *mm, pmd_t *pmd, unsigned long sz)
  {
  	pgtable_t new = pte_alloc_one(mm);
  	if (!new)
  		return -ENOMEM;
 
-	pmd_install(mm, pmd, &new);
+	pmd_install(mm, pmd, &new, sz);
  	if (new)
  		pte_free(mm, new);
  	return 0;
  }
 
-int __pte_alloc_kernel(pmd_t *pmd)
+int __pte_alloc_kernel(pmd_t *pmd, unsigned long sz)
  {
  	pte_t *new = pte_alloc_one_kernel(&init_mm);
  	if (!new)

@@ -456,7 +461,7 @@ int __pte_alloc_kernel(pmd_t *pmd)
  	spin_lock(&init_mm.page_table_lock);
  	if (likely(pmd_none(*pmd))) {	/* Has another populated it ? */
  		smp_wmb(); /* See comment in pmd_install() */
-		pmd_populate_kernel(&init_mm, pmd, new);
+		pmd_populate_kernel_size(&init_mm, pmd, new, sz);
  		new = NULL;
  	}
  	spin_unlock(&init_mm.page_table_lock);

@@ -4740,7 +4745,7 @@ vm_fault_t finish_fault(struct vm_fault *vmf)
  		}
 
  		if (vmf->prealloc_pte)
-			pmd_install(vma->vm_mm, vmf->pmd, &vmf->prealloc_pte);
+			pmd_install(vma->vm_mm, vmf->pmd, &vmf->prealloc_pte, PAGE_SIZE);
  		else if (unlikely(pte_alloc(vma->vm_mm, vmf->pmd)))
  			return VM_FAULT_OOM;
  	}

mm/pgalloc-track.h +1 -1

index e9e879de8649..90e37de7ab77 100644
@@ -45,7 +45,7 @@ static inline pmd_t *pmd_alloc_track(struct mm_struct *mm, pud_t *pud,
 
  #define pte_alloc_kernel_track(pmd, address, mask)			\
  	((unlikely(pmd_none(*(pmd))) &&					\
-	  (__pte_alloc_kernel(pmd) || ({*(mask)|=PGTBL_PMD_MODIFIED;0;})))?\
+	  (__pte_alloc_kernel(pmd, PAGE_SIZE) || ({*(mask)|=PGTBL_PMD_MODIFIED;0;})))?\
  		NULL: pte_offset_kernel(pmd, address))
 
  #endif /* _LINUX_PGALLOC_TRACK_H */

mm/userfaultfd.c +2 -2

index 3c3539c573e7..0f129d5c5aa2 100644
@@ -764,7 +764,7 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx,
  			break;
  		}
  		if (unlikely(pmd_none(dst_pmdval)) &&
-		    unlikely(__pte_alloc(dst_mm, dst_pmd))) {
+		    unlikely(__pte_alloc(dst_mm, dst_pmd, PAGE_SIZE))) {
  			err = -ENOMEM;
  			break;
  		}

@@ -1687,7 +1687,7 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
  					err = -ENOENT;
  					break;
  				}
-				if (unlikely(__pte_alloc(mm, src_pmd))) {
+				if (unlikely(__pte_alloc(mm, src_pmd, PAGE_SIZE))) {
  					err = -ENOMEM;
  					break;
  				}

Signature · 1 line

2.44.0

I had a quick look at that document and it seems to provide a good summary of MMU features and principles. However there are some theoritical information which is not fully right in practice. For instance when they say "Segment attributes. These fields define attributes common to all pages in this segment.". This is right in theory if you consider it from Linux page table topology point of view, hence what they call a segment is a PMD entry for Linux. However, in practice each page has its own L1 and L2 attributes and there is not requirement at HW level to have all L1 attributes of all pages of a segment the same.

rlwimi = Rotate Left Word Immediate then Mask Insert. Here it rotates r10 by 23 bits to the left (or 9 to the right) then masks with _PMD_PAGE_512K and inserts it into r11.

It means _PAGE_HUGE bit is copied into lower bit of PS attribute.

PS takes the following values:

PS = 00 ==> Small page (4k or 16k)
PS = 01 ==> 512k page
PS = 10 ==> Undefined
PS = 11 ==> 8M page

That's a RFC, all ideas are welcome, I needed something to replace hugepd_populate()

Quoted 10 lines

I had a quick look at that document and it seems to provide a good summary of MMU features and principles. However there are some theoritical information which is not fully right in practice. For instance when they say "Segment attributes. These fields define attributes common to all pages in this segment.". This is right in theory if you consider it from Linux page table topology point of view, hence what they call a segment is a PMD entry for Linux. However, in practice each page has its own L1 and L2 attributes and there is not requirement at HW level to have all L1 attributes of all pages of a segment the same.

Thanks for taking the time Christophe, highly appreciated.

Quoted 4 lines

rlwimi = Rotate Left Word Immediate then Mask Insert. Here it rotates r10 by 23 bits to the left (or 9 to the right) then masks with _PMD_PAGE_512K and inserts it into r11.

Quoted 8 lines

It means _PAGE_HUGE bit is copied into lower bit of PS attribute.

PS takes the following values:

PS = 00 ==> Small page (4k or 16k)
PS = 01 ==> 512k page
PS = 10 ==> Undefined
PS = 11 ==> 8M page

I see, thanks for the explanation.

Quoted 2 lines

That's a RFC, all ideas are welcome, I needed something to replace hugepd_populate()

The only user interested in pmd_populate() having a sz parameter is 8xx because it will toggle _PMD_PAGE_8M in case of a 8MB mapping.

Would it be possible for 8xx to encode the 'sz' in the *pmd pointer prior to calling down the chain? (something like as we do for PTR_ERR()). Then pmd_populate_{kernel_}size() from 8xx, would extract it like:

unsigned long sz = PTR_SIZE(pmd)

Then we would not need all these 'sz' parameters scattered.

Can that work?

PD: Do you know a way to emulate a 8xx VM? qemu seems to not have support support.

On powerpc 8xx, when a page is 8M size, the information is in the PMD entry. So provide it to pte_leaf_size().

Signed-off-by Christophe Leroy <[email protected]>

Quoted 4 lines

On powerpc 8xx, when a page is 8M size, the information is in the PMD entry. So provide it to pte_leaf_size().

Signed-off-by Christophe Leroy <[email protected]>

Overall looks good to me.

Would be nicer if we could left the arch code untouched.
I wanted to see how this would be if we go down that road and focus only on 8xx at the risk of being more esoteric. pmd_pte_leaf_size() is a name of hell, but could be replaced with __pte_leaf_size for example.

Worth it? Maybe not, anyway, just wanted to give it a go:

  diff --git a/arch/powerpc/include/asm/nohash/32/pte-8xx.h b/arch/powerpc/include/asm/nohash/32/pte-8xx.h
  index 137dc3c84e45..9e3fe6e1083f 100644
  --- a/arch/powerpc/include/asm/nohash/32/pte-8xx.h
  +++ b/arch/powerpc/include/asm/nohash/32/pte-8xx.h
  @@ -151,7 +151,7 @@ static inline unsigned long pgd_leaf_size(pgd_t pgd)

   #define pgd_leaf_size pgd_leaf_size

  -static inline unsigned long pte_leaf_size(pte_t pte)
  +static inline unsigned long pmd_pte_leaf_size(pte_t pte)
   {
          pte_basic_t val = pte_val(pte);
  
  @@ -162,7 +162,7 @@ static inline unsigned long pte_leaf_size(pte_t pte)
          return SZ_4K;
   }

-#define pte_leaf_size pte_leaf_size
+#define pmd_pte_leaf_size pmd_pte_leaf_size

   /*
    * On the 8xx, the page tables are a bit special. For 16k pages, we have
  diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
  index 18019f037bae..2bc2fe3b2b53 100644
  --- a/include/linux/pgtable.h
  +++ b/include/linux/pgtable.h
  @@ -1891,6 +1891,9 @@ typedef unsigned int pgtbl_mod_mask;
   #ifndef pte_leaf_size
   #define pte_leaf_size(x) PAGE_SIZE
   #endif
  +#ifndef pmd_pte_leaf_size
  +#define pmd_pte_leaf_size(x, y) pte_leaf_size(y)
  +#endif

/*
* We always define pmd_pfn for all archs as it's used in lots of generic
diff --git a/kernel/events/core.c b/kernel/events/core.c
index f0128c5ff278..e90a547d2fb2 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -7596,7 +7596,7 @@ static u64 perf_get_pgtable_size(struct mm_struct *mm, unsigned long addr)

          pte = ptep_get_lockless(ptep);
          if (pte_present(pte))
  -               size = pte_leaf_size(pte);
  +               size = pmd_pte_leaf_size(pmd, pte);
          pte_unmap(ptep);

#endif /* CONFIG_HAVE_GUP_FAST */

set_huge_pte_at() expects the real page size, not the psize which is

the index of the page definition in table mmu_psize_defs[]

Fixes 935d4f0c6dc8 ("mm: hugetlb: add huge page size param to set_huge_pte_at()")

Signed-off-by Christophe Leroy <[email protected]>

---
 arch/powerpc/mm/nohash/8xx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/powerpc/mm/nohash/8xx.c +2 -1

index 43d4842bb1c7..d93433e26ded 100644
@@ -94,7 +94,8 @@ static int __ref __early_map_kernel_hugepage(unsigned long va, phys_addr_t pa,
 		return -EINVAL;
 
 	set_huge_pte_at(&init_mm, va, ptep,
-			pte_mkhuge(pfn_pte(pa >> PAGE_SHIFT, prot)), psize);
+			pte_mkhuge(pfn_pte(pa >> PAGE_SHIFT, prot)),
+			1UL << mmu_psize_to_shift(psize));
 
 	return 0;
 }

Signature · 1 line

2.44.0

Quoted 1 line

set_huge_pte_at() expects the real page size, not the psize which is

"expects the size of the huge page" sounds bettter?

Quoted 5 lines

the index of the page definition in table mmu_psize_defs[]

Fixes 935d4f0c6dc8 ("mm: hugetlb: add huge page size param to set_huge_pte_at()")

Signed-off-by Christophe Leroy <[email protected]>

Reviewed-by Oscar Salvador <[email protected]>

AFAICS, this fixup is not related to the series, right? (yes, you will the parameter later)
I would have it at the very beginning of the series.

Quoted 23 lines

---
  arch/powerpc/mm/nohash/8xx.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

arch/powerpc/mm/nohash/8xx.c +2 -1

index 43d4842bb1c7..d93433e26ded 100644
@@ -94,7 +94,8 @@ static int __ref __early_map_kernel_hugepage(unsigned long va, phys_addr_t pa,
  		return -EINVAL;
 
  	set_huge_pte_at(&init_mm, va, ptep,
-			pte_mkhuge(pfn_pte(pa >> PAGE_SHIFT, prot)), psize);
+			pte_mkhuge(pfn_pte(pa >> PAGE_SHIFT, prot)),
+			1UL << mmu_psize_to_shift(psize));
 
  	return 0;
  }

Signature · 1 line

2.44.0

Hi Oscar, hi Michael,

Le 20/05/2024 à 11:14, Oscar Salvador a écrit :

Quoted 3 lines · Christophe Leroy

Quoted 1 line

set_huge_pte_at() expects the real page size, not the psize which is

Quoted 1 line

"expects the size of the huge page" sounds bettter?

Parameter 'pzize' already provides the size of the hugepage, but not in the way set_huge_pte_at() expects it.

psize has one of the values defined by MMU_PAGE_XXX macros defined in arch/powerpc/include/asm/mmu.h while set_huge_pte_at() expects the size as a value.

Quoted 6 lines

Hi Oscar, hi Michael,

Le 20/05/2024 à 11:14, Oscar Salvador a écrit :

Quoted 3 lines · Christophe Leroy

Quoted 1 line

set_huge_pte_at() expects the real page size, not the psize which is

Quoted 8 lines

Quoted 1 line

"expects the size of the huge page" sounds bettter?

Parameter 'pzize' already provides the size of the hugepage, but not in the way set_huge_pte_at() expects it.

psize has one of the values defined by MMU_PAGE_XXX macros defined in arch/powerpc/include/asm/mmu.h while set_huge_pte_at() expects the size as a value.

Yes, psize is an index, which is not a size by itself but used to get mmu_psize_def.shift to see the actual size, I guess.
This is why I thought that being explicit about "expects the size of the huge page" was better.

But no strong feelings here.

Hi Oscar, hi Michael,

Le 20/05/2024 à 11:14, Oscar Salvador a écrit :

Quoted 4 lines · Christophe Leroy

Quoted 1 line

set_huge_pte_at() expects the real page size, not the psize which is

"expects the size of the huge page" sounds bettter?

Parameter 'pzize' already provides the size of the hugepage, but not in the way set_huge_pte_at() expects it.

psize has one of the values defined by MMU_PAGE_XXX macros defined in arch/powerpc/include/asm/mmu.h while set_huge_pte_at() expects the size as a value.

Quoted 8 lines

Quoted 5 lines

the index of the page definition in table mmu_psize_defs[]

Fixes 935d4f0c6dc8 ("mm: hugetlb: add huge page size param to set_huge_pte_at()")

Signed-off-by Christophe Leroy <[email protected]>

Reviewed-by Oscar Salvador <[email protected]>

Quoted 3 lines

AFAICS, this fixup is not related to the series, right? (yes, you will the parameter later)
I would have it at the very beginning of the series.

You are right, I should have submitted it separately.

Michael can you take it as a fix for 6.10 ?

Yeah I can. Does it actually cause a bug at runtime (I assume so)?

Quoted 1 line

Yeah I can. Does it actually cause a bug at runtime (I assume so)?

No, currently set_huge_pte_at() from 8xx ignores the 'sz' parameter. But it will be used after this series.

Le 21/05/2024 à 11:26, Oscar Salvador a écrit :

Quoted 6 lines · Michael Ellerman

Quoted 1 line

Yeah I can. Does it actually cause a bug at runtime (I assume so)?

No, currently set_huge_pte_at() from 8xx ignores the 'sz' parameter. But it will be used after this series.

Ah yes, I mixed things up with something else in my mind.

So this patch doesn't qualify as a fix and doesn't need to be handled separately from the series and doesn't really need to go on top of the series either, I think it is better to keep it grouped with other 8xx changes.

In order to fit better with standard Linux page tables layout, add support for 8M pages using contiguous PTE entries in a standard page table. Page tables will then be populated with 1024 similar entries and two PMD entries will point to that page table.

The PMD entries also get a flag to tell it is addressing an 8M page, this is required for the HW tablewalk assistance.

Signed-off-by Christophe Leroy <[email protected]>

arch/powerpc/include/asm/hugetlb.h +2 -0

+		ptep = pte_offset_kernel(pmdp, 0);
+	return ptep_get(ptep);
 }
 
 #define __HAVE_ARCH_HUGE_SET_HUGE_PTE_AT

@@ -53,7 +33,14 @@ void set_huge_pte_at(struct mm_struct *mm, unsigned long addr, pte_t *ptep,
 static inline void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
 				  pte_t *ptep, unsigned long sz)

arch/powerpc/include/asm/hugetlb.h +0 -0

 }
 
 #define __HAVE_ARCH_HUGE_PTEP_SET_WRPROTECT

@@ -63,7 +50,14 @@ static inline void huge_ptep_set_wrprotect(struct mm_struct *mm,
 	unsigned long clr = ~pte_val(pte_wrprotect(__pte(~0)));

arch/powerpc/include/asm/hugetlb.h +0 -0

 #endif /* _ASM_POWERPC_PGALLOC_32_H */

arch/powerpc/include/asm/nohash/32/pte-8xx.h +0 -0

index 07df6b664861..b05cc4f87713 100644

- * For other page sizes, we have a single entry in the table.
+ * For 8M pages, we have 1024 entries as if it was
+ * 4M pages, but they are flagged as 8M pages for the hardware.

arch/powerpc/include/asm/nohash/32/pte-8xx.h +7 -0

+static inline void pmd_populate_kernel_size(struct mm_struct *mm, pmd_t *pmdp,
+					    pte_t *pte, unsigned long sz)
+{
+	if (sz == SZ_8M)
+		*pmdp = __pmd(__pa(pte) | _PMD_PRESENT | _PMD_PAGE_8M);
+	else
+		*pmdp = __pmd(__pa(pte) | _PMD_PRESENT);

arch/powerpc/mm/hugetlbpage.c +2 -4

@@ -183,9 +183,6 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, struct vm_area_struct *vma,
 	if (!hpdp)
 		return NULL;
 
-	if (IS_ENABLED(CONFIG_PPC_8xx) && pshift < PMD_SHIFT)
-		return pte_alloc_huge(mm, (pmd_t *)hpdp, addr, sz);
-
 	BUG_ON(!hugepd_none(*hpdp) && !hugepd_ok(*hpdp));
 
 	if (hugepd_none(*hpdp) && __hugepte_alloc(mm, hpdp, addr,

@@ -198,10 +195,18 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, struct vm_area_struct *vma,
 pte_t *huge_pte_alloc(struct mm_struct *mm, struct vm_area_struct *vma,
 		      unsigned long addr, unsigned long sz)
 {
+	pmd_t *pmd = pmd_off(mm, addr);
+
 	if (sz < PMD_SIZE)
-		return pte_alloc_huge(mm, pmd_off(mm, addr), addr, sz);

arch/powerpc/mm/nohash/8xx.c +0 -6

index d93433e26ded..99f656b3f9f3 100644
@@ -48,20 +48,6 @@ unsigned long p_block_mapped(phys_addr_t pa)
 	return 0;
 }
 
-static pte_t __init *early_hugepd_alloc_kernel(hugepd_t *pmdp, unsigned long va)
-{
-	if (hpd_val(*pmdp) == 0) {
-		pte_t *ptep = memblock_alloc(sizeof(pte_basic_t), SZ_4K);
-
-		if (!ptep)

arch/powerpc/mm/pgtable.c +0 -0

index acdf64c9b93e..59f0d7706d2f 100644
 

Quoted 8 lines

In order to fit better with standard Linux page tables layout, add support for 8M pages using contiguous PTE entries in a standard page table. Page tables will then be populated with 1024 similar entries and two PMD entries will point to that page table.

The PMD entries also get a flag to tell it is addressing an 8M page, this is required for the HW tablewalk assistance.

Quoted 1 line

Signed-off-by Christophe Leroy <[email protected]>

I guess that this will slightly change if you remove patch#1 and patch#2 as you said you will.
So I will not comment on the overall design because I do not know how it will look afterwards, but just some things that caught my eye

arch/powerpc/include/asm/hugetlb.h void hugetlb_free_pgd_range(struct mmu_gather *tlb, unsigned long addr, · line 47

Quoted 12 lines

arch/powerpc/include/asm/hugetlb.h +4 -1

@@ -41,7 +41,16 @@ void hugetlb_free_pgd_range(struct mmu_gather *tlb, unsigned long addr,
  static inline pte_t huge_ptep_get_and_clear(struct mm_struct *mm,
  					    unsigned long addr, pte_t *ptep)
  {
-	return __pte(pte_update(mm, addr, ptep, ~0UL, 0, 1));
+	pmd_t *pmdp = (pmd_t *)ptep;
+	pte_t pte;
+
+	if (IS_ENABLED(CONFIG_PPC_8xx) && pmdp == pmd_off(mm, ALIGN_DOWN(addr, SZ_8M))) {

There are quite some places where you do the "pmd_off" to check whether that is a 8MB entry.

I think it would make somse sense to have some kind of macro/function to make more clear what we are checking against.
e.g:

#define pmd_is_SZ_8M(mm, addr, pmdp) (pmdp == pmd_off(mm, ALIGN_DOWN(addr, SZ_8M))) (or whatever name you see fit)

then you would just need

  if (IS_ENABLED(CONFIG_PPC_8xx && pmd_is_SZ_8M(mm, addr, pdmp))

Because I see that is also scaterred in 8xx code.

arch/powerpc/include/asm/hugetlb.h void hugetlb_free_pgd_range(struct mmu_gather *tlb, unsigned long addr, · line 49

Quoted 2 lines

arch/powerpc/include/asm/hugetlb.h +2 -0

+		pte = __pte(pte_update(mm, addr, pte_offset_kernel(pmdp, 0), ~0UL, 0, 1));
+		pte_update(mm, addr, pte_offset_kernel(pmdp + 1, 0), ~0UL, 0, 1);

I have this fresh one because I recently read about 8xx pagetables, but not sure how my memory will survive this, so maybe throw a little comment in there that we are pointing the two pmds to the area.

Also, the way we pass the parameters here to pte_update() is a bit awkward. Ideally we should be using some meaningful names?

clr_all_bits = ~0UL
set_bits = 0
bool is_huge = true

pte_update(mm, addr, pte_offset_kernel(pmdp + 1, 0), clr_all_bits, set_bits, is_huge)

or something along those lines

arch/powerpc/include/asm/hugetlb.h void hugetlb_free_pgd_range(struct mmu_gather *tlb, unsigned long addr, · line 50

Quoted 5 lines

arch/powerpc/include/asm/hugetlb.h +1 -3

-static inline int check_and_get_huge_psize(int shift)
-{
-	return shift_to_mmu_psize(shift);
+	if (pmdp == pmd_off(mm, ALIGN_DOWN(addr, SZ_8M)))

Here you could also use the pmd_is_SZ_8M()

Quoted 5 lines

arch/powerpc/include/asm/hugetlb.h +2 -0

+		ptep = pte_offset_kernel(pmdp, 0);
+	return ptep_get(ptep);
  }

Quoted 4 lines

  #define __HAVE_ARCH_HUGE_SET_HUGE_PTE_AT

arch/powerpc/include/asm/hugetlb.h +0 -0

@@ -53,7 +33,14 @@ void set_huge_pte_at(struct mm_struct *mm, unsigned long addr, pte_t *ptep,
  static inline void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
  				  pte_t *ptep, unsigned long sz)

arch/powerpc/include/asm/hugetlb.h void set_huge_pte_at(struct mm_struct *mm, unsigned long addr, pte_t *ptep, · line 43

Quoted 10 lines

arch/powerpc/include/asm/hugetlb.h +8 -1

  {
-	pte_update(mm, addr, ptep, ~0UL, 0, 1);
+	pmd_t *pmdp = (pmd_t *)ptep;
+
+	if (pmdp == pmd_off(mm, ALIGN_DOWN(addr, SZ_8M))) {
+		pte_update(mm, addr, pte_offset_kernel(pmdp, 0), ~0UL, 0, 1);
+		pte_update(mm, addr, pte_offset_kernel(pmdp + 1, 0), ~0UL, 0, 1);
+	} else {
+		pte_update(mm, addr, ptep, ~0UL, 0, 1);
+	}

Could we not leverage this in huge_ptep_get_and_clear()?

AFAICS,

  huge_pet_get_and_clear(mm, addr, pte_t *p)
  {
       pte_t pte = pte_val(*p);

       huge_pte_clear(mm, addr, p);
       return pte;

}

Or maybe it is not that easy if different powerpc platforms provide their own. It might be worth checking though.

Quoted 7 lines

arch/powerpc/include/asm/hugetlb.h +0 -0

  }
 
  #define __HAVE_ARCH_HUGE_PTEP_SET_WRPROTECT

@@ -63,7 +50,14 @@ static inline void huge_ptep_set_wrprotect(struct mm_struct *mm,
  	unsigned long clr = ~pte_val(pte_wrprotect(__pte(~0)));
  	unsigned long set = pte_val(pte_wrprotect(__pte(0)));
 

arch/powerpc/include/asm/hugetlb.h static inline void huge_ptep_set_wrprotect(struct mm_struct *mm, · line 59

Quoted 8 lines

arch/powerpc/include/asm/hugetlb.h +7 -1

-	pte_update(mm, addr, ptep, clr, set, 1);
+	pmd_t *pmdp = (pmd_t *)ptep;
+
+	if (pmdp == pmd_off(mm, ALIGN_DOWN(addr, SZ_8M))) {
+		pte_update(mm, addr, pte_offset_kernel(pmdp, 0), clr, set, 1);
+		pte_update(mm, addr, pte_offset_kernel(pmdp + 1, 0), clr, set, 1);
+	} else {
+		pte_update(mm, addr, ptep, clr, set, 1);

I would replace the "1" with "is_huge" or "huge", as being done in __ptep_set_access_flags , something that makes it more clear without the need to check pte_update().

Quoted 7 lines

arch/powerpc/include/asm/hugetlb.h +0 -0

  #endif /* _ASM_POWERPC_PGALLOC_32_H */

arch/powerpc/include/asm/nohash/32/pte-8xx.h +0 -0

index 07df6b664861..b05cc4f87713 100644

...

Quoted 3 lines

- * For other page sizes, we have a single entry in the table.
+ * For 8M pages, we have 1024 entries as if it was
+ * 4M pages, but they are flagged as 8M pages for the hardware.

Maybe drop a comment that a single PMD entry is worth 4MB, so

Quoted 4 lines

+ * For 4k pages, we have a single entry in the table. */

arch/powerpc/include/asm/nohash/32/pte-8xx.h +0 -1

-static pmd_t *pmd_off(struct mm_struct *mm, unsigned long addr);

arch/powerpc/include/asm/nohash/32/pte-8xx.h static inline unsigned long pte_leaf_size(pmd_t pmd, pte_t pte) · line 182

Quoted 10 lines

arch/powerpc/include/asm/nohash/32/pte-8xx.h +2 -4

-static int hugepd_ok(hugepd_t hpd);
-
  static inline int number_of_cells_per_pte(pmd_t *pmd, pte_basic_t val, int huge)
  {
  	if (!huge)
  		return PAGE_SIZE / SZ_4K;
-	else if (hugepd_ok(*((hugepd_t *)pmd)))
-		return 1;
+	else if ((pmd_val(*pmd) & _PMD_PAGE_MASK) == _PMD_PAGE_8M)
+		return SZ_4M / SZ_4K;

this becomes more intuitive.

Quoted 12 lines

arch/powerpc/include/asm/nohash/32/pte-8xx.h +12 -0

+static inline void pmd_populate_kernel_size(struct mm_struct *mm, pmd_t *pmdp,
+					    pte_t *pte, unsigned long sz)
+{
+	if (sz == SZ_8M)
+		*pmdp = __pmd(__pa(pte) | _PMD_PRESENT | _PMD_PAGE_8M);
+	else
+		*pmdp = __pmd(__pa(pte) | _PMD_PRESENT);
+}
+
+static inline void pmd_populate_size(struct mm_struct *mm, pmd_t *pmdp,
+				     pgtable_t pte_page, unsigned long sz)
+{

arch/powerpc/include/asm/nohash/32/pte-8xx.h static inline pte_t ptep_get(pte_t *ptep) · line 241

Quoted 5 lines

arch/powerpc/include/asm/nohash/32/pte-8xx.h +5 -0

+	if (sz == SZ_8M)
+		*pmdp = __pmd(__pa(pte_page) | _PMD_USER | _PMD_PRESENT | _PMD_PAGE_8M);
+	else
+		*pmdp = __pmd(__pa(pte_page) | _PMD_USER | _PMD_PRESENT);
+}

In patch#1 you mentioned this will change with the removal of patch#1 and patch#2.

Quoted 23 lines

arch/powerpc/mm/hugetlbpage.c +3 -4

@@ -183,9 +183,6 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, struct vm_area_struct *vma,
  	if (!hpdp)
  		return NULL;
 
-	if (IS_ENABLED(CONFIG_PPC_8xx) && pshift < PMD_SHIFT)
-		return pte_alloc_huge(mm, (pmd_t *)hpdp, addr, sz);
-
  	BUG_ON(!hugepd_none(*hpdp) && !hugepd_ok(*hpdp));
 
  	if (hugepd_none(*hpdp) && __hugepte_alloc(mm, hpdp, addr,

@@ -198,10 +195,18 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, struct vm_area_struct *vma,
  pte_t *huge_pte_alloc(struct mm_struct *mm, struct vm_area_struct *vma,
  		      unsigned long addr, unsigned long sz)
  {
+	pmd_t *pmd = pmd_off(mm, addr);
+
  	if (sz < PMD_SIZE)
-		return pte_alloc_huge(mm, pmd_off(mm, addr), addr, sz);
+		return pte_alloc_huge(mm, pmd, addr, sz);
 

arch/powerpc/mm/hugetlbpage.c pte_t *huge_pte_alloc(struct mm_struct *mm, struct vm_area_struct *vma, · line 209

Quoted 8 lines

arch/powerpc/mm/hugetlbpage.c +7 -1

-	return NULL;
+	if (sz != SZ_8M)
+		return NULL;
+	if (!pte_alloc_huge(mm, pmd, addr, sz))
+		return NULL;
+	if (!pte_alloc_huge(mm, pmd + 1, addr, sz))
+		return NULL;
+	return (pte_t *)pmd;

I think that having the check for invalid huge page sizes upfront would make more sense, maybe just a matter of taste.

  /* Unsupported size */
  if (sz > PMD_SIZE && sz = SZ_8M)
      return NULL;

if (sz < PMD_SIZE)

     ...

/* 8MB huge pages */
...

  return (pte_t *) pmd;

Also, I am not a big fan of the two separate pte_alloc_huge() for pmd#0+pmd#1, and I am thinking we might want to hide that within a function and drop a comment in there explaining why we are updatng both pmds.

Quoted 22 lines

arch/powerpc/mm/nohash/8xx.c +0 -11

index d93433e26ded..99f656b3f9f3 100644
@@ -48,20 +48,6 @@ unsigned long p_block_mapped(phys_addr_t pa)
  	return 0;
  }
 
-static pte_t __init *early_hugepd_alloc_kernel(hugepd_t *pmdp, unsigned long va)
-{
-	if (hpd_val(*pmdp) == 0) {
-		pte_t *ptep = memblock_alloc(sizeof(pte_basic_t), SZ_4K);
-
-		if (!ptep)
-			return NULL;
-
-		hugepd_populate_kernel((hugepd_t *)pmdp, ptep, PAGE_SHIFT_8M);
-		hugepd_populate_kernel((hugepd_t *)pmdp + 1, ptep, PAGE_SHIFT_8M);
-	}

arch/powerpc/mm/nohash/8xx.c unsigned long p_block_mapped(phys_addr_t pa) · line 52

Quoted 5 lines

arch/powerpc/mm/nohash/8xx.c +0 -3

-	return hugepte_offset(*(hugepd_t *)pmdp, va, PGDIR_SHIFT);
-}
-
  static int __ref __early_map_kernel_hugepage(unsigned long va, phys_addr_t pa,
  					     pgprot_t prot, int psize, bool new)

Am I blind or do we never use the 'new' parameter? I checked the tree and it seems we always pass it 'true'.

arch/powerpc/mm/nohash/8xx.c: err = __early_map_kernel_hugepage(v, p, prot, MMU_PAGE_512K, new);
arch/powerpc/mm/nohash/8xx.c: err = __early_map_kernel_hugepage(v, p, prot, MMU_PAGE_8M, new);
arch/powerpc/mm/nohash/8xx.c: err = __early_map_kernel_hugepage(v, p, prot, MMU_PAGE_512K, new);
arch/powerpc/mm/nohash/8xx.c:
__early_map_kernel_hugepage(VIRT_IMMR_BASE, PHYS_IMMR_BASE, PAGE_KERNEL_NCG, MMU_PAGE_512K, true);

I think we can drop the 'new' and the block code that tries to handle it?

Quoted 4 lines

arch/powerpc/mm/pgtable.c +0 -0

index acdf64c9b93e..59f0d7706d2f 100644

arch/powerpc/mm/pgtable.c void set_huge_pte_at(struct mm_struct *mm, unsigned long addr, pte_t *ptep, · line 326

Quoted 10 lines

arch/powerpc/mm/pgtable.c +10 -0

+void set_huge_pte_at(struct mm_struct *mm, unsigned long addr, pte_t *ptep,
+		     pte_t pte, unsigned long sz)
+{
+	pmd_t *pmdp = pmd_off(mm, addr);
+
+	pte = set_pte_filter(pte, addr);
+
+	if (sz == SZ_8M) {
+		__set_huge_pte_at(pmdp, pte_offset_kernel(pmdp, 0), pte_val(pte));
+		__set_huge_pte_at(pmdp, pte_offset_kernel(pmdp + 1, 0), pte_val(pte) + SZ_4M);

You also mentioned that this would slightly change after you drop patch#0 and patch#1.
The only comment I have right know would be to add a little comment explaining the layout (the replication of 1024 entries), or just something like "see comment from number_of_cells_per_pte".

On 8xx, only the shift field is used in struct mmu_psize_def

Remove other fields and related macros.

Signed-off-by Christophe Leroy <[email protected]>

_PAGE_PSIZE macro is never used outside the place it is defined and is used only on 8xx and e500.

Remove indirection, remove it and use its content directly.

Signed-off-by Christophe Leroy <[email protected]>

Building on 32 bits with pmd_leaf() not returning always false leads to the following error:

CC arch/powerpc/mm/pgtable.o
arch/powerpc/mm/pgtable.c: In function '__find_linux_pte':
arch/powerpc/mm/pgtable.c:506:1: error: function may return address of local variable [-Werror=return-local-addr]
506 | }

      | ^

arch/powerpc/mm/pgtable.c:394:15: note: declared here
394 | pud_t pud, *pudp;
| ^~~
arch/powerpc/mm/pgtable.c:394:15: note: declared here

This is due to pmd_offset() being a no-op in that case.

So rework it for powerpc/32 so that pXd_offset() are used on real pointers and not on on-stack copies.

Signed-off-by Christophe Leroy <[email protected]>

---
 arch/powerpc/mm/pgtable.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

arch/powerpc/mm/pgtable.c +7 -2

index 59f0d7706d2f..51ee508eeb5b 100644
@@ -390,8 +390,12 @@ pte_t *__find_linux_pte(pgd_t *pgdir, unsigned long ea,
 			bool *is_thp, unsigned *hpage_shift)
 {
 	pgd_t *pgdp;
-	p4d_t p4d, *p4dp;
-	pud_t pud, *pudp;
+	p4d_t *p4dp;
+	pud_t *pudp;
+#ifdef CONFIG_PPC64
+	p4d_t p4d;
+	pud_t pud;
+#endif
 	pmd_t pmd, *pmdp;
 	pte_t *ret_pte;
 	hugepd_t *hpdp = NULL;

@@ -412,6 +416,7 @@ pte_t *__find_linux_pte(pgd_t *pgdir, unsigned long ea,
 	 */
 	pgdp = pgdir + pgd_index(ea);
 	p4dp = p4d_offset(pgdp, ea);
+#ifdef CONFIG_PPC64
 	p4d  = READ_ONCE(*p4dp);
 	pdshift = P4D_SHIFT;
 

Quoted 2 lines

Building on 32 bits with pmd_leaf() not returning always false leads to the following error:

I am curious though.
pmd_leaf is only defined in include/linux/pgtable.h for 32bits, and is hardcoded to false.
I do not see where we change it in previous patches, so is this artificial?

Quoted 6 lines

CC arch/powerpc/mm/pgtable.o
arch/powerpc/mm/pgtable.c: In function '__find_linux_pte':
arch/powerpc/mm/pgtable.c:506:1: error: function may return address of local variable [-Werror=return-local-addr]
506 | }

Quoted 7 lines

       | ^

arch/powerpc/mm/pgtable.c:394:15: note: declared here
394 | pud_t pud, *pudp;
| ^~~
arch/powerpc/mm/pgtable.c:394:15: note: declared here

This is due to pmd_offset() being a no-op in that case.

This is because 32bits powerpc include pgtable-nopmd.h?

Quoted 37 lines

So rework it for powerpc/32 so that pXd_offset() are used on real pointers and not on on-stack copies.

Signed-off-by Christophe Leroy <[email protected]>

---
  arch/powerpc/mm/pgtable.c | 14 ++++++++++++--
  1 file changed, 12 insertions(+), 2 deletions(-)

arch/powerpc/mm/pgtable.c +7 -2

index 59f0d7706d2f..51ee508eeb5b 100644
@@ -390,8 +390,12 @@ pte_t *__find_linux_pte(pgd_t *pgdir, unsigned long ea,
  			bool *is_thp, unsigned *hpage_shift)
  {
  	pgd_t *pgdp;
-	p4d_t p4d, *p4dp;
-	pud_t pud, *pudp;
+	p4d_t *p4dp;
+	pud_t *pudp;
+#ifdef CONFIG_PPC64
+	p4d_t p4d;
+	pud_t pud;
+#endif
  	pmd_t pmd, *pmdp;
  	pte_t *ret_pte;
  	hugepd_t *hpdp = NULL;

@@ -412,6 +416,7 @@ pte_t *__find_linux_pte(pgd_t *pgdir, unsigned long ea,
  	 */
  	pgdp = pgdir + pgd_index(ea);
  	p4dp = p4d_offset(pgdp, ea);
+#ifdef CONFIG_PPC64
  	p4d  = READ_ONCE(*p4dp);
  	pdshift = P4D_SHIFT;
 

@@ -452,6 +457,11 @@ pte_t *__find_linux_pte(pgd_t *pgdir, unsigned long ea,
 

arch/powerpc/mm/pgtable.c pte_t *__find_linux_pte(pgd_t *pgdir, unsigned long ea, · line 463

Quoted 6 lines

arch/powerpc/mm/pgtable.c +4 -0

  	pdshift = PMD_SHIFT;
  	pmdp = pmd_offset(&pud, ea);
+#else
+	p4dp = p4d_offset(pgdp, ea);
+	pudp = pud_offset(p4dp, ea);
+	pmdp = pmd_offset(pudp, ea);

I would drop a comment on top explaining that these are no-op for 32bits, otherwise it might not be obvious to people as why this distiction between 64 and 32bits.

Other than that looks good to me

huge_pte_alloc() for non-HUGEPD targets is reserved for 8xx at the moment. In order to convert other targets for non-HUGEPD, complement huge_pte_alloc() to support any standard cont-PxD setup.

Signed-off-by Christophe Leroy <[email protected]>

---
 arch/powerpc/mm/hugetlbpage.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

arch/powerpc/mm/hugetlbpage.c +17 -1

index 42b12e1ec851..f8aefa1e7363 100644
@@ -195,11 +195,34 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, struct vm_area_struct *vma,
 pte_t *huge_pte_alloc(struct mm_struct *mm, struct vm_area_struct *vma,
 		      unsigned long addr, unsigned long sz)
 {
-	pmd_t *pmd = pmd_off(mm, addr);
+	pgd_t *pgd;
+	p4d_t *p4d;
+	pud_t *pud;
+	pmd_t *pmd;
+
+	addr &= ~(sz - 1);
+	pgd = pgd_offset(mm, addr);
+
+	p4d = p4d_offset(pgd, addr);
+	if (sz >= PGDIR_SIZE)
+		return (pte_t *)p4d;
+
+	pud = pud_alloc(mm, p4d, addr);
+	if (!pud)
+		return NULL;
+	if (sz >= PUD_SIZE)
+		return (pte_t *)pud;

Quoted 40 lines

huge_pte_alloc() for non-HUGEPD targets is reserved for 8xx at the moment. In order to convert other targets for non-HUGEPD, complement huge_pte_alloc() to support any standard cont-PxD setup.

Signed-off-by Christophe Leroy <[email protected]>

---
  arch/powerpc/mm/hugetlbpage.c | 25 ++++++++++++++++++++++++-
  1 file changed, 24 insertions(+), 1 deletion(-)

arch/powerpc/mm/hugetlbpage.c +21 -1

index 42b12e1ec851..f8aefa1e7363 100644
@@ -195,11 +195,34 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, struct vm_area_struct *vma,
  pte_t *huge_pte_alloc(struct mm_struct *mm, struct vm_area_struct *vma,
  		      unsigned long addr, unsigned long sz)
  {
-	pmd_t *pmd = pmd_off(mm, addr);
+	pgd_t *pgd;
+	p4d_t *p4d;
+	pud_t *pud;
+	pmd_t *pmd;
+
+	addr &= ~(sz - 1);
+	pgd = pgd_offset(mm, addr);
+
+	p4d = p4d_offset(pgd, addr);
+	if (sz >= PGDIR_SIZE)
+		return (pte_t *)p4d;
+
+	pud = pud_alloc(mm, p4d, addr);
+	if (!pud)
+		return NULL;
+	if (sz >= PUD_SIZE)
+		return (pte_t *)pud;
+
+	pmd = pmd_alloc(mm, pud, addr);
+	if (!pmd)
+		return NULL;
 

arch/powerpc/mm/hugetlbpage.c pte_t *huge_pte_alloc(struct mm_struct *mm, struct vm_area_struct *vma, · line 224

Quoted 5 lines

arch/powerpc/mm/hugetlbpage.c +2 -0

  	if (sz < PMD_SIZE)
  		return pte_alloc_huge(mm, pmd, addr, sz);
 
+	if (!IS_ENABLED(CONFIG_PPC_8xx))
+		return (pte_t *)pmd;

So only 8xx has cont-PMD for hugepages?

arch/powerpc/mm/hugetlbpage.c pte_t *huge_pte_alloc(struct mm_struct *mm, struct vm_area_struct *vma, · line 227

Quoted 4 lines

arch/powerpc/mm/hugetlbpage.c +1 -0

+
  	if (sz != SZ_8M)
  		return NULL;

Since this function is the core for allocation huge pages, I think it would benefit from a comment at the top explaining the possible layouts. e.g: Who can have cont-{P4d,PUD,PMD} etc.
A brief explanation of the possible scheme for all powerpc platforms.