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

[Heinrich Schuchardt]

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