[RFC PATCH 2/2] ARMv7: Invalidate the TLB before freeing page tables

Newer processors like Cortex-A15 may cache entries in the higher page
table levels. These cached entries are ASID-tagged and are invalidated
during normal TLB operations.

When a level 2 (pte) page table is removed, the current code sequence
first clears the level 1 (pmd) entry, flushes the cache, frees the level
2 table and then invalidates the TLB. Because of the caching of the
higher page table entries, the processor may speculatively create a TLB
entry after the level 2 page table has been freed but before the TLB
invalidation. If such speculative PTW accesses random data, it could
create a global TLB entry that gets used for subsequent user space
accesses.

The patch ensures that the TLB is invalidated before the page table is
freed (pte_free_tlb). Since pte_free_tlb() does not get a vma structure,
the patch also introduces flush_tlb_user_page() which takes an mm_struct
rather than vma_struct. The original flush_tlb_page() is implemented as
a call to flush_tlb_user_page().

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm/include/asm/tlb.h      |   17 +++++++++++++++--
 arch/arm/include/asm/tlbflush.h |   16 +++++++++++-----
 arch/arm/kernel/smp_tlb.c       |   11 ++++++-----
 3 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/arch/arm/include/asm/tlb.h b/arch/arm/include/asm/tlb.h
index f41a6f5..565403a 100644
--- a/arch/arm/include/asm/tlb.h
+++ b/arch/arm/include/asm/tlb.h

@@ -102,8 +102,21 @@ tlb_end_vma(struct mmu_gather *tlb, struct vm_area_struct *vma)
 }
 
 #define tlb_remove_page(tlb,page)	free_page_and_swap_cache(page)
-#define pte_free_tlb(tlb, ptep, addr)	pte_free((tlb)->mm, ptep)
-#define pmd_free_tlb(tlb, pmdp, addr)	pmd_free((tlb)->mm, pmdp)
+
+static inline void pte_free_tlb(struct mmu_gather *tlb, pgtable_t pte,
+				unsigned long addr)
+{
+#if __LINUX_ARM_ARCH__ >= 7
+	flush_tlb_user_page(tlb->mm, addr);
+#endif
+	pte_free(tlb->mm, pte);
+}
+
+static inline void pmd_free_tlb(struct mmu_gather *tlb, pmd_t *pmdp,
+				unsigned long addr)
+{
+	pmd_free(tlb->mm, pmdp);
+}
 
 #define tlb_migrate_finish(mm)		do { } while (0)
 

diff --git a/arch/arm/include/asm/tlbflush.h b/arch/arm/include/asm/tlbflush.h
index ce7378e..7bd9c52cd 100644
--- a/arch/arm/include/asm/tlbflush.h
+++ b/arch/arm/include/asm/tlbflush.h

@@ -408,17 +408,17 @@ static inline void local_flush_tlb_mm(struct mm_struct *mm)
 }
 
 static inline void
-local_flush_tlb_page(struct vm_area_struct *vma, unsigned long uaddr)
+local_flush_tlb_user_page(struct mm_struct *mm, unsigned long uaddr)
 {
 	const int zero = 0;
 	const unsigned int __tlb_flag = __cpu_tlb_flags;
 
-	uaddr = (uaddr & PAGE_MASK) | ASID(vma->vm_mm);
+	uaddr = (uaddr & PAGE_MASK) | ASID(mm);
 
 	if (tlb_flag(TLB_WB))
 		dsb();
 
-	if (cpumask_test_cpu(smp_processor_id(), mm_cpumask(vma->vm_mm))) {
+	if (cpumask_test_cpu(smp_processor_id(), mm_cpumask(mm))) {
 		if (tlb_flag(TLB_V3_PAGE))
 			asm("mcr p15, 0, %0, c6, c0, 0" : : "r" (uaddr) : "cc");
 		if (tlb_flag(TLB_V4_U_PAGE))

@@ -556,19 +556,25 @@ static inline void clean_pmd_entry(pmd_t *pmd)
 #ifndef CONFIG_SMP
 #define flush_tlb_all		local_flush_tlb_all
 #define flush_tlb_mm		local_flush_tlb_mm
-#define flush_tlb_page		local_flush_tlb_page
+#define flush_tlb_user_page	local_flush_tlb_user_page
 #define flush_tlb_kernel_page	local_flush_tlb_kernel_page
 #define flush_tlb_range		local_flush_tlb_range
 #define flush_tlb_kernel_range	local_flush_tlb_kernel_range
 #else
 extern void flush_tlb_all(void);
 extern void flush_tlb_mm(struct mm_struct *mm);
-extern void flush_tlb_page(struct vm_area_struct *vma, unsigned long uaddr);
+extern void flush_tlb_user_page(struct mm_struct *mm, unsigned long uaddr);
 extern void flush_tlb_kernel_page(unsigned long kaddr);
 extern void flush_tlb_range(struct vm_area_struct *vma, unsigned long start, unsigned long end);
 extern void flush_tlb_kernel_range(unsigned long start, unsigned long end);
 #endif
 
+static inline void
+flush_tlb_page(struct vm_area_struct *vma, unsigned long uaddr)
+{
+	flush_tlb_user_page(vma->vm_mm, uaddr);
+}
+
 /*
  * If PG_dcache_clean is not set for the page, we need to ensure that any
  * cache entries for the kernels virtual memory range are written

diff --git a/arch/arm/kernel/smp_tlb.c b/arch/arm/kernel/smp_tlb.c
index 7dcb352..8f57f32 100644
--- a/arch/arm/kernel/smp_tlb.c
+++ b/arch/arm/kernel/smp_tlb.c

@@ -32,6 +32,7 @@ static void on_each_cpu_mask(void (*func)(void *), void *info, int wait,
  */
 struct tlb_args {
 	struct vm_area_struct *ta_vma;
+	struct mm_struct *ta_mm;
 	unsigned long ta_start;
 	unsigned long ta_end;
 };

@@ -52,7 +53,7 @@ static inline void ipi_flush_tlb_page(void *arg)
 {
 	struct tlb_args *ta = (struct tlb_args *)arg;
 
-	local_flush_tlb_page(ta->ta_vma, ta->ta_start);
+	local_flush_tlb_user_page(ta->ta_mm, ta->ta_start);
 }
 
 static inline void ipi_flush_tlb_kernel_page(void *arg)

@@ -92,15 +93,15 @@ void flush_tlb_mm(struct mm_struct *mm)
 		local_flush_tlb_mm(mm);
 }
 
-void flush_tlb_page(struct vm_area_struct *vma, unsigned long uaddr)
+void flush_tlb_user_page(struct mm_struct *mm, unsigned long uaddr)
 {
 	if (tlb_ops_need_broadcast()) {
 		struct tlb_args ta;
-		ta.ta_vma = vma;
+		ta.ta_mm = mm;
 		ta.ta_start = uaddr;
-		on_each_cpu_mask(ipi_flush_tlb_page, &ta, 1, mm_cpumask(vma->vm_mm));
+		on_each_cpu_mask(ipi_flush_tlb_page, &ta, 1, mm_cpumask(mm));
 	} else
-		local_flush_tlb_page(vma, uaddr);
+		local_flush_tlb_user_page(mm, uaddr);
 }
 
 void flush_tlb_kernel_page(unsigned long kaddr)

On Mon, Feb 14, 2011 at 05:39:58PM +0000, Catalin Marinas wrote:

Newer processors like Cortex-A15 may cache entries in the higher page
table levels. These cached entries are ASID-tagged and are invalidated
during normal TLB operations.

When a level 2 (pte) page table is removed, the current code sequence
first clears the level 1 (pmd) entry, flushes the cache, frees the level
2 table and then invalidates the TLB. Because of the caching of the
higher page table entries, the processor may speculatively create a TLB
entry after the level 2 page table has been freed but before the TLB
invalidation. If such speculative PTW accesses random data, it could
create a global TLB entry that gets used for subsequent user space
accesses.

The patch ensures that the TLB is invalidated before the page table is
freed (pte_free_tlb). Since pte_free_tlb() does not get a vma structure,
the patch also introduces flush_tlb_user_page() which takes an mm_struct
rather than vma_struct. The original flush_tlb_page() is implemented as
a call to flush_tlb_user_page().

We already have support for doing this, and Peter Zijlstra posted patches
to convert ARM to use a generic implementation of the TLB shootdown code.

http://marc.info/?l=linux-kernel&m=129604765010347&w=2

Does this patch solve your problem?

On Tue, 2011-02-15 at 10:31 +0000, Russell King - ARM Linux wrote:

On Mon, Feb 14, 2011 at 05:39:58PM +0000, Catalin Marinas wrote:

quoted

Newer processors like Cortex-A15 may cache entries in the higher page
table levels. These cached entries are ASID-tagged and are invalidated
during normal TLB operations.

When a level 2 (pte) page table is removed, the current code sequence
first clears the level 1 (pmd) entry, flushes the cache, frees the level
2 table and then invalidates the TLB. Because of the caching of the
higher page table entries, the processor may speculatively create a TLB
entry after the level 2 page table has been freed but before the TLB
invalidation. If such speculative PTW accesses random data, it could
create a global TLB entry that gets used for subsequent user space
accesses.

The patch ensures that the TLB is invalidated before the page table is
freed (pte_free_tlb). Since pte_free_tlb() does not get a vma structure,
the patch also introduces flush_tlb_user_page() which takes an mm_struct
rather than vma_struct. The original flush_tlb_page() is implemented as
a call to flush_tlb_user_page().

We already have support for doing this, and Peter Zijlstra posted patches
to convert ARM to use a generic implementation of the TLB shootdown code.

http://marc.info/?l=linux-kernel&m=129604765010347&w=2

Does this patch solve your problem?

I don't think it does. Peter's patch moves the ARM TLB support to the
generic one which is a good clean-up, however it doesn't look like
anything is invalidating the TLB entry between pmd_clear() and
pte_free(), only after. This is too late because we may speculatively
get a global TLB entry (which isn't invalidated by the ASID TLB
operations). So with Peter's patch we still have to implement
__pte_free_tlb().

An alternative would be that flush_tlb_page() flushes all the ASIDs for
the corresponding user address and this would include any speculatively
fetched global TLB entries.

-- 
Catalin

On Tue, Feb 15, 2011 at 11:02:28AM +0000, Catalin Marinas wrote:

On Tue, 2011-02-15 at 10:31 +0000, Russell King - ARM Linux wrote:

quoted

We already have support for doing this, and Peter Zijlstra posted patches
to convert ARM to use a generic implementation of the TLB shootdown code.

http://marc.info/?l=linux-kernel&m=129604765010347&w=2

Does this patch solve your problem?

I don't think it does. Peter's patch moves the ARM TLB support to the
generic one which is a good clean-up, however it doesn't look like
anything is invalidating the TLB entry between pmd_clear() and
pte_free(), only after.

Can we please stop thinking "ooh, lets use generic code, that's good" ?
It's a rediculous attitude.  The reason we have our own version is because
the generic version is too heavy for our UP implementations and it doesn't
do what we need to do there.

I believe we should stick with our own version for UP, and switch over to
the generic version for SMP.

This is too late because we may speculatively
get a global TLB entry (which isn't invalidated by the ASID TLB
operations). So with Peter's patch we still have to implement
__pte_free_tlb().

The point of TLB shootdown is that we unmap the entries from the page
tables, then issue the TLB flushes, and then free the pages and page
tables after that.  All that Peter's patch tries to do is to get ARM to
use the generic stuff.

On Tue, Feb 15, 2011 at 11:32:42AM +0000, Russell King - ARM Linux wrote:

The point of TLB shootdown is that we unmap the entries from the page
tables, then issue the TLB flushes, and then free the pages and page
tables after that.  All that Peter's patch tries to do is to get ARM to
use the generic stuff.

As Peter's patch preserves the current behaviour, that's not sufficient.
So, let's do this our own way and delay pages and page table frees on
ARMv6 and v7.  Untested.

Note that the generic code doesn't allow us to delay frees on UP as it
assumes that if there's no TLB entry, the CPU won't speculatively
prefetch.  This seems to be where ARM differs from the rest of the
planet.  Please confirm that this is indeed the case.

 arch/arm/include/asm/tlb.h |   79 +++++++++++++++++++++++++++++++++++++-------
 1 files changed, 67 insertions(+), 12 deletions(-)

diff --git a/arch/arm/include/asm/tlb.h b/arch/arm/include/asm/tlb.h
index f41a6f5..1ca3e16 100644
--- a/arch/arm/include/asm/tlb.h
+++ b/arch/arm/include/asm/tlb.h

@@ -30,6 +30,16 @@
 #include <asm/pgalloc.h>
 
 /*
+ * As v6 and v7 speculatively prefetch, which can drag new entries into the
+ * TLB, we need to delay freeing pages and page tables.
+ */
+#if defined(CONFIG_CPU_32v6) || defined(CONFIG_CPU_32v7)
+#define tlb_fast_mode(tlb)	0
+#else
+#define tlb_fast_mode(tlb)	1
+#endif
+
+/*
  * TLB handling.  This allows us to remove pages from the page
  * tables, and efficiently handle the TLB issues.
  */

@@ -38,10 +48,42 @@ struct mmu_gather {
 	unsigned int		fullmm;
 	unsigned long		range_start;
 	unsigned long		range_end;
+	unsigned int		nr;
+	struct page		*pages[FREE_PTE_NR];
 };
 
 DECLARE_PER_CPU(struct mmu_gather, mmu_gathers);
 
+static inline void tlb_flush(struct mmu_gather *tlb)
+{
+	if (tlb->fullmm)
+		flush_tlb_mm(tlb->mm);
+	else if (tlb->range_end > 0) {
+		flush_tlb_range(vma, tlb->range_start, tlb->range_end);
+		tlb->range_start = TASK_SIZE;
+		tlb->range_end = 0;
+	}
+}
+
+static inline void tlb_add_flush(struct mmu_gather *tlb, unsigned long addr)
+{
+	if (!tlb->fullmm) {
+		if (addr < tlb->range_start)
+			tlb->range_start = addr;
+		if (addr + PAGE_SIZE > tlb->range_end)
+			tlb->range_end = addr + PAGE_SIZE;
+	}
+}
+
+static inline void tlb_flush_mmu(struct mmu_gather *tlb)
+{
+	tlb_flush(tlb);
+	if (!tlb_fast_mode(tlb)) {
+		free_pages_and_swap_cache(tlb->pages, tlb->nr);
+		tlb->nr = 0;
+	}
+}
+
 static inline struct mmu_gather *
 tlb_gather_mmu(struct mm_struct *mm, unsigned int full_mm_flush)
 {

@@ -49,6 +91,7 @@ tlb_gather_mmu(struct mm_struct *mm, unsigned int full_mm_flush)
 
 	tlb->mm = mm;
 	tlb->fullmm = full_mm_flush;
+	tlb->nr = 0;
 
 	return tlb;
 }

@@ -56,8 +99,7 @@ tlb_gather_mmu(struct mm_struct *mm, unsigned int full_mm_flush)
 static inline void
 tlb_finish_mmu(struct mmu_gather *tlb, unsigned long start, unsigned long end)
 {
-	if (tlb->fullmm)
-		flush_tlb_mm(tlb->mm);
+	tlb_flush_mmu(tlb);
 
 	/* keep the page table cache within bounds */
 	check_pgt_cache();

@@ -71,12 +113,7 @@ tlb_finish_mmu(struct mmu_gather *tlb, unsigned long start, unsigned long end)
 static inline void
 tlb_remove_tlb_entry(struct mmu_gather *tlb, pte_t *ptep, unsigned long addr)
 {
-	if (!tlb->fullmm) {
-		if (addr < tlb->range_start)
-			tlb->range_start = addr;
-		if (addr + PAGE_SIZE > tlb->range_end)
-			tlb->range_end = addr + PAGE_SIZE;
-	}
+	tlb_add_flush(tlb, addr);
 }
 
 /*

@@ -97,12 +134,30 @@ tlb_start_vma(struct mmu_gather *tlb, struct vm_area_struct *vma)
 static inline void
 tlb_end_vma(struct mmu_gather *tlb, struct vm_area_struct *vma)
 {
-	if (!tlb->fullmm && tlb->range_end > 0)
-		flush_tlb_range(vma, tlb->range_start, tlb->range_end);
+	if (!tlb->fullmm)
+		tlb_flush(tlb);
+}
+
+static inline void tlb_remove_page(struct mmu_gather *tlb, struct page *page)
+{
+	if (tlb_fast_mode(tlb)) {
+		free_page_and_swap_cache(page);
+	} else {
+		tlb->pages[tlb->nr++] = page;
+		if (tlb->nr >= FREE_PTE_NR)
+			tlb_flush_mmu(tlb);
+	}
+}
+
+static inline void __pte_free_tlb(struct mmu_gather *tlb, pgtable_t pte,
+	unsigned long addr)
+{
+	pgtable_page_dtor(pte);
+	tlb_add_flush(addr);
+	tlb_remove_page(tlb, pte);
 }
 
-#define tlb_remove_page(tlb,page)	free_page_and_swap_cache(page)
-#define pte_free_tlb(tlb, ptep, addr)	pte_free((tlb)->mm, ptep)
+#define pte_free_tlb(tlb, ptep, addr)	__pte_free_tlb(tlb, ptep, addr)
 #define pmd_free_tlb(tlb, pmdp, addr)	pmd_free((tlb)->mm, pmdp)
 
 #define tlb_migrate_finish(mm)		do { } while (0)

On Tue, 2011-02-15 at 11:32 +0000, Russell King - ARM Linux wrote:

On Tue, Feb 15, 2011 at 11:02:28AM +0000, Catalin Marinas wrote:

quoted

On Tue, 2011-02-15 at 10:31 +0000, Russell King - ARM Linux wrote:

quoted

We already have support for doing this, and Peter Zijlstra posted patches
to convert ARM to use a generic implementation of the TLB shootdown code.

http://marc.info/?l=linux-kernel&m=129604765010347&w=2

Does this patch solve your problem?

I don't think it does. Peter's patch moves the ARM TLB support to the
generic one which is a good clean-up, however it doesn't look like
anything is invalidating the TLB entry between pmd_clear() and
pte_free(), only after.

Can we please stop thinking "ooh, lets use generic code, that's good" ?
It's a rediculous attitude.  The reason we have our own version is because
the generic version is too heavy for our UP implementations and it doesn't
do what we need to do there.

I believe we should stick with our own version for UP, and switch over to
the generic version for SMP.

Whatever we do, the issue with A15 is valid for both UP and SMP since we
can get an interrupt which allocates a page previously hosting a pte but
before the TLB was invalidated.

I'll follow up on your patch as well.

-- 
Catalin

On Tue, 2011-02-15 at 12:14 +0000, Russell King - ARM Linux wrote:

On Tue, Feb 15, 2011 at 11:32:42AM +0000, Russell King - ARM Linux wrote:

quoted

The point of TLB shootdown is that we unmap the entries from the page
tables, then issue the TLB flushes, and then free the pages and page
tables after that.  All that Peter's patch tries to do is to get ARM to
use the generic stuff.

As Peter's patch preserves the current behaviour, that's not sufficient.
So, let's do this our own way and delay pages and page table frees on
ARMv6 and v7.  Untested.

ARMv7 should be enough, I'm not aware of any pre-v7 with this behaviour.

Note that the generic code doesn't allow us to delay frees on UP as it
assumes that if there's no TLB entry, the CPU won't speculatively
prefetch.  This seems to be where ARM differs from the rest of the
planet.  Please confirm that this is indeed the case.

The CPU can speculatively prefetch instructions and access data as long
as there is a valid mapping in the page tables. There is no need to have
a TLB entry for the speculative access, this can be created
speculatively from existing page table entries. That's not the issue
(ARM has been doing this for ages, probably other architectures too).

With newer cores, apart from the TLB (which stores a virtual to physical
translation), the CPU is allowed to cache entries in the higher page
table levels. This is important especially for LPAE where the 1st level
covers 1GB and can be easily cached to avoid 3 levels of page table walk
(or 2 levels for the classic page tables).

So even when we clear a page table entry in RAM (pmd_clear), the
processor still has it in its page table cache (pretty much part of the
TLB, different from the D-cache) and that's why we need the TLB
invalidation before freeing the lower page table.

We could make this v7 only. If you want it to be more dynamic, we can
check the MMFR0[3:0] bits (Cortex-A15 sets them to 4). But
architecturally we should assume that intermediate page table levels may
be cached.

-#define tlb_remove_page(tlb,page)      free_page_and_swap_cache(page)
-#define pte_free_tlb(tlb, ptep, addr)  pte_free((tlb)->mm, ptep)
+#define pte_free_tlb(tlb, ptep, addr)  __pte_free_tlb(tlb, ptep, addr)
 #define pmd_free_tlb(tlb, pmdp, addr)  pmd_free((tlb)->mm, pmdp)

With LPAE, we'll need a __pmd_free_tlb() but I can add this as part of
my patches.

Apart from the need for ARMv6, the patch looks fine (I'll give it a try
as well).

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

On Tue, Feb 15, 2011 at 02:42:06PM +0000, Catalin Marinas wrote:

On Tue, 2011-02-15 at 12:14 +0000, Russell King - ARM Linux wrote:

quoted

On Tue, Feb 15, 2011 at 11:32:42AM +0000, Russell King - ARM Linux wrote:

quoted

The point of TLB shootdown is that we unmap the entries from the page
tables, then issue the TLB flushes, and then free the pages and page
tables after that.  All that Peter's patch tries to do is to get ARM to
use the generic stuff.

As Peter's patch preserves the current behaviour, that's not sufficient.
So, let's do this our own way and delay pages and page table frees on
ARMv6 and v7.  Untested.

ARMv7 should be enough, I'm not aware of any pre-v7 with this behaviour.

ARM11MPCore.  Any SMP system can access a page which was free'd by the
tlb code but hasn't been flushed from the hardware TLBs.  So maybe we
want it to be "defined(CONFIG_SMP) || defined(CONFIG_CPU_32v7)" ?

quoted

diff --git a/arch/arm/include/asm/tlb.h b/arch/arm/include/asm/tlb.h
index f41a6f5..1ca3e16 100644
--- a/arch/arm/include/asm/tlb.h
+++ b/arch/arm/include/asm/tlb.h

@@ -30,6 +30,16 @@
 #include <asm/pgalloc.h>

 /*
+ * As v6 and v7 speculatively prefetch, which can drag new entries into the
+ * TLB, we need to delay freeing pages and page tables.
+ */
+#if defined(CONFIG_CPU_32v6) || defined(CONFIG_CPU_32v7)
+#define tlb_fast_mode(tlb)     0
+#else
+#define tlb_fast_mode(tlb)     1
+#endif

We could make this v7 only. If you want it to be more dynamic, we can
check the MMFR0[3:0] bits (Cortex-A15 sets them to 4). But
architecturally we should assume that intermediate page table levels may
be cached.

I don't think that a runtime check justifies the optimization.  We're
talking about the difference between storing a set of pages in an array
and freeing them later vs freeing them one at a time.  Doing a test per
page is probably more expensive than just storing them in an array.

quoted

-#define tlb_remove_page(tlb,page)      free_page_and_swap_cache(page)
-#define pte_free_tlb(tlb, ptep, addr)  pte_free((tlb)->mm, ptep)
+#define pte_free_tlb(tlb, ptep, addr)  __pte_free_tlb(tlb, ptep, addr)
 #define pmd_free_tlb(tlb, pmdp, addr)  pmd_free((tlb)->mm, pmdp)

With LPAE, we'll need a __pmd_free_tlb() but I can add this as part of
my patches.

Yes.

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Thanks.

On 20 February 2011 12:12, Russell King - ARM Linux
[off-list ref] wrote:

On Tue, Feb 15, 2011 at 02:42:06PM +0000, Catalin Marinas wrote:

quoted

On Tue, 2011-02-15 at 12:14 +0000, Russell King - ARM Linux wrote:

quoted

On Tue, Feb 15, 2011 at 11:32:42AM +0000, Russell King - ARM Linux wrote:

quoted

The point of TLB shootdown is that we unmap the entries from the page
tables, then issue the TLB flushes, and then free the pages and page
tables after that. ?All that Peter's patch tries to do is to get ARM to
use the generic stuff.

As Peter's patch preserves the current behaviour, that's not sufficient.
So, let's do this our own way and delay pages and page table frees on
ARMv6 and v7. ?Untested.

ARMv7 should be enough, I'm not aware of any pre-v7 with this behaviour.

ARM11MPCore. ?Any SMP system can access a page which was free'd by the
tlb code but hasn't been flushed from the hardware TLBs. ?So maybe we
want it to be "defined(CONFIG_SMP) || defined(CONFIG_CPU_32v7)" ?

In practice, since the hardware TLB does not store higher level
entries on existing v6 cores, there is no cached value pointing to the
freed pte page. In theory, we first clear the pmd entry but another
CPU could be doing a PTW at the same time and had already read the pmd
before being cleared. But the timing constraints are difficult to
reproduce in practice.

Anyway, I'm ok with your original patch or the CONFIG_SMP case you
mentioned above. Whichever you prefer.

-- 
Catalin

On Mon, Feb 21, 2011 at 09:39:32AM +0000, Catalin Marinas wrote:

On 20 February 2011 12:12, Russell King - ARM Linux
[off-list ref] wrote:

quoted

On Tue, Feb 15, 2011 at 02:42:06PM +0000, Catalin Marinas wrote:

quoted

On Tue, 2011-02-15 at 12:14 +0000, Russell King - ARM Linux wrote:

quoted

On Tue, Feb 15, 2011 at 11:32:42AM +0000, Russell King - ARM Linux wrote:

quoted

The point of TLB shootdown is that we unmap the entries from the page
tables, then issue the TLB flushes, and then free the pages and page
tables after that. ?All that Peter's patch tries to do is to get ARM to
use the generic stuff.

As Peter's patch preserves the current behaviour, that's not sufficient.
So, let's do this our own way and delay pages and page table frees on
ARMv6 and v7. ?Untested.

ARMv7 should be enough, I'm not aware of any pre-v7 with this behaviour.

ARM11MPCore. ?Any SMP system can access a page which was free'd by the
tlb code but hasn't been flushed from the hardware TLBs. ?So maybe we
want it to be "defined(CONFIG_SMP) || defined(CONFIG_CPU_32v7)" ?

In practice, since the hardware TLB does not store higher level
entries on existing v6 cores, there is no cached value pointing to the
freed pte page.

It's not about cached values of PTE pointers.

In theory, we first clear the pmd entry but another
CPU could be doing a PTW at the same time and had already read the pmd
before being cleared. But the timing constraints are difficult to
reproduce in practice.

I don't think you properly understand the problem.

CPU#0 is unmapping page tables, eg due to munmap(), mremap(), etc.
CPU#1 is running a thread, and has TLB entries for the region being unmapped.

	CPU#0				CPU#1
	clear page table entry
	frees page
	loop continues
					accesses page
	...
	sometime in the future
	invalidates TLB

The point here is that user threads on CPU#1 should not have access to
a page which has been freed back into the pool, no matter how slim the
possibility of hitting such a condition.  What if a thread on CPU#2 is
given that free page which CPU#1 still has access to, and CPU#2 stores
your SSH private key there?

On 21 February 2011 10:30, Russell King - ARM Linux
[off-list ref] wrote:

On Mon, Feb 21, 2011 at 09:39:32AM +0000, Catalin Marinas wrote:

quoted

On 20 February 2011 12:12, Russell King - ARM Linux
[off-list ref] wrote:

quoted

On Tue, Feb 15, 2011 at 02:42:06PM +0000, Catalin Marinas wrote:

quoted

On Tue, 2011-02-15 at 12:14 +0000, Russell King - ARM Linux wrote:

quoted

On Tue, Feb 15, 2011 at 11:32:42AM +0000, Russell King - ARM Linux wrote:

quoted

The point of TLB shootdown is that we unmap the entries from the page
tables, then issue the TLB flushes, and then free the pages and page
tables after that. ?All that Peter's patch tries to do is to get ARM to
use the generic stuff.

As Peter's patch preserves the current behaviour, that's not sufficient.
So, let's do this our own way and delay pages and page table frees on
ARMv6 and v7. ?Untested.

ARMv7 should be enough, I'm not aware of any pre-v7 with this behaviour.

ARM11MPCore. ?Any SMP system can access a page which was free'd by the
tlb code but hasn't been flushed from the hardware TLBs. ?So maybe we
want it to be "defined(CONFIG_SMP) || defined(CONFIG_CPU_32v7)" ?

In practice, since the hardware TLB does not store higher level
entries on existing v6 cores, there is no cached value pointing to the
freed pte page.

It's not about cached values of PTE pointers.

My point is about cached values in the 1st level of page tables
pointing to the second level and the freeing of the 2nd level of page
tables.

I think we talk about two different cases (but could be fixable with
the same patch). Your scenario is valid as well, just different.

quoted

In theory, we first clear the pmd entry but another
CPU could be doing a PTW at the same time and had already read the pmd
before being cleared. But the timing constraints are difficult to
reproduce in practice.

I don't think you properly understand the problem.

CPU#0 is unmapping page tables, eg due to munmap(), mremap(), etc.
CPU#1 is running a thread, and has TLB entries for the region being unmapped.

? ? ? ?CPU#0 ? ? ? ? ? ? ? ? ? ? ? ? ? CPU#1
? ? ? ?clear page table entry
? ? ? ?frees page
? ? ? ?loop continues
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?accesses page
? ? ? ?...
? ? ? ?sometime in the future
? ? ? ?invalidates TLB

For this scenario we only need to implement tlb_remove_page() to
invalidate the TLB before releasing the page (called via
zap_pte_range). We currently delay the TLB operation until
tlb_end_vma() which happens after the page was released.

For my scenario, we also need pte_free_tlb() because of the pmd
caching in the TLB.

So your patch fixes both. My ack is still valid.

-- 
Catalin

On Mon, Feb 21, 2011 at 11:04:22AM +0000, Catalin Marinas wrote:

On 21 February 2011 10:30, Russell King - ARM Linux
[off-list ref] wrote:

quoted

On Mon, Feb 21, 2011 at 09:39:32AM +0000, Catalin Marinas wrote:

quoted

On 20 February 2011 12:12, Russell King - ARM Linux
[off-list ref] wrote:

quoted

On Tue, Feb 15, 2011 at 02:42:06PM +0000, Catalin Marinas wrote:

quoted

On Tue, 2011-02-15 at 12:14 +0000, Russell King - ARM Linux wrote:

quoted

On Tue, Feb 15, 2011 at 11:32:42AM +0000, Russell King - ARM Linux wrote:

quoted

The point of TLB shootdown is that we unmap the entries from the page
tables, then issue the TLB flushes, and then free the pages and page
tables after that. ?All that Peter's patch tries to do is to get ARM to
use the generic stuff.

As Peter's patch preserves the current behaviour, that's not sufficient.
So, let's do this our own way and delay pages and page table frees on
ARMv6 and v7. ?Untested.

ARMv7 should be enough, I'm not aware of any pre-v7 with this behaviour.

ARM11MPCore. ?Any SMP system can access a page which was free'd by the
tlb code but hasn't been flushed from the hardware TLBs. ?So maybe we
want it to be "defined(CONFIG_SMP) || defined(CONFIG_CPU_32v7)" ?

In practice, since the hardware TLB does not store higher level
entries on existing v6 cores, there is no cached value pointing to the
freed pte page.

It's not about cached values of PTE pointers.

My point is about cached values in the 1st level of page tables
pointing to the second level and the freeing of the 2nd level of page
tables.

Your point is about parts of the system keeping pointers to page tables
around.  This is no different from keeping pointers to pages themselves
around.  This is not something new.  This is something well known on
architectures such as x86.

I think we talk about two different cases (but could be fixable with
the same patch). Your scenario is valid as well, just different.

No, it's the same.  The way the hardware achieves it may be different
but from the software point of view it's the same.

quoted

quoted

In theory, we first clear the pmd entry but another
CPU could be doing a PTW at the same time and had already read the pmd
before being cleared. But the timing constraints are difficult to
reproduce in practice.

I don't think you properly understand the problem.

CPU#0 is unmapping page tables, eg due to munmap(), mremap(), etc.
CPU#1 is running a thread, and has TLB entries for the region being unmapped.

? ? ? ?CPU#0 ? ? ? ? ? ? ? ? ? ? ? ? ? CPU#1
? ? ? ?clear page table entry
? ? ? ?frees page
? ? ? ?loop continues
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?accesses page
? ? ? ?...
? ? ? ?sometime in the future
? ? ? ?invalidates TLB

For this scenario we only need to implement tlb_remove_page() to
invalidate the TLB before releasing the page (called via
zap_pte_range). We currently delay the TLB operation until
tlb_end_vma() which happens after the page was released.

Do you not understand that the TLB shootdown code is supposed to make this
more efficient than having to issue a broadcasted TLB invalidate for every
page as we remove each one in sequence?

You should look at include/asm-generic/tlb.h and arch/x86/include/asm/tlb.h
to see how the majority of other architectures handle this stuff...

Anyway, patch committed.

On 15 February 2011 12:14, Russell King - ARM Linux
[off-list ref] wrote:

[...]

+static inline void tlb_add_flush(struct mmu_gather *tlb, unsigned long addr)
+{
+ ? ? ? if (!tlb->fullmm) {
+ ? ? ? ? ? ? ? if (addr < tlb->range_start)
+ ? ? ? ? ? ? ? ? ? ? ? tlb->range_start = addr;
+ ? ? ? ? ? ? ? if (addr + PAGE_SIZE > tlb->range_end)
+ ? ? ? ? ? ? ? ? ? ? ? tlb->range_end = addr + PAGE_SIZE;
+ ? ? ? }
+}

[...]

+static inline void __pte_free_tlb(struct mmu_gather *tlb, pgtable_t pte,
+ ? ? ? unsigned long addr)
+{
+ ? ? ? pgtable_page_dtor(pte);
+ ? ? ? tlb_add_flush(addr);
+ ? ? ? tlb_remove_page(tlb, pte);
?}

The above call to tlb_add_flush() would only add a PAGE_SIZE. But
since we free an entire PTE, shouldn't the range cover addr ..
addr+PTRS_PER_PTE*PAGE_SIZE?

-- 
Catalin

On Wed, Mar 09, 2011 at 03:40:05PM +0000, Catalin Marinas wrote:

The above call to tlb_add_flush() would only add a PAGE_SIZE. But
since we free an entire PTE, shouldn't the range cover addr ..
addr+PTRS_PER_PTE*PAGE_SIZE?

Why do we need to?  We're not flushing away the individual PTE entries
when we remove an entire page table - we will have already walked the
page table removing those entries, which will already have been added.

This code is to cover the case with LPAE where we need to flush out the
L1 entries.  It's nothing to do with the TLB itself.

On Wed, 2011-03-09 at 18:35 +0000, Russell King - ARM Linux wrote:

On Wed, Mar 09, 2011 at 03:40:05PM +0000, Catalin Marinas wrote:

quoted

The above call to tlb_add_flush() would only add a PAGE_SIZE. But
since we free an entire PTE, shouldn't the range cover addr ..
addr+PTRS_PER_PTE*PAGE_SIZE?

Why do we need to?  We're not flushing away the individual PTE entries
when we remove an entire page table - we will have already walked the
page table removing those entries, which will already have been added.

Ah, I missed the fact that tlb_flush() invalidates the whole TLB when
there is no tlb->vma (the shift_arg_pages case). We could optimise this
to add the range covered by the PTE page and avoid the !tlb->vma check
(and a flush_tlb_mm), though not sure it's worth.

This code is to cover the case with LPAE where we need to flush out the
L1 entries.  It's nothing to do with the TLB itself.

This would happen even with the classic page tables on Cortex-A15, L1
entries are cached.

-- 
Catalin

On Fri, Mar 11, 2011 at 05:32:58PM +0000, Catalin Marinas wrote:

On Wed, 2011-03-09 at 18:35 +0000, Russell King - ARM Linux wrote:

quoted

On Wed, Mar 09, 2011 at 03:40:05PM +0000, Catalin Marinas wrote:

quoted

The above call to tlb_add_flush() would only add a PAGE_SIZE. But
since we free an entire PTE, shouldn't the range cover addr ..
addr+PTRS_PER_PTE*PAGE_SIZE?

Why do we need to?  We're not flushing away the individual PTE entries
when we remove an entire page table - we will have already walked the
page table removing those entries, which will already have been added.

Ah, I missed the fact that tlb_flush() invalidates the whole TLB when
there is no tlb->vma (the shift_arg_pages case). We could optimise this
to add the range covered by the PTE page and avoid the !tlb->vma check
(and a flush_tlb_mm), though not sure it's worth.

If we're removing pte entries then tlb->vma is non-NULL.  Please look at
the comments - I've documented the three modes of use there along with
how things are setup for each of those modes, and what we do with each.
I don't add comments just for the hell of it.

quoted

This code is to cover the case with LPAE where we need to flush out the
L1 entries.  It's nothing to do with the TLB itself.

This would happen even with the classic page tables on Cortex-A15, L1
entries are cached.

That's why its there at pte level.  On classic page tables there's no pmds
or puds to consider.

On Fri, 2011-03-11 at 19:24 +0000, Russell King - ARM Linux wrote:

On Fri, Mar 11, 2011 at 05:32:58PM +0000, Catalin Marinas wrote:

quoted

On Wed, 2011-03-09 at 18:35 +0000, Russell King - ARM Linux wrote:

quoted

On Wed, Mar 09, 2011 at 03:40:05PM +0000, Catalin Marinas wrote:

quoted

The above call to tlb_add_flush() would only add a PAGE_SIZE. But
since we free an entire PTE, shouldn't the range cover addr ..
addr+PTRS_PER_PTE*PAGE_SIZE?

Why do we need to?  We're not flushing away the individual PTE entries
when we remove an entire page table - we will have already walked the
page table removing those entries, which will already have been added.

Ah, I missed the fact that tlb_flush() invalidates the whole TLB when
there is no tlb->vma (the shift_arg_pages case). We could optimise this
to add the range covered by the PTE page and avoid the !tlb->vma check
(and a flush_tlb_mm), though not sure it's worth.

If we're removing pte entries then tlb->vma is non-NULL.  Please look at
the comments - I've documented the three modes of use there along with
how things are setup for each of those modes, and what we do with each.

Yes, I read them (they are useful) and I was only referring to point 3
in the comments, shift_arg_pages(). In this case tlb->vma is NULL and we
flush the full mm in tlb_flush(). Can we have a smaller address range
since we can get the from pte_free_tlb()?

This would mean removing the !tlb->vma check in tlb_flush() and change
tlb_add_flush() to take a range rather than just an address.

-- 
Catalin

On Mon, Mar 14, 2011 at 11:15:45AM +0000, Catalin Marinas wrote:

On Fri, 2011-03-11 at 19:24 +0000, Russell King - ARM Linux wrote:

quoted

On Fri, Mar 11, 2011 at 05:32:58PM +0000, Catalin Marinas wrote:

quoted

On Wed, 2011-03-09 at 18:35 +0000, Russell King - ARM Linux wrote:

quoted

On Wed, Mar 09, 2011 at 03:40:05PM +0000, Catalin Marinas wrote:

quoted

The above call to tlb_add_flush() would only add a PAGE_SIZE. But
since we free an entire PTE, shouldn't the range cover addr ..
addr+PTRS_PER_PTE*PAGE_SIZE?

Why do we need to?  We're not flushing away the individual PTE entries
when we remove an entire page table - we will have already walked the
page table removing those entries, which will already have been added.

Ah, I missed the fact that tlb_flush() invalidates the whole TLB when
there is no tlb->vma (the shift_arg_pages case). We could optimise this
to add the range covered by the PTE page and avoid the !tlb->vma check
(and a flush_tlb_mm), though not sure it's worth.

If we're removing pte entries then tlb->vma is non-NULL.  Please look at
the comments - I've documented the three modes of use there along with
how things are setup for each of those modes, and what we do with each.

Yes, I read them (they are useful) and I was only referring to point 3
in the comments, shift_arg_pages(). In this case tlb->vma is NULL and we
flush the full mm in tlb_flush(). Can we have a smaller address range
since we can get the from pte_free_tlb()?

This would mean removing the !tlb->vma check in tlb_flush() and change
tlb_add_flush() to take a range rather than just an address.

And the reason for doing so being what exactly?

We have very little in the way of userspace TLB entries (we shouldn't have
any other than for the existing arg pages).  So I don't see any benefit to
issuing several individual page TLB invalidates over a range for the arg
pages vs issuing one TLB invalidate for the entire ASID.

The latter seems to me to be the most efficient solution.