Re: [PATCH v2] parisc: Fix boot with kernel v5.14

[Helge Deller <deller@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 2836 bytes --]

On 9/2/21 8:35 PM, Arnd Bergmann wrote:
> On Thu, Sep 2, 2021 at 2:06 PM Helge Deller <deller@gmx.de> wrote:
>>
>> Kernel v5.14 has various changes to optimize unaligned memory accesses,
>> e.g. commit 0652035a5794 ("asm-generic: unaligned: remove byteshift helpers").
>>
>> Those changes break the bootloader and other places in kernel for parisc
>> which needs byte-wise accesses to unaligned memory.
>>
>> Here is an updated patch/hack which fixes those boot problems by adding
>> a compiler optimization barrier. More info and background can be found in BZ:
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102162
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>
> Right, this should fix it, but I tend to agree with what Andrew Pinski
> said: the existing version is actually correct and allows valid
> optimizations on static variables as long as those are correctly
> annotated in C.
Let's look at generic kernel code, e.g. in fs/btrfs/inode.c.
You will find many similiar cases all around the kernel.
------------
struct dir_entry {
         u64 ino;
         u64 offset;
         unsigned type;
         int name_len;
};

static int btrfs_filldir(void *addr, int entries, struct dir_context *ctx)
{
         while (entries--) {
                 struct dir_entry *entry = addr;
                 char *name = (char *)(entry + 1);

                 ctx->pos = get_unaligned(&entry->offset);
                 if (!dir_emit(ctx, name, get_unaligned(&entry->name_len),
                                          get_unaligned(&entry->ino),
                                          get_unaligned(&entry->type)))
                         return 1;
                 addr += sizeof(struct dir_entry) +
                         get_unaligned(&entry->name_len);
                 ctx->pos++;
         }
         return 0;
}
-----------
According to Andrew Pinski's statement, the compiler will assume here that all of
those get_unaligned() calls will access naturally aligned memory and I'm pretty
sure the compiler will generate native 4/8 byte accesses on all platforms.
Most likely you will not notice on most platforms because it will get fixed by
exception handlers or natively in hardware.
But anyway, it's not what the developers intended by adding get_unaligned().

I see no chance to change all those places in the kernel.

> The problem on parisc seems to be that at least
> one variable is generated by the linker in a way that is incompatible
> with the psABI but declared as a regular __u32.

I'm happy to change it if it's just this one variable.
Currently there are more, but I'm still testing.

But generally, would you be willing to consider applying something similiar to
the attached patch (untested) until we get it finally resolved on parisc?

Helge

[-- Attachment #2: force-byte-access.patch --]
[-- Type: text/x-patch, Size: 1265 bytes --]

diff --git a/arch/parisc/include/uapi/asm/byteorder.h b/arch/parisc/include/uapi/asm/byteorder.h
index a59d9b7e35a8..dcf103b5bd4b 100644
--- a/arch/parisc/include/uapi/asm/byteorder.h
+++ b/arch/parisc/include/uapi/asm/byteorder.h
@@ -2,6 +2,8 @@
 #ifndef _PARISC_BYTEORDER_H
 #define _PARISC_BYTEORDER_H

+#define ARCH_FORCE_BYTE_ACCESSES(ptr)	__asm__ ("" : "+r" (ptr))
+
 #include <linux/byteorder/big_endian.h>

 #endif /* _PARISC_BYTEORDER_H */
diff --git a/include/asm-generic/unaligned.h b/include/asm-generic/unaligned.h
index 1c4242416c9f..cf8c9460e575 100644
--- a/include/asm-generic/unaligned.h
+++ b/include/asm-generic/unaligned.h
@@ -9,13 +9,19 @@
 #include <linux/unaligned/packed_struct.h>
 #include <asm/byteorder.h>

+#ifndef ARCH_FORCE_BYTE_ACCESSES
+#define ARCH_FORCE_BYTE_ACCESSES(ptr)	do { } while (0)
+#endif
+
 #define __get_unaligned_t(type, ptr) ({						\
 	const struct { type x; } __packed *__pptr = (typeof(__pptr))(ptr);	\
+	ARCH_FORCE_BYTE_ACCESSES(__pptr);					\
 	__pptr->x;								\
 })

 #define __put_unaligned_t(type, val, ptr) do {					\
 	struct { type x; } __packed *__pptr = (typeof(__pptr))(ptr);		\
+	ARCH_FORCE_BYTE_ACCESSES(__pptr);					\
 	__pptr->x = (val);							\
 } while (0)