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

[Peter Jones <pjones@redhat.com>]

On Sat, Aug 05, 2017 at 06:52:59PM +0200, Heinrich Schuchardt 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.

Well, you do need it to be packed though.  We've tried in recent years
to make sure all the fields pack naturally in new device paths types,
but unfortunately some of the ones that have been in use for more than a
decade were defined poorly; in particular, Hard Drive device paths do
not pack naturally.  In Rob's patch he's got this:

struct efi_device_path_hard_drive_path {
        struct efi_device_path dp;
        u32 partition_number;
        u64 partition_start;
        u64 partition_end;
        u8 partition_signature[16];
        u8 partmap_type;
        u8 signature_type;
        EFI_DP_PAD(1);
} __packed;

If it's not packed, there's no circumstance that partition_number and
partition_start will ever both be correctly aligned.  Without the
padding (which is questionable under the spec), the next header won't
be.  That implies any code that's given this structure will have to do
fixups when checking the length, just to traverse the device path, even
when you don't do anything with the payload data.

Here's a quick test case that shows the different ways it can be packed
wrong, as well as the right way (I wrote this before looking at Rob's
definition above, but they're materially the same):

https://pjones.fedorapeople.org/efidp_packed/dp.c
https://pjones.fedorapeople.org/efidp_packed/output

There are a couple of different ways to get partition_number correct,
but only if both the header and the payload are packed does
payload.start ever get aligned correctly.

> To the idea of padding bytes and __packed does not make sense.

Padding I'm not so sure about; if everything is packed, the compiler
should generate direct accesses correctly.  So the only cases we
actually have to worry about are when a pointer to a field >u8 is passed
some place that dereferences it.

-- 
        Peter