On Sat, Aug 5, 2017 at 12:02 PM, Heinrich Schuchardt <xypron.g...@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.kette...@xs4all.nl> >> wrote: >>>> From: Rob Clark <robdcl...@gmail.com> >>>> Date: Sat, 5 Aug 2017 10:35:08 -0400 >>>> >>>> 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.) >>> >>> 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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot