diff mbox

[U-Boot,RFC] ARM: prevent misaligned array inits

Message ID 1349203564-27150-1-git-send-email-albert.u.boot@aribaud.net
State RFC
Headers show

Commit Message

Albert ARIBAUD Oct. 2, 2012, 6:46 p.m. UTC 
Under option -munaligned-access, gcc can perform local char
or 16-bit array initializations using misaligned native
accesses which will throw a data abort exception. Fix files
where these array initializations were unneeded, and for
files known to contain such initializations, enforce gcc
option -mno-unaligned-access.

Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
---
Please test this patch with gcc 4.7 on boards which do data aborts
or resets due to misaligned accesses and report result to me.

 arch/arm/cpu/arm926ejs/orion5x/cpu.c |    4 +-
 board/ti/omap2420h4/sys_info.c       |   24 ++++-----
 common/Makefile                      |    3 ++
 common/cmd_dfu.c                     |    2 +-
 doc/README.arm-unaligned-accesses    |   95 ++++++++++++++++++++++++++++++++++
 fs/fat/Makefile                      |    2 +
 fs/ubifs/Makefile                    |    3 ++
 lib/Makefile                         |    3 ++
 8 files changed, 121 insertions(+), 15 deletions(-)
 create mode 100644 doc/README.arm-unaligned-accesses

Comments

Joakim Tjernlund Oct. 2, 2012, 7:13 p.m. UTC | #1 
>   *********************************************************************/
>  void display_board_info(u32 btype)
>  {
> -   char cpu_2420[] = "2420";   /* cpu type */
> -   char cpu_2422[] = "2422";
> -   char cpu_2423[] = "2423";
> -   char db_men[] = "Menelaus"; /* board type */
> -   char db_ip[] = "IP";
> -   char mem_sdr[] = "mSDR";    /* memory type */
> -   char mem_ddr[] = "mDDR";
> -   char t_tst[] = "TST";       /* security level */
> -   char t_emu[] = "EMU";
> -   char t_hs[] = "HS";
> -   char t_gp[] = "GP";
> -   char unk[] = "?";
> +   char *cpu_2420 = "2420";   /* cpu type */
> +   char *cpu_2422 = "2422";
> +   char *cpu_2423 = "2423";
> +   char *db_men = "Menelaus"; /* board type */
> +   char *db_ip = "IP";
> +   char *mem_sdr = "mSDR";    /* memory type */
> +   char *mem_ddr = "mDDR";
> +   char *t_tst = "TST";       /* security level */
> +   char *t_emu = "EMU";
> +   char *t_hs = "HS";
> +   char *t_gp = "GP";
> +   char *unk = "?";

hmm, on ppc I think this will cause relocation entries which will build size.

 Jocke
Albert ARIBAUD Oct. 2, 2012, 8:05 p.m. UTC | #2 
Hi Joakim,

On Tue, 2 Oct 2012 21:13:39 +0200, Joakim Tjernlund
<joakim.tjernlund@transmode.se> wrote:

> 
> 
> >   *********************************************************************/
> >  void display_board_info(u32 btype)
> >  {
> > -   char cpu_2420[] = "2420";   /* cpu type */
> > -   char cpu_2422[] = "2422";
> > -   char cpu_2423[] = "2423";
> > -   char db_men[] = "Menelaus"; /* board type */
> > -   char db_ip[] = "IP";
> > -   char mem_sdr[] = "mSDR";    /* memory type */
> > -   char mem_ddr[] = "mDDR";
> > -   char t_tst[] = "TST";       /* security level */
> > -   char t_emu[] = "EMU";
> > -   char t_hs[] = "HS";
> > -   char t_gp[] = "GP";
> > -   char unk[] = "?";
> > +   char *cpu_2420 = "2420";   /* cpu type */
> > +   char *cpu_2422 = "2422";
> > +   char *cpu_2423 = "2423";
> > +   char *db_men = "Menelaus"; /* board type */
> > +   char *db_ip = "IP";
> > +   char *mem_sdr = "mSDR";    /* memory type */
> > +   char *mem_ddr = "mDDR";
> > +   char *t_tst = "TST";       /* security level */
> > +   char *t_emu = "EMU";
> > +   char *t_hs = "HS";
> > +   char *t_gp = "GP";
> > +   char *unk = "?";
> 
> hmm, on ppc I think this will cause relocation entries which will build size.
> 
>  Jocke

Can you try it and post results with and without it? Sizes and if
possible disassembly of the function for both cases.

Would it better if the strings were used directly in the function body?
I could replace the char* locals with #defines, for instance.

Amicalement,
Stephen Warren Oct. 2, 2012, 8:06 p.m. UTC | #3 
On 10/02/2012 12:46 PM, Albert ARIBAUD wrote:
> Under option -munaligned-access, gcc can perform local char
> or 16-bit array initializations using misaligned native
> accesses which will throw a data abort exception. Fix files
> where these array initializations were unneeded, and for
> files known to contain such initializations, enforce gcc
> option -mno-unaligned-access.

> +++ b/doc/README.arm-unaligned-accesses
...
> +However this will only apply to the version of gcc which will have such
> +an option. For other versions, there are four workarounds:
> +
> +a) Enforce as a rule that array initializations as described above
> +   are forbidden. This is generally not acceptable as they are valid,
> +   and usual, C constructs. The only case where they could be rejected
> +   is when they actually equate to a const char* declaration, i.e. the
> +   array is initialized and never modified in the function's scope.
> +
> +b) Drop the requirement on unaligned accesses at least for ARMv7,
> +   i.e. do not throw a data abort exception upon unaligned accesses.
> +   But that will allow adding badly aligned code to U-Boot, only for
> +   it to fail when re-used with another, more strict, target, possibly
> +   once the bad code is already in mainline.
> +
> +c) Relax the -munified-access rule globally. This will prevent native

I assume that's meant to say -munaligned-access?

> +   unaligned accesses of course, but that will also hide any bug caused
> +   by a bad unaligned access, making it much harder to diagnose it. It
> +   is actually what already happens when building ARM targets with a
> +   pre-4.7 gcc, and it may actually already hide some bugs yet unseen
> +   until the target gets compiled with m-unaligned-access.

s/m-/-m/

> +d) Relax the -munified-access rule only for for files susceptible to

I assume that's meant to say -munaligned-access?

