| Message ID | 20210307182301.20710-1-bjorn@mork.no (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | MIPS: vmlinux.lds.S: align raw appended dtb to 8 bytes | expand |
On Sun, Mar 07, 2021 at 07:23:01PM +0100, Bjørn Mork wrote: > The devicetree specification requires 8-byte alignment in > memory. This is now enforced by libfdt since commit 79edff12060f > ("scripts/dtc: Update to upstream version v1.6.0-51-g183df9e9c2b9") > which included the upstream commit 5e735860c478 ("libfdt: Check for > 8-byte address alignment in fdt_ro_probe_()"). > > This broke the MIPS raw appended DTBs which would be appended to > the image immediately following the initramfs section. This ends > with a 32bit size, resulting in a 4-byte alignment of the DTB. > > Fix by padding with zeroes to 8-bytes when MIPS_RAW_APPENDED_DTB > is defined. > > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Frank Rowand <frowand.list@gmail.com> > Signed-off-by: Bjørn Mork <bjorn@mork.no> > --- > arch/mips/kernel/vmlinux.lds.S | 5 +++++ > 1 file changed, 5 insertions(+) thank you for your patch, but there already was a fix for the problem pending from Paul, which I've applied to mips-fixes a few minutes ago. Thomas.
Thomas Bogendoerfer <tsbogend@alpha.franken.de> writes: > On Sun, Mar 07, 2021 at 07:23:01PM +0100, Bjørn Mork wrote: >> The devicetree specification requires 8-byte alignment in >> memory. This is now enforced by libfdt since commit 79edff12060f >> ("scripts/dtc: Update to upstream version v1.6.0-51-g183df9e9c2b9") >> which included the upstream commit 5e735860c478 ("libfdt: Check for >> 8-byte address alignment in fdt_ro_probe_()"). >> >> This broke the MIPS raw appended DTBs which would be appended to >> the image immediately following the initramfs section. This ends >> with a 32bit size, resulting in a 4-byte alignment of the DTB. >> >> Fix by padding with zeroes to 8-bytes when MIPS_RAW_APPENDED_DTB >> is defined. >> >> Cc: Rob Herring <robh+dt@kernel.org> >> Cc: Frank Rowand <frowand.list@gmail.com> >> Signed-off-by: Bjørn Mork <bjorn@mork.no> >> --- >> arch/mips/kernel/vmlinux.lds.S | 5 +++++ >> 1 file changed, 5 insertions(+) > > thank you for your patch, but there already was a fix for the problem > pending from Paul, which I've applied to mips-fixes a few minutes ago. Yes, I see. That does look much nicer. But I don't think it addresses the problem with an uncompressed kernel? Could we have the padding in vmlinux.lds.S as well? Or some other solution to ensure that it is possible to cat the DTB to the end of vmlinux.bin without manually aligning it to 8 bytes? Bjørn
On Mon, Mar 08, 2021 at 12:10:33PM +0100, Bjørn Mork wrote: > Thomas Bogendoerfer <tsbogend@alpha.franken.de> writes: > > > On Sun, Mar 07, 2021 at 07:23:01PM +0100, Bjørn Mork wrote: > >> The devicetree specification requires 8-byte alignment in > >> memory. This is now enforced by libfdt since commit 79edff12060f > >> ("scripts/dtc: Update to upstream version v1.6.0-51-g183df9e9c2b9") > >> which included the upstream commit 5e735860c478 ("libfdt: Check for > >> 8-byte address alignment in fdt_ro_probe_()"). > >> > >> This broke the MIPS raw appended DTBs which would be appended to > >> the image immediately following the initramfs section. This ends > >> with a 32bit size, resulting in a 4-byte alignment of the DTB. > >> > >> Fix by padding with zeroes to 8-bytes when MIPS_RAW_APPENDED_DTB > >> is defined. > >> > >> Cc: Rob Herring <robh+dt@kernel.org> > >> Cc: Frank Rowand <frowand.list@gmail.com> > >> Signed-off-by: Bjørn Mork <bjorn@mork.no> > >> --- > >> arch/mips/kernel/vmlinux.lds.S | 5 +++++ > >> 1 file changed, 5 insertions(+) > > > > thank you for your patch, but there already was a fix for the problem > > pending from Paul, which I've applied to mips-fixes a few minutes ago. > > Yes, I see. That does look much nicer. But I don't think it addresses > the problem with an uncompressed kernel? Could we have the padding in > vmlinux.lds.S as well? Or some other solution to ensure that it is > possible to cat the DTB to the end of vmlinux.bin without manually > aligning it to 8 bytes? I see 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; in that patch, and IMHO this does align the appended_dtb. What do I miss ? Thomas.
Thomas Bogendoerfer <tsbogend@alpha.franken.de> writes: > I see > > 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; > > in that patch, and IMHO this does align the appended_dtb. What do I miss ? I'll not pretend I know anything about this subject, so feel free to adjust as necessary. The problem with that patch is that it doesn't pad the image to the aligment. So you can't do cat my.dtb >> arch/mips/boot/vmlinux.bin anymore. This used to work before commit 79edff12060f. This is an image build with that patch and an initramfs: bjorn@canardo:/usr/local/src/build-tmp/linux$ ls -l arch/mips/boot/vmlinux.bin -rwxr-xr-x 1 bjorn src 15724964 Mar 8 14:21 arch/mips/boot/vmlinux.bin So that's aligned to 4 bytes, because it's terminated by the 32bit length of the initramfs: 15724964/8 1965620.50000000000000000000 Building with the attached patch results in: bjorn@canardo:/usr/local/src/build-tmp/linux$ ls -l arch/mips/boot/vmlinux.bin -rwxr-xr-x 1 bjorn src 15724992 Mar 8 14:29 arch/mips/boot/vmlinux.bin Which is aligned to the 32 bytes expected after STRUCT_ALIGN(): 15724992/8 1965624.00000000000000000000 The tail of the image shows the cpio trailer and cpio length (0x0061f400) followed by enough padding to align an appended DTB to 32 bytes: bjorn@canardo:/usr/local/src/build-tmp/linux$ ls -l arch/mips/boot/vmlinux.bin -rwxr-xr-x 1 bjorn src 15724992 Mar 8 14:29 arch/mips/boot/vmlinux.bin bjorn@canardo:/usr/local/src/build-tmp/linux$ hexdump -C arch/mips/boot/vmlinux.bin|tail 00eff090 30 30 30 30 30 30 30 30 30 31 30 30 30 30 30 30 |0000000001000000| 00eff0a0 30 30 30 30 30 30 30 30 30 30 30 30 30 30 30 30 |0000000000000000| * 00eff0d0 30 42 30 30 30 30 30 30 30 30 54 52 41 49 4c 45 |0B00000000TRAILE| 00eff0e0 52 21 21 21 00 00 00 00 00 00 00 00 00 00 00 00 |R!!!............| 00eff0f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................| * 00eff1a0 00 61 f4 00 00 00 00 00 00 00 00 00 00 00 00 00 |.a..............| 00eff1b0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................| 00eff1c0 Yes, you can always pad the image yourself if you know about this alignment requirement. But it gets more complicated. And it breaks my home grown hackish build script. I know I'm not the only one... Bjørn
On Mon, Mar 08, 2021 at 02:45:57PM +0100, Bjørn Mork wrote: > Thomas Bogendoerfer <tsbogend@alpha.franken.de> writes: > > > I see > > > > 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; > > > > in that patch, and IMHO this does align the appended_dtb. What do I miss ? > > I'll not pretend I know anything about this subject, so feel free to > adjust as necessary. > > The problem with that patch is that it doesn't pad the image to the > aligment. So you can't do > > cat my.dtb >> arch/mips/boot/vmlinux.bin > > anymore. This used to work before commit 79edff12060f. ok, took a little while to fully understand the problem. I've applied your patch to mips-fixes with a Fixes: tag added. Thomas.
diff --git a/arch/mips/kernel/vmlinux.lds.S b/arch/mips/kernel/vmlinux.lds.S index c1c345be04ff..850117efb09b 100644 --- a/arch/mips/kernel/vmlinux.lds.S +++ b/arch/mips/kernel/vmlinux.lds.S @@ -172,6 +172,11 @@ SECTIONS #endif #ifdef CONFIG_MIPS_RAW_APPENDED_DTB + .fill : { + FILL(0); + BYTE(0); + . = ALIGN(8); + } __appended_dtb = .; /* leave space for appended DTB */ . += 0x100000;
The devicetree specification requires 8-byte alignment in memory. This is now enforced by libfdt since commit 79edff12060f ("scripts/dtc: Update to upstream version v1.6.0-51-g183df9e9c2b9") which included the upstream commit 5e735860c478 ("libfdt: Check for 8-byte address alignment in fdt_ro_probe_()"). This broke the MIPS raw appended DTBs which would be appended to the image immediately following the initramfs section. This ends with a 32bit size, resulting in a 4-byte alignment of the DTB. Fix by padding with zeroes to 8-bytes when MIPS_RAW_APPENDED_DTB is defined. Cc: Rob Herring <robh+dt@kernel.org> Cc: Frank Rowand <frowand.list@gmail.com> Signed-off-by: Bjørn Mork <bjorn@mork.no> --- arch/mips/kernel/vmlinux.lds.S | 5 +++++ 1 file changed, 5 insertions(+)