[PATCH v12 0/5] Fix issues with huge mapping in ioremap for ARM64

[PATCH v12 0/5] Fix issues with huge mapping in ioremap for ARM64

[Chintan Pandya]

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

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation
Center, Inc., is a member of Code Aurora Forum, a Linux Foundation
Collaborative Project

[PATCH v12 1/5] ioremap: Update pgtable free interfaces with addr

[Chintan Pandya]

From: Chintan Pandya <cpandya@codeaurora.org>

The following kernel panic was observed on ARM64 platform due to a stale
TLB entry.

 1. ioremap with 4K size, a valid pte page table is set.
 2. iounmap it, its pte entry is set to 0.
 3. ioremap the same address with 2M size, update its pmd entry with
    a new value.
 4. CPU may hit an exception because the old pmd entry is still in TLB,
    which leads to a kernel panic.

Commit b6bdb7517c3d ("mm/vmalloc: add interfaces to free unmapped page
table") has addressed this panic by falling to pte mappings in the above
case on ARM64.

To support pmd mappings in all cases, TLB purge needs to be performed
in this case on ARM64.

Add a new arg, 'addr', to pud_free_pmd_page() and pmd_free_pte_page()
so that TLB purge can be added later in seprate patches.

[toshi@hpe.com: merge changes, rewrite patch description]
Fixes: 28ee90fe6048 ("x86/mm: implement free pmd/pte page interfaces")
Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: <stable@vger.kernel.org>

---
 arch/arm64/mm/mmu.c           | 4 ++--
 arch/x86/mm/pgtable.c         | 8 +++++---
 include/asm-generic/pgtable.h | 8 ++++----
 lib/ioremap.c                 | 4 ++--
 4 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 493ff75..8ae5d7a 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -977,12 +977,12 @@ int pmd_clear_huge(pmd_t *pmdp)
 	return 1;
 }
 
-int pud_free_pmd_page(pud_t *pud)
+int pud_free_pmd_page(pud_t *pud, unsigned long addr)
 {
 	return pud_none(*pud);
 }
 
-int pmd_free_pte_page(pmd_t *pmd)
+int pmd_free_pte_page(pmd_t *pmd, unsigned long addr)
 {
 	return pmd_none(*pmd);
 }
diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
index ffc8c13..37e3cba 100644
--- a/arch/x86/mm/pgtable.c
+++ b/arch/x86/mm/pgtable.c
@@ -718,11 +718,12 @@ int pmd_clear_huge(pmd_t *pmd)
 /**
  * pud_free_pmd_page - Clear pud entry and free pmd page.
  * @pud: Pointer to a PUD.
+ * @addr: Virtual address associated with pud.
  *
  * Context: The pud range has been unmaped and TLB purged.
  * Return: 1 if clearing the entry succeeded. 0 otherwise.
  */
