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 BR, -R _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot