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

Tue Aug 8 18:20:51 UTC 2017

On Tue, Aug 8, 2017 at 10:03 AM, Rob Clark <robdclark at gmail.com> wrote:
> On Tue, Aug 8, 2017 at 9:33 AM, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
>>> From: Rob Clark <robdclark at gmail.com>
>>> Date: Tue, 8 Aug 2017 08:54:26 -0400
>>>
>>> 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.
>>
>> Sorry, but I object to using the term broken here.  Alignment is a
>> rather fundamental aspect of how computers work and even on x86 you
>> can't completely ignore that aspect.  And from an EFI standard...
>
> ok, s/broken/defies expectations/ if you prefer then.  I think it
> amounts to the same thing.
>
>> The EFI standard clearly states that for Aarch32 (2.3.5 AArch32
>> Platforms):
>>
>>   Unaligned access should be enabled if supported; Alignment faults
>>   are enabled otherwise.
>
> Yes, unaligned access should be enabled if supported.. not "if the
> UEFI implementation supports it".  Presumably the "alignment faults
> are enabled otherwise" is referring to armv4 and armv5.
>
> It also says that MMU should be enabled and SCTLR.A=0 on armv6 and
> armv7 (so I guess even armv6 supports unaligned access).
>
> In that regard, I still think the term "broken" fits.
>
>> In other words, a generic Aarch32 EFI payload should expect that
>> unaligned access faults.
>>
>> The EFI standard also says (2.3.1 Data Types):
>>
>>   Unless otherwise specified all data types are naturally
>>   aligned. Structures are aligned on boundaries equal to the largest
>>   internal datum of the structure and internal data are implicitly
>>   padded to achieve natural alignment.
>>
>> The "unless otherwise specified" clearly applies to device path nodes,
>> but outside that scope utf16 strings should be 16-bit aligned.  So if
>> the file path passed to efi_load_image() isn't 16-bit aligned that is
>> a U-Boot bug.  Note that this became a bug in patch 13/20 where you
>> added __packed.  I understand that you introduced __packed to fix
>> another bug, but you'll have to deal with the consequences.
>>
>>> 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.
>>
>> Adding the padding *will not* fix anything.  It just makes the
>> structures bigger.  The compiler is still free to align them on an odd
>> boundary.
>
> It will "fix" what was "broken" by adding __packed ;-)
>
> It makes the structures bigger to align the next structure to a word
> boundary, which *mostly* aligns fields within the structure to their
> natural alignment.
>
> I agree it isn't really a fix.  It's a workaround.  But it has the
> consequence that utf16 strings in a device-path will be naturally
> aligned, in case they get passed back into some other efi api.  (And
> that also applies to USB WWID nodes, we we aren't using for anything
> else yet.. from a quick look I don't see anywhere else where utf16
> strings are used in a device-path.)
>
>> Sorry there is no way around it.  You'll need to be careful about
>> alignment anywhere where you convert data into and out of device
>> paths.  But you certainly don't need to check every place where utf16
>> strings are used.  The compiler will make sure things will be
>> naturally aligned unless you explicitly tell it not to do that.
>
> I'm worried about the case where efi payload takes a utf16 path out of
> a device-path and passes it back to file->open().
>
> I suppose special case handling in efi_load_image() wouldn't be too
> bad.  It won't fix every theoretical problem on armv7 without
> unaligned access enabled.
>

Since I don't have an armv7 board w/ u-boot to test, I've done some
experiments w/ SCTLR.A=1 on armv8:

GRUB:

Note, by default built w/out -mstrict-align on aarch64, which crashes.
With -mstrict-align, it seems to work with or without extra padding.

Kernel does crash relatively early, I guess not expecting to have
unaligned accesses disabled.

SHIM/FALLBACK:

Both seem to crash in both shim and fallback with and without padding.
Note that shim and fallback do not seem to ever be built with
-mstrict-align, which is probably the issue.  So I wouldn't expect
these to work on armv7 right now without at least recompiling.

fallback.efi is the one where I half suspect we'll see issues if there
are any, but without my patchset it won't run on u-boot at all, and
with my patchset it will run on aarch64 (but not armv7, at least until
it is recompiled).  So I'm tempted not to care at this point.

OpenBSD bootloader:

Works both with and without padding, at least until the point where it
can't find an OpenBSD filesystem and then sits there at boot prompt.

SUMMARY:

Mark's initial problem wasn't even with device-paths, but was with
guidstr(), iirc.  Which is superceded by the patch adding vsprintf
support for %pG (which works bytewise, so it doesn't have the same
problem as guidstr).

Beyond that, right now the only place we construct a file-path (and
call ascii2unicode()) is the EFI_LOADED_IMAGE::FilePath (which is not
preceded by the "device" part of a device-path, so it is already word
aligned).

So tl;dr: we can skip the padding for now, it doesn't fix anything
enough to go from broken to working.  And without it nothing that was
working before stops working.

BR,
-R