diff mbox series

armv8: cpu: Implement allow_unaligned()

Message ID 20241031220605.31684-1-semen.protsenko@linaro.org
State Superseded
Headers show
Series armv8: cpu: Implement allow_unaligned() | expand

Commit Message

Sam Protsenko Oct. 31, 2024, 10:06 p.m. UTC
Usually ARMv8 platforms allow unaligned access for Normal memory. But
some chips might not allow it by default, having SCTLR.A bit set to 1
before U-Boot execution. One such example is Exynos850 SoC. As
allow_unaligned() is not implemented for ARMv8 at the moment, its __weak
implementation is used, which does nothing. That might lead to unaligned
access abort, for example when running EFI selftest. Fix that by
implementing allow_unaligned() for ARMv8.

The issue was found when running EFI selftest on E850-96 board
(Exynos850 based):

    => bootefi selftest $fdtcontroladdr

    ...
    Executing 'HII database protocols'
    "Synchronous Abort" handler, esr 0x96000021, far 0xbaac0991
    ...
    resetting ...

Unaligned abort happens in u16_strnlen(), which is called from
efi_hii_sibt_string_ucs2_block_next():

    u16_strlen(blk->string_text)

where 'blk' type is struct efi_hii_sibt_string_ucs2_block. Because this
struct is packed, doing "->string_text" makes 'blk' address incremented
by 1 byte, which makes it unaligned. Although allow_unaligned() was
called in efi_init_early() before EFI selftest execution, it wasn't
implemented for ARMv8 CPUs, so data abort happened.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 arch/arm/cpu/armv8/cpu.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Heinrich Schuchardt Nov. 1, 2024, 11:46 a.m. UTC | #1
On 10/31/24 23:06, Sam Protsenko wrote:
> Usually ARMv8 platforms allow unaligned access for Normal memory. But
> some chips might not allow it by default, having SCTLR.A bit set to 1
> before U-Boot execution. One such example is Exynos850 SoC. As
> allow_unaligned() is not implemented for ARMv8 at the moment, its __weak
> implementation is used, which does nothing. That might lead to unaligned
> access abort, for example when running EFI selftest. Fix that by
> implementing allow_unaligned() for ARMv8.
>
> The issue was found when running EFI selftest on E850-96 board
> (Exynos850 based):
>
>      => bootefi selftest $fdtcontroladdr
>
>      ...
>      Executing 'HII database protocols'
>      "Synchronous Abort" handler, esr 0x96000021, far 0xbaac0991
>      ...
>      resetting ...
>
> Unaligned abort happens in u16_strnlen(), which is called from
> efi_hii_sibt_string_ucs2_block_next():
>
>      u16_strlen(blk->string_text)
>
> where 'blk' type is struct efi_hii_sibt_string_ucs2_block. Because this
> struct is packed, doing "->string_text" makes 'blk' address incremented
> by 1 byte, which makes it unaligned. Although allow_unaligned() was
> called in efi_init_early() before EFI selftest execution, it wasn't
> implemented for ARMv8 CPUs, so data abort happened.
>
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>   arch/arm/cpu/armv8/cpu.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/arch/arm/cpu/armv8/cpu.c b/arch/arm/cpu/armv8/cpu.c
> index d568efa427ab..82ecf02f4b03 100644
> --- a/arch/arm/cpu/armv8/cpu.c
> +++ b/arch/arm/cpu/armv8/cpu.c
> @@ -94,3 +94,8 @@ void armv8_setup_psci(void)
>   	secure_ram_addr(psci_arch_init)();
>   }
>   #endif
> +
> +void allow_unaligned(void)
> +{
> +	set_sctlr(get_sctlr() & ~CR_A);

Looking at the ARM documentation SCTLR_EL2 is a 64bit register.

https://developer.arm.com/documentation/ddi0595/2021-06/AArch64-Registers/SCTLR-EL2--System-Control-Register--EL2-?lang=en

But get_sctlr() is defined as

arch/arm/include/asm/system.h:174:
static inline unsigned int get_sctlr(void)

Your code might not work as expected. Should we adjust the definition of
get_sctlr() to return a long value?

Best regards

Heinrich

> +}
Sam Protsenko Nov. 6, 2024, 1:09 a.m. UTC | #2
On Fri, Nov 1, 2024 at 6:46 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>

[snip]

> > +void allow_unaligned(void)
> > +{
> > +     set_sctlr(get_sctlr() & ~CR_A);
>
> Looking at the ARM documentation SCTLR_EL2 is a 64bit register.
>
> https://developer.arm.com/documentation/ddi0595/2021-06/AArch64-Registers/SCTLR-EL2--System-Control-Register--EL2-?lang=en
>
> But get_sctlr() is defined as
>
> arch/arm/include/asm/system.h:174:
> static inline unsigned int get_sctlr(void)
>
> Your code might not work as expected. Should we adjust the definition of
> get_sctlr() to return a long value?
>

Hi Heinrich,

Good catch! I totally overlooked it. And it's already used in a
similar way in a couple of different places. I'll handle that in my v2
soon.

Thanks!

> Best regards
>
> Heinrich
>
> > +}
>
diff mbox series

Patch

diff --git a/arch/arm/cpu/armv8/cpu.c b/arch/arm/cpu/armv8/cpu.c
index d568efa427ab..82ecf02f4b03 100644
--- a/arch/arm/cpu/armv8/cpu.c
+++ b/arch/arm/cpu/armv8/cpu.c
@@ -94,3 +94,8 @@  void armv8_setup_psci(void)
 	secure_ram_addr(psci_arch_init)();
 }
 #endif
+
+void allow_unaligned(void)
+{
+	set_sctlr(get_sctlr() & ~CR_A);
+}