From patchwork Tue Feb 4 17:05:33 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tom Rini X-Patchwork-Id: 316687 X-Patchwork-Delegate: albert.aribaud@free.fr 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 951A32C0089 for ; Wed, 5 Feb 2014 04:05:40 +1100 (EST) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 601B44B980; Tue, 4 Feb 2014 18:05:39 +0100 (CET) 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 IazeFaiKM2ue; Tue, 4 Feb 2014 18:05:39 +0100 (CET) Received: from theia.denx.de (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 4D5F84B7F3; Tue, 4 Feb 2014 18:05:37 +0100 (CET) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 573C44B7F3 for ; Tue, 4 Feb 2014 18:05:33 +0100 (CET) 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 tZXzPmUFcdvz for ; Tue, 4 Feb 2014 18:05:29 +0100 (CET) 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 mail-qc0-f178.google.com (mail-qc0-f178.google.com [209.85.216.178]) by theia.denx.de (Postfix) with ESMTPS id 0BDC74B74D for ; Tue, 4 Feb 2014 18:05:24 +0100 (CET) Received: by mail-qc0-f178.google.com with SMTP id m20so13939127qcx.37 for ; Tue, 04 Feb 2014 09:05:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:from:to:cc:subject:date:message-id; bh=RqciIz70yDsEqcaV2N4nHOsemBqCV8Ge3npW4Zyh6r4=; b=GlWYjo/ji33ScOhkVTewCEJuZ3Zu/XLgq1AHhntNtIUN+E497d4Fe8HVDtZy/MXewp SwualCiwCmkF3Qn9s5F8vRuDh0pCCvWLonFMwn1B3hll/xCSQst1b9VXGH6iE9vaCKTy +IoMfgl60XudHTQ8292BCfTk7g6jDCcDDDuQn91ZcrXWzdHz5a3I39zCeXP4kpQlwYJO PN7uoxA5H9VEdXaWOk+/4DHsgraoaskUxM7fq/AO+VWBVvn+stgQ9Pcn4McAZA5gzE9I B9Sw6YRrQJ4qeyw1VzdDvxz99m5upqLejOX6OQz85dQNlQtTcKvvncIpwe6wBJKosBA/ FmQg== X-Received: by 10.224.68.70 with SMTP id u6mr67557152qai.5.1391533523123; Tue, 04 Feb 2014 09:05:23 -0800 (PST) Received: from localhost.localdomain (cpe-174-106-216-211.ec.res.rr.com. [174.106.216.211]) by mx.google.com with ESMTPSA id j50sm32985198qgf.14.2014.02.04.09.05.21 for (version=TLSv1.1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 04 Feb 2014 09:05:21 -0800 (PST) From: Tom Rini To: u-boot@lists.denx.de Date: Tue, 4 Feb 2014 12:05:33 -0500 Message-Id: <1391533533-21664-1-git-send-email-trini@ti.com> X-Mailer: git-send-email 1.7.9.5 Cc: Mans Rullgard Subject: [U-Boot] [PATCH v2] arm: Switch to -mno-unaligned-access when supported by the compiler 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 When we tell the compiler to optimize for ARMv7 it assumes a default of unaligned accesses being supported at the hardware level and can make use of this to perform what it deems as an optimization in any case, including allowing for data to become unaligned. We explicitly disallow this hardware feature so we must tell the compiler. Cc: Albert ARIBAUD Cc: Mans Rullgard Signed-off-by: Tom Rini Acked-by: Mans Rullgard --- Changes in v2: - Drop references to README.arm-unaligned-accesses from arch/arm/lib/interrupts.c and top-level README --- README | 2 +- arch/arm/cpu/armv7/config.mk | 6 +- arch/arm/cpu/armv8/config.mk | 5 +- arch/arm/lib/interrupts.c | 1 - common/Makefile | 4 -- doc/README.arm-unaligned-accesses | 122 ------------------------------------- fs/ubifs/Makefile | 3 - lib/Makefile | 3 - 8 files changed, 6 insertions(+), 140 deletions(-) delete mode 100644 doc/README.arm-unaligned-accesses diff --git a/README b/README index fe48ccd..1de9162 100644 --- a/README +++ b/README @@ -1725,7 +1725,7 @@ CBFS (Coreboot Filesystem) support If this option is set, then U-Boot will prevent the environment variable "splashimage" from being set to a problematic address - (see README.displaying-bmps and README.arm-unaligned-accesses). + (see README.displaying-bmps). This option is useful for targets where, due to alignment restrictions, an improperly aligned BMP image will cause a data abort. If you think you will not have problems with unaligned diff --git a/arch/arm/cpu/armv7/config.mk b/arch/arm/cpu/armv7/config.mk index 38b7c40..ae4427c 100644 --- a/arch/arm/cpu/armv7/config.mk +++ b/arch/arm/cpu/armv7/config.mk @@ -10,9 +10,11 @@ PF_CPPFLAGS_ARMV7 := $(call cc-option, -march=armv7-a, -march=armv5) PLATFORM_CPPFLAGS += $(PF_CPPFLAGS_ARMV7) -# SEE README.arm-unaligned-accesses +# On supported platforms we clear the bit which allows for hardware +# unaligned access so we must tell the compiler so it can make the correct +# decision. PF_NO_UNALIGNED := $(call cc-option, -mno-unaligned-access,) -PLATFORM_NO_UNALIGNED := $(PF_NO_UNALIGNED) +PLATFORM_CPPFLAGS += $(PF_NO_UNALIGNED) ifneq ($(CONFIG_IMX_CONFIG),) ifdef CONFIG_SPL diff --git a/arch/arm/cpu/armv8/config.mk b/arch/arm/cpu/armv8/config.mk index 027a68c..f5b9559 100644 --- a/arch/arm/cpu/armv8/config.mk +++ b/arch/arm/cpu/armv8/config.mk @@ -6,10 +6,7 @@ # PLATFORM_RELFLAGS += -fno-common -ffixed-x18 -# SEE README.arm-unaligned-accesses -PF_NO_UNALIGNED := $(call cc-option, -mstrict-align) -PLATFORM_NO_UNALIGNED := $(PF_NO_UNALIGNED) - PF_CPPFLAGS_ARMV8 := $(call cc-option, -march=armv8-a) +PF_NO_UNALIGNED := $(call cc-option, -mstrict-align) PLATFORM_CPPFLAGS += $(PF_CPPFLAGS_ARMV8) PLATFORM_CPPFLAGS += $(PF_NO_UNALIGNED) diff --git a/arch/arm/lib/interrupts.c b/arch/arm/lib/interrupts.c index 603bf14..b4e331e 100644 --- a/arch/arm/lib/interrupts.c +++ b/arch/arm/lib/interrupts.c @@ -153,7 +153,6 @@ void do_prefetch_abort (struct pt_regs *pt_regs) void do_data_abort (struct pt_regs *pt_regs) { - 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/common/Makefile b/common/Makefile index a83246e..c06d307 100644 --- a/common/Makefile +++ b/common/Makefile @@ -239,7 +239,3 @@ $(obj)env_embedded.o: $(src)env_embedded.c $(CC) $(AFLAGS) -Wa,--no-warn \ -DENV_CRC=$(shell $(obj)../tools/envcrc) \ -c -o $@ $(src)env_embedded.c - -# SEE README.arm-unaligned-accesses -$(obj)hush.o: CFLAGS += $(PLATFORM_NO_UNALIGNED) -$(obj)fdt_support.o: CFLAGS += $(PLATFORM_NO_UNALIGNED) diff --git a/doc/README.arm-unaligned-accesses b/doc/README.arm-unaligned-accesses deleted file mode 100644 index c37d135..0000000 --- a/doc/README.arm-unaligned-accesses +++ /dev/null @@ -1,122 +0,0 @@ -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/ubifs/Makefile b/fs/ubifs/Makefile index 389b0e3..8c8c6ac 100644 --- a/fs/ubifs/Makefile +++ b/fs/ubifs/Makefile @@ -13,6 +13,3 @@ obj-y := ubifs.o io.o super.o sb.o master.o lpt.o obj-y += lpt_commit.o scan.o lprops.o obj-y += tnc.o tnc_misc.o debug.o crc16.o budget.o obj-y += log.o orphan.o recovery.o replay.o - -# SEE README.arm-unaligned-accesses -$(obj)super.o: CFLAGS += $(PLATFORM_NO_UNALIGNED) diff --git a/lib/Makefile b/lib/Makefile index 760340f..dedb97b 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -65,6 +65,3 @@ obj-y += vsprintf.o obj-$(CONFIG_RANDOM_MACADDR) += rand.o obj-$(CONFIG_BOOTP_RANDOM_DELAY) += rand.o obj-$(CONFIG_CMD_LINK_LOCAL) += rand.o - -# SEE README.arm-unaligned-accesses -$(obj)bzlib.o: CFLAGS += $(PLATFORM_NO_UNALIGNED)