-int pud_free_pmd_page(pud_t *pud)
+int pud_free_pmd_page(pud_t *pud, unsigned long addr)
 {
 	pmd_t *pmd;
 	int i;
@@ -733,7 +734,7 @@ int pud_free_pmd_page(pud_t *pud)
 	pmd = (pmd_t *)pud_page_vaddr(*pud);
 
 	for (i = 0; i < PTRS_PER_PMD; i++)
-		if (!pmd_free_pte_page(&pmd[i]))
+		if (!pmd_free_pte_page(&pmd[i], addr + (i * PMD_SIZE)))
 			return 0;
 
 	pud_clear(pud);
@@ -745,11 +746,12 @@ int pud_free_pmd_page(pud_t *pud)
 /**
  * pmd_free_pte_page - Clear pmd entry and free pte page.
  * @pmd: Pointer to a PMD.
+ * @addr: Virtual address associated with pmd.
  *
  * Context: The pmd range has been unmaped and TLB purged.
  * Return: 1 if clearing the entry succeeded. 0 otherwise.
  */
-int pmd_free_pte_page(pmd_t *pmd)
+int pmd_free_pte_page(pmd_t *pmd, unsigned long addr)
 {
 	pte_t *pte;
 
diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
index f59639a..b081794 100644
--- a/include/asm-generic/pgtable.h
+++ b/include/asm-generic/pgtable.h
@@ -1019,8 +1019,8 @@ static inline int p4d_clear_huge(p4d_t *p4d)
 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);
-int pud_free_pmd_page(pud_t *pud);
-int pmd_free_pte_page(pmd_t *pmd);
+int pud_free_pmd_page(pud_t *pud, unsigned long addr);
+int pmd_free_pte_page(pmd_t *pmd, unsigned long addr);
 #else	/* !CONFIG_HAVE_ARCH_HUGE_VMAP */
 static inline int p4d_set_huge(p4d_t *p4d, phys_addr_t addr, pgprot_t prot)
 {
@@ -1046,11 +1046,11 @@ static inline int pmd_clear_huge(pmd_t *pmd)
 {
 	return 0;
 }
-static inline int pud_free_pmd_page(pud_t *pud)
+static inline int pud_free_pmd_page(pud_t *pud, unsigned long addr)
 {
 	return 0;
 }
-static inline int pmd_free_pte_page(pmd_t *pmd)
+static inline int pmd_free_pte_page(pmd_t *pmd, unsigned long addr)
 {
 	return 0;
 }
diff --git a/lib/ioremap.c b/lib/ioremap.c
index 54e5bba..517f585 100644
--- a/lib/ioremap.c
+++ b/lib/ioremap.c
@@ -92,7 +92,7 @@ static inline int ioremap_pmd_range(pud_t *pud, unsigned long addr,
 		if (ioremap_pmd_enabled() &&
 		    ((next - addr) == PMD_SIZE) &&
 		    IS_ALIGNED(phys_addr + addr, PMD_SIZE) &&
-		    pmd_free_pte_page(pmd)) {
+		    pmd_free_pte_page(pmd, addr)) {
 			if (pmd_set_huge(pmd, phys_addr + addr, prot))
 				continue;
 		}
@@ -119,7 +119,7 @@ static inline int ioremap_pud_range(p4d_t *p4d, unsigned long addr,
 		if (ioremap_pud_enabled() &&
 		    ((next - addr) == PUD_SIZE) &&
 		    IS_ALIGNED(phys_addr + addr, PUD_SIZE) &&
-		    pud_free_pmd_page(pud)) {
+		    pud_free_pmd_page(pud, addr)) {
 			if (pud_set_huge(pud, phys_addr + addr, prot))
 				continue;
 		}
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation
Center, Inc., is a member of Code Aurora Forum, a Linux Foundation
Collaborative Project

[PATCH v12 2/5] arm64: tlbflush: Introduce __flush_tlb_kernel_pgtable

[Chintan Pandya]

Add an interface to invalidate intermediate page tables
from TLB for kernel.

Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
---
 arch/arm64/include/asm/tlbflush.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/include/asm/tlbflush.h b/arch/arm64/include/asm/tlbflush.h
index dfc61d7..a4a1901 100644
--- a/arch/arm64/include/asm/tlbflush.h
+++ b/arch/arm64/include/asm/tlbflush.h
@@ -218,6 +218,13 @@ static inline void __flush_tlb_pgtable(struct mm_struct *mm,
 	dsb(ish);
 }
 
+static inline void __flush_tlb_kernel_pgtable(unsigned long kaddr)
+{
+	unsigned long addr = __TLBI_VADDR(kaddr, 0);
+
+	__tlbi(vaae1is, addr);
+	dsb(ish);
+}
 #endif
 
 #endif
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation
Center, Inc., is a member of Code Aurora Forum, a Linux Foundation
Collaborative Project

[PATCH v12 3/5] arm64: pgtable: Add p*d_page_vaddr helper macros

[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 <cpandya@codeaurora.org>
---
 arch/arm64/include/asm/pgtable.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index 7c4c8f3..ef4047f 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -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))
+
 #define pgd_ERROR(pgd)		__pgd_error(__FILE__, __LINE__, pgd_val(pgd))
 
 /* to find an entry in a page-table-directory */
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation
Center, Inc., is a member of Code Aurora Forum, a Linux Foundation
Collaborative Project

[PATCH v12 4/5] arm64: Implement page table free interfaces

[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

Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
---
 arch/arm64/mm/mmu.c | 38 ++++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 8ae5d7a..6e7e16c 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -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)
+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)) {
+		table = pud_page_vaddr(pud);
+		entry = table;
+		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;
 }
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation
Center, Inc., is a member of Code Aurora Forum, a Linux Foundation
Collaborative Project

[PATCH v12 5/5] arm64: Allow huge io mappings again

[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 <cpandya@codeaurora.org>
---
 arch/arm64/mm/mmu.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 6e7e16c..c65abc4 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -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;
-
 	BUG_ON(phys & ~PUD_MASK);
-	set_pud(pudp, new_pud);
+	set_pud(pudp, pfn_pud(__phys_to_pfn(phys), sect_prot));
 	return 1;
 }
 
@@ -950,15 +943,8 @@ int pmd_set_huge(pmd_t *pmdp, phys_addr_t phys, pgprot_t prot)
 {
 	pgprot_t sect_prot = __pgprot(PMD_TYPE_SECT |
 					pgprot_val(mk_sect_prot(prot)));
-	pmd_t new_pmd = pfn_pmd(__phys_to_pfn(phys), sect_prot);
-
-	/* Only allow permission changes for now */
-	if (!pgattr_change_is_safe(READ_ONCE(pmd_val(*pmdp)),
-				   pmd_val(new_pmd)))
-		return 0;
-
 	BUG_ON(phys & ~PMD_MASK);
-	set_pmd(pmdp, new_pmd);
+	set_pmd(pmdp, pfn_pmd(__phys_to_pfn(phys), sect_prot));
 	return 1;
 }
 
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation
Center, Inc., is a member of Code Aurora Forum, a Linux Foundation
Collaborative Project

Re: [PATCH v12 0/5] Fix issues with huge mapping in ioremap for ARM64

[Chintan Pandya]

Hi Will,

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.

Thanks,

On 6/1/2018 6:09 PM, Chintan Pandya wrote:
> 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(-)
> 

Chintan
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation
Collaborative Project

Re: [PATCH v12 0/5] Fix issues with huge mapping in ioremap for ARM64

[Will Deacon]

Hi Chintan,

On Mon, Jun 04, 2018 at 11:26:28AM +0530, Chintan Pandya wrote:
> 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.

I'll try to review this version today.

Will

Re: [PATCH v12 4/5] arm64: Implement page table free interfaces

[Will Deacon]

On Fri, Jun 01, 2018 at 06:09:17PM +0530, Chintan Pandya wrote:
> 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?

> Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
> ---
>  arch/arm64/mm/mmu.c | 38 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 8ae5d7a..6e7e16c 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -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)
> +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.

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

