From patchwork Tue Oct 9 19:28:15 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Albert ARIBAUD X-Patchwork-Id: 190418 X-Patchwork-Delegate: trini@ti.com Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from theia.denx.de (theia.denx.de [85.214.87.163]) by ozlabs.org (Postfix) with ESMTP id 98DC42C0095 for ; Wed, 10 Oct 2012 06:29:13 +1100 (EST) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 127FE28071; Tue, 9 Oct 2012 21:29:04 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at theia.denx.de Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id zPzw-rDQnnef; Tue, 9 Oct 2012 21:29:03 +0200 (CEST) Received: from theia.denx.de (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id C3FC02809D; Tue, 9 Oct 2012 21:28:59 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id D8EAB28071 for ; Tue, 9 Oct 2012 21:28:29 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at theia.denx.de Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id vNsi7dU9JSlf for ; Tue, 9 Oct 2012 21:28:29 +0200 (CEST) X-policyd-weight: NOT_IN_SBL_XBL_SPAMHAUS=-1.5 NOT_IN_SPAMCOP=-1.5 NOT_IN_BL_NJABL=-1.5 (only DNSBL check requested) Received: from smtp1-g21.free.fr (smtp1-g21.free.fr [212.27.42.1]) by theia.denx.de (Postfix) with ESMTP id B50522809E for ; Tue, 9 Oct 2012 21:28:24 +0200 (CEST) Received: from localhost.localdomain (unknown [IPv6:2a01:e35:2eb9:20:bd4b:b9f2:ac5d:eda]) (Authenticated sender: albert.aribaud) by smtp1-g21.free.fr (Postfix) with ESMTPSA id 22A8D9400AD; Tue, 9 Oct 2012 21:28:18 +0200 (CEST) From: Albert ARIBAUD To: u-boot@lists.denx.de Date: Tue, 9 Oct 2012 21:28:15 +0200 Message-Id: <1349810895-25809-1-git-send-email-albert.u.boot@aribaud.net> X-Mailer: git-send-email 1.7.9.5 In-Reply-To: <1349809714-16267-1-git-send-email-albert.u.boot@aribaud.net> References: <1349809714-16267-1-git-send-email-albert.u.boot@aribaud.net> <1349725818-8500-1-git-send-email-albert.u.boot@aribaud.net> <1349723944-8188-1-git-send-email-albert.u.boot@aribaud.net> <1349464558-7311-1-git-send-email-albert.u.boot@aribaud.net> Subject: [U-Boot] [PATCH V6] [RESEND] ARM: prevent misaligned array inits X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.11 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: u-boot-bounces@lists.denx.de Errors-To: u-boot-bounces@lists.denx.de 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 --- V6: Make sure that gcc does not silently drop -mno-unaligned-access if it does not support it. V5: -mno-unaligned-access was applied to all platforms. Make it apply only to armv7. Clarified README.arm-unaligned-accesses re how to fix issue. Included revert of 'release-only' workaround. V4: added information on how to find relocation offset for pc common/Makefile missed a comment re README.arm-unaligned-accesses V3: *really* fix incorrect doc file name in dabort handler message clarifications and typo fixes in README.arm-unaligned-accesses V2: fix incorrect doc file name in dabort handler message arch/arm/cpu/arm926ejs/orion5x/cpu.c | 4 +- arch/arm/cpu/armv7/config.mk | 6 +- arch/arm/lib/interrupts.c | 2 +- board/ti/omap2420h4/sys_info.c | 28 ++++---- common/Makefile | 4 ++ common/cmd_dfu.c | 2 +- doc/README.arm-unaligned-accesses | 122 ++++++++++++++++++++++++++++++++++ fs/fat/Makefile | 2 + fs/ubifs/Makefile | 3 + lib/Makefile | 3 + 10 files changed, 156 insertions(+), 20 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/arch/arm/cpu/armv7/config.mk b/arch/arm/cpu/armv7/config.mk index 560c084..3c5ca23 100644 --- a/arch/arm/cpu/armv7/config.mk +++ b/arch/arm/cpu/armv7/config.mk @@ -26,8 +26,6 @@ PLATFORM_RELFLAGS += -fno-common -ffixed-r8 -msoft-float # supported by more tool-chains PF_CPPFLAGS_ARMV7 := $(call cc-option, -march=armv7-a, -march=armv5) PLATFORM_CPPFLAGS += $(PF_CPPFLAGS_ARMV7) -PF_CPPFLAGS_NO_UNALIGNED := $(call cc-option, -mno-unaligned-access,) -PLATFORM_CPPFLAGS += $(PF_CPPFLAGS_NO_UNALIGNED) # ========================================================================= # @@ -36,6 +34,10 @@ PLATFORM_CPPFLAGS += $(PF_CPPFLAGS_NO_UNALIGNED) # ========================================================================= PF_RELFLAGS_SLB_AT := $(call cc-option,-mshort-load-bytes,$(call cc-option,-malignment-traps,)) PLATFORM_RELFLAGS += $(PF_RELFLAGS_SLB_AT) + +# SEE README.arm-unaligned-accesses +PLATFORM_NO_UNALIGNED := -mno-unaligned-access + ifneq ($(CONFIG_IMX_CONFIG),) ALL-y += $(obj)u-boot.imx endif diff --git a/arch/arm/lib/interrupts.c b/arch/arm/lib/interrupts.c index 74ff5ce..02124a7 100644 --- a/arch/arm/lib/interrupts.c +++ b/arch/arm/lib/interrupts.c @@ -169,7 +169,7 @@ void do_prefetch_abort (struct pt_regs *pt_regs) void do_data_abort (struct pt_regs *pt_regs) { - printf ("data abort\n"); + printf ("data abort\n\n MAYBE you should read doc/README.arm-unaligned-accesses\n\n"); show_regs (pt_regs); bad_mode (); } diff --git a/board/ti/omap2420h4/sys_info.c b/board/ti/omap2420h4/sys_info.c index a9f7241..b12011e 100644 --- a/board/ti/omap2420h4/sys_info.c +++ b/board/ti/omap2420h4/sys_info.c @@ -237,20 +237,20 @@ 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_s, *db_s, *mem_s, *sec_s; + 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; rev = get_cpu_rev(); diff --git a/common/Makefile b/common/Makefile index 5442fbb..8a85dec 100644 --- a/common/Makefile +++ b/common/Makefile @@ -231,6 +231,10 @@ $(obj)env_embedded.o: $(src)env_embedded.c $(obj)../tools/envcrc $(obj)../tools/envcrc: $(MAKE) -C ../tools +# SEE README.arm-unaligned-accesses +$(obj)hush.o: CFLAGS += $(PLATFORM_NO_UNALIGNED) +$(obj)fdt_support.o: CFLAGS += $(PLATFORM_NO_UNALIGNED) + ######################################################################### # 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..c37d135 --- /dev/null +++ b/doc/README.arm-unaligned-accesses @@ -0,0 +1,122 @@ +If you are reading this because of a data abort: the following MIGHT +be relevant to your abort, if it was caused by an alignment violation. +In order to determine this, use the PC from the abort dump along with +an objdump -s -S of the u-boot ELF binary to locate the function where +the abort happened; then compare this function with the examples below. +If they match, then you've been hit with a compiler generated unaligned +access, and you should rewrite your code or add -mno-unaligned-access +to the command line of the offending file. + +Note that the PC shown in the abort message is relocated. In order to +be able to match it to an address in the ELF binary dump, you will need +to know the relocation offset. If your target defines CONFIG_CMD_BDI +and if you can get to the prompt and enter commands before the abort +happens, then command "bdinfo" will give you the offset. Otherwise you +will need to try a build with DEBUG set, which will display the offset, +or use a debugger and set a breakpoint at relocate_code() to see the +offset (passed as an argument). + +* + +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 starting at armv7 architectures, the default for +performing unaligned accesses is to use unaligned native loads and +stores (-munaligned-access), because the cost of unaligned accesses +has dropped on armv7 and beyond. This should not affect U-Boot's +policy of controlling unaligned accesses, however the compiler may +generate uncontrolled unaligned accesses 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 a stricter target, possibly once the + bad code is already in mainline. + +c) Relax the -munaligned-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 -munaligned-access. + +d) Relax the -munaligned-access rule only for for files susceptible to + the local initialized array issue and for armv7 architectures and + beyond. This minimizes the quantity of code which can hide unwanted + misaligned accesses. + +The option retained is d). + +Considering that actual occurrences of the issue are rare (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*), +contributors should not be required to systematically try and detect +the issue in their patches. + +Detecting files susceptible to the issue can be automated through a +filter installed as a hook in .git which recognizes local char array +initializations. Automation 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 not prevent committing the patch, but +shall pre-populate the commit message with a note to the effect that +this patch contains an initialized local char or 16-bit array and thus +should be protected from the gcc 4.7 issue. + +Upon a positive detection, either $(PLATFORM_NO_UNALIGNED) should be +added to CFLAGS for the affected file(s), or if the array is a pseudo +const char*, it should be replaced by an actual one. diff --git a/fs/fat/Makefile b/fs/fat/Makefile index 9635d36..02e6881 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 += $(PLATFORM_NO_UNALIGNED) ######################################################################### diff --git a/fs/ubifs/Makefile b/fs/ubifs/Makefile index ccffe85..bfe6874 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 += $(PLATFORM_NO_UNALIGNED) + ######################################################################### # defines $(obj).depend target diff --git a/lib/Makefile b/lib/Makefile index a099885..e44e045 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -83,6 +83,9 @@ OBJS := $(addprefix $(obj),$(COBJS)) $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS)) +# SEE README.arm-unaligned-accesses +$(obj)bzlib.o: CFLAGS += $(PLATFORM_NO_UNALIGNED) + ######################################################################### # defines $(obj).depend target