> +   the local initialized array issue. This minimizes the quantity of
> +   code which can hide unwanted misaligned accesses.
> +
> +Considering the rarity of actual occurrences (as of this writing, 5
> +files out of 7840 in U-Boot, or .3%, contain an initialized local char
> +array which cannot actually be replaced with a const char*), detection
> +if the issue in patches should not be asked from contributors.

I assume therefore that option (d) was chosen. Perhaps state this
explicitly?
Albert ARIBAUD Oct. 2, 2012, 9:20 p.m. UTC | #4 
Hi Stephen,

On Tue, 02 Oct 2012 14:06:53 -0600, Stephen Warren
<swarren@wwwdotorg.org> wrote:

> > +c) Relax the -munified-access rule globally. This will prevent native
> 
> I assume that's meant to say -munaligned-access?

> > +   until the target gets compiled with m-unaligned-access.
> 
> s/m-/-m/

> > +d) Relax the -munified-access rule only for for files susceptible to
> 
> I assume that's meant to say -munaligned-access?

Thanks for spotting these. Fixed in next round.

> > +   the local initialized array issue. This minimizes the quantity of
> > +   code which can hide unwanted misaligned accesses.
> > +
> > +Considering the rarity of actual occurrences (as of this writing, 5
> > +files out of 7840 in U-Boot, or .3%, contain an initialized local char
> > +array which cannot actually be replaced with a const char*), detection
> > +if the issue in patches should not be asked from contributors.
> 
> I assume therefore that option (d) was chosen. Perhaps state this
> explicitly?

Thanks for pointing out the ambiguity: indeed option d) is the one
chosen. Made explicit in next round.

Thanks again!

Amicalement,
Måns Rullgård Oct. 2, 2012, 9:42 p.m. UTC | #5 
Albert ARIBAUD <albert.u.boot@aribaud.net> writes:

> Under option -munaligned-access, gcc can perform local char
> or 16-bit array initializations using misaligned native
> accesses which will throw a data abort exception. Fix files
> where these array initializations were unneeded, and for
> files known to contain such initializations, enforce gcc
> option -mno-unaligned-access.

Why don't you just stop setting the damn strict alignment flag on ARMv6
and later instead this endless hacking around?

You really have only two sane choices here:

1. Keep strict alignment checking and build with -mno-unaligned-access.
2. Drop strict alignment checking and build with (implicit) -munaligned-access.

Option 2 gives faster, smaller code, so the choice should be an easy one
to make.
Mark Marshall Oct. 3, 2012, 7:29 a.m. UTC | #6 
I've just had a quick look at this in passing, but at least some of
these changes seem wrong to me.  For example, the code in
board/ti/omap2420h4/sys_info.c :: display_board_info
should be:

void display_board_info(u32 btype)
{
	static const char cpu_2420[] = "2420";   /* cpu type */
	static const char cpu_2422[] = "2422";
	static const char cpu_2423[] = "2423";
	static const char db_men[] = "Menelaus"; /* board type */
	static const char db_ip[] = "IP";
	static const char mem_sdr[] = "mSDR";    /* memory type */
	static const char mem_ddr[] = "mDDR";
	static const char t_tst[] = "TST";	    /* security level */
	static const char t_emu[] = "EMU";
	static const char t_hs[] = "HS";
	static const char t_gp[] = "GP";
	static const char unk[] = "?";

	const char *cpu_s, *db_s, *mem_s, *sec_s;
	u32 cpu, rev, sec;

This produces smaller code and is probably what the original
author wanted the compiler to do.  I've only compile tested
this, not actually run it.

Original file:

Sections:
Idx Name          Size      VMA       LMA       File off  Algn
   0 .text         000004b4  00000000  00000000  00000034  2**2
                   CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
   1 .data         00000000  00000000  00000000  000004e8  2**0
                   CONTENTS, ALLOC, LOAD, DATA
   2 .bss          00000000  00000000  00000000  000004e8  2**0
                   ALLOC
   3 .rodata.str1.1 00000072  00000000  00000000  000004e8  2**0
                   CONTENTS, ALLOC, LOAD, READONLY, DATA

After my changes:

Sections:
Idx Name          Size      VMA       LMA       File off  Algn
   0 .text         000003ac  00000000  00000000  00000034  2**2
                   CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
   1 .data         00000000  00000000  00000000  000003e0  2**0
                   CONTENTS, ALLOC, LOAD, DATA
   2 .bss          00000000  00000000  00000000  000003e0  2**0
                   ALLOC
   3 .rodata       00000048  00000000  00000000  000003e0  2**2
                   CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA
   4 .rodata.str1.1 00000047  00000000  00000000  00000428  2**0
                   CONTENTS, ALLOC, LOAD, READONLY, DATA


Regards,

Mark Marshall.

On 10/02/2012 08:46 PM, Albert ARIBAUD wrote:
> Under option -munaligned-access, gcc can perform local char
> or 16-bit array initializations using misaligned native
> accesses which will throw a data abort exception. Fix files
> where these array initializations were unneeded, and for
> files known to contain such initializations, enforce gcc
> option -mno-unaligned-access.
>
> Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> ---
> Please test this patch with gcc 4.7 on boards which do data aborts
> or resets due to misaligned accesses and report result to me.
>
>   arch/arm/cpu/arm926ejs/orion5x/cpu.c |    4 +-
>   board/ti/omap2420h4/sys_info.c       |   24 ++++-----
>   common/Makefile                      |    3 ++
>   common/cmd_dfu.c                     |    2 +-
>   doc/README.arm-unaligned-accesses    |   95 ++++++++++++++++++++++++++++++++++
>   fs/fat/Makefile                      |    2 +
>   fs/ubifs/Makefile                    |    3 ++
>   lib/Makefile                         |    3 ++
>   8 files changed, 121 insertions(+), 15 deletions(-)
>   create mode 100644 doc/README.arm-unaligned-accesses
>
> diff --git a/arch/arm/cpu/arm926ejs/orion5x/cpu.c b/arch/arm/cpu/arm926ejs/orion5x/cpu.c
> index c3948d3..5a4775a 100644
> --- a/arch/arm/cpu/arm926ejs/orion5x/cpu.c
> +++ b/arch/arm/cpu/arm926ejs/orion5x/cpu.c
> @@ -194,8 +194,8 @@ u32 orion5x_device_rev(void)
>    */
>   int print_cpuinfo(void)
>   {
> -	char dev_str[] = "0x0000";
> -	char rev_str[] = "0x00";
> +	char dev_str[7]; /* room enough for 0x0000 plus null byte */
> +	char rev_str[5]; /* room enough for 0x00 plus null byte */
>   	char *dev_name = NULL;
>   	char *rev_name = NULL;
>
> diff --git a/board/ti/omap2420h4/sys_info.c b/board/ti/omap2420h4/sys_info.c
> index a9f7241..b462aa5 100644
> --- a/board/ti/omap2420h4/sys_info.c
> +++ b/board/ti/omap2420h4/sys_info.c
> @@ -237,18 +237,18 @@ u32 wait_on_value(u32 read_bit_mask, u32 match_value, u32 read_addr, u32 bound)
>    *********************************************************************/
>   void display_board_info(u32 btype)
>   {
> -	char cpu_2420[] = "2420";   /* cpu type */
> -	char cpu_2422[] = "2422";
> -	char cpu_2423[] = "2423";
> -	char db_men[] = "Menelaus"; /* board type */
> -	char db_ip[] = "IP";
> -	char mem_sdr[] = "mSDR";    /* memory type */
> -	char mem_ddr[] = "mDDR";
> -	char t_tst[] = "TST";	    /* security level */
> -	char t_emu[] = "EMU";
> -	char t_hs[] = "HS";
> -	char t_gp[] = "GP";
> -	char unk[] = "?";
> +	char *cpu_2420 = "2420";   /* cpu type */
> +	char *cpu_2422 = "2422";
> +	char *cpu_2423 = "2423";
> +	char *db_men = "Menelaus"; /* board type */
> +	char *db_ip = "IP";
> +	char *mem_sdr = "mSDR";    /* memory type */
> +	char *mem_ddr = "mDDR";
> +	char *t_tst = "TST";	    /* security level */
> +	char *t_emu = "EMU";
> +	char *t_hs = "HS";
> +	char *t_gp = "GP";
> +	char *unk = "?";
>
>   	char *cpu_s, *db_s, *mem_s, *sec_s;
>   	u32 cpu, rev, sec;
> diff --git a/common/Makefile b/common/Makefile
> index 125b2be..19b2130 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -227,6 +227,9 @@ $(obj)env_embedded.o: $(src)env_embedded.c $(obj)../tools/envcrc
>   $(obj)../tools/envcrc:
>   	$(MAKE) -C ../tools
>
> +$(obj)hush.o: CFLAGS += -mno-unaligned-access
> +$(obj)fdt_support.o: CFLAGS += -mno-unaligned-access
> +
>   #########################################################################
>
>   # defines $(obj).depend target
> diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c
> index 62fb890..01d6b3a 100644
> --- a/common/cmd_dfu.c
> +++ b/common/cmd_dfu.c
> @@ -30,7 +30,7 @@
>   static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>   {
>   	const char *str_env;
> -	char s[] = "dfu";
> +	char *s = "dfu";
>   	char *env_bkp;
>   	int ret;
>
> diff --git a/doc/README.arm-unaligned-accesses b/doc/README.arm-unaligned-accesses
> new file mode 100644
> index 0000000..00fb1c0
> --- /dev/null
> +++ b/doc/README.arm-unaligned-accesses
> @@ -0,0 +1,95 @@
> +Since U-Boot runs on a variety of hardware, some only able to perform
> +unaligned accesses with a strong penalty, some unable to perform them
> +at all, the policy regarding unaligned accesses is to not perform any,
> +unless absolutely necessary because of hardware or standards.
> +
> +Also, on hardware which permits it, the core is configured to throw
> +data abort exceptions on unaligned accesses  in order to catch these
> +unallowed accesses as early as possible.
> +
> +Until version 4.7, the gcc default for performing unaligned accesses
> +(-mno-unaligned-access) is to emulate unaligned accesses using aligned
> +loads and stores plus shifts and masks. Emulated unaligned accesses
> +will not be caught by hardware. These accesses may be costly and may
> +be  actually unnecessary. In order to catch these accesses and remove
> +or optimize them, option -munaligned-access is explicitly set for all
> +versions of gcc which support it.
> +
> +From gcc 4.7 onward, the default for performing unaligned accesses
> +is to use unaligned native loads and stores (-munaligned-access),
> +because the cost of unaligned accesses has dropped. This should not
> +affect U-Boot's policy of controlling unaligned accesses, however the
> +compiler may generate uncontrolled unaligned on its own in at least
> +one known case: when declaring a local initialized char array, e.g.
> +
> +function foo()
> +{
> +	char buffer[] = "initial value";
> +/* or */
> +	char buffer[] = { 'i', 'n', 'i', 't', 0 };
> +	...
> +}
> +
> +Under -munaligned-accesses with optimizations on, this declaration
> +causes the compiler to generate native loads from the literal string
> +and native stores to the buffer, and the literal string alignment
> +cannot be controlled. If it is misaligned, then the core will throw
> +a data abort exception.
> +
> +Quite probably the same might happen for 16-bit array initializations
> +where the constant is aligned on a boundary which is a multiple of 2
> +but not of 4:
> +
> +function foo()
> +{
> +	u16 buffer[] = { 1, 2, 3 };
> +	...
> +}
> +
> +The long term solution to this issue is to add an option to gcc to
> +allow controlling the general alignment of data, including constant
> +initialization values.
> +
> +However this will only apply to the version of gcc which will have such
> +an option. For other versions, there are four workarounds:
> +
> +a) Enforce as a rule that array initializations as described above
> +   are forbidden. This is generally not acceptable as they are valid,
> +   and usual, C constructs. The only case where they could be rejected
> +   is when they actually equate to a const char* declaration, i.e. the
> +   array is initialized and never modified in the function's scope.
> +
> +b) Drop the requirement on unaligned accesses at least for ARMv7,
> +   i.e. do not throw a data abort exception upon unaligned accesses.
> +   But that will allow adding badly aligned code to U-Boot, only for
> +   it to fail when re-used with another, more strict, target, possibly
> +   once the bad code is already in mainline.
> +
> +c) Relax the -munified-access rule globally. This will prevent native
> +   unaligned accesses of course, but that will also hide any bug caused
> +   by a bad unaligned access, making it much harder to diagnose it. It
> +   is actually what already happens when building ARM targets with a
> +   pre-4.7 gcc, and it may actually already hide some bugs yet unseen
> +   until the target gets compiled with m-unaligned-access.
> +
> +d) Relax the -munified-access rule only for for files susceptible to
> +   the local initialized array issue. This minimizes the quantity of
> +   code which can hide unwanted misaligned accesses.
> +
> +Considering the rarity of actual occurrences (as of this writing, 5
> +files out of 7840 in U-Boot, or .3%, contain an initialized local char
> +array which cannot actually be replaced with a const char*), detection
> +if the issue in patches should not be asked from contributors.
> +
> +Luckily, detecting files susceptible to the issue can be automated
> +through a filter/regexp/script which would recognize local char array
> +initializations. Automated should err on the false positive side, for
> +instance flagging non-local arrays as if they were local if they cannot
> +be told apart.
> +
> +In any case, detection shall be informative only and shall not prevent
> +applying the patch.
> +
> +Upon a positive detection, either option -mno-unaligned-access is
> +applied (if not already) to the affected file(s), or if the array is a
> +hidden const char*, it should be replaced by one.
> diff --git a/fs/fat/Makefile b/fs/fat/Makefile
> index 9635d36..5c4a2aa 100644
> --- a/fs/fat/Makefile
> +++ b/fs/fat/Makefile
> @@ -39,6 +39,8 @@ all:	$(LIB) $(AOBJS)
>   $(LIB):	$(obj).depend $(OBJS)
>   	$(call cmd_link_o_target, $(OBJS))
>
> +# SEE README.arm-unaligned-accesses
> +$(obj)file.o: CFLAGS += -mno-unaligned-access
>
>   #########################################################################
>
> diff --git a/fs/ubifs/Makefile b/fs/ubifs/Makefile
> index ccffe85..71c40f2 100644
> --- a/fs/ubifs/Makefile
> +++ b/fs/ubifs/Makefile
> @@ -42,6 +42,9 @@ all:	$(LIB) $(AOBJS)
>   $(LIB):	$(obj).depend $(OBJS)
>   	$(call cmd_link_o_target, $(OBJS))
>
> +# SEE README.arm-unaligned-accesses
> +$(obj)super.o: CFLAGS += -mno-unaligned-access
> +
>   #########################################################################
>
>   # defines $(obj).depend target
> diff --git a/lib/Makefile b/lib/Makefile
> index 45798de..44393ed 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -78,6 +78,9 @@ OBJS	:= $(addprefix $(obj),$(COBJS))
>   $(LIB):	$(obj).depend $(OBJS)
>   	$(call cmd_link_o_target, $(OBJS))
>
> +# SEE README.arm-unaligned-accesses
> +$(obj)bzlib.o: CFLAGS += -mno-unaligned-access
> +
>   #########################################################################
>
>   # defines $(obj).depend target
>
Joakim Tjernlund Oct. 3, 2012, 8:22 a.m. UTC | #7 
Albert ARIBAUD <albert.u.boot@aribaud.net> wrote on 2012/10/02 22:05:40:
>
> Hi Joakim,
>
> On Tue, 2 Oct 2012 21:13:39 +0200, Joakim Tjernlund
> <joakim.tjernlund@transmode.se> wrote:
>
> >
> >
> > >   *********************************************************************/
> > >  void display_board_info(u32 btype)
> > >  {
> > > -   char cpu_2420[] = "2420";   /* cpu type */
> > > -   char cpu_2422[] = "2422";
> > > -   char cpu_2423[] = "2423";
> > > -   char db_men[] = "Menelaus"; /* board type */
> > > -   char db_ip[] = "IP";
> > > -   char mem_sdr[] = "mSDR";    /* memory type */
> > > -   char mem_ddr[] = "mDDR";
> > > -   char t_tst[] = "TST";       /* security level */
> > > -   char t_emu[] = "EMU";
> > > -   char t_hs[] = "HS";
> > > -   char t_gp[] = "GP";
> > > -   char unk[] = "?";
> > > +   char *cpu_2420 = "2420";   /* cpu type */
> > > +   char *cpu_2422 = "2422";
> > > +   char *cpu_2423 = "2423";
> > > +   char *db_men = "Menelaus"; /* board type */
> > > +   char *db_ip = "IP";
> > > +   char *mem_sdr = "mSDR";    /* memory type */
> > > +   char *mem_ddr = "mDDR";
> > > +   char *t_tst = "TST";       /* security level */
> > > +   char *t_emu = "EMU";
> > > +   char *t_hs = "HS";
> > > +   char *t_gp = "GP";
> > > +   char *unk = "?";
> >
> > hmm, on ppc I think this will cause relocation entries which will build size.
> >
> >  Jocke
>
> Can you try it and post results with and without it? Sizes and if
> possible disassembly of the function for both cases.

Since you asked, I had to check :)
Did this:
#include <stdio.h>
#ifdef NO_RELOC
void display_board_info1(int btype)
{
	char cpu_2420[] = "2420";   /* cpu type */
	char cpu_2422[] = "2422";
	char cpu_2423[] = "2423";
	char db_men[] = "Menelaus"; /* board type */
	char db_ip[] = "IP";
	char mem_sdr[] = "mSDR";    /* memory type */
	char mem_ddr[] = "mDDR";
	char t_tst[] = "TST";		     /* security level */
	char t_emu[] = "EMU";
	char t_hs[] = "HS";
	char t_gp[] = "GP";
	char unk[] = "?";
	printf("%s,%s,%s,%s,%s,%s,%s,%s,%s,%s,%s,%s\n",
	       cpu_2420, cpu_2422, cpu_2423, db_men, db_ip, mem_sdr,
	       mem_ddr, t_tst, t_emu, t_hs, t_gp, unk);
}
#else
void display_board_info2(int btype)
{
	char *cpu_2420 = "2420";   /* cpu type */
	char *cpu_2422 = "2422";
	char *cpu_2423 = "2423";
	char *db_men = "Menelaus"; /* board type */
	char *db_ip = "IP";
	char *mem_sdr = "mSDR";    /* memory type */
	char *mem_ddr = "mDDR";
	char *t_tst = "TST";		     /* security level */
	char *t_emu = "EMU";
	char *t_hs = "HS";
	char *t_gp = "GP";
	char *unk = "?";
	printf("%s,%s,%s,%s,%s,%s,%s,%s,%s,%s,%s,%s\n",
	       cpu_2420, cpu_2422, cpu_2423, db_men, db_ip, mem_sdr,
	       mem_ddr, t_tst, t_emu, t_hs, t_gp, unk);
}
#endif
and built it with:
powerpc-softfloat_4.5.3-linux-gnu-gcc -O2 -fpic -mrelocatable ppc-str-reloc.c -c -DNO_RELOC -o no_reloc.o

