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

[Rob Clark]

On Tue, Aug 8, 2017 at 10:03 AM, Rob Clark  wrote:
> On Tue, Aug 8, 2017 at 9:33 AM, Mark Kettenis  wrote:
>>> From: Rob Clark 
>>> Date: Tue, 8 Aug 2017 08:54:26 -0400
>>>
>>> On Tue, Aug 8, 2017 at 8:26 AM, Mark Kettenis  
>>> wrote:
>>> >> From: Rob Clark 
>>> >> Date: Mon, 7 Aug 2017 18:18:50 -0400
>>> >>
>>> >> On Mon, Aug 7, 2017 at 5:14 PM, Mark Kettenis  
>>> >> wrote:
>>> >> >> From: Alexander Graf 
>>> >> >> Date: Mon, 7 Aug 2017 21:19:37 +0100
>>> >> >>
>>> >> >> For AArch64 things are different. There we should strive for full UEFI
>>> >> >> compliance as it's "the future" :).
>>> >> >
>>> >> > Even there just making things work would be good enough for me ;).
>>> >> > Our AArach64 bootloader is almost identical to the AArch32 one and
>>> >> > works on a real UEFI implementation as well (SoftIron Overdrive 1000).
>>> >> >
>>> >>
>>> >> I think we should make the aarch64 implementation fully compliant (ie.
>>> >> addition of missing __packed's and no extra padding bytes).  I won't
>>> >> loose sleep if some efi payloads don't work on pre-aarch64 (we should
>>> >> be able to keep things that were working before working).  If adding
>>> >> missing __packed breaks things on platforms that can't do unaligned
>>> >> access, the solution is not to remove __packed, but to conditionally
>>> >> add padding bytes at the end of the struct.  That way we keep things
>>> >> working as before on the old platforms, but make things work correctly
>>> >> on aarch64.
>>> >>
>>> >> I'll add back my patch for EFI_DP_PAD() to this patchset, since this
>>> >> seems like the sensible way forward.
>>> >
>>> > Adding padding does not magically align things on a >8-bit boundary.
>>> > And as Peter pointed out, the hard drive device path type is
>>> > inherently unalignable.  The UEFI payload simply has to deal with
>>> > that.  If grub/shim/fallback.efi is currently broken, it will have to
>>> > be fixed to be usable with U-Boot on 32-bit ARM.
>>> >
>>> > The problem that needs to be fixed is the manipulation of device path
>>> > nodes in U-Boot itself.  Using __packed and -mno-aligned-access should
>>> > take care of any parsing done through the device path node struct
>>> > types.  But whenever a member of such a struct is passed to some other
>>> > function that function should be careful not to do unaligned access.
>>> > After you fixed the GUID printing code it seems ascii2unicode() is the
>>> > only function left that does an unaligned access.  Perhaps the
>>> > solution here is to simply stop using file path nodes in U-Boot like
>>> > you already suggested.  Or otherwise just do the ascii2unicode()
>>> > conversion into a suitably aligned buffer and memcpy() that into
>>> > place.
>>> >
>>>
>>> It is also a problem with all the utf16 string handling, which shows
>>> up all over the place.  For example if efi payload (or
>>> efi_load_image()) took the file path string from a device-path, which
>>> was unaligned.  I'm not really a fan of trying to deal with this
>>> everywhere, since it is making the code much more cumbersome for the
>>> benefit of platforms that are broken (and still will be broken in ways
>>> we cannot prevent) from an EFI standpoint.
>>
>> Sorry, but I object to using the term broken here.  Alignment is a
>> rather fundamental aspect of how computers work and even on x86 you
>> can't completely ignore that aspect.  And from an EFI standard...
>
> ok, s/broken/defies expectations/ if you prefer then.  I think it
> amounts to the same thing.
>
>> The EFI standard clearly states that for Aarch32 (2.3.5 AArch32
>> Platforms):
>>
>>   Unaligned access should be enabled if supported; Alignment faults
>>   are enabled otherwise.
>
> Yes, unaligned access should be enabled if supported.. not "if the
> UEFI implementation supports it".  Presumably the "alignment faults
> are enabled otherwise" is referring to armv4 and armv5.
>
> It also says that MMU should be enabled and SCTLR.A=0 on armv6 and
> armv7 (so I guess even armv6 supports unaligned access).
>
> In that regard, I still think the term "broken" fits.
>
>> In other words, a generic Aarch32 EFI payload should expect that
>> unaligned access faults.
>>
>> The EFI standard also says (2.3.1 Data Types):
>>
>>   Unless otherwise specified all data types are naturally
>>   aligned. Structures are aligned on boundaries equal to the largest
>>   internal datum of the structure and internal data are implicitly
>>   padded to achieve natural alignment.
>>
>> The "unless otherwise specified" clearly applies to device path nodes,
>> but outside that scope utf16 strings should be 16-bit aligned.  So if
>> the file path passed to efi_load_image() isn't 16-bit aligned that is
>> a U-Boot bug.  Note that this became a bug in patch 13/20 where you
>> added __packed.  I understand that you introduced __packed to 

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

[Rob Clark]

On Tue, Aug 8, 2017 at 10:03 AM, Rob Clark  wrote:
> I suppose special case handling in efi_load_image() wouldn't be too
> bad.  It won't fix every theoretical problem on armv7 without
> unaligned access disabled.
>

s/disabled/enabled/

BR,
-R
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Rob Clark]

On Tue, Aug 8, 2017 at 9:33 AM, Mark Kettenis  wrote:
>> From: Rob Clark 
>> Date: Tue, 8 Aug 2017 08:54:26 -0400
>>
>> On Tue, Aug 8, 2017 at 8:26 AM, Mark Kettenis  
>> wrote:
>> >> From: Rob Clark 
>> >> Date: Mon, 7 Aug 2017 18:18:50 -0400
>> >>
>> >> On Mon, Aug 7, 2017 at 5:14 PM, Mark Kettenis  
>> >> wrote:
>> >> >> From: Alexander Graf 
>> >> >> Date: Mon, 7 Aug 2017 21:19:37 +0100
>> >> >>
>> >> >> For AArch64 things are different. There we should strive for full UEFI
>> >> >> compliance as it's "the future" :).
>> >> >
>> >> > Even there just making things work would be good enough for me ;).
>> >> > Our AArach64 bootloader is almost identical to the AArch32 one and
>> >> > works on a real UEFI implementation as well (SoftIron Overdrive 1000).
>> >> >
>> >>
>> >> I think we should make the aarch64 implementation fully compliant (ie.
>> >> addition of missing __packed's and no extra padding bytes).  I won't
>> >> loose sleep if some efi payloads don't work on pre-aarch64 (we should
>> >> be able to keep things that were working before working).  If adding
>> >> missing __packed breaks things on platforms that can't do unaligned
>> >> access, the solution is not to remove __packed, but to conditionally
>> >> add padding bytes at the end of the struct.  That way we keep things
>> >> working as before on the old platforms, but make things work correctly
>> >> on aarch64.
>> >>
>> >> I'll add back my patch for EFI_DP_PAD() to this patchset, since this
>> >> seems like the sensible way forward.
>> >
>> > Adding padding does not magically align things on a >8-bit boundary.
>> > And as Peter pointed out, the hard drive device path type is
>> > inherently unalignable.  The UEFI payload simply has to deal with
>> > that.  If grub/shim/fallback.efi is currently broken, it will have to
>> > be fixed to be usable with U-Boot on 32-bit ARM.
>> >
>> > The problem that needs to be fixed is the manipulation of device path
>> > nodes in U-Boot itself.  Using __packed and -mno-aligned-access should
>> > take care of any parsing done through the device path node struct
>> > types.  But whenever a member of such a struct is passed to some other
>> > function that function should be careful not to do unaligned access.
>> > After you fixed the GUID printing code it seems ascii2unicode() is the
>> > only function left that does an unaligned access.  Perhaps the
>> > solution here is to simply stop using file path nodes in U-Boot like
>> > you already suggested.  Or otherwise just do the ascii2unicode()
>> > conversion into a suitably aligned buffer and memcpy() that into
>> > place.
>> >
>>
>> It is also a problem with all the utf16 string handling, which shows
>> up all over the place.  For example if efi payload (or
>> efi_load_image()) took the file path string from a device-path, which
>> was unaligned.  I'm not really a fan of trying to deal with this
>> everywhere, since it is making the code much more cumbersome for the
>> benefit of platforms that are broken (and still will be broken in ways
>> we cannot prevent) from an EFI standpoint.
>
> Sorry, but I object to using the term broken here.  Alignment is a
> rather fundamental aspect of how computers work and even on x86 you
> can't completely ignore that aspect.  And from an EFI standard...

ok, s/broken/defies expectations/ if you prefer then.  I think it
amounts to the same thing.

> The EFI standard clearly states that for Aarch32 (2.3.5 AArch32
> Platforms):
>
>   Unaligned access should be enabled if supported; Alignment faults
>   are enabled otherwise.

Yes, unaligned access should be enabled if supported.. not "if the
UEFI implementation supports it".  Presumably the "alignment faults
are enabled otherwise" is referring to armv4 and armv5.

It also says that MMU should be enabled and SCTLR.A=0 on armv6 and
armv7 (so I guess even armv6 supports unaligned access).

In that regard, I still think the term "broken" fits.

> In other words, a generic Aarch32 EFI payload should expect that
> unaligned access faults.
>
> The EFI standard also says (2.3.1 Data Types):
>
>   Unless otherwise specified all data types are naturally
>   aligned. Structures are aligned on boundaries equal to the largest
>   internal datum of the structure and internal data are implicitly
>   padded to achieve natural alignment.
>
> The "unless otherwise specified" clearly applies to device path nodes,
> but outside that scope utf16 strings should be 16-bit aligned.  So if
> the file path passed to efi_load_image() isn't 16-bit aligned that is
> a U-Boot bug.  Note that this became a bug in patch 13/20 where you
> added __packed.  I understand that you introduced __packed to fix
> another bug, but you'll have to deal with the consequences.
>
>> So I'm pretty much NAK on playing whack-a-mole with unaligned utf16
>> string handling in u-boot.  Padding 

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

[Mark Kettenis]

