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

[Rob Clark]

On Sat, Aug 5, 2017 at 12:02 PM, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> 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?

We probably crossed threads, but see the other email I sent that
quoted the relevant part from the UEFI spec.

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

Actually, the mac is fixed size and zero padded, see 10.3.5.11.  The
only thing incorrect here was the missing __packed.

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

The problem isn't *just* u-boot.  We could do that, but it would be
annoying and make the code much more convoluted.  But that doesn't
solve the problem for grub/shim/fallback/etc.

BR,
-R