On 04/04/2018 06:11 PM, Alexander Graf wrote: > > > On 04.04.18 17:10, Heinrich Schuchardt wrote: >> On 04/04/2018 02:32 PM, Alexander Graf wrote: >>> >>> >>> On 03.04.18 21:59, Heinrich Schuchardt wrote: >>>> The UEFI spec mandates that unaligned memory access should be enabled if >>>> supported by the CPU architecture. >>>> >>>> This patch adds an empty weak function unaligned_access() that can be >>>> overridden by an architecture specific routine. >>>> >>>> Signed-off-by: Heinrich Schuchardt <xypron.g...@gmx.de> >>>> --- >>>> cmd/bootefi.c | 13 +++++++++++++ >>>> include/asm-generic/unaligned.h | 3 +++ >>>> 2 files changed, 16 insertions(+) >>>> >>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >>>> index ba258ac9f3..412e6519ba 100644 >>>> --- a/cmd/bootefi.c >>>> +++ b/cmd/bootefi.c >>>> @@ -18,6 +18,7 @@ >>>> #include <memalign.h> >>>> #include <asm/global_data.h> >>>> #include <asm-generic/sections.h> >>>> +#include <asm-generic/unaligned.h> >>>> #include <linux/linkage.h> >>>> >>>> DECLARE_GLOBAL_DATA_PTR; >>>> @@ -89,6 +90,15 @@ out: >>>> return ret; >>>> } >>>> >>>> +/* >>>> + * Allow unaligned memory access. >>>> + * >>>> + * This routine is overridden by architectures providing this feature. >>>> + */ >>>> +void __weak allow_unaligned(void) >>>> +{ >>>> +} >>>> + >>> >>> I'd prefer to guard the body of the function with an #ifdef CONFIG_ARM >>> so everyone knows why it's there. Then call straight into a function >>> provided in the ARM core code: >> >> The same visibility can be achieved with a comment. > > It's not as obvious. The default really should be to map memory as > cached and allow for unaligned accesses. > >> >>> >>> static void allow_unaligned(void) >>> { >>> /* On ARM we prohibit unaligned accesses by default, enable them here */ >>> #if defined(CONFIG_ARM) && !defined(CONFIG_ARM64) && >>> !defined(CONFIG_CPU_V7M) >>> mmu_enable_unaligned(); >>> #endif >>> } >> >> RISC-V also supports traps for unaligned access. > > Just because it supports them doesn't mean we should use them. AArch64 > also allows for unaligned access traps and I went through great length > to refactor the mm code in U-Boot to ensure that we do not trap. > >> Using architecture specific flags outside arch/ is a mess. >> We should not commit new sins but eliminate the existing deviations. >> >>> >>> And then in arch/arm/lib/cache-cp15.c: >>> >>> void mmu_enable_unaligned(void) >>> { >>> set_cr(get_cr() & ~CR_A); >>> } >> >> For some ARM architecture versions the bit is reserved and not used for >> unaligned access. No clue what this function would do in this case. > > Do you have pointers? Anything defined in the UEFI spec should have the bit.
UEFI spec 2.7: <cite>2.3.5 AArch32 Platforms In addition, the invoking OSs can assume that unaligned access support is enabled if it is present in the processor.</cite> So the UEFI spec foresees processors supporting unaligned memory access and others that do not support it. > >> That is why I used a weak function that does nothing if the CPU does not >> support the flag. > > Any platform that uses cache-cp15 also supports the CR_A bit FWIW. So it > really belongs there.> > And again, I do not want to prettify a special hack for a broken > architecture. People should see warts when they're there. > > The real fix IMHO is to enable aligned accesses always, like we do on > any sane architecture. > Is this a typo: "enable aligned accesses"? Aligned access is always enabled. It is unaligned access that currently is not enabled in U-Boot. Regards Heinrich > > Alex > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot