On Sat, Aug 5, 2017 at 12:02 PM, Heinrich Schuchardt xypron.glpk@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.kettenis@xs4all.nl wrote:
From: Rob Clark robdclark@gmail.com Date: Sat, 5 Aug 2017 10:35:08 -0400
On Sat, Aug 5, 2017 at 10:28 AM, Mark Kettenis mark.kettenis@xs4all.nl wrote:
Date: Sat, 5 Aug 2017 16:01:51 +0200 (CEST) From: Mark Kettenis mark.kettenis@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@redhat.com Date: Wed Jun 21 16:39:02 2017 -0400
efi: add some more device path structures Signed-off-by: Peter Jones <pjones@redhat.com> Signed-off-by: Rob Clark <robdclark@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