Fix issues with huge mapping in ioremap for ARM64

From V11->V12:

From V10->V11:

From V9->V10:

From V8->V9:

From V7->V8:

From V6->V7:

From V5->V6:

From V4->V5:

From V3->V4:

From V2->V3:

From V1->V2:

Add helper macros to give virtual references to page tables. These will be used while freeing dangling page tables.

Signed-off-by Chintan Pandya <[email protected]>

---
 arch/arm64/include/asm/pgtable.h | 3 +++
 1 file changed, 3 insertions(+)

arch/arm64/include/asm/pgtable.h +0 -0

index 7c4c8f3..ef4047f 100644

Quoted 15 lines · Chintan Pandya

Add helper macros to give virtual references to page tables. These will be used while freeing dangling page tables.

Signed-off-by Chintan Pandya <[email protected]>

---
  arch/arm64/include/asm/pgtable.h | 3 +++
  1 file changed, 3 insertions(+)

arch/arm64/include/asm/pgtable.h +0 -0

index 7c4c8f3..ef4047f 100644
@@ -580,6 +580,9 @@ static inline phys_addr_t pgd_page_paddr(pgd_t pgd)
 

arch/arm64/include/asm/pgtable.h static inline phys_addr_t pgd_page_paddr(pgd_t pgd) · line 584

Quoted 4 lines

arch/arm64/include/asm/pgtable.h +2 -0

  #endif  /* CONFIG_PGTABLE_LEVELS > 3 */
 
+#define pmd_page_vaddr(pmd) __va(pmd_page_paddr(pmd))
+#define pud_page_vaddr(pud) __va(pud_page_paddr(pud))

Are these actually needed, or do pte_offset_kernel and pmd_offset do the job already?

Will

Quoted 10 lines · Will Deacon

Quoted 9 lines · Chintan Pandya

Add helper macros to give virtual references to page tables. These will be used while freeing dangling page tables.

Signed-off-by Chintan Pandya <[email protected]>

---
 arch/arm64/include/asm/pgtable.h | 3 +++
 1 file changed, 3 insertions(+)

Quoted 12 lines

arch/arm64/include/asm/pgtable.h static inline phys_addr_t pgd_page_paddr(pgd_t pgd) · line 582

Quoted 8 lines

arch/arm64/include/asm/pgtable.h +2 -0

index 7c4c8f3..ef4047f 100644
@@ -580,6 +580,9 @@ static inline phys_addr_t pgd_page_paddr(pgd_t pgd)
 #endif  /* CONFIG_PGTABLE_LEVELS > 3 */
+#define pmd_page_vaddr(pmd) __va(pmd_page_paddr(pmd))
+#define pud_page_vaddr(pud) __va(pud_page_paddr(pud))

Are these actually needed, or do pte_offset_kernel and pmd_offset do the job already?

I introduced these macros for consistency across different arch.

Looking at pte_offset_kernel, it seems to use READ_ONCE() which looks little costly for its intended use (in next patch) where we already have dereferenced value. Do you still suggest to remove this ?

Implement pud_free_pmd_page() and pmd_free_pte_page().

Implementation requires,
1) Clearing off the current pud/pmd entry
2) Invalidate TLB which could have previously
valid but not stale entry
3) Freeing of the un-used next level page tables

Signed-off-by Chintan Pandya <[email protected]>

---
 arch/arm64/mm/mmu.c | 38 ++++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

arch/arm64/mm/mmu.c +13 -3

index 8ae5d7a..6e7e16c 100644
@@ -45,6 +45,7 @@
 #include <asm/memblock.h>
 #include <asm/mmu_context.h>
 #include <asm/ptdump.h>
+#include <asm/tlbflush.h>
 
 #define NO_BLOCK_MAPPINGS	BIT(0)
 #define NO_CONT_MAPPINGS	BIT(1)

@@ -977,12 +978,41 @@ int pmd_clear_huge(pmd_t *pmdp)
 	return 1;
 }
 
-int pud_free_pmd_page(pud_t *pud, unsigned long addr)
+int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
 {
-	return pud_none(*pud);
+	pte_t *table;
+	pmd_t pmd;
+
+	pmd = READ_ONCE(*pmdp);
+	if (pmd_present(pmd)) {
+		table = pmd_page_vaddr(pmd);
+		pmd_clear(pmdp);
+		__flush_tlb_kernel_pgtable(addr);
+		pte_free_kernel(NULL, table);
+	}
+	return 1;
 }
 
-int pmd_free_pte_page(pmd_t *pmd, unsigned long addr)

Quoted 7 lines · Chintan Pandya

Implement pud_free_pmd_page() and pmd_free_pte_page().

Implementation requires,
1) Clearing off the current pud/pmd entry
2) Invalidate TLB which could have previously valid but not stale entry
3) Freeing of the un-used next level page tables

Please can you rewrite this describing the problem that you're solving, rather than a brief summary of some requirements?

Quoted 40 lines

Signed-off-by Chintan Pandya <[email protected]>

---
  arch/arm64/mm/mmu.c | 38 ++++++++++++++++++++++++++++++++++----
  1 file changed, 34 insertions(+), 4 deletions(-)

arch/arm64/mm/mmu.c +13 -3

index 8ae5d7a..6e7e16c 100644
@@ -45,6 +45,7 @@
  #include <asm/memblock.h>
  #include <asm/mmu_context.h>
  #include <asm/ptdump.h>
+#include <asm/tlbflush.h>
 
  #define NO_BLOCK_MAPPINGS	BIT(0)
  #define NO_CONT_MAPPINGS	BIT(1)

@@ -977,12 +978,41 @@ int pmd_clear_huge(pmd_t *pmdp)
  	return 1;
  }
 
-int pud_free_pmd_page(pud_t *pud, unsigned long addr)
+int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
  {
-	return pud_none(*pud);
+	pte_t *table;
+	pmd_t pmd;
+
+	pmd = READ_ONCE(*pmdp);
+	if (pmd_present(pmd)) {
+		table = pmd_page_vaddr(pmd);
+		pmd_clear(pmdp);
+		__flush_tlb_kernel_pgtable(addr);
+		pte_free_kernel(NULL, table);
+	}
+	return 1;
  }
 
-int pmd_free_pte_page(pmd_t *pmd, unsigned long addr)

arch/arm64/mm/mmu.c int pmd_clear_huge(pmd_t *pmdp) · line 1004

Quoted 10 lines

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

+int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
  {
-	return pmd_none(*pmd);
+	pmd_t *table;
+	pmd_t *entry;
+	pud_t pud;
+	unsigned long next, end;
+
+	pud = READ_ONCE(*pudp);
+	if (pud_present(pud)) {

Just some stylistic stuff, but please can you rewrite this as:

	if (!pud_present(pud) || VM_WARN_ON(!pud_table(pud)))
		return 1;

similarly for the pmd/pte code above.

arch/arm64/mm/mmu.c int pmd_clear_huge(pmd_t *pmdp) · line 1006

Quoted 3 lines

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

+		table = pud_page_vaddr(pud);
+		entry = table;

Could you rename entry -> pmdp, please?

arch/arm64/mm/mmu.c int pmd_clear_huge(pmd_t *pmdp) · line 1017

Quoted 12 lines

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

+		next = addr;
+		end = addr + PUD_SIZE;
+		do {
+			pmd_free_pte_page(entry, next);
+		} while (entry++, next += PMD_SIZE, next != end);
+
+		pud_clear(pudp);
+		__flush_tlb_kernel_pgtable(addr);
+		pmd_free(NULL, table);
+	}
+	return 1;

So with these patches, we only ever return 1 from these helpers. It looks like the same is true for x86, so how about we make them void and move the calls inside the conditionals in lib/ioremap.c? Obviously, this would be a separate patch on the end.

Will

Quoted 17 lines · Will Deacon

arch/arm64/mm/mmu.c int pmd_clear_huge(pmd_t *pmdp) · line 1017

Quoted 11 lines · Chintan Pandya

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

+		next = addr;
+		end = addr + PUD_SIZE;
+		do {
+			pmd_free_pte_page(entry, next);
+		} while (entry++, next += PMD_SIZE, next != end);
+
+		pud_clear(pudp);
+		__flush_tlb_kernel_pgtable(addr);
+		pmd_free(NULL, table);
+	}
+	return 1;

So with these patches, we only ever return 1 from these helpers. It looks like the same is true for x86, so how about we make them void and move the calls inside the conditionals in lib/ioremap.c? Obviously, this would be a separate patch on the end.

That sounds valid code churn to me. But since x86 discussion is not concluded yet, I would wait to share until that gets resolved. May be not in v13 but separate effort. Would that be okay to you ?

Huge mappings have had stability issues due to stale TLB entry and memory leak issues. Since, those are addressed in this series of patches, it is now safe to allow huge mappings.

Signed-off-by Chintan Pandya <[email protected]>

---
 arch/arm64/mm/mmu.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

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

index 6e7e16c..c65abc4 100644

Quoted 15 lines · Chintan Pandya

