[PATCH] Revert "mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge"

[PATCH] Revert "mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge"

[Jonathan Marek]

c742199a breaks arm64 for some configs because it stubs out functions which
should not have been stubbed out.

With 4K pages and ARM64_VA_BITS_39=y, the kernel crashes early on unmapped
1G pages in the linear map caused by pud_set_huge() in alloc_init_pud()
being stubbed out. Reverting c742199a fixes the crash.

Fixes: c742199a ("mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge")
Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 arch/arm64/mm/mmu.c     | 20 ++++++++------------
 arch/x86/mm/pgtable.c   | 34 +++++++++++++++-------------------
 include/linux/pgtable.h | 26 +-------------------------
 3 files changed, 24 insertions(+), 56 deletions(-)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index d745865084488..9ff0de1b2b93c 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -1339,7 +1339,6 @@ void *__init fixmap_remap_fdt(phys_addr_t dt_phys, int *size, pgprot_t prot)
 	return dt_virt;
 }
 
-#if CONFIG_PGTABLE_LEVELS > 3
 int pud_set_huge(pud_t *pudp, phys_addr_t phys, pgprot_t prot)
 {
 	pud_t new_pud = pfn_pud(__phys_to_pfn(phys), mk_pud_sect_prot(prot));
@@ -1354,16 +1353,6 @@ int pud_set_huge(pud_t *pudp, phys_addr_t phys, pgprot_t prot)
 	return 1;
 }
 
-int pud_clear_huge(pud_t *pudp)
-{
-	if (!pud_sect(READ_ONCE(*pudp)))
-		return 0;
-	pud_clear(pudp);
-	return 1;
-}
-#endif
-
-#if CONFIG_PGTABLE_LEVELS > 2
 int pmd_set_huge(pmd_t *pmdp, phys_addr_t phys, pgprot_t prot)
 {
 	pmd_t new_pmd = pfn_pmd(__phys_to_pfn(phys), mk_pmd_sect_prot(prot));
@@ -1378,6 +1367,14 @@ int pmd_set_huge(pmd_t *pmdp, phys_addr_t phys, pgprot_t prot)
 	return 1;
 }
 
+int pud_clear_huge(pud_t *pudp)
+{
+	if (!pud_sect(READ_ONCE(*pudp)))
+		return 0;
+	pud_clear(pudp);
+	return 1;
+}
+
 int pmd_clear_huge(pmd_t *pmdp)
 {
 	if (!pmd_sect(READ_ONCE(*pmdp)))
@@ -1385,7 +1382,6 @@ int pmd_clear_huge(pmd_t *pmdp)
 	pmd_clear(pmdp);
 	return 1;
 }
-#endif
 
 int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
 {
diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
index 3364fe62b9037..3481b35cb4ec7 100644
--- a/arch/x86/mm/pgtable.c
+++ b/arch/x86/mm/pgtable.c
@@ -682,7 +682,6 @@ int p4d_clear_huge(p4d_t *p4d)
 }
 #endif
 
-#if CONFIG_PGTABLE_LEVELS > 3
 /**
  * pud_set_huge - setup kernel PUD mapping
  *
@@ -721,23 +720,6 @@ int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
 	return 1;
 }
 
-/**
- * pud_clear_huge - clear kernel PUD mapping when it is set
- *
- * Returns 1 on success and 0 on failure (no PUD map is found).
- */
-int pud_clear_huge(pud_t *pud)
-{
-	if (pud_large(*pud)) {
-		pud_clear(pud);
-		return 1;
-	}
-
-	return 0;
-}
-#endif
-
-#if CONFIG_PGTABLE_LEVELS > 2
 /**
  * pmd_set_huge - setup kernel PMD mapping
  *
@@ -768,6 +750,21 @@ int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
 	return 1;
 }
 
+/**
+ * pud_clear_huge - clear kernel PUD mapping when it is set
+ *
+ * Returns 1 on success and 0 on failure (no PUD map is found).
+ */
+int pud_clear_huge(pud_t *pud)
+{
+	if (pud_large(*pud)) {
+		pud_clear(pud);
+		return 1;
+	}
+
+	return 0;
+}
+
 /**
  * pmd_clear_huge - clear kernel PMD mapping when it is set
  *
@@ -782,7 +779,6 @@ int pmd_clear_huge(pmd_t *pmd)
 
 	return 0;
 }
-#endif
 
 #ifdef CONFIG_X86_64
 /**
diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
index d147480cdefc7..e24d2c992b112 100644
--- a/include/linux/pgtable.h
+++ b/include/linux/pgtable.h
@@ -1397,34 +1397,10 @@ static inline int p4d_clear_huge(p4d_t *p4d)
 }
 #endif /* !__PAGETABLE_P4D_FOLDED */
 
-#ifndef __PAGETABLE_PUD_FOLDED
 int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot);
-int pud_clear_huge(pud_t *pud);
-#else
-static inline int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
-{
-	return 0;
-}
-static inline int pud_clear_huge(pud_t *pud)
-{
-	return 0;
-}
-#endif /* !__PAGETABLE_PUD_FOLDED */
-
-#ifndef __PAGETABLE_PMD_FOLDED
 int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot);
+int pud_clear_huge(pud_t *pud);
 int pmd_clear_huge(pmd_t *pmd);
-#else
-static inline int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
-{
-	return 0;
-}
-static inline int pmd_clear_huge(pmd_t *pmd)
-{
-	return 0;
-}
-#endif /* !__PAGETABLE_PMD_FOLDED */
-
 int p4d_free_pud_page(p4d_t *p4d, unsigned long addr);
 int pud_free_pmd_page(pud_t *pud, unsigned long addr);
 int pmd_free_pte_page(pmd_t *pmd, unsigned long addr);
-- 
2.26.1

[PATCH] Revert "mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge"

[Jonathan Marek]

c742199a breaks arm64 for some configs because it stubs out functions which
should not have been stubbed out.

With 4K pages and ARM64_VA_BITS_39=y, the kernel crashes early on unmapped
1G pages in the linear map caused by pud_set_huge() in alloc_init_pud()
being stubbed out. Reverting c742199a fixes the crash.

Fixes: c742199a ("mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge")
Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 arch/arm64/mm/mmu.c     | 20 ++++++++------------
 arch/x86/mm/pgtable.c   | 34 +++++++++++++++-------------------
 include/linux/pgtable.h | 26 +-------------------------
 3 files changed, 24 insertions(+), 56 deletions(-)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index d745865084488..9ff0de1b2b93c 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -1339,7 +1339,6 @@ void *__init fixmap_remap_fdt(phys_addr_t dt_phys, int *size, pgprot_t prot)
 	return dt_virt;
 }
 
-#if CONFIG_PGTABLE_LEVELS > 3
 int pud_set_huge(pud_t *pudp, phys_addr_t phys, pgprot_t prot)
 {
 	pud_t new_pud = pfn_pud(__phys_to_pfn(phys), mk_pud_sect_prot(prot));
@@ -1354,16 +1353,6 @@ int pud_set_huge(pud_t *pudp, phys_addr_t phys, pgprot_t prot)
 	return 1;
 }
 
-int pud_clear_huge(pud_t *pudp)
-{
-	if (!pud_sect(READ_ONCE(*pudp)))
-		return 0;
-	pud_clear(pudp);
-	return 1;
-}
-#endif
-
-#if CONFIG_PGTABLE_LEVELS > 2
 int pmd_set_huge(pmd_t *pmdp, phys_addr_t phys, pgprot_t prot)
 {
 	pmd_t new_pmd = pfn_pmd(__phys_to_pfn(phys), mk_pmd_sect_prot(prot));
@@ -1378,6 +1367,14 @@ int pmd_set_huge(pmd_t *pmdp, phys_addr_t phys, pgprot_t prot)
 	return 1;
 }
 
+int pud_clear_huge(pud_t *pudp)
+{
+	if (!pud_sect(READ_ONCE(*pudp)))
+		return 0;
+	pud_clear(pudp);
+	return 1;
+}
+
 int pmd_clear_huge(pmd_t *pmdp)
 {
 	if (!pmd_sect(READ_ONCE(*pmdp)))
@@ -1385,7 +1382,6 @@ int pmd_clear_huge(pmd_t *pmdp)
 	pmd_clear(pmdp);
 	return 1;
 }
-#endif
 
 int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
 {
diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
index 3364fe62b9037..3481b35cb4ec7 100644
--- a/arch/x86/mm/pgtable.c
+++ b/arch/x86/mm/pgtable.c
@@ -682,7 +682,6 @@ int p4d_clear_huge(p4d_t *p4d)
 }
 #endif
 
-#if CONFIG_PGTABLE_LEVELS > 3
 /**
  * pud_set_huge - setup kernel PUD mapping
  *
@@ -721,23 +720,6 @@ int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
 	return 1;
 }
 
-/**
- * pud_clear_huge - clear kernel PUD mapping when it is set
- *
- * Returns 1 on success and 0 on failure (no PUD map is found).
- */
-int pud_clear_huge(pud_t *pud)
-{
-	if (pud_large(*pud)) {
-		pud_clear(pud);
-		return 1;
-	}
-
-	return 0;
-}
-#endif
-
-#if CONFIG_PGTABLE_LEVELS > 2
 /**
  * pmd_set_huge - setup kernel PMD mapping
  *
@@ -768,6 +750,21 @@ int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
 	return 1;
 }
 
+/**
+ * pud_clear_huge - clear kernel PUD mapping when it is set
+ *
+ * Returns 1 on success and 0 on failure (no PUD map is found).
+ */
+int pud_clear_huge(pud_t *pud)
+{
+	if (pud_large(*pud)) {
+		pud_clear(pud);
+		return 1;
+	}
+
+	return 0;
+}
+
 /**
  * pmd_clear_huge - clear kernel PMD mapping when it is set
  *
@@ -782,7 +779,6 @@ int pmd_clear_huge(pmd_t *pmd)
 
 	return 0;
 }
-#endif
 
 #ifdef CONFIG_X86_64
 /**
diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
index d147480cdefc7..e24d2c992b112 100644
--- a/include/linux/pgtable.h
+++ b/include/linux/pgtable.h
@@ -1397,34 +1397,10 @@ static inline int p4d_clear_huge(p4d_t *p4d)
 }
 #endif /* !__PAGETABLE_P4D_FOLDED */
 
-#ifndef __PAGETABLE_PUD_FOLDED
 int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot);
-int pud_clear_huge(pud_t *pud);
-#else
-static inline int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
-{
-	return 0;
-}
-static inline int pud_clear_huge(pud_t *pud)
-{
-	return 0;
-}
-#endif /* !__PAGETABLE_PUD_FOLDED */
-
-#ifndef __PAGETABLE_PMD_FOLDED
 int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot);
+int pud_clear_huge(pud_t *pud);
 int pmd_clear_huge(pmd_t *pmd);
-#else
-static inline int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
-{
-	return 0;
-}
-static inline int pmd_clear_huge(pmd_t *pmd)
-{
-	return 0;
-}
-#endif /* !__PAGETABLE_PMD_FOLDED */
-
 int p4d_free_pud_page(p4d_t *p4d, unsigned long addr);
 int pud_free_pmd_page(pud_t *pud, unsigned long addr);
 int pmd_free_pte_page(pmd_t *pmd, unsigned long addr);
-- 
2.26.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Revert "mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge"

[Christophe Leroy]

Jonathan Marek <jonathan@marek.ca> a écrit :

> c742199a breaks arm64 for some configs because it stubs out functions which
> should not have been stubbed out.
>
> With 4K pages and ARM64_VA_BITS_39=y, the kernel crashes early on unmapped
> 1G pages in the linear map caused by pud_set_huge() in alloc_init_pud()
> being stubbed out. Reverting c742199a fixes the crash.

This patch is there for a reason. Reverting it will break some other config.

The fix needs to allow it work with all platforms.

I don't understand, why does arm64 needs these PUD helpers when it  
defines __PAGETABLE_PUD_FOLDED ?

Maybe the fix should simply be to ifdef on __PAGETABLE_PUD_FOLDED  
instead of using CONFIG_PGTABLE_LEVELS ?

Thanks
Christophe

>
> Fixes: c742199a ("mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge")
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>  arch/arm64/mm/mmu.c     | 20 ++++++++------------
>  arch/x86/mm/pgtable.c   | 34 +++++++++++++++-------------------
>  include/linux/pgtable.h | 26 +-------------------------
>  3 files changed, 24 insertions(+), 56 deletions(-)
>
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index d745865084488..9ff0de1b2b93c 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1339,7 +1339,6 @@ void *__init fixmap_remap_fdt(phys_addr_t  
> dt_phys, int *size, pgprot_t prot)
>  	return dt_virt;
>  }
>
> -#if CONFIG_PGTABLE_LEVELS > 3
>  int pud_set_huge(pud_t *pudp, phys_addr_t phys, pgprot_t prot)
>  {
>  	pud_t new_pud = pfn_pud(__phys_to_pfn(phys), mk_pud_sect_prot(prot));
> @@ -1354,16 +1353,6 @@ int pud_set_huge(pud_t *pudp, phys_addr_t  
> phys, pgprot_t prot)
>  	return 1;
>  }
>
> -int pud_clear_huge(pud_t *pudp)
> -{
> -	if (!pud_sect(READ_ONCE(*pudp)))
> -		return 0;
> -	pud_clear(pudp);
> -	return 1;
> -}
> -#endif
> -
> -#if CONFIG_PGTABLE_LEVELS > 2
>  int pmd_set_huge(pmd_t *pmdp, phys_addr_t phys, pgprot_t prot)
>  {
>  	pmd_t new_pmd = pfn_pmd(__phys_to_pfn(phys), mk_pmd_sect_prot(prot));
> @@ -1378,6 +1367,14 @@ int pmd_set_huge(pmd_t *pmdp, phys_addr_t  
> phys, pgprot_t prot)
>  	return 1;
>  }
>
> +int pud_clear_huge(pud_t *pudp)
> +{
> +	if (!pud_sect(READ_ONCE(*pudp)))
> +		return 0;
> +	pud_clear(pudp);
> +	return 1;
> +}
> +
>  int pmd_clear_huge(pmd_t *pmdp)
>  {
>  	if (!pmd_sect(READ_ONCE(*pmdp)))
> @@ -1385,7 +1382,6 @@ int pmd_clear_huge(pmd_t *pmdp)
>  	pmd_clear(pmdp);
>  	return 1;
>  }
> -#endif
>
>  int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
>  {
> diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
> index 3364fe62b9037..3481b35cb4ec7 100644
> --- a/arch/x86/mm/pgtable.c
> +++ b/arch/x86/mm/pgtable.c
> @@ -682,7 +682,6 @@ int p4d_clear_huge(p4d_t *p4d)
>  }
>  #endif
>
> -#if CONFIG_PGTABLE_LEVELS > 3
>  /**
>   * pud_set_huge - setup kernel PUD mapping
>   *
> @@ -721,23 +720,6 @@ int pud_set_huge(pud_t *pud, phys_addr_t addr,  
> pgprot_t prot)
>  	return 1;
>  }
>
> -/**
> - * pud_clear_huge - clear kernel PUD mapping when it is set
> - *
> - * Returns 1 on success and 0 on failure (no PUD map is found).
> - */
> -int pud_clear_huge(pud_t *pud)
> -{
> -	if (pud_large(*pud)) {
> -		pud_clear(pud);
> -		return 1;
> -	}
> -
> -	return 0;
> -}
> -#endif
> -
> -#if CONFIG_PGTABLE_LEVELS > 2
>  /**
>   * pmd_set_huge - setup kernel PMD mapping
>   *
> @@ -768,6 +750,21 @@ int pmd_set_huge(pmd_t *pmd, phys_addr_t addr,  
> pgprot_t prot)
>  	return 1;
>  }
>
> +/**
> + * pud_clear_huge - clear kernel PUD mapping when it is set
> + *
> + * Returns 1 on success and 0 on failure (no PUD map is found).
> + */
> +int pud_clear_huge(pud_t *pud)
> +{
> +	if (pud_large(*pud)) {
> +		pud_clear(pud);
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * pmd_clear_huge - clear kernel PMD mapping when it is set
>   *
> @@ -782,7 +779,6 @@ int pmd_clear_huge(pmd_t *pmd)
>
>  	return 0;
>  }
> -#endif
>
>  #ifdef CONFIG_X86_64
>  /**
> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> index d147480cdefc7..e24d2c992b112 100644
> --- a/include/linux/pgtable.h
> +++ b/include/linux/pgtable.h
> @@ -1397,34 +1397,10 @@ static inline int p4d_clear_huge(p4d_t *p4d)
>  }
>  #endif /* !__PAGETABLE_P4D_FOLDED */
>
> -#ifndef __PAGETABLE_PUD_FOLDED
>  int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot);
> -int pud_clear_huge(pud_t *pud);
> -#else
> -static inline int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
> -{
> -	return 0;
> -}
> -static inline int pud_clear_huge(pud_t *pud)
> -{
> -	return 0;
> -}
> -#endif /* !__PAGETABLE_PUD_FOLDED */
> -
> -#ifndef __PAGETABLE_PMD_FOLDED
>  int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot);
> +int pud_clear_huge(pud_t *pud);
>  int pmd_clear_huge(pmd_t *pmd);
> -#else
> -static inline int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
> -{
> -	return 0;
> -}
> -static inline int pmd_clear_huge(pmd_t *pmd)
> -{
> -	return 0;
> -}
> -#endif /* !__PAGETABLE_PMD_FOLDED */
> -
>  int p4d_free_pud_page(p4d_t *p4d, unsigned long addr);
>  int pud_free_pmd_page(pud_t *pud, unsigned long addr);
>  int pmd_free_pte_page(pmd_t *pmd, unsigned long addr);
> --
> 2.26.1



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Revert "mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge"

