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

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

[Helge Deller]

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 on parisc which needs byte-wise
accesses to unaligned memory.

Below is a *** temporary *** patch/hack which fixes those boot problems.

Signed-off-by: Helge Deller <deller@gmx.de>
---

diff --git a/include/asm-generic/unaligned.h b/include/asm-generic/unaligned.h
index 1c4242416c9f..3ef9a5dd35b5 100644
--- a/include/asm-generic/unaligned.h
+++ b/include/asm-generic/unaligned.h
@@ -9,10 +9,21 @@
 #include <linux/unaligned/packed_struct.h>
 #include <asm/byteorder.h>

+#if 0
 #define __get_unaligned_t(type, ptr) ({						\
 	const struct { type x; } __packed *__pptr = (typeof(__pptr))(ptr);	\
 	__pptr->x;								\
 })
+#else
+#define __get_unaligned_t(type, ptr) ({						\
+	unsigned char *a = (unsigned char *)(unsigned long)(ptr);		\
+	sizeof(type) == 1 ? a[0] :						\
+	sizeof(type) == 2 ? a[0] << 8  | a[1] : 				\
+	sizeof(type) == 3 ? a[0] << 16 | a[1] << 8  | a[2] : 			\
+	sizeof(type) == 4 ? a[0] << 24 | a[1] << 16 | a[2] << 8 | a[3] :	\
+	 0 ; \
+})
+#endif

 #define __put_unaligned_t(type, val, ptr) do {					\
 	struct { type x; } __packed *__pptr = (typeof(__pptr))(ptr);		\

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

[Helge Deller]

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>

diff --git a/include/asm-generic/unaligned.h b/include/asm-generic/unaligned.h
index 1c4242416c9f..fdb6864514b1 100644
--- a/include/asm-generic/unaligned.h
+++ b/include/asm-generic/unaligned.h
@@ -11,11 +11,13 @@

 #define __get_unaligned_t(type, ptr) ({						\
 	const struct { type x; } __packed *__pptr = (typeof(__pptr))(ptr);	\
+	__asm__ ("" : "+r" (__pptr));						\
 	__pptr->x;								\
 })

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

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

[Arnd Bergmann]

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. 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.

       Arnd

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

[Helge Deller]

[-- 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)

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

[Helge Deller]

> > 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 only applying this patch seems to fix the boot issue.
Maybe it's just luck that compressed kernel files are correctly aligned.

Anyway, Arnd, please just wait and do not consider applying my previous patch.

Helge

diff --git a/arch/parisc/boot/compressed/misc.c b/arch/parisc/boot/compressed/misc.c
index 2d395998f524..a9949ab9b283 100644
--- a/arch/parisc/boot/compressed/misc.c
+++ b/arch/parisc/boot/compressed/misc.c
@@ -26,7 +26,7 @@
 extern char input_data[];
 extern int input_len;
 /* output_len is inserted by the linker possibly at an unaligned address */
-extern __le32 output_len __aligned(1);
+extern char output_len;
 extern char _text, _end;
 extern char _bss, _ebss;
 extern char _startcode_end;

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

[Arnd Bergmann]

On Thu, Sep 2, 2021 at 9:48 PM Helge Deller <deller@gmx.de> wrote:
> 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().

No, this case is completely different: 'entry' points to dynamically allocated
memory that gets passed in via a void pointer, so gcc has no knowledge of
the alignment of the underlying storage, and it will do the access according to
the __packed constrains in the get_unaligned() helper. When you look at the
assembler output for this function on a 5.14 parisc kernel, I'm sure you will
see the correct byte accesses, just like the trivial example I posted
in bugzilla.

The reason that the "output_len" access breaks is that gcc explicitly optimizes
the bytewise access  into word accesses because it assumes that global variables
are correctly declared, and that they are aligned according to the requirements
of the ABI.
This may be surprising and even unfortunate, but I can see why they did
this optimization, and that it helps in other cases as well.

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

No, that would mean changing all get_unaligned() accesses to pointer
dereferences on types that are declared as __packed themselves.
The get_unaligned()/put_unaligned() helpers generally do what they
are designed for, it just breaks when you have misaligned global
variables that are created by a linker script.

     Arnd

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

[Helge Deller]

On 9/2/21 10:41 PM, Arnd Bergmann wrote:
> On Thu, Sep 2, 2021 at 9:48 PM Helge Deller <deller@gmx.de> wrote:
>> 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().
>
> No, this case is completely different: 'entry' points to dynamically allocated
> memory that gets passed in via a void pointer, so gcc has no knowledge of
> the alignment of the underlying storage, and it will do the access according to
> the __packed constrains in the get_unaligned() helper. When you look at the
> assembler output for this function on a 5.14 parisc kernel, I'm sure you will
> see the correct byte accesses, just like the trivial example I posted
> in bugzilla.
>
> The reason that the "output_len" access breaks is that gcc explicitly optimizes
> the bytewise access  into word accesses because it assumes that global variables
> are correctly declared, and that they are aligned according to the requirements
> of the ABI.
> This may be surprising and even unfortunate, but I can see why they did
> this optimization, and that it helps in other cases as well.

Arnd, you were absolutely correct and I was wrong.

It seems to work nicely now after I changed the output_len variable to
become an "extern char".

Thanks!
Helge


>> I see no chance to change all those places in the kernel.
>
> No, that would mean changing all get_unaligned() accesses to pointer
> dereferences on types that are declared as __packed themselves.
> The get_unaligned()/put_unaligned() helpers generally do what they
> are designed for, it just breaks when you have misaligned global
> variables that are created by a linker script.

Yes.

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

[Arnd Bergmann]

On Sun, Sep 5, 2021 at 11:40 PM Helge Deller <deller@gmx.de> wrote:
> On 9/2/21 10:41 PM, Arnd Bergmann wrote:
> >
> > The reason that the "output_len" access breaks is that gcc explicitly optimizes
> > the bytewise access  into word accesses because it assumes that global variables
> > are correctly declared, and that they are aligned according to the requirements
> > of the ABI.
> > This may be surprising and even unfortunate, but I can see why they did
> > this optimization, and that it helps in other cases as well.
>
> Arnd, you were absolutely correct and I was wrong.
>
> It seems to work nicely now after I changed the output_len variable to
> become an "extern char".

Ok, that's a relief, at least my patch wasn't the main cause then.

Changing the declaration to 'extern char' of course is still incorrect, so
this might cause other problems in the future, the same way that the
old declaration caused the problem by decaring the wrong alignment.

I think declaring it as an array of four characters, or a struct with reduced
alignment would be the safer choice here. Ideally however you would
change the linker script to insert a

    . = ALIGN(4);

before the output to make the variable properly aligned according to
the ABI. See 'git log arch/arm/kernel/vmlinux.lds.S' for a long history
of alignment changes we did there.

        Arnd

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

[Helge Deller]

On 9/6/21 12:54 PM, Arnd Bergmann wrote:
> On Sun, Sep 5, 2021 at 11:40 PM Helge Deller <deller@gmx.de> wrote:
>> On 9/2/21 10:41 PM, Arnd Bergmann wrote:
>>>
>>> The reason that the "output_len" access breaks is that gcc explicitly optimizes
>>> the bytewise access  into word accesses because it assumes that global variables
>>> are correctly declared, and that they are aligned according to the requirements
>>> of the ABI.
>>> This may be surprising and even unfortunate, but I can see why they did
>>> this optimization, and that it helps in other cases as well.
>>
>> Arnd, you were absolutely correct and I was wrong.
>>
>> It seems to work nicely now after I changed the output_len variable to
>> become an "extern char".
>
> Ok, that's a relief, at least my patch wasn't the main cause then.

Yes, that's good!

> Changing the declaration to 'extern char' of course is still incorrect, so
> this might cause other problems in the future, the same way that the
> old declaration caused the problem by decaring the wrong alignment.

Possibly. For the current use case it's enough.

> I think declaring it as an array of four characters, or a struct with reduced
> alignment would be the safer choice here.

Sure, I'll change that if it becomes necessary.

> Ideally however you would
> change the linker script to insert a
>
>      . = ALIGN(4);
>
> before the output to make the variable properly aligned according to
> the ABI. See 'git log arch/arm/kernel/vmlinux.lds.S' for a long history
> of alignment changes we did there.

This here is a special case and it's not just parisc.
This happens when the linux kernel gets compressed, and at the end of
the compressed file bzip/gzip/lz4 and such gets added the size of the
compressed file.
See in generic Makefile scripts/Makefile.lib, line 392ff.
During make then this happens (for lz4 compression):
  { cat arch/parisc/boot/compressed/vmlinux.bin | lz4c -l -c1 stdin stdout; printf \\350\\044\\232\\000; } > arch/parisc/boot/compressed/vmlinux.bin.lz4
which is then later added as binary input to the linker to generate final file.

The printf() is the one which is then read afterwards.
Changing this isn't IMHO simply possible or useful, because gzip already does it that way
and it should be compatible independend which compression algorithm is used.
And if I'd change it I would need to change it for x86, sh and um arches too.
So, for now I don't think it's worth messing around here.

Helge

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

[Arnd Bergmann]

On Mon, Sep 6, 2021 at 10:15 PM Helge Deller <deller@gmx.de> wrote:
> On 9/6/21 12:54 PM, Arnd Bergmann wrote:
> > Ideally however you would
> > change the linker script to insert a
> >
> >      . = ALIGN(4);
> >
> > before the output to make the variable properly aligned according to
> > the ABI. See 'git log arch/arm/kernel/vmlinux.lds.S' for a long history
> > of alignment changes we did there.
>
> This here is a special case and it's not just parisc.
> This happens when the linux kernel gets compressed, and at the end of
> the compressed file bzip/gzip/lz4 and such gets added the size of the
> compressed file.
> See in generic Makefile scripts/Makefile.lib, line 392ff.
> During make then this happens (for lz4 compression):
>   { cat arch/parisc/boot/compressed/vmlinux.bin | lz4c -l -c1 stdin stdout; printf \\350\\044\\232\\000; } > arch/parisc/boot/compressed/vmlinux.bin.lz4
> which is then later added as binary input to the linker to generate final file.

Ok, I see. It looks like x86 rewrote this part in 02a884c0fe7e ("x86, boot:
determine compressed code offset at compile time") to parse the size
at compile time, but recreating this on parisc and the other architectures
that have the vmlinux.scr linker script would be a lot of work for little
benefit.

       Arnd