[U-Boot] [PATCH v2] arm: Switch to -mno-unaligned-access when supported by the compiler

[Albert ARIBAUD]

Hi Tom,

On Mon, 10 Feb 2014 11:24:03 -0500, Tom Rini <trini at ti.com> wrote:

> On Mon, Feb 10, 2014 at 05:12:24PM +0100, Albert ARIBAUD wrote:
> > Hi Måns,
> > 
> > On Mon, 10 Feb 2014 15:14:49 +0000, Måns Rullgård <mans at mansr.com>
> > wrote:
> > 
> > > Albert ARIBAUD <albert.u.boot at aribaud.net> writes:
> > > 
> > > > Hi Tom,
> > > >
> > > > On Mon, 10 Feb 2014 08:21:39 -0500, Tom Rini <trini at ti.com> wrote:
> > > >
> > > >> On Mon, Feb 10, 2014 at 10:24:47AM +0100, Albert ARIBAUD wrote:
> > > >> > Hi Tom,
> > > >> > 
> > > >> > On Tue,  4 Feb 2014 12:05:33 -0500, Tom Rini <trini at ti.com> wrote:
> > > >> > 
> > > >> > > When we tell the compiler to optimize for ARMv7 it assumes a default of
> > > >> > > unaligned accesses being supported at the hardware level and can make
> > > >> > > use of this to perform what it deems as an optimization in any case,
> > > >> > > including allowing for data to become unaligned.  We explicitly disallow
> > > >> > > this hardware feature so we must tell the compiler.
> > > >> > > 
> > > >> > > Cc: Albert ARIBAUD <albert.u.boot at aribaud.net>
> > > >> > > Cc: Mans Rullgard <mans at mansr.com>
> > > >> > > Signed-off-by: Tom Rini <trini at ti.com>
> > > >> > 
> > > >> > NAK -- the discrepancy between the compiler being told to allow native
> > > >> > unaligned accesses while at the same time telling the hardware to trap
> > > >> > them is conscious and voluntary. It was chosen to help detect unaligned
> > > >> > accesses which are rarely necessary and can be explicitly performed by
> > > >> > software on a case by case basis.
> > > >> > 
> > > >> > If and when a specific file requires unaligned access which cannot be
> > > >> > made by some other mean than enabling -mno-unaligned-access, then this
> > > >> > file should have it added, not the whole of U-Boot.
> > > >> 
> > > >> Right, I recall the discussion, and we chose wrong.
> > > >
> > > > I am quite prepared to discuss whether we chose wrong or right, and
> > > > to change my mind when the conditions are right, but I'll need more than
> > > > such a short and simple statement. :)
> > > 
> > > I already gave you a detailed explanation some months ago.  You refused
> > > to read it.
> > 
> > I can hardly have "refused to read" a message which I *answered*, laid
> > out how I thought the issue should be solved... and got no answer after
> > this.
> > 
> > Now, are we going to discuss the technical issue or is this going to go
> > debian-TC -- which I wouldn't see the point of.
> 
> Well, here's the point that I haven't seen an answer to.  If we tell the
> compiler "you may choose to use unaligned accesses as an optimization,
> we support this", the compiler says "OK, I shall do that", and then we
> fail at run time because we don't actually allow the unaligned access,
> how is this not a problem on our end for the first part of the equation,
> keeping in mind that the real world is poorly designed and when we write
> code to this reality the compiler does the correct thing in all cases
> (or it's a compiler bug).

Both in the thread you are referring to, and in this current thread, I
have already explained why we allow native unaligned accesses in the
compiler and prevent them in the hardware, and how this may seem
contradictory but is not ; but I'm ok with explaining it again, so here
comes.

U-Boot runs on all sorts of hardware. Some hardware can perform native
unaligned accesses without any penalty; some can, but with a penalty;
some just can't.

Therefore, except for some parts of it which are specific to a given
architecture, the U-Boot source code should not assume that native
unaligned accesses are possible. In fact, it should assume they are
impossible.

Now if we restrict ourselves to a subset of ARM architectures, then
native unaligned accesses could be used. And if we restrict ourselves
to ARMv7, they are almost costless.

So on ARMv7 we could write code which does not care about alignment
because native unaligned accesses are just as cheap as aligned ones.

The problem is, since U-Boot runs on a lot of platforms, such
alignment-agnostic code might end up built and run on hardware which
does not allow native unaligned accesses. Granted, this hardware will
detect it; but this might happen quite some time after the code was
pulled in.

Therefore, we should try and detect unaligned accesses as early as
possible.

To perform such a detection, there is a bit in ARM architectures which,
when set, causes native unaligned accesses to trap, allowing us to
see them as early as possible, which is good: we want to catch such
accesses.

Now, there is also a compiler flag related to alignments, and that is
-m[no-]unaligned-access. Its effect is not to allow or disallow
unaligned accesses, but to allow or disallow *native* unaligned
accesses. If -mno-unaligned-access is in effect, then native unaligned
accesses will be replaced by software unaligned accesses, e.g. an
even-address dword access will be actually performed as a combination
of byte and word accesses, all aligned. This will prevent the hardware
from detecting the dword unaligned access, which is bad: we don't want
to miss such accesses.

*That* is the reason for forbidding unaligned accesses at the hardware
level while enabling them at the compiler level: it is the only
combination which ensures that unsuspected unaligned accesses are
detected at run time.

Now back to your question, "how is this not a problem on our end for the
first part of the equation":

- first, if "the first part of the equation" means "the compiler
  setting" as opposed to the hardware setting, then the question fails
  to realize that we don't (and should not) consider the compiler and
  hardware settings separately; they work in *combination*.

- second, assuming the question is "how is it not a problem on our end
  that some code traps due to the combined hardware and compiler
  settings", the answer is: because the setting was not designed to
  catch *ARMv7* issues; it was designed to catch *U-Boot* issues. In
  other words, such traps show that there is code which won't work
  elsewhere than on ARMv7-like hardware which does not care about
  alignment.

The only drawback of this setup is that while some code does unneeded
unaligned accesses (and is thus obviously wrong), some other code
*requires* unaligned accesses (because of standards, or of hardware).
Such code will trap... but *only* if it performs unaligned accesses
*natively*, which it should not since it might run on a hardware not
capable of native unaligned accesses.

That is why I consider that the fault is in the trapped software, not
in the trap. The solution is to make the software use software, not
native, unaligned accesses. The exact solution depends on whether the
code has only a few such unaligned accesses (in which case we should
use explicit unaligned access macros) or many (in which case, for the
file considered, we can enable -mno-unaligned-access). You'll find
instances of both in the U-Boot code.

Therefore:

- I am ok with -mno-unaligned-access applied to files which *require*
  unaligned access and where individual access macros would be
  impractical.

- I am NOT OK with blanket -mno-unaligned-access applied on a file where
individual macros are feasible, and

- I am certainly NOT OK with a blanket -mno-unaligned-access on all code
  and the removal of the whole ARM misalignment detection setup.

Regarding the EFI code, there was a patch submitted (see the thread you
have pointed me to) which had the proper unaligned access macros and
thus did not require -mno-unaligned-access for this file, and certainly
does not require it for the whole of ARM.

Amicalement,
-- 
Albert.