Huge mappings have had stability issues due to stale TLB entry and memory leak issues. Since, those are addressed in this series of patches, it is now safe to allow huge mappings.

Signed-off-by Chintan Pandya <[email protected]>

---
  arch/arm64/mm/mmu.c | 18 ++----------------
  1 file changed, 2 insertions(+), 16 deletions(-)

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

index 6e7e16c..c65abc4 100644

arch/arm64/mm/mmu.c int pud_set_huge(pud_t *pudp, phys_addr_t phys, pgprot_t prot) · line 942

Quoted 9 lines

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

@@ -934,15 +934,8 @@ int pud_set_huge(pud_t *pudp, phys_addr_t phys, pgprot_t prot)
  {
  	pgprot_t sect_prot = __pgprot(PUD_TYPE_SECT |
  					pgprot_val(mk_sect_prot(prot)));
-	pud_t new_pud = pfn_pud(__phys_to_pfn(phys), sect_prot);
-
-	/* Only allow permission changes for now */
-	if (!pgattr_change_is_safe(READ_ONCE(pud_val(*pudp)),
-				   pud_val(new_pud)))
-		return 0;

Do you actually need to remove these checks? If we're doing break-before-make properly, then the check won't fire but it would be good to keep it there so we can catch misuse of these in future.

In other words, can we drop this patch?

Will

This series of patches re-bring huge vmap back for arm64.

Patch 1/4 has been taken by Toshi in his series of patches by name "[PATCH v3 0/3] fix free pmd/pte page handlings on x86" to avoid merge conflict with this series.

These patches are tested on 4.16 kernel with Cortex-A75 based SoC.

The test used for verifying these patches is a stress test on ioremap/unmap which tries to re-use same io-address but changes size of mapping randomly i.e. 4K to 2M to 1G etc. The same test used to reproduce 3rd level translation fault without these fixes (and also of course with Revert "arm64: Enforce BBM for huge IO/VMAP mappings" being part of the tree).

These patches can also go into '-stable' branch (if accepted) for 4.6 onwards.

From V11->V12:
- Introduced p*d_page_vaddr helper macros and using them
- Rebased over current tip

From V10->V11:
- Updated pud_free_pmd_page & pmd_free_pte_page to use consistent conding style
- Fixed few bugs by using pmd_page_paddr & pud_page_paddr

From V9->V10:
- Updated commit log for patch 1/4 by Toshi
- Addressed review comments by Will on patch 3/4

From V8->V9:
- Used __TLBI_VADDR macros in new TLB flush API

From V7->V8:
- Properly fixed compilation issue in x86 file

From V6->V7:
- Fixed compilation issue in x86 case
- V6 patches were not properly enumarated

From V5->V6:
- Use __flush_tlb_kernel_pgtable() for both PUD and PMD. Remove "bool tlb_inv" based variance as it is not need now
- Re-naming for consistency

From V4->V5:
- Add new API __flush_tlb_kernel_pgtable(unsigned long addr) for kernel addresses

From V3->V4:
- Add header for 'addr' in x86 implementation
- Re-order pmd/pud clear and table free
- Avoid redundant TLB invalidatation in one perticular case

From V2->V3:
- Use the exisiting page table free interface to do arm64 specific things

From V1->V2:
- Rebased my patches on top of "[PATCH v2 1/2] mm/vmalloc:
Add interfaces to free unmapped page table"
- Honored BBM for ARM64

Chintan Pandya (5):
ioremap: Update pgtable free interfaces with addr
arm64: tlbflush: Introduce __flush_tlb_kernel_pgtable
arm64: pgtable: Add p*d_page_vaddr helper macros
arm64: Implement page table free interfaces
arm64: Re-enable huge io mappings

  arch/arm64/include/asm/pgtable.h  |  3 +++
  arch/arm64/include/asm/tlbflush.h |  7 +++++
  arch/arm64/mm/mmu.c               | 56 +++++++++++++++++++++++++--------------
  arch/x86/mm/pgtable.c             |  8 +++---
  include/asm-generic/pgtable.h     |  8 +++---
  lib/ioremap.c                     |  4 +--
  6 files changed, 57 insertions(+), 29 deletions(-)

Just curious to know, is there anything that I should be addressing in these patches ? For now, I don't see anything from my side that requires modification, unless one has some more review comments on this.

Status so far on and around this:
- Status of Toshi's series of patches is still not clear to me. However, if this series can get through first, there won't be conflicting scenarios as far as arm64 is concerned.
- I've rebased these patches on tip
- Also re-tested these patches for long duration tests with 1 GB mapping case also exercised enough. Test ended positively.