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.

Amicalement,

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,

Hi Måns,

On Mon, 14 Oct 2013 16:59:56 +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 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.

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,

Hi Måns,

On Tue, 15 Oct 2013 16:23:44 +0100, Måns Rullgård mans@mansr.com wrote:

Albert ARIBAUD albert.u.boot@aribaud.net writes:

...

...

...

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.

Let's go back to the basics.

In ARMv6 and later there is a bit in the system control register (SCTLR.A) which decides whether or not unaligned memory accesses are allowed. The reset value of this bit allows unaligned accesses.

When unaligned accesses are allowed, word and halfword load/store instructions (LDR, STR, LDRH, LDRSH, STRH) with an unaligned address simply perform the requested memory operation. When unaligned accesses are disallowed (SCTLR.A set), these instructions cause an alignment fault if used with an unaligned address. The load/store double and multiple instructions (LDRD, STRD, LDM, STM) always trap on unaligned addresses.

This is all described in the ARM Architecture Reference Manual (DDI0406C) section A3.2.

That's the hardware side.

Your description is correct, although this bit is not specific to "ARMv6 and later", since ARMv5 has alignment checks too.

On the compiler side, gcc traditionally did not issue unaligned load/store instructions on ARM.

Please be specific: gcc did not emit *native* unaligned accesses.

Since version 4.7, gcc does issue unaligned accesses when the target is ARMv6 or later. This makes sense since a hardware unaligned access is faster than doing it byte-wise in software, and the default for the CPU is to permit unaligned accesses. Needless to say, a potentially unaligned address will only be accessed using the subset of load/store instructions for which this is supported.

Indeed. Note that this is stated in doc/README.arm-unaligned-accesses.

To support configurations where SCTR.A is set (disallowing unaligned accesses), gcc 4.7 also adds a flag (-mno-unaligned-access) causing it to never emit potentially unaligned loads or stores.

Maybe the intent which governed the addition of this option was indeed to support configurations where alignment check is enabled; what we can tell is what this option does, and yes, it controls whether the compiler will use native unaligned accesses.

The compiler behaviour described above is true only for well-behaved code. In standard C, pointers must always be aligned according to their target type. For instance, a pointer to a 32-bit integer type must typically be 32-bit aligned. Thus, if a pointer is constructed with incorrect alignment, any attempt to use it may result in invalid memory access instructions being executed.

Correct, although I'm not sure why you're mentioning this (and, strictly speaking, all of the compiler's behavior is defined only for 'well-behaved' C code).

In practice, various situations arise where there is a need to work with unaligned data, for example when parsing some communication protocols. To simplify such code, most compilers provide some language extension allowing the programmer to annotate a type definition or pointer as being potentially unaligned. In gcc, the 'packed' attribute on struct and union types serves this purpose.

Correct, and again, I fail to see why you mention this.

Any access to a member of a 'packed' struct/union is assumed to be potentially unaligned, and the instructions selection is limited accordingly. When -munaligned-access is in effect, unaligned word or halfword load/store instructions may be used here. When this feature is disabled (-mno-unaligned-access), only aligned loads and stores (typically bytes) are permitted.

Ditto.

The situations where the compiler will issue an unaligned memory access are generally not predictable. Currently, they tend to occur in struct/array assignment (including initialisation), inline expansion of memcpy/memset, and accesses to 'packed' struct members. As compiler optimisations improve, these cases will likely increase in number.

This last statement has no solid foundation. On the contrary, there is no reason that a compiler emit unaligned accesses when by default it is expected to align data to their natural boundaries.

As we can see, enabling the -munaligned-access flag results in load/store instructions occasionally accessing unaligned memory, and the precise places where this happens are not predictable. It is thus a requirement that SCTLR.A be clear when this compiler flag is set. Otherwise alignment faults will occur.

As I said, and as documented in doc/README.arm-unaligned-accesses for a whole year now, the only case where native unaligned accesses are emitted is with string literals. Even char arrays are aligned. There's actually a case to be made that string literals should be aligned, too, like all other strings are.

If for whatever reason SCTLR.A is set, it is required to use the -mno-unaligned-access compiler flag in order for the code to run cleanly. Failure to do so will result in alignment faults when the code is executed.

It is never *required* to use -mno-unaligned-access unless your hardware is unable to perform unaligned accesses for some reason external to the CPU.

One reason for setting SCTLR.A is to aid in catching programming errors whereby a normal pointer is assigned an unaligned value. Since these pointers are assumed by the compiler to be correctly aligned, accesses through them are unaffected by the -m[no-]unaligned-access flag, and any such errors will thus trigger an alignment fault.

This is one use of alignment checks. Another use is to catch code which intends to do unaligned accesses even though the policy of the project says otherwise.

If any of the above is unclear, please let me know, and I will try to explain it better.

All this is perfectly clear and essentially valid... if your goal is to use all the features of ARMv6+ to simplify the developer's life under the assumption that the code is well-behaved (and the compiler is error-free, for that matter).

It is also still not valid with respect to my goal, which is to make sure that the actual code of a multi-architecture project running on a variety of compilers will be as correct as possible.

Let me state this again: while the approach you describe is the logical one to make life as easy as possible for the developer (and compiler write) on ARMv6+ architectures, it is not *mandated* in any sense, and it is not the approach I have chosen.

I suggest now that you leave aside any assumptions on "how things must be done", then read my answers again in the context I have just described, and fin "why things are done this way" in ARM U-Boot.

Amicalement,

Hi Måns,

On Tue, 15 Oct 2013 17:29:24 +0100, Måns Rullgård mans@mansr.com wrote:

Albert ARIBAUD albert.u.boot@aribaud.net writes:

...

Hi Måns,

On Tue, 15 Oct 2013 16:23:44 +0100, Måns Rullgård mans@mansr.com wrote:

...

On the compiler side, gcc traditionally did not issue unaligned load/store instructions on ARM.

Please be specific: gcc did not emit *native* unaligned accesses.

Please explain what you mean by "native unaligned access".

By this I mean an unaligned access performed with a single transfer, initiated by a single processor instruction, as opposed to an unaligned access performed with a series of smaller, aligned, transfers (and possibly additional operations) initiated by a sequence of processor instructions (emulated, if you will).

Prior to 4.7, gcc did emit unaligned accesses if instructed to at the source code level; they were however emulated. From 4.7 onward, it was possible to choose whether unaligned transfers should be performed native or emulated, the default being native for ARMv6+, and emulated for the rest.

Amicalement,

Hi Piotr,

On Mon, 14 Oct 2013 15:49:43 +0200, Piotr Wilczek p.wilczek@samsung.com wrote:

Dear Albert and Måns,

...

-----Original Message----- From: Måns Rullgård [mailto:mans@mansr.com] Sent: Monday, October 14, 2013 3:05 PM To: Albert ARIBAUD Cc: Piotr Wilczek; 'Tom Rini'; u-boot@lists.denx.de; 'Kyungmin Park' Subject: Re: [U-Boot] [PATCH] disk:efi: avoid unaligned access on efi partition

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?

Thank you for your comments, I will follow your advice.

I will NAK building part_efi.c with options to allow native unaligned access.

I will, OTOH, be ok with explicit unaligned reads/writes.

Best regards Piotr Wilczek

Amicalement,

On Tue, Jan 28, 2014 at 01:46:52PM +0100, Piotr Wilczek wrote:

This patch fixes part_efi code to avoid unaligned access exception on some ARM platforms.

Signed-off-by: Piotr Wilczek p.wilczek@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com CC: Tom Rini trini@ti.com CC: Albert ARIBAUD albert.u.boot@aribaud.net

---

Chnages for V2:

• used put_unaligned to copy value;
• use __aligned to align local array;

disk/part_efi.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)

Sorry. After re-reading the thread here again I've posted a patch to globally switch to -mno-unaligned-access for our armv7 tunes instead. Thanks for your effort however!