On Sun, Aug 6, 2017 at 10:17 AM, Rob Clark <[email protected]> wrote: > On Sun, Aug 6, 2017 at 9:16 AM, Mark Kettenis <[email protected]> wrote: >>> From: Rob Clark <[email protected]> >>> Date: Sat, 5 Aug 2017 11:36:25 -0400 >>> >>> On Sat, Aug 5, 2017 at 11:24 AM, Rob Clark <[email protected]> wrote: >>> > On Sat, Aug 5, 2017 at 11:10 AM, Heinrich Schuchardt <[email protected]> >>> > wrote: >>> >> On 08/05/2017 04:35 PM, Rob Clark wrote: >>> >>> On Sat, Aug 5, 2017 at 10:28 AM, Mark Kettenis >>> >>> <[email protected]> wrote: >>> >>>>> Date: Sat, 5 Aug 2017 16:01:51 +0200 (CEST) >>> >>>>> From: Mark Kettenis <[email protected]> >>> >>>>> >>> >>>>> 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 <[email protected]> >>> >>>> Date: Wed Jun 21 16:39:02 2017 -0400 >>> >>>> >>> >>>> efi: add some more device path structures >>> >>>> >>> >>>> Signed-off-by: Peter Jones <[email protected]> >>> >>>> Signed-off-by: Rob Clark <[email protected]> >>> >>> >>> >>> >>> >>> 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 [email protected] https://lists.denx.de/listinfo/u-boot
