[U-Boot] [PATCH v0 21/20] efi_loader: hack for archs that cannot do unaligned accesses

[Heinrich Schuchardt]

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.

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));
>