Patchwork [v3,1/5] ARM: protect usage of cr_alignment by #ifdef CONFIG_CPU_CP15

login
register
mail settings
Submitter Uwe Kleine-König
Date March 14, 2012, 10:21 a.m.
Message ID <1331720483-10075-1-git-send-email-u.kleine-koenig@pengutronix.de>
Download mbox | patch
Permalink /patch/146592/
State New
Headers show

Comments

Uwe Kleine-König - March 14, 2012, 10:21 a.m.
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 arch/arm/include/asm/system.h |    2 ++
 arch/arm/kernel/entry-armv.S  |    4 ++++
 arch/arm/kernel/head-common.S |   10 ++++++++--
 arch/arm/kernel/setup.c       |    8 ++++++--
 arch/arm/mm/alignment.c       |    6 ++++++
 arch/arm/mm/mmu.c             |    8 +++++++-
 6 files changed, 33 insertions(+), 5 deletions(-)
Nicolas Pitre - March 14, 2012, 3:24 p.m.
On Wed, 14 Mar 2012, Uwe Kleine-König wrote:

> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

I think you could reduce the amount of #ifdef's significantly here.  
While the cr_alignment variable and its sister are of no use in a 
no-CP15 system, you could still leave them there initialized to zero and 
only conditionally compile out the CP15 modifyers with a dummy version.  
This is only 2 words wasted in your kernel image to keep the code 
cleaner.

More comments below.

> index 854bd22..efeb2d0 100644
> --- a/arch/arm/kernel/head-common.S
> +++ b/arch/arm/kernel/head-common.S
> @@ -98,8 +98,10 @@ __mmap_switched:
>  	str	r9, [r4]			@ Save processor ID
>  	str	r1, [r5]			@ Save machine type
>  	str	r2, [r6]			@ Save atags pointer
> -	bic	r4, r0, #CR_A			@ Clear 'A' bit
> -	stmia	r7, {r0, r4}			@ Save control register values
> +	cmp	r7, #0
> +	itt	ne
> +	bicne	r4, r0, #CR_A			@ Clear 'A' bit
> +	stmneia	r7, {r0, r4}			@ Save control register values

The kernel is compiled with -mimplicit-it so you do not need to specify 
any it here.

> index a255c39..50d3df8 100644
> --- a/arch/arm/kernel/setup.c
> +++ b/arch/arm/kernel/setup.c
> @@ -472,9 +472,13 @@ static void __init setup_processor(void)
>  	cpu_cache = *list->cache;
>  #endif
>  
> -	printk("CPU: %s [%08x] revision %d (ARMv%s), cr=%08lx\n",
> +	printk("CPU: %s [%08x] revision %d (ARMv%s)",
>  	       cpu_name, read_cpuid_id(), read_cpuid_id() & 15,
> -	       proc_arch[cpu_architecture()], cr_alignment);
> +	       proc_arch[cpu_architecture()]);
> +
> +#ifdef CONFIG_CPU_CP15
> +	printk(KERN_CONT ", cr=%08lx\n", cr_alignment);
> +#endif
>  

Again, you could just leave the original display, with cr=00000000 which 
is a fairly good representation of reality.

> diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
> index caf14dc..119d178 100644
> --- a/arch/arm/mm/alignment.c
> +++ b/arch/arm/mm/alignment.c
> @@ -89,7 +89,11 @@ core_param(alignment, ai_usermode, int, 0600);
>  /* Return true if and only if the ARMv6 unaligned access model is in use. */
>  static bool cpu_is_v6_unaligned(void)
>  {
> +#ifdef CONFIG_CPU_CP15
>  	return cpu_architecture() >= CPU_ARCH_ARMv6 && (cr_alignment & CR_U);
> +#else
> +	return 0;
> +#endif
>  }

Same here.  With cr_alignment set to zero, you don't need the above 
#ifdef.

>  static int safe_usermode(int new_usermode, bool warn)
> @@ -961,12 +965,14 @@ static int __init alignment_init(void)
>  		return -ENOMEM;
>  #endif
>  
> +#ifdef CONFIG_CPU_CP15
>  	if (cpu_is_v6_unaligned()) {
>  		cr_alignment &= ~CR_A;
>  		cr_no_alignment &= ~CR_A;
>  		set_cr(cr_alignment);
>  		ai_usermode = safe_usermode(ai_usermode, false);
>  	}
> +#endif

