Where is the rest of this change? Just defining an option won't make it so.

I've been wondering for a while why we still enforce alignment on armv6+ platforms that don't require it in hardware. I've been assuming that it's because we need extra support code or to change/remove some existing code, but I haven't had time to investigate it yet.

There is no other change required. All the code is already there (cpufunc.c).
It was simply not possible to enable it through the option in kernel configuration file.

I think if this option is set we may need to notice that and set __NO_STRICT_ALIGNMENT accordingly (I'm not sure how/where that gets set).

I would prefer it, if we do enable this, we do it on all armv6 kernels so we can rely on this in userland. If it's only enabled on some we could see issues where a binaries may or may not work under some kernels depending on if it has unaligned access enabled.

In D2017#6, @ian wrote:

I think if this option is set we may need to notice that and set __NO_STRICT_ALIGNMENT accordingly (I'm not sure how/where that gets set).

__NO_STRICT_ALIGNMENT gets set in machine/_types.h right now.

However, it can not be set based on kernel options because it is shared with user land. It is supposed
to describe all users of the architecture. Since we have no clue what kernel will be used, we'd have
to assume all or nothing based on, say, the architecture level.

I'm not familiar enough with all the implications at the hardware level to know what ARM32_DISABLE_ALIGNMENT_FAULTS would really do, so I can't say if making it default is a good idea or not.

Jakub Palider wrote:

Unaligned access is controlled by two bits in SCTRL register on ARMv6+
architectures, the U bit for unaligned support (on ARMv7+ always set)
and the A bit. This patch refers to bit A only, so we can just tell the
processor not to raise exception when such situation happens, but rather
silently perform additional fetches etc. I believe that has nothing to
do with userspace, unless someone aims at fine-tuning their application
and wants notification on unaligned accesses just to eventually get rid
of them, but that is another story. In kernel we do have unaligned
accesses, so extra handling results in unnecessary overhead and
performance degradation.

I don't think it's that simple.
The "unaligned access" term on ARM consist of two things. First is "A" bit in SCTRL and second is option for compiler "-mno-unaligned-access / -munaligned-access". Unfortunately, because CPU allows unaligned accesses only for some instruction, the compiler must still know if accesses aligned or unaligned data.

Therefore, properly written code compiled cannot cause unaligned abort if is compiled with with -mno-unaligned-access and runs on CPU that have CTRL.A bit set.
And in opposite, bad code can cause unaligned abort even if is complied with -munaligned-access and runs on CPU that have CTRL.A bit cleared.

Please see ARM ARM, section "A3.2.1 Unaligned data access" for details.

Jakub Palider wrote:

Yes, I think I understand your reasoning, but you refer to a more
generic problem, related to CPU settings vs compiler code generation.
Let me show a more concrete example. Below is an extract from fault when
there is no ARM32_DISABLE_ALIGNMENT_FAULTS set.

r7=a5b60b36a
Stopped at tcp_input+0x738: ldr r0, [r7, #0x004]

Here, we can see instruction (LDR) which *does* operate on unaligned
addresses, so I assume compiler was clever enough not to use any of
those "forbidden" instructions (e.g. LDMIA/STMIA). Nevertheless the bit
SCTRL.A set still causes that fault happen and handled in the kernel.
As for alignment correctness in network code... well, I know it is
theoretically possible, but ensuring that would come with an
unacceptable performance cost.

Summing up, I think that the option ought to be exported - especially
that this patch does no more than just changing feature visibility.
Feature which had been added to cpufunc.c already - not without a
reason.

I personally don't understand about what is this discussion because from my point of view we have some portion of code that depends on an option that cannot be set in the configuration file. In this patch we just want to add the possibility to enable it.

In D2017#12, @zbb wrote:

I personally don't understand about what is this discussion because from my point of view we have some portion of code that depends on an option that cannot be set in the configuration file. In this patch we just want to add the possibility to enable it.

Adding this config option isn't a big deal.

However, switching the architecture from one that advises that it can't do alignment fixups in hardware to one that can is, and this review has been hijacked to discuss that possibility.

I disagree, adding this IS a big deal. As soon as we have a kernel out there that can support unaligned accesses, people will build userland apps that rely on that being true, and at that point we can't change it back without effectively breaking the ABI.

Having the discussion about the implications of doing this is definitely something we should do up-front, because changing our minds will be somewhere between painful and impossible. (Look at the other things we've done without much thought, such as labeling an architecture "armv6" even though it's really armv7, then spending years trying to explain that armv6 means something different in FreeBSD.)

Jakub Palider wrote:
Yes, I think I understand your reasoning, but you refer to a more
generic problem, related to CPU settings vs compiler code generation.
Let me show a more concrete example. Below is an extract from fault when
there is no ARM32_DISABLE_ALIGNMENT_FAULTS set.

r7=a5b60b36a
Stopped at tcp_input+0x738: ldr r0, [r7, #0x004]

Here, we can see instruction (LDR) which *does* operate on unaligned
addresses, so I assume compiler was clever enough not to use any of
those "forbidden" instructions (e.g. LDMIA/STMIA).

This resolution is invalid, imho.
The kernel is compiled with “-mno-unaligned-access”, and if everything is in order, the compiler cannot generate any unaligned access. The fault in tcp_input() indicates problem in code, compiler evidently assumes aligned address here. Due to this (incorrect) assumption compiler is free to generate any instruction (LDM/STM including).
Or I miss something?

Enabling of unaligned access is big win for network code. But this case is evident papering over real problem. And this problem must be solved first.