and

powerpc-softfloat_4.5.3-linux-gnu-gcc -O2 -fpic -mrelocatable ppc-str-reloc.c -c -o reloc.o

Result is:
#> size reloc.o no_reloc.o    text	   data	    bss	    dec	    hex	filename
    248	     52	      0	    300	    12c	reloc.o
    538	      0	      0	    538	    21a	no_reloc.o

Turns out that gcc still uses const string ptrs which makes for really bad code.
So your char * conversion is better for ppc too given the way gcc operates

 Jocke
Thierry Reding Oct. 4, 2012, 12:02 p.m. UTC | #8 
On Tue, Oct 02, 2012 at 08:46:04PM +0200, Albert ARIBAUD wrote:
> Under option -munaligned-access, gcc can perform local char
> or 16-bit array initializations using misaligned native
> accesses which will throw a data abort exception. Fix files
> where these array initializations were unneeded, and for
> files known to contain such initializations, enforce gcc
> option -mno-unaligned-access.
> 
> Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> ---
> Please test this patch with gcc 4.7 on boards which do data aborts
> or resets due to misaligned accesses and report result to me.

I've tested this on TEC, on top of Tom's tegra/next branch. Without the
patch, U-Boot hangs right after:

	Loading Device Tree to 010f9000, end 010ff2d4 ... OK

With the patch applied the system boots to the login prompt, so:

Tested-by: Thierry Reding <thierry.reding@avionic-design.de>
Lucas Stach Oct. 4, 2012, 7:10 p.m. UTC | #9 
Hi Albert,

Am Dienstag, den 02.10.2012, 20:46 +0200 schrieb Albert ARIBAUD:
> Under option -munaligned-access, gcc can perform local char
> or 16-bit array initializations using misaligned native
> accesses which will throw a data abort exception. Fix files
> where these array initializations were unneeded, and for
> files known to contain such initializations, enforce gcc
> option -mno-unaligned-access.
> 
> Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> ---
> Please test this patch with gcc 4.7 on boards which do data aborts
> or resets due to misaligned accesses and report result to me.
> 
Although, as you know, I don't like the general direction in which this
is heading you get a

Tested-by: Lucas Stach <dev@lynxeye.de>

As it at least allows for a booting machine in various configurations on
my Colibri T20.

>  arch/arm/cpu/arm926ejs/orion5x/cpu.c |    4 +-
>  board/ti/omap2420h4/sys_info.c       |   24 ++++-----
>  common/Makefile                      |    3 ++
>  common/cmd_dfu.c                     |    2 +-
>  doc/README.arm-unaligned-accesses    |   95 ++++++++++++++++++++++++++++++++++
>  fs/fat/Makefile                      |    2 +
>  fs/ubifs/Makefile                    |    3 ++
>  lib/Makefile                         |    3 ++
>  8 files changed, 121 insertions(+), 15 deletions(-)
>  create mode 100644 doc/README.arm-unaligned-accesses
> 
> diff --git a/arch/arm/cpu/arm926ejs/orion5x/cpu.c b/arch/arm/cpu/arm926ejs/orion5x/cpu.c
> index c3948d3..5a4775a 100644
> --- a/arch/arm/cpu/arm926ejs/orion5x/cpu.c
> +++ b/arch/arm/cpu/arm926ejs/orion5x/cpu.c
> @@ -194,8 +194,8 @@ u32 orion5x_device_rev(void)
>   */
>  int print_cpuinfo(void)
>  {
> -	char dev_str[] = "0x0000";
> -	char rev_str[] = "0x00";
> +	char dev_str[7]; /* room enough for 0x0000 plus null byte */
> +	char rev_str[5]; /* room enough for 0x00 plus null byte */
>  	char *dev_name = NULL;
>  	char *rev_name = NULL;
>  
> diff --git a/board/ti/omap2420h4/sys_info.c b/board/ti/omap2420h4/sys_info.c
> index a9f7241..b462aa5 100644
> --- a/board/ti/omap2420h4/sys_info.c
> +++ b/board/ti/omap2420h4/sys_info.c
> @@ -237,18 +237,18 @@ u32 wait_on_value(u32 read_bit_mask, u32 match_value, u32 read_addr, u32 bound)
>   *********************************************************************/
>  void display_board_info(u32 btype)
>  {
> -	char cpu_2420[] = "2420";   /* cpu type */
> -	char cpu_2422[] = "2422";
> -	char cpu_2423[] = "2423";
> -	char db_men[] = "Menelaus"; /* board type */
> -	char db_ip[] = "IP";
> -	char mem_sdr[] = "mSDR";    /* memory type */
> -	char mem_ddr[] = "mDDR";
> -	char t_tst[] = "TST";	    /* security level */
> -	char t_emu[] = "EMU";
> -	char t_hs[] = "HS";
> -	char t_gp[] = "GP";
> -	char unk[] = "?";
> +	char *cpu_2420 = "2420";   /* cpu type */
> +	char *cpu_2422 = "2422";
> +	char *cpu_2423 = "2423";
> +	char *db_men = "Menelaus"; /* board type */
> +	char *db_ip = "IP";
> +	char *mem_sdr = "mSDR";    /* memory type */
> +	char *mem_ddr = "mDDR";
> +	char *t_tst = "TST";	    /* security level */
> +	char *t_emu = "EMU";
> +	char *t_hs = "HS";
> +	char *t_gp = "GP";
> +	char *unk = "?";
>  
>  	char *cpu_s, *db_s, *mem_s, *sec_s;
>  	u32 cpu, rev, sec;
> diff --git a/common/Makefile b/common/Makefile
> index 125b2be..19b2130 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -227,6 +227,9 @@ $(obj)env_embedded.o: $(src)env_embedded.c $(obj)../tools/envcrc
>  $(obj)../tools/envcrc:
>  	$(MAKE) -C ../tools
>  
> +$(obj)hush.o: CFLAGS += -mno-unaligned-access
> +$(obj)fdt_support.o: CFLAGS += -mno-unaligned-access
> +
>  #########################################################################
>  
>  # defines $(obj).depend target
> diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c
> index 62fb890..01d6b3a 100644
> --- a/common/cmd_dfu.c
> +++ b/common/cmd_dfu.c
> @@ -30,7 +30,7 @@
>  static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  {
>  	const char *str_env;
> -	char s[] = "dfu";
> +	char *s = "dfu";
>  	char *env_bkp;
>  	int ret;
>  
> diff --git a/doc/README.arm-unaligned-accesses b/doc/README.arm-unaligned-accesses
> new file mode 100644
> index 0000000..00fb1c0
> --- /dev/null
> +++ b/doc/README.arm-unaligned-accesses
> @@ -0,0 +1,95 @@
> +Since U-Boot runs on a variety of hardware, some only able to perform
> +unaligned accesses with a strong penalty, some unable to perform them
> +at all, the policy regarding unaligned accesses is to not perform any,
> +unless absolutely necessary because of hardware or standards.
> +
> +Also, on hardware which permits it, the core is configured to throw
> +data abort exceptions on unaligned accesses  in order to catch these
> +unallowed accesses as early as possible.
> +
> +Until version 4.7, the gcc default for performing unaligned accesses
> +(-mno-unaligned-access) is to emulate unaligned accesses using aligned
> +loads and stores plus shifts and masks. Emulated unaligned accesses
> +will not be caught by hardware. These accesses may be costly and may
> +be  actually unnecessary. In order to catch these accesses and remove
> +or optimize them, option -munaligned-access is explicitly set for all
> +versions of gcc which support it.
> +
> +From gcc 4.7 onward, the default for performing unaligned accesses
> +is to use unaligned native loads and stores (-munaligned-access),
> +because the cost of unaligned accesses has dropped. This should not
> +affect U-Boot's policy of controlling unaligned accesses, however the
> +compiler may generate uncontrolled unaligned on its own in at least
> +one known case: when declaring a local initialized char array, e.g.
> +
> +function foo()
> +{
> +	char buffer[] = "initial value";
> +/* or */
> +	char buffer[] = { 'i', 'n', 'i', 't', 0 };
> +	...
> +}
> +
> +Under -munaligned-accesses with optimizations on, this declaration
> +causes the compiler to generate native loads from the literal string
> +and native stores to the buffer, and the literal string alignment
> +cannot be controlled. If it is misaligned, then the core will throw
> +a data abort exception.
> +
> +Quite probably the same might happen for 16-bit array initializations
> +where the constant is aligned on a boundary which is a multiple of 2
> +but not of 4:
> +
> +function foo()
> +{
> +	u16 buffer[] = { 1, 2, 3 };
> +	...
> +}
> +
> +The long term solution to this issue is to add an option to gcc to
> +allow controlling the general alignment of data, including constant
> +initialization values.
> +
> +However this will only apply to the version of gcc which will have such
> +an option. For other versions, there are four workarounds:
> +
> +a) Enforce as a rule that array initializations as described above
> +   are forbidden. This is generally not acceptable as they are valid,
> +   and usual, C constructs. The only case where they could be rejected
> +   is when they actually equate to a const char* declaration, i.e. the
> +   array is initialized and never modified in the function's scope.
> +
> +b) Drop the requirement on unaligned accesses at least for ARMv7,
> +   i.e. do not throw a data abort exception upon unaligned accesses.
> +   But that will allow adding badly aligned code to U-Boot, only for
> +   it to fail when re-used with another, more strict, target, possibly
> +   once the bad code is already in mainline.
> +
> +c) Relax the -munified-access rule globally. This will prevent native
> +   unaligned accesses of course, but that will also hide any bug caused
> +   by a bad unaligned access, making it much harder to diagnose it. It
> +   is actually what already happens when building ARM targets with a
> +   pre-4.7 gcc, and it may actually already hide some bugs yet unseen
> +   until the target gets compiled with m-unaligned-access.
> +
> +d) Relax the -munified-access rule only for for files susceptible to
> +   the local initialized array issue. This minimizes the quantity of
> +   code which can hide unwanted misaligned accesses.
> +
> +Considering the rarity of actual occurrences (as of this writing, 5
> +files out of 7840 in U-Boot, or .3%, contain an initialized local char
> +array which cannot actually be replaced with a const char*), detection
> +if the issue in patches should not be asked from contributors.
> +
> +Luckily, detecting files susceptible to the issue can be automated
> +through a filter/regexp/script which would recognize local char array
> +initializations. Automated should err on the false positive side, for
> +instance flagging non-local arrays as if they were local if they cannot
> +be told apart.
> +
> +In any case, detection shall be informative only and shall not prevent
> +applying the patch.
> +
> +Upon a positive detection, either option -mno-unaligned-access is
> +applied (if not already) to the affected file(s), or if the array is a
> +hidden const char*, it should be replaced by one.
> diff --git a/fs/fat/Makefile b/fs/fat/Makefile
> index 9635d36..5c4a2aa 100644
> --- a/fs/fat/Makefile
> +++ b/fs/fat/Makefile
> @@ -39,6 +39,8 @@ all:	$(LIB) $(AOBJS)
>  $(LIB):	$(obj).depend $(OBJS)
>  	$(call cmd_link_o_target, $(OBJS))
>  
> +# SEE README.arm-unaligned-accesses
> +$(obj)file.o: CFLAGS += -mno-unaligned-access
>  
>  #########################################################################
>  
> diff --git a/fs/ubifs/Makefile b/fs/ubifs/Makefile
> index ccffe85..71c40f2 100644
> --- a/fs/ubifs/Makefile
> +++ b/fs/ubifs/Makefile
> @@ -42,6 +42,9 @@ all:	$(LIB) $(AOBJS)
>  $(LIB):	$(obj).depend $(OBJS)
>  	$(call cmd_link_o_target, $(OBJS))
>  
> +# SEE README.arm-unaligned-accesses
> +$(obj)super.o: CFLAGS += -mno-unaligned-access
> +
>  #########################################################################
>  
>  # defines $(obj).depend target
> diff --git a/lib/Makefile b/lib/Makefile
> index 45798de..44393ed 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -78,6 +78,9 @@ OBJS	:= $(addprefix $(obj),$(COBJS))
>  $(LIB):	$(obj).depend $(OBJS)
>  	$(call cmd_link_o_target, $(OBJS))
>  
> +# SEE README.arm-unaligned-accesses
> +$(obj)bzlib.o: CFLAGS += -mno-unaligned-access
> +
>  #########################################################################
>  
>  # defines $(obj).depend target
Albert ARIBAUD Oct. 4, 2012, 8:22 p.m. UTC | #10 
Hi Mark,

On Wed, 3 Oct 2012 09:29:09 +0200, Mark Marshall
<mark.marshall@omicron.at> wrote:

> I've just had a quick look at this in passing, but at least some of
> these changes seem wrong to me.  For example, the code in
> board/ti/omap2420h4/sys_info.c :: display_board_info
> should be:
> 
> void display_board_info(u32 btype)
> {
> 	static const char cpu_2420[] = "2420";   /* cpu type */
> 	static const char cpu_2422[] = "2422";
> 	static const char cpu_2423[] = "2423";
> 	static const char db_men[] = "Menelaus"; /* board type */
> 	static const char db_ip[] = "IP";
> 	static const char mem_sdr[] = "mSDR";    /* memory type */
> 	static const char mem_ddr[] = "mDDR";
> 	static const char t_tst[] = "TST";	    /* security level */
> 	static const char t_emu[] = "EMU";
> 	static const char t_hs[] = "HS";
> 	static const char t_gp[] = "GP";
> 	static const char unk[] = "?";
> 
> 	const char *cpu_s, *db_s, *mem_s, *sec_s;
> 	u32 cpu, rev, sec;
> 
> This produces smaller code and is probably what the original
> author wanted the compiler to do.  I've only compile tested
> this, not actually run it.
> 
> Original file:
> 
> Sections:
> Idx Name          Size      VMA       LMA       File off  Algn
>    0 .text         000004b4  00000000  00000000  00000034  2**2
>                    CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
>    1 .data         00000000  00000000  00000000  000004e8  2**0
>                    CONTENTS, ALLOC, LOAD, DATA
>    2 .bss          00000000  00000000  00000000  000004e8  2**0
>                    ALLOC
>    3 .rodata.str1.1 00000072  00000000  00000000  000004e8  2**0
>                    CONTENTS, ALLOC, LOAD, READONLY, DATA
> 
> After my changes:
> 
> Sections:
> Idx Name          Size      VMA       LMA       File off  Algn
>    0 .text         000003ac  00000000  00000000  00000034  2**2
>                    CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
>    1 .data         00000000  00000000  00000000  000003e0  2**0
>                    CONTENTS, ALLOC, LOAD, DATA
>    2 .bss          00000000  00000000  00000000  000003e0  2**0
>                    ALLOC
>    3 .rodata       00000048  00000000  00000000  000003e0  2**2
>                    CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA
>    4 .rodata.str1.1 00000047  00000000  00000000  00000428  2**0
>                    CONTENTS, ALLOC, LOAD, READONLY, DATA
> 

Thanks Mike. Indeed, there is a range of choices to replace the
original local char arrays: global arrays, static local arrays, 
char pointers, const char pointers, or even direct strings in code. I'd
originally gone for const chars because the idea was not optimizing but
getting rid of a compiler issue, however, with my toolchain, this led
to warnings about losing the const qualifier, so I went to simple char*.
I'll recheck with (const) static char[] and chose whetever gets the
best score.

> Regards,
> 
> Mark Marshall.

Amicalement,
diff mbox

Patch

diff --git a/arch/arm/cpu/arm926ejs/orion5x/cpu.c b/arch/arm/cpu/arm926ejs/orion5x/cpu.c
index c3948d3..5a4775a 100644
--- a/arch/arm/cpu/arm926ejs/orion5x/cpu.c
+++ b/arch/arm/cpu/arm926ejs/orion5x/cpu.c
@@ -194,8 +194,8 @@  u32 orion5x_device_rev(void)
  */
 int print_cpuinfo(void)
 {
-	char dev_str[] = "0x0000";
-	char rev_str[] = "0x00";
+	char dev_str[7]; /* room enough for 0x0000 plus null byte */
+	char rev_str[5]; /* room enough for 0x00 plus null byte */
 	char *dev_name = NULL;
 	char *rev_name = NULL;
 
diff --git a/board/ti/omap2420h4/sys_info.c b/board/ti/omap2420h4/sys_info.c
index a9f7241..b462aa5 100644
--- a/board/ti/omap2420h4/sys_info.c
+++ b/board/ti/omap2420h4/sys_info.c
@@ -237,18 +237,18 @@  u32 wait_on_value(u32 read_bit_mask, u32 match_value, u32 read_addr, u32 bound)
  *********************************************************************/
 void display_board_info(u32 btype)
 {
-	char cpu_2420[] = "2420";   /* cpu type */
-	char cpu_2422[] = "2422";
-	char cpu_2423[] = "2423";
-	char db_men[] = "Menelaus"; /* board type */
-	char db_ip[] = "IP";
-	char mem_sdr[] = "mSDR";    /* memory type */
-	char mem_ddr[] = "mDDR";
-	char t_tst[] = "TST";	    /* security level */
-	char t_emu[] = "EMU";
-	char t_hs[] = "HS";
-	char t_gp[] = "GP";
-	char unk[] = "?";
+	char *cpu_2420 = "2420";   /* cpu type */
+	char *cpu_2422 = "2422";
+	char *cpu_2423 = "2423";
+	char *db_men = "Menelaus"; /* board type */
+	char *db_ip = "IP";
+	char *mem_sdr = "mSDR";    /* memory type */
+	char *mem_ddr = "mDDR";
+	char *t_tst = "TST";	    /* security level */
+	char *t_emu = "EMU";
+	char *t_hs = "HS";
+	char *t_gp = "GP";
+	char *unk = "?";
 
 	char *cpu_s, *db_s, *mem_s, *sec_s;
 	u32 cpu, rev, sec;
diff --git a/common/Makefile b/common/Makefile
index 125b2be..19b2130 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -227,6 +227,9 @@  $(obj)env_embedded.o: $(src)env_embedded.c $(obj)../tools/envcrc
 $(obj)../tools/envcrc:
 	$(MAKE) -C ../tools
 
+$(obj)hush.o: CFLAGS += -mno-unaligned-access
+$(obj)fdt_support.o: CFLAGS += -mno-unaligned-access
+
 #########################################################################
 
 # defines $(obj).depend target
diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c
index 62fb890..01d6b3a 100644
--- a/common/cmd_dfu.c
+++ b/common/cmd_dfu.c
@@ -30,7 +30,7 @@ 
 static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	const char *str_env;
-	char s[] = "dfu";
+	char *s = "dfu";
 	char *env_bkp;
 	int ret;
 
diff --git a/doc/README.arm-unaligned-accesses b/doc/README.arm-unaligned-accesses
new file mode 100644
index 0000000..00fb1c0
--- /dev/null
+++ b/doc/README.arm-unaligned-accesses
@@ -0,0 +1,95 @@ 
+Since U-Boot runs on a variety of hardware, some only able to perform
+unaligned accesses with a strong penalty, some unable to perform them
+at all, the policy regarding unaligned accesses is to not perform any,
+unless absolutely necessary because of hardware or standards.
+
+Also, on hardware which permits it, the core is configured to throw
+data abort exceptions on unaligned accesses  in order to catch these
+unallowed accesses as early as possible.
+
+Until version 4.7, the gcc default for performing unaligned accesses
+(-mno-unaligned-access) is to emulate unaligned accesses using aligned
+loads and stores plus shifts and masks. Emulated unaligned accesses
+will not be caught by hardware. These accesses may be costly and may
+be  actually unnecessary. In order to catch these accesses and remove
+or optimize them, option -munaligned-access is explicitly set for all
+versions of gcc which support it.
+
+From gcc 4.7 onward, the default for performing unaligned accesses
+is to use unaligned native loads and stores (-munaligned-access),
+because the cost of unaligned accesses has dropped. This should not
+affect U-Boot's policy of controlling unaligned accesses, however the
+compiler may generate uncontrolled unaligned on its own in at least
+one known case: when declaring a local initialized char array, e.g.
+
+function foo()
+{
+	char buffer[] = "initial value";
+/* or */
+	char buffer[] = { 'i', 'n', 'i', 't', 0 };
+	...
+}
+
+Under -munaligned-accesses with optimizations on, this declaration
+causes the compiler to generate native loads from the literal string
+and native stores to the buffer, and the literal string alignment
+cannot be controlled. If it is misaligned, then the core will throw
+a data abort exception.
+
+Quite probably the same might happen for 16-bit array initializations
+where the constant is aligned on a boundary which is a multiple of 2
+but not of 4:
+
+function foo()
+{
+	u16 buffer[] = { 1, 2, 3 };
+	...
+}
+
+The long term solution to this issue is to add an option to gcc to
+allow controlling the general alignment of data, including constant
+initialization values.
+
+However this will only apply to the version of gcc which will have such
+an option. For other versions, there are four workarounds:
+
+a) Enforce as a rule that array initializations as described above
+   are forbidden. This is generally not acceptable as they are valid,
+   and usual, C constructs. The only case where they could be rejected
+   is when they actually equate to a const char* declaration, i.e. the
+   array is initialized and never modified in the function's scope.
+
+b) Drop the requirement on unaligned accesses at least for ARMv7,
+   i.e. do not throw a data abort exception upon unaligned accesses.
+   But that will allow adding badly aligned code to U-Boot, only for
+   it to fail when re-used with another, more strict, target, possibly
+   once the bad code is already in mainline.
+
+c) Relax the -munified-access rule globally. This will prevent native
+   unaligned accesses of course, but that will also hide any bug caused
+   by a bad unaligned access, making it much harder to diagnose it. It
+   is actually what already happens when building ARM targets with a
+   pre-4.7 gcc, and it may actually already hide some bugs yet unseen
+   until the target gets compiled with m-unaligned-access.
+
+d) Relax the -munified-access rule only for for files susceptible to
+   the local initialized array issue. This minimizes the quantity of
+   code which can hide unwanted misaligned accesses.
+
+Considering the rarity of actual occurrences (as of this writing, 5
+files out of 7840 in U-Boot, or .3%, contain an initialized local char
+array which cannot actually be replaced with a const char*), detection
+if the issue in patches should not be asked from contributors.
+
+Luckily, detecting files susceptible to the issue can be automated
+through a filter/regexp/script which would recognize local char array
+initializations. Automated should err on the false positive side, for
+instance flagging non-local arrays as if they were local if they cannot
+be told apart.
+
+In any case, detection shall be informative only and shall not prevent
+applying the patch.
+
+Upon a positive detection, either option -mno-unaligned-access is
+applied (if not already) to the affected file(s), or if the array is a
+hidden const char*, it should be replaced by one.
diff --git a/fs/fat/Makefile b/fs/fat/Makefile
index 9635d36..5c4a2aa 100644
--- a/fs/fat/Makefile
+++ b/fs/fat/Makefile
@@ -39,6 +39,8 @@  all:	$(LIB) $(AOBJS)
 $(LIB):	$(obj).depend $(OBJS)
 	$(call cmd_link_o_target, $(OBJS))
 
+# SEE README.arm-unaligned-accesses
+$(obj)file.o: CFLAGS += -mno-unaligned-access
 
 #########################################################################
 
diff --git a/fs/ubifs/Makefile b/fs/ubifs/Makefile
index ccffe85..71c40f2 100644
--- a/fs/ubifs/Makefile
+++ b/fs/ubifs/Makefile
@@ -42,6 +42,9 @@  all:	$(LIB) $(AOBJS)
 $(LIB):	$(obj).depend $(OBJS)
 	$(call cmd_link_o_target, $(OBJS))
 
+# SEE README.arm-unaligned-accesses
+$(obj)super.o: CFLAGS += -mno-unaligned-access
+
 #########################################################################
 
 # defines $(obj).depend target
diff --git a/lib/Makefile b/lib/Makefile
index 45798de..44393ed 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -78,6 +78,9 @@  OBJS	:= $(addprefix $(obj),$(COBJS))
 $(LIB):	$(obj).depend $(OBJS)
 	$(call cmd_link_o_target, $(OBJS))
 
+# SEE README.arm-unaligned-accesses
+$(obj)bzlib.o: CFLAGS += -mno-unaligned-access
+
 #########################################################################
 
 # defines $(obj).depend target