On Sat, Aug 5, 2017 at 1:06 PM, Rob Clark robdclark@gmail.com wrote:
On Sat, Aug 5, 2017 at 12:52 PM, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 08/05/2017 06:16 PM, Rob Clark wrote:
On Sat, Aug 5, 2017 at 12:12 PM, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 08/05/2017 05:58 PM, Rob Clark wrote:
Some arch's have trouble with unaligned accesses. Technically EFI device-path structs should be byte aligned, and the next node in the path starts immediately after the previous. Meaning that a pointer to an 'struct efi_device_path' is not necessarily word aligned. See section 10.3.1 in v2.7 of UEFI spec.
This causes problems not just for u-boot, but also most/all EFI payloads loaded by u-boot on these archs. Fortunately the common practice for traversing a device path is to rely on the length field in the header, rather than the specified length of the particular device path type+subtype. So the EFI_DP_PAD() macro will add the specified number of bytes to the tail of device path structs to pad them to word alignment.
Technically this is non-compliant, BROKEN_UNALIGNED should *only* be defined on archs that cannot do unaligned accesses.
Signed-off-by: Rob Clark robdclark@gmail.com
I'm not sure if there are other arch's that need -DBROKEN_UNALIGNED
Mark, this is untested but I think it should solve your crash on the Banana Pi. Could you give it a try when you get a chance?
arch/arm/config.mk | 2 +- include/efi_api.h | 30 ++++++++++++++++++++++++++++++ lib/efi_loader/efi_device_path.c | 3 +++ 3 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/arch/arm/config.mk b/arch/arm/config.mk index 1a77779db4..067dc93a9d 100644 --- a/arch/arm/config.mk +++ b/arch/arm/config.mk @@ -28,7 +28,7 @@ LLVMS_RELFLAGS := $(call cc-option,-mllvm,) \ $(call cc-option,-arm-use-movt=0,) PLATFORM_RELFLAGS += $(LLVM_RELFLAGS)
-PLATFORM_CPPFLAGS += -D__ARM__ +PLATFORM_CPPFLAGS += -D__ARM__ -DBROKEN_UNALIGNED
NAK
We have more then ARM. And other architectures also create exceptions for unaligned access.
I hate platform specific code. It should not be used outside /arch.
To play it save you should not use _packed at all! Use memcpy to transfer between aligned and unaligned memory.
except for reasons I explained in the thread on the patch that added the __packed in the first place. Sorry, this is ugly but we have to do it.
BR, -R
According to the UEFI standard the nodes in paths are not to be assumed to be aligned.
So even if you use padding bytes in paths that you pass to the EFI application you should not expect that the EFI application does the same. Expect the EFI application either to remove them or send new nodes without padding.
To the idea of padding bytes and __packed does not make sense.
Ok, to be fair, you are right about device-paths passed too u-boot. On BROKEN_UNALIGNED archs, we should sanitise with a copy to an aligned device-path in *addition* to what I proposed. I can make a patch to add a helper to do this a bit later.
But the padding bytes + __packed does make total sense because it avoids breaking efi payloads that already exist in the field. The crash that Mark saw was not in u-boot, but in openbsd's bootaa64.efi. (It is possibly that we just get lucky here in u-boot since I add the /End node after the mac address by something the compiler turns into a memcpy.)
My proposal simply preserves the bug that we already have on BROKEN_UNALIGNED archs (but makes the improvement that it fixes the bug on aarch64 and any other arch that can do unaligned access), to keep existing efi payloads working.
Ok, so I took a closer look at the assembly generated, and I realized that with __packed structs, gcc seems to be generating ldrb+orr's for *all* the fields, in other words it isn't assuming alignment of the device-path pointer. The only potential problem right now is that we are missing __packed on 'struct efi_device_path' itself, so dereferencing the length field could cause unaligned access. Adding the missing __packed annotation turns the generated code into a pair of ldrb's plus an orr. I'll add the missing __packed on efi_device_path to my patchset.
(I'm basing this on the asm generated for vexpress_ca15_tc2 build.)
That is the good news.. the bad news is this probably still ends up being a problem in a few places w/ utf16 strings (ie. ascii2unicode()'ing into a filepath node, or converting filenames passed from efi payload from utf16..).. I'll have to think a bit about how best to handle that. But at least this is all stuff that never worked before in the first place, so I guess we don't have to solve it immediately.
BR, -R