[U-Boot] [PATCH] disk:efi: avoid unaligned access on efi partition

[Albert ARIBAUD <albert.u.boot@aribaud.net>]

Hi M?ns,

On Mon, 14 Oct 2013 15:09:39 +0100, M?ns Rullg?rd <mans@mansr.com>
wrote:

> Albert ARIBAUD <albert.u.boot@aribaud.net> writes:
> 
> > Hi M?ns,
> >
> > On Mon, 14 Oct 2013 14:05:13 +0100, M?ns Rullg?rd <mans@mansr.com>
> > wrote:
> >
> >> Albert ARIBAUD <albert.u.boot@aribaud.net> writes:
> >> 
> >> >> > Please do not advise using native unaligned accesses on code that is
> >> >> > not strictly used by ARMv6+ architectures: the present code, for
> >> >> > instance, might be run on pre-ARMv6 or non-ARM platforms, and thus,
> >> >> > should never assume ability to perform unaligned accesses natively.
> >> >> 
> >> >> I'm advising no such thing.  I said two things:
> >> >> 
> >> >> 1.  Declaring a struct with the 'packed' attribute makes gcc
> >> >>     automatically generate correct code for all targets.  _IF_ the
> >> >>     selected target supports unaligned ldr/str, these might get used.
> >> >> 
> >> >> 2.  If your target is ARMv6 or later _AND_ you enable strict alignment
> >> >>     checking in the system control register, you _MUST_ build with the
> >> >>     -mno-unaligned-access flag.
> >> >
> >> > Then I apologize; I had read "Note that on ARMv6 and later ldr/str
> >> > support unaligned addresses unless this is explicitly disabled in
> >> > the system control register" as a suggestion to use that capability.
> >> 
> >> If building for ARMv6 or later, I do suggest allowing unaligned
> >> accesses.  The moment you add -march=armv6 (or equivalent), you allow
> >> for a number of things not supported by older versions, so why not
> >> unaligned memory accesses?
> >
> > doc/README.arm-unaligned-accesses probably has the answer to your
> > question, especially from line 21 onward. If not, I'll be happy to
> > provide further clarification.
> 
> That is about as backwards as it can get.  By adding -munaligned-access
> you are telling gcc that unaligned accesses are supported and welcome.
> With this setting enabled, gcc can and will generate unaligned accesses
> just about anywhere.  This setting is NOT compatible with the SCTRL.A
> bit being set (strict hardware alignment checking).
> 
> You cannot enable strict alignment checking in hardware, tell the
> compiler unaligned accesses are OK, and then expect not to have
> problems.  This is no more a valid combination than allowing
> floating-point instructions when the target has no FPU.

I sense that you have not understood the reason why I want alignment
checking enabled in ARM yet also want ARMv6+ builds to emit native
unaligned accesses if they consider it needed.

The reason is, if we prevent ARMv6 builds from using native unaligned
accesses, they would replace *all* such accesses with smaller, aligned,
ones, which would not trigger a data abort; even those unaligned
accesses cased by programming errors.

Whereas if we allow ARMv6+ builds to use native unaligned accesses, yet
enable alignment checks, then any native unaligned access will be
caught as early as possible, and we'll find and cure the issue faster.  

This is, of course, assuming we don't voluntarily do native unaligned
accesses, and in U-Boot, we indeed don't: in U-Boot, accesses are done
on natural alignments. 

Since I have set up this policy, experience (and it has been a while)
shows that very few problems arise from alignment checks + native
unaligned accesses. These roughly come from hardware- or standards-
mandated unaligned fields, in which case they are worth explicitly
accessing with "unaligned" macros, or from programming errors, which
should be fixed.

Another benefit of it is, if the code builds and runs in mainline with
alignment checks *and* native unaligned accesses enabled, then it
builds and runs also if you disable either one; whereas code that 
builds and runs with alignment checks or native unaligned accesses
disabled might fail if both are enabled.

And I don't see what we would gain by going from a strict "natural
alignment only" policy to a relaxed "unalignment allowed" one.

Amicalement,
-- 
Albert.