HAVE_EFFICIENT_UNALIGNED_ACCESS on ARM32 (was: Alignment issues in zImage with Linux 4.12, LZ4 and GCC5.3)

On 7 September 2017 at 00:18, Arnd Bergmann [off-list ref] wrote:

On Thu, Sep 7, 2017 at 12:48 AM, Ard Biesheuvel
[off-list ref] wrote:

quoted

On 6 September 2017 at 23:38, Arnd Bergmann [off-list ref] wrote:

quoted

On Thu, Sep 7, 2017 at 12:23 AM, Ard Biesheuvel
[off-list ref] wrote:

quoted

On 6 September 2017 at 21:57, Arnd Bergmann [off-list ref] wrote:

quoted

On Mon, Sep 4, 2017 at 6:19 PM, Romain Izard [off-list ref] wrote:

HAVE_EFFICIENT_UNALIGNED_ACCESS only affects explicit unaligned
accesses, and selects between fixups in hardware or in software.
AFAICT the issue here is implicit unaligned accesses, where char
pointers are passed as u32 * arguments.

The problem with include/linux/unaligned/access_ok.h is that it
converts pointers
that are known by the caller to be potentially unaligned and accesses them as if
they were aligned. This means we require a software fixup through the
trap handler
on ARM in cases that the compiler already knows how to handle correctly when
using linux/unaligned/le_struct.h. On ARMv7 this means it ends up using normal
load/store instructures but not the ldm/stm or ldrd/stdr instructions
that are not
allowed on unaligned pointers.

Ah ok, I missed that part. The distinction between ldr/str and
ldm/stm/ldrd is a bit fiddly, but if we can solve this using C code, I
am all for it.

quoted

Doing that solves the problem that Romain ran into and also makes other
code much more efficient on ARMv7.

It is not entirely clear to me why casting to a pointer-to-struct type
makes any difference here. Is it simply because of the __packed
attribute?

The problem is code like

struct twoint {
   int a; int b;
};
void __noinline access_unaligned_8bytes(struct twoint *s, int a, int b)
{
       put_unaligned(a, &s->a);
       put_unaligned(b, &s->b);
}
int caller(char *c, int offset, int a, int b)
{
       access_unaligned_8bytes((void *)c + offset, a, b);
}

With include/linux/unaligned/access_ok.h, this turns into two stores
that gcc can combine into a single 'strd' or 'stm'. With the
linux/unaligned/le_struct.h version, gcc knows that the pointer
may be unaligned, so it will use instructions that it knows are
safe, either byte accesses (on armv5 and earlier) or normal
str (on armv6+).

quoted

Anyway, the issue I spotted in the LZ4 code did not use unaligned
accessors at all, so we must be talking about different things here.

I see lots of unaligned helpers in the lz4 code, is this not what
we hit?

$ git grep unaligned lib/
lib/lz4/lz4_compress.c:#include <asm/unaligned.h>
lib/lz4/lz4_decompress.c:#include <asm/unaligned.h>
lib/lz4/lz4defs.h:#include <asm/unaligned.h>
lib/lz4/lz4defs.h:      return get_unaligned((const U16 *)ptr);
lib/lz4/lz4defs.h:      return get_unaligned((const U32 *)ptr);
lib/lz4/lz4defs.h:      return get_unaligned((const size_t *)ptr);
lib/lz4/lz4defs.h:      put_unaligned(value, (U16 *)memPtr);

Yes, you are right. The code I looked at before does cast a char* to a
U32*, but it is in the compression path, so it has nothing to do with
this issue.

So I agree that access_ok.h is unsuitable for any 32-bit ARM core, and
we should be using the struct version instead. My only remaining
question is why we need access_ok.h in the first place: it is worth a
try to check whether both produce the same code on AArch64.