> From: Rob Clark 
> Date: Tue, 8 Aug 2017 08:54:26 -0400
> 
> On Tue, Aug 8, 2017 at 8:26 AM, Mark Kettenis  wrote:
> >> From: Rob Clark 
> >> Date: Mon, 7 Aug 2017 18:18:50 -0400
> >>
> >> On Mon, Aug 7, 2017 at 5:14 PM, Mark Kettenis  
> >> wrote:
> >> >> From: Alexander Graf 
> >> >> Date: Mon, 7 Aug 2017 21:19:37 +0100
> >> >>
> >> >> For AArch64 things are different. There we should strive for full UEFI
> >> >> compliance as it's "the future" :).
> >> >
> >> > Even there just making things work would be good enough for me ;).
> >> > Our AArach64 bootloader is almost identical to the AArch32 one and
> >> > works on a real UEFI implementation as well (SoftIron Overdrive 1000).
> >> >
> >>
> >> I think we should make the aarch64 implementation fully compliant (ie.
> >> addition of missing __packed's and no extra padding bytes).  I won't
> >> loose sleep if some efi payloads don't work on pre-aarch64 (we should
> >> be able to keep things that were working before working).  If adding
> >> missing __packed breaks things on platforms that can't do unaligned
> >> access, the solution is not to remove __packed, but to conditionally
> >> add padding bytes at the end of the struct.  That way we keep things
> >> working as before on the old platforms, but make things work correctly
> >> on aarch64.
> >>
> >> I'll add back my patch for EFI_DP_PAD() to this patchset, since this
> >> seems like the sensible way forward.
> >
> > Adding padding does not magically align things on a >8-bit boundary.
> > And as Peter pointed out, the hard drive device path type is
> > inherently unalignable.  The UEFI payload simply has to deal with
> > that.  If grub/shim/fallback.efi is currently broken, it will have to
> > be fixed to be usable with U-Boot on 32-bit ARM.
> >
> > The problem that needs to be fixed is the manipulation of device path
> > nodes in U-Boot itself.  Using __packed and -mno-aligned-access should
> > take care of any parsing done through the device path node struct
> > types.  But whenever a member of such a struct is passed to some other
> > function that function should be careful not to do unaligned access.
> > After you fixed the GUID printing code it seems ascii2unicode() is the
> > only function left that does an unaligned access.  Perhaps the
> > solution here is to simply stop using file path nodes in U-Boot like
> > you already suggested.  Or otherwise just do the ascii2unicode()
> > conversion into a suitably aligned buffer and memcpy() that into
> > place.
> >
> 
> It is also a problem with all the utf16 string handling, which shows
> up all over the place.  For example if efi payload (or
> efi_load_image()) took the file path string from a device-path, which
> was unaligned.  I'm not really a fan of trying to deal with this
> everywhere, since it is making the code much more cumbersome for the
> benefit of platforms that are broken (and still will be broken in ways
> we cannot prevent) from an EFI standpoint.

Sorry, but I object to using the term broken here.  Alignment is a
rather fundamental aspect of how computers work and even on x86 you
can't completely ignore that aspect.  And from an EFI standard...

The EFI standard clearly states that for Aarch32 (2.3.5 AArch32
Platforms):

  Unaligned access should be enabled if supported; Alignment faults
  are enabled otherwise.

In other words, a generic Aarch32 EFI payload should expect that
unaligned access faults.  

The EFI standard also says (2.3.1 Data Types):

  Unless otherwise specified all data types are naturally
  aligned. Structures are aligned on boundaries equal to the largest
  internal datum of the structure and internal data are implicitly
  padded to achieve natural alignment.

The "unless otherwise specified" clearly applies to device path nodes,
but outside that scope utf16 strings should be 16-bit aligned.  So if
the file path passed to efi_load_image() isn't 16-bit aligned that is
a U-Boot bug.  Note that this became a bug in patch 13/20 where you
added __packed.  I understand that you introduced __packed to fix
another bug, but you'll have to deal with the consequences.

> So I'm pretty much NAK on playing whack-a-mole with unaligned utf16
> string handling in u-boot.  Padding bytes at end of device-path
> structs won't solve everything.. that can only be done by making
> unaligned accesses work.  (And hey, maybe we can trap the faults and
> emulate on armv6 if someone cares enough?)  But the padding bytes will
> keep things working to the extent that they work today, in an
> non-obtrusive way, that is good enough for armv6/armv7 and it lets us
> fix things properly on aarch64.  So that is progress.

Adding the padding *will not* fix anything.  It just makes the
structures bigger.  The compiler is still free to align them on an odd
boundary.

Sorry there is no way around it.  You'll need to be careful about
alignment 

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

[Rob Clark]

On Tue, Aug 8, 2017 at 8:26 AM, Mark Kettenis  wrote:
>> From: Rob Clark 
>> Date: Mon, 7 Aug 2017 18:18:50 -0400
>>
>> On Mon, Aug 7, 2017 at 5:14 PM, Mark Kettenis  
>> wrote:
>> >> From: Alexander Graf 
>> >> Date: Mon, 7 Aug 2017 21:19:37 +0100
>> >>
>> >> For AArch64 things are different. There we should strive for full UEFI
>> >> compliance as it's "the future" :).
>> >
>> > Even there just making things work would be good enough for me ;).
>> > Our AArach64 bootloader is almost identical to the AArch32 one and
>> > works on a real UEFI implementation as well (SoftIron Overdrive 1000).
>> >
>>
>> I think we should make the aarch64 implementation fully compliant (ie.
>> addition of missing __packed's and no extra padding bytes).  I won't
>> loose sleep if some efi payloads don't work on pre-aarch64 (we should
>> be able to keep things that were working before working).  If adding
>> missing __packed breaks things on platforms that can't do unaligned
>> access, the solution is not to remove __packed, but to conditionally
>> add padding bytes at the end of the struct.  That way we keep things
>> working as before on the old platforms, but make things work correctly
>> on aarch64.
>>
>> I'll add back my patch for EFI_DP_PAD() to this patchset, since this
>> seems like the sensible way forward.
>
> Adding padding does not magically align things on a >8-bit boundary.
> And as Peter pointed out, the hard drive device path type is
> inherently unalignable.  The UEFI payload simply has to deal with
> that.  If grub/shim/fallback.efi is currently broken, it will have to
> be fixed to be usable with U-Boot on 32-bit ARM.
>
> The problem that needs to be fixed is the manipulation of device path
> nodes in U-Boot itself.  Using __packed and -mno-aligned-access should
> take care of any parsing done through the device path node struct
> types.  But whenever a member of such a struct is passed to some other
> function that function should be careful not to do unaligned access.
> After you fixed the GUID printing code it seems ascii2unicode() is the
> only function left that does an unaligned access.  Perhaps the
> solution here is to simply stop using file path nodes in U-Boot like
> you already suggested.  Or otherwise just do the ascii2unicode()
> conversion into a suitably aligned buffer and memcpy() that into
> place.
>

