| Message ID | 20171018050108.10352-1-jeffy.chen@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Tested-by: Chris Zhong <zyw@rock-chips.com> On Wednesday, October 18, 2017 01:01 PM, Jeffy Chen wrote: > The zImage file size should be aligned. > > Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections") > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> > --- > > arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S > index b38dcef90756..1636fa259577 100644 > --- a/arch/arm/boot/compressed/vmlinux.lds.S > +++ b/arch/arm/boot/compressed/vmlinux.lds.S > @@ -70,10 +70,6 @@ SECTIONS > .got : { *(.got) } > _got_end = .; > > - /* ensure the zImage file size is always a multiple of 64 bits */ > - /* (without a dummy byte, ld just ignores the empty section) */ > - .pad : { BYTE(0); . = ALIGN(8); } > - > #ifdef CONFIG_EFI_STUB > .data : ALIGN(4096) { > __pecoff_data_start = .; > @@ -93,6 +89,10 @@ SECTIONS > __pecoff_data_rawsize = . - ADDR(.data); > #endif > > + /* ensure the zImage file size is always a multiple of 64 bits */ > + /* (without a dummy byte, ld just ignores the empty section) */ > + .pad : { BYTE(0); . = ALIGN(8); } > + > _edata = .; > > _magic_sig = ZIMAGE_MAGIC(0x016f2818);
On 18 October 2017 at 06:01, Jeffy Chen <jeffy.chen@rock-chips.com> wrote: > The zImage file size should be aligned. > > Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections") > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> > --- > > arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S > index b38dcef90756..1636fa259577 100644 > --- a/arch/arm/boot/compressed/vmlinux.lds.S > +++ b/arch/arm/boot/compressed/vmlinux.lds.S > @@ -70,10 +70,6 @@ SECTIONS > .got : { *(.got) } > _got_end = .; > > - /* ensure the zImage file size is always a multiple of 64 bits */ > - /* (without a dummy byte, ld just ignores the empty section) */ > - .pad : { BYTE(0); . = ALIGN(8); } > - > #ifdef CONFIG_EFI_STUB > .data : ALIGN(4096) { > __pecoff_data_start = .; > @@ -93,6 +89,10 @@ SECTIONS > __pecoff_data_rawsize = . - ADDR(.data); > #endif > > + /* ensure the zImage file size is always a multiple of 64 bits */ > + /* (without a dummy byte, ld just ignores the empty section) */ > + .pad : { BYTE(0); . = ALIGN(8); } > + > _edata = .; > > _magic_sig = ZIMAGE_MAGIC(0x016f2818); > -- > 2.11.0 > This is not the right fix. If CONFIG_EFI_STUB is enabled, the zImage filesize should be rounded up to 512 bytes not 8 bytes. The '. = ALIGN(512);' in the .data section appears to ensure that, but for some reason, that appears not to be working. Could you share the output of $ readelf -S arch/arm/boot/compressed/vmlinux please? And could you please check whether this patch https://marc.info/?l=linux-arm-kernel&m=150488477807353 fixes the issue for you? Thanks, Ard.
On Sun, Oct 22, 2017 at 12:01:13PM +0100, Ard Biesheuvel wrote: > On 18 October 2017 at 06:01, Jeffy Chen <jeffy.chen@rock-chips.com> wrote: > > The zImage file size should be aligned. > > > > Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections") > > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> > > --- > > > > arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S > > index b38dcef90756..1636fa259577 100644 > > --- a/arch/arm/boot/compressed/vmlinux.lds.S > > +++ b/arch/arm/boot/compressed/vmlinux.lds.S > > @@ -70,10 +70,6 @@ SECTIONS > > .got : { *(.got) } > > _got_end = .; > > > > - /* ensure the zImage file size is always a multiple of 64 bits */ > > - /* (without a dummy byte, ld just ignores the empty section) */ > > - .pad : { BYTE(0); . = ALIGN(8); } > > - > > #ifdef CONFIG_EFI_STUB > > .data : ALIGN(4096) { > > __pecoff_data_start = .; > > @@ -93,6 +89,10 @@ SECTIONS > > __pecoff_data_rawsize = . - ADDR(.data); > > #endif > > > > + /* ensure the zImage file size is always a multiple of 64 bits */ > > + /* (without a dummy byte, ld just ignores the empty section) */ > > + .pad : { BYTE(0); . = ALIGN(8); } > > + > > _edata = .; > > > > _magic_sig = ZIMAGE_MAGIC(0x016f2818); > > -- > > 2.11.0 > > > > This is not the right fix. If CONFIG_EFI_STUB is enabled, the zImage > filesize should be rounded up to 512 bytes not 8 bytes. The '. = > ALIGN(512);' in the .data section appears to ensure that, but for some > reason, that appears not to be working. Actually, the existing .pad section is totally and utterly bogus when EFI is enabled: . = ALIGN(4); _etext = .; .got.plt : { *(.got.plt) } _got_start = .; .got : { *(.got) } _got_end = .; The .got.plt and .got are always word-based. This is then followed by .pad, which does nothing but pad out to a multiple of 64 bit: /* ensure the zImage file size is always a multiple of 64 bits */ /* (without a dummy byte, ld just ignores the empty section) */ .pad : { BYTE(0); . = ALIGN(8); } So this may add zero or 4 bytes of padding. This is then followed by the EFI data: .data : ALIGN(4096) { ... . = ALIGN(512); } which is aligned to 4K but aligns the end of itself to 512. So, we have the end of .got aligned to 4, followed by .pad that tries to align to 8, followed by an optional .data section. This is pointless. A sane patch would be to choose between the EFI .data section and the .pad section. So, it should be: #ifdef CONFIG_EFI_STUB .data : ALIGN(4096) { ... . = ALIGN(512); } #else .pad : { BYTE(0); . = ALIGN(8); } #endif
On 22 October 2017 at 13:47, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Sun, Oct 22, 2017 at 12:01:13PM +0100, Ard Biesheuvel wrote: >> On 18 October 2017 at 06:01, Jeffy Chen <jeffy.chen@rock-chips.com> wrote: >> > The zImage file size should be aligned. >> > >> > Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections") >> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> >> > --- >> > >> > arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++---- >> > 1 file changed, 4 insertions(+), 4 deletions(-) >> > >> > diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S >> > index b38dcef90756..1636fa259577 100644 >> > --- a/arch/arm/boot/compressed/vmlinux.lds.S >> > +++ b/arch/arm/boot/compressed/vmlinux.lds.S >> > @@ -70,10 +70,6 @@ SECTIONS >> > .got : { *(.got) } >> > _got_end = .; >> > >> > - /* ensure the zImage file size is always a multiple of 64 bits */ >> > - /* (without a dummy byte, ld just ignores the empty section) */ >> > - .pad : { BYTE(0); . = ALIGN(8); } >> > - >> > #ifdef CONFIG_EFI_STUB >> > .data : ALIGN(4096) { >> > __pecoff_data_start = .; >> > @@ -93,6 +89,10 @@ SECTIONS >> > __pecoff_data_rawsize = . - ADDR(.data); >> > #endif >> > >> > + /* ensure the zImage file size is always a multiple of 64 bits */ >> > + /* (without a dummy byte, ld just ignores the empty section) */ >> > + .pad : { BYTE(0); . = ALIGN(8); } >> > + >> > _edata = .; >> > >> > _magic_sig = ZIMAGE_MAGIC(0x016f2818); >> > -- >> > 2.11.0 >> > >> >> This is not the right fix. If CONFIG_EFI_STUB is enabled, the zImage >> filesize should be rounded up to 512 bytes not 8 bytes. The '. = >> ALIGN(512);' in the .data section appears to ensure that, but for some >> reason, that appears not to be working. > > Actually, the existing .pad section is totally and utterly bogus when > EFI is enabled: > > . = ALIGN(4); > _etext = .; > > .got.plt : { *(.got.plt) } > _got_start = .; > .got : { *(.got) } > _got_end = .; > > The .got.plt and .got are always word-based. This is then followed by > .pad, which does nothing but pad out to a multiple of 64 bit: > > /* ensure the zImage file size is always a multiple of 64 bits */ > /* (without a dummy byte, ld just ignores the empty section) */ > .pad : { BYTE(0); . = ALIGN(8); } > > So this may add zero or 4 bytes of padding. > > This is then followed by the EFI data: > > .data : ALIGN(4096) { > ... > . = ALIGN(512); > } > > which is aligned to 4K but aligns the end of itself to 512. > > So, we have the end of .got aligned to 4, followed by .pad that tries to > align to 8, followed by an optional .data section. This is pointless. > > A sane patch would be to choose between the EFI .data section and the > .pad section. So, it should be: > > #ifdef CONFIG_EFI_STUB > .data : ALIGN(4096) { > ... > . = ALIGN(512); > } > #else > .pad : { BYTE(0); . = ALIGN(8); } > #endif > Agreed, the .pad section has no point for EFI_STUB=y. However, it seems this symptom is caused by the same issues I am trying to address here https://marc.info/?l=linux-arm-kernel&m=150488477807353 which is that we have __ksymtab_xxx sections that we should discard, because the linker will otherwise emit them /after/ .data or .pad. This is caused by the use of lib/sort.c in the EFI stub, which contains an EXPORT_SYMBOL(). Would you perhaps prefer that I clone sort.c into its own .c file specifically for the EFI stub? (under drivers/firmware/efi/libstub) That should get rid of these spurious sections and thus the misalignments and/or movements that are causing all of these issues.
Hi Ard, On 10/22/2017 09:01 PM, Ard Biesheuvel wrote: > On 22 October 2017 at 13:47, Russell King - ARM Linux > <linux@armlinux.org.uk> wrote: >> On Sun, Oct 22, 2017 at 12:01:13PM +0100, Ard Biesheuvel wrote: >>> On 18 October 2017 at 06:01, Jeffy Chen <jeffy.chen@rock-chips.com> wrote: >>>> The zImage file size should be aligned. >>>> >>>> Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections") >>>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> >>>> --- >>>> >>>> arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++---- >>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S >>>> index b38dcef90756..1636fa259577 100644 >>>> --- a/arch/arm/boot/compressed/vmlinux.lds.S >>>> +++ b/arch/arm/boot/compressed/vmlinux.lds.S >>>> @@ -70,10 +70,6 @@ SECTIONS >>>> .got : { *(.got) } >>>> _got_end = .; >>>> >>>> - /* ensure the zImage file size is always a multiple of 64 bits */ >>>> - /* (without a dummy byte, ld just ignores the empty section) */ >>>> - .pad : { BYTE(0); . = ALIGN(8); } >>>> - >>>> #ifdef CONFIG_EFI_STUB >>>> .data : ALIGN(4096) { >>>> __pecoff_data_start = .; >>>> @@ -93,6 +89,10 @@ SECTIONS >>>> __pecoff_data_rawsize = . - ADDR(.data); >>>> #endif >>>> >>>> + /* ensure the zImage file size is always a multiple of 64 bits */ >>>> + /* (without a dummy byte, ld just ignores the empty section) */ >>>> + .pad : { BYTE(0); . = ALIGN(8); } >>>> + >>>> _edata = .; >>>> >>>> _magic_sig = ZIMAGE_MAGIC(0x016f2818); >>>> -- >>>> 2.11.0 >>>> >>> >>> This is not the right fix. If CONFIG_EFI_STUB is enabled, the zImage >>> filesize should be rounded up to 512 bytes not 8 bytes. The '. = >>> ALIGN(512);' in the .data section appears to ensure that, but for some >>> reason, that appears not to be working. >> >> Actually, the existing .pad section is totally and utterly bogus when >> EFI is enabled: >> >> . = ALIGN(4); >> _etext = .; >> >> .got.plt : { *(.got.plt) } >> _got_start = .; >> .got : { *(.got) } >> _got_end = .; >> >> The .got.plt and .got are always word-based. This is then followed by >> .pad, which does nothing but pad out to a multiple of 64 bit: >> >> /* ensure the zImage file size is always a multiple of 64 bits */ >> /* (without a dummy byte, ld just ignores the empty section) */ >> .pad : { BYTE(0); . = ALIGN(8); } >> >> So this may add zero or 4 bytes of padding. >> >> This is then followed by the EFI data: >> >> .data : ALIGN(4096) { >> ... >> . = ALIGN(512); >> } >> >> which is aligned to 4K but aligns the end of itself to 512. >> >> So, we have the end of .got aligned to 4, followed by .pad that tries to >> align to 8, followed by an optional .data section. This is pointless. >> >> A sane patch would be to choose between the EFI .data section and the >> .pad section. So, it should be: >> >> #ifdef CONFIG_EFI_STUB >> .data : ALIGN(4096) { >> ... >> . = ALIGN(512); >> } >> #else >> .pad : { BYTE(0); . = ALIGN(8); } >> #endif >> > > Agreed, the .pad section has no point for EFI_STUB=y. However, it > seems this symptom is caused by the same issues I am trying to address > here > > https://marc.info/?l=linux-arm-kernel&m=150488477807353 > > which is that we have __ksymtab_xxx sections that we should discard, > because the linker will otherwise emit them /after/ .data or .pad. > This is caused by the use of lib/sort.c in the EFI stub, which > contains an EXPORT_SYMBOL(). hmm, right, didn't notice the data is already aligned... so it's indeed caused by the ksym: [ 9] .data PROGBITS 006ce000 6d6000 000200 00 WA 0 0 4096 [10] ___ksymtab+sort PROGBITS 006ce200 6d6200 000008 00 WA 0 0 4 [11] .bss NOBITS 006ce208 6d6208 00001c 00 WA 0 0 4 and both of your old([PATCH] ARM: compressed: discard ksym/kcrctab input section) and new([PATCH] efi/libstub: arm: omit sorting of the UEFI memory map) patches fix the issue i meet, thanks:) > > Would you perhaps prefer that I clone sort.c into its own .c file > specifically for the EFI stub? (under drivers/firmware/efi/libstub) > That should get rid of these spurious sections and thus the > misalignments and/or movements that are causing all of these issues. > > >
On Mon, Oct 23, 2017 at 11:26:49AM +0800, jeffy wrote: > Hi Ard, > > On 10/22/2017 09:01 PM, Ard Biesheuvel wrote: > >On 22 October 2017 at 13:47, Russell King - ARM Linux > ><linux@armlinux.org.uk> wrote: > >>On Sun, Oct 22, 2017 at 12:01:13PM +0100, Ard Biesheuvel wrote: > >>>On 18 October 2017 at 06:01, Jeffy Chen <jeffy.chen@rock-chips.com> wrote: > >>>>The zImage file size should be aligned. > >>>> > >>>>Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections") > >>>>Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> > >>>>--- > >>>> > >>>> arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++---- > >>>> 1 file changed, 4 insertions(+), 4 deletions(-) > >>>> > >>>>diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S > >>>>index b38dcef90756..1636fa259577 100644 > >>>>--- a/arch/arm/boot/compressed/vmlinux.lds.S > >>>>+++ b/arch/arm/boot/compressed/vmlinux.lds.S > >>>>@@ -70,10 +70,6 @@ SECTIONS > >>>> .got : { *(.got) } > >>>> _got_end = .; > >>>> > >>>>- /* ensure the zImage file size is always a multiple of 64 bits */ > >>>>- /* (without a dummy byte, ld just ignores the empty section) */ > >>>>- .pad : { BYTE(0); . = ALIGN(8); } > >>>>- > >>>> #ifdef CONFIG_EFI_STUB > >>>> .data : ALIGN(4096) { > >>>> __pecoff_data_start = .; > >>>>@@ -93,6 +89,10 @@ SECTIONS > >>>> __pecoff_data_rawsize = . - ADDR(.data); > >>>> #endif > >>>> > >>>>+ /* ensure the zImage file size is always a multiple of 64 bits */ > >>>>+ /* (without a dummy byte, ld just ignores the empty section) */ > >>>>+ .pad : { BYTE(0); . = ALIGN(8); } > >>>>+ > >>>> _edata = .; > >>>> > >>>> _magic_sig = ZIMAGE_MAGIC(0x016f2818); > >>>>-- > >>>>2.11.0 > >>>> > >>> > >>>This is not the right fix. If CONFIG_EFI_STUB is enabled, the zImage > >>>filesize should be rounded up to 512 bytes not 8 bytes. The '. = > >>>ALIGN(512);' in the .data section appears to ensure that, but for some > >>>reason, that appears not to be working. > >> > >>Actually, the existing .pad section is totally and utterly bogus when > >>EFI is enabled: > >> > >> . = ALIGN(4); > >> _etext = .; > >> > >> .got.plt : { *(.got.plt) } > >> _got_start = .; > >> .got : { *(.got) } > >> _got_end = .; > >> > >>The .got.plt and .got are always word-based. This is then followed by > >>.pad, which does nothing but pad out to a multiple of 64 bit: > >> > >> /* ensure the zImage file size is always a multiple of 64 bits */ > >> /* (without a dummy byte, ld just ignores the empty section) */ > >> .pad : { BYTE(0); . = ALIGN(8); } > >> > >>So this may add zero or 4 bytes of padding. > >> > >>This is then followed by the EFI data: > >> > >> .data : ALIGN(4096) { > >> ... > >> . = ALIGN(512); > >> } > >> > >>which is aligned to 4K but aligns the end of itself to 512. > >> > >>So, we have the end of .got aligned to 4, followed by .pad that tries to > >>align to 8, followed by an optional .data section. This is pointless. > >> > >>A sane patch would be to choose between the EFI .data section and the > >>.pad section. So, it should be: > >> > >>#ifdef CONFIG_EFI_STUB > >> .data : ALIGN(4096) { > >> ... > >> . = ALIGN(512); > >> } > >>#else > >> .pad : { BYTE(0); . = ALIGN(8); } > >>#endif > >> > > > >Agreed, the .pad section has no point for EFI_STUB=y. However, it > >seems this symptom is caused by the same issues I am trying to address > >here > > > >https://marc.info/?l=linux-arm-kernel&m=150488477807353 > > > >which is that we have __ksymtab_xxx sections that we should discard, > >because the linker will otherwise emit them /after/ .data or .pad. > >This is caused by the use of lib/sort.c in the EFI stub, which > >contains an EXPORT_SYMBOL(). > > hmm, right, didn't notice the data is already aligned... > so it's indeed caused by the ksym: > > [ 9] .data PROGBITS 006ce000 6d6000 000200 00 WA 0 > 0 4096 > [10] ___ksymtab+sort PROGBITS 006ce200 6d6200 000008 00 WA 0 > 0 4 > [11] .bss NOBITS 006ce208 6d6208 00001c 00 WA 0 > 0 4 It's earlier - look for __ksymtab_strings.
Hi Russell, Thanks for your reply. On 10/23/2017 04:50 PM, Russell King - ARM Linux wrote: >> > >> >hmm, right, didn't notice the data is already aligned... >> >so it's indeed caused by the ksym: >> > >> > [ 9] .data PROGBITS 006ce000 6d6000 000200 00 WA 0 >> >0 4096 >> > [10] ___ksymtab+sort PROGBITS 006ce200 6d6200 000008 00 WA 0 >> >0 4 >> > [11] .bss NOBITS 006ce208 6d6208 00001c 00 WA 0 >> >0 4 > It's earlier - look for __ksymtab_strings. the problem i meet is the appended dtb code found dtb invalid. i thought that is because of unaligned zImage size, but i was wrong... it looks like the size is still aligned, and after add more logs, it seems the problem is due to _edata not matched the real file size, which is because of the unexpected ___ksymtab+sort: currently: zImage size is 6d6208: -rwxr-xr-x 1 root root 7135752 Oct 23 18:12 zImage _edata is 006ce200: 006ce200 0 NOTYPE GLOBAL DEFAULT 9 _edata vmlinux sections: Section Headers: [Nr] Name Type Addr Off Size ES Flg Lk Inf Al [ 0] NULL 00000000 000000 000000 00 0 0 0 [ 1] .text PROGBITS 00000000 008000 00b7a0 00 AX 0 0 4096 [ 2] .table PROGBITS 0000b7a0 0137a0 000014 00 WA 0 0 4 [ 3] .rodata PROGBITS 0000b7b4 0137b4 0015ef 00 A 0 0 2 [ 4] __ksymtab_strings PROGBITS 0000cda3 014da3 000005 00 A 0 0 1 [ 5] .piggydata PROGBITS 0000cda8 014da8 6c026f 00 A 0 0 1 [ 6] .got.plt PROGBITS 006cd018 6d5018 00000c 04 WA 0 0 4 [ 7] .got PROGBITS 006cd024 6d5024 000028 00 WA 0 0 4 [ 8] .pad PROGBITS 006cd04c 6d504c 000004 00 WA 0 0 1 [ 9] .data PROGBITS 006ce000 6d6000 000200 00 WA 0 0 4096 [10] ___ksymtab+sort PROGBITS 006ce200 6d6200 000008 00 WA 0 0 4 [11] .bss NOBITS 006ce208 6d6208 00001c 00 WA 0 0 4 and it turns out moving around .pad section only hide the problem by placing the .pad after the ___ksymtab+sort: Section Headers: [Nr] Name Type Addr Off Size ES Flg Lk Inf Al [ 0] NULL 00000000 000000 000000 00 0 0 0 [ 1] .text PROGBITS 00000000 008000 00b7a0 00 AX 0 0 4096 [ 2] .table PROGBITS 0000b7a0 0137a0 000014 00 WA 0 0 4 [ 3] .rodata PROGBITS 0000b7b4 0137b4 0015ef 00 A 0 0 2 [ 4] __ksymtab_strings PROGBITS 0000cda3 014da3 000005 00 A 0 0 1 [ 5] .piggydata PROGBITS 0000cda8 014da8 6c026f 00 A 0 0 1 [ 6] .got.plt PROGBITS 006cd018 6d5018 00000c 04 WA 0 0 4 [ 7] .got PROGBITS 006cd024 6d5024 000028 00 WA 0 0 4 [ 8] .data PROGBITS 006ce000 6d6000 000200 00 WA 0 0 4096 [ 9] ___ksymtab+sort PROGBITS 006ce200 6d6200 000008 00 WA 0 0 4 [10] .pad PROGBITS 006ce208 6d6208 000008 00 WA 0 0 1 [11] .bss NOBITS 006ce210 6d6210 00001c 00 WA 0 0 4 -rwxr-xr-x 1 root root 7135760 Oct 23 18:09 zImage 006ce210 0 NOTYPE GLOBAL DEFAULT 10 _edata and i think Ard's new patch could be the right way to fix it :)
On Mon, Oct 23, 2017 at 06:24:12PM +0800, jeffy wrote: > Hi Russell, > > Thanks for your reply. > > On 10/23/2017 04:50 PM, Russell King - ARM Linux wrote: > >>> > >>>hmm, right, didn't notice the data is already aligned... > >>>so it's indeed caused by the ksym: > >>> > >>> [ 9] .data PROGBITS 006ce000 6d6000 000200 00 WA 0 > >>>0 4096 > >>> [10] ___ksymtab+sort PROGBITS 006ce200 6d6200 000008 00 WA 0 > >>>0 4 > >>> [11] .bss NOBITS 006ce208 6d6208 00001c 00 WA 0 > >>>0 4 > >It's earlier - look for __ksymtab_strings. > > the problem i meet is the appended dtb code found dtb invalid. i thought > that is because of unaligned zImage size, but i was wrong... Hmm, you really ought not to be using the appended dtb code for modern systems - the appended dtb system is there for old boot loaders that are incapable of dealing with a dtb. As is said in the option's help text: This is meant as a backward compatibility convenience for those systems with a bootloader that can't be upgraded to accommodate the documented boot protocol using a device tree. Beware that there is very little in terms of protection against this option being confused by leftover garbage in memory that might look like a DTB header after a reboot if no actual DTB is appended to zImage. Do not leave this option active in a production kernel if you don't intend to always append a DTB. Proper passing of the location into r2 of a bootloader provided DTB is always preferable to this option. If you rely on it, and you have something that looks like a dtb after the image, then things will go wrong, so it's better _not_ to use it and to keep it disabled. That aside, thanks for doing a more in-depth analysis of what is going on, which helps to understand /why/ Ard's fix works (whereas before it was rather nebulous.) I wonder whether we ought to tell the linker to discard any unknown sections by adding at the bottom: /DISCARD/ { *(*) } but I do think we need to document this, specifically that _edata must point to the first byte after the binary file, and that the only sections after it are allowed to be the .bss and stack sections.
diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S index b38dcef90756..1636fa259577 100644 --- a/arch/arm/boot/compressed/vmlinux.lds.S +++ b/arch/arm/boot/compressed/vmlinux.lds.S @@ -70,10 +70,6 @@ SECTIONS .got : { *(.got) } _got_end = .; - /* ensure the zImage file size is always a multiple of 64 bits */ - /* (without a dummy byte, ld just ignores the empty section) */ - .pad : { BYTE(0); . = ALIGN(8); } - #ifdef CONFIG_EFI_STUB .data : ALIGN(4096) { __pecoff_data_start = .; @@ -93,6 +89,10 @@ SECTIONS __pecoff_data_rawsize = . - ADDR(.data); #endif + /* ensure the zImage file size is always a multiple of 64 bits */ + /* (without a dummy byte, ld just ignores the empty section) */ + .pad : { BYTE(0); . = ALIGN(8); } + _edata = .; _magic_sig = ZIMAGE_MAGIC(0x016f2818);
The zImage file size should be aligned. Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections") Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> --- arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)