[U-Boot] [PATCH v0 13/20] efi_loader: use proper device-paths for partitions

[Rob Clark]

On Sat, Aug 5, 2017 at 11:08 AM, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
>> From: Rob Clark <robdclark at gmail.com>
>> Date: Sat, 5 Aug 2017 10:35:08 -0400
>>
>> On Sat, Aug 5, 2017 at 10:28 AM, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
>> >> Date: Sat, 5 Aug 2017 16:01:51 +0200 (CEST)
>> >> From: Mark Kettenis <mark.kettenis at xs4all.nl>
>> >>
>> >> Unfortunately something in this patch series breaks things for me on a
>> >> Banana Pi:
>> >
>> > And according to git bisect:
>> >
>> > 4e3e748a50fc3f43e20c7ff407184596d7c9a589 is the first bad commit
>> > commit 4e3e748a50fc3f43e20c7ff407184596d7c9a589
>> > Author: Peter Jones <pjones at redhat.com>
>> > Date:   Wed Jun 21 16:39:02 2017 -0400
>> >
>> >     efi: add some more device path structures
>> >
>> >     Signed-off-by: Peter Jones <pjones at redhat.com>
>> >     Signed-off-by: Rob Clark <robdclark at gmail.com>
>>
>>
>> hmm, odd.. it is only adding some #define's and structs that are not
>> used until a later commit..
>>
>> although it does also make 'struct efi_device_path_mac_addr' __packed,
>> which it should have been before.  Is this an armv7?  I wonder if we
>> have some troubles with unaligned accesses on armv7 that we don't have
>> on aarch64 (or maybe compiler isn't turning access to device-path
>> nodes into byte accesses if it can't do unaligned accesses.  (The node
>> in the device-path structure are byte-packed.)
>
> This is indeed armv7.
>
>> addr2line the faulting address I guess should confirm that.  If this
>> is the issue, it's going to be a bit sad since we'll have to do a lot
>> of copying back/forth of efi_device_path ptrs to aligned addresses :-/
>
> Sadly that's not going to help you:
>
>   $ arm-none-eabi-addr2line -e u-boot 7ef8d878
>   ??:0
>
> I suppose it is faulting somewhere in BOOTARM.EFI,
>
> Anyway, removing __packed from struct efi_device_path_file_path makes
> me boot again with a tree checked out out with that commit.
>
> Our bootloader code doesn't explicitly enable alignment faults.  But
> of course the UEFI standard says that for AArc32 platforms:
>
>  * Unaligned access should be enabled if supported; Alignment faults
>     are enabled otherwise.
>
> So the efi_loader code has to align things properly I fear.

Ok, so I have an idea for a reasonably (imho) sane way forward:

  struct efi_device_path_mac_addr {
      struct efi_device_path dp;
      struct efi_mac_addr mac;
      u8 if_type;
  #ifdef BROKEN_UNALIGNED
      u8 pad[3];
  #endif
  } __packed;

We'll just define BROKEN_UNALIGNED for armv7 and any other arch's that
can't handle unaligned accesses.  Technically it is a bit outside the
way things are *supposed* to work according to my understanding of the
UEFI spec.  But all the code I've seen that parses the device-paths
honors the length field in the efi_device_path header to find the
start of the next node in the path.  I can't guarantee that you'll be
able to boot windows from u-boot (but I guess that isn't what most
people care about ;-)), but it at least won't be more broken than it
was before on these archs.

It will take a bit of extra special handling for
efi_device_path_file_path (which is variable length) but with my
patchset that only gets constructed in one place, so it isn't so bad.

BR,
-R