[U-Boot] [PATCH v2 1/3] efi_loader: allow unaligned memory access

[Alexander Graf]

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 <xypron.glpk at 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.

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:

   https://patchwork.ozlabs.org/patch/893014/


Alex