[PATCH v3 0/3] Fix issues with huge mapping in ioremap for ARM64

[PATCH v3 0/3] Fix issues with huge mapping in ioremap for ARM64

[Chintan Pandya]

This series of patches are follow up work (and depends on)
Toshi Kani <toshi.kani@hpe.com>'s patches "fix memory leak/
panic in ioremap huge pages".

This series of patches are tested on 4.9 kernel with Cortex-A75
based SoC.

Chintan Pandya (3):
  ioremap: Update pgtable free interfaces with addr
  arm64: Implement page table free interfaces
  Revert "arm64: Enforce BBM for huge IO/VMAP mappings"

>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

 arch/arm64/mm/mmu.c           | 40 ++++++++++++++++++++++++++++------------
 arch/x86/mm/pgtable.c         |  4 ++--
 include/asm-generic/pgtable.h |  8 ++++----
 lib/ioremap.c                 |  4 ++--
 4 files changed, 36 insertions(+), 20 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 v3 1/3] ioremap: Update pgtable free interfaces with addr

[Chintan Pandya]

This patch ("mm/vmalloc: Add interfaces to free unmapped
page table") adds following 2 interfaces to free the page
table in case we implement huge mapping.

pud_free_pmd_page() and pmd_free_pte_page()

Some architectures (like arm64) needs to do proper TLB
maintanance after updating pagetable entry even in map.
Why ? Read this,
https://patchwork.kernel.org/patch/10134581/

Pass 'addr' in these interfaces so that proper TLB ops
can be performed.

Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
---
 arch/arm64/mm/mmu.c           | 4 ++--
 arch/x86/mm/pgtable.c         | 4 ++--
 include/asm-generic/pgtable.h | 8 ++++----
 lib/ioremap.c                 | 4 ++--
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 2dbb2c9..da98828 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -973,12 +973,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 34cda7e..20b0322 100644
--- a/arch/x86/mm/pgtable.c
+++ b/arch/x86/mm/pgtable.c
@@ -710,7 +710,7 @@ int pmd_clear_huge(pmd_t *pmd)
  * 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;
@@ -737,7 +737,7 @@ int pud_free_pmd_page(pud_t *pud)
  * 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 2490800..7f00c2d 100644
--- a/include/asm-generic/pgtable.h
+++ b/include/asm-generic/pgtable.h
@@ -983,8 +983,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)
 {
@@ -1010,11 +1010,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(pud_t *pmd)
+static inline int pmd_free_pte_page(pud_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 v3 1/3] ioremap: Update pgtable free interfaces with addr

[Kani, Toshi]

On Mon, 2018-03-19 at 18:10 +0530, Chintan Pandya wrote:
> This patch ("mm/vmalloc: Add interfaces to free unmapped
> page table") adds following 2 interfaces to free the page
> table in case we implement huge mapping.
> 
> pud_free_pmd_page() and pmd_free_pte_page()
> 
> Some architectures (like arm64) needs to do proper TLB
> maintanance after updating pagetable entry even in map.
> Why ? Read this,
> https://patchwork.kernel.org/patch/10134581/
> 
> Pass 'addr' in these interfaces so that proper TLB ops
> can be performed.
> 
> Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
 :
> diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
> index 34cda7e..20b0322 100644
> --- a/arch/x86/mm/pgtable.c
> +++ b/arch/x86/mm/pgtable.c
> @@ -710,7 +710,7 @@ int pmd_clear_huge(pmd_t *pmd)
>   * 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)

Please update the function header as well.  Same for pmd.

+ * @addr: Virtual address associated with pud.

--- a/include/asm-generic/pgtable.h
> +++ b/include/asm-generic/pgtable.h
> @@ -983,8 +983,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)
>  {
> @@ -1010,11 +1010,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(pud_t *pmd)
> +static inline int pmd_free_pte_page(pud_t *pmd, unsigned long addr)

Please base your patches on top of the mm tree or at least pick up
8cab93de994f6 in the mm tree.  Andrew fixed the typo; the 'pud_t' above
should have been 'pmd_t'.

Thanks,
-Toshi

[PATCH v3 1/3] ioremap: Update pgtable free interfaces with addr

[Chintan Pandya]

On 3/20/2018 12:31 AM, Kani, Toshi wrote:
> On Mon, 2018-03-19 at 18:10 +0530, Chintan Pandya wrote:
>> This patch ("mm/vmalloc: Add interfaces to free unmapped
>> page table") adds following 2 interfaces to free the page
>> table in case we implement huge mapping.
>>
>> pud_free_pmd_page() and pmd_free_pte_page()
>>
>> Some architectures (like arm64) needs to do proper TLB
>> maintanance after updating pagetable entry even in map.
>> Why ? Read this,
>> https://patchwork.kernel.org/patch/10134581/
>>
>> Pass 'addr' in these interfaces so that proper TLB ops
>> can be performed.
>>
>> Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
>   :
>> diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
>> index 34cda7e..20b0322 100644
>> --- a/arch/x86/mm/pgtable.c
>> +++ b/arch/x86/mm/pgtable.c
>> @@ -710,7 +710,7 @@ int pmd_clear_huge(pmd_t *pmd)
>>    * 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)
> 
> Please update the function header as well.  Same for pmd.
> 
> + * @addr: Virtual address associated with pud.
Noted.
> 
> --- a/include/asm-generic/pgtable.h
>> +++ b/include/asm-generic/pgtable.h
>> @@ -983,8 +983,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)
>>   {
>> @@ -1010,11 +1010,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(pud_t *pmd)
>> +static inline int pmd_free_pte_page(pud_t *pmd, unsigned long addr)
> 
> Please base your patches on top of the mm tree or at least pick up
> 8cab93de994f6 in the mm tree.  Andrew fixed the typo; the 'pud_t' above
> should have been 'pmd_t'.

Right. Noted.

> 
> Thanks,
> -Toshi
> 

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

[PATCH v3 2/3] arm64: Implement page table free interfaces

[Chintan Pandya]

Implement pud_free_pmd_page() and pmd_free_pte_page().

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

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

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index da98828..c70f139 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)
@@ -975,10 +976,35 @@ int pmd_clear_huge(pmd_t *pmdp)
 
 int pud_free_pmd_page(pud_t *pud, unsigned long addr)
 {
-	return pud_none(*pud);
+	pmd_t *pmd;
+	int i;
+
+	pmd = __va(pud_val(*pud));
+	if (pud_val(*pud)) {
+		for (i = 0; i < PTRS_PER_PMD; i++)
+			pmd_free_pte_page(&pmd[i], addr + (i * PMD_SIZE));
+
+		free_page((unsigned long) pmd);
+		pud_clear(pud);
+		flush_tlb_kernel_range(addr, addr + PUD_SIZE);
+	}
+	return 1;
 }
 
 int pmd_free_pte_page(pmd_t *pmd, unsigned long addr)
 {
-	return pmd_none(*pmd);
+	if (pmd_val(*pmd)) {
+		free_page((unsigned long)__va(pmd_val(*pmd)));
+
+		pmd_clear(pmd);
+		/*
+		 * FIXME: __flush_tlb_pgtable(&init_mm, addr) is
+		 *        ideal candidate here, which exactly
+		 *        flushes intermediate pgtables. But,
+		 *        this is broken (evident from tests).
+		 *        So, use safe TLB op unless that is fixed.
+		 */
+		flush_tlb_kernel_range(addr, addr + PMD_SIZE);
+	}
+	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 v3 2/3] arm64: Implement page table free interfaces

[Kani, Toshi]

On Mon, 2018-03-19 at 18:10 +0530, Chintan Pandya wrote:
> Implement pud_free_pmd_page() and pmd_free_pte_page().
> 
> Implementation requires,
>  1) Freeing of the un-used next level page tables
>  2) Clearing off the current pud/pmd entry
>  3) Invalidate TLB which could have previously
>     valid but not stale entry
> 
> Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
> ---
>  arch/arm64/mm/mmu.c | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index da98828..c70f139 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)
> @@ -975,10 +976,35 @@ int pmd_clear_huge(pmd_t *pmdp)
>  
>  int pud_free_pmd_page(pud_t *pud, unsigned long addr)
>  {
> -	return pud_none(*pud);
> +	pmd_t *pmd;
> +	int i;
> +
> +	pmd = __va(pud_val(*pud));
> +	if (pud_val(*pud)) {
> +		for (i = 0; i < PTRS_PER_PMD; i++)
> +			pmd_free_pte_page(&pmd[i], addr + (i * PMD_SIZE));
> +
> +		free_page((unsigned long) pmd);

Why do you want to free this pmd page before clearing the pud entry on
this arm64 version (as it seems you intentionally changed it from the
x86 version)?  It can be reused while being pointed by the pud.  Same
for pmd.

> +		pud_clear(pud);
> +		flush_tlb_kernel_range(addr, addr + PUD_SIZE);

Since you purge the entire pud range here, do you still need to call 
pmd_free_pte_page() to purge each pmd range?  This looks very expensive.
You may want to consider if calling internal __pmd_free_pte_page()
without the purge operation works.

-Toshi

[PATCH v3 2/3] arm64: Implement page table free interfaces

[Chintan Pandya]

On 3/20/2018 12:59 AM, Kani, Toshi wrote:
> On Mon, 2018-03-19 at 18:10 +0530, Chintan Pandya wrote:
>> Implement pud_free_pmd_page() and pmd_free_pte_page().
>>
>> Implementation requires,
>>   1) Freeing of the un-used next level page tables
>>   2) Clearing off the current pud/pmd entry
>>   3) Invalidate TLB which could have previously
>>      valid but not stale entry
>>
>> Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
>> ---
>>   arch/arm64/mm/mmu.c | 30 ++++++++++++++++++++++++++++--
>>   1 file changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index da98828..c70f139 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)
>> @@ -975,10 +976,35 @@ int pmd_clear_huge(pmd_t *pmdp)
>>   
>>   int pud_free_pmd_page(pud_t *pud, unsigned long addr)
>>   {
>> -	return pud_none(*pud);
>> +	pmd_t *pmd;
>> +	int i;
>> +
>> +	pmd = __va(pud_val(*pud));
>> +	if (pud_val(*pud)) {
>> +		for (i = 0; i < PTRS_PER_PMD; i++)
>> +			pmd_free_pte_page(&pmd[i], addr + (i * PMD_SIZE));
>> +
>> +		free_page((unsigned long) pmd);
> 
> Why do you want to free this pmd page before clearing the pud entry on
> this arm64 version (as it seems you intentionally changed it from the
> x86 version)?  It can be reused while being pointed by the pud.  Same
> for pmd.
Noted.
> 
>> +		pud_clear(pud);
>> +		flush_tlb_kernel_range(addr, addr + PUD_SIZE);
> 
> Since you purge the entire pud range here, do you still need to call
> pmd_free_pte_page() to purge each pmd range?  This looks very expensive.
> You may want to consider if calling internal __pmd_free_pte_page()
> without the purge operation works.
I completely missed that. Sure, will fix this.

I will upload v4 fixing all 4 comments.
> 
> -Toshi
> 

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

[PATCH v3 3/3] Revert "arm64: Enforce BBM for huge IO/VMAP mappings"

[Chintan Pandya]

This commit 15122ee2c515a ("arm64: Enforce BBM for huge
IO/VMAP mappings") is a temporary work-around until the
issues with CONFIG_HAVE_ARCH_HUGE_VMAP gets fixed.

Revert this change as we have fixes for the issue.

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

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index c70f139..a29a684 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -935,10 +935,6 @@ 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)));
 
-	/* ioremap_page_range doesn't honour BBM */
-	if (pud_present(READ_ONCE(*pudp)))
-		return 0;
-
 	BUG_ON(phys & ~PUD_MASK);
 	set_pud(pudp, pfn_pud(__phys_to_pfn(phys), sect_prot));
 	return 1;
@@ -949,10 +945,6 @@ 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)));
 
-	/* ioremap_page_range doesn't honour BBM */
-	if (pmd_present(READ_ONCE(*pmdp)))
-		return 0;
-
 	BUG_ON(phys & ~PMD_MASK);
 	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