[PATCH] ARM: alignment: setup alignment handler earlier

[PATCH] ARM: alignment: setup alignment handler earlier

[John Ogness]

[PATCH] ARM: alignment: setup alignment handler earlier

[Måns Rullgård]

John Ogness <john.ogness@linutronix.de> writes:

> From 6f3f381800367127dc6430d9b9fa9bd6fc6d8ed0 Mon Sep 17 00:00:00 2001
> From: John Ogness <john.ogness@linutronix.de>
>
> The alignment exception handler is setup fairly late in
> the boot process (fs_initcall). However, with newer gcc
> versions and the compiler option -fconserve-stack, code
> accessing unaligned data is generated in functions that
> are called earlier, for example pcpu_dump_alloc_info().

This is a gcc bug and should not be worked around like this.  There was
another patch[1] to disable -fconserve-stack on ARM due to this broken
behaviour.  This patch was the outcome of a rather lengthy discussion[2].

[1] http://article.gmane.org/gmane.linux.kernel/1148272
[2] http://thread.gmane.org/gmane.linux.ports.arm.kernel/117863

> This results in unhandled alignment exceptions during
> boot. By setting up the exception handler sooner, we
> reduce the window where a compiler may generate code
> accessing unaligned data.

I will also restate my opinion that enabling strict alignment checking
on ARMv6 and later is *wrong* in the first place.

-- 
M?ns Rullg?rd
mans at mansr.com

[PATCH] ARM: alignment: setup alignment handler earlier

[Catalin Marinas]

2011/9/7 M?ns Rullg?rd <mans@mansr.com>:
> John Ogness <john.ogness@linutronix.de> writes:
>
>> From 6f3f381800367127dc6430d9b9fa9bd6fc6d8ed0 Mon Sep 17 00:00:00 2001
>> From: John Ogness <john.ogness@linutronix.de>
>>
>> The alignment exception handler is setup fairly late in
>> the boot process (fs_initcall). However, with newer gcc
>> versions and the compiler option -fconserve-stack, code
>> accessing unaligned data is generated in functions that
>> are called earlier, for example pcpu_dump_alloc_info().
>
> This is a gcc bug and should not be worked around like this. ?There was
> another patch[1] to disable -fconserve-stack on ARM due to this broken
> behaviour. ?This patch was the outcome of a rather lengthy discussion[2].
>
> [1] http://article.gmane.org/gmane.linux.kernel/1148272
> [2] http://thread.gmane.org/gmane.linux.ports.arm.kernel/117863

I got some feedback from the gcc guys here in ARM (but then I went on
holiday and forgot about this). They convinced me that there isn't any
gcc bug, it is behaving correctly. Even with -fconserve-stack, the
variables on the stack are naturally aligned as expected (ints to
32-bit boundary etc.) The unaligned access happens when gcc populates
a char[9] string on the stack - see the pcpu_dump_alloc_info()
function. Since unaligned accesses are on by default, gcc generates
some unaligned STR to populate the string on the stack. This looks
like a perfectly valid behaviour.

>> This results in unhandled alignment exceptions during
>> boot. By setting up the exception handler sooner, we
>> reduce the window where a compiler may generate code
>> accessing unaligned data.
>
> I will also restate my opinion that enabling strict alignment checking
> on ARMv6 and later is *wrong* in the first place.

That's probably the solution.

-- 
Catalin

[PATCH] ARM: alignment: setup alignment handler earlier

[Catalin Marinas]

On Wed, Sep 07, 2011 at 02:35:04PM +0100, John Ogness wrote:
> From 6f3f381800367127dc6430d9b9fa9bd6fc6d8ed0 Mon Sep 17 00:00:00 2001
> From: John Ogness <john.ogness@linutronix.de>
> 
> The alignment exception handler is setup fairly late in
> the boot process (fs_initcall). However, with newer gcc
> versions and the compiler option -fconserve-stack, code
> accessing unaligned data is generated in functions that
> are called earlier, for example pcpu_dump_alloc_info().
> This results in unhandled alignment exceptions during
> boot. By setting up the exception handler sooner, we
> reduce the window where a compiler may generate code
> accessing unaligned data.

While this reduces the window and fixes this particular case, it still
doesn't solve the problem. We never know when some unaligned access
would be generated for some earlier code.

I would just make sure that SCTLR.A bit is always cleared on ARMv6+,
independent of CONFIG_ALIGNMENT_TRAP.

-- 
Catalin

[PATCH] ARM: alignment: setup alignment handler earlier

[Russell King - ARM Linux]

On Wed, Sep 07, 2011 at 03:40:16PM +0100, Catalin Marinas wrote:
> On Wed, Sep 07, 2011 at 02:35:04PM +0100, John Ogness wrote:
> > From 6f3f381800367127dc6430d9b9fa9bd6fc6d8ed0 Mon Sep 17 00:00:00 2001
> > From: John Ogness <john.ogness@linutronix.de>
> > 
> > The alignment exception handler is setup fairly late in
> > the boot process (fs_initcall). However, with newer gcc
> > versions and the compiler option -fconserve-stack, code
> > accessing unaligned data is generated in functions that
> > are called earlier, for example pcpu_dump_alloc_info().
> > This results in unhandled alignment exceptions during
> > boot. By setting up the exception handler sooner, we
> > reduce the window where a compiler may generate code
> > accessing unaligned data.
> 
> While this reduces the window and fixes this particular case, it still
> doesn't solve the problem. We never know when some unaligned access
> would be generated for some earlier code.

Is the problem even solvable?  There are instructions on ARMv6+ which
always produce an alignment fault (eg, ldrd) irrespective of strict
alignment checking.

There will always be a window for those - when we don't have the
vectors setup - where we potentically could take such a fault and end
up crashing.  So I'm not sure that the right answer is what's being
proposed.

What it's saying to me is that building the kernel in a way that gcc
intentionally generates misaligned accesses _will_ bite us in random
unknown ways sooner or later.

I don't think its feasible to build some of the kernel with alignment
faults enabled and other parts with it disabled - that's going to be
very fragile and probably be hell to identify which parts should and
should not.

So I think where we're heading is to need gcc _not_ to create any code
what so ever which would create a misalignment fault.

[PATCH] ARM: alignment: setup alignment handler earlier

[Måns Rullgård]

Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

> On Wed, Sep 07, 2011 at 03:40:16PM +0100, Catalin Marinas wrote:
>> On Wed, Sep 07, 2011 at 02:35:04PM +0100, John Ogness wrote:
>> > From 6f3f381800367127dc6430d9b9fa9bd6fc6d8ed0 Mon Sep 17 00:00:00 2001
>> > From: John Ogness <john.ogness@linutronix.de>
>> > 
>> > The alignment exception handler is setup fairly late in
>> > the boot process (fs_initcall). However, with newer gcc
>> > versions and the compiler option -fconserve-stack, code
>> > accessing unaligned data is generated in functions that
>> > are called earlier, for example pcpu_dump_alloc_info().
>> > This results in unhandled alignment exceptions during
>> > boot. By setting up the exception handler sooner, we
>> > reduce the window where a compiler may generate code
>> > accessing unaligned data.
>> 
>> While this reduces the window and fixes this particular case, it still
>> doesn't solve the problem. We never know when some unaligned access
>> would be generated for some earlier code.
>
> Is the problem even solvable?  There are instructions on ARMv6+ which
> always produce an alignment fault (eg, ldrd) irrespective of strict
> alignment checking.

There are such instructions (ldrd, ldm), but gcc will not emit those
unless the address is known to be aligned.  For ARMv6 and later, gcc 4.6
*will* emit potentially unaligned ldr and ldrh since these very clearly
allow an unaligned address and are faster than the alternatives in all
implementations to date.  This is unless strict alignment checking is
explicitly enabled, which unfortunately the Linux kernel does for no
apparent reason at all.

> There will always be a window for those - when we don't have the
> vectors setup - where we potentically could take such a fault and end
> up crashing.  So I'm not sure that the right answer is what's being
> proposed.

The right answer is to not enable strict alignment checking in the first
place.

> What it's saying to me is that building the kernel in a way that gcc
> intentionally generates misaligned accesses _will_ bite us in random
> unknown ways sooner or later.

GCC only generates unaligned accesses using instructions which support
this.

> I don't think its feasible to build some of the kernel with alignment
> faults enabled and other parts with it disabled - that's going to be
> very fragile and probably be hell to identify which parts should and
> should not.

Indeed, so the sane solution must be to not enable strict checking
anywhere.

> So I think where we're heading is to need gcc _not_ to create any code
> what so ever which would create a misalignment fault.

This is already the case provided the alignment requirements are not
artificially tightened.  What we need is to not enable strict alignment
checking in the kernel as is currently done.

-- 
M?ns Rullg?rd
mans at mansr.com

[PATCH] ARM: alignment: setup alignment handler earlier

[Catalin Marinas]

On 7 September 2011 17:28, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Sep 07, 2011 at 03:40:16PM +0100, Catalin Marinas wrote:
>> On Wed, Sep 07, 2011 at 02:35:04PM +0100, John Ogness wrote:
>> > From 6f3f381800367127dc6430d9b9fa9bd6fc6d8ed0 Mon Sep 17 00:00:00 2001
>> > From: John Ogness <john.ogness@linutronix.de>
>> >
>> > The alignment exception handler is setup fairly late in
>> > the boot process (fs_initcall). However, with newer gcc
>> > versions and the compiler option -fconserve-stack, code
>> > accessing unaligned data is generated in functions that
>> > are called earlier, for example pcpu_dump_alloc_info().
>> > This results in unhandled alignment exceptions during
>> > boot. By setting up the exception handler sooner, we
>> > reduce the window where a compiler may generate code
>> > accessing unaligned data.
>>
>> While this reduces the window and fixes this particular case, it still
>> doesn't solve the problem. We never know when some unaligned access
>> would be generated for some earlier code.
>
> Is the problem even solvable? ?There are instructions on ARMv6+ which
> always produce an alignment fault (eg, ldrd) irrespective of strict
> alignment checking.

