[PATCH 1/2] ARM: use cache type functions for arch_get_unmapped_area

[PATCH 1/2] ARM: use cache type functions for arch_get_unmapped_area

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

There are already cache type decoding functions, so use those instead
of custom decode code which only works for ARMv6.

This change also correctly enables cache colour alignment on Cortex-A9
whose I-cache is aliasing VIPT.

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
---
 arch/arm/mm/mmap.c |   23 ++++++-----------------
 1 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/arch/arm/mm/mmap.c b/arch/arm/mm/mmap.c
index 74be05f..80e70ef 100644
--- a/arch/arm/mm/mmap.c
+++ b/arch/arm/mm/mmap.c
@@ -9,8 +9,7 @@
 #include <linux/io.h>
 #include <linux/personality.h>
 #include <linux/random.h>
-#include <asm/cputype.h>
-#include <asm/system.h>
+#include <asm/cachetype.h>
 
 #define COLOUR_ALIGN(addr,pgoff)		\
 	((((addr)+SHMLBA-1)&~(SHMLBA-1)) +	\
@@ -32,25 +31,15 @@ arch_get_unmapped_area(struct file *filp, unsigned long addr,
 	struct mm_struct *mm = current->mm;
 	struct vm_area_struct *vma;
 	unsigned long start_addr;
-#if defined(CONFIG_CPU_V6) || defined(CONFIG_CPU_V6K)
-	unsigned int cache_type;
-	int do_align = 0, aliasing = 0;
+	int do_align = 0;
+	int aliasing = cache_is_vipt_aliasing() || icache_is_vipt_aliasing();
 
 	/*
 	 * We only need to do colour alignment if either the I or D
-	 * caches alias.  This is indicated by bits 9 and 21 of the
-	 * cache type register.
+	 * caches alias.
 	 */
-	cache_type = read_cpuid_cachetype();
-	if (cache_type != read_cpuid_id()) {
-		aliasing = (cache_type | cache_type >> 12) & (1 << 11);
-		if (aliasing)
-			do_align = filp || flags & MAP_SHARED;
-	}
-#else
-#define do_align 0
-#define aliasing 0
-#endif
+	if (aliasing)
+		do_align = filp || (flags & MAP_SHARED);
 
 	/*
 	 * We enforce the MAP_FIXED case.
-- 
1.7.5.4

[PATCH 2/2] ARM: topdown mmap support

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

Similar to other architectures, this adds topdown mmap support in user
process address space allocation policy. This allows mmap sizes greater
than 2GB. This support is largely copied from MIPS and the generic
implementations.

The address space randomization is moved into arch_pick_mmap_layout.

Tested on V-Express with ubuntu and a mmap test from here:
https://bugs.launchpad.net/bugs/861296

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
---
 arch/arm/include/asm/pgtable.h   |    1 +
 arch/arm/include/asm/processor.h |    2 +
 arch/arm/mm/mmap.c               |  173 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 171 insertions(+), 5 deletions(-)

diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
index 9451dce..2f659e2 100644
--- a/arch/arm/include/asm/pgtable.h
+++ b/arch/arm/include/asm/pgtable.h
@@ -336,6 +336,7 @@ static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
  * We provide our own arch_get_unmapped_area to cope with VIPT caches.
  */
 #define HAVE_ARCH_UNMAPPED_AREA
+#define HAVE_ARCH_UNMAPPED_AREA_TOPDOWN
 
 /*
  * remap a physical page `pfn' of size `size' with page protection `prot'
diff --git a/arch/arm/include/asm/processor.h b/arch/arm/include/asm/processor.h
index b2d9df5..ce280b8 100644
--- a/arch/arm/include/asm/processor.h
+++ b/arch/arm/include/asm/processor.h
@@ -123,6 +123,8 @@ static inline void prefetch(const void *ptr)
 
 #endif
 
+#define HAVE_ARCH_PICK_MMAP_LAYOUT
+
 #endif
 
 #endif /* __ASM_ARM_PROCESSOR_H */
diff --git a/arch/arm/mm/mmap.c b/arch/arm/mm/mmap.c
index 80e70ef..c2809d7 100644
--- a/arch/arm/mm/mmap.c
+++ b/arch/arm/mm/mmap.c
@@ -11,10 +11,49 @@
 #include <linux/random.h>
 #include <asm/cachetype.h>
 
+static inline unsigned long COLOUR_ALIGN_DOWN(unsigned long addr,
+					      unsigned long pgoff)
+{
+	unsigned long base = addr & ~(SHMLBA-1);
+	unsigned long off = (pgoff << PAGE_SHIFT) & (SHMLBA-1);
+
+	if (base + off <= addr)
+		return base + off;
+
+	return base - off;
+}
+
 #define COLOUR_ALIGN(addr,pgoff)		\
 	((((addr)+SHMLBA-1)&~(SHMLBA-1)) +	\
 	 (((pgoff)<<PAGE_SHIFT) & (SHMLBA-1)))
 
+/* gap between mmap and stack */
+#define MIN_GAP (128*1024*1024UL)
+#define MAX_GAP ((TASK_SIZE)/6*5)
+
+static int mmap_is_legacy(void)
+{
+	if (current->personality & ADDR_COMPAT_LAYOUT)
+		return 1;
+
+	if (rlimit(RLIMIT_STACK) == RLIM_INFINITY)
+		return 1;
+
+	return sysctl_legacy_va_layout;
+}
+
+static unsigned long mmap_base(unsigned long rnd)
+{
+	unsigned long gap = rlimit(RLIMIT_STACK);
+
+	if (gap < MIN_GAP)
+		gap = MIN_GAP;
+	else if (gap > MAX_GAP)
+		gap = MAX_GAP;
+
+	return PAGE_ALIGN(TASK_SIZE - gap - rnd);
+}
+
 /*
  * We need to ensure that shared mappings are correctly aligned to
  * avoid aliasing issues with VIPT caches.  We need to ensure that
@@ -68,13 +107,9 @@ arch_get_unmapped_area(struct file *filp, unsigned long addr,
 	if (len > mm->cached_hole_size) {
 	        start_addr = addr = mm->free_area_cache;
 	} else {
-	        start_addr = addr = TASK_UNMAPPED_BASE;
+	        start_addr = addr = mm->mmap_base;
 	        mm->cached_hole_size = 0;
 	}
-	/* 8 bits of randomness in 20 address space bits */
-	if ((current->flags & PF_RANDOMIZE) &&
-	    !(current->personality & ADDR_NO_RANDOMIZE))
-		addr += (get_random_int() % (1 << 8)) << PAGE_SHIFT;
 
 full_search:
 	if (do_align)
@@ -111,6 +146,134 @@ full_search:
 	}
 }
 
