[PATCH] MIPS: boot/compressed: Copy DTB to aligned address

[PATCH] MIPS: boot/compressed: Copy DTB to aligned address

[Paul Cercueil]

Since 5.12-rc1, the Device Tree blob must now be properly aligned.

Therefore, the decompress routine must be careful to copy the blob at
the next aligned address after the kernel image.

This commit fixes the kernel sometimes not booting with a Device Tree
blob appended to it.

Fixes: c4d5e638d6e9 ("scripts/dtc: Update to upstream version v1.6.0-51-g183df9e9c2b9")
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 arch/mips/boot/compressed/decompress.c | 8 ++++++++
 arch/mips/kernel/vmlinux.lds.S         | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/arch/mips/boot/compressed/decompress.c b/arch/mips/boot/compressed/decompress.c
index e3946b06e840..3d70d15ada28 100644
--- a/arch/mips/boot/compressed/decompress.c
+++ b/arch/mips/boot/compressed/decompress.c
@@ -14,6 +14,7 @@
 
 #include <asm/addrspace.h>
 #include <asm/unaligned.h>
+#include <asm-generic/vmlinux.lds.h>
 
 /*
  * These two variables specify the free mem region
@@ -120,6 +121,13 @@ void decompress_kernel(unsigned long boot_heap_start)
 		/* last four bytes is always image size in little endian */
 		image_size = get_unaligned_le32((void *)&__image_end - 4);
 
+		/* The device tree's address must be properly aligned  */
+		image_size = ALIGN(image_size, STRUCT_ALIGNMENT);
+
+		puts("Copy device tree to address  ");
+		puthex(VMLINUX_LOAD_ADDRESS_ULL + image_size);
+		puts("\n");
+
 		/* copy dtb to where the booted kernel will expect it */
 		memcpy((void *)VMLINUX_LOAD_ADDRESS_ULL + image_size,
 		       __appended_dtb, dtb_size);
diff --git a/arch/mips/kernel/vmlinux.lds.S b/arch/mips/kernel/vmlinux.lds.S
index c1c345be04ff..4b4e39b7c79b 100644
--- a/arch/mips/kernel/vmlinux.lds.S
+++ b/arch/mips/kernel/vmlinux.lds.S
@@ -145,6 +145,7 @@ SECTIONS
 	}
 
 #ifdef CONFIG_MIPS_ELF_APPENDED_DTB
+	STRUCT_ALIGN();
 	.appended_dtb : AT(ADDR(.appended_dtb) - LOAD_OFFSET) {
 		*(.appended_dtb)
 		KEEP(*(.appended_dtb))
@@ -172,6 +173,7 @@ SECTIONS
 #endif
 
 #ifdef CONFIG_MIPS_RAW_APPENDED_DTB
+	STRUCT_ALIGN();
 	__appended_dtb = .;
 	/* leave space for appended DTB */
 	. += 0x100000;
-- 
2.30.1

Re: [PATCH] MIPS: boot/compressed: Copy DTB to aligned address

[Rob Herring]

On Wed, Mar 3, 2021 at 1:33 PM Paul Cercueil <paul@crapouillou.net> wrote:
>
> Since 5.12-rc1, the Device Tree blob must now be properly aligned.

I had checked the other built-in cases as microblaze broke too, but
missed some of the many ways MIPS can have a dtb. Appended and
built-in DTBs were supposed to be temporary. :(

> Therefore, the decompress routine must be careful to copy the blob at
> the next aligned address after the kernel image.
>
> This commit fixes the kernel sometimes not booting with a Device Tree
> blob appended to it.
>
> Fixes: c4d5e638d6e9 ("scripts/dtc: Update to upstream version v1.6.0-51-g183df9e9c2b9")
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  arch/mips/boot/compressed/decompress.c | 8 ++++++++
>  arch/mips/kernel/vmlinux.lds.S         | 2 ++
>  2 files changed, 10 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH] MIPS: boot/compressed: Copy DTB to aligned address

[Paul Cercueil]

Hi Rob,

Le mer. 3 mars 2021 à 14:37, Rob Herring <robh@kernel.org> a écrit :
> On Wed, Mar 3, 2021 at 1:33 PM Paul Cercueil <paul@crapouillou.net> 
> wrote:
>> 
>>  Since 5.12-rc1, the Device Tree blob must now be properly aligned.
> 
> I had checked the other built-in cases as microblaze broke too, but
> missed some of the many ways MIPS can have a dtb. Appended and
> built-in DTBs were supposed to be temporary. :(

Actually I'm glad these options are here, they make debugging much 
easier, when working on new SoCs.

-Paul

>>  Therefore, the decompress routine must be careful to copy the blob 
>> at
>>  the next aligned address after the kernel image.
>> 
>>  This commit fixes the kernel sometimes not booting with a Device 
>> Tree
>>  blob appended to it.
>> 
>>  Fixes: c4d5e638d6e9 ("scripts/dtc: Update to upstream version 
>> v1.6.0-51-g183df9e9c2b9")
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   arch/mips/boot/compressed/decompress.c | 8 ++++++++
>>   arch/mips/kernel/vmlinux.lds.S         | 2 ++
>>   2 files changed, 10 insertions(+)
> 
> Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH] MIPS: boot/compressed: Copy DTB to aligned address

[Maciej W. Rozycki]

On Wed, 3 Mar 2021, Rob Herring wrote:

> > Since 5.12-rc1, the Device Tree blob must now be properly aligned.
> 
> I had checked the other built-in cases as microblaze broke too, but
> missed some of the many ways MIPS can have a dtb. Appended and
> built-in DTBs were supposed to be temporary. :(

 How is it supposed to work otherwise when all that a piece of firmware 
loads is an SREC image (over TFTP)?

  Maciej

Re: [PATCH] MIPS: boot/compressed: Copy DTB to aligned address

[Thomas Bogendoerfer]

On Wed, Mar 03, 2021 at 02:37:55PM -0600, Rob Herring wrote:
> On Wed, Mar 3, 2021 at 1:33 PM Paul Cercueil <paul@crapouillou.net> wrote:
> >
> > Since 5.12-rc1, the Device Tree blob must now be properly aligned.
> 
> I had checked the other built-in cases as microblaze broke too, but
> missed some of the many ways MIPS can have a dtb. Appended and
> built-in DTBs were supposed to be temporary. :(

and a fdt can also be provided by firmware. And according to spec
there is no aligmnet requirement. So this whole change will break
then. What was the reason for the whole churn ?

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] MIPS: boot/compressed: Copy DTB to aligned address

[Rob Herring]

On Sat, Mar 6, 2021 at 1:45 AM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:
>
> On Wed, Mar 03, 2021 at 02:37:55PM -0600, Rob Herring wrote:
> > On Wed, Mar 3, 2021 at 1:33 PM Paul Cercueil <paul@crapouillou.net> wrote:
> > >
> > > Since 5.12-rc1, the Device Tree blob must now be properly aligned.
> >
> > I had checked the other built-in cases as microblaze broke too, but
> > missed some of the many ways MIPS can have a dtb. Appended and
> > built-in DTBs were supposed to be temporary. :(
>
> and a fdt can also be provided by firmware. And according to spec
> there is no aligmnet requirement. So this whole change will break
> then. What was the reason for the whole churn ?

There was a long discussion on devicetree-compiler list a few months
ago. In summary, a while back libfdt switched to accessors from raw
pointer accesses to avoid any possible unaligned accesses (is MIPS
always okay with unaligned accesses?). This was determined to be a
performance regression and an overkill as the DT structure itself
should always be naturally aligned if the dtb is 64-bit aligned. I
think 32-bit aligned has some possible misaligned accesses.

As part of this, a dtb alignment check was added. So worst case, we
could disable that if need be.

Rob

Re: [PATCH] MIPS: boot/compressed: Copy DTB to aligned address

[Thomas Bogendoerfer]

On Sat, Mar 06, 2021 at 02:35:21PM -0700, Rob Herring wrote:
> On Sat, Mar 6, 2021 at 1:45 AM Thomas Bogendoerfer
> <tsbogend@alpha.franken.de> wrote:
> >
> > On Wed, Mar 03, 2021 at 02:37:55PM -0600, Rob Herring wrote:
> > > On Wed, Mar 3, 2021 at 1:33 PM Paul Cercueil <paul@crapouillou.net> wrote:
> > > >
> > > > Since 5.12-rc1, the Device Tree blob must now be properly aligned.
> > >
> > > I had checked the other built-in cases as microblaze broke too, but
> > > missed some of the many ways MIPS can have a dtb. Appended and
> > > built-in DTBs were supposed to be temporary. :(
> >
> > and a fdt can also be provided by firmware. And according to spec
> > there is no aligmnet requirement. So this whole change will break
> > then. What was the reason for the whole churn ?
> 
> There was a long discussion on devicetree-compiler list a few months
> ago. In summary, a while back libfdt switched to accessors from raw
> pointer accesses to avoid any possible unaligned accesses (is MIPS
> always okay with unaligned accesses?).

no, it will trap unaligned accesses, that's the reason for Paul's problem.

> This was determined to be a
> performance regression and an overkill as the DT structure itself
> should always be naturally aligned if the dtb is 64-bit aligned. I
> think 32-bit aligned has some possible misaligned accesses.

the access macros are using *(unsigned long long *), which isn't
even nice for 32bit CPUs...

> As part of this, a dtb alignment check was added. So worst case, we
> could disable that if need be.

yeah, or override fdt32/64_to_cpu, if I understood the code correctly.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] MIPS: boot/compressed: Copy DTB to aligned address

[Thomas Bogendoerfer]

On Wed, Mar 03, 2021 at 07:33:05PM +0000, Paul Cercueil wrote:
> Since 5.12-rc1, the Device Tree blob must now be properly aligned.
> 
> Therefore, the decompress routine must be careful to copy the blob at
> the next aligned address after the kernel image.
> 
> This commit fixes the kernel sometimes not booting with a Device Tree
> blob appended to it.
> 
> Fixes: c4d5e638d6e9 ("scripts/dtc: Update to upstream version v1.6.0-51-g183df9e9c2b9")
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  arch/mips/boot/compressed/decompress.c | 8 ++++++++
>  arch/mips/kernel/vmlinux.lds.S         | 2 ++
>  2 files changed, 10 insertions(+)

applied to mips-fixes.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] MIPS: boot/compressed: Copy DTB to aligned address

[Rob Herring]

On Sat, Mar 6, 2021 at 3:59 PM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:
>
> On Sat, Mar 06, 2021 at 02:35:21PM -0700, Rob Herring wrote:
> > On Sat, Mar 6, 2021 at 1:45 AM Thomas Bogendoerfer
> > <tsbogend@alpha.franken.de> wrote:
> > >
> > > On Wed, Mar 03, 2021 at 02:37:55PM -0600, Rob Herring wrote:
> > > > On Wed, Mar 3, 2021 at 1:33 PM Paul Cercueil <paul@crapouillou.net> wrote:
> > > > >
> > > > > Since 5.12-rc1, the Device Tree blob must now be properly aligned.
> > > >
> > > > I had checked the other built-in cases as microblaze broke too, but
> > > > missed some of the many ways MIPS can have a dtb. Appended and
> > > > built-in DTBs were supposed to be temporary. :(
> > >
> > > and a fdt can also be provided by firmware. And according to spec
> > > there is no aligmnet requirement. So this whole change will break
> > > then. What was the reason for the whole churn ?

Actually, that is wrong. The spec defines the alignment (from
flattened format appendix):

"Alignment

For the data in the memory reservation and structure blocks to be used
without unaligned memory accesses, they shall lie at suitably aligned
memory addresses. Specifically, the memory reservation block shall be
aligned to an 8-byte boundary and the structure block to a 4-byte
boundary.

Furthermore, the devicetree blob as a whole can be relocated without
destroying the alignment of the subblocks.

As described in the previous sections, the structure and strings
blocks shall have aligned offsets from the beginning of the devicetree
blob. To ensure the in-memory alignment of the blocks, it is
sufficient to ensure that the devicetree as a whole is loaded at an
address aligned to the largest alignment of any of the subblocks, that
is, to an 8-byte boundary. A |spec| compliant boot program shall load
the devicetree blob at such an aligned address before passing it to
the client program. If an |spec| client program relocates the
devicetree blob in memory, it should only do so to another 8-byte
aligned address."


> > There was a long discussion on devicetree-compiler list a few months
> > ago. In summary, a while back libfdt switched to accessors from raw
> > pointer accesses to avoid any possible unaligned accesses (is MIPS
> > always okay with unaligned accesses?).
>
> no, it will trap unaligned accesses, that's the reason for Paul's problem.
>
> > This was determined to be a
> > performance regression and an overkill as the DT structure itself
> > should always be naturally aligned if the dtb is 64-bit aligned. I
> > think 32-bit aligned has some possible misaligned accesses.
>
> the access macros are using *(unsigned long long *), which isn't
> even nice for 32bit CPUs...

Where are those?

> > As part of this, a dtb alignment check was added. So worst case, we
> > could disable that if need be.
>
> yeah, or override fdt32/64_to_cpu, if I understood the code correctly.

No, fdt32/64_to_cpu don't dereference the pointer.

Rob

Re: [PATCH] MIPS: boot/compressed: Copy DTB to aligned address

[Thomas Bogendoerfer]

On Mon, Mar 08, 2021 at 10:04:15AM -0700, Rob Herring wrote:
> On Sat, Mar 6, 2021 at 3:59 PM Thomas Bogendoerfer
> <tsbogend@alpha.franken.de> wrote:
> >
> > On Sat, Mar 06, 2021 at 02:35:21PM -0700, Rob Herring wrote:
> > > On Sat, Mar 6, 2021 at 1:45 AM Thomas Bogendoerfer
> > > <tsbogend@alpha.franken.de> wrote:
> > > >
> > > > On Wed, Mar 03, 2021 at 02:37:55PM -0600, Rob Herring wrote:
> > > > > On Wed, Mar 3, 2021 at 1:33 PM Paul Cercueil <paul@crapouillou.net> wrote:
> > > > > >
> > > > > > Since 5.12-rc1, the Device Tree blob must now be properly aligned.
> > > > >
> > > > > I had checked the other built-in cases as microblaze broke too, but
> > > > > missed some of the many ways MIPS can have a dtb. Appended and
> > > > > built-in DTBs were supposed to be temporary. :(
> > > >
> > > > and a fdt can also be provided by firmware. And according to spec
> > > > there is no aligmnet requirement. So this whole change will break
> > > > then. What was the reason for the whole churn ?
> 
> Actually, that is wrong. The spec defines the alignment (from
> flattened format appendix):

I was talking about the "Unified Hosting Interface" from MIPS/Imagination.
As the spec talks about device tree blob all firmware developer knew
about the fdt alignment rules.

> > the access macros are using *(unsigned long long *), which isn't
> > even nice for 32bit CPUs...
> 
> Where are those?

nowhere, I've missread the code in libfdt_env.h

> > > As part of this, a dtb alignment check was added. So worst case, we
> > > could disable that if need be.
> >
> > yeah, or override fdt32/64_to_cpu, if I understood the code correctly.
> 
> No, fdt32/64_to_cpu don't dereference the pointer.

you are right, brainfart on my side.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]