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

[Rob Clark]

On Tue, Aug 8, 2017 at 8:26 AM, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
>> From: Rob Clark <robdclark at gmail.com>
>> Date: Mon, 7 Aug 2017 18:18:50 -0400
>>
>> 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
>> >>
>> >> 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.
>
> Adding padding does not magically align things on a >8-bit boundary.
> And as Peter pointed out, the hard drive device path type is
> inherently unalignable.  The UEFI payload simply has to deal with
> that.  If grub/shim/fallback.efi is currently broken, it will have to
> be fixed to be usable with U-Boot on 32-bit ARM.
>
> The problem that needs to be fixed is the manipulation of device path
> nodes in U-Boot itself.  Using __packed and -mno-aligned-access should
> take care of any parsing done through the device path node struct
> types.  But whenever a member of such a struct is passed to some other
> function that function should be careful not to do unaligned access.
> After you fixed the GUID printing code it seems ascii2unicode() is the
> only function left that does an unaligned access.  Perhaps the
> solution here is to simply stop using file path nodes in U-Boot like
> you already suggested.  Or otherwise just do the ascii2unicode()
> conversion into a suitably aligned buffer and memcpy() that into
> place.
>

It is also a problem with all the utf16 string handling, which shows
up all over the place.  For example if efi payload (or
efi_load_image()) took the file path string from a device-path, which
was unaligned.  I'm not really a fan of trying to deal with this
everywhere, since it is making the code much more cumbersome for the
benefit of platforms that are broken (and still will be broken in ways
we cannot prevent) from an EFI standpoint.

So I'm pretty much NAK on playing whack-a-mole with unaligned utf16
string handling in u-boot.  Padding bytes at end of device-path
structs won't solve everything.. that can only be done by making
unaligned accesses work.  (And hey, maybe we can trap the faults and
emulate on armv6 if someone cares enough?)  But the padding bytes will
keep things working to the extent that they work today, in an
non-obtrusive way, that is good enough for armv6/armv7 and it lets us
fix things properly on aarch64.  So that is progress.

(And I kind of agree with Leif from the other email on this thread
that efi_*install* should probably scream scary warning msgs on
BROKEN_UNALIGNED platforms.)

BR,
-R