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

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.

-- 
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/20180507/5e748156/attachment.sig>