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

[Tom Rini]

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:
> 
>   https://patchwork.ozlabs.org/patch/893014/
> 
> 
> Alex
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180506/af236501/attachment.sig>