It is also a problem with all the utf16 string handling, which shows
up all over the place.  For example if efi payload (or
efi_load_image()) took the file path string from a device-path, which
was unaligned.  I'm not really a fan of trying to deal with this
everywhere, since it is making the code much more cumbersome for the
benefit of platforms that are broken (and still will be broken in ways
we cannot prevent) from an EFI standpoint.

So I'm pretty much NAK on playing whack-a-mole with unaligned utf16
string handling in u-boot.  Padding bytes at end of device-path
structs won't solve everything.. that can only be done by making
unaligned accesses work.  (And hey, maybe we can trap the faults and
emulate on armv6 if someone cares enough?)  But the padding bytes will
keep things working to the extent that they work today, in an
non-obtrusive way, that is good enough for armv6/armv7 and it lets us
fix things properly on aarch64.  So that is progress.

(And I kind of agree with Leif from the other email on this thread
that efi_*install* should probably scream scary warning msgs on
BROKEN_UNALIGNED platforms.)

BR,
-R
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Leif Lindholm]

On Tue, Aug 08, 2017 at 08:01:10AM -0400, Rob Clark wrote:
> On Tue, Aug 8, 2017 at 7:32 AM, Leif Lindholm  
> wrote:
> > On Tue, Aug 08, 2017 at 09:11:14AM +0100, Ard Biesheuvel wrote:
> >> On 8 August 2017 at 07:52, Alexander Graf  wrote:
> >> >> Am 07.08.2017 um 23:18 schrieb Rob Clark :
> >> >>
> >> >> This is problematic around file nodes in the device path.  Adding the
> >> >> padding bytes to the end of each device-path struct would "solve"
> >> >> that, and if pre-aarch64 we are aiming at "good enough to work", I
> >> >> kinda think that this the approach we should go for.  Other than
> >> >> file-path nodes, the rest of the issues in u-boot should be solved by
> >> >> addition of missing __packed on 'struct efi_device_path' (which I've
> >> >> added locally and will be in the next revision of the patchset).
> >> >
> >> > Let's ask Leif and Ard if that is actually correct. If I remember
> >> > correctly, edk2 some times leverages automatic padding from the
> >> > compiler on structs.
> >>
> >> I guess that that might work, if U-boot is the only agent
> >> instantiating device path structures. But what do you mean by
> >> 'correct' in this context?
> >
> > Well, if that is the case, are we risking the ability to in the future
> > support loading drivers or protocols at runtime. (This would for
> > example exclude Shim compatibility or running the UEFI Shell.)
> 
> My proposal (and this is only for <=armv6 and armv7 until someone gets
> around to enabling mmu and disabling alignment faults) is to add
> padding bytes at the end of the various device-path structs to at
> least keep the structs (and things like utf16 string in file-path
> struct) aligned, and rely on efi payload and u-boot to be compiled
> with -mno-unaligned-access if it needs to access fields within the
> device-path structs.
> 
> This is *essentially* what u-boot does implicitly at the moment (by
> missing __packed attribute on certain structs).  I want to fix that on
> aarch64, but without the padding bytes it causes a some unaligned
> accesses in u-boot on armv7 devices.
>
> I think the goal for armv7 is more to have enough uefi for grub and
> OpenBSD's bootloader.  If fancy things like loading drivers and
> protocols at runtime doesn't work, well these didn't work before so I
> won't loose much sleep.  But I would like that to work properly on
> aarch64.

I'm all for the just enough approach (I just keep hoping for feature
creep). If this means certain aspects will not be supportable, what I
want is for it to be not supportable in a predictable manner.

So I guess what I'd like is that if we do this, then we either turn
the efi_*install_* functions back into just returning
EFI_OUT_OF_RESOURCES on these platforms, or worst case make them
scream bloody murder (but progress and hope for the best - like [1]).

[1] https://lists.denx.de/pipermail/u-boot/2016-January/241454.html

/
Leif
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Mark Kettenis]

> From: Rob Clark 
> Date: Mon, 7 Aug 2017 18:18:50 -0400
> 
> On Mon, Aug 7, 2017 at 5:14 PM, Mark Kettenis  wrote:
> >> From: Alexander Graf 
> >> Date: Mon, 7 Aug 2017 21:19:37 +0100
> >>
> >> On 05.08.17 21:31, Rob Clark wrote:
> >> > On Sat, Aug 5, 2017 at 4:05 PM, Heinrich Schuchardt  
> >> > wrote:
> >> >> On 08/05/2017 08:43 PM, Rob Clark wrote:
> >> >>> On Sat, Aug 5, 2017 at 1:06 PM, Rob Clark  wrote:
> >>  On Sat, Aug 5, 2017 at 12:52 PM, Heinrich Schuchardt 
> >>   wrote:
> >> > On 08/05/2017 06:16 PM, Rob Clark wrote:
> >> >> On Sat, Aug 5, 2017 at 12:12 PM, Heinrich Schuchardt 
> >> >>  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 
> >>  ---
> >>  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 1a9db4..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.
> >> >>>
> >> >>> so thinking about this a bit, I have two options in mind:
> >> >>>
> >> >>>   + efi_dp_sanitize() + efi_dp_free() helpers that are no-ops on
> >> >>> archs that can do unaligned access, but efi_dp_sanitize() always
> >> >>> allocates and copies on BROKEN_UNALIGNED archs and efi_dp_free()
> >> >>> always free's on BROKEN_UNALIGNED archs, even if the dp passed

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

[Rob Clark]

On Tue, Aug 8, 2017 at 7:32 AM, Leif Lindholm  wrote:
> On Tue, Aug 08, 2017 at 09:11:14AM +0100, Ard Biesheuvel wrote:
>> On 8 August 2017 at 07:52, Alexander Graf  wrote:
>> >
>> >
>> >> Am 07.08.2017 um 23:18 schrieb Rob Clark :
>> >>
>> >> This is problematic around file nodes in the device path.  Adding the
>> >> padding bytes to the end of each device-path struct would "solve"
>> >> that, and if pre-aarch64 we are aiming at "good enough to work", I
>> >> kinda think that this the approach we should go for.  Other than
>> >> file-path nodes, the rest of the issues in u-boot should be solved by
>> >> addition of missing __packed on 'struct efi_device_path' (which I've
>> >> added locally and will be in the next revision of the patchset).
>> >
>> > Let's ask Leif and Ard if that is actually correct. If I remember
>> > correctly, edk2 some times leverages automatic padding from the
>> > compiler on structs.
>>
>> I guess that that might work, if U-boot is the only agent
>> instantiating device path structures. But what do you mean by
>> 'correct' in this context?
>
> Well, if that is the case, are we risking the ability to in the future
> support loading drivers or protocols at runtime. (This would for
> example exclude Shim compatibility or running the UEFI Shell.)
>

My proposal (and this is only for <=armv6 and armv7 until someone gets
around to enabling mmu and disabling alignment faults) is to add
padding bytes at the end of the various device-path structs to at
least keep the structs (and things like utf16 string in file-path
struct) aligned, and rely on efi payload and u-boot to be compiled
with -mno-unaligned-access if it needs to access fields within the
device-path structs.

This is *essentially* what u-boot does implicitly at the moment (by
missing __packed attribute on certain structs).  I want to fix that on
aarch64, but without the padding bytes it causes a some unaligned
accesses in u-boot on armv7 devices.

I think the goal for armv7 is more to have enough uefi for grub and
OpenBSD's bootloader.  If fancy things like loading drivers and
protocols at runtime doesn't work, well these didn't work before so I
won't loose much sleep.  But I would like that to work properly on
aarch64.

BR,
-R
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Leif Lindholm]

On Tue, Aug 08, 2017 at 09:11:14AM +0100, Ard Biesheuvel wrote:
> On 8 August 2017 at 07:52, Alexander Graf  wrote:
> >
> >
> >> Am 07.08.2017 um 23:18 schrieb Rob Clark :
> >>
> >> On Mon, Aug 7, 2017 at 5:14 PM, Mark Kettenis  
> >> wrote:
>  From: Alexander Graf 
>  Date: Mon, 7 Aug 2017 21:19:37 +0100
> 
> > On 05.08.17 21:31, Rob Clark wrote:
> >> On Sat, Aug 5, 2017 at 4:05 PM, Heinrich Schuchardt 
> >>  wrote:
> >>> On 08/05/2017 08:43 PM, Rob Clark wrote:
>  On Sat, Aug 5, 2017 at 1:06 PM, Rob Clark  
>  wrote:
> > On Sat, Aug 5, 2017 at 12:52 PM, Heinrich Schuchardt 
> >  wrote:
> >> On 08/05/2017 06:16 PM, Rob Clark wrote:
> >>> On Sat, Aug 5, 2017 at 12:12 PM, Heinrich Schuchardt 
> >>>  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 
>  ---
>  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 1a9db4..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.
> >>>
> >>> so thinking about this a bit, I have two options in mind:
> >>>
> 

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

[Rob Clark]

On Tue, Aug 8, 2017 at 2:52 AM, Alexander Graf  wrote:
>
>
>> Am 07.08.2017 um 23:18 schrieb Rob Clark :
>>
>> On Mon, Aug 7, 2017 at 5:14 PM, Mark Kettenis  
>> wrote:
 From: Alexander Graf 
 Date: Mon, 7 Aug 2017 21:19:37 +0100

> On 05.08.17 21:31, Rob Clark wrote:
>> On Sat, Aug 5, 2017 at 4:05 PM, Heinrich Schuchardt  
>> wrote:
>>> On 08/05/2017 08:43 PM, Rob Clark wrote:
 On Sat, Aug 5, 2017 at 1:06 PM, Rob Clark  wrote:
> On Sat, Aug 5, 2017 at 12:52 PM, Heinrich Schuchardt 
>  wrote:
>> On 08/05/2017 06:16 PM, Rob Clark wrote:
>>> On Sat, Aug 5, 2017 at 12:12 PM, Heinrich Schuchardt 
>>>  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 
 ---
 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 1a9db4..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.
>>>
>>> so thinking about this a bit, I have two options in mind:
>>>
>>>  + efi_dp_sanitize() + efi_dp_free() helpers that are no-ops on
>>>archs that can do unaligned access, but efi_dp_sanitize() always
>>>allocates and copies on BROKEN_UNALIGNED archs and efi_dp_free()
>>>always free's on BROKEN_UNALIGNED archs, even if the dp passed
>>>from efi payload doesn't 

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

[Ard Biesheuvel]

On 8 August 2017 at 07:52, Alexander Graf  wrote:
>
>
>> Am 07.08.2017 um 23:18 schrieb Rob Clark :
>>
>> On Mon, Aug 7, 2017 at 5:14 PM, Mark Kettenis  
>> wrote:
 From: Alexander Graf 
 Date: Mon, 7 Aug 2017 21:19:37 +0100

> On 05.08.17 21:31, Rob Clark wrote:
>> On Sat, Aug 5, 2017 at 4:05 PM, Heinrich Schuchardt  
>> wrote:
>>> On 08/05/2017 08:43 PM, Rob Clark wrote:
 On Sat, Aug 5, 2017 at 1:06 PM, Rob Clark  wrote:
> On Sat, Aug 5, 2017 at 12:52 PM, Heinrich Schuchardt 
>  wrote:
>> On 08/05/2017 06:16 PM, Rob Clark wrote:
>>> On Sat, Aug 5, 2017 at 12:12 PM, Heinrich Schuchardt 
>>>  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 
 ---
 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 1a9db4..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.
