| Message ID | 20210303193305.924384-1-paul@crapouillou.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7a05293af39fc716d0f51c0164cbb727302396a2 |
| Headers | show |
| Series | MIPS: boot/compressed: Copy DTB to aligned address | expand |
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>
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
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.
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
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.
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.
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
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.
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;
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(+)