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

[Heinrich Schuchardt]

On 08/05/2017 05:22 PM, Rob Clark wrote:
> 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;

Why do you need _packed here?

These are the current definitions (before your patches):

struct efi_device_path {
        u8 type;
        u8 sub_type;
        u16 length;
};

struct efi_mac_addr {
        u8 addr[32];
};

struct efi_device_path_mac_addr {
        struct efi_device_path dp;
        struct efi_mac_addr mac;
        u8 if_type;
};

Everything is perfectly aligned to natural bounderies.
The only thing that does not conform to the UEFI standard is the length
of efi_mac_addr, which should be 6 for if_type in {0, 1}.

If you want to copy the data to or from unaligned addresses use memcpy.

Best regards

Heinrich

> 
> 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
>