Messages in this thread | | | From | Ard Biesheuvel <> | Date | Sun, 22 Oct 2017 14:01:51 +0100 | Subject | Re: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled |
| |
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.
| |