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

[Heinrich Schuchardt]

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.

> 
>> 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
>