Here the #ifdef is probably legitimate.

>  	hook_fault_code(FAULT_CODE_ALIGNMENT, do_alignment, SIGBUS, BUS_ADRALN,
>  			"alignment exception");
> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index 94c5a0c..f6dbe1a 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -109,8 +109,10 @@ static int __init early_cachepolicy(char *p)
>  
>  		if (memcmp(p, cache_policies[i].policy, len) == 0) {
>  			cachepolicy = i;
> +#ifdef CONFIG_CPU_CP15
>  			cr_alignment &= ~cache_policies[i].cr_mask;
>  			cr_no_alignment &= ~cache_policies[i].cr_mask;
> +#endif

Probably here as well.

>  			break;
>  		}
>  	}
> @@ -128,7 +130,9 @@ static int __init early_cachepolicy(char *p)
>  		cachepolicy = CPOLICY_WRITEBACK;
>  	}
>  	flush_cache_all();
> +#ifdef CONFIG_CPU_CP15
>  	set_cr(cr_alignment);
> +#endif

However it might be best to provide a dummy set_cr() instead of 
#ifdef'ing it out everywhere.


Nicolas
Uwe Kleine-König - March 14, 2012, 7:51 p.m.
On Wed, Mar 14, 2012 at 11:24:36AM -0400, Nicolas Pitre wrote:
> On Wed, 14 Mar 2012, Uwe Kleine-König wrote:
> 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> I think you could reduce the amount of #ifdef's significantly here.  
> While the cr_alignment variable and its sister are of no use in a 
> no-CP15 system, you could still leave them there initialized to zero and 
> only conditionally compile out the CP15 modifyers with a dummy version.  
> This is only 2 words wasted in your kernel image to keep the code 
> cleaner.
The main purpose to create this patch is that for v7m I don't compile
entry-armv.S. As cr_alignment is defined there I don't "have" this
variable. So I have to move the definition to a different file to waste
it :-)

> More comments below.
> 
> > index 854bd22..efeb2d0 100644
> > --- a/arch/arm/kernel/head-common.S
> > +++ b/arch/arm/kernel/head-common.S
> > @@ -98,8 +98,10 @@ __mmap_switched:
> >  	str	r9, [r4]			@ Save processor ID
> >  	str	r1, [r5]			@ Save machine type
> >  	str	r2, [r6]			@ Save atags pointer
> > -	bic	r4, r0, #CR_A			@ Clear 'A' bit
> > -	stmia	r7, {r0, r4}			@ Save control register values
> > +	cmp	r7, #0
> > +	itt	ne
> > +	bicne	r4, r0, #CR_A			@ Clear 'A' bit
> > +	stmneia	r7, {r0, r4}			@ Save control register values
> 
> The kernel is compiled with -mimplicit-it so you do not need to specify 
> any it here.
ok
 
> > index a255c39..50d3df8 100644
> > --- a/arch/arm/kernel/setup.c
> > +++ b/arch/arm/kernel/setup.c
> > @@ -472,9 +472,13 @@ static void __init setup_processor(void)
> >  	cpu_cache = *list->cache;
> >  #endif
> >  
> > -	printk("CPU: %s [%08x] revision %d (ARMv%s), cr=%08lx\n",
> > +	printk("CPU: %s [%08x] revision %d (ARMv%s)",
> >  	       cpu_name, read_cpuid_id(), read_cpuid_id() & 15,
> > -	       proc_arch[cpu_architecture()], cr_alignment);
> > +	       proc_arch[cpu_architecture()]);
> > +
> > +#ifdef CONFIG_CPU_CP15
> > +	printk(KERN_CONT ", cr=%08lx\n", cr_alignment);
> > +#endif
> >  
> 
> Again, you could just leave the original display, with cr=00000000 which 
> is a fairly good representation of reality.
Maybe having

	#define cr_alignment 0

for the !CONFIG_CPU_CP15 case would be a nice alternative. This way all
places that want to modify cr_alignment fail to compile, but reading
gives a "fairly good representation of reality".

Having said that I'm not sure about "fairly good". v7m also supports
unaligned accesses. But it's not configured in a cp15 register
(obviously) but in a system register. Quoting ARMARM-v7m:

	Configuration and Control Register, CCR
	[...]
	Bit [3] UNALIGN_TRP	Controls the trapping of unaligned word or
				halfword accesses:
				0 = Trapping disabled.
				1 = Trapping enabled.
				Unaligned load-store multiples and word
				or halfword exclusive accesses always
				fault.

So we need a more general abstraction to have correct and clean code?

But note I didn't check the two different implementations deeply to be
sure not to compare two different things.

> > diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
> > index caf14dc..119d178 100644
> > --- a/arch/arm/mm/alignment.c
> > +++ b/arch/arm/mm/alignment.c
> > @@ -89,7 +89,11 @@ core_param(alignment, ai_usermode, int, 0600);
> >  /* Return true if and only if the ARMv6 unaligned access model is in use. */
> >  static bool cpu_is_v6_unaligned(void)
> >  {
> > +#ifdef CONFIG_CPU_CP15
> >  	return cpu_architecture() >= CPU_ARCH_ARMv6 && (cr_alignment & CR_U);
> > +#else
> > +	return 0;
> > +#endif
> >  }
> 
> Same here.  With cr_alignment set to zero, you don't need the above 
> #ifdef.
> 
> >  static int safe_usermode(int new_usermode, bool warn)
> > @@ -961,12 +965,14 @@ static int __init alignment_init(void)
> >  		return -ENOMEM;
> >  #endif
> >  
> > +#ifdef CONFIG_CPU_CP15
> >  	if (cpu_is_v6_unaligned()) {
> >  		cr_alignment &= ~CR_A;
> >  		cr_no_alignment &= ~CR_A;
> >  		set_cr(cr_alignment);
> >  		ai_usermode = safe_usermode(ai_usermode, false);
> >  	}
> > +#endif
> 
> Here the #ifdef is probably legitimate.
> 
> >  	hook_fault_code(FAULT_CODE_ALIGNMENT, do_alignment, SIGBUS, BUS_ADRALN,
> >  			"alignment exception");
> > diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> > index 94c5a0c..f6dbe1a 100644
> > --- a/arch/arm/mm/mmu.c
> > +++ b/arch/arm/mm/mmu.c
> > @@ -109,8 +109,10 @@ static int __init early_cachepolicy(char *p)
> >  
> >  		if (memcmp(p, cache_policies[i].policy, len) == 0) {
> >  			cachepolicy = i;
> > +#ifdef CONFIG_CPU_CP15
> >  			cr_alignment &= ~cache_policies[i].cr_mask;
> >  			cr_no_alignment &= ~cache_policies[i].cr_mask;
> > +#endif
> 
> Probably here as well.
> 
> >  			break;
> >  		}
> >  	}
> > @@ -128,7 +130,9 @@ static int __init early_cachepolicy(char *p)
> >  		cachepolicy = CPOLICY_WRITEBACK;
> >  	}
> >  	flush_cache_all();
> > +#ifdef CONFIG_CPU_CP15
> >  	set_cr(cr_alignment);
> > +#endif
> 
> However it might be best to provide a dummy set_cr() instead of 
> #ifdef'ing it out everywhere.
What should the dummy set_cr do? Print a runtime warning if called with
!= 0?

Best regards
Uwe
Nicolas Pitre - March 14, 2012, 9:19 p.m.
On Wed, 14 Mar 2012, Uwe Kleine-König wrote:

> On Wed, Mar 14, 2012 at 11:24:36AM -0400, Nicolas Pitre wrote:
> > Again, you could just leave the original display, with cr=00000000 which 
> > is a fairly good representation of reality.
> Maybe having
> 
> 	#define cr_alignment 0
> 
> for the !CONFIG_CPU_CP15 case would be a nice alternative. This way all
> places that want to modify cr_alignment fail to compile, but reading
> gives a "fairly good representation of reality".

Even better, yes.

> Having said that I'm not sure about "fairly good". v7m also supports
> unaligned accesses. But it's not configured in a cp15 register
> (obviously) but in a system register.

Hence having a "control register" displaying as zero is still fairly 
good.

