[U-Boot] [PATCH v0 21/20] efi_loader: hack for archs that cannot do unaligned accesses

[Rob Clark]

On Sat, Aug 5, 2017 at 12:26 PM, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> On 08/05/2017 06:19 PM, Rob Clark wrote:
>> On Sat, Aug 5, 2017 at 12:16 PM, Rob Clark <robdclark at gmail.com> wrote:
>>> On Sat, Aug 5, 2017 at 12:12 PM, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>>> On 08/05/2017 05:58 PM, Rob Clark wrote:
>>>>> Some arch's have trouble with unaligned accesses.  Technically
>>>>> EFI device-path structs should be byte aligned, and the next node
>>>>> in the path starts immediately after the previous.  Meaning that
>>>>> a pointer to an 'struct efi_device_path' is not necessarily word
>>>>> aligned.  See section 10.3.1 in v2.7 of UEFI spec.
>>>>>
>>>>> This causes problems not just for u-boot, but also most/all EFI
>>>>> payloads loaded by u-boot on these archs.  Fortunately the common
>>>>> practice for traversing a device path is to rely on the length
>>>>> field in the header, rather than the specified length of the
>>>>> particular device path type+subtype.  So the EFI_DP_PAD() macro
>>>>> will add the specified number of bytes to the tail of device path
>>>>> structs to pad them to word alignment.
>>>>>
>>>>> Technically this is non-compliant, BROKEN_UNALIGNED should *only*
>>>>> be defined on archs that cannot do unaligned accesses.
>>>>>
>>>>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>>>>> ---
>>>>> I'm not sure if there are other arch's that need -DBROKEN_UNALIGNED
>>>>>
>>>>> Mark, this is untested but I think it should solve your crash on the
>>>>> Banana Pi.  Could you give it a try when you get a chance?
>>>>>
>>>>>  arch/arm/config.mk               |  2 +-
>>>>>  include/efi_api.h                | 30 ++++++++++++++++++++++++++++++
>>>>>  lib/efi_loader/efi_device_path.c |  3 +++
>>>>>  3 files changed, 34 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/arm/config.mk b/arch/arm/config.mk
>>>>> index 1a77779db4..067dc93a9d 100644
>>>>> --- a/arch/arm/config.mk
>>>>> +++ b/arch/arm/config.mk
>>>>> @@ -28,7 +28,7 @@ LLVMS_RELFLAGS              := $(call cc-option,-mllvm,) \
>>>>>                       $(call cc-option,-arm-use-movt=0,)
>>>>>  PLATFORM_RELFLAGS    += $(LLVM_RELFLAGS)
>>>>>
>>>>> -PLATFORM_CPPFLAGS += -D__ARM__
>>>>> +PLATFORM_CPPFLAGS += -D__ARM__ -DBROKEN_UNALIGNED
>>>>
>>>> NAK
>>>>
>>>> We have more then ARM. And other architectures also create exceptions
>>>> for unaligned access.
>>>>
>>>> I hate platform specific code. It should not be used outside /arch.
>>>>
>>>> To play it save you should not use _packed at all!
>>>> Use memcpy to transfer between aligned and unaligned memory.
>>>
>>> except for reasons I explained in the thread on the patch that added
>>> the __packed in the first place.  Sorry, this is ugly but we have to
>>> do it.
>>
>> well, to be fair, we don't *have* to do it.  The alternative is
>> disable EFI_LOADER on archs that cannot do unaligned accesses.  But
>> this seemed like the better option.
>>
>
> In which UEFI protocol do you need the packed structures?
> Why can't you use memcpy to provide/read the data in these protocols?
> Why can't you use padding on all architectures?


The device-path protocol, as described in section 10.3 in the UEFI
spec.  And as I already explained, the problem isn't just u-boot but
also all the various efi payloads loaded and executed directly or
indirectly by u-boot.  Please feel free to send patches to these
various projects to fix those problems on armv7/etc.  I guess it will
take a while to get them all upstream, if they are accepted (really,
*why* are we using efi on these archs?) and trickle out into distros.

I'll repeat the section I previously quoted from 10.3.1 in the other
email thread:

   A Device Path is a series of generic Device Path nodes. The first
   Device Path node starts at byte offset zero of the Device Path.
   The next Device Path node starts at the end of the previous Device
   Path node. Therefore all nodes are byte-packed data structures that
   may appear on any byte boundary. All code references to device path
   notes must assume all fields are unaligned. Since every Device Path
   node contains a length field in a known place, it is possible to
   traverse Device Path nodes that are of an unknown type. There is
   no limit to the number, type, or sequence of nodes in a Device Path.

What we were doing before was incorrect.  Sorry, but if we are
implementing the UEFI spec, we need to implement the UEFI spec, not
what we wish the UEFI spec was.  I fixed the bug in u-boot, and that
exposed the problem in openbsd's bootaa64.efi.  But I've spent enough
time looking at shim/fallback/grub to say that they probably have the
same problem on armv7 (and any other arch's that cannot do unaligned
access).

This patch allows us to not break stuff that currently works by
accident on these archs, but do things correctly on aarch64 and archs
that *can* do unaligned access.  This is better than doing things
incorrectly on aarch64, and better than breaking archs that relied on
this existing bug in u-boot's UEFI implementation.

BR,
-R