[U-Boot] [PATCH v0 21/20] efi_loader: hack for archs that cannot do unaligned accesses
Rob Clark
robdclark at gmail.com
Sat Aug 5 16:19:04 UTC 2017
On Sat, Aug 5, 2017 at 12:16 PM, Rob Clark <robdclark at gmail.com> wrote:
> On Sat, Aug 5, 2017 at 12:12 PM, Heinrich Schuchardt <xypron.glpk at 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 at 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.
well, to be fair, we don't *have* to do it. The alternative is
disable EFI_LOADER on archs that cannot do unaligned accesses. But
this seemed like the better option.
BR,
-R
> BR,
> -R
>
>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> ifdef CONFIG_ARM64
>>> PLATFORM_ELFFLAGS += -B aarch64 -O elf64-littleaarch64
>>> diff --git a/include/efi_api.h b/include/efi_api.h
>>> index ef91e34c7b..ddd1e6100a 100644
>>> --- a/include/efi_api.h
>>> +++ b/include/efi_api.h
>>> @@ -284,6 +284,31 @@ struct efi_loaded_image {
>>> #define DEVICE_PATH_TYPE_END 0x7f
>>> # define DEVICE_PATH_SUB_TYPE_END 0xff
>>>
>>> +/*
>>> + * 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.
>>> + */
>>> +
>>> +#ifdef BROKEN_UNALIGNED
>>> +# define EFI_DP_PAD(n) u8 __pad[n]
>>> +#else
>>> +# define EFI_DP_PAD(n)
>>> +#endif
>>> +
>>> struct efi_device_path {
>>> u8 type;
>>> u8 sub_type;
>>> @@ -318,12 +343,14 @@ struct efi_device_path_usb {
>>> struct efi_device_path dp;
>>> u8 parent_port_number;
>>> u8 usb_interface;
>>> + EFI_DP_PAD(2);
>>> } __packed;
>>>
>>> struct efi_device_path_mac_addr {
>>> struct efi_device_path dp;
>>> struct efi_mac_addr mac;
>>> u8 if_type;
>>> + EFI_DP_PAD(3);
>>> } __packed;
>>>
>>> struct efi_device_path_usb_class {
>>> @@ -333,11 +360,13 @@ struct efi_device_path_usb_class {
>>> u8 device_class;
>>> u8 device_subclass;
>>> u8 device_protocol;
>>> + EFI_DP_PAD(1);
>>> } __packed;
>>>
>>> struct efi_device_path_sd_mmc_path {
>>> struct efi_device_path dp;
>>> u8 slot_number;
>>> + EFI_DP_PAD(3);
>>> } __packed;
>>>
>>> #define DEVICE_PATH_TYPE_MEDIA_DEVICE 0x04
>>> @@ -353,6 +382,7 @@ struct efi_device_path_hard_drive_path {
>>> u8 partition_signature[16];
>>> u8 partmap_type;
>>> u8 signature_type;
>>> + EFI_DP_PAD(1);
>>> } __packed;
>>>
>>> struct efi_device_path_cdrom_path {
>>> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
>>> index b5acf73f98..515a1f4737 100644
>>> --- a/lib/efi_loader/efi_device_path.c
>>> +++ b/lib/efi_loader/efi_device_path.c
>>> @@ -402,6 +402,9 @@ struct efi_device_path *efi_dp_from_file(struct blk_desc *desc, int part,
>>>
>>> // TODO efi_device_path_file_path should be variable length:
>>> fpsize = sizeof(struct efi_device_path) + 2 * (strlen(path) + 1);
>>> +#ifdef BROKEN_UNALIGNED
>>> + fpsize = ALIGN(fpsize, 4);
>>> +#endif
>>> dpsize += fpsize;
>>>
>>> start = buf = calloc(1, dpsize + sizeof(END));
>>>
>>
More information about the U-Boot
mailing list