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

PATCH In review Jonathan Marek 4 participants 13 messages Jul 17, 2021 – Aug 2, 2021

Subsystems (4)

x86 MM core arm64 x86 MM

lore

Share & tools

lore

Jonathan Marek

author

2021-07-17 16:06 UTC #0 lore raw

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

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 <[email protected]>

---
 arch/arm64/mm/mmu.c     | 20 ++++++++------------
 arch/x86/mm/pgtable.c   | 34 +++++++++++++++-------------------
 include/linux/pgtable.h | 26 +-------------------------
 3 files changed, 24 insertions(+), 56 deletions(-)

arch/arm64/mm/mmu.c +8 ~~-12~~

index d745865084488..9ff0de1b2b93c 100644
@@ -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)
 {

arch/x86/mm/pgtable.c +15 ~~-19~~

index 3364fe62b9037..3481b35cb4ec7 100644
@@ -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
 /**

include/linux/pgtable.h +1 ~~-25~~

index d147480cdefc7..e24d2c992b112 100644
@@ -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);

Signature · 1 line

2.26.1

Christophe Leroy

2021-07-17 16:28 UTC #1 lore raw

Jonathan Marek <[email protected]> a écrit :

Quoted 6 lines

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

Quoted 173 lines

Fixes c742199a ("mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge")

Signed-off-by Jonathan Marek <[email protected]>

---
 arch/arm64/mm/mmu.c     | 20 ++++++++------------
 arch/x86/mm/pgtable.c   | 34 +++++++++++++++-------------------
 include/linux/pgtable.h | 26 +-------------------------
 3 files changed, 24 insertions(+), 56 deletions(-)

arch/arm64/mm/mmu.c +0 -1

index d745865084488..9ff0de1b2b93c 100644
@@ -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)
 {

arch/arm64/mm/mmu.c +0 ~~-10~~

 	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)
 {

arch/arm64/mm/mmu.c +8 -0

 	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)
 {

arch/arm64/mm/mmu.c +0 -1

 	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)
 {

arch/x86/mm/pgtable.c +0 ~~-18~~

index 3364fe62b9037..3481b35cb4ec7 100644
@@ -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

arch/x86/mm/pgtable.c +15 -0

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

arch/x86/mm/pgtable.c +0 -1

@@ -782,7 +779,6 @@ int pmd_clear_huge(pmd_t *pmd)
 
 	return 0;
 }
-#endif
 
 #ifdef CONFIG_X86_64
 /**

include/linux/pgtable.h +1 ~~-26~~

index d147480cdefc7..e24d2c992b112 100644
@@ -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);
--

Quoted 1 line

2.26.1

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Will Deacon

maintainer

2021-07-19 11:03 UTC #2 lore raw

Quoted 10 lines · Christophe Leroy

Jonathan Marek <[email protected]> a écrit :
Quoted 6 lines
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.

Quoted 1 line

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.

Quoted 2 lines

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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Ard Biesheuvel

2021-07-19 14:01 UTC #3 lore raw

Quoted 17 lines · Will Deacon

Quoted 10 lines · Christophe Leroy
Quoted 6 lines · 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.
This patch is there for a reason. Reverting it will break some other config.
Which config? Not reverting it breaks arm64.
Quoted 1 line
The fix needs to allow it work with all platforms.

Quoted 11 lines

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.
Quoted 2 lines
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.

Quoted 4 lines

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.

Quoted 4 lines

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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Ard Biesheuvel

2021-07-19 14:20 UTC #4 lore raw

(add some folks back to cc:)

Quoted 49 lines · Ard Biesheuvel

Quoted 17 lines · Will Deacon
Quoted 10 lines · Christophe Leroy
Quoted 6 lines · 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.
This patch is there for a reason. Reverting it will break some other config.
Which config? Not reverting it breaks arm64.
Quoted 1 line
The fix needs to allow it work with all platforms.
Quoted 11 lines
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.
Quoted 2 lines
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.
Quoted 4 lines
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.

Quoted 7 lines

Quoted 4 lines
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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Christophe Leroy

2021-07-19 15:51 UTC #5 lore raw

Ard Biesheuvel <[email protected]> a écrit :

Quoted 49 lines

(add some folks back to cc:)
Quoted 46 lines · Ard Biesheuvel
Quoted 5 lines · Will Deacon
Quoted 3 lines · Christophe Leroy
Quoted 1 line · Jonathan Marek
c742199a breaks arm64 for some configs because it stubs out
functions which
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
should not have been stubbed out.
With 4K pages and ARM64_VA_BITS_39=y, the kernel crashes
early on unmapped
Quoted 1 line
Quoted 1 line
Quoted 1 line
1G pages in the linear map caused by pud_set_huge() in
alloc_init_pud()
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
being stubbed out. Reverting c742199a fixes the crash.
This patch is there for a reason. Reverting it will break some
other config.
Quoted 9 lines
Which config? Not reverting it breaks arm64.
Quoted 1 line
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/
Quoted 2 lines
That was a week ago and doesn't seem to have progressed, so I'm
inclined to
Quoted 1 line
revert this if it's not resolved this week as we usually use -rc3
as a base
Quoted 3 lines
for queuing patches for the next release.
Quoted 1 line
I don't understand, why does arm64 needs these PUD helpers when
it defines
Quoted 1 line
Quoted 1 line
__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.

Quoted 11 lines

Quoted 11 lines
Quoted 5 lines
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.

Quoted 7 lines

Quoted 7 lines
Quoted 4 lines
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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Ard Biesheuvel

2021-07-19 16:24 UTC #6 lore raw

Quoted 53 lines · Christophe Leroy

Quoted 49 lines · Ard Biesheuvel
(add some folks back to cc:)
Quoted 46 lines · Ard Biesheuvel
Quoted 5 lines · Will Deacon
Quoted 3 lines · Christophe Leroy
Quoted 1 line · Jonathan Marek
c742199a breaks arm64 for some configs because it stubs out
functions which
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
should not have been stubbed out.
With 4K pages and ARM64_VA_BITS_39=y, the kernel crashes
early on unmapped
Quoted 1 line
Quoted 1 line
Quoted 1 line
1G pages in the linear map caused by pud_set_huge() in
alloc_init_pud()
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
being stubbed out. Reverting c742199a fixes the crash.
This patch is there for a reason. Reverting it will break some
other config.
Quoted 9 lines
Which config? Not reverting it breaks arm64.
Quoted 1 line
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/
Quoted 2 lines
That was a week ago and doesn't seem to have progressed, so I'm
inclined to
Quoted 1 line
revert this if it's not resolved this week as we usually use -rc3
as a base
Quoted 3 lines
for queuing patches for the next release.
Quoted 1 line
I don't understand, why does arm64 needs these PUD helpers when
it defines
Quoted 1 line
Quoted 1 line
__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.

Quoted 3 lines

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.

Quoted 4 lines

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/

Quoted 3 lines

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)

Quoted 17 lines

Quoted 17 lines
Quoted 17 lines
Quoted 6 lines
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.
Quoted 4 lines
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).

Quoted 6 lines

Quoted 2 lines
Quoted 2 lines
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.

Quoted 3 lines

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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Christophe Leroy

2021-07-19 16:54 UTC #7 lore raw

Ard Biesheuvel <[email protected]> a écrit :

Quoted 63 lines

Quoted 54 lines · Christophe Leroy
Quoted 31 lines · Ard Biesheuvel
(add some folks back to cc:)
Quoted 28 lines · Ard Biesheuvel
Quoted 5 lines · Will Deacon
Quoted 3 lines · Christophe Leroy
Quoted 1 line · Jonathan Marek
c742199a breaks arm64 for some configs because it stubs out
functions which
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
should not have been stubbed out.
With 4K pages and ARM64_VA_BITS_39=y, the kernel crashes
early on unmapped
Quoted 1 line
Quoted 1 line
Quoted 1 line
1G pages in the linear map caused by pud_set_huge() in
alloc_init_pud()
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
being stubbed out. Reverting c742199a fixes the crash.
This patch is there for a reason. Reverting it will break some
other config.
Quoted 9 lines
Which config? Not reverting it breaks arm64.
Quoted 1 line
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/
Quoted 11 lines
Quoted 11 lines
Quoted 2 lines
That was a week ago and doesn't seem to have progressed, so I'm
inclined to
Quoted 1 line
revert this if it's not resolved this week as we usually use -rc3
as a base
Quoted 3 lines
for queuing patches for the next release.
Quoted 1 line
I don't understand, why does arm64 needs these PUD helpers when
it defines
Quoted 1 line
Quoted 1 line
__PAGETABLE_PUD_FOLDED ?
Quoted 7 lines
Quoted 7 lines
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.
Quoted 3 lines
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.

Quoted 7 lines

Quoted 4 lines
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.

Quoted 31 lines

Quoted 4 lines
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)
Quoted 22 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Probably for the huge vmap code; see
arch_vmap_pXd_supported(). That also
Quoted 9 lines
Quoted 9 lines
Quoted 2 lines
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.
Quoted 1 line
For 4k pages with 3 levels of walk, we want to be able to use
1GB mappings
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines
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.

Quoted 7 lines

Quoted 3 lines
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 ?

Quoted 8 lines

Quoted 5 lines
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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Will Deacon

maintainer

2021-07-20 11:28 UTC #8 lore raw

Quoted 19 lines · Christophe Leroy

Ard Biesheuvel <[email protected]> a écrit :
Quoted 18 lines
Quoted 15 lines · Christophe Leroy
Quoted 3 lines · Ard Biesheuvel
Quoted 2 lines · Ard Biesheuvel
Quoted 1 line · Will Deacon
For 4k pages with 3 levels of walk, we want to be able to use
1GB mappings
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines
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.

Quoted 7 lines

Quoted 3 lines
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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Christophe Leroy

2021-07-20 15:11 UTC #9 lore raw

Will Deacon <[email protected]> a écrit :

Quoted 29 lines · Christophe Leroy

Quoted 20 lines
Ard Biesheuvel <[email protected]> a écrit :
Quoted 8 lines
Quoted 6 lines · Christophe Leroy
Quoted 3 lines · Ard Biesheuvel
Quoted 2 lines · Ard Biesheuvel
Quoted 1 line · Will Deacon
For 4k pages with 3 levels of walk, we want to be able to use
1GB mappings
Quoted 1 line
Quoted 1 line
Quoted 1 line
at level 1 (pgd), 2MB mappings at level 2 (pmd) as well as
the usual 4k
Quoted 10 lines
Quoted 9 lines
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
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.
Quoted 7 lines
Quoted 3 lines
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 ?

Quoted 7 lines

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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

LEROY Christophe

2021-08-02 11:43 UTC #10 lore raw

In reply to Will Deacon

Le 20/07/2021 à 13:27, Will Deacon a écrit :

Quoted 24 lines · Christophe Leroy

Quoted 23 lines
Ard Biesheuvel <[email protected]> a écrit :
Quoted 21 lines
Quoted 9 lines · Christophe Leroy
Quoted 3 lines · Ard Biesheuvel
Quoted 2 lines · Ard Biesheuvel
Quoted 1 line · Will Deacon
For 4k pages with 3 levels of walk, we want to be able to use
1GB mappings
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
at level 1 (pgd), 2MB mappings at level 2 (pmd) as well as the usual 4k page mappings at level 3 (pte).
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
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.

Quoted 8 lines

Quoted 3 lines
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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Christophe Leroy

2021-07-19 15:02 UTC #11 lore raw

In reply to Will Deacon

Will Deacon <[email protected]> a écrit :

Quoted 7 lines · Christophe Leroy

Quoted 6 lines
Jonathan Marek <[email protected]> a écrit :
Quoted 1 line
c742199a breaks arm64 for some configs because it stubs out
functions which
Quoted 2 lines
should not have been stubbed out.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
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

Quoted 11 lines

Quoted 2 lines
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.

Quoted 10 lines

Quoted 3 lines
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

Quoted 2 lines

Will

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Ard Biesheuvel

Acked-by on v1 1/1

2021-07-20 12:33 UTC #12 lore raw

In reply to Jonathan Marek

Quoted 10 lines · 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 <[email protected]>

(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 <[email protected]>

Quoted 167 lines

---
 arch/arm64/mm/mmu.c     | 20 ++++++++------------
 arch/x86/mm/pgtable.c   | 34 +++++++++++++++-------------------
 include/linux/pgtable.h | 26 +-------------------------
 3 files changed, 24 insertions(+), 56 deletions(-)

arch/arm64/mm/mmu.c +8 ~~-12~~

index d745865084488..9ff0de1b2b93c 100644
@@ -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)
 {

arch/x86/mm/pgtable.c +15 ~~-19~~

index 3364fe62b9037..3481b35cb4ec7 100644
@@ -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
 /**

include/linux/pgtable.h +1 ~~-26~~

index d147480cdefc7..e24d2c992b112 100644
@@ -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