[Will Deacon]

On Sat, Jul 17, 2021 at 06:31:08PM +0200, Christophe Leroy wrote:
> Jonathan Marek <jonathan@marek.ca> a écrit :
> 
> > c742199a breaks arm64 for some configs because it stubs out functions which
> > should not have been stubbed out.
> > 
> > With 4K pages and ARM64_VA_BITS_39=y, the kernel crashes early on unmapped
> > 1G pages in the linear map caused by pud_set_huge() in alloc_init_pud()
> > being stubbed out. Reverting c742199a fixes the crash.
> 
> This patch is there for a reason. Reverting it will break some other config.

Which config? Not reverting it breaks arm64.

> The fix needs to allow it work with all platforms.

Hmm, there was already a report that selftests were failing with this
change:

https://lore.kernel.org/linux-arm-kernel/CAMuHMdXShORDox-xxaeUfDW3wx2PeggFSqhVSHVZNKCGK-y_vQ@mail.gmail.com/

That was a week ago and doesn't seem to have progressed, so I'm inclined to
revert this if it's not resolved this week as we usually use -rc3 as a base
for queuing patches for the next release.

> I don't understand, why does arm64 needs these PUD helpers when it defines
> __PAGETABLE_PUD_FOLDED ?

Probably for the huge vmap code; see arch_vmap_pXd_supported(). That also
lines up with the failing selftests afaict.

For 4k pages with 3 levels of walk, we want to be able to use 1GB mappings
at level 1 (pgd), 2MB mappings at level 2 (pmd) as well as the usual 4k
page mappings at level 3 (pte).

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Revert "mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge"

[Ard Biesheuvel]

On Mon, 19 Jul 2021 at 12:49, Will Deacon <will@kernel.org> wrote:
>
> On Sat, Jul 17, 2021 at 06:31:08PM +0200, Christophe Leroy wrote:
> > Jonathan Marek <jonathan@marek.ca> a écrit :
> >
> > > c742199a breaks arm64 for some configs because it stubs out functions which
> > > should not have been stubbed out.
> > >
> > > With 4K pages and ARM64_VA_BITS_39=y, the kernel crashes early on unmapped
> > > 1G pages in the linear map caused by pud_set_huge() in alloc_init_pud()
> > > being stubbed out. Reverting c742199a fixes the crash.
> >
> > This patch is there for a reason. Reverting it will break some other config.
>
> Which config? Not reverting it breaks arm64.
>
> > The fix needs to allow it work with all platforms.
>
> Hmm, there was already a report that selftests were failing with this
> change:
>
> https://lore.kernel.org/linux-arm-kernel/CAMuHMdXShORDox-xxaeUfDW3wx2PeggFSqhVSHVZNKCGK-y_vQ@mail.gmail.com/
>
> That was a week ago and doesn't seem to have progressed, so I'm inclined to
> revert this if it's not resolved this week as we usually use -rc3 as a base
> for queuing patches for the next release.
>
> > I don't understand, why does arm64 needs these PUD helpers when it defines
> > __PAGETABLE_PUD_FOLDED ?

So if I am not mistaken, you modified arch/arm64 MMU code without
understanding it and without cc'ing any of the arm64 maintainers,
ignored reports that they were causing problems, and now you are
objecting to them being reverted because it break 'some other config'?
I don't think that is entirely reasonable, and the fact that the
maintainers weren't even cc'ed on the patch is enough justification to
simply revert it. IMHO.

>
> Probably for the huge vmap code; see arch_vmap_pXd_supported(). That also
> lines up with the failing selftests afaict.
>

The patch actually explains it: alloc_init_pud() in the swapper init
code uses it to lay down 1 GB block mappings for the linear map. That
code could quite easily be updated to work around this, but I guess it
would be better to fix this more comprehensively.

> For 4k pages with 3 levels of walk, we want to be able to use 1GB mappings
> at level 1 (pgd), 2MB mappings at level 2 (pmd) as well as the usual 4k
> page mappings at level 3 (pte).
>

Yes, we appear to use PUDs for 4k pages kernel regardless of whether
they are folded into PGDs to refer to level 1/1GB block mappings.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Revert "mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge"

[Ard Biesheuvel]

(add some folks back to cc:)

On Mon, 19 Jul 2021 at 15:58, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Mon, 19 Jul 2021 at 12:49, Will Deacon <will@kernel.org> wrote:
> >
> > On Sat, Jul 17, 2021 at 06:31:08PM +0200, Christophe Leroy wrote:
> > > Jonathan Marek <jonathan@marek.ca> a écrit :
> > >
> > > > c742199a breaks arm64 for some configs because it stubs out functions which
> > > > should not have been stubbed out.
> > > >
> > > > With 4K pages and ARM64_VA_BITS_39=y, the kernel crashes early on unmapped
> > > > 1G pages in the linear map caused by pud_set_huge() in alloc_init_pud()
> > > > being stubbed out. Reverting c742199a fixes the crash.
> > >
> > > This patch is there for a reason. Reverting it will break some other config.
> >
> > Which config? Not reverting it breaks arm64.
> >
> > > The fix needs to allow it work with all platforms.
> >
> > Hmm, there was already a report that selftests were failing with this
> > change:
> >
> > https://lore.kernel.org/linux-arm-kernel/CAMuHMdXShORDox-xxaeUfDW3wx2PeggFSqhVSHVZNKCGK-y_vQ@mail.gmail.com/
> >
> > That was a week ago and doesn't seem to have progressed, so I'm inclined to
> > revert this if it's not resolved this week as we usually use -rc3 as a base
> > for queuing patches for the next release.
> >
> > > I don't understand, why does arm64 needs these PUD helpers when it defines
> > > __PAGETABLE_PUD_FOLDED ?
>
> So if I am not mistaken, you modified arch/arm64 MMU code without
> understanding it and without cc'ing any of the arm64 maintainers,
> ignored reports that they were causing problems, and now you are
> objecting to them being reverted because it break 'some other config'?
> I don't think that is entirely reasonable, and the fact that the
> maintainers weren't even cc'ed on the patch is enough justification to
> simply revert it. IMHO.
>
> >
> > Probably for the huge vmap code; see arch_vmap_pXd_supported(). That also
> > lines up with the failing selftests afaict.
> >
>
> The patch actually explains it: alloc_init_pud() in the swapper init
> code uses it to lay down 1 GB block mappings for the linear map. That
> code could quite easily be updated to work around this, but I guess it
> would be better to fix this more comprehensively.
>
> > For 4k pages with 3 levels of walk, we want to be able to use 1GB mappings
> > at level 1 (pgd), 2MB mappings at level 2 (pmd) as well as the usual 4k
> > page mappings at level 3 (pte).
> >
>
> Yes, we appear to use PUDs for 4k pages kernel regardless of whether
> they are folded into PGDs to refer to level 1/1GB block mappings.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Revert "mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge"

[Christophe Leroy]

Ard Biesheuvel <ardb@kernel.org> a écrit :

> (add some folks back to cc:)
>
> On Mon, 19 Jul 2021 at 15:58, Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Mon, 19 Jul 2021 at 12:49, Will Deacon <will@kernel.org> wrote:
>> >
>> > On Sat, Jul 17, 2021 at 06:31:08PM +0200, Christophe Leroy wrote:
>> > > Jonathan Marek <jonathan@marek.ca> a écrit :
>> > >
>> > > > c742199a breaks arm64 for some configs because it stubs out  
>> functions which
>> > > > should not have been stubbed out.
>> > > >
>> > > > With 4K pages and ARM64_VA_BITS_39=y, the kernel crashes  
>> early on unmapped
>> > > > 1G pages in the linear map caused by pud_set_huge() in  
>> alloc_init_pud()
>> > > > being stubbed out. Reverting c742199a fixes the crash.
>> > >
>> > > This patch is there for a reason. Reverting it will break some  
>> other config.
>> >
>> > Which config? Not reverting it breaks arm64.
>> >
>> > > The fix needs to allow it work with all platforms.
>> >
>> > Hmm, there was already a report that selftests were failing with this
>> > change:
>> >
>> >  
>> https://lore.kernel.org/linux-arm-kernel/CAMuHMdXShORDox-xxaeUfDW3wx2PeggFSqhVSHVZNKCGK-y_vQ@mail.gmail.com/
>> >
>> > That was a week ago and doesn't seem to have progressed, so I'm  
>> inclined to
>> > revert this if it's not resolved this week as we usually use -rc3  
>> as a base
>> > for queuing patches for the next release.
>> >
>> > > I don't understand, why does arm64 needs these PUD helpers when  
>> it defines
>> > > __PAGETABLE_PUD_FOLDED ?
>>
>> So if I am not mistaken, you modified arch/arm64 MMU code without
>> understanding it and without cc'ing any of the arm64 maintainers,
>> ignored reports that they were causing problems, and now you are
>> objecting to them being reverted because it break 'some other config'?
>> I don't think that is entirely reasonable, and the fact that the
>> maintainers weren't even cc'ed on the patch is enough justification to
>> simply revert it. IMHO.

arm maintainers were in copy, see  
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/73ec95f40cafbbb69bdfb43a7f53876fd845b0ce.1620990479.git.christophe.leroy@csgroup.eu/

I don't know what report you mention, I m only aware of one that I got  
after the start of my holidays and that I plan to handle first week of  
August when I'm back at work.

As I answered to Will, now that we are aware of an issue on Arm64 we  
need to work together and find a fix that works for all.


>>
>> >
>> > Probably for the huge vmap code; see arch_vmap_pXd_supported(). That also
>> > lines up with the failing selftests afaict.
>> >
>>
>> The patch actually explains it: alloc_init_pud() in the swapper init
>> code uses it to lay down 1 GB block mappings for the linear map. That
>> code could quite easily be updated to work around this, but I guess it
>> would be better to fix this more comprehensively.
>>
>> > For 4k pages with 3 levels of walk, we want to be able to use 1GB mappings
>> > at level 1 (pgd), 2MB mappings at level 2 (pmd) as well as the usual 4k
>> > page mappings at level 3 (pte).
>> >
>>
>> Yes, we appear to use PUDs for 4k pages kernel regardless of whether
>> they are folded into PGDs to refer to level 1/1GB block mappings.

And that's probably the source of the misunderstanding, because a 3  
level architecture is supposed to only have pgd, pmd and pud.

Could the problematic arm pud_clear_huge and pud_set_huge be renamed  
pgd_clear_huge and pgd_set_huge ?

Christophe


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Revert "mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge"

[Ard Biesheuvel]

On Mon, 19 Jul 2021 at 17:45, Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
> Ard Biesheuvel <ardb@kernel.org> a écrit :
>
> > (add some folks back to cc:)
> >
> > On Mon, 19 Jul 2021 at 15:58, Ard Biesheuvel <ardb@kernel.org> wrote:
> >>
> >> On Mon, 19 Jul 2021 at 12:49, Will Deacon <will@kernel.org> wrote:
> >> >
> >> > On Sat, Jul 17, 2021 at 06:31:08PM +0200, Christophe Leroy wrote:
> >> > > Jonathan Marek <jonathan@marek.ca> a écrit :
> >> > >
> >> > > > c742199a breaks arm64 for some configs because it stubs out
> >> functions which
> >> > > > should not have been stubbed out.
> >> > > >
> >> > > > With 4K pages and ARM64_VA_BITS_39=y, the kernel crashes
> >> early on unmapped
> >> > > > 1G pages in the linear map caused by pud_set_huge() in
> >> alloc_init_pud()
> >> > > > being stubbed out. Reverting c742199a fixes the crash.
> >> > >
> >> > > This patch is there for a reason. Reverting it will break some
> >> other config.
> >> >
> >> > Which config? Not reverting it breaks arm64.
> >> >
> >> > > The fix needs to allow it work with all platforms.
> >> >
> >> > Hmm, there was already a report that selftests were failing with this
> >> > change:
> >> >
> >> >
> >> https://lore.kernel.org/linux-arm-kernel/CAMuHMdXShORDox-xxaeUfDW3wx2PeggFSqhVSHVZNKCGK-y_vQ@mail.gmail.com/
> >> >
> >> > That was a week ago and doesn't seem to have progressed, so I'm
> >> inclined to
> >> > revert this if it's not resolved this week as we usually use -rc3
> >> as a base
> >> > for queuing patches for the next release.
> >> >
> >> > > I don't understand, why does arm64 needs these PUD helpers when
> >> it defines
> >> > > __PAGETABLE_PUD_FOLDED ?
> >>
> >> So if I am not mistaken, you modified arch/arm64 MMU code without
> >> understanding it and without cc'ing any of the arm64 maintainers,
> >> ignored reports that they were causing problems, and now you are
> >> objecting to them being reverted because it break 'some other config'?
> >> I don't think that is entirely reasonable, and the fact that the
> >> maintainers weren't even cc'ed on the patch is enough justification to
> >> simply revert it. IMHO.
>
> arm maintainers were in copy, see
> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/73ec95f40cafbbb69bdfb43a7f53876fd845b0ce.1620990479.git.christophe.leroy@csgroup.eu/
>

I don't see Will or Catalin cc'ed on that patch, so no, the arm64
maintainers were not in copy. cc'ing a mailing list is not sufficient.

> I don't know what report you mention, I m only aware of one that I got
> after the start of my holidays and that I plan to handle first week of
> August when I'm back at work.
>

The one that Will just referred to:
https://lore.kernel.org/linux-arm-kernel/CAMuHMdXShORDox-xxaeUfDW3wx2PeggFSqhVSHVZNKCGK-y_vQ@mail.gmail.com/

> As I answered to Will, now that we are aware of an issue on Arm64 we
> need to work together and find a fix that works for all.
>

Sure. As long as arm64 gets fixed (and note that 39-bit VA/4k pages is
a very widely used config)


>
> >>
> >> >
> >> > Probably for the huge vmap code; see arch_vmap_pXd_supported(). That also
> >> > lines up with the failing selftests afaict.
> >> >
> >>
> >> The patch actually explains it: alloc_init_pud() in the swapper init
> >> code uses it to lay down 1 GB block mappings for the linear map. That
> >> code could quite easily be updated to work around this, but I guess it
> >> would be better to fix this more comprehensively.
> >>
> >> > For 4k pages with 3 levels of walk, we want to be able to use 1GB mappings
> >> > at level 1 (pgd), 2MB mappings at level 2 (pmd) as well as the usual 4k
> >> > page mappings at level 3 (pte).
> >> >
> >>
> >> Yes, we appear to use PUDs for 4k pages kernel regardless of whether
> >> they are folded into PGDs to refer to level 1/1GB block mappings.
>
> And that's probably the source of the misunderstanding, because a 3
> level architecture is supposed to only have pgd, pmd and pud.
>

Is this requirement documented somewhere? Having folded PUDs does not
necessarily mean that PUDs don't exist, but simply that PUDs and PGDs
are the same.


> Could the problematic arm pud_clear_huge and pud_set_huge be renamed
> pgd_clear_huge and pgd_set_huge ?
>

No, that would break 48-bit VA (4 level paging) configurations, as in
that case, PGDs and PUDs are different levels/sizes.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Revert "mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge"

[Christophe Leroy]

Ard Biesheuvel <ardb@kernel.org> a écrit :

> On Mon, 19 Jul 2021 at 17:45, Christophe Leroy
> <christophe.leroy@csgroup.eu> wrote:
>>
>> Ard Biesheuvel <ardb@kernel.org> a écrit :
>>
>> > (add some folks back to cc:)
>> >
>> > On Mon, 19 Jul 2021 at 15:58, Ard Biesheuvel <ardb@kernel.org> wrote:
>> >>
>> >> On Mon, 19 Jul 2021 at 12:49, Will Deacon <will@kernel.org> wrote:
>> >> >
>> >> > On Sat, Jul 17, 2021 at 06:31:08PM +0200, Christophe Leroy wrote:
>> >> > > Jonathan Marek <jonathan@marek.ca> a écrit :
>> >> > >
>> >> > > > c742199a breaks arm64 for some configs because it stubs out
>> >> functions which
>> >> > > > should not have been stubbed out.
>> >> > > >
>> >> > > > With 4K pages and ARM64_VA_BITS_39=y, the kernel crashes
>> >> early on unmapped
>> >> > > > 1G pages in the linear map caused by pud_set_huge() in
>> >> alloc_init_pud()
>> >> > > > being stubbed out. Reverting c742199a fixes the crash.
>> >> > >
>> >> > > This patch is there for a reason. Reverting it will break some
>> >> other config.
>> >> >
>> >> > Which config? Not reverting it breaks arm64.
>> >> >
>> >> > > The fix needs to allow it work with all platforms.
>> >> >
>> >> > Hmm, there was already a report that selftests were failing with this
>> >> > change:
>> >> >
>> >> >
>> >>  
>> https://lore.kernel.org/linux-arm-kernel/CAMuHMdXShORDox-xxaeUfDW3wx2PeggFSqhVSHVZNKCGK-y_vQ@mail.gmail.com/
>> >> >
>> >> > That was a week ago and doesn't seem to have progressed, so I'm
>> >> inclined to
>> >> > revert this if it's not resolved this week as we usually use -rc3
>> >> as a base
>> >> > for queuing patches for the next release.
>> >> >
>> >> > > I don't understand, why does arm64 needs these PUD helpers when
>> >> it defines
>> >> > > __PAGETABLE_PUD_FOLDED ?
>> >>
>> >> So if I am not mistaken, you modified arch/arm64 MMU code without
>> >> understanding it and without cc'ing any of the arm64 maintainers,
>> >> ignored reports that they were causing problems, and now you are
>> >> objecting to them being reverted because it break 'some other config'?
>> >> I don't think that is entirely reasonable, and the fact that the
>> >> maintainers weren't even cc'ed on the patch is enough justification to
>> >> simply revert it. IMHO.
>>
>> arm maintainers were in copy, see
>> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/73ec95f40cafbbb69bdfb43a7f53876fd845b0ce.1620990479.git.christophe.leroy@csgroup.eu/
>>
>
> I don't see Will or Catalin cc'ed on that patch, so no, the arm64
> maintainers were not in copy. cc'ing a mailing list is not sufficient.
>
>> I don't know what report you mention, I m only aware of one that I got
>> after the start of my holidays and that I plan to handle first week of
>> August when I'm back at work.
>>
>
> The one that Will just referred to:
> https://lore.kernel.org/linux-arm-kernel/CAMuHMdXShORDox-xxaeUfDW3wx2PeggFSqhVSHVZNKCGK-y_vQ@mail.gmail.com/

Yes it is that one.

>
>> As I answered to Will, now that we are aware of an issue on Arm64 we
>> need to work together and find a fix that works for all.
>>
>
> Sure. As long as arm64 gets fixed (and note that 39-bit VA/4k pages is
> a very widely used config)
>
>
>>
>> >>
>> >> >
>> >> > Probably for the huge vmap code; see  
>> arch_vmap_pXd_supported(). That also
>> >> > lines up with the failing selftests afaict.
>> >> >
>> >>
>> >> The patch actually explains it: alloc_init_pud() in the swapper init
>> >> code uses it to lay down 1 GB block mappings for the linear map. That
>> >> code could quite easily be updated to work around this, but I guess it
>> >> would be better to fix this more comprehensively.
>> >>
>> >> > For 4k pages with 3 levels of walk, we want to be able to use  
>> 1GB mappings
>> >> > at level 1 (pgd), 2MB mappings at level 2 (pmd) as well as the usual 4k
>> >> > page mappings at level 3 (pte).
>> >> >
>> >>
>> >> Yes, we appear to use PUDs for 4k pages kernel regardless of whether
>> >> they are folded into PGDs to refer to level 1/1GB block mappings.
>>
>> And that's probably the source of the misunderstanding, because a 3
>> level architecture is supposed to only have pgd, pmd and pud.
>>
>
> Is this requirement documented somewhere? Having folded PUDs does not
> necessarily mean that PUDs don't exist, but simply that PUDs and PGDs
> are the same.

