diff mbox series

efi_loader: define allow_unaligned as 'asm volatile'

Message ID 20230320171335.1368308-1-ilias.apalodimas@linaro.org
State New
Headers show
Series efi_loader: define allow_unaligned as 'asm volatile' | expand

Commit Message

Ilias Apalodimas March 20, 2023, 5:13 p.m. UTC
Tom reports that compiling with LTO & EFI breaks armv7 and arm11*
builds.  The reason is that LTO (maybe a binutils bug?) replaces the
asm version of allow_unaligned() with the __weak definition of the
function.  As a result EFI code ends up running with unaligned accesses
disabled and eventually crashes.

allow_unaligned() hardware specific variants are usually defined in .S
files.  Switching those to inline assembly fixes the problem and the
linker keeps the correct version in the final binary

Reported-by: Tom Rini <trini@konsulko.com>
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
This is an alternative approach to https://lore.kernel.org/u-boot/ZBRy1oSZ5iVFqrdV@hera/
since enabling unaligned accesses by default has been rejected in the past

 arch/arm/cpu/arm11/Makefile |  4 ----
 arch/arm/cpu/arm11/cpu.c    | 13 +++++++++++++
 arch/arm/cpu/arm11/sctlr.S  | 25 -------------------------
 arch/arm/cpu/armv7/Makefile |  1 -
 arch/arm/cpu/armv7/cpu.c    | 11 +++++++++++
 arch/arm/cpu/armv7/sctlr.S  | 22 ----------------------
 6 files changed, 24 insertions(+), 52 deletions(-)
 delete mode 100644 arch/arm/cpu/arm11/sctlr.S
 delete mode 100644 arch/arm/cpu/armv7/sctlr.S

--
2.39.2

Comments

Heinrich Schuchardt March 21, 2023, 7:43 p.m. UTC | #1
On 3/20/23 18:13, Ilias Apalodimas wrote:
> Tom reports that compiling with LTO & EFI breaks armv7 and arm11*
> builds.  The reason is that LTO (maybe a binutils bug?) replaces the
> asm version of allow_unaligned() with the __weak definition of the
> function.  As a result EFI code ends up running with unaligned accesses
> disabled and eventually crashes.
> 
> allow_unaligned() hardware specific variants are usually defined in .S
> files.  Switching those to inline assembly fixes the problem and the
> linker keeps the correct version in the final binary
> 
> Reported-by: Tom Rini <trini@konsulko.com>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> This is an alternative approach to https://lore.kernel.org/u-boot/ZBRy1oSZ5iVFqrdV@hera/
> since enabling unaligned accesses by default has been rejected in the past
> 
>   arch/arm/cpu/arm11/Makefile |  4 ----
>   arch/arm/cpu/arm11/cpu.c    | 13 +++++++++++++
>   arch/arm/cpu/arm11/sctlr.S  | 25 -------------------------
>   arch/arm/cpu/armv7/Makefile |  1 -
>   arch/arm/cpu/armv7/cpu.c    | 11 +++++++++++
>   arch/arm/cpu/armv7/sctlr.S  | 22 ----------------------
>   6 files changed, 24 insertions(+), 52 deletions(-)
>   delete mode 100644 arch/arm/cpu/arm11/sctlr.S
>   delete mode 100644 arch/arm/cpu/armv7/sctlr.S
> 
> diff --git a/arch/arm/cpu/arm11/Makefile b/arch/arm/cpu/arm11/Makefile
> index 5dfa01ae8d05..5d721fce12b5 100644
> --- a/arch/arm/cpu/arm11/Makefile
> +++ b/arch/arm/cpu/arm11/Makefile
> @@ -4,7 +4,3 @@
>   # Wolfgang Denk, DENX Software Engineering, wd@denx.de.
> 
>   obj-y	= cpu.o
> -
> -ifneq ($(CONFIG_SPL_BUILD),y)
> -obj-$(CONFIG_EFI_LOADER) += sctlr.o
> -endif
> diff --git a/arch/arm/cpu/arm11/cpu.c b/arch/arm/cpu/arm11/cpu.c
> index ffe35111d589..b19c4fc4a6c2 100644
> --- a/arch/arm/cpu/arm11/cpu.c
> +++ b/arch/arm/cpu/arm11/cpu.c
> @@ -111,3 +111,16 @@ void enable_caches(void)
>   #endif
>   }
>   #endif
> +
> +#if !IS_ENABLED(CONFIG_SPL_BUILD)
> +void allow_unaligned(void)
> +{
> +	asm volatile(
> +	"mrc	p15, 0, r0, c1, c0, 0\n" //load system control register
> +	"orr	r0, r0, #1 << 22\n"	 //set unaligned data support flag
> +	"bic	r0, r0, #2\n"		 //clear aligned flag
> +	"mcr	p15, 0, r0, c1, c0, 0\n" // write system control register
> +	"bx	lr\n"			 //return
> +	);
> +}
> +#endif
> diff --git a/arch/arm/cpu/arm11/sctlr.S b/arch/arm/cpu/arm11/sctlr.S
> deleted file mode 100644
> index 74a7fc4a25b6..000000000000
> --- a/arch/arm/cpu/arm11/sctlr.S
> +++ /dev/null
> @@ -1,25 +0,0 @@
> -/* SPDX-License-Identifier:	GPL-2.0+ */
> -/*
> - *  Routines to access the system control register
> - *
> - *  Copyright (c) 2019 Heinrich Schuchardt
> - */
> -
> -#include <linux/linkage.h>
> -
> -/*
> - * void allow_unaligned(void) - allow unaligned access
> - *
> - * This routine sets the enable unaligned data support flag and clears the
> - * aligned flag in the system control register.
> - * After calling this routine unaligned access does no longer leads to a
> - * data abort or undefined behavior but is handled by the CPU.
> - * For details see the "ARM Architecture Reference Manual" for ARMv6.
> - */
> -ENTRY(allow_unaligned)
> -	mrc	p15, 0, r0, c1, c0, 0	@ load system control register
> -	orr	r0, r0, #1 << 22	@ set unaligned data support flag
> -	bic	r0, r0, #2		@ clear aligned flag
> -	mcr	p15, 0, r0, c1, c0, 0	@ write system control register
> -	bx	lr			@ return
> -ENDPROC(allow_unaligned)
> diff --git a/arch/arm/cpu/armv7/Makefile b/arch/arm/cpu/armv7/Makefile
> index 653eef8ad79e..ca50f6e93e10 100644
> --- a/arch/arm/cpu/armv7/Makefile
> +++ b/arch/arm/cpu/armv7/Makefile
> @@ -13,7 +13,6 @@ obj-y	+= syslib.o
>   obj-$(CONFIG_SYS_ARM_MPU) += mpu_v7r.o
> 
>   ifneq ($(CONFIG_SPL_BUILD),y)
> -obj-$(CONFIG_EFI_LOADER) += sctlr.o
>   obj-$(CONFIG_ARMV7_NONSEC) += exception_level.o
>   endif
> 
> diff --git a/arch/arm/cpu/armv7/cpu.c b/arch/arm/cpu/armv7/cpu.c
> index 68807d209978..9742fa65e3cf 100644
> --- a/arch/arm/cpu/armv7/cpu.c
> +++ b/arch/arm/cpu/armv7/cpu.c
> @@ -83,3 +83,14 @@ int cleanup_before_linux(void)
>   {
>   	return cleanup_before_linux_select(CBL_ALL);
>   }
> +
> +#if !IS_ENABLED(CONFIG_SPL_BUILD)

