[U-Boot] [PATCH] arm: armv7: add compile option -mno-unaligned-access if available

[Albert ARIBAUD]

on Thu, 5 Jul 2012 09:57:19 +0200,
Albert ARIBAUD <albert.u.boot at aribaud.net> wrote :

> Hi Måns,
> 
> On Mon, 02 Jul 2012 17:14:40 +0100, Måns Rullgård <mans at mansr.com>
> wrote:
> 
> > > IMHO, our recent discussion showed that both ways are wrong.
> > > "-mno-unaligned-access" works around misaligned data on the software
> > > level, while allowing unaligned access does on the hardware level.
> > >
> > > What we really want is no unaligned access in U-Boot at all. Just
> > > because "-mno-unaligned-access" is the default on ARMv5, we should
> > > not consider it a gold standard.
> > 
> > It's slightly more complicated than that.  Data can be misaligned for
> > a variety of reasons:
> > 
> > 1. Errors in software.
> > 2. Specified by a file format or communication protocol.
> > 3. Deliberately misaligned by the compiler.
> > 
> > Misaligned data of type 1 should of course be fixed properly, not
> > worked around in any way.
> 
> Agreed.
> 
> > Type 2 happens all the time, and has to be dealt with one way or
> > another.  If the hardware supports unaligned accesses, this is usually
> > faster than reading a byte at a time.
> 
> Sorry but I don't accept a systemic change as a good solution to a
> specific issue. The best fix is "don't accept using a protocol or file
> format which was not designed to avoid native misaligned accesses", and
> the second best fix is "if you really must deal with a protocol or file
> format which causes native alignment issues, then the fix should only
> affect the protocol or file format's issue, not with your system which
> is not the root cause".
> 
> But to be honest, I haven't seen such badly a designed protocol or file
> format; their designers tend to make sure that (assuming the start of a
> protocol or file 'content' (frame, record, etc) is aligned, then all
> fields in it are aligned as well. Can someone point me to an example of
> a protocol or file format which does present such a misalignment risk ?
> 
> > When targeting ARMv6 and later, recent gcc versions have started
> > issuing deliberate unaligned accesses where previously byte by byte
> > accesses would have been done.
> 
> Wrongly formulated: "mis-aligning deliberately" seems to imply that
> GCC did away with the possibility of properly aligning, which they of
> course did not. What they did is switch its default behavior regarding
> native misaligned accesses from forbidding to allowing them. The change
> here is of policy, a change which we may or may not want to follow. 
> 
> > This happens with "packed" structs
> > and sometimes to write multiple smaller values at once, typically when
> > zero-initialising things.  These unaligned accesses are *good*.  They
> > make code smaller and faster.
> 
> Again, "good" is a policy, or subjective, statement, not an absolute.
> Just as "good" is correctly aligning data in the first place (to begin
> with, in protocols and file formats) and keeping the compiler's native
> misaligned access policy set to "do not allow".
> 
> > The real problem here is that u-boot is setting the strict alignment
> > checking flag, invalidating the assumption of the compiler that the
> > system allows unaligned accesses.  For ARMv5 and earlier, setting this
> > flag is usually advisable since it makes finding accidental unaligned
> > accesses much easier.
> 
> Just as it is for ARMv6 and up. Again, just because the compiler folks
> changed their default policy does not mean we should change ours,
> which is not based on the same goals. 
> 
> > This was debated in the context of the kernel a while ago, ultimately
> > leading to strict alignment being disabled for ARMv6 and up [1].
> > 
> > [1]
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=8428e84d42179c2a00f5f6450866e70d802d1d05
> 
> I'd rather have a link to the rationale than to the commit, but anyway,
> the kernel folks' decision is theirs and does not necessarily apply to
> us. I have mailed Catalin Marinas off-list to get details on the
> rationale and context of the kernel patch; I will report conclusions
> here.
> 
> Meanwhile, our policy regarding misalignment accesses is to only allow
> them when externally required (by something other than a bad design).
> Someone please show me such an external requirement for U-Boot, and I
> will reconsider -- and then, other arch custodians may have a problem
> with that too.
> 
> Regarding the origin of this patch, i.e. a mis-alignment of USB fields,
> and unless U-Boot USB folks say otherwise, this issue should be fixed
> by aligning said fields properly.

We are nearing the release, and we obviously won't have misalignments
fixed and tested in time for it.

So I suspect that if I want the ARM U-Boot release to work I'll have to
allow this patch in my master branch to be pulled in for 12.07, so that
the compiler keeps behaving as it did before gcc changed the default.

... but only for the upcoming release, i.e. I will revert the patch in
my 'next' branch, which will apply right after 12.07 is out. Therefore,
before next release, misalignments will have to be fixed at the root.

Amicalement,
-- 
Albert.