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

[Rob Clark]

On Sat, Aug 5, 2017 at 11:24 AM, Rob Clark <robdclark at gmail.com> wrote:
> On Sat, Aug 5, 2017 at 11:10 AM, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>> On 08/05/2017 04:35 PM, Rob Clark wrote:
>>> 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.)
>>
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.faqs/ka15414.html
>>
>> <cite>On older processors, such as ARM9 family based processors, an
>> unaligned load had to be synthesised in software.  Typically by doing a
>> series of small accesses, and combining the results. ... Unaligned
>> access support must be enabled by setting the SCTLR.A bit in the system
>> control coprocessor</cite>
>>
>> Generally packing structures is not a good idea performance-wise. The
>> sequence of fields should be carefully chosen to fill up to powers of
>> two (2, 4 , 8).
>
> Yeah, it was clearly a dumb idea for UEFI to not make device-path
> nodes word aligned.  But when implementing a standard, we don't have a
> choice but to implement it properly, warts and all :-/
>

btw, just for reference (if anyone is curious), see sect 10.3.1 in
UEFI spec v2.7.  If you don't want to bother looking it up, here is
the exact wording:

   A Device Path is a series of generic Device Path nodes. The first
   Device Path node starts at byte offset zero of the Device Path.
   The next Device Path node starts at the end of the previous Device
   Path node. Therefore all nodes are byte-packed data structures that
   may appear on any byte boundary. All code references to device path
   notes must assume all fields are unaligned. Since every Device Path
   node contains a length field in a known place, it is possible to
   traverse Device Path nodes that are of an unknown type. There is
   no limit to the number, type, or sequence of nodes in a Device Path.

So clearly what we were doing before was incorrect.. but cheating w/
extra padding bytes on arch's that cannot handle unaligned accesses
will avoid having to go fix grub, bootaa64/shim/fallback (and anything
else that uses gnu-efi), and apparently openbsd's bootaa64.efi.  It is
kinda weird to be using efi on these arch's in the first place, so I
guess I don't feel as badly about the padding bytes hack on those
arch's.  (But we should not use the hack on aarch64.)

BR,
-R