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

Tue Aug 8 06:52:14 UTC 2017

> Am 07.08.2017 um 23:18 schrieb Rob Clark <robdclark at gmail.com>:
> 
> On Mon, Aug 7, 2017 at 5:14 PM, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
>>> From: Alexander Graf <agraf at suse.de>
>>> Date: Mon, 7 Aug 2017 21:19:37 +0100
>>> 
>>>> On 05.08.17 21:31, Rob Clark wrote:
>>>>> On Sat, Aug 5, 2017 at 4:05 PM, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>>>>> On 08/05/2017 08:43 PM, Rob Clark wrote:
>>>>>>> On Sat, Aug 5, 2017 at 1:06 PM, Rob Clark <robdclark at gmail.com> wrote:
>>>>>>>> On Sat, Aug 5, 2017 at 12:52 PM, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>>>>>>>> On 08/05/2017 06:16 PM, Rob Clark 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.
>>>>>>>>> 
>>>>>>>>> BR,
>>>>>>>>> -R
>>>>>>>> 
>>>>>>>> According to the UEFI standard the nodes in paths are not to be assumed
>>>>>>>> to be aligned.
>>>>>>>> 
>>>>>>>> So even if you use padding bytes in paths that you pass to the EFI
>>>>>>>> application you should not expect that the EFI application does the
>>>>>>>> same. Expect the EFI application either to remove them or send new nodes
>>>>>>>> without padding.
>>>>>>>> 
>>>>>>>> To the idea of padding bytes and __packed does not make sense.
>>>>>>> 
>>>>>>> Ok, to be fair, you are right about device-paths passed too u-boot.
>>>>>>> On BROKEN_UNALIGNED archs, we should sanitise with a copy to an
>>>>>>> aligned device-path in *addition* to what I proposed.  I can make a
>>>>>>> patch to add a helper to do this a bit later.
>>>>>> 
>>>>>> so thinking about this a bit, I have two options in mind:
>>>>>> 
>>>>>>  + efi_dp_sanitize() + efi_dp_free() helpers that are no-ops on
>>>>>>    archs that can do unaligned access, but efi_dp_sanitize() always
>>>>>>    allocates and copies on BROKEN_UNALIGNED archs and efi_dp_free()
>>>>>>    always free's on BROKEN_UNALIGNED archs, even if the dp passed
>>>>>>    from efi payload doesn't require it.
>>>>>> 
>>>>>>  + efi_dp_sanitize() that is no-op on archs that can do unaligned
>>>>>>    access but only allocates/copies when passed a device path that
>>>>>>    would result in unaligned access, plus hook some mechanism to
>>>>>>    auto-free on EFI_EXIT().. which is slightly tricky w/ nesting of
>>>>>>    efi calls, but not impossible and at least avoids the problem
>>>>>>    of missing calls to free the dup'd device-path.. which is the
>>>>>>    sort of leak I might miss since I'm not using EFI_LOADER on any
>>>>>>    BROKEN_UNALIGNED arch
>>>>>> 
>>>>>> anyone who cares about armv7 + efi have a preference between always
>>>>>> doing an extra alloc+copy+free vs EFI_EXIT() magic?
>>>>>> 
>>>>> 
>>>>> Please, go for the simplest code.
>>>> 
>>>> well, the source of my question was there are two definitions of
>>>> "simplest" here, ie "simplest to use" and "simplest helpers".. I'm
>>>> mostly worried about future patches that use the helpers introducing
>>>> leaks.  But actually I think a reasonable middle ground is "simplest
>>>> helpers plus always make the helpers not no-ops for DEBUG builds"..
>>>> 
>>>>> I cannot imagine that copying takes more than 10ms for starting grub on
>>>>> any architecture. So the user will not notice it anyway.
>>>>> Assume every architecture requiring alignment for which you do not have
>>>>> proof of the contrary.
>>>>> 
>>>>> Even on ARM64 there are op codes that fail without alignment.
>>>> 
>>>> iirc the restrictions where mostly about device memory, and atomics.
>>>> Which should not apply here.  (And we have aarch64 servers in
>>>> production with grub, which doesn't take any protections about
>>>> unaligned device-path nodes, using non u-boot EFI implementations.  So
>>>> maybe that counts as proof to the contrary ;-))
>>> 
>>> The UEFI spec mandates that unaligned access are fixed up properly
>>> (usually by hardware). We violate the spec on 32bit ARM, but do fulfill
>>> it on AArch64.
>> 
>> Actually for AArch32 the spec says:
>> 
>>  Unaligned access should be enabled if supported; Alignment faults
>>  are enabled otherwise.
> 
> It is a bit strange to me that alignment faults are disabled by
> default, but presumably is disabled out of reset..  I don't totally
> understand the connection to having mmu disabled, unless all memory is
> treated like device memory without mmu enabled.  (Which, also,
> wouldn't that be super-slow?)  But anyway, I guess that is a bit
> beside the point.

It is treated as device memory with mmu disabled, yes.

> 
>> So UEFI payload that wants to support pre-ARMv7 hardware should make
>> sure it doesn't do any unaligned access.
>> 
>>> The reason things worked before really was just that both U-Boot and
>>> grub were compiled with -mno-unaligned-access, so they never issued
>>> unaligned accesses.
>> 
>> That in itself is not enough to prevent unaligned access.  If you
>> explicitly cast an unaligned pointer to say (uint32_t *) and then
>> dereference it, you'll create an unaligned access.
> 
> This is problematic around file nodes in the device path.  Adding the
> padding bytes to the end of each device-path struct would "solve"
> that, and if pre-aarch64 we are aiming at "good enough to work", I
> kinda think that this the approach we should go for.  Other than
> file-path nodes, the rest of the issues in u-boot should be solved by
> addition of missing __packed on 'struct efi_device_path' (which I've
> added locally and will be in the next revision of the patchset).

Let's ask Leif and Ard if that is actually correct. If I remember correctly, edk2 some times leverages automatic padding from the compiler on structs.

> 
>>> Which system broke for Mark? Banana pi mostly has 32bit SBCs, so I
>>> assume it's one of those? In that case, we would need to either
>>> 
>>>   1) hack up the openbsd loader to also get compiled with
>>> -mno-unaligned-access
>>> 
>>> or
>>> 
>>>   2) rework the 32bit arm mmu code to enable the MMU plus hardware
>>> unaligned fixups on all systems. (that's a *lot* of work)
>> 
>> The system that broke was Cortex-A7 based, so ARMv7.
>> 
>> *However*, the OpenBSD bootloader does not perform any unaligned
>> access.  *All* cases of unaligned access that I encountered while
>> testing/debugging Rob's code were in U-Boot itself.  This was mostly a
>> consequence from a diff in the series that added __packed to the
>> device path node structs and then passed members of those structs to
>> functions that accessed the contents of tose (now byte aligned
>> structs) through (uint16_t *) or (uint32_t *) pointers.
> 
> Yeah.. the padding byte approach I suggested would solve this.  Not
> *really* compliant, but should work for all the payload code I've come
> across.  We definitely shouldn't enable the padding bytes on aarch64,
> but I think padding bytes would be a pragmatic solution for armv7 and
> earlier.  (And hopefully someday someone enables mmu on armv7 so we
> can turn off the hack there too.)

The problem really is inconsistency. Some 32bit arm platforms enable the mmu, some don't.

> 
>> There was initially some confusion because I used the pc reported by
>> U-Boot to look up the faulting instruction whereas I should have been
>> using the reloc_pc.
>> 
>> I think Rob is worried about grub/shim/fallback.efi doing unaligned
>> access of device path nodes on armv7.  But I'm not sure he has any
>> evidence beyond looking at code.
> 
> It will definitely be an issue (if grub/shim/fallback were not
> compiled with -mno-unaligned-access) in some of the debug code that
> prints out device-paths.  I haven't checked the compiler flags used w/
> shim/fallback.
> 
> Parsing MEDIA_DEVICE:HARD_DRIVE nodes would be an issue regardless
> without -mno-unaligned-access, although we might be getting lucky by
> just not hitting debug paths.  Some of the other issues would be
> hidden by padding bytes at the end of the DP structs which would keep
> the file-path nodes word aligned.
> 
>>> To me personally, UEFI in 32bit ARM land is a nice to have standardized
>>> platform, but not something where we really need to be fully standards
>>> compliant in every aspect. I think "making things work" is good enough
>>> there.
>> 
>> Right.  For us UEFI us just a convenient way to re-use the U-Boot
>> device driver code to load a kernel from a UFS/FFS filesystem.
>> 
>>> For AArch64 things are different. There we should strive for full UEFI
>>> compliance as it's "the future" :).
>> 
>> Even there just making things work would be good enough for me ;).
>> Our AArach64 bootloader is almost identical to the AArch32 one and
>> works on a real UEFI implementation as well (SoftIron Overdrive 1000).
>> 
> 
> I think we should make the aarch64 implementation fully compliant (ie.
> addition of missing __packed's and no extra padding bytes).  I won't
> loose sleep if some efi payloads don't work on pre-aarch64 (we should
> be able to keep things that were working before working).  If adding
> missing __packed breaks things on platforms that can't do unaligned
> access, the solution is not to remove __packed, but to conditionally
> add padding bytes at the end of the struct.  That way we keep things
> working as before on the old platforms, but make things work correctly
> on aarch64.
> 
> I'll add back my patch for EFI_DP_PAD() to this patchset, since this
> seems like the sensible way forward.

Let's make sure we get an ack from Leif/Ard for that approach :)

Alex