* [PATCH] ARM: Add TLB flushing for both entries in a PMD
@ 2011-11-16 10:38 Catalin Marinas
2011-11-23 10:21 ` Changhwan Youn
0 siblings, 1 reply; 6+ messages in thread
From: Catalin Marinas @ 2011-11-16 10:38 UTC (permalink / raw)
To: linux-arm-kernel
Linux uses two PMD entries for a PTE with the classic page table format,
covering 2MB range. However, the __pte_free_tlb() function only adds a
single TLB flush corresponding to 1MB range covering 'addr'. On
Cortex-A15, level 1 entries can be cached by the TLB independently of
the level 2 entries and without additional flushing a PMD entry would be
left pointing at the wrong PTE. The patch limits the TLB flushing range
to two 4KB pages around the 1MB boundary within PMD.
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
arch/arm/include/asm/tlb.h | 10 +++++++++-
1 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/arch/arm/include/asm/tlb.h b/arch/arm/include/asm/tlb.h
index 265f908..0504364 100644
--- a/arch/arm/include/asm/tlb.h
+++ b/arch/arm/include/asm/tlb.h
@@ -198,7 +198,15 @@ static inline void __pte_free_tlb(struct mmu_gather *tlb, pgtable_t pte,
unsigned long addr)
{
pgtable_page_dtor(pte);
- tlb_add_flush(tlb, addr);
+
+ /*
+ * With the classic ARM MMU, a pte page has two corresponding pmd
+ * entries, each covering 1MB.
+ */
+ addr &= PMD_MASK;
+ tlb_add_flush(tlb, addr + SZ_1M - PAGE_SIZE);
+ tlb_add_flush(tlb, addr + SZ_1M);
+
tlb_remove_page(tlb, pte);
}
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH] ARM: Add TLB flushing for both entries in a PMD
2011-11-16 10:38 [PATCH] ARM: Add TLB flushing for both entries in a PMD Catalin Marinas
@ 2011-11-23 10:21 ` Changhwan Youn
2011-11-23 10:45 ` Catalin Marinas
0 siblings, 1 reply; 6+ messages in thread
From: Changhwan Youn @ 2011-11-23 10:21 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
I have tested this patch on several exynos machines which
have a9 cores and it worked fine.
Though I'm not sure that android boot and running simple applications
are enough test for this patch.
On Wednesday, November 16, 2011 7:38 PM, Catalin Marinas wrote:
>
> Linux uses two PMD entries for a PTE with the classic page table format,
> covering 2MB range. However, the __pte_free_tlb() function only adds a
> single TLB flush corresponding to 1MB range covering 'addr'. On
> Cortex-A15, level 1 entries can be cached by the TLB independently of
> the level 2 entries and without additional flushing a PMD entry would be
> left pointing at the wrong PTE. The patch limits the TLB flushing range
> to two 4KB pages around the 1MB boundary within PMD.
>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> ---
> arch/arm/include/asm/tlb.h | 10 +++++++++-
> 1 files changed, 9 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/include/asm/tlb.h b/arch/arm/include/asm/tlb.h
> index 265f908..0504364 100644
> --- a/arch/arm/include/asm/tlb.h
> +++ b/arch/arm/include/asm/tlb.h
> @@ -198,7 +198,15 @@ static inline void __pte_free_tlb(struct mmu_gather *tlb,
pgtable_t pte,
> unsigned long addr)
> {
> pgtable_page_dtor(pte);
> - tlb_add_flush(tlb, addr);
> +
> + /*
> + * With the classic ARM MMU, a pte page has two corresponding pmd
> + * entries, each covering 1MB.
> + */
> + addr &= PMD_MASK;
> + tlb_add_flush(tlb, addr + SZ_1M - PAGE_SIZE);
The address of tlb_add_flush is modified from 'addr' to 'addr + SZ_1M -
PAGE_SIZE'.
Is the previous implementation wrong or just the address of tlb_add_flush is not
important
if it's in the same PMD?
> + tlb_add_flush(tlb, addr + SZ_1M);
> +
> tlb_remove_page(tlb, pte);
> }
>
Regards,
Changhwan
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] ARM: Add TLB flushing for both entries in a PMD
2011-11-23 10:21 ` Changhwan Youn
@ 2011-11-23 10:45 ` Catalin Marinas
2011-11-25 8:50 ` Changhwan Youn
0 siblings, 1 reply; 6+ messages in thread
From: Catalin Marinas @ 2011-11-23 10:45 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Nov 23, 2011 at 10:21:37AM +0000, Changhwan Youn wrote:
> I have tested this patch on several exynos machines which
> have a9 cores and it worked fine.
> Though I'm not sure that android boot and running simple applications
> are enough test for this patch.
Thanks for testing but the A9 would work fine without this patch. The
problem is on A15 where level 1 page table entries (pgd) are cached by
the TLB independently of level 2 entries (pte). The original code is
only flushing one entry in level 1 rather than 2.
The problem was visible with a stress-test application doing mmap/munmap
and then continuously getting level 2 translation fault even if the page
table looked fine to the kernel (because of stale level 1 entry in the
TLB).
LPAE is not affected, only the classic page table format.
--
Catalin
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ARM: Add TLB flushing for both entries in a PMD
2011-11-23 10:45 ` Catalin Marinas
@ 2011-11-25 8:50 ` Changhwan Youn
2011-11-25 10:17 ` Catalin Marinas
0 siblings, 1 reply; 6+ messages in thread
From: Changhwan Youn @ 2011-11-25 8:50 UTC (permalink / raw)
To: linux-arm-kernel
On Wednesday, November 23, 2011 7:46 PM, Catalin Marinas wrote:
> To: Changhwan Youn
> Cc: linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH] ARM: Add TLB flushing for both entries in a PMD
>
> On Wed, Nov 23, 2011 at 10:21:37AM +0000, Changhwan Youn wrote:
> > I have tested this patch on several exynos machines which
> > have a9 cores and it worked fine.
> > Though I'm not sure that android boot and running simple applications
> > are enough test for this patch.
>
> Thanks for testing but the A9 would work fine without this patch. The
> problem is on A15 where level 1 page table entries (pgd) are cached by
> the TLB independently of level 2 entries (pte). The original code is
> only flushing one entry in level 1 rather than 2.
Thank you for the answer.
The one thing I don't understand is why A9 works fine without this
patch. I know that A9 has worked fine without this patch.
It seems that without this patch, invalid VA->PA mapping can remains in TLB
and this can cause wrong PA access by user process.
Can you explain why there's no wrong PA access in A9?
Thank you.
>
> The problem was visible with a stress-test application doing mmap/munmap
> and then continuously getting level 2 translation fault even if the page
> table looked fine to the kernel (because of stale level 1 entry in the
> TLB).
>
> LPAE is not affected, only the classic page table format.
>
> --
> Catalin
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Regards,
Changhwan
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ARM: Add TLB flushing for both entries in a PMD
2011-11-25 8:50 ` Changhwan Youn
@ 2011-11-25 10:17 ` Catalin Marinas
2011-11-28 8:00 ` Changhwan Youn
0 siblings, 1 reply; 6+ messages in thread
From: Catalin Marinas @ 2011-11-25 10:17 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Nov 25, 2011 at 08:50:48AM +0000, Changhwan Youn wrote:
> On Wednesday, November 23, 2011 7:46 PM, Catalin Marinas wrote:
> > To: Changhwan Youn
> > Cc: linux-arm-kernel at lists.infradead.org
> > Subject: Re: [PATCH] ARM: Add TLB flushing for both entries in a PMD
> >
> > On Wed, Nov 23, 2011 at 10:21:37AM +0000, Changhwan Youn wrote:
> > > I have tested this patch on several exynos machines which
> > > have a9 cores and it worked fine.
> > > Though I'm not sure that android boot and running simple applications
> > > are enough test for this patch.
> >
> > Thanks for testing but the A9 would work fine without this patch. The
> > problem is on A15 where level 1 page table entries (pgd) are cached by
> > the TLB independently of level 2 entries (pte). The original code is
> > only flushing one entry in level 1 rather than 2.
>
> Thank you for the answer.
> The one thing I don't understand is why A9 works fine without this
> patch. I know that A9 has worked fine without this patch.
> It seems that without this patch, invalid VA->PA mapping can remains in TLB
> and this can cause wrong PA access by user process.
> Can you explain why there's no wrong PA access in A9?
This patch only fixes a bug in the pte_free_tlb() function, used when
freeing the page tables and its goal is to make sure pmd level entries
to not point to an already freed pte. Unmapping user VA->PA mappings
happens at the pte level and page TLBs are flushed separately. If you
look at the original code, it only adds a TLB flush for a single address
while a pte table covers 2MB.
In theory, on A9 SMP there could also be an issue but given the timing
and memory ordering conditions that's impossible in practice. Basically,
the requirement is that clearing of the pmd entry on one CPU does not
become visible to another CPU before the corresponding pte page has been
re-used. The A9 only caches a page table walk in the TLB if there is a
complete VA->PA translation.
--
Catalin
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ARM: Add TLB flushing for both entries in a PMD
2011-11-25 10:17 ` Catalin Marinas
@ 2011-11-28 8:00 ` Changhwan Youn
0 siblings, 0 replies; 6+ messages in thread
From: Changhwan Youn @ 2011-11-28 8:00 UTC (permalink / raw)
To: linux-arm-kernel
So practically Cortex A9 doesn't need tlb_add_flush() in pte_free_tlb().
I was a little confused because of the address of tlb_add_flush().
I really appreciate your answering.
On Friday, November 25, 2011 7:18 PM, Catalin Marinas wrote
> On Fri, Nov 25, 2011 at 08:50:48AM +0000, Changhwan Youn wrote:
> > On Wednesday, November 23, 2011 7:46 PM, Catalin Marinas wrote:
> > > To: Changhwan Youn
> > > Cc: linux-arm-kernel at lists.infradead.org
> > > Subject: Re: [PATCH] ARM: Add TLB flushing for both entries in a PMD
> > >
> > > On Wed, Nov 23, 2011 at 10:21:37AM +0000, Changhwan Youn wrote:
> > > > I have tested this patch on several exynos machines which
> > > > have a9 cores and it worked fine.
> > > > Though I'm not sure that android boot and running simple applications
> > > > are enough test for this patch.
> > >
> > > Thanks for testing but the A9 would work fine without this patch. The
> > > problem is on A15 where level 1 page table entries (pgd) are cached by
> > > the TLB independently of level 2 entries (pte). The original code is
> > > only flushing one entry in level 1 rather than 2.
> >
> > Thank you for the answer.
> > The one thing I don't understand is why A9 works fine without this
> > patch. I know that A9 has worked fine without this patch.
> > It seems that without this patch, invalid VA->PA mapping can remains in TLB
> > and this can cause wrong PA access by user process.
> > Can you explain why there's no wrong PA access in A9?
>
> This patch only fixes a bug in the pte_free_tlb() function, used when
> freeing the page tables and its goal is to make sure pmd level entries
> to not point to an already freed pte. Unmapping user VA->PA mappings
> happens at the pte level and page TLBs are flushed separately. If you
> look at the original code, it only adds a TLB flush for a single address
> while a pte table covers 2MB.
>
> In theory, on A9 SMP there could also be an issue but given the timing
> and memory ordering conditions that's impossible in practice. Basically,
> the requirement is that clearing of the pmd entry on one CPU does not
> become visible to another CPU before the corresponding pte page has been
> re-used. The A9 only caches a page table walk in the TLB if there is a
> complete VA->PA translation.
Regards,
Changhwan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-11-28 8:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-11-16 10:38 [PATCH] ARM: Add TLB flushing for both entries in a PMD Catalin Marinas
2011-11-23 10:21 ` Changhwan Youn
2011-11-23 10:45 ` Catalin Marinas
2011-11-25 8:50 ` Changhwan Youn
2011-11-25 10:17 ` Catalin Marinas
2011-11-28 8:00 ` Changhwan Youn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).