> Quoting ARMARM-v7m:
> 
> 	Configuration and Control Register, CCR
> 	[...]
> 	Bit [3] UNALIGN_TRP	Controls the trapping of unaligned word or
> 				halfword accesses:
> 				0 = Trapping disabled.
> 				1 = Trapping enabled.
> 				Unaligned load-store multiples and word
> 				or halfword exclusive accesses always
> 				fault.
> 
> So we need a more general abstraction to have correct and clean code?

In the context of the alignment trap code, yes.  Maybe on v7m there is 
no point having the alignment trap disabled at all.  It's been almost 
forever that gcc is not relying on the side effect of misaligned 
accesses anymore.

> > >  	flush_cache_all();
> > > +#ifdef CONFIG_CPU_CP15
> > >  	set_cr(cr_alignment);
> > > +#endif
> > 
> > However it might be best to provide a dummy set_cr() instead of 
> > #ifdef'ing it out everywhere.
> What should the dummy set_cr do? Print a runtime warning if called with
> != 0?

Maybe not.  In fact if that code path is used, then something might be 
wrong at a higher level.


Nicolas
Uwe Kleine-König - March 14, 2012, 9:53 p.m.
Hello Nicolas,

On Wed, Mar 14, 2012 at 05:19:53PM -0400, Nicolas Pitre wrote:
> On Wed, 14 Mar 2012, Uwe Kleine-König wrote:
> > On Wed, Mar 14, 2012 at 11:24:36AM -0400, Nicolas Pitre wrote:
> > > >  	flush_cache_all();
> > > > +#ifdef CONFIG_CPU_CP15
> > > >  	set_cr(cr_alignment);
> > > > +#endif
> > > 
> > > However it might be best to provide a dummy set_cr() instead of 
> > > #ifdef'ing it out everywhere.
> > What should the dummy set_cr do? Print a runtime warning if called with
> > != 0?
> 
> Maybe not.  In fact if that code path is used, then something might be 
> wrong at a higher level.
And that's why I choosed not to implement a dummy set_cr ...

Best regards
Uwe

Patch

diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
index e4c96cc..de46477 100644
--- a/arch/arm/include/asm/system.h
+++ b/arch/arm/include/asm/system.h
@@ -185,6 +185,7 @@  extern unsigned int user_debug;
 #define set_mb(var, value)	do { var = value; smp_mb(); } while (0)
 #define nop() __asm__ __volatile__("mov\tr0,r0\t@ nop\n\t");
 
+#ifdef CONFIG_CPU_CP15
 extern unsigned long cr_no_alignment;	/* defined in entry-armv.S */
 extern unsigned long cr_alignment;	/* defined in entry-armv.S */
 
@@ -224,6 +225,7 @@  static inline void set_copro_access(unsigned int val)
 	  : : "r" (val) : "cc");
 	isb();
 }
+#endif
 
 /*
  * switch_mm() may do a full cache flush over the context switch,
diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
index be16a48..a6f2ef5 100644
--- a/arch/arm/kernel/entry-armv.S
+++ b/arch/arm/kernel/entry-armv.S
@@ -314,8 +314,10 @@  __pabt_svc:
 ENDPROC(__pabt_svc)
 
 	.align	5
+#ifdef CONFIG_CPU_CP15
 .LCcralign:
 	.word	cr_alignment
+#endif
 #ifdef MULTI_DABORT
 .LCprocfns:
 	.word	processor
@@ -1146,12 +1148,14 @@  __vectors_end:
 
 	.data
 
+#ifdef CONFIG_CPU_CP15
 	.globl	cr_alignment
 	.globl	cr_no_alignment
 cr_alignment:
 	.space	4
 cr_no_alignment:
 	.space	4
+#endif
 
 #ifdef CONFIG_MULTI_IRQ_HANDLER
 	.globl	handle_arch_irq
diff --git a/arch/arm/kernel/head-common.S b/arch/arm/kernel/head-common.S
index 854bd22..efeb2d0 100644
--- a/arch/arm/kernel/head-common.S
+++ b/arch/arm/kernel/head-common.S
@@ -98,8 +98,10 @@  __mmap_switched:
 	str	r9, [r4]			@ Save processor ID
 	str	r1, [r5]			@ Save machine type
 	str	r2, [r6]			@ Save atags pointer
-	bic	r4, r0, #CR_A			@ Clear 'A' bit
-	stmia	r7, {r0, r4}			@ Save control register values
+	cmp	r7, #0
+	itt	ne
+	bicne	r4, r0, #CR_A			@ Clear 'A' bit
+	stmneia	r7, {r0, r4}			@ Save control register values
 	b	start_kernel
 ENDPROC(__mmap_switched)
 
@@ -113,7 +115,11 @@  __mmap_switched_data:
 	.long	processor_id			@ r4
 	.long	__machine_arch_type		@ r5
 	.long	__atags_pointer			@ r6
+#ifdef CONFIG_CPU_CP15
 	.long	cr_alignment			@ r7
+#else
+	.long	0				@ r7
+#endif
 	.long	init_thread_union + THREAD_START_SP @ sp
 	.size	__mmap_switched_data, . - __mmap_switched_data
 
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index a255c39..50d3df8 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -472,9 +472,13 @@  static void __init setup_processor(void)
 	cpu_cache = *list->cache;
 #endif
 
-	printk("CPU: %s [%08x] revision %d (ARMv%s), cr=%08lx\n",
+	printk("CPU: %s [%08x] revision %d (ARMv%s)",
 	       cpu_name, read_cpuid_id(), read_cpuid_id() & 15,
-	       proc_arch[cpu_architecture()], cr_alignment);
+	       proc_arch[cpu_architecture()]);
+
+#ifdef CONFIG_CPU_CP15
+	printk(KERN_CONT ", cr=%08lx\n", cr_alignment);
+#endif
 
 	snprintf(init_utsname()->machine, __NEW_UTS_LEN + 1, "%s%c",
 		 list->arch_name, ENDIANNESS);
diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
index caf14dc..119d178 100644
--- a/arch/arm/mm/alignment.c
+++ b/arch/arm/mm/alignment.c
@@ -89,7 +89,11 @@  core_param(alignment, ai_usermode, int, 0600);
 /* Return true if and only if the ARMv6 unaligned access model is in use. */
 static bool cpu_is_v6_unaligned(void)
 {
+#ifdef CONFIG_CPU_CP15
 	return cpu_architecture() >= CPU_ARCH_ARMv6 && (cr_alignment & CR_U);
+#else
+	return 0;
+#endif
 }
 
 static int safe_usermode(int new_usermode, bool warn)
@@ -961,12 +965,14 @@  static int __init alignment_init(void)
 		return -ENOMEM;
 #endif
 
+#ifdef CONFIG_CPU_CP15
 	if (cpu_is_v6_unaligned()) {
 		cr_alignment &= ~CR_A;
 		cr_no_alignment &= ~CR_A;
 		set_cr(cr_alignment);
 		ai_usermode = safe_usermode(ai_usermode, false);
 	}
+#endif
 
 	hook_fault_code(FAULT_CODE_ALIGNMENT, do_alignment, SIGBUS, BUS_ADRALN,
 			"alignment exception");
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 94c5a0c..f6dbe1a 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -109,8 +109,10 @@  static int __init early_cachepolicy(char *p)
 
 		if (memcmp(p, cache_policies[i].policy, len) == 0) {
 			cachepolicy = i;
+#ifdef CONFIG_CPU_CP15
 			cr_alignment &= ~cache_policies[i].cr_mask;
 			cr_no_alignment &= ~cache_policies[i].cr_mask;
+#endif
 			break;
 		}
 	}
@@ -128,7 +130,9 @@  static int __init early_cachepolicy(char *p)
 		cachepolicy = CPOLICY_WRITEBACK;
 	}
 	flush_cache_all();
+#ifdef CONFIG_CPU_CP15
 	set_cr(cr_alignment);
+#endif
 	return 0;
 }
 early_param("cachepolicy", early_cachepolicy);
@@ -163,6 +167,7 @@  static int __init early_ecc(char *p)
 early_param("ecc", early_ecc);
 #endif
 
+#ifdef CONFIG_CPU_CP15
 static int __init noalign_setup(char *__unused)
 {
 	cr_alignment &= ~CR_A;
@@ -171,8 +176,9 @@  static int __init noalign_setup(char *__unused)
 	return 1;
 }
 __setup("noalign", noalign_setup);
+#endif
 
-#ifndef CONFIG_SMP
+#if !defined(CONFIG_SMP) && defined(CONFIG_CPU_CP15)
 void adjust_cr(unsigned long mask, unsigned long set)
 {
 	unsigned long flags;