+unsigned long
+arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
+			const unsigned long len, const unsigned long pgoff,
+			const unsigned long flags)
+{
+	struct vm_area_struct *vma;
+	struct mm_struct *mm = current->mm;
+	unsigned long addr = addr0;
+	int do_align = 0;
+	int aliasing = cache_is_vipt_aliasing() || icache_is_vipt_aliasing();
+
+	/*
+	 * We only need to do colour alignment if either the I or D
+	 * caches alias.
+	 */
+	if (aliasing)
+		do_align = filp || (flags & MAP_SHARED);
+
+	/* requested length too big for entire address space */
+	if (len > TASK_SIZE)
+		return -ENOMEM;
+
+	if (flags & MAP_FIXED) {
+		if (aliasing && flags & MAP_SHARED &&
+		    (addr - (pgoff << PAGE_SHIFT)) & (SHMLBA - 1))
+			return -EINVAL;
+		return addr;
+	}
+
+	/* requesting a specific address */
+	if (addr) {
+		if (do_align)
+			addr = COLOUR_ALIGN(addr, pgoff);
+		else
+			addr = PAGE_ALIGN(addr);
+		vma = find_vma(mm, addr);
+		if (TASK_SIZE - len >= addr &&
+				(!vma || addr + len <= vma->vm_start))
+			return addr;
+	}
+
+	/* check if free_area_cache is useful for us */
+	if (len <= mm->cached_hole_size) {
+		mm->cached_hole_size = 0;
+		mm->free_area_cache = mm->mmap_base;
+	}
+
+	/* either no address requested or can't fit in requested address hole */
+	addr = mm->free_area_cache;
+	if (do_align) {
+		unsigned long base = COLOUR_ALIGN_DOWN(addr - len, pgoff);
+		addr = base + len;
+	}
+
+	/* make sure it can fit in the remaining address space */
+	if (addr > len) {
+		vma = find_vma(mm, addr-len);
+		if (!vma || addr <= vma->vm_start)
+			/* remember the address as a hint for next time */
+			return (mm->free_area_cache = addr-len);
+	}
+
+	if (mm->mmap_base < len)
+		goto bottomup;
+
+	addr = mm->mmap_base - len;
+	if (do_align)
+		addr = COLOUR_ALIGN_DOWN(addr, pgoff);
+
+	do {
+		/*
+		 * Lookup failure means no vma is above this address,
+		 * else if new region fits below vma->vm_start,
+		 * return with success:
+		 */
+		vma = find_vma(mm, addr);
+		if (!vma || addr+len <= vma->vm_start)
+			/* remember the address as a hint for next time */
+			return (mm->free_area_cache = addr);
+
+		/* remember the largest hole we saw so far */
+		if (addr + mm->cached_hole_size < vma->vm_start)
+			mm->cached_hole_size = vma->vm_start - addr;
+
+		/* try just below the current vma->vm_start */
+		addr = vma->vm_start - len;
+		if (do_align)
+			addr = COLOUR_ALIGN_DOWN(addr, pgoff);
+	} while (len < vma->vm_start);
+
+bottomup:
+	/*
+	 * A failed mmap() very likely causes application failure,
+	 * so fall back to the bottom-up function here. This scenario
+	 * can happen with large stack limits and large mmap()
+	 * allocations.
+	 */
+	mm->cached_hole_size = ~0UL;
+	mm->free_area_cache = TASK_UNMAPPED_BASE;
+	addr = arch_get_unmapped_area(filp, addr0, len, pgoff, flags);
+	/*
+	 * Restore the topdown base:
+	 */
+	mm->free_area_cache = mm->mmap_base;
+	mm->cached_hole_size = ~0UL;
+
+	return addr;
+}
+
+void arch_pick_mmap_layout(struct mm_struct *mm)
+{
+	unsigned long random_factor = 0UL;
+
+	/* 8 bits of randomness in 20 address space bits */
+	if ((current->flags & PF_RANDOMIZE) &&
+	    !(current->personality & ADDR_NO_RANDOMIZE))
+		random_factor = (get_random_int() % (1 << 8)) << PAGE_SHIFT;
+
+	if (mmap_is_legacy()) {
+		mm->mmap_base = TASK_UNMAPPED_BASE + random_factor;
+		mm->get_unmapped_area = arch_get_unmapped_area;
+		mm->unmap_area = arch_unmap_area;
+	} else {
+		mm->mmap_base = mmap_base(random_factor);
+		mm->get_unmapped_area = arch_get_unmapped_area_topdown;
+		mm->unmap_area = arch_unmap_area_topdown;
+	}
+}
 
 /*
  * You really shouldn't be using read() or write() on /dev/mem.  This
-- 
1.7.5.4

[PATCH 2/2] ARM: topdown mmap support

[Nicolas Pitre]

On Mon, 7 Nov 2011, Rob Herring wrote:

> From: Rob Herring <rob.herring@calxeda.com>
> 
> Similar to other architectures, this adds topdown mmap support in user
> process address space allocation policy. This allows mmap sizes greater
> than 2GB. This support is largely copied from MIPS and the generic
> implementations.
> 
> The address space randomization is moved into arch_pick_mmap_layout.

This is a problem by effectively weakening the randomization greatly.  
Now you get a random starting point but all mmaps are otherwise fixed 
relative to each other, whereas you had random distances between each 
mmaps before.


Nicolas

[PATCH 2/2] ARM: topdown mmap support

[Rob Herring]

On 11/14/2011 11:41 AM, Nicolas Pitre wrote:
> On Mon, 7 Nov 2011, Rob Herring wrote:
> 
>> From: Rob Herring <rob.herring@calxeda.com>
>>
>> Similar to other architectures, this adds topdown mmap support in user
>> process address space allocation policy. This allows mmap sizes greater
>> than 2GB. This support is largely copied from MIPS and the generic
>> implementations.
>>
>> The address space randomization is moved into arch_pick_mmap_layout.
> 
> This is a problem by effectively weakening the randomization greatly.  
> Now you get a random starting point but all mmaps are otherwise fixed 
> relative to each other, whereas you had random distances between each 
> mmaps before.
> 

You mean within a single process the mmap's are not randomized? Couldn't
that end up wasting a lot of virtual space if you have a lot of mmaps?

It is aligned with other arch's and the generic implementation. The
generic implementation doesn't even do randomization for legacy layouts.

Rob

[PATCH 2/2] ARM: topdown mmap support

[Nicolas Pitre]

On Mon, 14 Nov 2011, Rob Herring wrote:

> On 11/14/2011 11:41 AM, Nicolas Pitre wrote:
> > On Mon, 7 Nov 2011, Rob Herring wrote:
> > 
> >> From: Rob Herring <rob.herring@calxeda.com>
> >>
> >> Similar to other architectures, this adds topdown mmap support in user
> >> process address space allocation policy. This allows mmap sizes greater
> >> than 2GB. This support is largely copied from MIPS and the generic
> >> implementations.
> >>
> >> The address space randomization is moved into arch_pick_mmap_layout.
> > 
> > This is a problem by effectively weakening the randomization greatly.  
> > Now you get a random starting point but all mmaps are otherwise fixed 
> > relative to each other, whereas you had random distances between each 
> > mmaps before.
> > 
> 
> You mean within a single process the mmap's are not randomized? 

With regard to each other: not anymore with your patch.

> Couldn't that end up wasting a lot of virtual space if you have a lot 
> of mmaps?

That is what we have now without this patch.  The Potential for wasted 
space is 8 bits on a page level, i.e. 1MB max or 512 KB on average, per 
mmap.  I don't think this is that bad.

> It is aligned with other arch's and the generic implementation. The
> generic implementation doesn't even do randomization for legacy layouts.

Only x86 does complete ASLR besides ARM.


Nicolas

[PATCH 2/2] ARM: topdown mmap support

[Rob Herring]

Nicolas,

On 11/14/2011 12:22 PM, Nicolas Pitre wrote:
> On Mon, 14 Nov 2011, Rob Herring wrote:
> 
>> On 11/14/2011 11:41 AM, Nicolas Pitre wrote:
>>> On Mon, 7 Nov 2011, Rob Herring wrote:
>>>
>>>> From: Rob Herring <rob.herring@calxeda.com>
>>>>
>>>> Similar to other architectures, this adds topdown mmap support in user
>>>> process address space allocation policy. This allows mmap sizes greater
>>>> than 2GB. This support is largely copied from MIPS and the generic
>>>> implementations.
>>>>
>>>> The address space randomization is moved into arch_pick_mmap_layout.
>>>
>>> This is a problem by effectively weakening the randomization greatly.  
>>> Now you get a random starting point but all mmaps are otherwise fixed 
>>> relative to each other, whereas you had random distances between each 
>>> mmaps before.
>>>
>>
>> You mean within a single process the mmap's are not randomized? 
> 
> With regard to each other: not anymore with your patch.
> 
>> Couldn't that end up wasting a lot of virtual space if you have a lot 
>> of mmaps?
> 
> That is what we have now without this patch.  The Potential for wasted 
> space is 8 bits on a page level, i.e. 1MB max or 512 KB on average, per 
> mmap.  I don't think this is that bad.
> 

No one cared about or noticed this issue until recently. That is
changing as we start to see ARM systems with more memory and I'm sure
we'll see more issues like this. Someone may care that they get less
memory than other 32-bit arches. Can you really trust that apps don't do
a large number of mmaps.

>> It is aligned with other arch's and the generic implementation. The
>> generic implementation doesn't even do randomization for legacy layouts.
> 
> Only x86 does complete ASLR besides ARM.

x86 does not do per mmap randomization. Here's the output of a test
program which does 1 to 23 MB size mmaps in 1MB steps. The gap is the
last address - current address. I'm not sure what's going on with the
2nd mmap gap though, but otherwise the gap matches the mmap size.

mmap at 0x7f74bd1b7000, gap = 42e49000
mmap at 0x7f74bcb1e000, gap = 699000
mmap at 0x7f74bc81e000, gap = 300000
mmap at 0x7f74bc41e000, gap = 400000
mmap at 0x7f74bbf1e000, gap = 500000
mmap at 0x7f74bb91e000, gap = 600000
mmap at 0x7f74bb21e000, gap = 700000
mmap at 0x7f74baa1e000, gap = 800000
mmap at 0x7f74ba11e000, gap = 900000
mmap at 0x7f74b971e000, gap = a00000
mmap at 0x7f74b8c1e000, gap = b00000
mmap at 0x7f74b801e000, gap = c00000
mmap at 0x7f74b731e000, gap = d00000
mmap at 0x7f74b651e000, gap = e00000
mmap at 0x7f74b561e000, gap = f00000
mmap at 0x7f74b461e000, gap = 1000000
mmap at 0x7f74b351e000, gap = 1100000
mmap at 0x7f74b231e000, gap = 1200000
mmap at 0x7f74b101e000, gap = 1300000
mmap at 0x7f74afc1e000, gap = 1400000
mmap at 0x7f74ae71e000, gap = 1500000
mmap at 0x7f74ad11e000, gap = 1600000

My patch exceeds 32-bit x86 functionality which does no randomization
for legacy layouts. It matches 32-bit and 64-bit x86 for topdown layout.

I'm happy to revert the legacy part of my patch, but it's a bit mute
point as legacy is not used by default.

Rob

[PATCH 2/2] ARM: topdown mmap support

[Nicolas Pitre]

On Tue, 15 Nov 2011, Rob Herring wrote:

> Nicolas,
> 
> On 11/14/2011 12:22 PM, Nicolas Pitre wrote:
> > On Mon, 14 Nov 2011, Rob Herring wrote:
> > 
> >> On 11/14/2011 11:41 AM, Nicolas Pitre wrote:
> >>> On Mon, 7 Nov 2011, Rob Herring wrote:
> >>>
> >>>> From: Rob Herring <rob.herring@calxeda.com>
> >>>>
> >>>> Similar to other architectures, this adds topdown mmap support in user
> >>>> process address space allocation policy. This allows mmap sizes greater
> >>>> than 2GB. This support is largely copied from MIPS and the generic
> >>>> implementations.
> >>>>
> >>>> The address space randomization is moved into arch_pick_mmap_layout.
> >>>
> >>> This is a problem by effectively weakening the randomization greatly.  
> >>> Now you get a random starting point but all mmaps are otherwise fixed 
> >>> relative to each other, whereas you had random distances between each 
> >>> mmaps before.
> >>>
> >>
> >> You mean within a single process the mmap's are not randomized? 
> > 
> > With regard to each other: not anymore with your patch.
> > 
> >> Couldn't that end up wasting a lot of virtual space if you have a lot 
> >> of mmaps?
> > 
> > That is what we have now without this patch.  The Potential for wasted 
> > space is 8 bits on a page level, i.e. 1MB max or 512 KB on average, per 
> > mmap.  I don't think this is that bad.
> > 
> 
> No one cared about or noticed this issue until recently. That is
> changing as we start to see ARM systems with more memory and I'm sure
> we'll see more issues like this. Someone may care that they get less
> memory than other 32-bit arches. Can you really trust that apps don't do
> a large number of mmaps.
> 
> >> It is aligned with other arch's and the generic implementation. The
> >> generic implementation doesn't even do randomization for legacy layouts.
> > 
> > Only x86 does complete ASLR besides ARM.
> 
> x86 does not do per mmap randomization. Here's the output of a test
> program which does 1 to 23 MB size mmaps in 1MB steps. The gap is the
> last address - current address. I'm not sure what's going on with the
> 2nd mmap gap though, but otherwise the gap matches the mmap size.

OK, after verification I agree.

> My patch exceeds 32-bit x86 functionality which does no randomization
> for legacy layouts. It matches 32-bit and 64-bit x86 for topdown layout.

I don't think we should skip randomization for the legacy layout.  
Without your patch we have only one layout and it is already randomized 
by default. There are other knobs for inhibiting randomization already.

> I'm happy to revert the legacy part of my patch, but it's a bit mute
> point as legacy is not used by default.

I'm actually wondering if we should carry a legacy layout at all on 
ARM...

Otherwise for the patch:

Acked-by: Nicolas Pitre <nico@linaro.org>


Nicolas

[PATCH 1/2] ARM: use cache type functions for arch_get_unmapped_area

[Nicolas Pitre]

On Mon, 7 Nov 2011, Rob Herring wrote:

> From: Rob Herring <rob.herring@calxeda.com>
> 
> There are already cache type decoding functions, so use those instead
> of custom decode code which only works for ARMv6.
> 
> This change also correctly enables cache colour alignment on Cortex-A9
> whose I-cache is aliasing VIPT.
> 
> Signed-off-by: Rob Herring <rob.herring@calxeda.com>

Acked-by: Nicolas Pitre <nico@linaro.org>


> ---
>  arch/arm/mm/mmap.c |   23 ++++++-----------------
>  1 files changed, 6 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/arm/mm/mmap.c b/arch/arm/mm/mmap.c
> index 74be05f..80e70ef 100644
> --- a/arch/arm/mm/mmap.c
> +++ b/arch/arm/mm/mmap.c
> @@ -9,8 +9,7 @@
>  #include <linux/io.h>
>  #include <linux/personality.h>
>  #include <linux/random.h>
> -#include <asm/cputype.h>
> -#include <asm/system.h>
> +#include <asm/cachetype.h>
>  
>  #define COLOUR_ALIGN(addr,pgoff)		\
>  	((((addr)+SHMLBA-1)&~(SHMLBA-1)) +	\
> @@ -32,25 +31,15 @@ arch_get_unmapped_area(struct file *filp, unsigned long addr,
>  	struct mm_struct *mm = current->mm;
>  	struct vm_area_struct *vma;
>  	unsigned long start_addr;
> -#if defined(CONFIG_CPU_V6) || defined(CONFIG_CPU_V6K)
> -	unsigned int cache_type;
> -	int do_align = 0, aliasing = 0;
> +	int do_align = 0;
> +	int aliasing = cache_is_vipt_aliasing() || icache_is_vipt_aliasing();
>  
>  	/*
>  	 * We only need to do colour alignment if either the I or D
> -	 * caches alias.  This is indicated by bits 9 and 21 of the
> -	 * cache type register.
> +	 * caches alias.
>  	 */
> -	cache_type = read_cpuid_cachetype();
> -	if (cache_type != read_cpuid_id()) {
> -		aliasing = (cache_type | cache_type >> 12) & (1 << 11);
> -		if (aliasing)
> -			do_align = filp || flags & MAP_SHARED;
> -	}
> -#else
> -#define do_align 0
> -#define aliasing 0
> -#endif
> +	if (aliasing)
> +		do_align = filp || (flags & MAP_SHARED);
>  
>  	/*
>  	 * We enforce the MAP_FIXED case.
> -- 
> 1.7.5.4
> 

[PATCH 1/2] ARM: use cache type functions for arch_get_unmapped_area

[Will Deacon]

Hi Rob, Nico,

On Mon, Nov 14, 2011 at 05:36:57PM +0000, Nicolas Pitre wrote:
> On Mon, 7 Nov 2011, Rob Herring wrote:
> 
> > From: Rob Herring <rob.herring@calxeda.com>
> > 
> > There are already cache type decoding functions, so use those instead
> > of custom decode code which only works for ARMv6.
> > 
> > This change also correctly enables cache colour alignment on Cortex-A9
> > whose I-cache is aliasing VIPT.

I appreciate that this is preserving the current behaviour, but why do we need
to ensure colour alignment when only the I-side is aliasing?

In the case that there is a MAP_SHARED, PROT_WRITE|PROT_EXEC mapping then
explicit cache maintenance will still be required by the task writing to the
page and also the task executing the written data (not to mention the
synchronisation requirements). Russell confirmed this recently:

http://lists.arm.linux.org.uk/lurker/message/20110923.193941.47decb13.en.html

no amount of colouring can avoid that, so why not only bother with it when
we alias on the D-side? On a coherent system it's reasonable to expect that
to work across tasks, so we definitely need the colouring correction there.

Will

[PATCH 1/2] ARM: use cache type functions for arch_get_unmapped_area

[Rob Herring]

On 11/14/2011 02:27 PM, Will Deacon wrote:
> Hi Rob, Nico,
> 
> On Mon, Nov 14, 2011 at 05:36:57PM +0000, Nicolas Pitre wrote:
>> On Mon, 7 Nov 2011, Rob Herring wrote:
>>
>>> From: Rob Herring <rob.herring@calxeda.com>
>>>
>>> There are already cache type decoding functions, so use those instead
>>> of custom decode code which only works for ARMv6.
>>>
>>> This change also correctly enables cache colour alignment on Cortex-A9
>>> whose I-cache is aliasing VIPT.
> 
> I appreciate that this is preserving the current behaviour, but why do we need
> to ensure colour alignment when only the I-side is aliasing?
> 
> In the case that there is a MAP_SHARED, PROT_WRITE|PROT_EXEC mapping then
> explicit cache maintenance will still be required by the task writing to the
> page and also the task executing the written data (not to mention the
> synchronisation requirements). Russell confirmed this recently:
> 
> http://lists.arm.linux.org.uk/lurker/message/20110923.193941.47decb13.en.html
> 
> no amount of colouring can avoid that, so why not only bother with it when
> we alias on the D-side? On a coherent system it's reasonable to expect that
> to work across tasks, so we definitely need the colouring correction there.

I wasn't too sure about that, so I'll drop the i-cache part.

Rob

[PATCH 1/2] ARM: use cache type functions for arch_get_unmapped_area

[Rob Herring]

On 11/14/2011 02:36 PM, Rob Herring wrote:
> On 11/14/2011 02:27 PM, Will Deacon wrote:
>> Hi Rob, Nico,
>>
>> On Mon, Nov 14, 2011 at 05:36:57PM +0000, Nicolas Pitre wrote:
>>> On Mon, 7 Nov 2011, Rob Herring wrote:
>>>
>>>> From: Rob Herring <rob.herring@calxeda.com>
>>>>
>>>> There are already cache type decoding functions, so use those instead
>>>> of custom decode code which only works for ARMv6.
>>>>
>>>> This change also correctly enables cache colour alignment on Cortex-A9
>>>> whose I-cache is aliasing VIPT.
>>
>> I appreciate that this is preserving the current behaviour, but why do we need
>> to ensure colour alignment when only the I-side is aliasing?
>>
>> In the case that there is a MAP_SHARED, PROT_WRITE|PROT_EXEC mapping then
>> explicit cache maintenance will still be required by the task writing to the
>> page and also the task executing the written data (not to mention the
>> synchronisation requirements). Russell confirmed this recently:
>>

What if you have 2 tasks that are executing code from the same page?
Perhaps it doesn't matter as the I caches are separate?

>> http://lists.arm.linux.org.uk/lurker/message/20110923.193941.47decb13.en.html
>>
>> no amount of colouring can avoid that, so why not only bother with it when
>> we alias on the D-side? On a coherent system it's reasonable to expect that
>> to work across tasks, so we definitely need the colouring correction there.
> 
> I wasn't too sure about that, so I'll drop the i-cache part.
> 

However, the original code and comments did check for aliasing I cache.
So dropping I-cache check could change v6 behavior if there are any
systems with only aliasing I cache.

Rob

[PATCH 1/2] ARM: use cache type functions for arch_get_unmapped_area

[Will Deacon]

On Tue, Nov 15, 2011 at 11:37:10PM +0000, Rob Herring wrote:
> On 11/14/2011 02:36 PM, Rob Herring wrote:
> > On 11/14/2011 02:27 PM, Will Deacon wrote:
> >> In the case that there is a MAP_SHARED, PROT_WRITE|PROT_EXEC mapping then
> >> explicit cache maintenance will still be required by the task writing to the
> >> page and also the task executing the written data (not to mention the
> >> synchronisation requirements). Russell confirmed this recently:
> >>
> 
> What if you have 2 tasks that are executing code from the same page?
> Perhaps it doesn't matter as the I caches are separate?

Indeed, in that case there's no problem. The only case where you might try
to use this is when you have one task writing code and another one executing
it. But in this case, the task doing the executing will need to take care of
the I-cache invalidation anyway because it could be VIVT ASID-tagged.

> >> http://lists.arm.linux.org.uk/lurker/message/20110923.193941.47decb13.en.html
> >>
> >> no amount of colouring can avoid that, so why not only bother with it when
> >> we alias on the D-side? On a coherent system it's reasonable to expect that
> >> to work across tasks, so we definitely need the colouring correction there.
> > 
> > I wasn't too sure about that, so I'll drop the i-cache part.
> > 
> 
> However, the original code and comments did check for aliasing I cache.
> So dropping I-cache check could change v6 behavior if there are any
> systems with only aliasing I cache.

It shouldn't affect correct code but I can always post this as a separate
patch if you're not happy with it.

Will