Why do we need this #if? The linker should eliminate unused functions.

Best regards

Heinrich

> +void allow_unaligned(void)
> +{
> +	asm volatile(
> +	"mrc	p15, 0, r0, c1, c0, 0\n" //load system control register
> +	"bic	r0, r0, #2\n"		 //clear aligned flag
> +	"mcr	p15, 0, r0, c1, c0, 0\n" //write system control register
> +	);
> +}
> +#endif
> diff --git a/arch/arm/cpu/armv7/sctlr.S b/arch/arm/cpu/armv7/sctlr.S
> deleted file mode 100644
> index bd56e41afe18..000000000000
> --- a/arch/arm/cpu/armv7/sctlr.S
> +++ /dev/null
> @@ -1,22 +0,0 @@
> -/* SPDX-License-Identifier:     GPL-2.0+ */
> -/*
> - *  Routines to access the system control register
> - *
> - *  Copyright (c) 2018 Heinrich Schuchardt
> - */
> -
> -#include <linux/linkage.h>
> -
> -/*
> - * void allow_unaligned(void) - allow unaligned access
> - *
> - * This routine clears the aligned flag in the system control register.
> - * After calling this routine unaligned access does no longer lead to a
> - * data abort but is handled by the CPU.
> - */
> -ENTRY(allow_unaligned)
> -	mrc	p15, 0, r0, c1, c0, 0	@ load system control register
> -	bic	r0, r0, #2		@ clear aligned flag
> -	mcr	p15, 0, r0, c1, c0, 0	@ write system control register
> -	bx	lr			@ return
> -ENDPROC(allow_unaligned)
> --
> 2.39.2
>
diff mbox series

Patch

diff --git a/arch/arm/cpu/arm11/Makefile b/arch/arm/cpu/arm11/Makefile
index 5dfa01ae8d05..5d721fce12b5 100644
--- a/arch/arm/cpu/arm11/Makefile
+++ b/arch/arm/cpu/arm11/Makefile
@@ -4,7 +4,3 @@ 
 # Wolfgang Denk, DENX Software Engineering, wd@denx.de.

 obj-y	= cpu.o
