Re: [PATCH] arm: enable unaligned accesses by default if EFI is configured

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Heinrich,

On Fri, Mar 17, 2023 at 02:56:02PM +0100, Heinrich Schuchardt wrote:
> On 3/17/23 14:42, Ilias Apalodimas wrote:
> > Heinrich 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.
> >
> > So let's enable unaligned access for those architectures during
> > our start.S routines and avoid the linker issues.
> >
> > Reported-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>
> The problem was originally reported by Tom. My contribution was to track it
> down to missing enabling of unaligned access due to a linker problem.
>

Ah thanks, I'll fix that in v2

[...]

> >   	bic	r0, r0, #0x00002300	@ clear bits 13, 9:8 (--V- --RS)
> >   	bic	r0, r0, #0x00000087	@ clear bits 7, 2:0 (B--- -CAM)
> > +#if !CONFIG_IS_ENABLED(EFI_LOADER)
> > +	/* allow unaligned accesses since EFI requires it */
>
> This comment line is only reached without UEFI support. So here you should
> explain why we forbid unaligned access without UEFI.

I am not sure I am following.  I don't really remember why we had UA
disabled on armv7.  Do you have any suggestions?

regards
/Ilias
>
> Best regards
>
> Heinrich
>
> >   	orr	r0, r0, #0x00000002	@ set bit 1 (A) Align
> > +#endif
> >   	orr	r0, r0, #0x00001000	@ set bit 12 (I) I-Cache
> >   	mcr	p15, 0, r0, c1, c0, 0
> > diff --git a/arch/arm/cpu/arm1176/start.S b/arch/arm/cpu/arm1176/start.S
> > index 78a9cc173a39..0c80160702ba 100644
> > --- a/arch/arm/cpu/arm1176/start.S
> > +++ b/arch/arm/cpu/arm1176/start.S
> > @@ -79,7 +79,10 @@ cpu_init_crit:
> >   	mrc	p15, 0, r0, c1, c0, 0
> >   	bic	r0, r0, #0x00002300	@ clear bits 13, 9:8 (--V- --RS)
> >   	bic	r0, r0, #0x00000087	@ clear bits 7, 2:0 (B--- -CAM)
> > +#if !CONFIG_IS_ENABLED(EFI_LOADER)
> > +	/* allow unailgned accesses since EFI requires it */
> >   	orr	r0, r0, #0x00000002	@ set bit 1 (A) Align
> > +#endif
> >   	orr	r0, r0, #0x00001000	@ set bit 12 (I) I-Cache
> >   	/* Prepare to disable the MMU */
> > 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/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)
> > diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
> > index 7d7aac021e22..abb89efb9ed5 100644
> > --- a/arch/arm/cpu/armv7/start.S
> > +++ b/arch/arm/cpu/armv7/start.S
> > @@ -197,7 +197,10 @@ ENTRY(cpu_init_cp15)
> >   	mrc	p15, 0, r0, c1, c0, 0
> >   	bic	r0, r0, #0x00002000	@ clear bits 13 (--V-)
> >   	bic	r0, r0, #0x00000007	@ clear bits 2:0 (-CAM)
> > +#if !CONFIG_IS_ENABLED(EFI_LOADER)
> > +	/* allow unaligned accesses since EFI requires it */
> >   	orr	r0, r0, #0x00000002	@ set bit 1 (--A-) Align
> > +#endif
> >   	orr	r0, r0, #0x00000800	@ set bit 11 (Z---) BTB
> >   #if CONFIG_IS_ENABLED(SYS_ICACHE_OFF)
> >   	bic	r0, r0, #0x00001000	@ clear bit 12 (I) I-cache
> > diff --git a/include/asm-generic/unaligned.h b/include/asm-generic/unaligned.h
> > index 3d33a5a063e8..cf032ce1d4cc 100644
> > --- a/include/asm-generic/unaligned.h
> > +++ b/include/asm-generic/unaligned.h
> > @@ -19,8 +19,4 @@
> >   #else
> >   #error invalid endian
> >   #endif
> > -
> > -/* Allow unaligned memory access */
> > -void allow_unaligned(void);
> > -
> >   #endif
> > diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> > index 3b267b713e98..bbaaf626d9cb 100644
> > --- a/lib/efi_loader/efi_device_path.c
> > +++ b/lib/efi_loader/efi_device_path.c
> > @@ -954,13 +954,6 @@ static void path_to_uefi(void *uefi, const char *src)
> >   {
> >   	u16 *pos = uefi;
> > -	/*
> > -	 * efi_set_bootdev() calls this routine indirectly before the UEFI
> > -	 * subsystem is initialized. So we cannot assume unaligned access to be
> > -	 * enabled.
> > -	 */
> > -	allow_unaligned();
> > -
> >   	while (*src) {
> >   		s32 code = utf8_get(&src);
> > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> > index 58d4e1340233..bd17db61c697 100644
> > --- a/lib/efi_loader/efi_setup.c
> > +++ b/lib/efi_loader/efi_setup.c
> > @@ -17,15 +17,6 @@
> >   efi_status_t efi_obj_list_initialized = OBJ_LIST_NOT_INITIALIZED;
> > -/*
> > - * Allow unaligned memory access.
> > - *
> > - * This routine is overridden by architectures providing this feature.
> > - */
> > -void __weak allow_unaligned(void)
> > -{
> > -}
> > -
> >   /**
> >    * efi_init_platform_lang() - define supported languages
> >    *
> > @@ -191,9 +182,6 @@ int efi_init_early(void)
> >   {
> >   	efi_status_t ret;
> > -	/* Allow unaligned memory access */
> > -	allow_unaligned();
> > -
> >   	/* Initialize root node */
> >   	ret = efi_root_node_register();
> >   	if (ret != EFI_SUCCESS)
>