If you look at linux/pgtable.h you can see that p4d_set_huge and  
p4d_clear_huge are only for 5 level MMUs. Why shouldn't the same  
principle apply to PUD and PMD ?


>
>
>> Could the problematic arm pud_clear_huge and pud_set_huge be renamed
>> pgd_clear_huge and pgd_set_huge ?
>>
>
> No, that would break 48-bit VA (4 level paging) configurations, as in
> that case, PGDs and PUDs are different levels/sizes.


I meant for 3 level paging only. Of course for 4 level it must apply  
on the PUD.



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Revert "mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge"

[Will Deacon]

On Mon, Jul 19, 2021 at 06:58:43PM +0200, Christophe Leroy wrote:
> Ard Biesheuvel <ardb@kernel.org> a écrit :
> > On Mon, 19 Jul 2021 at 17:45, Christophe Leroy
> > <christophe.leroy@csgroup.eu> wrote:
> > > Ard Biesheuvel <ardb@kernel.org> a écrit :
> > > > On Mon, 19 Jul 2021 at 15:58, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >> On Mon, 19 Jul 2021 at 12:49, Will Deacon <will@kernel.org> wrote:
> > > >> > For 4k pages with 3 levels of walk, we want to be able to use
> > > 1GB mappings
> > > >> > at level 1 (pgd), 2MB mappings at level 2 (pmd) as well as the usual 4k
> > > >> > page mappings at level 3 (pte).
> > > >> >
> > > >>
> > > >> Yes, we appear to use PUDs for 4k pages kernel regardless of whether
> > > >> they are folded into PGDs to refer to level 1/1GB block mappings.
> > > 
> > > And that's probably the source of the misunderstanding, because a 3
> > > level architecture is supposed to only have pgd, pmd and pud.
> > > 
> > 
> > Is this requirement documented somewhere? Having folded PUDs does not
> > necessarily mean that PUDs don't exist, but simply that PUDs and PGDs
> > are the same.
> 
> If you look at linux/pgtable.h you can see that p4d_set_huge and
> p4d_clear_huge are only for 5 level MMUs. Why shouldn't the same principle
> apply to PUD and PMD ?

Prior to your patch, you could make the opposite argument. Why are
p4x_{set_clear}_huge() guarded? Those were introduced after the others
as well.

Reverting looks easiest for now. Let's do that so we can figure this ou
properly without breaking mainline in the meantime. That also means you
can enjoy your holiday without looking at email on your phone!

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Revert "mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge"

[Christophe Leroy]

Will Deacon <will@kernel.org> a écrit :

> On Mon, Jul 19, 2021 at 06:58:43PM +0200, Christophe Leroy wrote:
>> Ard Biesheuvel <ardb@kernel.org> a écrit :
>> > On Mon, 19 Jul 2021 at 17:45, Christophe Leroy
>> > <christophe.leroy@csgroup.eu> wrote:
>> > > Ard Biesheuvel <ardb@kernel.org> a écrit :
>> > > > On Mon, 19 Jul 2021 at 15:58, Ard Biesheuvel <ardb@kernel.org> wrote:
>> > > >> On Mon, 19 Jul 2021 at 12:49, Will Deacon <will@kernel.org> wrote:
>> > > >> > For 4k pages with 3 levels of walk, we want to be able to use
>> > > 1GB mappings
>> > > >> > at level 1 (pgd), 2MB mappings at level 2 (pmd) as well as  
>> the usual 4k
>> > > >> > page mappings at level 3 (pte).
>> > > >> >
>> > > >>
>> > > >> Yes, we appear to use PUDs for 4k pages kernel regardless of whether
>> > > >> they are folded into PGDs to refer to level 1/1GB block mappings.
>> > >
>> > > And that's probably the source of the misunderstanding, because a 3
>> > > level architecture is supposed to only have pgd, pmd and pud.
>> > >
>> >
>> > Is this requirement documented somewhere? Having folded PUDs does not
>> > necessarily mean that PUDs don't exist, but simply that PUDs and PGDs
>> > are the same.
>>
>> If you look at linux/pgtable.h you can see that p4d_set_huge and
>> p4d_clear_huge are only for 5 level MMUs. Why shouldn't the same principle
>> apply to PUD and PMD ?
>
> Prior to your patch, you could make the opposite argument. Why are
> p4x_{set_clear}_huge() guarded? Those were introduced after the others
> as well.
>
> Reverting looks easiest for now. Let's do that so we can figure this ou
> properly without breaking mainline in the meantime. That also means you
> can enjoy your holiday without looking at email on your phone!

No need to be that extreme. See my comment to the revert patch, all  
you need to do is to add the two missing functions in 8xx.c as part of  
the patch reverting the add of the stubs in linux/pgtable.h

Christophe




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Revert "mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge"

[LEROY Christophe]

Le 20/07/2021 à 13:27, Will Deacon a écrit :
> On Mon, Jul 19, 2021 at 06:58:43PM +0200, Christophe Leroy wrote:
>> Ard Biesheuvel <ardb@kernel.org> a écrit :
>>> On Mon, 19 Jul 2021 at 17:45, Christophe Leroy
>>> <christophe.leroy@csgroup.eu> wrote:
>>>> Ard Biesheuvel <ardb@kernel.org> a écrit :
>>>>> On Mon, 19 Jul 2021 at 15:58, Ard Biesheuvel <ardb@kernel.org> wrote:
>>>>>> On Mon, 19 Jul 2021 at 12:49, Will Deacon <will@kernel.org> wrote:
>>>>>>> For 4k pages with 3 levels of walk, we want to be able to use
>>>> 1GB mappings
>>>>>>> at level 1 (pgd), 2MB mappings at level 2 (pmd) as well as the usual 4k
>>>>>>> page mappings at level 3 (pte).
>>>>>>>
>>>>>>
>>>>>> Yes, we appear to use PUDs for 4k pages kernel regardless of whether
>>>>>> they are folded into PGDs to refer to level 1/1GB block mappings.
>>>>
>>>> And that's probably the source of the misunderstanding, because a 3
>>>> level architecture is supposed to only have pgd, pmd and pud.
>>>>
>>>
>>> Is this requirement documented somewhere? Having folded PUDs does not
>>> necessarily mean that PUDs don't exist, but simply that PUDs and PGDs
>>> are the same.
>>
>> If you look at linux/pgtable.h you can see that p4d_set_huge and
>> p4d_clear_huge are only for 5 level MMUs. Why shouldn't the same principle
>> apply to PUD and PMD ?
> 
> Prior to your patch, you could make the opposite argument. Why are
> p4x_{set_clear}_huge() guarded? Those were introduced after the others
> as well.
> 

Why are they guarded ? Probably because when they aren't, with ARM64 
defconfig we get:

   LD      vmlinux.o
   MODPOST vmlinux.symvers
   MODINFO modules.builtin.modinfo
   GEN     modules.builtin
   LD      .tmp_vmlinux.kallsyms1
aarch64-linux-gnu-ld: Unexpected GOT/PLT entries detected!
aarch64-linux-gnu-ld: Unexpected run-time procedure linkages detected!
aarch64-linux-gnu-ld: mm/vmalloc.o: in function `vunmap_p4d_range':
/home/chleroy/linux-powerpc/mm/vmalloc.c:385: undefined reference to 
`p4d_clear_huge'
aarch64-linux-gnu-ld: /home/chleroy/linux-powerpc/mm/vmalloc.c:385: 
undefined reference to `p4d_clear_huge'
make: *** [Makefile:1177: vmlinux] Error 1


Christophe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Revert "mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge"

[Christophe Leroy]

Will Deacon <will@kernel.org> a écrit :

> On Sat, Jul 17, 2021 at 06:31:08PM +0200, Christophe Leroy wrote:
>> Jonathan Marek <jonathan@marek.ca> a écrit :
>>
>> > c742199a breaks arm64 for some configs because it stubs out  
>> functions which
>> > should not have been stubbed out.
>> >
>> > With 4K pages and ARM64_VA_BITS_39=y, the kernel crashes early on unmapped
>> > 1G pages in the linear map caused by pud_set_huge() in alloc_init_pud()
>> > being stubbed out. Reverting c742199a fixes the crash.
>>
>> This patch is there for a reason. Reverting it will break some other config.
>
> Which config? Not reverting it breaks arm64.

Build will fail at least on powerpc 8xx without the stubs in linux/pgtable.h
That's the reason why I say that we need to find a common fix that  
works for both arm64 and powerpc

>
>> The fix needs to allow it work with all platforms.
>
> Hmm, there was already a report that selftests were failing with this
> change:
>
> https://lore.kernel.org/linux-arm-kernel/CAMuHMdXShORDox-xxaeUfDW3wx2PeggFSqhVSHVZNKCGK-y_vQ@mail.gmail.com/
>
> That was a week ago and doesn't seem to have progressed, so I'm inclined to
> revert this if it's not resolved this week as we usually use -rc3 as a base
> for queuing patches for the next release.

I have been on hollydays for 10 days and for another 2 weeks, with  
only my phone to play with. I pplan to look at it  first week of august.


>
>> I don't understand, why does arm64 needs these PUD helpers when it defines
>> __PAGETABLE_PUD_FOLDED ?
>
> Probably for the huge vmap code; see arch_vmap_pXd_supported(). That also
> lines up with the failing selftests afaict.
>
> For 4k pages with 3 levels of walk, we want to be able to use 1GB mappings
> at level 1 (pgd), 2MB mappings at level 2 (pmd) as well as the usual 4k
> page mappings at level 3 (pte).

The initial issue was a conflict between pud_clear_huge in arm64 and  
the generic stub I had introduced for configs having no PUD . We  
thought that it was a left over from config with 4 levels that was  
left unused for 3 levels config. It seems not. That's strange that the  
problem pops up only now, the change have been in the mm tree for one  
cycle.

Christophe

>
> Will



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Revert "mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge"

[Ard Biesheuvel]

On Sat, 17 Jul 2021 at 18:06, Jonathan Marek <jonathan@marek.ca> wrote:
>
> c742199a breaks arm64 for some configs because it stubs out functions which
> should not have been stubbed out.
>
> With 4K pages and ARM64_VA_BITS_39=y, the kernel crashes early on unmapped
> 1G pages in the linear map caused by pud_set_huge() in alloc_init_pud()
> being stubbed out. Reverting c742199a fixes the crash.
>
> Fixes: c742199a ("mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge")
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>

(replying here because some cc'ees were dropped from Christophe's
reply and the thread that followed it - unfortunately, lore does not
seem to have captured any of that discussion either)

Acked-by: Ard Biesheuvel <ardb@kernel.org>

> ---
>  arch/arm64/mm/mmu.c     | 20 ++++++++------------
>  arch/x86/mm/pgtable.c   | 34 +++++++++++++++-------------------
>  include/linux/pgtable.h | 26 +-------------------------
>  3 files changed, 24 insertions(+), 56 deletions(-)
>
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index d745865084488..9ff0de1b2b93c 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1339,7 +1339,6 @@ void *__init fixmap_remap_fdt(phys_addr_t dt_phys, int *size, pgprot_t prot)
>         return dt_virt;
>  }
>
> -#if CONFIG_PGTABLE_LEVELS > 3
>  int pud_set_huge(pud_t *pudp, phys_addr_t phys, pgprot_t prot)
>  {
>         pud_t new_pud = pfn_pud(__phys_to_pfn(phys), mk_pud_sect_prot(prot));
> @@ -1354,16 +1353,6 @@ int pud_set_huge(pud_t *pudp, phys_addr_t phys, pgprot_t prot)
>         return 1;
>  }
>
> -int pud_clear_huge(pud_t *pudp)
> -{
> -       if (!pud_sect(READ_ONCE(*pudp)))
> -               return 0;
> -       pud_clear(pudp);
> -       return 1;
> -}
> -#endif
> -
> -#if CONFIG_PGTABLE_LEVELS > 2
>  int pmd_set_huge(pmd_t *pmdp, phys_addr_t phys, pgprot_t prot)
>  {
>         pmd_t new_pmd = pfn_pmd(__phys_to_pfn(phys), mk_pmd_sect_prot(prot));
> @@ -1378,6 +1367,14 @@ int pmd_set_huge(pmd_t *pmdp, phys_addr_t phys, pgprot_t prot)
>         return 1;
>  }
>
> +int pud_clear_huge(pud_t *pudp)
> +{
> +       if (!pud_sect(READ_ONCE(*pudp)))
> +               return 0;
> +       pud_clear(pudp);
> +       return 1;
> +}
> +
>  int pmd_clear_huge(pmd_t *pmdp)
>  {
>         if (!pmd_sect(READ_ONCE(*pmdp)))
> @@ -1385,7 +1382,6 @@ int pmd_clear_huge(pmd_t *pmdp)
>         pmd_clear(pmdp);
>         return 1;
>  }
> -#endif
>
>  int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
>  {
> diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
> index 3364fe62b9037..3481b35cb4ec7 100644
> --- a/arch/x86/mm/pgtable.c
> +++ b/arch/x86/mm/pgtable.c
> @@ -682,7 +682,6 @@ int p4d_clear_huge(p4d_t *p4d)
>  }
>  #endif
>
> -#if CONFIG_PGTABLE_LEVELS > 3
>  /**
>   * pud_set_huge - setup kernel PUD mapping
>   *
> @@ -721,23 +720,6 @@ int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
>         return 1;
>  }
>
> -/**
> - * pud_clear_huge - clear kernel PUD mapping when it is set
> - *
> - * Returns 1 on success and 0 on failure (no PUD map is found).
> - */
> -int pud_clear_huge(pud_t *pud)
> -{
> -       if (pud_large(*pud)) {
> -               pud_clear(pud);
> -               return 1;
> -       }
> -
> -       return 0;
> -}
> -#endif
> -
> -#if CONFIG_PGTABLE_LEVELS > 2
>  /**
>   * pmd_set_huge - setup kernel PMD mapping
>   *
> @@ -768,6 +750,21 @@ int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
>         return 1;
>  }
>
> +/**
> + * pud_clear_huge - clear kernel PUD mapping when it is set
> + *
> + * Returns 1 on success and 0 on failure (no PUD map is found).
> + */
> +int pud_clear_huge(pud_t *pud)
> +{
> +       if (pud_large(*pud)) {
> +               pud_clear(pud);
> +               return 1;
> +       }
> +
> +       return 0;
> +}
> +
>  /**
>   * pmd_clear_huge - clear kernel PMD mapping when it is set
>   *
> @@ -782,7 +779,6 @@ int pmd_clear_huge(pmd_t *pmd)
>
>         return 0;
>  }
> -#endif
>
>  #ifdef CONFIG_X86_64
>  /**
> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> index d147480cdefc7..e24d2c992b112 100644
> --- a/include/linux/pgtable.h
> +++ b/include/linux/pgtable.h
> @@ -1397,34 +1397,10 @@ static inline int p4d_clear_huge(p4d_t *p4d)
>  }
>  #endif /* !__PAGETABLE_P4D_FOLDED */
>
> -#ifndef __PAGETABLE_PUD_FOLDED
>  int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot);
> -int pud_clear_huge(pud_t *pud);
> -#else
> -static inline int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
> -{
> -       return 0;
> -}
> -static inline int pud_clear_huge(pud_t *pud)
> -{
> -       return 0;
> -}
> -#endif /* !__PAGETABLE_PUD_FOLDED */
> -
> -#ifndef __PAGETABLE_PMD_FOLDED
>  int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot);
> +int pud_clear_huge(pud_t *pud);
>  int pmd_clear_huge(pmd_t *pmd);
> -#else
> -static inline int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
> -{
> -       return 0;
> -}
> -static inline int pmd_clear_huge(pmd_t *pmd)
> -{
> -       return 0;
> -}
> -#endif /* !__PAGETABLE_PMD_FOLDED */
> -
>  int p4d_free_pud_page(p4d_t *p4d, unsigned long addr);
>  int pud_free_pmd_page(pud_t *pud, unsigned long addr);
>  int pmd_free_pte_page(pmd_t *pmd, unsigned long addr);
> --
> 2.26.1
>

Re: [PATCH] Revert "mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge"

[Ard Biesheuvel]

On Sat, 17 Jul 2021 at 18:06, Jonathan Marek <jonathan@marek.ca> wrote:
>
> c742199a breaks arm64 for some configs because it stubs out functions which
> should not have been stubbed out.
>
> With 4K pages and ARM64_VA_BITS_39=y, the kernel crashes early on unmapped
> 1G pages in the linear map caused by pud_set_huge() in alloc_init_pud()
> being stubbed out. Reverting c742199a fixes the crash.
>
> Fixes: c742199a ("mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge")
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>

(replying here because some cc'ees were dropped from Christophe's
reply and the thread that followed it - unfortunately, lore does not
seem to have captured any of that discussion either)

Acked-by: Ard Biesheuvel <ardb@kernel.org>

> ---
>  arch/arm64/mm/mmu.c     | 20 ++++++++------------
>  arch/x86/mm/pgtable.c   | 34 +++++++++++++++-------------------
>  include/linux/pgtable.h | 26 +-------------------------
>  3 files changed, 24 insertions(+), 56 deletions(-)
>
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index d745865084488..9ff0de1b2b93c 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1339,7 +1339,6 @@ void *__init fixmap_remap_fdt(phys_addr_t dt_phys, int *size, pgprot_t prot)
>         return dt_virt;
>  }
>
> -#if CONFIG_PGTABLE_LEVELS > 3
>  int pud_set_huge(pud_t *pudp, phys_addr_t phys, pgprot_t prot)
>  {
>         pud_t new_pud = pfn_pud(__phys_to_pfn(phys), mk_pud_sect_prot(prot));
> @@ -1354,16 +1353,6 @@ int pud_set_huge(pud_t *pudp, phys_addr_t phys, pgprot_t prot)
>         return 1;
>  }
>
> -int pud_clear_huge(pud_t *pudp)
> -{
> -       if (!pud_sect(READ_ONCE(*pudp)))
> -               return 0;
> -       pud_clear(pudp);
> -       return 1;
> -}
> -#endif
> -
> -#if CONFIG_PGTABLE_LEVELS > 2
>  int pmd_set_huge(pmd_t *pmdp, phys_addr_t phys, pgprot_t prot)
>  {
>         pmd_t new_pmd = pfn_pmd(__phys_to_pfn(phys), mk_pmd_sect_prot(prot));
> @@ -1378,6 +1367,14 @@ int pmd_set_huge(pmd_t *pmdp, phys_addr_t phys, pgprot_t prot)
>         return 1;
>  }
>
> +int pud_clear_huge(pud_t *pudp)
> +{
> +       if (!pud_sect(READ_ONCE(*pudp)))
> +               return 0;
> +       pud_clear(pudp);
> +       return 1;
> +}
> +
>  int pmd_clear_huge(pmd_t *pmdp)
>  {
>         if (!pmd_sect(READ_ONCE(*pmdp)))
> @@ -1385,7 +1382,6 @@ int pmd_clear_huge(pmd_t *pmdp)
>         pmd_clear(pmdp);
>         return 1;
>  }
> -#endif
>
>  int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
>  {
> diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
> index 3364fe62b9037..3481b35cb4ec7 100644
> --- a/arch/x86/mm/pgtable.c
> +++ b/arch/x86/mm/pgtable.c
> @@ -682,7 +682,6 @@ int p4d_clear_huge(p4d_t *p4d)
>  }
>  #endif
>
> -#if CONFIG_PGTABLE_LEVELS > 3
>  /**
>   * pud_set_huge - setup kernel PUD mapping
>   *
> @@ -721,23 +720,6 @@ int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
>         return 1;
>  }
>
> -/**
> - * pud_clear_huge - clear kernel PUD mapping when it is set
> - *
> - * Returns 1 on success and 0 on failure (no PUD map is found).
> - */
> -int pud_clear_huge(pud_t *pud)
> -{
> -       if (pud_large(*pud)) {
> -               pud_clear(pud);
> -               return 1;
> -       }
> -
> -       return 0;
> -}
> -#endif
> -
> -#if CONFIG_PGTABLE_LEVELS > 2
>  /**
>   * pmd_set_huge - setup kernel PMD mapping
>   *
> @@ -768,6 +750,21 @@ int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
>         return 1;
>  }
>
> +/**
> + * pud_clear_huge - clear kernel PUD mapping when it is set
> + *
> + * Returns 1 on success and 0 on failure (no PUD map is found).
> + */
> +int pud_clear_huge(pud_t *pud)
> +{
> +       if (pud_large(*pud)) {
> +               pud_clear(pud);
> +               return 1;
> +       }
> +
> +       return 0;
> +}
> +
>  /**
>   * pmd_clear_huge - clear kernel PMD mapping when it is set
>   *
> @@ -782,7 +779,6 @@ int pmd_clear_huge(pmd_t *pmd)
>
>         return 0;
>  }
> -#endif
>
>  #ifdef CONFIG_X86_64
>  /**
> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> index d147480cdefc7..e24d2c992b112 100644
> --- a/include/linux/pgtable.h
> +++ b/include/linux/pgtable.h
> @@ -1397,34 +1397,10 @@ static inline int p4d_clear_huge(p4d_t *p4d)
>  }
>  #endif /* !__PAGETABLE_P4D_FOLDED */
>
> -#ifndef __PAGETABLE_PUD_FOLDED
>  int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot);
> -int pud_clear_huge(pud_t *pud);
> -#else
> -static inline int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
> -{
> -       return 0;
> -}
> -static inline int pud_clear_huge(pud_t *pud)
> -{
> -       return 0;
> -}
> -#endif /* !__PAGETABLE_PUD_FOLDED */
> -
> -#ifndef __PAGETABLE_PMD_FOLDED
>  int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot);
> +int pud_clear_huge(pud_t *pud);
>  int pmd_clear_huge(pmd_t *pmd);
> -#else
> -static inline int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
> -{
> -       return 0;
> -}
> -static inline int pmd_clear_huge(pmd_t *pmd)
> -{
> -       return 0;
> -}
> -#endif /* !__PAGETABLE_PMD_FOLDED */
> -
>  int p4d_free_pud_page(p4d_t *p4d, unsigned long addr);
>  int pud_free_pmd_page(pud_t *pud, unsigned long addr);
>  int pmd_free_pte_page(pmd_t *pmd, unsigned long addr);
> --
> 2.26.1
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel