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

[Albert ARIBAUD]

Hi Måns,

On Mon, 14 Oct 2013 16:59:56 +0100, Måns Rullgård <mans at mansr.com>
wrote:

> Albert ARIBAUD <albert.u.boot at aribaud.net> writes:
> 
> > Hi Måns,
> >
> > On Mon, 14 Oct 2013 15:09:39 +0100, Måns Rullgård <mans at mansr.com>
> > wrote:
> >
> >> Albert ARIBAUD <albert.u.boot at aribaud.net> writes:
> >> 
> >> > Hi Måns,
> >> >
> >> > On Mon, 14 Oct 2013 14:05:13 +0100, Måns Rullgård <mans at mansr.com>
> >> > wrote:
> >> >
> >> >> Albert ARIBAUD <albert.u.boot at 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.
> 
> Your wishes are mutually exclusive.  You cannot both allow hardware
> unaligned access AND at the same time trap them.

These are not wishes, there are actual settings chosen for the reason
already laid out. They do appear contradictory if your goal is to use
ARMv6+ features to their maximum, but this is not the goal here.

> > 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.
> 
> If you disable unaligned accesses in hardware (as u-boot does), you have
> no option but doing them a byte at a time.

Indeed, but I do *not* *disable* native unaligned accesses, I *allow*
them; and I do not *want* them to be replaced by byte-by-byte emulation.

> > 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. 
> 
> The hardware does not differentiate between intentional and
> unintentional unaligned accesses.  Unlike some architectures, which have
> dedicated instructions for unaligned accesses, ARM uses the same
> instructions in both cases (with some limitations).

Indeed, the hardware does not differentiate between intentional vs
unintentional unaligned accesses, which is why I decided that rather
than suffer the latter, we should trap them both and fix them
according to their nature: intentional ones get replaced by emulation,
and the cause of unintentional ones gets fixed.

> > 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.
> 
> The problem is that when you tell gcc (using -munaligned-access) that
> hardware unaligned accesses are supported, you give it permission to
> compile even get/put_unaligned() calls (or otherwise annotated accesses)
> using regular LDR/STR instructions.  If this code runs with strict
> checking enabled in hardware (SCTRL.A set), it will trap.

This is not "the problem", this is "the intended effect": I *want* it to
generate native unaligned accesses when conditions allow it to, because
such conditions (except one single known, identified, case [1]) indicate
that there is an actual unaligned access statement in the source code
which was not fixed or made explicit, and trapping the native access
will lead us immediately to the corresponding source code point.

> What you probably want is to build with -mno-unaligned-access and enable
> strict hardware alignment checking.  This ensures that any deliberate
> unaligned accesses (e.g. through get_unaligned) are split into multiple
> smaller accesses while trapping any (unintentional) unaligned word
> accesses.

No, I don't want this. I don't want to escape the alignment check trap
I also set up. I want to get caught.

> The most common alignment-related programming mistake is to dereference
> a pointer with insufficient alignment.  It is far less common for
> pointers to be marked as unaligned when they do not need to be.

Possibly, but a typology of possible causes is slightly beyond the
point of this discussion.

> > 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.
> 
> The benefit of allowing hardware unaligned accesses when supported is
> that code which for some reason must do these things (as you said,
> sometimes this is unavoidable) will be faster.

Which actual use case in U-Boot would show a substantial speed increase
from allowing native unaligned accesses? I don't consider accessing a
few fields occasionally as 'substantial'. And I don't see any large
operation, e.g. a memory transfer, requiring unaligned accesses. 

[1] This case is when a local char array is initialized with a string
constant the size of which is a multiple of 4; then gcc uses native
ldr/str instructions even though the string literal is not aligned.
The workarounds for this are documented.

Amicalement,
-- 
Albert.