-
-ifneq ($(CONFIG_SPL_BUILD),y)
-obj-$(CONFIG_EFI_LOADER) += sctlr.o
-endif
diff --git a/arch/arm/cpu/arm11/cpu.c b/arch/arm/cpu/arm11/cpu.c
index ffe35111d589..b19c4fc4a6c2 100644
--- a/arch/arm/cpu/arm11/cpu.c
+++ b/arch/arm/cpu/arm11/cpu.c
@@ -111,3 +111,16 @@  void enable_caches(void)
 #endif
 }
 #endif
+
+#if !IS_ENABLED(CONFIG_SPL_BUILD)
+void allow_unaligned(void)
+{
+	asm volatile(
+	"mrc	p15, 0, r0, c1, c0, 0\n" //load system control register
+	"orr	r0, r0, #1 << 22\n"	 //set unaligned data support flag
+	"bic	r0, r0, #2\n"		 //clear aligned flag
+	"mcr	p15, 0, r0, c1, c0, 0\n" // write system control register
+	"bx	lr\n"			 //return
+	);
+}
+#endif
diff --git a/arch/arm/cpu/arm11/sctlr.S b/arch/arm/cpu/arm11/sctlr.S
deleted file mode 100644
index 74a7fc4a25b6..000000000000
--- a/arch/arm/cpu/arm11/sctlr.S
+++ /dev/null
@@ -1,25 +0,0 @@ 
-/* SPDX-License-Identifier:	GPL-2.0+ */
-/*
- *  Routines to access the system control register
- *
- *  Copyright (c) 2019 Heinrich Schuchardt
- */
-
-#include <linux/linkage.h>
-
-/*
- * void allow_unaligned(void) - allow unaligned access
- *
- * This routine sets the enable unaligned data support flag and clears the
- * aligned flag in the system control register.
- * After calling this routine unaligned access does no longer leads to a
- * data abort or undefined behavior but is handled by the CPU.
- * For details see the "ARM Architecture Reference Manual" for ARMv6.
- */
-ENTRY(allow_unaligned)
-	mrc	p15, 0, r0, c1, c0, 0	@ load system control register
-	orr	r0, r0, #1 << 22	@ set unaligned data support flag
-	bic	r0, r0, #2		@ clear aligned flag
-	mcr	p15, 0, r0, c1, c0, 0	@ write system control register
-	bx	lr			@ return
-ENDPROC(allow_unaligned)
diff --git a/arch/arm/cpu/armv7/Makefile b/arch/arm/cpu/armv7/Makefile
index 653eef8ad79e..ca50f6e93e10 100644
--- a/arch/arm/cpu/armv7/Makefile
+++ b/arch/arm/cpu/armv7/Makefile
@@ -13,7 +13,6 @@  obj-y	+= syslib.o
 obj-$(CONFIG_SYS_ARM_MPU) += mpu_v7r.o

 ifneq ($(CONFIG_SPL_BUILD),y)
-obj-$(CONFIG_EFI_LOADER) += sctlr.o
 obj-$(CONFIG_ARMV7_NONSEC) += exception_level.o
 endif

diff --git a/arch/arm/cpu/armv7/cpu.c b/arch/arm/cpu/armv7/cpu.c
index 68807d209978..9742fa65e3cf 100644
--- a/arch/arm/cpu/armv7/cpu.c
+++ b/arch/arm/cpu/armv7/cpu.c
@@ -83,3 +83,14 @@  int cleanup_before_linux(void)
 {
 	return cleanup_before_linux_select(CBL_ALL);
 }
+
+#if !IS_ENABLED(CONFIG_SPL_BUILD)
+void allow_unaligned(void)
+{
+	asm volatile(
+	"mrc	p15, 0, r0, c1, c0, 0\n" //load system control register
+	"bic	r0, r0, #2\n"		 //clear aligned flag
+	"mcr	p15, 0, r0, c1, c0, 0\n" //write system control register
+	);
+}
+#endif
diff --git a/arch/arm/cpu/armv7/sctlr.S b/arch/arm/cpu/armv7/sctlr.S
deleted file mode 100644
index bd56e41afe18..000000000000
--- a/arch/arm/cpu/armv7/sctlr.S
+++ /dev/null
@@ -1,22 +0,0 @@ 
-/* SPDX-License-Identifier:     GPL-2.0+ */
-/*
- *  Routines to access the system control register
- *
- *  Copyright (c) 2018 Heinrich Schuchardt
- */
-
-#include <linux/linkage.h>
-
-/*
- * void allow_unaligned(void) - allow unaligned access
- *
- * This routine clears the aligned flag in the system control register.
- * After calling this routine unaligned access does no longer lead to a
- * data abort but is handled by the CPU.
- */
-ENTRY(allow_unaligned)
-	mrc	p15, 0, r0, c1, c0, 0	@ load system control register
-	bic	r0, r0, #2		@ clear aligned flag
-	mcr	p15, 0, r0, c1, c0, 0	@ write system control register
-	bx	lr			@ return
-ENDPROC(allow_unaligned)