Could you rename entry -> pmdp, please?

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

Re: [PATCH v12 3/5] arm64: pgtable: Add p*d_page_vaddr helper macros

[Will Deacon]

On Fri, Jun 01, 2018 at 06:09:16PM +0530, Chintan Pandya wrote:
> Add helper macros to give virtual references to page
> tables. These will be used while freeing dangling
> page tables.
> 
> Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
> ---
>  arch/arm64/include/asm/pgtable.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index 7c4c8f3..ef4047f 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -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?

Will

Re: [PATCH v12 5/5] arm64: Allow huge io mappings again

[Will Deacon]

On Fri, Jun 01, 2018 at 06:09:18PM +0530, Chintan Pandya wrote:
> 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 <cpandya@codeaurora.org>
> ---
>  arch/arm64/mm/mmu.c | 18 ++----------------
>  1 file changed, 2 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 6e7e16c..c65abc4 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -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

Re: [PATCH v12 5/5] arm64: Allow huge io mappings again

[Chintan Pandya]

On 6/4/2018 5:44 PM, Will Deacon wrote:
> On Fri, Jun 01, 2018 at 06:09:18PM +0530, Chintan Pandya wrote:
>> 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 <cpandya@codeaurora.org>
>> ---
>>   arch/arm64/mm/mmu.c | 18 ++----------------
>>   1 file changed, 2 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index 6e7e16c..c65abc4 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -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?

Yes, we don't need this patch as BBM is happening before this. I missed
that. I'll remove this patch in v13.

> 
> Will
> 

Chintan
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation
Collaborative Project

Re: [PATCH v12 4/5] arm64: Implement page table free interfaces

[Chintan Pandya]

On 6/4/2018 5:43 PM, Will Deacon wrote:
> On Fri, Jun 01, 2018 at 06:09:17PM +0530, Chintan Pandya wrote:
>> 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?

Okay. I'll fix this in v13.

> 
>> Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
>> ---
>>   arch/arm64/mm/mmu.c | 38 ++++++++++++++++++++++++++++++++++----
>>   1 file changed, 34 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index 8ae5d7a..6e7e16c 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -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)
>> +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.

Okay. v13 will have this.

> 
>> +		table = pud_page_vaddr(pud);
>> +		entry = table;
> 
> Could you rename entry -> pmdp, please?

Sure.

> 
>> +		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 ?

> 
> Will
> 

Chintan
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation
Collaborative Project

Re: [PATCH v12 3/5] arm64: pgtable: Add p*d_page_vaddr helper macros

[Chintan Pandya]

On 6/4/2018 5:43 PM, Will Deacon wrote:
> On Fri, Jun 01, 2018 at 06:09:16PM +0530, Chintan Pandya wrote:
>> Add helper macros to give virtual references to page
>> tables. These will be used while freeing dangling
>> page tables.
>>
>> Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
>> ---
>>   arch/arm64/include/asm/pgtable.h | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
>> index 7c4c8f3..ef4047f 100644
>> --- a/arch/arm64/include/asm/pgtable.h
>> +++ b/arch/arm64/include/asm/pgtable.h
>> @@ -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 ?

> Will
> 

Chintan
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation
Collaborative Project

Re: [PATCH v12 4/5] arm64: Implement page table free interfaces

[Will Deacon]

On Mon, Jun 04, 2018 at 07:13:18PM +0530, Chintan Pandya wrote:
> On 6/4/2018 5:43 PM, Will Deacon wrote:
> >On Fri, Jun 01, 2018 at 06:09:17PM +0530, Chintan Pandya wrote:
> >>+		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 ?

Yes, fine by me.

Will

Re: [PATCH v12 3/5] arm64: pgtable: Add p*d_page_vaddr helper macros

[Will Deacon]

On Mon, Jun 04, 2018 at 07:13:48PM +0530, Chintan Pandya wrote:
> 
> 
> On 6/4/2018 5:43 PM, Will Deacon wrote:
> >On Fri, Jun 01, 2018 at 06:09:16PM +0530, Chintan Pandya wrote:
> >>Add helper macros to give virtual references to page
> >>tables. These will be used while freeing dangling
> >>page tables.
> >>
> >>Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
> >>---
> >>  arch/arm64/include/asm/pgtable.h | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >>diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> >>index 7c4c8f3..ef4047f 100644
> >>--- a/arch/arm64/include/asm/pgtable.h
> >>+++ b/arch/arm64/include/asm/pgtable.h
> >>@@ -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 ?

It's only an additional load instruction on the freeing path and it matches
what we do in other page table code, so I'd rather use the existing API
unless we have numbers to show otherwise.

Will