>>>
>>> so thinking about this a bit, I have two options in mind:
>>>
>>>  + efi_dp_sanitize() + efi_dp_free() helpers that are no-ops on
>>>archs that can do unaligned access, but efi_dp_sanitize() always
>>>allocates and copies on BROKEN_UNALIGNED archs and efi_dp_free()
>>>always free's on BROKEN_UNALIGNED archs, even if the dp passed
>>>from efi payload doesn't require 

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

[Alexander Graf]

> Am 07.08.2017 um 23:18 schrieb Rob Clark :
> 
> On Mon, Aug 7, 2017 at 5:14 PM, Mark Kettenis  wrote:
>>> From: Alexander Graf 
>>> Date: Mon, 7 Aug 2017 21:19:37 +0100
>>> 
 On 05.08.17 21:31, Rob Clark wrote:
> On Sat, Aug 5, 2017 at 4:05 PM, Heinrich Schuchardt  
> wrote:
>> On 08/05/2017 08:43 PM, Rob Clark wrote:
>>> On Sat, Aug 5, 2017 at 1:06 PM, Rob Clark  wrote:
 On Sat, Aug 5, 2017 at 12:52 PM, Heinrich Schuchardt 
  wrote:
> On 08/05/2017 06:16 PM, Rob Clark wrote:
>> On Sat, Aug 5, 2017 at 12:12 PM, Heinrich Schuchardt 
>>  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 
>>> ---
>>> 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 1a9db4..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.
>> 
>> so thinking about this a bit, I have two options in mind:
>> 
>>  + efi_dp_sanitize() + efi_dp_free() helpers that are no-ops on
>>archs that can do unaligned access, but efi_dp_sanitize() always
>>allocates and copies on BROKEN_UNALIGNED archs and efi_dp_free()
>>always free's on BROKEN_UNALIGNED archs, even if the dp passed
>>from efi payload doesn't require it.
>> 
>>  + efi_dp_sanitize() that is no-op on archs that can do unaligned
>>access but only allocates/copies when passed a device path that
>>would result in 

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

[Rob Clark]

On Mon, Aug 7, 2017 at 5:14 PM, Mark Kettenis  wrote:
>> From: Alexander Graf 
>> Date: Mon, 7 Aug 2017 21:19:37 +0100
>>
>> On 05.08.17 21:31, Rob Clark wrote:
>> > On Sat, Aug 5, 2017 at 4:05 PM, Heinrich Schuchardt  
>> > wrote:
>> >> On 08/05/2017 08:43 PM, Rob Clark wrote:
>> >>> On Sat, Aug 5, 2017 at 1:06 PM, Rob Clark  wrote:
>>  On Sat, Aug 5, 2017 at 12:52 PM, Heinrich Schuchardt 
>>   wrote:
>> > On 08/05/2017 06:16 PM, Rob Clark wrote:
>> >> On Sat, Aug 5, 2017 at 12:12 PM, Heinrich Schuchardt 
>> >>  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 
>>  ---
>>  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 1a9db4..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.
>> >>>
>> >>> so thinking about this a bit, I have two options in mind:
>> >>>
>> >>>   + efi_dp_sanitize() + efi_dp_free() helpers that are no-ops on
>> >>> archs that can do unaligned access, but efi_dp_sanitize() always
>> >>> allocates and copies on BROKEN_UNALIGNED archs and efi_dp_free()
>> >>> always free's on BROKEN_UNALIGNED archs, even if the dp passed
>> >>> from efi payload doesn't require it.
>> >>>
>> >>>   + efi_dp_sanitize() that is no-op on archs that can do unaligned
>> >>> access but only allocates/copies when passed a device path that
>> >>> would result in unaligned access, plus hook some mechanism to
>> >>> auto-free on EFI_EXIT().. which is 

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

[Mark Kettenis]

> From: Alexander Graf 
> Date: Mon, 7 Aug 2017 21:19:37 +0100
> 
> On 05.08.17 21:31, Rob Clark wrote:
> > On Sat, Aug 5, 2017 at 4:05 PM, Heinrich Schuchardt  
> > wrote:
> >> On 08/05/2017 08:43 PM, Rob Clark wrote:
> >>> On Sat, Aug 5, 2017 at 1:06 PM, Rob Clark  wrote:
>  On Sat, Aug 5, 2017 at 12:52 PM, Heinrich Schuchardt 
>   wrote:
> > On 08/05/2017 06:16 PM, Rob Clark wrote:
> >> On Sat, Aug 5, 2017 at 12:12 PM, Heinrich Schuchardt 
> >>  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 
>  ---
>  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 1a9db4..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.
> >>>
> >>> so thinking about this a bit, I have two options in mind:
> >>>
> >>>   + efi_dp_sanitize() + efi_dp_free() helpers that are no-ops on
> >>> archs that can do unaligned access, but efi_dp_sanitize() always
> >>> allocates and copies on BROKEN_UNALIGNED archs and efi_dp_free()
> >>> always free's on BROKEN_UNALIGNED archs, even if the dp passed
> >>> from efi payload doesn't require it.
> >>>
> >>>   + efi_dp_sanitize() that is no-op on archs that can do unaligned
> >>> access but only allocates/copies when passed a device path that
> >>> would result in unaligned access, plus hook some mechanism to
> >>> auto-free on EFI_EXIT().. which is slightly tricky w/ nesting of
> >>> efi calls, but not impossible and at least avoids the problem
> >>> of missing calls to free the dup'd device-path.. which is the
> >>> sort of 

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

[Alexander Graf]

On 05.08.17 21:31, Rob Clark wrote:

On Sat, Aug 5, 2017 at 4:05 PM, Heinrich Schuchardt  wrote:

On 08/05/2017 08:43 PM, Rob Clark wrote:

On Sat, Aug 5, 2017 at 1:06 PM, Rob Clark  wrote:

On Sat, Aug 5, 2017 at 12:52 PM, Heinrich Schuchardt  wrote:

On 08/05/2017 06:16 PM, Rob Clark wrote:

On Sat, Aug 5, 2017 at 12:12 PM, Heinrich Schuchardt  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 
---
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 1a9db4..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.


so thinking about this a bit, I have two options in mind:

  + efi_dp_sanitize() + efi_dp_free() helpers that are no-ops on
archs that can do unaligned access, but efi_dp_sanitize() always
allocates and copies on BROKEN_UNALIGNED archs and efi_dp_free()
always free's on BROKEN_UNALIGNED archs, even if the dp passed
from efi payload doesn't require it.

  + efi_dp_sanitize() that is no-op on archs that can do unaligned
access but only allocates/copies when passed a device path that
would result in unaligned access, plus hook some mechanism to
auto-free on EFI_EXIT().. which is slightly tricky w/ nesting of
efi calls, but not impossible and at least avoids the problem
of missing calls to free the dup'd device-path.. which is the
sort of leak I might miss since I'm not using EFI_LOADER on any
BROKEN_UNALIGNED arch

anyone who cares about armv7 + efi have a preference between always
doing an extra alloc+copy+free vs EFI_EXIT() magic?



Please, go for the simplest code.


well, the source of my question was there are two definitions of
"simplest" here, ie "simplest to use" and "simplest helpers".. I'm
mostly worried about future patches that use the helpers introducing
leaks.  But actually I think a reasonable middle ground is "simplest
helpers plus always make the helpers not no-ops for DEBUG builds"..


I cannot imagine that copying takes more than 10ms for starting grub on
any architecture. So the user will not notice it anyway.
Assume every architecture requiring alignment for which you do not have
proof of the contrary.

Even on ARM64 there are op codes that fail without alignment.


iirc the restrictions where mostly about device memory, and atomics.
Which 

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

[Peter Jones]

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  
> > 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 
> >>> ---
> >>> 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 1a9db4..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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Rob Clark]

On Sat, Aug 5, 2017 at 1:06 PM, Rob Clark  wrote:
> On Sat, Aug 5, 2017 at 12:52 PM, Heinrich Schuchardt  
> wrote:
>> On 08/05/2017 06:16 PM, Rob Clark wrote:
>>> On Sat, Aug 5, 2017 at 12:12 PM, Heinrich Schuchardt  
>>> 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 
> ---
> 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 1a9db4..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 

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

[Rob Clark]

On Sat, Aug 5, 2017 at 4:05 PM, Heinrich Schuchardt  wrote:
> On 08/05/2017 08:43 PM, Rob Clark wrote:
>> On Sat, Aug 5, 2017 at 1:06 PM, Rob Clark  wrote:
>>> On Sat, Aug 5, 2017 at 12:52 PM, Heinrich Schuchardt  
>>> wrote:
 On 08/05/2017 06:16 PM, Rob Clark wrote:
> On Sat, Aug 5, 2017 at 12:12 PM, Heinrich Schuchardt  
> 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 
>>> ---
>>> 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 1a9db4..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.
>>
>> so thinking about this a bit, I have two options in mind:
>>
>>  + efi_dp_sanitize() + efi_dp_free() helpers that are no-ops on
>>archs that can do unaligned access, but efi_dp_sanitize() always
>>allocates and copies on BROKEN_UNALIGNED archs and efi_dp_free()
>>always free's on BROKEN_UNALIGNED archs, even if the dp passed
>>from efi payload doesn't require it.
>>
>>  + efi_dp_sanitize() that is no-op on archs that can do unaligned
>>access but only allocates/copies when passed a device path that
>>would result in unaligned access, plus hook some mechanism to
>>auto-free on EFI_EXIT().. which is slightly tricky w/ nesting of
>>efi calls, but not impossible and at least avoids the problem
>>of missing calls to free the dup'd device-path.. which is the
>>sort of leak I might miss since I'm not using EFI_LOADER on any
>>BROKEN_UNALIGNED arch
>>
>> anyone who cares about armv7 + efi have a preference between always
>> doing an extra alloc+copy+free vs EFI_EXIT() magic?
>>
>
> Please, go for the simplest code.

well, the source of my question was there are two definitions of
"simplest" here, ie "simplest to use" and "simplest helpers".. I'm
mostly worried about future patches that use the helpers 

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

[Heinrich Schuchardt]

On 08/05/2017 08:43 PM, Rob Clark wrote:
> On Sat, Aug 5, 2017 at 1:06 PM, Rob Clark  wrote:
>> On Sat, Aug 5, 2017 at 12:52 PM, Heinrich Schuchardt  
>> wrote:
>>> On 08/05/2017 06:16 PM, Rob Clark wrote:
 On Sat, Aug 5, 2017 at 12:12 PM, Heinrich Schuchardt  
 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 
>> ---
>> 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 1a9db4..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.
> 
> so thinking about this a bit, I have two options in mind:
> 
>  + efi_dp_sanitize() + efi_dp_free() helpers that are no-ops on
>archs that can do unaligned access, but efi_dp_sanitize() always
>allocates and copies on BROKEN_UNALIGNED archs and efi_dp_free()
>always free's on BROKEN_UNALIGNED archs, even if the dp passed
>from efi payload doesn't require it.
> 
>  + efi_dp_sanitize() that is no-op on archs that can do unaligned
>access but only allocates/copies when passed a device path that
>would result in unaligned access, plus hook some mechanism to
>auto-free on EFI_EXIT().. which is slightly tricky w/ nesting of
>efi calls, but not impossible and at least avoids the problem
>of missing calls to free the dup'd device-path.. which is the
>sort of leak I might miss since I'm not using EFI_LOADER on any
>BROKEN_UNALIGNED arch
> 
> anyone who cares about armv7 + efi have a preference between always
> doing an extra alloc+copy+free vs EFI_EXIT() magic?
> 
> BR,
> -R
> 
> 
>> 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 

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

[Rob Clark]

On Sat, Aug 5, 2017 at 1:06 PM, Rob Clark  wrote:
> On Sat, Aug 5, 2017 at 12:52 PM, Heinrich Schuchardt  
> wrote:
>> On 08/05/2017 06:16 PM, Rob Clark wrote:
>>> On Sat, Aug 5, 2017 at 12:12 PM, Heinrich Schuchardt  
>>> 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 
> ---
> 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 1a9db4..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.

so thinking about this a bit, I have two options in mind:

 + efi_dp_sanitize() + efi_dp_free() helpers that are no-ops on
   archs that can do unaligned access, but efi_dp_sanitize() always
   allocates and copies on BROKEN_UNALIGNED archs and efi_dp_free()
   always free's on BROKEN_UNALIGNED archs, even if the dp passed
   from efi payload doesn't require it.

 + efi_dp_sanitize() that is no-op on archs that can do unaligned
   access but only allocates/copies when passed a device path that
   would result in unaligned access, plus hook some mechanism to
   auto-free on EFI_EXIT().. which is slightly tricky w/ nesting of
   efi calls, but not impossible and at least avoids the problem
   of missing calls to free the dup'd device-path.. which is the
   sort of leak I might miss since I'm not using EFI_LOADER on any
   BROKEN_UNALIGNED arch

anyone who cares about armv7 + efi have a preference between always
doing an extra alloc+copy+free vs EFI_EXIT() magic?

BR,
-R


> 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 

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

[Rob Clark]

On Sat, Aug 5, 2017 at 12:52 PM, Heinrich Schuchardt  wrote:
> On 08/05/2017 06:16 PM, Rob Clark wrote:
>> On Sat, Aug 5, 2017 at 12:12 PM, Heinrich Schuchardt  
>> 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 
 ---
 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 1a9db4..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.

BR,
-R

> Best regards
>
> Heinrich
>
>>
>>
>>> 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 

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

[Heinrich Schuchardt]

On 08/05/2017 06:16 PM, Rob Clark wrote:
> On Sat, Aug 5, 2017 at 12:12 PM, Heinrich Schuchardt  
> 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 
>>> ---
>>> 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 1a9db4..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.

Best regards

Heinrich

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

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

[Rob Clark]

On Sat, Aug 5, 2017 at 12:26 PM, Heinrich Schuchardt  wrote:
> On 08/05/2017 06:19 PM, Rob Clark wrote:
>> On Sat, Aug 5, 2017 at 12:16 PM, Rob Clark  wrote:
>>> On Sat, Aug 5, 2017 at 12:12 PM, Heinrich Schuchardt  
>>> 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 
> ---
> 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 1a9db4..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.
>>
>
> In which UEFI protocol do you need the packed structures?
> Why can't you use memcpy to provide/read the data in these protocols?
> Why can't you use padding on all architectures?


The device-path protocol, as described in section 10.3 in the UEFI
spec.  And as I already explained, the problem isn't just u-boot but
also all the various efi payloads loaded and executed directly or
indirectly by u-boot.  Please feel free to send patches to these
various projects to fix those problems on armv7/etc.  I guess it will
take a while to get them all upstream, if they are accepted (really,
*why* are we using efi on these archs?) and trickle out into distros.

I'll repeat the section I previously quoted from 10.3.1 in the other
email thread:

   A Device Path is a series of generic Device Path nodes. The first
   Device Path node starts at byte offset zero of the Device Path.
   The next Device Path node starts at the end of the previous Device
   Path node. Therefore all nodes are byte-packed data structures that
   may appear on any byte boundary. All code references to device path
   notes must assume all fields are unaligned. Since every Device Path
   node contains a length field in a known place, it is possible to
   traverse Device Path nodes that are of an unknown type. There is
   no limit to the number, type, or sequence of nodes in a Device Path.

What we were doing before was incorrect.  Sorry, but if we are
implementing the UEFI spec, we need to implement the UEFI spec, not
what we wish the UEFI spec was.  I fixed the bug in u-boot, and that
exposed the problem in openbsd's bootaa64.efi.  But I've spent enough
time looking at shim/fallback/grub to say that they probably have the
same problem on armv7 (and any other arch's that cannot do unaligned
access).

This patch allows us to not break stuff that currently works by
accident on these archs, but do things correctly on aarch64 and archs
that *can* do unaligned access.  This is better than doing things
incorrectly on aarch64, and better than breaking archs 

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

[Heinrich Schuchardt]

On 08/05/2017 06:19 PM, Rob Clark wrote:
> On Sat, Aug 5, 2017 at 12:16 PM, Rob Clark  wrote:
>> On Sat, Aug 5, 2017 at 12:12 PM, Heinrich Schuchardt  
>> 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 
 ---
 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 1a9db4..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.
> 

In which UEFI protocol do you need the packed structures?
Why can't you use memcpy to provide/read the data in these protocols?
Why can't you use padding on all architectures?

Best regards

Heinrich

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

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

[Rob Clark]

On Sat, Aug 5, 2017 at 12:16 PM, Rob Clark  wrote:
> On Sat, Aug 5, 2017 at 12:12 PM, Heinrich Schuchardt  
> 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 
>>> ---
>>> 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 1a9db4..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 

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

[Rob Clark]

On Sat, Aug 5, 2017 at 12:12 PM, Heinrich Schuchardt  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 
>> ---
>> 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 1a9db4..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


> 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_DEVICE0x04
>> @@ -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;
>>
>>  

Re: [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 
> ---
> 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 1a9db4..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_DEVICE0x04
> @@ -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,
>  
>   

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

[Rob Clark]

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 
---
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 1a9db4..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
 
 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));
-- 
2.13.0

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot