This series of patches re-bring huge vmap back for arm64.
Patch 1/3 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 V12->V13:
- Dropped v12 3/5 and v12 5/5
- Using existing APIs like pte_offset_kernel and pmd_offset
instead of pmd_page_vaddr & pud_page_vaddr
- Updated commit log message for 3/3
- Some other cosmetic corrections in 3/3
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 (3):
ioremap: Update pgtable free interfaces with addr
arm64: tlbflush: Introduce __flush_tlb_kernel_pgtable
arm64: Implement page table free interfaces
arch/arm64/include/asm/tlbflush.h | 7 ++++++
arch/arm64/mm/mmu.c | 48 +++++++++++++++++++++++++++++++++++----
arch/x86/mm/pgtable.c | 8 ++++---
include/asm-generic/pgtable.h | 8 +++----
lib/ioremap.c | 4 ++--
5 files changed, 62 insertions(+), 13 deletions(-)
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation
Center, Inc., is a member of Code Aurora Forum, a Linux Foundation
Collaborative Project
arm64 requires break-before-make. Originally, before
setting up new pmd/pud entry for huge mapping, in few
cases, the modifying pmd/pud entry was still valid
and pointing to next level page table as we only
clear off leaf PTE in unmap leg.
a) This was resulting into stale entry in TLBs (as few
TLBs also cache intermediate mapping for performance
reasons)
b) Also, modifying pmd/pud was the only reference to
next level page table and it was getting lost without
freeing it. So, page leaks were happening.
Implement pud_free_pmd_page() and pmd_free_pte_page() to
enforce BBM and also free the leaking page tables.
Implementation requires,
1) Clearing off the current pud/pmd entry
2) Invalidation of TLB
3) Freeing of the un-used next level page tables
Signed-off-by: Chintan Pandya <[email protected]>
---
arch/arm64/mm/mmu.c | 48 ++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 44 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 8ae5d7a..65f8627 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,51 @@ 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);
+
+ /* No-op for empty entry and WARN_ON for valid entry */
+ if (!pmd_present(pmd) || !pmd_table(pmd)) {
+ VM_WARN_ON(!pmd_table(pmd));
+ return 1;
+ }
+
+ table = pte_offset_kernel(pmdp, addr);
+ 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 *pmdp;
+ pud_t pud;
+ unsigned long next, end;
+
+ pud = READ_ONCE(*pudp);
+
+ /* No-op for empty entry and WARN_ON for valid entry */
+ if (!pud_present(pud) || !pud_table(pud)) {
+ VM_WARN_ON(!pud_table(pud));
+ return 1;
+ }
+
+ table = pmd_offset(pudp, addr);
+ pmdp = table;
+ next = addr;
+ end = addr + PUD_SIZE;
+ do {
+ pmd_free_pte_page(pmdp, next);
+ } while (pmdp++, 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
From: Chintan Pandya <[email protected]>
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.
[[email protected]: merge changes, rewrite patch description]
Fixes: 28ee90fe6048 ("x86/mm: implement free pmd/pte page interfaces")
Signed-off-by: Chintan Pandya <[email protected]>
Signed-off-by: Toshi Kani <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Joerg Roedel <[email protected]>
Cc: <[email protected]>
---
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
Add an interface to invalidate intermediate page tables
from TLB for kernel.
Signed-off-by: Chintan Pandya <[email protected]>
---
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
On Wed, Jun 06, 2018 at 12:31:21PM +0530, Chintan Pandya wrote:
> arm64 requires break-before-make. Originally, before
> setting up new pmd/pud entry for huge mapping, in few
> cases, the modifying pmd/pud entry was still valid
> and pointing to next level page table as we only
> clear off leaf PTE in unmap leg.
>
> a) This was resulting into stale entry in TLBs (as few
> TLBs also cache intermediate mapping for performance
> reasons)
> b) Also, modifying pmd/pud was the only reference to
> next level page table and it was getting lost without
> freeing it. So, page leaks were happening.
>
> Implement pud_free_pmd_page() and pmd_free_pte_page() to
> enforce BBM and also free the leaking page tables.
>
> Implementation requires,
> 1) Clearing off the current pud/pmd entry
> 2) Invalidation of TLB
> 3) Freeing of the un-used next level page tables
>
> Signed-off-by: Chintan Pandya <[email protected]>
> ---
> arch/arm64/mm/mmu.c | 48 ++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 44 insertions(+), 4 deletions(-)
Thanks, I think this looks really good now:
Reviewed-by: Will Deacon <[email protected]>
Will
On Wed, Jun 06, 2018 at 12:31:20PM +0530, Chintan Pandya wrote:
> Add an interface to invalidate intermediate page tables
> from TLB for kernel.
>
> Signed-off-by: Chintan Pandya <[email protected]>
> ---
> 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
Acked-by: Will Deacon <[email protected]>
Will
Hi Chintan,
Thanks for sticking with this. I've reviewed the series now and I'm keen
for it to land in mainline. Just a couple of things below.
On Wed, Jun 06, 2018 at 12:31:18PM +0530, Chintan Pandya wrote:
> This series of patches re-bring huge vmap back for arm64.
>
> Patch 1/3 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.
Not sure we need to target -stable, since we solved the crash by disabling
the use of huge io mappings.
> arch/arm64/include/asm/tlbflush.h | 7 ++++++
> arch/arm64/mm/mmu.c | 48 +++++++++++++++++++++++++++++++++++----
> arch/x86/mm/pgtable.c | 8 ++++---
> include/asm-generic/pgtable.h | 8 +++----
> lib/ioremap.c | 4 ++--
If you get an ack from the x86 folks, then I could take all of this via
arm64. Alternatively, now that I've reviewed the series this could happily
go via another tree (e.g. akpm).
Thanks,
Will
On Wed, Jun 06, 2018 at 12:31:19PM +0530, Chintan Pandya wrote:
> From: Chintan Pandya <[email protected]>
>
> 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.
>
> [[email protected]: merge changes, rewrite patch description]
> Fixes: 28ee90fe6048 ("x86/mm: implement free pmd/pte page interfaces")
> Signed-off-by: Chintan Pandya <[email protected]>
> Signed-off-by: Toshi Kani <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Michal Hocko <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Joerg Roedel <[email protected]>
> Cc: <[email protected]>
>
> ---
> 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(-)
Reviewed-by: Will Deacon <[email protected]>
Thanks,
Will
On 6/6/2018 9:15 PM, Will Deacon wrote:
> Hi Chintan,
Hi Will,
>
> Thanks for sticking with this. I've reviewed the series now and I'm keen
> for it to land in mainline. Just a couple of things below.
>
Thanks for all the reviews so far.
> On Wed, Jun 06, 2018 at 12:31:18PM +0530, Chintan Pandya wrote:
>> This series of patches re-bring huge vmap back for arm64.
...
>> These patches can also go into '-stable' branch (if accepted)
>> for 4.6 onwards.
>
> Not sure we need to target -stable, since we solved the crash by disabling
> the use of huge io mappings.
If disabling of huge io mappings have gone into stable trees, then I
won't push for these changes there.
>
>> arch/arm64/include/asm/tlbflush.h | 7 ++++++
>> arch/arm64/mm/mmu.c | 48 +++++++++++++++++++++++++++++++++++----
>> arch/x86/mm/pgtable.c | 8 ++++---
>> include/asm-generic/pgtable.h | 8 +++----
>> lib/ioremap.c | 4 ++--
>
> If you get an ack from the x86 folks, then I could take all of this via
> arm64. Alternatively, now that I've reviewed the series this could happily
> go via another tree (e.g. akpm).
Sure. I would wait for few days before either of them take notice of
this. If required, I will communicated with them.
>
> Thanks,
>
> Will
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
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
Hi Andrew,
On 6/6/2018 9:15 PM, Will Deacon wrote:
[...]
> On Wed, Jun 06, 2018 at 12:31:18PM +0530, Chintan Pandya wrote:
>> This series of patches re-bring huge vmap back for arm64.
>>
>> Patch 1/3 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.
>>
[...]
>
>> arch/arm64/include/asm/tlbflush.h | 7 ++++++
>> arch/arm64/mm/mmu.c | 48 +++++++++++++++++++++++++++++++++++----
>> arch/x86/mm/pgtable.c | 8 ++++---
>> include/asm-generic/pgtable.h | 8 +++----
>> lib/ioremap.c | 4 ++--
>
> If you get an ack from the x86 folks, then I could take all of this via
> arm64. Alternatively, now that I've reviewed the series this could happily
> go via another tree (e.g. akpm).
>
Would you be able to take a look at this series ?
- 1/3 has been reviewed by Will and Toshi (as it touches arm64 and x86).
- 2/3 & 3/3 are arm64 specific changes.
If you think to take these patches, great ! Otherwise, I will try to
reach-out to x86 folks for their ack.
> Thanks,
>
> Will
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
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
Hi Chintan,
On Wed, Jun 06, 2018 at 12:31:21PM +0530, Chintan Pandya wrote:
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 8ae5d7a..65f8627 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,51 @@ 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);
> +
> + /* No-op for empty entry and WARN_ON for valid entry */
> + if (!pmd_present(pmd) || !pmd_table(pmd)) {
> + VM_WARN_ON(!pmd_table(pmd));
> + return 1;
> + }
What's this VM_WARN_ON supposed to do here? If the pmd is 0, we trigger
it all the time. Did you actually mean something like:
VM_WARN_ON(!pmd_none(pmd));
or pmd_present(pmd)?
Since the comment mentions empty entry, I'd rather make it explicit:
if (pmd_none(pmd) || !pmd_table(pmd))
VM_WARN_ON(!pmd_none(pmd));
return 1;
}
Similarly for the pud_free_pmd_page():
----------8<--------------------------
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 65f86271f02b..2662937ef879 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -986,8 +986,8 @@ int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
pmd = READ_ONCE(*pmdp);
/* No-op for empty entry and WARN_ON for valid entry */
- if (!pmd_present(pmd) || !pmd_table(pmd)) {
- VM_WARN_ON(!pmd_table(pmd));
+ if (pmd_none(pmd) || !pmd_table(pmd)) {
+ VM_WARN_ON(!pmd_none(pmd));
return 1;
}
@@ -1008,8 +1008,8 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
pud = READ_ONCE(*pudp);
/* No-op for empty entry and WARN_ON for valid entry */
- if (!pud_present(pud) || !pud_table(pud)) {
- VM_WARN_ON(!pud_table(pud));
+ if (pud_none(pud) || !pud_table(pud)) {
+ VM_WARN_ON(!pud_none(pud));
return 1;
}
On Thu, Sep 20, 2018 at 06:25:29PM +0100, Catalin Marinas wrote:
> On Wed, Jun 06, 2018 at 12:31:21PM +0530, Chintan Pandya wrote:
> > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> > index 8ae5d7a..65f8627 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,51 @@ 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);
> > +
> > + /* No-op for empty entry and WARN_ON for valid entry */
> > + if (!pmd_present(pmd) || !pmd_table(pmd)) {
> > + VM_WARN_ON(!pmd_table(pmd));
> > + return 1;
> > + }
>
> What's this VM_WARN_ON supposed to do here? If the pmd is 0, we trigger
> it all the time. Did you actually mean something like:
>
> VM_WARN_ON(!pmd_none(pmd));
>
> or pmd_present(pmd)?
>
> Since the comment mentions empty entry, I'd rather make it explicit:
>
> if (pmd_none(pmd) || !pmd_table(pmd))
> VM_WARN_ON(!pmd_none(pmd));
> return 1;
> }
Ignore this, fixed in -rc4 (fac880c7d074 "arm64: fix erroneous warnings
in page freeing functions").
--
Catalin