On 05/07/2018 11:53 PM, Tom Rini wrote: > On Thu, Apr 05, 2018 at 09:39:20AM +0200, Alexander Graf wrote: >> On 04/04/2018 09:14 PM, Heinrich Schuchardt wrote: >>> 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 <[email protected]> >>>>>>> --- >>>>>>> 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. >> >> I think the only corner case is Cortex-M, but that's not terribly high up on >> my priority list. And if that requires everything to be aligned, so be 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. >> >> Yes, enable unaligned accesses of course :). >> >> Albert, this is your call I think. Would you be heavily opposed to >> Heinrich's initial patch? It really is the best option IMHO: > > Let me try actually saying something this time. We had a large amount > of back and forth over "unaligned access" over the last several years. > Heinrich's is doing something we've expressly chosen not to do (and > there's pages and pages of discussion in the archives). So the changes > here to enter an EFI application in the way it expects need to be done > for EFI as part of entering EFI. I would almost suggest something like > a prepare_for_efi (and a matching unwind) function. >
The bootefi command may serve to load an EFI driver binary which will stay active after returning to the U-Boot prompt. So we cannot disallow unaligned access after calling bootefi. So there is nothing we could "unwind". As this patch series moves allowing unaligned access to the bootefi call I suggest to procede with it. Best regards Heinrich _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
