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.g...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