We can just hope that the compiler isn't that bad as to generate
unaligned accesses with those instructions that are known to fault.
That's what I would consider a compiler bug.

-- 
Catalin

[PATCH] ARM: alignment: setup alignment handler earlier

[Russell King - ARM Linux]

On Wed, Sep 07, 2011 at 05:42:19PM +0100, M?ns Rullg?rd wrote:
> There are such instructions (ldrd, ldm), but gcc will not emit those
> unless the address is known to be aligned.  For ARMv6 and later, gcc 4.6
> *will* emit potentially unaligned ldr and ldrh since these very clearly
> allow an unaligned address and are faster than the alternatives in all
> implementations to date.  This is unless strict alignment checking is
> explicitly enabled, which unfortunately the Linux kernel does for no
> apparent reason at all.

"no apparant reason at all" heh.  The reason is to keep the code
simple and free from bugs.  To do otherwise means that each of the
CPU files needs to be littered with ifdefs to deal with the alignment
fault configuration, of which there are 16 of them (ignoring v6 and v7.)

If you think code maintanence of the same thing in 16 places is efficient
then I guess there is "no apparant reason".  I beg to differ, being one
of those folk who have had to edit 18 different places several times.

So no, I do not intend to move this:

#ifdef CONFIG_ALIGNMENT_TRAP
        orr     r0, r0, #CR_A
#else
        bic     r0, r0, #CR_A
#endif

into 16 separate places in the kernel.

[PATCH] ARM: alignment: setup alignment handler earlier

[Catalin Marinas]

On Wed, Sep 07, 2011 at 06:04:52PM +0100, Russell King - ARM Linux wrote:
> On Wed, Sep 07, 2011 at 05:42:19PM +0100, M?ns Rullg?rd wrote:
> > There are such instructions (ldrd, ldm), but gcc will not emit those
> > unless the address is known to be aligned.  For ARMv6 and later, gcc 4.6
> > *will* emit potentially unaligned ldr and ldrh since these very clearly
> > allow an unaligned address and are faster than the alternatives in all
> > implementations to date.  This is unless strict alignment checking is
> > explicitly enabled, which unfortunately the Linux kernel does for no
> > apparent reason at all.
> 
> "no apparant reason at all" heh.  The reason is to keep the code
> simple and free from bugs.  To do otherwise means that each of the
> CPU files needs to be littered with ifdefs to deal with the alignment
> fault configuration, of which there are 16 of them (ignoring v6 and v7.)
> 
> If you think code maintanence of the same thing in 16 places is efficient
> then I guess there is "no apparant reason".  I beg to differ, being one
> of those folk who have had to edit 18 different places several times.
> 
> So no, I do not intend to move this:
> 
> #ifdef CONFIG_ALIGNMENT_TRAP
>         orr     r0, r0, #CR_A
> #else
>         bic     r0, r0, #CR_A
> #endif
> 
> into 16 separate places in the kernel.

What about something like this (untested):

#if defined(CONFIG_ALIGNMENT_TRAP) && __LINUX_ARM_ARCH__ < 6
	orr	r0, r0, #CR_A
#else
	bic	r0, r0, #CR_A
#endif

-- 
Catalin

[PATCH] ARM: alignment: setup alignment handler earlier

[Måns Rullgård]

Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

> On Wed, Sep 07, 2011 at 05:42:19PM +0100, M?ns Rullg?rd wrote:
>> There are such instructions (ldrd, ldm), but gcc will not emit those
>> unless the address is known to be aligned.  For ARMv6 and later, gcc 4.6
>> *will* emit potentially unaligned ldr and ldrh since these very clearly
>> allow an unaligned address and are faster than the alternatives in all
>> implementations to date.  This is unless strict alignment checking is
>> explicitly enabled, which unfortunately the Linux kernel does for no
>> apparent reason at all.
>
> "no apparant reason at all" heh.  The reason is to keep the code
> simple and free from bugs.

Some people, myself included, consider the current behaviour a bug.

> To do otherwise means that each of the CPU files needs to be littered
> with ifdefs to deal with the alignment fault configuration, of which
> there are 16 of them (ignoring v6 and v7.)
>
> If you think code maintanence of the same thing in 16 places is efficient
> then I guess there is "no apparant reason".  I beg to differ, being one
> of those folk who have had to edit 18 different places several times.
>
> So no, I do not intend to move this:
>
> #ifdef CONFIG_ALIGNMENT_TRAP
>         orr     r0, r0, #CR_A
> #else
>         bic     r0, r0, #CR_A
> #endif
>
> into 16 separate places in the kernel.

So change that condition to also depend on !CPU_V6 && !CPU_V7 or
something equivalent.

-- 
M?ns Rullg?rd
mans at mansr.com