| Message ID | 1350236542-96465-1-git-send-email-yanpai.chen@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 15, 2012 at 1:42 AM, Andrew Yan-Pai Chen <yanpai.chen@gmail.com> wrote: > From: Yan-Pai Chen <ypchen@faraday-tech.com> > > Since flush_pfn_alias() is preemptible, it is possible to be > preempted just after set_top_pte() is done. If the process > which preempts the previous happened to invoke flush_pfn_alias() > with the same colour vaddr as that of the previous, the same > top pte will be overwritten. When switching back to the previous, > it attempts to flush cache lines with incorrect mapping. Then > no lines (or wrong lines) will be flushed because of the nature > of vipt caches. > > flush_icache_alias() has the same problem as well. However, as it > could be called in SMP setups, we prevent concurrent overwrites of > top pte by having a lock on it. > > Signed-off-by: JasonLin <wwlin@faraday-tech.com> > Signed-off-by: Yan-Pai Chen <ypchen@faraday-tech.com> > --- > arch/arm/mm/flush.c | 11 +++++++++++ > 1 files changed, 11 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c > index 40ca11e..b6510f4 100644 > --- a/arch/arm/mm/flush.c > +++ b/arch/arm/mm/flush.c > @@ -11,6 +11,7 @@ > #include <linux/mm.h> > #include <linux/pagemap.h> > #include <linux/highmem.h> > +#include <linux/spinlock.h> > > #include <asm/cacheflush.h> > #include <asm/cachetype.h> > @@ -22,11 +23,15 @@ > > #ifdef CONFIG_CPU_CACHE_VIPT > > +static DEFINE_RAW_SPINLOCK(flush_lock); > + > +/* Beware that this function is not to be called for SMP setups. */ > static void flush_pfn_alias(unsigned long pfn, unsigned long vaddr) > { > unsigned long to = FLUSH_ALIAS_START + (CACHE_COLOUR(vaddr) << > PAGE_SHIFT); > const int zero = 0; > > + preempt_disable(); > set_top_pte(to, pfn_pte(pfn, PAGE_KERNEL)); > > asm( "mcrr p15, 0, %1, %0, c14\n" > @@ -34,6 +39,8 @@ static void flush_pfn_alias(unsigned long pfn, unsigned > long vaddr) > : > : "r" (to), "r" (to + PAGE_SIZE - L1_CACHE_BYTES), "r" (zero) > : "cc"); > + > + preempt_enable(); > } > > static void flush_icache_alias(unsigned long pfn, unsigned long vaddr, > unsigned long len) > @@ -42,9 +49,13 @@ static void flush_icache_alias(unsigned long pfn, > unsigned long vaddr, unsigned > unsigned long offset = vaddr & (PAGE_SIZE - 1); > unsigned long to; > > + raw_spin_lock(&flush_lock); > + > set_top_pte(va, pfn_pte(pfn, PAGE_KERNEL)); > to = va + offset; > flush_icache_range(to, to + len); > + > + raw_spin_unlock(&flush_lock); > } > > void flush_cache_mm(struct mm_struct *mm) > -- > 1.7.4.1 > Hi all, Any ideas?
On Wed, Oct 17, 2012 at 09:42:19AM +0100, Andrew Yan-Pai Chen wrote: > On Mon, Oct 15, 2012 at 1:42 AM, Andrew Yan-Pai Chen > <yanpai.chen@gmail.com> wrote: > > +static DEFINE_RAW_SPINLOCK(flush_lock); > > + > > +/* Beware that this function is not to be called for SMP setups. */ > > static void flush_pfn_alias(unsigned long pfn, unsigned long vaddr) > > { > > unsigned long to = FLUSH_ALIAS_START + (CACHE_COLOUR(vaddr) << > > PAGE_SHIFT); > > const int zero = 0; > > > > + preempt_disable(); > > set_top_pte(to, pfn_pte(pfn, PAGE_KERNEL)); > > > > asm( "mcrr p15, 0, %1, %0, c14\n" > > @@ -34,6 +39,8 @@ static void flush_pfn_alias(unsigned long pfn, unsigned > > long vaddr) > > : > > : "r" (to), "r" (to + PAGE_SIZE - L1_CACHE_BYTES), "r" (zero) > > : "cc"); > > + > > + preempt_enable(); > > } > > > > static void flush_icache_alias(unsigned long pfn, unsigned long vaddr, > > unsigned long len) > > @@ -42,9 +49,13 @@ static void flush_icache_alias(unsigned long pfn, > > unsigned long vaddr, unsigned > > unsigned long offset = vaddr & (PAGE_SIZE - 1); > > unsigned long to; > > > > + raw_spin_lock(&flush_lock); > > + > > set_top_pte(va, pfn_pte(pfn, PAGE_KERNEL)); > > to = va + offset; > > flush_icache_range(to, to + len); > > + > > + raw_spin_unlock(&flush_lock); > > } > > > > void flush_cache_mm(struct mm_struct *mm) > > -- > > 1.7.4.1 > > > > Hi all, > > Any ideas? I wonder if we could use different ptes for each CPU (by hacking pte_offset_kernel) and then get away with just disabling preemption in both cases? Will
2012/10/17 Will Deacon <will.deacon@arm.com>: > On Wed, Oct 17, 2012 at 09:42:19AM +0100, Andrew Yan-Pai Chen wrote: >> On Mon, Oct 15, 2012 at 1:42 AM, Andrew Yan-Pai Chen >> <yanpai.chen@gmail.com> wrote: >> > +static DEFINE_RAW_SPINLOCK(flush_lock); >> > + >> > +/* Beware that this function is not to be called for SMP setups. */ >> > static void flush_pfn_alias(unsigned long pfn, unsigned long vaddr) >> > { >> > unsigned long to = FLUSH_ALIAS_START + (CACHE_COLOUR(vaddr) << >> > PAGE_SHIFT); >> > const int zero = 0; >> > >> > + preempt_disable(); >> > set_top_pte(to, pfn_pte(pfn, PAGE_KERNEL)); >> > >> > asm( "mcrr p15, 0, %1, %0, c14\n" >> > @@ -34,6 +39,8 @@ static void flush_pfn_alias(unsigned long pfn, unsigned >> > long vaddr) >> > : >> > : "r" (to), "r" (to + PAGE_SIZE - L1_CACHE_BYTES), "r" (zero) >> > : "cc"); >> > + >> > + preempt_enable(); >> > } >> > >> > static void flush_icache_alias(unsigned long pfn, unsigned long vaddr, >> > unsigned long len) >> > @@ -42,9 +49,13 @@ static void flush_icache_alias(unsigned long pfn, >> > unsigned long vaddr, unsigned >> > unsigned long offset = vaddr & (PAGE_SIZE - 1); >> > unsigned long to; >> > >> > + raw_spin_lock(&flush_lock); >> > + >> > set_top_pte(va, pfn_pte(pfn, PAGE_KERNEL)); >> > to = va + offset; >> > flush_icache_range(to, to + len); >> > + >> > + raw_spin_unlock(&flush_lock); >> > } >> > >> > void flush_cache_mm(struct mm_struct *mm) >> > -- >> > 1.7.4.1 >> > >> >> Hi all, >> >> Any ideas? > > I wonder if we could use different ptes for each CPU (by hacking > pte_offset_kernel) and then get away with just disabling preemption in both > cases? > > Will Single core processor will also cause this issue in flush_pfn_alias(). So it should use different ptes for each task. Will it be so complicated? Jason
On Wed, Oct 17, 2012 at 10:54:53AM +0100, Jason Lin wrote: > 2012/10/17 Will Deacon <will.deacon@arm.com>: > > On Wed, Oct 17, 2012 at 09:42:19AM +0100, Andrew Yan-Pai Chen wrote: > >> On Mon, Oct 15, 2012 at 1:42 AM, Andrew Yan-Pai Chen > >> > >> Any ideas? > > > > I wonder if we could use different ptes for each CPU (by hacking > > pte_offset_kernel) and then get away with just disabling preemption in both > > cases? > > > > Will > > Single core processor will also cause this issue in flush_pfn_alias(). > So it should use different ptes for each task. > Will it be so complicated? You can just disable preemption in that case -- it's the spin_lock that I'd like to avoid. Will
On Wed, Oct 17, 2012 at 5:57 PM, Will Deacon <will.deacon@arm.com> wrote: > On Wed, Oct 17, 2012 at 10:54:53AM +0100, Jason Lin wrote: >> 2012/10/17 Will Deacon <will.deacon@arm.com>: >> > On Wed, Oct 17, 2012 at 09:42:19AM +0100, Andrew Yan-Pai Chen wrote: >> >> On Mon, Oct 15, 2012 at 1:42 AM, Andrew Yan-Pai Chen >> >> >> >> Any ideas? >> > >> > I wonder if we could use different ptes for each CPU (by hacking >> > pte_offset_kernel) and then get away with just disabling preemption in both >> > cases? >> > >> > Will >> >> Single core processor will also cause this issue in flush_pfn_alias(). >> So it should use different ptes for each task. >> Will it be so complicated? > > You can just disable preemption in that case -- it's the spin_lock that I'd > like to avoid. > > Will If we have larger address space for VIPT aliasing flushing, 4 pages per CPU (for 4 colours) then we can avoid spinlocks by using per-cpu pte: #define percpu_colour_offset (0x4000 * smp_processor_id()) static inline void set_top_pte(unsigned long va, pte_t pte) { pte_t *ptep = pte_offset_kernel(top_pmd, va + percpu_colour_offset); set_pte_ext(ptep, pte, 0); local_flush_tlb_kernel_page(va + percpu_colour_offset); } Is this what you mean, Will? But considering set_top_pte() will also be called in other paths (functions in highmem.c), maybe we should keep set_top_pte() untouched but change the caller: static void flush_icache_alias(unsigned long pfn, unsigned long vaddr, unsigned long len) { unsigned long va = FLUSH_ALIAS_START + (CACHE_COLOUR(vaddr) << PAGE_SHIFT); unsigned long offset = vaddr & (PAGE_SIZE - 1); unsigned long to; preempt_disable(); va += percpu_colour_offset; set_top_pte(va, pfn_pte(pfn, PAGE_KERNEL)); to = va + offset; flush_icache_range(to, to + len); preempt_enable(); } BTW, shouldn't the spinlock in v6_*_user_highpage_aliasing be removed as well? (disabling preemption instead) There seems to be no multi-core processors using VIPT aliasing D-cache? Any comment would be appreciated. -- Regards, Andrew
diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c index 40ca11e..b6510f4 100644 --- a/arch/arm/mm/flush.c +++ b/arch/arm/mm/flush.c @@ -11,6 +11,7 @@ #include <linux/mm.h> #include <linux/pagemap.h> #include <linux/highmem.h> +#include <linux/spinlock.h> #include <asm/cacheflush.h> #include <asm/cachetype.h> @@ -22,11 +23,15 @@ #ifdef CONFIG_CPU_CACHE_VIPT +static DEFINE_RAW_SPINLOCK(flush_lock); + +/* Beware that this function is not to be called for SMP setups. */ static void flush_pfn_alias(unsigned long pfn, unsigned long vaddr) { unsigned long to = FLUSH_ALIAS_START + (CACHE_COLOUR(vaddr) << PAGE_SHIFT); const int zero = 0; + preempt_disable(); set_top_pte(to, pfn_pte(pfn, PAGE_KERNEL)); asm( "mcrr p15, 0, %1, %0, c14\n" @@ -34,6 +39,8 @@ static void flush_pfn_alias(unsigned long pfn, unsigned long vaddr) : : "r" (to), "r" (to + PAGE_SIZE - L1_CACHE_BYTES), "r" (zero) : "cc"); + + preempt_enable(); } static void flush_icache_alias(unsigned long pfn, unsigned long vaddr, unsigned long len) @@ -42,9 +49,13 @@ static void flush_icache_alias(unsigned long pfn, unsigned long vaddr, unsigned unsigned long offset = vaddr & (PAGE_SIZE - 1); unsigned long to; + raw_spin_lock(&flush_lock); + set_top_pte(va, pfn_pte(pfn, PAGE_KERNEL)); to = va + offset; flush_icache_range(to, to + len); + + raw_spin_unlock(&flush_lock); } void flush_cache_mm(struct mm_struct *mm)