On Sun, Aug 6, 2017 at 10:17 AM, Rob Clark <robdcl...@gmail.com> wrote:
> On Sun, Aug 6, 2017 at 9:16 AM, Mark Kettenis <mark.kette...@xs4all.nl> wrote:
>>> From: Rob Clark <robdcl...@gmail.com>
>>> Date: Sat, 5 Aug 2017 11:36:25 -0400
>>>
>>> On Sat, Aug 5, 2017 at 11:24 AM, Rob Clark <robdcl...@gmail.com> wrote:
>>> > On Sat, Aug 5, 2017 at 11:10 AM, Heinrich Schuchardt <xypron.g...@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.kette...@xs4all.nl> wrote:
>>> >>>>> Date: Sat, 5 Aug 2017 16:01:51 +0200 (CEST)
>>> >>>>> From: Mark Kettenis <mark.kette...@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 <pjo...@redhat.com>
>>> >>>> Date:   Wed Jun 21 16:39:02 2017 -0400
>>> >>>>
>>> >>>>     efi: add some more device path structures
>>> >>>>
>>> >>>>     Signed-off-by: Peter Jones <pjo...@redhat.com>
>>> >>>>     Signed-off-by: Rob Clark <robdcl...@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.)
>>
>> Looking a bit more closely at the OpenBSD efiboot code, I'm pretty
>> sure we handle the parsing of device path nodes correctly.  We use an
>> incarnation of the Intel EFI header files which have:
>>
>> typedef struct _EFI_DEVICE_PATH {
>>         UINT8                           Type;
>>         UINT8                           SubType;
>>         UINT8                           Length[2];
>> } EFI_DEVICE_PATH;
>>
>> #define DevicePathNodeLength(a)     ( ((a)->Length[0]) | ((a)->Length[1] << 
>> 8) )
>>
>> so this is all done using byte access.
>
> Hmm, I assume the OpenBSD efiboot code does look at the payload of
> device-path nodes, like HARDDRIVE_DEVICE_PATH.. which does use u32 and
> u64 fields (which would be unaligned).  Although that might not be the
> problem here.
>
>> Going back to the original crash report:
>>
>> data abort
>> pc : [<7ef8d878>]          lr : [<7ef8d874>]
>> reloc pc : [<4a039878>]    lr : [<4a039874>]
>> sp : 7af29660  ip : 00000000     fp : 7af29774
>> r10: 7efec230  r9 : 7af33ee0     r8 : 00000000
>> r7 : 00000009  r6 : 7ef9e8b8     r5 : 7af296a0  r4 : 7efa4495
>> r3 : 7af296a0  r2 : 0000008c     r1 : 7af29658  r0 : 00000004
>> Flags: nzCV  IRQs off  FIQs off  Mode SVC_32
>>
>> I think it is actually "reloc pc" instead of "pc" that we need to look
>> at:
>>
>> $ addr2line -e u-boot 0x4a039874
>> /home/kettenis/tmp/rclark/u-boot/include/efi_loader.h:272
>>
>> which points at the guidstr() function.  That code certainly looks
>> suspicious.  It will defenitely trigger alignment faults if the guid
>> isn't 32-bit aligned.
>
> hmm, interesting.  At least gnu-efi's EFI_GUID uses the same
> u32+u16+u16+u8[8] layout.  And iirc so did linux kernel and grub, so
> it seemed like u-boot was the odd one out for using u8[16].  Although
> maybe we are printing one of our own guid's or openbsd efiboot is also
> using u8[16].
>
> Either way I've dropped this patch and instead added %pG support to
> vsprintf, using uuid_bin_to_str() which only does byte accesses.
>
> The latest can be found here:
>
>   https://github.com/robclark/u-boot/commits/enough-uefi-for-shim
>
>   https://github.com/robclark/u-boot.git enough-uefi-for-shim
>
>
>> The relevant instruction is a 16-bit load:
>>
>>   4a039878:       e1d430b4        ldrh    r3, [r4, #4]
>>
>> and with r4 = 7efa4495 that will defenitely trap.
>>
>> Looking at the defenition efi_guid_t in u-boot:
>>
>>   typedef struct {
>>           u8 b[16];
>>   } efi_guid_t;
>>
>> there is no guarantee that GUIDs are properly aligned, so you'll need
>> to fix the guidstr function introduced in commit
>> b6d913c6101ba891eb2bcb08a4ee9fc8fb57367.
>>
>> Things are already broken before that commit though, so there is
>> another problem.  I'll see if I can figure out what it is...
>
> Thanks
>

btw, we do have some travis tests that run grub.efi (in qemu) on armv7
and others.. maybe adding OpenBSD's efiboot to the test suit would be
a good idea?   (And eventually shim+fallback.efi after this patchset
is merged..)

BR,
-R
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Reply via email to