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

[Alexander Graf]

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.

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


Alex