| Message ID | 20170908153143.27279-1-ard.biesheuvel@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Ard, On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > As it turns out, building the ARM kernel with EFI support pulls in > a couple of sections that we don't really need in the decompressor. > This is due to the fact the the UEFI stub uses sort() to sort the UEFI > memory map, which is an exported symbol pulled in from lib/sort.c. > > Before commit e4bae4d0b5f3 ("arm/efi: Split zImage code and data into > separate PE/COFF sections"), this resulted in the following layout > for the decompressor ELF binary. > > [Nr] Name Type Addr Off Size ES Flg Lk Inf Al > [ 0] NULL 00000000 000000 000000 00 0 0 0 > [ 1] .text PROGBITS 00000000 010000 009b3c 00 AX 0 0 512 > [ 2] .rodata PROGBITS 00009b3c 019b3c 001684 00 A 0 0 4 > [ 3] __ksymtab_strings PROGBITS 0000b1c0 01b1c0 000005 00 A 0 0 1 > [ 4] .data PROGBITS 0000b1c8 01b1c8 000020 00 WA 0 0 8 > [ 5] ___ksymtab+sort PROGBITS 0000b1e8 01b1e8 000008 00 WA 0 0 4 > [ 6] .piggydata PROGBITS 0000b1f0 01b1f0 77ac38 00 A 0 0 1 > [ 7] .got.plt PROGBITS 00785e28 795e28 00000c 04 WA 0 0 4 > [ 8] .got PROGBITS 00785e34 795e34 000028 00 WA 0 0 4 > [ 9] .pad PROGBITS 00785e5c 795e5c 000004 00 WA 0 0 1 > [10] .bss NOBITS 00785e60 795e60 00001c 00 WA 0 0 4 > [11] .stack NOBITS 00785e80 795e60 001000 00 WA 0 0 1 > > Commit e4bae4d0b5f3 made some changes to the linker script to allow the > UEFI firmware to map the decompressor with strict R-X/RW- permissions > before invoking it. Unfortunately, this turns out to break the boot on > some systems, because the linker now also moves the ksymtab/kcrctab > sections around, resulting in .piggydata to appear misaligned. > > [Nr] Name Type Addr Off Size ES Flg Lk Inf Al > [ 0] NULL 00000000 000000 000000 00 0 0 0 > [ 1] .text PROGBITS 00000000 010000 00a93c 00 AX 0 0 4096 > [ 2] .rodata PROGBITS 0000a93c 01a93c 001684 00 A 0 0 4 > [ 3] __ksymtab_strings PROGBITS 0000bfc0 01bfc0 000005 00 A 0 0 1 > [ 4] .piggydata PROGBITS 0000bfc5 01bfc5 77ac47 00 A 0 0 1 > [ 5] .got.plt PROGBITS 00786c0c 796c0c 00000c 04 WA 0 0 4 > [ 6] .got PROGBITS 00786c18 796c18 000028 00 WA 0 0 4 > [ 7] .pad PROGBITS 00786c40 796c40 000008 00 WA 0 0 1 > [ 8] .data PROGBITS 00787000 797000 000200 00 WA 0 0 4096 > [ 9] ___ksymtab+sort PROGBITS 00787200 797200 000008 00 WA 0 0 4 > [10] .bss NOBITS 00787208 797208 00001c 00 WA 0 0 4 > [11] .stack NOBITS 00787228 797208 001000 00 WA 0 0 1 > > So let's align piggydata explicitly, and discard these sections from the > binary. > > Cc: Russell King <linux@armlinux.org.uk> > Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate ...") > Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com> Actually I had not tested a kernel with these two changes in the same time. But now I've just done it, and it still works, so the patch and my tested-by remain valid. Thanks! > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/arm/boot/compressed/piggy.S | 1 + > arch/arm/boot/compressed/vmlinux.lds.S | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/arch/arm/boot/compressed/piggy.S b/arch/arm/boot/compressed/piggy.S > index f72088495f43..5d52c556dd32 100644 > --- a/arch/arm/boot/compressed/piggy.S > +++ b/arch/arm/boot/compressed/piggy.S > @@ -1,5 +1,6 @@ > .section .piggydata,#alloc > .globl input_data > + .align 2 > input_data: > .incbin "arch/arm/boot/compressed/piggy_data" > .globl input_data_end > diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S > index 7a4c59154361..5c5265be4605 100644 > --- a/arch/arm/boot/compressed/vmlinux.lds.S > +++ b/arch/arm/boot/compressed/vmlinux.lds.S > @@ -29,6 +29,7 @@ SECTIONS > * of the text/got segments. > */ > *(.data) > + *(*ksymtab* *kcrctab*) > } > > . = TEXT_START; > -- > 2.11.0 >
On 8 September 2017 at 16:39, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > Hi Ard, > > On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >> As it turns out, building the ARM kernel with EFI support pulls in >> a couple of sections that we don't really need in the decompressor. >> This is due to the fact the the UEFI stub uses sort() to sort the UEFI >> memory map, which is an exported symbol pulled in from lib/sort.c. >> >> Before commit e4bae4d0b5f3 ("arm/efi: Split zImage code and data into >> separate PE/COFF sections"), this resulted in the following layout >> for the decompressor ELF binary. >> >> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >> [ 0] NULL 00000000 000000 000000 00 0 0 0 >> [ 1] .text PROGBITS 00000000 010000 009b3c 00 AX 0 0 512 >> [ 2] .rodata PROGBITS 00009b3c 019b3c 001684 00 A 0 0 4 >> [ 3] __ksymtab_strings PROGBITS 0000b1c0 01b1c0 000005 00 A 0 0 1 >> [ 4] .data PROGBITS 0000b1c8 01b1c8 000020 00 WA 0 0 8 >> [ 5] ___ksymtab+sort PROGBITS 0000b1e8 01b1e8 000008 00 WA 0 0 4 >> [ 6] .piggydata PROGBITS 0000b1f0 01b1f0 77ac38 00 A 0 0 1 >> [ 7] .got.plt PROGBITS 00785e28 795e28 00000c 04 WA 0 0 4 >> [ 8] .got PROGBITS 00785e34 795e34 000028 00 WA 0 0 4 >> [ 9] .pad PROGBITS 00785e5c 795e5c 000004 00 WA 0 0 1 >> [10] .bss NOBITS 00785e60 795e60 00001c 00 WA 0 0 4 >> [11] .stack NOBITS 00785e80 795e60 001000 00 WA 0 0 1 >> >> Commit e4bae4d0b5f3 made some changes to the linker script to allow the >> UEFI firmware to map the decompressor with strict R-X/RW- permissions >> before invoking it. Unfortunately, this turns out to break the boot on >> some systems, because the linker now also moves the ksymtab/kcrctab >> sections around, resulting in .piggydata to appear misaligned. >> >> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >> [ 0] NULL 00000000 000000 000000 00 0 0 0 >> [ 1] .text PROGBITS 00000000 010000 00a93c 00 AX 0 0 4096 >> [ 2] .rodata PROGBITS 0000a93c 01a93c 001684 00 A 0 0 4 >> [ 3] __ksymtab_strings PROGBITS 0000bfc0 01bfc0 000005 00 A 0 0 1 >> [ 4] .piggydata PROGBITS 0000bfc5 01bfc5 77ac47 00 A 0 0 1 >> [ 5] .got.plt PROGBITS 00786c0c 796c0c 00000c 04 WA 0 0 4 >> [ 6] .got PROGBITS 00786c18 796c18 000028 00 WA 0 0 4 >> [ 7] .pad PROGBITS 00786c40 796c40 000008 00 WA 0 0 1 >> [ 8] .data PROGBITS 00787000 797000 000200 00 WA 0 0 4096 >> [ 9] ___ksymtab+sort PROGBITS 00787200 797200 000008 00 WA 0 0 4 >> [10] .bss NOBITS 00787208 797208 00001c 00 WA 0 0 4 >> [11] .stack NOBITS 00787228 797208 001000 00 WA 0 0 1 >> >> So let's align piggydata explicitly, and discard these sections from the >> binary. >> >> Cc: Russell King <linux@armlinux.org.uk> >> Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate ...") >> Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com> > > Actually I had not tested a kernel with these two changes in the same > time. But now I've just done it, and it still works, so the patch and my > tested-by remain valid. > > Thanks! > Thanks for confirming. I was a bit sneaky, but the added .align can never make a difference when the section is already aligned. I don't understand why just the .align is not sufficient, but the patch is simple enough so I think I am not going to investigate any further. Thanks
Hi Ard, On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > As it turns out, building the ARM kernel with EFI support pulls in > a couple of sections that we don't really need in the decompressor. > This is due to the fact the the UEFI stub uses sort() to sort the UEFI > memory map, which is an exported symbol pulled in from lib/sort.c. > > Before commit e4bae4d0b5f3 ("arm/efi: Split zImage code and data into > separate PE/COFF sections"), this resulted in the following layout > for the decompressor ELF binary. > > [Nr] Name Type Addr Off Size ES Flg Lk Inf Al > [ 0] NULL 00000000 000000 000000 00 0 0 0 > [ 1] .text PROGBITS 00000000 010000 009b3c 00 AX 0 0 512 > [ 2] .rodata PROGBITS 00009b3c 019b3c 001684 00 A 0 0 4 > [ 3] __ksymtab_strings PROGBITS 0000b1c0 01b1c0 000005 00 A 0 0 1 > [ 4] .data PROGBITS 0000b1c8 01b1c8 000020 00 WA 0 0 8 > [ 5] ___ksymtab+sort PROGBITS 0000b1e8 01b1e8 000008 00 WA 0 0 4 > [ 6] .piggydata PROGBITS 0000b1f0 01b1f0 77ac38 00 A 0 0 1 > [ 7] .got.plt PROGBITS 00785e28 795e28 00000c 04 WA 0 0 4 > [ 8] .got PROGBITS 00785e34 795e34 000028 00 WA 0 0 4 > [ 9] .pad PROGBITS 00785e5c 795e5c 000004 00 WA 0 0 1 > [10] .bss NOBITS 00785e60 795e60 00001c 00 WA 0 0 4 > [11] .stack NOBITS 00785e80 795e60 001000 00 WA 0 0 1 > > Commit e4bae4d0b5f3 made some changes to the linker script to allow the > UEFI firmware to map the decompressor with strict R-X/RW- permissions > before invoking it. Unfortunately, this turns out to break the boot on > some systems, because the linker now also moves the ksymtab/kcrctab > sections around, resulting in .piggydata to appear misaligned. > > [Nr] Name Type Addr Off Size ES Flg Lk Inf Al > [ 0] NULL 00000000 000000 000000 00 0 0 0 > [ 1] .text PROGBITS 00000000 010000 00a93c 00 AX 0 0 4096 > [ 2] .rodata PROGBITS 0000a93c 01a93c 001684 00 A 0 0 4 > [ 3] __ksymtab_strings PROGBITS 0000bfc0 01bfc0 000005 00 A 0 0 1 > [ 4] .piggydata PROGBITS 0000bfc5 01bfc5 77ac47 00 A 0 0 1 > [ 5] .got.plt PROGBITS 00786c0c 796c0c 00000c 04 WA 0 0 4 > [ 6] .got PROGBITS 00786c18 796c18 000028 00 WA 0 0 4 > [ 7] .pad PROGBITS 00786c40 796c40 000008 00 WA 0 0 1 > [ 8] .data PROGBITS 00787000 797000 000200 00 WA 0 0 4096 > [ 9] ___ksymtab+sort PROGBITS 00787200 797200 000008 00 WA 0 0 4 > [10] .bss NOBITS 00787208 797208 00001c 00 WA 0 0 4 > [11] .stack NOBITS 00787228 797208 001000 00 WA 0 0 1 > > So let's align piggydata explicitly, and discard these sections from the > binary. > > Cc: Russell King <linux@armlinux.org.uk> > Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate ...") > Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> Any new for this patch? It is not yet in v4.14-rc whereas "arm/efi: Split zImage code and data into separate ..." was already merged. So currently I have many boards which still does not boot in v4.14-rc3. Gregory > --- > arch/arm/boot/compressed/piggy.S | 1 + > arch/arm/boot/compressed/vmlinux.lds.S | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/arch/arm/boot/compressed/piggy.S b/arch/arm/boot/compressed/piggy.S > index f72088495f43..5d52c556dd32 100644 > --- a/arch/arm/boot/compressed/piggy.S > +++ b/arch/arm/boot/compressed/piggy.S > @@ -1,5 +1,6 @@ > .section .piggydata,#alloc > .globl input_data > + .align 2 > input_data: > .incbin "arch/arm/boot/compressed/piggy_data" > .globl input_data_end > diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S > index 7a4c59154361..5c5265be4605 100644 > --- a/arch/arm/boot/compressed/vmlinux.lds.S > +++ b/arch/arm/boot/compressed/vmlinux.lds.S > @@ -29,6 +29,7 @@ SECTIONS > * of the text/got segments. > */ > *(.data) > + *(*ksymtab* *kcrctab*) > } > > . = TEXT_START; > -- > 2.11.0 >
On 4 October 2017 at 13:16, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > Hi Ard, > > On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >> As it turns out, building the ARM kernel with EFI support pulls in >> a couple of sections that we don't really need in the decompressor. >> This is due to the fact the the UEFI stub uses sort() to sort the UEFI >> memory map, which is an exported symbol pulled in from lib/sort.c. >> >> Before commit e4bae4d0b5f3 ("arm/efi: Split zImage code and data into >> separate PE/COFF sections"), this resulted in the following layout >> for the decompressor ELF binary. >> >> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >> [ 0] NULL 00000000 000000 000000 00 0 0 0 >> [ 1] .text PROGBITS 00000000 010000 009b3c 00 AX 0 0 512 >> [ 2] .rodata PROGBITS 00009b3c 019b3c 001684 00 A 0 0 4 >> [ 3] __ksymtab_strings PROGBITS 0000b1c0 01b1c0 000005 00 A 0 0 1 >> [ 4] .data PROGBITS 0000b1c8 01b1c8 000020 00 WA 0 0 8 >> [ 5] ___ksymtab+sort PROGBITS 0000b1e8 01b1e8 000008 00 WA 0 0 4 >> [ 6] .piggydata PROGBITS 0000b1f0 01b1f0 77ac38 00 A 0 0 1 >> [ 7] .got.plt PROGBITS 00785e28 795e28 00000c 04 WA 0 0 4 >> [ 8] .got PROGBITS 00785e34 795e34 000028 00 WA 0 0 4 >> [ 9] .pad PROGBITS 00785e5c 795e5c 000004 00 WA 0 0 1 >> [10] .bss NOBITS 00785e60 795e60 00001c 00 WA 0 0 4 >> [11] .stack NOBITS 00785e80 795e60 001000 00 WA 0 0 1 >> >> Commit e4bae4d0b5f3 made some changes to the linker script to allow the >> UEFI firmware to map the decompressor with strict R-X/RW- permissions >> before invoking it. Unfortunately, this turns out to break the boot on >> some systems, because the linker now also moves the ksymtab/kcrctab >> sections around, resulting in .piggydata to appear misaligned. >> >> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >> [ 0] NULL 00000000 000000 000000 00 0 0 0 >> [ 1] .text PROGBITS 00000000 010000 00a93c 00 AX 0 0 4096 >> [ 2] .rodata PROGBITS 0000a93c 01a93c 001684 00 A 0 0 4 >> [ 3] __ksymtab_strings PROGBITS 0000bfc0 01bfc0 000005 00 A 0 0 1 >> [ 4] .piggydata PROGBITS 0000bfc5 01bfc5 77ac47 00 A 0 0 1 >> [ 5] .got.plt PROGBITS 00786c0c 796c0c 00000c 04 WA 0 0 4 >> [ 6] .got PROGBITS 00786c18 796c18 000028 00 WA 0 0 4 >> [ 7] .pad PROGBITS 00786c40 796c40 000008 00 WA 0 0 1 >> [ 8] .data PROGBITS 00787000 797000 000200 00 WA 0 0 4096 >> [ 9] ___ksymtab+sort PROGBITS 00787200 797200 000008 00 WA 0 0 4 >> [10] .bss NOBITS 00787208 797208 00001c 00 WA 0 0 4 >> [11] .stack NOBITS 00787228 797208 001000 00 WA 0 0 1 >> >> So let's align piggydata explicitly, and discard these sections from the >> binary. >> >> Cc: Russell King <linux@armlinux.org.uk> >> Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate ...") >> Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Any new for this patch? > > It is not yet in v4.14-rc whereas "arm/efi: Split zImage code and data > into separate ..." was already merged. So currently I have many boards > which still does not boot in v4.14-rc3. > Russell, any objections? >> --- >> arch/arm/boot/compressed/piggy.S | 1 + >> arch/arm/boot/compressed/vmlinux.lds.S | 1 + >> 2 files changed, 2 insertions(+) >> >> diff --git a/arch/arm/boot/compressed/piggy.S b/arch/arm/boot/compressed/piggy.S >> index f72088495f43..5d52c556dd32 100644 >> --- a/arch/arm/boot/compressed/piggy.S >> +++ b/arch/arm/boot/compressed/piggy.S >> @@ -1,5 +1,6 @@ >> .section .piggydata,#alloc >> .globl input_data >> + .align 2 >> input_data: >> .incbin "arch/arm/boot/compressed/piggy_data" >> .globl input_data_end >> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S >> index 7a4c59154361..5c5265be4605 100644 >> --- a/arch/arm/boot/compressed/vmlinux.lds.S >> +++ b/arch/arm/boot/compressed/vmlinux.lds.S >> @@ -29,6 +29,7 @@ SECTIONS >> * of the text/got segments. >> */ >> *(.data) >> + *(*ksymtab* *kcrctab*) >> } >> >> . = TEXT_START; >> -- >> 2.11.0 >> > > -- > Gregory Clement, Free Electrons > Kernel, drivers, real-time and embedded Linux > development, consulting, training and support. > http://free-electrons.com
On Wed, Oct 04, 2017 at 01:20:26PM +0100, Ard Biesheuvel wrote: > On 4 October 2017 at 13:16, Gregory CLEMENT > <gregory.clement@free-electrons.com> wrote: > > Hi Ard, > > > > On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > > >> As it turns out, building the ARM kernel with EFI support pulls in > >> a couple of sections that we don't really need in the decompressor. > >> This is due to the fact the the UEFI stub uses sort() to sort the UEFI > >> memory map, which is an exported symbol pulled in from lib/sort.c. > >> > >> Before commit e4bae4d0b5f3 ("arm/efi: Split zImage code and data into > >> separate PE/COFF sections"), this resulted in the following layout > >> for the decompressor ELF binary. > >> > >> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al > >> [ 0] NULL 00000000 000000 000000 00 0 0 0 > >> [ 1] .text PROGBITS 00000000 010000 009b3c 00 AX 0 0 512 > >> [ 2] .rodata PROGBITS 00009b3c 019b3c 001684 00 A 0 0 4 > >> [ 3] __ksymtab_strings PROGBITS 0000b1c0 01b1c0 000005 00 A 0 0 1 > >> [ 4] .data PROGBITS 0000b1c8 01b1c8 000020 00 WA 0 0 8 > >> [ 5] ___ksymtab+sort PROGBITS 0000b1e8 01b1e8 000008 00 WA 0 0 4 > >> [ 6] .piggydata PROGBITS 0000b1f0 01b1f0 77ac38 00 A 0 0 1 > >> [ 7] .got.plt PROGBITS 00785e28 795e28 00000c 04 WA 0 0 4 > >> [ 8] .got PROGBITS 00785e34 795e34 000028 00 WA 0 0 4 > >> [ 9] .pad PROGBITS 00785e5c 795e5c 000004 00 WA 0 0 1 > >> [10] .bss NOBITS 00785e60 795e60 00001c 00 WA 0 0 4 > >> [11] .stack NOBITS 00785e80 795e60 001000 00 WA 0 0 1 > >> > >> Commit e4bae4d0b5f3 made some changes to the linker script to allow the > >> UEFI firmware to map the decompressor with strict R-X/RW- permissions > >> before invoking it. Unfortunately, this turns out to break the boot on > >> some systems, because the linker now also moves the ksymtab/kcrctab > >> sections around, resulting in .piggydata to appear misaligned. > >> > >> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al > >> [ 0] NULL 00000000 000000 000000 00 0 0 0 > >> [ 1] .text PROGBITS 00000000 010000 00a93c 00 AX 0 0 4096 > >> [ 2] .rodata PROGBITS 0000a93c 01a93c 001684 00 A 0 0 4 > >> [ 3] __ksymtab_strings PROGBITS 0000bfc0 01bfc0 000005 00 A 0 0 1 > >> [ 4] .piggydata PROGBITS 0000bfc5 01bfc5 77ac47 00 A 0 0 1 > >> [ 5] .got.plt PROGBITS 00786c0c 796c0c 00000c 04 WA 0 0 4 > >> [ 6] .got PROGBITS 00786c18 796c18 000028 00 WA 0 0 4 > >> [ 7] .pad PROGBITS 00786c40 796c40 000008 00 WA 0 0 1 > >> [ 8] .data PROGBITS 00787000 797000 000200 00 WA 0 0 4096 > >> [ 9] ___ksymtab+sort PROGBITS 00787200 797200 000008 00 WA 0 0 4 > >> [10] .bss NOBITS 00787208 797208 00001c 00 WA 0 0 4 > >> [11] .stack NOBITS 00787228 797208 001000 00 WA 0 0 1 > >> > >> So let's align piggydata explicitly, and discard these sections from the > >> binary. > >> > >> Cc: Russell King <linux@armlinux.org.uk> > >> Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate ...") > >> Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com> > >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > > > Any new for this patch? > > > > It is not yet in v4.14-rc whereas "arm/efi: Split zImage code and data > > into separate ..." was already merged. So currently I have many boards > > which still does not boot in v4.14-rc3. > > > > Russell, any objections? It would be nice if there was something in the commit log that described why we need to align data that is basically a byte stream, and which decompressor methods it affects. Maybe the decompressors should cope with a misaligned byte stream - what if (for example) someone supplies the kernel with a compressed initramfs image that is not word aligned? We already have people using non-page aligned compressed initramfs images.
On 4 October 2017 at 13:43, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Wed, Oct 04, 2017 at 01:20:26PM +0100, Ard Biesheuvel wrote: >> On 4 October 2017 at 13:16, Gregory CLEMENT >> <gregory.clement@free-electrons.com> wrote: >> > Hi Ard, >> > >> > On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> > >> >> As it turns out, building the ARM kernel with EFI support pulls in >> >> a couple of sections that we don't really need in the decompressor. >> >> This is due to the fact the the UEFI stub uses sort() to sort the UEFI >> >> memory map, which is an exported symbol pulled in from lib/sort.c. >> >> >> >> Before commit e4bae4d0b5f3 ("arm/efi: Split zImage code and data into >> >> separate PE/COFF sections"), this resulted in the following layout >> >> for the decompressor ELF binary. >> >> >> >> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >> >> [ 0] NULL 00000000 000000 000000 00 0 0 0 >> >> [ 1] .text PROGBITS 00000000 010000 009b3c 00 AX 0 0 512 >> >> [ 2] .rodata PROGBITS 00009b3c 019b3c 001684 00 A 0 0 4 >> >> [ 3] __ksymtab_strings PROGBITS 0000b1c0 01b1c0 000005 00 A 0 0 1 >> >> [ 4] .data PROGBITS 0000b1c8 01b1c8 000020 00 WA 0 0 8 >> >> [ 5] ___ksymtab+sort PROGBITS 0000b1e8 01b1e8 000008 00 WA 0 0 4 >> >> [ 6] .piggydata PROGBITS 0000b1f0 01b1f0 77ac38 00 A 0 0 1 >> >> [ 7] .got.plt PROGBITS 00785e28 795e28 00000c 04 WA 0 0 4 >> >> [ 8] .got PROGBITS 00785e34 795e34 000028 00 WA 0 0 4 >> >> [ 9] .pad PROGBITS 00785e5c 795e5c 000004 00 WA 0 0 1 >> >> [10] .bss NOBITS 00785e60 795e60 00001c 00 WA 0 0 4 >> >> [11] .stack NOBITS 00785e80 795e60 001000 00 WA 0 0 1 >> >> >> >> Commit e4bae4d0b5f3 made some changes to the linker script to allow the >> >> UEFI firmware to map the decompressor with strict R-X/RW- permissions >> >> before invoking it. Unfortunately, this turns out to break the boot on >> >> some systems, because the linker now also moves the ksymtab/kcrctab >> >> sections around, resulting in .piggydata to appear misaligned. >> >> >> >> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >> >> [ 0] NULL 00000000 000000 000000 00 0 0 0 >> >> [ 1] .text PROGBITS 00000000 010000 00a93c 00 AX 0 0 4096 >> >> [ 2] .rodata PROGBITS 0000a93c 01a93c 001684 00 A 0 0 4 >> >> [ 3] __ksymtab_strings PROGBITS 0000bfc0 01bfc0 000005 00 A 0 0 1 >> >> [ 4] .piggydata PROGBITS 0000bfc5 01bfc5 77ac47 00 A 0 0 1 >> >> [ 5] .got.plt PROGBITS 00786c0c 796c0c 00000c 04 WA 0 0 4 >> >> [ 6] .got PROGBITS 00786c18 796c18 000028 00 WA 0 0 4 >> >> [ 7] .pad PROGBITS 00786c40 796c40 000008 00 WA 0 0 1 >> >> [ 8] .data PROGBITS 00787000 797000 000200 00 WA 0 0 4096 >> >> [ 9] ___ksymtab+sort PROGBITS 00787200 797200 000008 00 WA 0 0 4 >> >> [10] .bss NOBITS 00787208 797208 00001c 00 WA 0 0 4 >> >> [11] .stack NOBITS 00787228 797208 001000 00 WA 0 0 1 >> >> >> >> So let's align piggydata explicitly, and discard these sections from the >> >> binary. >> >> >> >> Cc: Russell King <linux@armlinux.org.uk> >> >> Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate ...") >> >> Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com> >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> > >> > Any new for this patch? >> > >> > It is not yet in v4.14-rc whereas "arm/efi: Split zImage code and data >> > into separate ..." was already merged. So currently I have many boards >> > which still does not boot in v4.14-rc3. >> > >> >> Russell, any objections? > > It would be nice if there was something in the commit log that described > why we need to align data that is basically a byte stream, and which > decompressor methods it affects. Maybe the decompressors should cope > with a misaligned byte stream - what if (for example) someone supplies > the kernel with a compressed initramfs image that is not word aligned? The decompressor copes with a misaligned byte stream by using get_unaligned et al. Only, on v7, these are simply converted to word wide unaligned accesses, which the compiler may merge into ldm/stm if they occur adjacently. In the kernel proper, this is caught and fixed up by the alignment fixup code, but in the decompressor you hit the fault. > We already have people using non-page aligned compressed initramfs > images. > Yes, but initramfs accesses are fixed up by the alignment fixup code as well. So I suppose Arnd's patch to switch to the struct type unaligned accessor would deal with this issue as well.
Hi Ard, On lun., oct. 09 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 4 October 2017 at 13:43, Russell King - ARM Linux > <linux@armlinux.org.uk> wrote: >> On Wed, Oct 04, 2017 at 01:20:26PM +0100, Ard Biesheuvel wrote: >>> On 4 October 2017 at 13:16, Gregory CLEMENT >>> <gregory.clement@free-electrons.com> wrote: >>> > Hi Ard, >>> > >>> > On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >>> > >>> >> As it turns out, building the ARM kernel with EFI support pulls in >>> >> a couple of sections that we don't really need in the decompressor. >>> >> This is due to the fact the the UEFI stub uses sort() to sort the UEFI >>> >> memory map, which is an exported symbol pulled in from lib/sort.c. >>> >> >>> >> Before commit e4bae4d0b5f3 ("arm/efi: Split zImage code and data into >>> >> separate PE/COFF sections"), this resulted in the following layout >>> >> for the decompressor ELF binary. >>> >> >>> >> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >>> >> [ 0] NULL 00000000 000000 000000 00 0 0 0 >>> >> [ 1] .text PROGBITS 00000000 010000 009b3c 00 AX 0 0 512 >>> >> [ 2] .rodata PROGBITS 00009b3c 019b3c 001684 00 A 0 0 4 >>> >> [ 3] __ksymtab_strings PROGBITS 0000b1c0 01b1c0 000005 00 A 0 0 1 >>> >> [ 4] .data PROGBITS 0000b1c8 01b1c8 000020 00 WA 0 0 8 >>> >> [ 5] ___ksymtab+sort PROGBITS 0000b1e8 01b1e8 000008 00 WA 0 0 4 >>> >> [ 6] .piggydata PROGBITS 0000b1f0 01b1f0 77ac38 00 A 0 0 1 >>> >> [ 7] .got.plt PROGBITS 00785e28 795e28 00000c 04 WA 0 0 4 >>> >> [ 8] .got PROGBITS 00785e34 795e34 000028 00 WA 0 0 4 >>> >> [ 9] .pad PROGBITS 00785e5c 795e5c 000004 00 WA 0 0 1 >>> >> [10] .bss NOBITS 00785e60 795e60 00001c 00 WA 0 0 4 >>> >> [11] .stack NOBITS 00785e80 795e60 001000 00 WA 0 0 1 >>> >> >>> >> Commit e4bae4d0b5f3 made some changes to the linker script to allow the >>> >> UEFI firmware to map the decompressor with strict R-X/RW- permissions >>> >> before invoking it. Unfortunately, this turns out to break the boot on >>> >> some systems, because the linker now also moves the ksymtab/kcrctab >>> >> sections around, resulting in .piggydata to appear misaligned. >>> >> >>> >> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >>> >> [ 0] NULL 00000000 000000 000000 00 0 0 0 >>> >> [ 1] .text PROGBITS 00000000 010000 00a93c 00 AX 0 0 4096 >>> >> [ 2] .rodata PROGBITS 0000a93c 01a93c 001684 00 A 0 0 4 >>> >> [ 3] __ksymtab_strings PROGBITS 0000bfc0 01bfc0 000005 00 A 0 0 1 >>> >> [ 4] .piggydata PROGBITS 0000bfc5 01bfc5 77ac47 00 A 0 0 1 >>> >> [ 5] .got.plt PROGBITS 00786c0c 796c0c 00000c 04 WA 0 0 4 >>> >> [ 6] .got PROGBITS 00786c18 796c18 000028 00 WA 0 0 4 >>> >> [ 7] .pad PROGBITS 00786c40 796c40 000008 00 WA 0 0 1 >>> >> [ 8] .data PROGBITS 00787000 797000 000200 00 WA 0 0 4096 >>> >> [ 9] ___ksymtab+sort PROGBITS 00787200 797200 000008 00 WA 0 0 4 >>> >> [10] .bss NOBITS 00787208 797208 00001c 00 WA 0 0 4 >>> >> [11] .stack NOBITS 00787228 797208 001000 00 WA 0 0 1 >>> >> >>> >> So let's align piggydata explicitly, and discard these sections from the >>> >> binary. >>> >> >>> >> Cc: Russell King <linux@armlinux.org.uk> >>> >> Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate ...") >>> >> Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com> >>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> > >>> > Any new for this patch? >>> > >>> > It is not yet in v4.14-rc whereas "arm/efi: Split zImage code and data >>> > into separate ..." was already merged. So currently I have many boards >>> > which still does not boot in v4.14-rc3. >>> > >>> >>> Russell, any objections? >> >> It would be nice if there was something in the commit log that described >> why we need to align data that is basically a byte stream, and which >> decompressor methods it affects. Maybe the decompressors should cope >> with a misaligned byte stream - what if (for example) someone supplies >> the kernel with a compressed initramfs image that is not word aligned? > > The decompressor copes with a misaligned byte stream by using > get_unaligned et al. Only, on v7, these are simply converted to word > wide unaligned accesses, which the compiler may merge into ldm/stm if > they occur adjacently. In the kernel proper, this is caught and fixed > up by the alignment fixup code, but in the decompressor you hit the > fault. > Can we move forward to fix the booting problem ? What about amending your commit log with this new information and then submit it to Russell patch system? Thanks, Gregory >> We already have people using non-page aligned compressed initramfs >> images. >> > > Yes, but initramfs accesses are fixed up by the alignment fixup code as well. > > So I suppose Arnd's patch to switch to the struct type unaligned > accessor would deal with this issue as well.
On Thu, Oct 12, 2017 at 11:24:57AM +0200, Gregory CLEMENT wrote: > Hi Ard, > > Can we move forward to fix the booting problem ? > > What about amending your commit log with this new information and then > submit it to Russell patch system? Well, I think there's a choice that needs to be made between this approach and Arnd's approach. I'm not all that thrilled with the need to add explicit alignment to data that is inherently a byte stream, and that invariably results in unaligned data words even if you do align the start of it. That sounds to me very much like a hack rather than a proper solution. So, right now I'm leaning more towards Arnd's solution than Ard's from what's been said in this thread. However, I don't recall Arnd's patch, it's probably buried deep in my mailbox.
On 12 October 2017 at 10:45, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Thu, Oct 12, 2017 at 11:24:57AM +0200, Gregory CLEMENT wrote: >> Hi Ard, >> >> Can we move forward to fix the booting problem ? >> >> What about amending your commit log with this new information and then >> submit it to Russell patch system? > > Well, I think there's a choice that needs to be made between this > approach and Arnd's approach. > > I'm not all that thrilled with the need to add explicit alignment to > data that is inherently a byte stream, and that invariably results in > unaligned data words even if you do align the start of it. That > sounds to me very much like a hack rather than a proper solution. > So, right now I'm leaning more towards Arnd's solution than Ard's > from what's been said in this thread. > I agree that the struct type unaligned accessors are the best choice for ARM in any case, given that it will also prevent hitting the alignment fixup handler in the kernel unnecessarily. > However, I don't recall Arnd's patch, it's probably buried deep in > my mailbox. > Well, unless you are considering changing the unaligned accessors from access_ok.h to le_struct.h as a bugfix, I think we need both patches.
Hi Ard, On jeu., oct. 12 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 12 October 2017 at 10:45, Russell King - ARM Linux > <linux@armlinux.org.uk> wrote: >> On Thu, Oct 12, 2017 at 11:24:57AM +0200, Gregory CLEMENT wrote: >>> Hi Ard, >>> >>> Can we move forward to fix the booting problem ? >>> >>> What about amending your commit log with this new information and then >>> submit it to Russell patch system? >> >> Well, I think there's a choice that needs to be made between this >> approach and Arnd's approach. >> >> I'm not all that thrilled with the need to add explicit alignment to >> data that is inherently a byte stream, and that invariably results in >> unaligned data words even if you do align the start of it. That >> sounds to me very much like a hack rather than a proper solution. >> So, right now I'm leaning more towards Arnd's solution than Ard's >> from what's been said in this thread. >> > > I agree that the struct type unaligned accessors are the best choice > for ARM in any case, given that it will also prevent hitting the > alignment fixup handler in the kernel unnecessarily. > >> However, I don't recall Arnd's patch, it's probably buried deep in >> my mailbox. >> > > Well, unless you are considering changing the unaligned accessors from > access_ok.h to le_struct.h as a bugfix, I think we need both patches. We will soon reach v4.14-rc6 and the Armada XP and Armada 370 still not boot. I also didn't see your patch in rmk patch system. Waiting for you find a agreement an other option is to remove CONFIG_EFI from multi_v7_defconfig as I don't really see any armv7 base board using EFI. Thanks, Gregory
On 20 October 2017 at 16:25, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > Hi Ard, > > On jeu., oct. 12 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >> On 12 October 2017 at 10:45, Russell King - ARM Linux >> <linux@armlinux.org.uk> wrote: >>> On Thu, Oct 12, 2017 at 11:24:57AM +0200, Gregory CLEMENT wrote: >>>> Hi Ard, >>>> >>>> Can we move forward to fix the booting problem ? >>>> >>>> What about amending your commit log with this new information and then >>>> submit it to Russell patch system? >>> >>> Well, I think there's a choice that needs to be made between this >>> approach and Arnd's approach. >>> >>> I'm not all that thrilled with the need to add explicit alignment to >>> data that is inherently a byte stream, and that invariably results in >>> unaligned data words even if you do align the start of it. That >>> sounds to me very much like a hack rather than a proper solution. >>> So, right now I'm leaning more towards Arnd's solution than Ard's >>> from what's been said in this thread. >>> >> >> I agree that the struct type unaligned accessors are the best choice >> for ARM in any case, given that it will also prevent hitting the >> alignment fixup handler in the kernel unnecessarily. >> >>> However, I don't recall Arnd's patch, it's probably buried deep in >>> my mailbox. >>> >> >> Well, unless you are considering changing the unaligned accessors from >> access_ok.h to le_struct.h as a bugfix, I think we need both patches. > > We will soon reach v4.14-rc6 and the Armada XP and Armada 370 still not > boot. I also didn't see your patch in rmk patch system. > > Waiting for you find a agreement an other option is to remove CONFIG_EFI > from multi_v7_defconfig as I don't really see any armv7 base board using > EFI. > It is up to Russell to decide how he wants to proceed. Russell?
On Fri, Oct 20, 2017 at 04:28:49PM +0100, Ard Biesheuvel wrote: > On 20 October 2017 at 16:25, Gregory CLEMENT > <gregory.clement@free-electrons.com> wrote: > > Hi Ard, > > > > On jeu., oct. 12 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > > >> On 12 October 2017 at 10:45, Russell King - ARM Linux > >> <linux@armlinux.org.uk> wrote: > >>> On Thu, Oct 12, 2017 at 11:24:57AM +0200, Gregory CLEMENT wrote: > >>>> Hi Ard, > >>>> > >>>> Can we move forward to fix the booting problem ? > >>>> > >>>> What about amending your commit log with this new information and then > >>>> submit it to Russell patch system? > >>> > >>> Well, I think there's a choice that needs to be made between this > >>> approach and Arnd's approach. > >>> > >>> I'm not all that thrilled with the need to add explicit alignment to > >>> data that is inherently a byte stream, and that invariably results in > >>> unaligned data words even if you do align the start of it. That > >>> sounds to me very much like a hack rather than a proper solution. > >>> So, right now I'm leaning more towards Arnd's solution than Ard's > >>> from what's been said in this thread. > >>> > >> > >> I agree that the struct type unaligned accessors are the best choice > >> for ARM in any case, given that it will also prevent hitting the > >> alignment fixup handler in the kernel unnecessarily. > >> > >>> However, I don't recall Arnd's patch, it's probably buried deep in > >>> my mailbox. > >>> > >> > >> Well, unless you are considering changing the unaligned accessors from > >> access_ok.h to le_struct.h as a bugfix, I think we need both patches. > > > > We will soon reach v4.14-rc6 and the Armada XP and Armada 370 still not > > boot. I also didn't see your patch in rmk patch system. > > > > Waiting for you find a agreement an other option is to remove CONFIG_EFI > > from multi_v7_defconfig as I don't really see any armv7 base board using > > EFI. > > > > It is up to Russell to decide how he wants to proceed. Russell? Well, having failed to attract Arnd's attention, I've spent 20 minutes searching my mailbox to find it. It turns out that there was never a proper patch from Arnd - it was a patch in pastebin. It's not ARM specific either, it's an asm-generic change, for which Arnd is the maintainer for. I've just replied to that old thread and I've included Gregory and some PXA folk who are also having alignment problems in the decompressor. It could all be due to this same issue. There's also one additional factor that hasn't been considered, and I'm scared to point it out, but: 1. Does Arnd's patch fix the PXA problem as well? 2. What is the performance impact of Arnd's fix?
On 20 October 2017 at 17:11, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Fri, Oct 20, 2017 at 04:28:49PM +0100, Ard Biesheuvel wrote: >> On 20 October 2017 at 16:25, Gregory CLEMENT >> <gregory.clement@free-electrons.com> wrote: >> > Hi Ard, >> > >> > On jeu., oct. 12 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> > >> >> On 12 October 2017 at 10:45, Russell King - ARM Linux >> >> <linux@armlinux.org.uk> wrote: >> >>> On Thu, Oct 12, 2017 at 11:24:57AM +0200, Gregory CLEMENT wrote: >> >>>> Hi Ard, >> >>>> >> >>>> Can we move forward to fix the booting problem ? >> >>>> >> >>>> What about amending your commit log with this new information and then >> >>>> submit it to Russell patch system? >> >>> >> >>> Well, I think there's a choice that needs to be made between this >> >>> approach and Arnd's approach. >> >>> >> >>> I'm not all that thrilled with the need to add explicit alignment to >> >>> data that is inherently a byte stream, and that invariably results in >> >>> unaligned data words even if you do align the start of it. That >> >>> sounds to me very much like a hack rather than a proper solution. >> >>> So, right now I'm leaning more towards Arnd's solution than Ard's >> >>> from what's been said in this thread. >> >>> >> >> >> >> I agree that the struct type unaligned accessors are the best choice >> >> for ARM in any case, given that it will also prevent hitting the >> >> alignment fixup handler in the kernel unnecessarily. >> >> >> >>> However, I don't recall Arnd's patch, it's probably buried deep in >> >>> my mailbox. >> >>> >> >> >> >> Well, unless you are considering changing the unaligned accessors from >> >> access_ok.h to le_struct.h as a bugfix, I think we need both patches. >> > >> > We will soon reach v4.14-rc6 and the Armada XP and Armada 370 still not >> > boot. I also didn't see your patch in rmk patch system. >> > >> > Waiting for you find a agreement an other option is to remove CONFIG_EFI >> > from multi_v7_defconfig as I don't really see any armv7 base board using >> > EFI. >> > >> >> It is up to Russell to decide how he wants to proceed. Russell? > > Well, having failed to attract Arnd's attention, I've spent 20 minutes > searching my mailbox to find it. > > It turns out that there was never a proper patch from Arnd - it was a > patch in pastebin. It's not ARM specific either, it's an asm-generic > change, for which Arnd is the maintainer for. > > I've just replied to that old thread and I've included Gregory and some > PXA folk who are also having alignment problems in the decompressor. It > could all be due to this same issue. > > There's also one additional factor that hasn't been considered, and I'm > scared to point it out, but: > > 1. Does Arnd's patch fix the PXA problem as well? > I don't think it will help configs that don't have HAVE_EFFICIENT_UNALIGNED_ACCESS set, given that they don't use access_ok.h in the first place. > 2. What is the performance impact of Arnd's fix? > Well, given that we may be relying on the alignment fixup handler to fix up kernel accesses, the performance impact could actually be favorable in some cases. But I don't have any numbers, nor do I have access to a representative sampling of ARM hardware so I can't be of any help here, unfortunately.
On Fri, Oct 20, 2017 at 05:20:26PM +0100, Ard Biesheuvel wrote: > On 20 October 2017 at 17:11, Russell King - ARM Linux > <linux@armlinux.org.uk> wrote: > > On Fri, Oct 20, 2017 at 04:28:49PM +0100, Ard Biesheuvel wrote: > >> On 20 October 2017 at 16:25, Gregory CLEMENT > >> <gregory.clement@free-electrons.com> wrote: > >> > Hi Ard, > >> > > >> > On jeu., oct. 12 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >> > > >> >> On 12 October 2017 at 10:45, Russell King - ARM Linux > >> >> <linux@armlinux.org.uk> wrote: > >> >>> On Thu, Oct 12, 2017 at 11:24:57AM +0200, Gregory CLEMENT wrote: > >> >>>> Hi Ard, > >> >>>> > >> >>>> Can we move forward to fix the booting problem ? > >> >>>> > >> >>>> What about amending your commit log with this new information and then > >> >>>> submit it to Russell patch system? > >> >>> > >> >>> Well, I think there's a choice that needs to be made between this > >> >>> approach and Arnd's approach. > >> >>> > >> >>> I'm not all that thrilled with the need to add explicit alignment to > >> >>> data that is inherently a byte stream, and that invariably results in > >> >>> unaligned data words even if you do align the start of it. That > >> >>> sounds to me very much like a hack rather than a proper solution. > >> >>> So, right now I'm leaning more towards Arnd's solution than Ard's > >> >>> from what's been said in this thread. > >> >>> > >> >> > >> >> I agree that the struct type unaligned accessors are the best choice > >> >> for ARM in any case, given that it will also prevent hitting the > >> >> alignment fixup handler in the kernel unnecessarily. > >> >> > >> >>> However, I don't recall Arnd's patch, it's probably buried deep in > >> >>> my mailbox. > >> >>> > >> >> > >> >> Well, unless you are considering changing the unaligned accessors from > >> >> access_ok.h to le_struct.h as a bugfix, I think we need both patches. > >> > > >> > We will soon reach v4.14-rc6 and the Armada XP and Armada 370 still not > >> > boot. I also didn't see your patch in rmk patch system. > >> > > >> > Waiting for you find a agreement an other option is to remove CONFIG_EFI > >> > from multi_v7_defconfig as I don't really see any armv7 base board using > >> > EFI. > >> > > >> > >> It is up to Russell to decide how he wants to proceed. Russell? > > > > Well, having failed to attract Arnd's attention, I've spent 20 minutes > > searching my mailbox to find it. > > > > It turns out that there was never a proper patch from Arnd - it was a > > patch in pastebin. It's not ARM specific either, it's an asm-generic > > change, for which Arnd is the maintainer for. > > > > I've just replied to that old thread and I've included Gregory and some > > PXA folk who are also having alignment problems in the decompressor. It > > could all be due to this same issue. > > > > There's also one additional factor that hasn't been considered, and I'm > > scared to point it out, but: > > > > 1. Does Arnd's patch fix the PXA problem as well? > > > > I don't think it will help configs that don't have > HAVE_EFFICIENT_UNALIGNED_ACCESS set, given that they don't use > access_ok.h in the first place. > > > 2. What is the performance impact of Arnd's fix? > > > > Well, given that we may be relying on the alignment fixup handler to > fix up kernel accesses, the performance impact could actually be > favorable in some cases. But I don't have any numbers, nor do I have > access to a representative sampling of ARM hardware so I can't be of > any help here, unfortunately. Well, everyone can tell whether alignment fixups get used on their platforms, by looking at /proc/cpu/alignment - this counts any alignment faults and their types. If it's all zeros, then the alignment fixup handler isn't being used. I just checked one of my imx6 platforms (3G/wifi gateway), all the stats are zero. Another imx6 platform (whose port 80 is open to the world) has one double-word fault in nsm_get_handle+0x200/0x484. My ARMv5 gateway here also has all zero stats. However, all these are built with GCC 4.7.4, and we already know that newer compilers behave significantly differently. So, I don't think what I see here is representative, so I can't decide based on what I see locally. So, it seems we're rather stuck. I suppose I could set up a dart board, and throw a dart blind folded and if it hits a red area, pick one patch, otherwise take the other. Or toss a coin. Or some other rediculous game. It would be much better if there was some way to evaluate the impact, but I see that no one is interested in lifting a finger to help with that. Meanwhile, I'm quite sure there'll be an on-going cry for me to make a decision. A decision based on a whim. Yea, great basis for taking decisions.
On 20 October 2017 at 17:32, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Fri, Oct 20, 2017 at 05:20:26PM +0100, Ard Biesheuvel wrote: >> On 20 October 2017 at 17:11, Russell King - ARM Linux >> <linux@armlinux.org.uk> wrote: >> > On Fri, Oct 20, 2017 at 04:28:49PM +0100, Ard Biesheuvel wrote: >> >> On 20 October 2017 at 16:25, Gregory CLEMENT >> >> <gregory.clement@free-electrons.com> wrote: >> >> > Hi Ard, >> >> > >> >> > On jeu., oct. 12 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> >> > >> >> >> On 12 October 2017 at 10:45, Russell King - ARM Linux >> >> >> <linux@armlinux.org.uk> wrote: >> >> >>> On Thu, Oct 12, 2017 at 11:24:57AM +0200, Gregory CLEMENT wrote: >> >> >>>> Hi Ard, >> >> >>>> >> >> >>>> Can we move forward to fix the booting problem ? >> >> >>>> >> >> >>>> What about amending your commit log with this new information and then >> >> >>>> submit it to Russell patch system? >> >> >>> >> >> >>> Well, I think there's a choice that needs to be made between this >> >> >>> approach and Arnd's approach. >> >> >>> >> >> >>> I'm not all that thrilled with the need to add explicit alignment to >> >> >>> data that is inherently a byte stream, and that invariably results in >> >> >>> unaligned data words even if you do align the start of it. That >> >> >>> sounds to me very much like a hack rather than a proper solution. >> >> >>> So, right now I'm leaning more towards Arnd's solution than Ard's >> >> >>> from what's been said in this thread. >> >> >>> >> >> >> >> >> >> I agree that the struct type unaligned accessors are the best choice >> >> >> for ARM in any case, given that it will also prevent hitting the >> >> >> alignment fixup handler in the kernel unnecessarily. >> >> >> >> >> >>> However, I don't recall Arnd's patch, it's probably buried deep in >> >> >>> my mailbox. >> >> >>> >> >> >> >> >> >> Well, unless you are considering changing the unaligned accessors from >> >> >> access_ok.h to le_struct.h as a bugfix, I think we need both patches. >> >> > >> >> > We will soon reach v4.14-rc6 and the Armada XP and Armada 370 still not >> >> > boot. I also didn't see your patch in rmk patch system. >> >> > >> >> > Waiting for you find a agreement an other option is to remove CONFIG_EFI >> >> > from multi_v7_defconfig as I don't really see any armv7 base board using >> >> > EFI. >> >> > >> >> >> >> It is up to Russell to decide how he wants to proceed. Russell? >> > >> > Well, having failed to attract Arnd's attention, I've spent 20 minutes >> > searching my mailbox to find it. >> > >> > It turns out that there was never a proper patch from Arnd - it was a >> > patch in pastebin. It's not ARM specific either, it's an asm-generic >> > change, for which Arnd is the maintainer for. >> > >> > I've just replied to that old thread and I've included Gregory and some >> > PXA folk who are also having alignment problems in the decompressor. It >> > could all be due to this same issue. >> > >> > There's also one additional factor that hasn't been considered, and I'm >> > scared to point it out, but: >> > >> > 1. Does Arnd's patch fix the PXA problem as well? >> > >> >> I don't think it will help configs that don't have >> HAVE_EFFICIENT_UNALIGNED_ACCESS set, given that they don't use >> access_ok.h in the first place. >> >> > 2. What is the performance impact of Arnd's fix? >> > >> >> Well, given that we may be relying on the alignment fixup handler to >> fix up kernel accesses, the performance impact could actually be >> favorable in some cases. But I don't have any numbers, nor do I have >> access to a representative sampling of ARM hardware so I can't be of >> any help here, unfortunately. > > Well, everyone can tell whether alignment fixups get used on their > platforms, by looking at /proc/cpu/alignment - this counts any > alignment faults and their types. If it's all zeros, then the > alignment fixup handler isn't being used. > > I just checked one of my imx6 platforms (3G/wifi gateway), all the > stats are zero. Another imx6 platform (whose port 80 is open to the > world) has one double-word fault in nsm_get_handle+0x200/0x484. > My ARMv5 gateway here also has all zero stats. > > However, all these are built with GCC 4.7.4, and we already know > that newer compilers behave significantly differently. So, I don't > think what I see here is representative, so I can't decide based on > what I see locally. > > So, it seems we're rather stuck. > > I suppose I could set up a dart board, and throw a dart blind folded > and if it hits a red area, pick one patch, otherwise take the other. > Or toss a coin. Or some other rediculous game. > Why would you have to choose between these patches? You can simply apply mine as a bug fix, and postpone the le_struct.h discussion for the next cycle. I know that does not fix PXA, but Arnd's patch is unlikely to fix that anyway. > It would be much better if there was some way to evaluate the impact, > but I see that no one is interested in lifting a finger to help with > that. > > Meanwhile, I'm quite sure there'll be an on-going cry for me to make > a decision. A decision based on a whim. Yea, great basis for taking > decisions. > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up > According to speedtest.net: 8.21Mbps down 510kbps up
On Fri, Oct 20, 2017 at 05:36:08PM +0100, Ard Biesheuvel wrote: > Why would you have to choose between these patches? You can simply > apply mine as a bug fix, and postpone the le_struct.h discussion for > the next cycle. It's not a choice between these two patches. I think we need Arnd's patch, because we know that fixes Romain Izard's problem. If I apply just your patch, then Gregory stops complaining, and there'll be nothing to remind us of this issue - Romain isn't shouting about it. That's a problem, because it's a real bug that then gets forgotten about. Or do we have evidence that your patch fixes Romain Izard's problem as well? I don't see any such suggestion. So, let me go back to the dart board... I just don't have the information to be able to make any decision on this, so stop asking me for a decision on something I know nothing about. Either provide me with some information I can base a decision on, or stop asking for a decision.
On 20 October 2017 at 17:54, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Fri, Oct 20, 2017 at 05:36:08PM +0100, Ard Biesheuvel wrote: >> Why would you have to choose between these patches? You can simply >> apply mine as a bug fix, and postpone the le_struct.h discussion for >> the next cycle. > > It's not a choice between these two patches. I think we need Arnd's > patch, because we know that fixes Romain Izard's problem. If I apply > just your patch, then Gregory stops complaining, and there'll be > nothing to remind us of this issue - Romain isn't shouting about it. > That's a problem, because it's a real bug that then gets forgotten > about. > > Or do we have evidence that your patch fixes Romain Izard's problem > as well? I don't see any such suggestion. > OK, so you are reluctant to apply patch A which fixes issue X, because it will result in you forgetting about patch B that fixes issue Y. I am sorry, but that does not make any sense to me. > So, let me go back to the dart board... I just don't have the > information to be able to make any decision on this, so stop asking > me for a decision on something I know nothing about. Either provide > me with some information I can base a decision on, or stop asking > for a decision. > Nobody is asking you to choose. I am asking you to apply this patch: piggydata used to be word aligned, and there is no harm in restoring that behavior explicitly.
On 09/08/2017 05:31 PM, Ard Biesheuvel wrote: > As it turns out, building the ARM kernel with EFI support pulls in > a couple of sections that we don't really need in the decompressor. > This is due to the fact the the UEFI stub uses sort() to sort the UEFI > memory map, which is an exported symbol pulled in from lib/sort.c. > > Before commit e4bae4d0b5f3 ("arm/efi: Split zImage code and data into > separate PE/COFF sections"), this resulted in the following layout > for the decompressor ELF binary. > > [Nr] Name Type Addr Off Size ES Flg Lk Inf Al > [ 0] NULL 00000000 000000 000000 00 0 0 0 > [ 1] .text PROGBITS 00000000 010000 009b3c 00 AX 0 0 512 > [ 2] .rodata PROGBITS 00009b3c 019b3c 001684 00 A 0 0 4 > [ 3] __ksymtab_strings PROGBITS 0000b1c0 01b1c0 000005 00 A 0 0 1 > [ 4] .data PROGBITS 0000b1c8 01b1c8 000020 00 WA 0 0 8 > [ 5] ___ksymtab+sort PROGBITS 0000b1e8 01b1e8 000008 00 WA 0 0 4 > [ 6] .piggydata PROGBITS 0000b1f0 01b1f0 77ac38 00 A 0 0 1 > [ 7] .got.plt PROGBITS 00785e28 795e28 00000c 04 WA 0 0 4 > [ 8] .got PROGBITS 00785e34 795e34 000028 00 WA 0 0 4 > [ 9] .pad PROGBITS 00785e5c 795e5c 000004 00 WA 0 0 1 > [10] .bss NOBITS 00785e60 795e60 00001c 00 WA 0 0 4 > [11] .stack NOBITS 00785e80 795e60 001000 00 WA 0 0 1 > > Commit e4bae4d0b5f3 made some changes to the linker script to allow the > UEFI firmware to map the decompressor with strict R-X/RW- permissions > before invoking it. Unfortunately, this turns out to break the boot on > some systems, because the linker now also moves the ksymtab/kcrctab > sections around, resulting in .piggydata to appear misaligned. > > [Nr] Name Type Addr Off Size ES Flg Lk Inf Al > [ 0] NULL 00000000 000000 000000 00 0 0 0 > [ 1] .text PROGBITS 00000000 010000 00a93c 00 AX 0 0 4096 > [ 2] .rodata PROGBITS 0000a93c 01a93c 001684 00 A 0 0 4 > [ 3] __ksymtab_strings PROGBITS 0000bfc0 01bfc0 000005 00 A 0 0 1 > [ 4] .piggydata PROGBITS 0000bfc5 01bfc5 77ac47 00 A 0 0 1 > [ 5] .got.plt PROGBITS 00786c0c 796c0c 00000c 04 WA 0 0 4 > [ 6] .got PROGBITS 00786c18 796c18 000028 00 WA 0 0 4 > [ 7] .pad PROGBITS 00786c40 796c40 000008 00 WA 0 0 1 > [ 8] .data PROGBITS 00787000 797000 000200 00 WA 0 0 4096 > [ 9] ___ksymtab+sort PROGBITS 00787200 797200 000008 00 WA 0 0 4 > [10] .bss NOBITS 00787208 797208 00001c 00 WA 0 0 4 > [11] .stack NOBITS 00787228 797208 001000 00 WA 0 0 1 > > So let's align piggydata explicitly, and discard these sections from the > binary. > > Cc: Russell King <linux@armlinux.org.uk> > Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate ...") > Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/arm/boot/compressed/piggy.S | 1 + > arch/arm/boot/compressed/vmlinux.lds.S | 1 + > 2 files changed, 2 insertions(+) > This fixes the boot regression on bananapi-r2. Thanks! Feel free to add: Tested-by: Matthias Brugger <mbrugger@suse.com> > diff --git a/arch/arm/boot/compressed/piggy.S b/arch/arm/boot/compressed/piggy.S > index f72088495f43..5d52c556dd32 100644 > --- a/arch/arm/boot/compressed/piggy.S > +++ b/arch/arm/boot/compressed/piggy.S > @@ -1,5 +1,6 @@ > .section .piggydata,#alloc > .globl input_data > + .align 2 > input_data: > .incbin "arch/arm/boot/compressed/piggy_data" > .globl input_data_end > diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S > index 7a4c59154361..5c5265be4605 100644 > --- a/arch/arm/boot/compressed/vmlinux.lds.S > +++ b/arch/arm/boot/compressed/vmlinux.lds.S > @@ -29,6 +29,7 @@ SECTIONS > * of the text/got segments. > */ > *(.data) > + *(*ksymtab* *kcrctab*) > } > > . = TEXT_START; >
On 21 October 2017 at 08:56, Matthias Brugger <mbrugger@suse.com> wrote: > On 09/08/2017 05:31 PM, Ard Biesheuvel wrote: >> As it turns out, building the ARM kernel with EFI support pulls in >> a couple of sections that we don't really need in the decompressor. >> This is due to the fact the the UEFI stub uses sort() to sort the UEFI >> memory map, which is an exported symbol pulled in from lib/sort.c. >> >> Before commit e4bae4d0b5f3 ("arm/efi: Split zImage code and data into >> separate PE/COFF sections"), this resulted in the following layout >> for the decompressor ELF binary. >> >> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >> [ 0] NULL 00000000 000000 000000 00 0 0 0 >> [ 1] .text PROGBITS 00000000 010000 009b3c 00 AX 0 0 512 >> [ 2] .rodata PROGBITS 00009b3c 019b3c 001684 00 A 0 0 4 >> [ 3] __ksymtab_strings PROGBITS 0000b1c0 01b1c0 000005 00 A 0 0 1 >> [ 4] .data PROGBITS 0000b1c8 01b1c8 000020 00 WA 0 0 8 >> [ 5] ___ksymtab+sort PROGBITS 0000b1e8 01b1e8 000008 00 WA 0 0 4 >> [ 6] .piggydata PROGBITS 0000b1f0 01b1f0 77ac38 00 A 0 0 1 >> [ 7] .got.plt PROGBITS 00785e28 795e28 00000c 04 WA 0 0 4 >> [ 8] .got PROGBITS 00785e34 795e34 000028 00 WA 0 0 4 >> [ 9] .pad PROGBITS 00785e5c 795e5c 000004 00 WA 0 0 1 >> [10] .bss NOBITS 00785e60 795e60 00001c 00 WA 0 0 4 >> [11] .stack NOBITS 00785e80 795e60 001000 00 WA 0 0 1 >> >> Commit e4bae4d0b5f3 made some changes to the linker script to allow the >> UEFI firmware to map the decompressor with strict R-X/RW- permissions >> before invoking it. Unfortunately, this turns out to break the boot on >> some systems, because the linker now also moves the ksymtab/kcrctab >> sections around, resulting in .piggydata to appear misaligned. >> >> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >> [ 0] NULL 00000000 000000 000000 00 0 0 0 >> [ 1] .text PROGBITS 00000000 010000 00a93c 00 AX 0 0 4096 >> [ 2] .rodata PROGBITS 0000a93c 01a93c 001684 00 A 0 0 4 >> [ 3] __ksymtab_strings PROGBITS 0000bfc0 01bfc0 000005 00 A 0 0 1 >> [ 4] .piggydata PROGBITS 0000bfc5 01bfc5 77ac47 00 A 0 0 1 >> [ 5] .got.plt PROGBITS 00786c0c 796c0c 00000c 04 WA 0 0 4 >> [ 6] .got PROGBITS 00786c18 796c18 000028 00 WA 0 0 4 >> [ 7] .pad PROGBITS 00786c40 796c40 000008 00 WA 0 0 1 >> [ 8] .data PROGBITS 00787000 797000 000200 00 WA 0 0 4096 >> [ 9] ___ksymtab+sort PROGBITS 00787200 797200 000008 00 WA 0 0 4 >> [10] .bss NOBITS 00787208 797208 00001c 00 WA 0 0 4 >> [11] .stack NOBITS 00787228 797208 001000 00 WA 0 0 1 >> >> So let's align piggydata explicitly, and discard these sections from the >> binary. >> >> Cc: Russell King <linux@armlinux.org.uk> >> Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate ...") >> Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> arch/arm/boot/compressed/piggy.S | 1 + >> arch/arm/boot/compressed/vmlinux.lds.S | 1 + >> 2 files changed, 2 insertions(+) >> > > This fixes the boot regression on bananapi-r2. > Thanks! Feel free to add: > > Tested-by: Matthias Brugger <mbrugger@suse.com> > Thanks for confirming Matthias. Could you please check whether this patch from Arnd https://marc.info/?l=linux-kernel&m=150852980119217&w=2 fixes the issue as well? (after reverting this one) Thanks, Ard.
Hi Ard, On 10/21/2017 10:14 AM, Ard Biesheuvel wrote: > On 21 October 2017 at 08:56, Matthias Brugger <mbrugger@suse.com> wrote: >> On 09/08/2017 05:31 PM, Ard Biesheuvel wrote: >>> As it turns out, building the ARM kernel with EFI support pulls in >>> a couple of sections that we don't really need in the decompressor. >>> This is due to the fact the the UEFI stub uses sort() to sort the UEFI >>> memory map, which is an exported symbol pulled in from lib/sort.c. >>> >>> Before commit e4bae4d0b5f3 ("arm/efi: Split zImage code and data into >>> separate PE/COFF sections"), this resulted in the following layout >>> for the decompressor ELF binary. >>> >>> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >>> [ 0] NULL 00000000 000000 000000 00 0 0 0 >>> [ 1] .text PROGBITS 00000000 010000 009b3c 00 AX 0 0 512 >>> [ 2] .rodata PROGBITS 00009b3c 019b3c 001684 00 A 0 0 4 >>> [ 3] __ksymtab_strings PROGBITS 0000b1c0 01b1c0 000005 00 A 0 0 1 >>> [ 4] .data PROGBITS 0000b1c8 01b1c8 000020 00 WA 0 0 8 >>> [ 5] ___ksymtab+sort PROGBITS 0000b1e8 01b1e8 000008 00 WA 0 0 4 >>> [ 6] .piggydata PROGBITS 0000b1f0 01b1f0 77ac38 00 A 0 0 1 >>> [ 7] .got.plt PROGBITS 00785e28 795e28 00000c 04 WA 0 0 4 >>> [ 8] .got PROGBITS 00785e34 795e34 000028 00 WA 0 0 4 >>> [ 9] .pad PROGBITS 00785e5c 795e5c 000004 00 WA 0 0 1 >>> [10] .bss NOBITS 00785e60 795e60 00001c 00 WA 0 0 4 >>> [11] .stack NOBITS 00785e80 795e60 001000 00 WA 0 0 1 >>> >>> Commit e4bae4d0b5f3 made some changes to the linker script to allow the >>> UEFI firmware to map the decompressor with strict R-X/RW- permissions >>> before invoking it. Unfortunately, this turns out to break the boot on >>> some systems, because the linker now also moves the ksymtab/kcrctab >>> sections around, resulting in .piggydata to appear misaligned. >>> >>> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >>> [ 0] NULL 00000000 000000 000000 00 0 0 0 >>> [ 1] .text PROGBITS 00000000 010000 00a93c 00 AX 0 0 4096 >>> [ 2] .rodata PROGBITS 0000a93c 01a93c 001684 00 A 0 0 4 >>> [ 3] __ksymtab_strings PROGBITS 0000bfc0 01bfc0 000005 00 A 0 0 1 >>> [ 4] .piggydata PROGBITS 0000bfc5 01bfc5 77ac47 00 A 0 0 1 >>> [ 5] .got.plt PROGBITS 00786c0c 796c0c 00000c 04 WA 0 0 4 >>> [ 6] .got PROGBITS 00786c18 796c18 000028 00 WA 0 0 4 >>> [ 7] .pad PROGBITS 00786c40 796c40 000008 00 WA 0 0 1 >>> [ 8] .data PROGBITS 00787000 797000 000200 00 WA 0 0 4096 >>> [ 9] ___ksymtab+sort PROGBITS 00787200 797200 000008 00 WA 0 0 4 >>> [10] .bss NOBITS 00787208 797208 00001c 00 WA 0 0 4 >>> [11] .stack NOBITS 00787228 797208 001000 00 WA 0 0 1 >>> >>> So let's align piggydata explicitly, and discard these sections from the >>> binary. >>> >>> Cc: Russell King <linux@armlinux.org.uk> >>> Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate ...") >>> Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> --- >>> arch/arm/boot/compressed/piggy.S | 1 + >>> arch/arm/boot/compressed/vmlinux.lds.S | 1 + >>> 2 files changed, 2 insertions(+) >>> >> >> This fixes the boot regression on bananapi-r2. >> Thanks! Feel free to add: >> >> Tested-by: Matthias Brugger <mbrugger@suse.com> >> > > Thanks for confirming Matthias. Could you please check whether this > patch from Arnd > > https://marc.info/?l=linux-kernel&m=150852980119217&w=2 > > fixes the issue as well? (after reverting this one) > After only applying this patch on top of v4.14-rc5 I was not able to boot. Regards, Matthias
On Mon, Oct 23, 2017 at 12:00:46AM +0200, Matthias Brugger wrote: > Hi Ard, > > On 10/21/2017 10:14 AM, Ard Biesheuvel wrote: > > On 21 October 2017 at 08:56, Matthias Brugger <mbrugger@suse.com> wrote: > >> On 09/08/2017 05:31 PM, Ard Biesheuvel wrote: > >>> As it turns out, building the ARM kernel with EFI support pulls in > >>> a couple of sections that we don't really need in the decompressor. > >>> This is due to the fact the the UEFI stub uses sort() to sort the UEFI > >>> memory map, which is an exported symbol pulled in from lib/sort.c. > >>> > >>> Before commit e4bae4d0b5f3 ("arm/efi: Split zImage code and data into > >>> separate PE/COFF sections"), this resulted in the following layout > >>> for the decompressor ELF binary. > >>> > >>> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al > >>> [ 0] NULL 00000000 000000 000000 00 0 0 0 > >>> [ 1] .text PROGBITS 00000000 010000 009b3c 00 AX 0 0 512 > >>> [ 2] .rodata PROGBITS 00009b3c 019b3c 001684 00 A 0 0 4 > >>> [ 3] __ksymtab_strings PROGBITS 0000b1c0 01b1c0 000005 00 A 0 0 1 > >>> [ 4] .data PROGBITS 0000b1c8 01b1c8 000020 00 WA 0 0 8 > >>> [ 5] ___ksymtab+sort PROGBITS 0000b1e8 01b1e8 000008 00 WA 0 0 4 > >>> [ 6] .piggydata PROGBITS 0000b1f0 01b1f0 77ac38 00 A 0 0 1 > >>> [ 7] .got.plt PROGBITS 00785e28 795e28 00000c 04 WA 0 0 4 > >>> [ 8] .got PROGBITS 00785e34 795e34 000028 00 WA 0 0 4 > >>> [ 9] .pad PROGBITS 00785e5c 795e5c 000004 00 WA 0 0 1 > >>> [10] .bss NOBITS 00785e60 795e60 00001c 00 WA 0 0 4 > >>> [11] .stack NOBITS 00785e80 795e60 001000 00 WA 0 0 1 > >>> > >>> Commit e4bae4d0b5f3 made some changes to the linker script to allow the > >>> UEFI firmware to map the decompressor with strict R-X/RW- permissions > >>> before invoking it. Unfortunately, this turns out to break the boot on > >>> some systems, because the linker now also moves the ksymtab/kcrctab > >>> sections around, resulting in .piggydata to appear misaligned. > >>> > >>> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al > >>> [ 0] NULL 00000000 000000 000000 00 0 0 0 > >>> [ 1] .text PROGBITS 00000000 010000 00a93c 00 AX 0 0 4096 > >>> [ 2] .rodata PROGBITS 0000a93c 01a93c 001684 00 A 0 0 4 > >>> [ 3] __ksymtab_strings PROGBITS 0000bfc0 01bfc0 000005 00 A 0 0 1 > >>> [ 4] .piggydata PROGBITS 0000bfc5 01bfc5 77ac47 00 A 0 0 1 > >>> [ 5] .got.plt PROGBITS 00786c0c 796c0c 00000c 04 WA 0 0 4 > >>> [ 6] .got PROGBITS 00786c18 796c18 000028 00 WA 0 0 4 > >>> [ 7] .pad PROGBITS 00786c40 796c40 000008 00 WA 0 0 1 > >>> [ 8] .data PROGBITS 00787000 797000 000200 00 WA 0 0 4096 > >>> [ 9] ___ksymtab+sort PROGBITS 00787200 797200 000008 00 WA 0 0 4 > >>> [10] .bss NOBITS 00787208 797208 00001c 00 WA 0 0 4 > >>> [11] .stack NOBITS 00787228 797208 001000 00 WA 0 0 1 > >>> > >>> So let's align piggydata explicitly, and discard these sections from the > >>> binary. > >>> > >>> Cc: Russell King <linux@armlinux.org.uk> > >>> Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate ...") > >>> Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com> > >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >>> --- > >>> arch/arm/boot/compressed/piggy.S | 1 + > >>> arch/arm/boot/compressed/vmlinux.lds.S | 1 + > >>> 2 files changed, 2 insertions(+) > >>> > >> > >> This fixes the boot regression on bananapi-r2. > >> Thanks! Feel free to add: > >> > >> Tested-by: Matthias Brugger <mbrugger@suse.com> > >> > > > > Thanks for confirming Matthias. Could you please check whether this > > patch from Arnd > > > > https://marc.info/?l=linux-kernel&m=150852980119217&w=2 > > > > fixes the issue as well? (after reverting this one) > > > > After only applying this patch on top of v4.14-rc5 I was not able to boot. Which decompression method are you using? What does objdump -h arch/arm/boot/vmlinux say? Thanks.
On Mon, Oct 23, 2017 at 10:29:56AM +0100, Russell King - ARM Linux wrote: > On Mon, Oct 23, 2017 at 12:00:46AM +0200, Matthias Brugger wrote: > > Hi Ard, > > > > On 10/21/2017 10:14 AM, Ard Biesheuvel wrote: > > > On 21 October 2017 at 08:56, Matthias Brugger <mbrugger@suse.com> wrote: > > >> On 09/08/2017 05:31 PM, Ard Biesheuvel wrote: > > >>> As it turns out, building the ARM kernel with EFI support pulls in > > >>> a couple of sections that we don't really need in the decompressor. > > >>> This is due to the fact the the UEFI stub uses sort() to sort the UEFI > > >>> memory map, which is an exported symbol pulled in from lib/sort.c. > > >>> > > >>> Before commit e4bae4d0b5f3 ("arm/efi: Split zImage code and data into > > >>> separate PE/COFF sections"), this resulted in the following layout > > >>> for the decompressor ELF binary. > > >>> > > >>> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al > > >>> [ 0] NULL 00000000 000000 000000 00 0 0 0 > > >>> [ 1] .text PROGBITS 00000000 010000 009b3c 00 AX 0 0 512 > > >>> [ 2] .rodata PROGBITS 00009b3c 019b3c 001684 00 A 0 0 4 > > >>> [ 3] __ksymtab_strings PROGBITS 0000b1c0 01b1c0 000005 00 A 0 0 1 > > >>> [ 4] .data PROGBITS 0000b1c8 01b1c8 000020 00 WA 0 0 8 > > >>> [ 5] ___ksymtab+sort PROGBITS 0000b1e8 01b1e8 000008 00 WA 0 0 4 > > >>> [ 6] .piggydata PROGBITS 0000b1f0 01b1f0 77ac38 00 A 0 0 1 > > >>> [ 7] .got.plt PROGBITS 00785e28 795e28 00000c 04 WA 0 0 4 > > >>> [ 8] .got PROGBITS 00785e34 795e34 000028 00 WA 0 0 4 > > >>> [ 9] .pad PROGBITS 00785e5c 795e5c 000004 00 WA 0 0 1 > > >>> [10] .bss NOBITS 00785e60 795e60 00001c 00 WA 0 0 4 > > >>> [11] .stack NOBITS 00785e80 795e60 001000 00 WA 0 0 1 > > >>> > > >>> Commit e4bae4d0b5f3 made some changes to the linker script to allow the > > >>> UEFI firmware to map the decompressor with strict R-X/RW- permissions > > >>> before invoking it. Unfortunately, this turns out to break the boot on > > >>> some systems, because the linker now also moves the ksymtab/kcrctab > > >>> sections around, resulting in .piggydata to appear misaligned. > > >>> > > >>> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al > > >>> [ 0] NULL 00000000 000000 000000 00 0 0 0 > > >>> [ 1] .text PROGBITS 00000000 010000 00a93c 00 AX 0 0 4096 > > >>> [ 2] .rodata PROGBITS 0000a93c 01a93c 001684 00 A 0 0 4 > > >>> [ 3] __ksymtab_strings PROGBITS 0000bfc0 01bfc0 000005 00 A 0 0 1 > > >>> [ 4] .piggydata PROGBITS 0000bfc5 01bfc5 77ac47 00 A 0 0 1 > > >>> [ 5] .got.plt PROGBITS 00786c0c 796c0c 00000c 04 WA 0 0 4 > > >>> [ 6] .got PROGBITS 00786c18 796c18 000028 00 WA 0 0 4 > > >>> [ 7] .pad PROGBITS 00786c40 796c40 000008 00 WA 0 0 1 > > >>> [ 8] .data PROGBITS 00787000 797000 000200 00 WA 0 0 4096 > > >>> [ 9] ___ksymtab+sort PROGBITS 00787200 797200 000008 00 WA 0 0 4 > > >>> [10] .bss NOBITS 00787208 797208 00001c 00 WA 0 0 4 > > >>> [11] .stack NOBITS 00787228 797208 001000 00 WA 0 0 1 > > >>> > > >>> So let's align piggydata explicitly, and discard these sections from the > > >>> binary. > > >>> > > >>> Cc: Russell King <linux@armlinux.org.uk> > > >>> Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate ...") > > >>> Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com> > > >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > >>> --- > > >>> arch/arm/boot/compressed/piggy.S | 1 + > > >>> arch/arm/boot/compressed/vmlinux.lds.S | 1 + > > >>> 2 files changed, 2 insertions(+) > > >>> > > >> > > >> This fixes the boot regression on bananapi-r2. > > >> Thanks! Feel free to add: > > >> > > >> Tested-by: Matthias Brugger <mbrugger@suse.com> > > >> > > > > > > Thanks for confirming Matthias. Could you please check whether this > > > patch from Arnd > > > > > > https://marc.info/?l=linux-kernel&m=150852980119217&w=2 > > > > > > fixes the issue as well? (after reverting this one) > > > > > > > After only applying this patch on top of v4.14-rc5 I was not able to boot. > > Which decompression method are you using? > > What does objdump -h arch/arm/boot/vmlinux say? Oh, and one more question: are you appending a dtb to your zImage (and if so, why are you doing that, that's supposed to be for obsolete boot loaders only.)
On 10/23/2017 11:29 AM, Russell King - ARM Linux wrote: > On Mon, Oct 23, 2017 at 12:00:46AM +0200, Matthias Brugger wrote: >> Hi Ard, >> >> On 10/21/2017 10:14 AM, Ard Biesheuvel wrote: >>> On 21 October 2017 at 08:56, Matthias Brugger <mbrugger@suse.com> wrote: >>>> On 09/08/2017 05:31 PM, Ard Biesheuvel wrote: >>>>> As it turns out, building the ARM kernel with EFI support pulls in >>>>> a couple of sections that we don't really need in the decompressor. >>>>> This is due to the fact the the UEFI stub uses sort() to sort the UEFI >>>>> memory map, which is an exported symbol pulled in from lib/sort.c. >>>>> >>>>> Before commit e4bae4d0b5f3 ("arm/efi: Split zImage code and data into >>>>> separate PE/COFF sections"), this resulted in the following layout >>>>> for the decompressor ELF binary. >>>>> >>>>> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >>>>> [ 0] NULL 00000000 000000 000000 00 0 0 0 >>>>> [ 1] .text PROGBITS 00000000 010000 009b3c 00 AX 0 0 512 >>>>> [ 2] .rodata PROGBITS 00009b3c 019b3c 001684 00 A 0 0 4 >>>>> [ 3] __ksymtab_strings PROGBITS 0000b1c0 01b1c0 000005 00 A 0 0 1 >>>>> [ 4] .data PROGBITS 0000b1c8 01b1c8 000020 00 WA 0 0 8 >>>>> [ 5] ___ksymtab+sort PROGBITS 0000b1e8 01b1e8 000008 00 WA 0 0 4 >>>>> [ 6] .piggydata PROGBITS 0000b1f0 01b1f0 77ac38 00 A 0 0 1 >>>>> [ 7] .got.plt PROGBITS 00785e28 795e28 00000c 04 WA 0 0 4 >>>>> [ 8] .got PROGBITS 00785e34 795e34 000028 00 WA 0 0 4 >>>>> [ 9] .pad PROGBITS 00785e5c 795e5c 000004 00 WA 0 0 1 >>>>> [10] .bss NOBITS 00785e60 795e60 00001c 00 WA 0 0 4 >>>>> [11] .stack NOBITS 00785e80 795e60 001000 00 WA 0 0 1 >>>>> >>>>> Commit e4bae4d0b5f3 made some changes to the linker script to allow the >>>>> UEFI firmware to map the decompressor with strict R-X/RW- permissions >>>>> before invoking it. Unfortunately, this turns out to break the boot on >>>>> some systems, because the linker now also moves the ksymtab/kcrctab >>>>> sections around, resulting in .piggydata to appear misaligned. >>>>> >>>>> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >>>>> [ 0] NULL 00000000 000000 000000 00 0 0 0 >>>>> [ 1] .text PROGBITS 00000000 010000 00a93c 00 AX 0 0 4096 >>>>> [ 2] .rodata PROGBITS 0000a93c 01a93c 001684 00 A 0 0 4 >>>>> [ 3] __ksymtab_strings PROGBITS 0000bfc0 01bfc0 000005 00 A 0 0 1 >>>>> [ 4] .piggydata PROGBITS 0000bfc5 01bfc5 77ac47 00 A 0 0 1 >>>>> [ 5] .got.plt PROGBITS 00786c0c 796c0c 00000c 04 WA 0 0 4 >>>>> [ 6] .got PROGBITS 00786c18 796c18 000028 00 WA 0 0 4 >>>>> [ 7] .pad PROGBITS 00786c40 796c40 000008 00 WA 0 0 1 >>>>> [ 8] .data PROGBITS 00787000 797000 000200 00 WA 0 0 4096 >>>>> [ 9] ___ksymtab+sort PROGBITS 00787200 797200 000008 00 WA 0 0 4 >>>>> [10] .bss NOBITS 00787208 797208 00001c 00 WA 0 0 4 >>>>> [11] .stack NOBITS 00787228 797208 001000 00 WA 0 0 1 >>>>> >>>>> So let's align piggydata explicitly, and discard these sections from the >>>>> binary. >>>>> >>>>> Cc: Russell King <linux@armlinux.org.uk> >>>>> Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate ...") >>>>> Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com> >>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>>> --- >>>>> arch/arm/boot/compressed/piggy.S | 1 + >>>>> arch/arm/boot/compressed/vmlinux.lds.S | 1 + >>>>> 2 files changed, 2 insertions(+) >>>>> >>>> >>>> This fixes the boot regression on bananapi-r2. >>>> Thanks! Feel free to add: >>>> >>>> Tested-by: Matthias Brugger <mbrugger@suse.com> >>>> >>> >>> Thanks for confirming Matthias. Could you please check whether this >>> patch from Arnd >>> >>> https://marc.info/?l=linux-kernel&m=150852980119217&w=2 >>> >>> fixes the issue as well? (after reverting this one) >>> >> >> After only applying this patch on top of v4.14-rc5 I was not able to boot. > > Which decompression method are you using? uImage/zImage > > What does objdump -h arch/arm/boot/vmlinux say? > arch/arm/boot/compressed/vmlinux: file format elf32-littlearm Sections: Idx Name Size VMA LMA File off Algn 0 .text 0000b06c 00000000 00000000 00010000 2**12 CONTENTS, ALLOC, LOAD, READONLY, CODE 1 .rodata 000016cc 0000b06c 0000b06c 0001b06c 2**2 CONTENTS, ALLOC, LOAD, READONLY, DATA 2 __ksymtab_strings 00000005 0000c738 0000c738 0001c738 2**0 CONTENTS, ALLOC, LOAD, READONLY, DATA 3 .piggydata 007ce9c5 0000c73d 0000c73d 0001c73d 2**0 CONTENTS, ALLOC, LOAD, READONLY, DATA 4 .got.plt 0000000c 007db104 007db104 007eb104 2**2 CONTENTS, ALLOC, LOAD, DATA 5 .got 00000028 007db110 007db110 007eb110 2**2 CONTENTS, ALLOC, LOAD, DATA 6 .pad 00000008 007db138 007db138 007eb138 2**0 CONTENTS, ALLOC, LOAD, DATA 7 .data 00000200 007dc000 007dc000 007ec000 2**12 CONTENTS, ALLOC, LOAD, DATA 8 ___ksymtab+sort 00000008 007dc200 007dc200 007ec200 2**2 CONTENTS, ALLOC, LOAD, DATA 9 .bss 0000001c 007dc208 007dc208 007ec208 2**2 ALLOC 10 .stack 00001000 007dc228 007dc228 007ec208 2**0 ALLOC 11 .comment 0000002d 00000000 00000000 007ec208 2**0 CONTENTS, READONLY 12 .ARM.attributes 0000002d 00000000 00000000 007ec235 2**0 CONTENTS, READONLY 13 .debug_line 00007088 00000000 00000000 007ec262 2**0 CONTENTS, READONLY, DEBUGGING 14 .debug_info 00057677 00000000 00000000 007f32ea 2**0 CONTENTS, READONLY, DEBUGGING 15 .debug_abbrev 00003bc2 00000000 00000000 0084a961 2**0 CONTENTS, READONLY, DEBUGGING 16 .debug_aranges 00000308 00000000 00000000 0084e528 2**3 CONTENTS, READONLY, DEBUGGING 17 .debug_ranges 00000e78 00000000 00000000 0084e830 2**3 CONTENTS, READONLY, DEBUGGING 18 .debug_frame 00002228 00000000 00000000 0084f6a8 2**2 CONTENTS, READONLY, DEBUGGING 19 .debug_loc 00005bff 00000000 00000000 008518d0 2**0 CONTENTS, READONLY, DEBUGGING 20 .debug_str 00008c41 00000000 00000000 008574cf 2**0 CONTENTS, READONLY, DEBUGGING This objdump is with Arnd Bergmans patch applied on top of v4.14-rc5. Beware that Ard provided a patch against efi/libstub which independently fixes the boot regression: "efi/libstub: arm: omit sorting of the UEFI memory map" Regards, Matthias
On 10/23/2017 01:48 PM, Russell King - ARM Linux wrote: > On Mon, Oct 23, 2017 at 10:29:56AM +0100, Russell King - ARM Linux wrote: >> On Mon, Oct 23, 2017 at 12:00:46AM +0200, Matthias Brugger wrote: >>> Hi Ard, >>> >>> On 10/21/2017 10:14 AM, Ard Biesheuvel wrote: >>>> On 21 October 2017 at 08:56, Matthias Brugger <mbrugger@suse.com> wrote: >>>>> On 09/08/2017 05:31 PM, Ard Biesheuvel wrote: >>>>>> As it turns out, building the ARM kernel with EFI support pulls in >>>>>> a couple of sections that we don't really need in the decompressor. >>>>>> This is due to the fact the the UEFI stub uses sort() to sort the UEFI >>>>>> memory map, which is an exported symbol pulled in from lib/sort.c. >>>>>> >>>>>> Before commit e4bae4d0b5f3 ("arm/efi: Split zImage code and data into >>>>>> separate PE/COFF sections"), this resulted in the following layout >>>>>> for the decompressor ELF binary. >>>>>> >>>>>> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >>>>>> [ 0] NULL 00000000 000000 000000 00 0 0 0 >>>>>> [ 1] .text PROGBITS 00000000 010000 009b3c 00 AX 0 0 512 >>>>>> [ 2] .rodata PROGBITS 00009b3c 019b3c 001684 00 A 0 0 4 >>>>>> [ 3] __ksymtab_strings PROGBITS 0000b1c0 01b1c0 000005 00 A 0 0 1 >>>>>> [ 4] .data PROGBITS 0000b1c8 01b1c8 000020 00 WA 0 0 8 >>>>>> [ 5] ___ksymtab+sort PROGBITS 0000b1e8 01b1e8 000008 00 WA 0 0 4 >>>>>> [ 6] .piggydata PROGBITS 0000b1f0 01b1f0 77ac38 00 A 0 0 1 >>>>>> [ 7] .got.plt PROGBITS 00785e28 795e28 00000c 04 WA 0 0 4 >>>>>> [ 8] .got PROGBITS 00785e34 795e34 000028 00 WA 0 0 4 >>>>>> [ 9] .pad PROGBITS 00785e5c 795e5c 000004 00 WA 0 0 1 >>>>>> [10] .bss NOBITS 00785e60 795e60 00001c 00 WA 0 0 4 >>>>>> [11] .stack NOBITS 00785e80 795e60 001000 00 WA 0 0 1 >>>>>> >>>>>> Commit e4bae4d0b5f3 made some changes to the linker script to allow the >>>>>> UEFI firmware to map the decompressor with strict R-X/RW- permissions >>>>>> before invoking it. Unfortunately, this turns out to break the boot on >>>>>> some systems, because the linker now also moves the ksymtab/kcrctab >>>>>> sections around, resulting in .piggydata to appear misaligned. >>>>>> >>>>>> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >>>>>> [ 0] NULL 00000000 000000 000000 00 0 0 0 >>>>>> [ 1] .text PROGBITS 00000000 010000 00a93c 00 AX 0 0 4096 >>>>>> [ 2] .rodata PROGBITS 0000a93c 01a93c 001684 00 A 0 0 4 >>>>>> [ 3] __ksymtab_strings PROGBITS 0000bfc0 01bfc0 000005 00 A 0 0 1 >>>>>> [ 4] .piggydata PROGBITS 0000bfc5 01bfc5 77ac47 00 A 0 0 1 >>>>>> [ 5] .got.plt PROGBITS 00786c0c 796c0c 00000c 04 WA 0 0 4 >>>>>> [ 6] .got PROGBITS 00786c18 796c18 000028 00 WA 0 0 4 >>>>>> [ 7] .pad PROGBITS 00786c40 796c40 000008 00 WA 0 0 1 >>>>>> [ 8] .data PROGBITS 00787000 797000 000200 00 WA 0 0 4096 >>>>>> [ 9] ___ksymtab+sort PROGBITS 00787200 797200 000008 00 WA 0 0 4 >>>>>> [10] .bss NOBITS 00787208 797208 00001c 00 WA 0 0 4 >>>>>> [11] .stack NOBITS 00787228 797208 001000 00 WA 0 0 1 >>>>>> >>>>>> So let's align piggydata explicitly, and discard these sections from the >>>>>> binary. >>>>>> >>>>>> Cc: Russell King <linux@armlinux.org.uk> >>>>>> Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate ...") >>>>>> Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com> >>>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>>>> --- >>>>>> arch/arm/boot/compressed/piggy.S | 1 + >>>>>> arch/arm/boot/compressed/vmlinux.lds.S | 1 + >>>>>> 2 files changed, 2 insertions(+) >>>>>> >>>>> >>>>> This fixes the boot regression on bananapi-r2. >>>>> Thanks! Feel free to add: >>>>> >>>>> Tested-by: Matthias Brugger <mbrugger@suse.com> >>>>> >>>> >>>> Thanks for confirming Matthias. Could you please check whether this >>>> patch from Arnd >>>> >>>> https://marc.info/?l=linux-kernel&m=150852980119217&w=2 >>>> >>>> fixes the issue as well? (after reverting this one) >>>> >>> >>> After only applying this patch on top of v4.14-rc5 I was not able to boot. >> >> Which decompression method are you using? >> >> What does objdump -h arch/arm/boot/vmlinux say? > > Oh, and one more question: are you appending a dtb to your zImage (and > if so, why are you doing that, that's supposed to be for obsolete boot > loaders only.) > Yes, I apply a dtb to my zImage. I do that because the board has no support on mainline u-boot right now and I do kernel enablement on the board. Support in u-boot is no priority ATM. Regards, Matthias
On 10/21/2017 10:14 AM, Ard Biesheuvel wrote: > On 21 October 2017 at 08:56, Matthias Brugger <mbrugger@suse.com> wrote: >> On 09/08/2017 05:31 PM, Ard Biesheuvel wrote: >>> As it turns out, building the ARM kernel with EFI support pulls in >>> a couple of sections that we don't really need in the decompressor. >>> This is due to the fact the the UEFI stub uses sort() to sort the UEFI >>> memory map, which is an exported symbol pulled in from lib/sort.c. >>> >>> Before commit e4bae4d0b5f3 ("arm/efi: Split zImage code and data into >>> separate PE/COFF sections"), this resulted in the following layout >>> for the decompressor ELF binary. >>> >>> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >>> [ 0] NULL 00000000 000000 000000 00 0 0 0 >>> [ 1] .text PROGBITS 00000000 010000 009b3c 00 AX 0 0 512 >>> [ 2] .rodata PROGBITS 00009b3c 019b3c 001684 00 A 0 0 4 >>> [ 3] __ksymtab_strings PROGBITS 0000b1c0 01b1c0 000005 00 A 0 0 1 >>> [ 4] .data PROGBITS 0000b1c8 01b1c8 000020 00 WA 0 0 8 >>> [ 5] ___ksymtab+sort PROGBITS 0000b1e8 01b1e8 000008 00 WA 0 0 4 >>> [ 6] .piggydata PROGBITS 0000b1f0 01b1f0 77ac38 00 A 0 0 1 >>> [ 7] .got.plt PROGBITS 00785e28 795e28 00000c 04 WA 0 0 4 >>> [ 8] .got PROGBITS 00785e34 795e34 000028 00 WA 0 0 4 >>> [ 9] .pad PROGBITS 00785e5c 795e5c 000004 00 WA 0 0 1 >>> [10] .bss NOBITS 00785e60 795e60 00001c 00 WA 0 0 4 >>> [11] .stack NOBITS 00785e80 795e60 001000 00 WA 0 0 1 >>> >>> Commit e4bae4d0b5f3 made some changes to the linker script to allow the >>> UEFI firmware to map the decompressor with strict R-X/RW- permissions >>> before invoking it. Unfortunately, this turns out to break the boot on >>> some systems, because the linker now also moves the ksymtab/kcrctab >>> sections around, resulting in .piggydata to appear misaligned. >>> >>> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >>> [ 0] NULL 00000000 000000 000000 00 0 0 0 >>> [ 1] .text PROGBITS 00000000 010000 00a93c 00 AX 0 0 4096 >>> [ 2] .rodata PROGBITS 0000a93c 01a93c 001684 00 A 0 0 4 >>> [ 3] __ksymtab_strings PROGBITS 0000bfc0 01bfc0 000005 00 A 0 0 1 >>> [ 4] .piggydata PROGBITS 0000bfc5 01bfc5 77ac47 00 A 0 0 1 >>> [ 5] .got.plt PROGBITS 00786c0c 796c0c 00000c 04 WA 0 0 4 >>> [ 6] .got PROGBITS 00786c18 796c18 000028 00 WA 0 0 4 >>> [ 7] .pad PROGBITS 00786c40 796c40 000008 00 WA 0 0 1 >>> [ 8] .data PROGBITS 00787000 797000 000200 00 WA 0 0 4096 >>> [ 9] ___ksymtab+sort PROGBITS 00787200 797200 000008 00 WA 0 0 4 >>> [10] .bss NOBITS 00787208 797208 00001c 00 WA 0 0 4 >>> [11] .stack NOBITS 00787228 797208 001000 00 WA 0 0 1 >>> >>> So let's align piggydata explicitly, and discard these sections from the >>> binary. >>> >>> Cc: Russell King <linux@armlinux.org.uk> >>> Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate ...") >>> Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> --- >>> arch/arm/boot/compressed/piggy.S | 1 + >>> arch/arm/boot/compressed/vmlinux.lds.S | 1 + >>> 2 files changed, 2 insertions(+) >>> >> >> This fixes the boot regression on bananapi-r2. >> Thanks! Feel free to add: >> >> Tested-by: Matthias Brugger <mbrugger@suse.com> >> > > Thanks for confirming Matthias. Could you please check whether this > patch from Arnd > > https://marc.info/?l=linux-kernel&m=150852980119217&w=2 > > fixes the issue as well? (after reverting this one) > For the record, gcc7 has a alignment problem which was fixed for PR82445 [1]. I tried to boot with the aforementioned patch from Arnd and the fixed gcc, but that didn't help. Regards, Matthias [1] https://github.com/gcc-mirror/gcc/commit/f59996b56aaa1c1d62a16cbb4010775b624cbde0
On Mon, Oct 23, 2017 at 11:17:35PM +0200, Matthias Brugger wrote: > Yes, I apply a dtb to my zImage. I do that because the board has no support on > mainline u-boot right now and I do kernel enablement on the board. Support in > u-boot is no priority ATM. Right, so the reason your system isn't booting is because of the additional section in the zImage, which causes additional bytes after _edata. That causes the appending of the dtb to be in the wrong place, so it doesn't get found. With Arnd's patch applied, if you do: dd if=arch/arm/boot/zImage bs=1 count=$(( $(stat -c %s arch/arm/boot/zImage) - 8)) of=zImg cat zImg $dtb > zImg.boot and then try booting zImg.boot instead, that should work, and would prove my point.
On Mon, Oct 23, 2017 at 11:32:11PM +0200, Matthias Brugger wrote: > For the record, gcc7 has a alignment problem which was fixed for PR82445 [1]. > > I tried to boot with the aforementioned patch from Arnd and the fixed > gcc, but that didn't help. That's because we have two entirely separate problems here, both leading to the same "it doesn't boot" outcome. 1. We have additional bytes in the zImage file after _edata, which means that appending a dtb using the usual "cat" method doesn't work. We look for the appended dtb at _edata. 2. Misaligned loads in the decompressor causing faults. While removing "sort" and the ksymtab sections appears to fix the problem, it does so by removing the troublesome sections. However, what it's actually highlighting is that we have more fundamental issues here. Additional sections that are not mentioned in the linker script will be passed through by the linker to the output file, and can result in exactly the same issue. So, a patch that discards the current sections doesn't really fix the issue, it papers it over. I also feel that removing the "sort" code from the EFI stub also papers over the problem. Both of those remove the /current/ cause of a more fundamental problem without addressing that fundamental problem. I'm not saying we shouldn't discard the sections, I'm saying we need to do more to detect the fundamental problem, rather than hiding it. That fundamental problem is that we allow the build to succeed when the results of the build are obviously incorrect. I've proposed a patch that causes the copy of vmlinux to zImage to fail if the zImage size does not match the expected size of the binary image. We should also eliminate the reason for (2), which is what Arnds patch addresses. So, in total, I think we need three patches: 1. Arnds patch to fix the alignment issues. 2. My patch to detect wrong zImage size. 3. A patch to discard troublesome sections. Optionally, removal of the sort code from the EFI stub is an orthogonal issue - the sort code is merely the vehicle by which the real problems have been found.
On 10/24/2017 12:19 AM, Russell King - ARM Linux wrote: > On Mon, Oct 23, 2017 at 11:17:35PM +0200, Matthias Brugger wrote: >> Yes, I apply a dtb to my zImage. I do that because the board has no support on >> mainline u-boot right now and I do kernel enablement on the board. Support in >> u-boot is no priority ATM. > > Right, so the reason your system isn't booting is because of the > additional section in the zImage, which causes additional bytes > after _edata. > > That causes the appending of the dtb to be in the wrong place, > so it doesn't get found. > > With Arnd's patch applied, if you do: > > dd if=arch/arm/boot/zImage bs=1 count=$(( $(stat -c %s arch/arm/boot/zImage) - 8)) of=zImg > cat zImg $dtb > zImg.boot > > and then try booting zImg.boot instead, that should work, and > would prove my point. > I can confirm that this fixes the issue. Regards, Matthias
On Mon, Oct 23, 2017 at 11:32 PM, Matthias Brugger <matthias.bgg@gmail.com> wrote: > > > On 10/21/2017 10:14 AM, Ard Biesheuvel wrote: >> On 21 October 2017 at 08:56, Matthias Brugger <mbrugger@suse.com> wrote: >>> On 09/08/2017 05:31 PM, Ard Biesheuvel wrote: >>>> As it turns out, building the ARM kernel with EFI support pulls in >>>> a couple of sections that we don't really need in the decompressor. >>>> This is due to the fact the the UEFI stub uses sort() to sort the UEFI >>>> memory map, which is an exported symbol pulled in from lib/sort.c. >>>> >>>> Before commit e4bae4d0b5f3 ("arm/efi: Split zImage code and data into >>>> separate PE/COFF sections"), this resulted in the following layout >>>> for the decompressor ELF binary. >>>> >>>> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >>>> [ 0] NULL 00000000 000000 000000 00 0 0 0 >>>> [ 1] .text PROGBITS 00000000 010000 009b3c 00 AX 0 0 512 >>>> [ 2] .rodata PROGBITS 00009b3c 019b3c 001684 00 A 0 0 4 >>>> [ 3] __ksymtab_strings PROGBITS 0000b1c0 01b1c0 000005 00 A 0 0 1 >>>> [ 4] .data PROGBITS 0000b1c8 01b1c8 000020 00 WA 0 0 8 >>>> [ 5] ___ksymtab+sort PROGBITS 0000b1e8 01b1e8 000008 00 WA 0 0 4 >>>> [ 6] .piggydata PROGBITS 0000b1f0 01b1f0 77ac38 00 A 0 0 1 >>>> [ 7] .got.plt PROGBITS 00785e28 795e28 00000c 04 WA 0 0 4 >>>> [ 8] .got PROGBITS 00785e34 795e34 000028 00 WA 0 0 4 >>>> [ 9] .pad PROGBITS 00785e5c 795e5c 000004 00 WA 0 0 1 >>>> [10] .bss NOBITS 00785e60 795e60 00001c 00 WA 0 0 4 >>>> [11] .stack NOBITS 00785e80 795e60 001000 00 WA 0 0 1 >>>> >>>> Commit e4bae4d0b5f3 made some changes to the linker script to allow the >>>> UEFI firmware to map the decompressor with strict R-X/RW- permissions >>>> before invoking it. Unfortunately, this turns out to break the boot on >>>> some systems, because the linker now also moves the ksymtab/kcrctab >>>> sections around, resulting in .piggydata to appear misaligned. >>>> >>>> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >>>> [ 0] NULL 00000000 000000 000000 00 0 0 0 >>>> [ 1] .text PROGBITS 00000000 010000 00a93c 00 AX 0 0 4096 >>>> [ 2] .rodata PROGBITS 0000a93c 01a93c 001684 00 A 0 0 4 >>>> [ 3] __ksymtab_strings PROGBITS 0000bfc0 01bfc0 000005 00 A 0 0 1 >>>> [ 4] .piggydata PROGBITS 0000bfc5 01bfc5 77ac47 00 A 0 0 1 >>>> [ 5] .got.plt PROGBITS 00786c0c 796c0c 00000c 04 WA 0 0 4 >>>> [ 6] .got PROGBITS 00786c18 796c18 000028 00 WA 0 0 4 >>>> [ 7] .pad PROGBITS 00786c40 796c40 000008 00 WA 0 0 1 >>>> [ 8] .data PROGBITS 00787000 797000 000200 00 WA 0 0 4096 >>>> [ 9] ___ksymtab+sort PROGBITS 00787200 797200 000008 00 WA 0 0 4 >>>> [10] .bss NOBITS 00787208 797208 00001c 00 WA 0 0 4 >>>> [11] .stack NOBITS 00787228 797208 001000 00 WA 0 0 1 >>>> >>>> So let's align piggydata explicitly, and discard these sections from the >>>> binary. >>>> >>>> Cc: Russell King <linux@armlinux.org.uk> >>>> Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate ...") >>>> Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com> >>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>> --- >>>> arch/arm/boot/compressed/piggy.S | 1 + >>>> arch/arm/boot/compressed/vmlinux.lds.S | 1 + >>>> 2 files changed, 2 insertions(+) >>>> >>> >>> This fixes the boot regression on bananapi-r2. >>> Thanks! Feel free to add: >>> >>> Tested-by: Matthias Brugger <mbrugger@suse.com> >>> >> >> Thanks for confirming Matthias. Could you please check whether this >> patch from Arnd >> >> https://marc.info/?l=linux-kernel&m=150852980119217&w=2 >> >> fixes the issue as well? (after reverting this one) >> > > For the record, gcc7 has a alignment problem which was fixed for PR82445 [1]. > This fixes the decompressor error on pxa [1]. Thanks for the pointer! Cheers Andrea [1] https://www.spinics.net/lists/arm-kernel/msg594654.html > I tried to boot with the aforementioned patch from Arnd and the fixed gcc, but > that didn't help. > > Regards, > Matthias > > [1] > https://github.com/gcc-mirror/gcc/commit/f59996b56aaa1c1d62a16cbb4010775b624cbde0 > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/arch/arm/boot/compressed/piggy.S b/arch/arm/boot/compressed/piggy.S index f72088495f43..5d52c556dd32 100644 --- a/arch/arm/boot/compressed/piggy.S +++ b/arch/arm/boot/compressed/piggy.S @@ -1,5 +1,6 @@ .section .piggydata,#alloc .globl input_data + .align 2 input_data: .incbin "arch/arm/boot/compressed/piggy_data" .globl input_data_end diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S index 7a4c59154361..5c5265be4605 100644 --- a/arch/arm/boot/compressed/vmlinux.lds.S +++ b/arch/arm/boot/compressed/vmlinux.lds.S @@ -29,6 +29,7 @@ SECTIONS * of the text/got segments. */ *(.data) + *(*ksymtab* *kcrctab*) } . = TEXT_START;
As it turns out, building the ARM kernel with EFI support pulls in a couple of sections that we don't really need in the decompressor. This is due to the fact the the UEFI stub uses sort() to sort the UEFI memory map, which is an exported symbol pulled in from lib/sort.c. Before commit e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections"), this resulted in the following layout for the decompressor ELF binary. [Nr] Name Type Addr Off Size ES Flg Lk Inf Al [ 0] NULL 00000000 000000 000000 00 0 0 0 [ 1] .text PROGBITS 00000000 010000 009b3c 00 AX 0 0 512 [ 2] .rodata PROGBITS 00009b3c 019b3c 001684 00 A 0 0 4 [ 3] __ksymtab_strings PROGBITS 0000b1c0 01b1c0 000005 00 A 0 0 1 [ 4] .data PROGBITS 0000b1c8 01b1c8 000020 00 WA 0 0 8 [ 5] ___ksymtab+sort PROGBITS 0000b1e8 01b1e8 000008 00 WA 0 0 4 [ 6] .piggydata PROGBITS 0000b1f0 01b1f0 77ac38 00 A 0 0 1 [ 7] .got.plt PROGBITS 00785e28 795e28 00000c 04 WA 0 0 4 [ 8] .got PROGBITS 00785e34 795e34 000028 00 WA 0 0 4 [ 9] .pad PROGBITS 00785e5c 795e5c 000004 00 WA 0 0 1 [10] .bss NOBITS 00785e60 795e60 00001c 00 WA 0 0 4 [11] .stack NOBITS 00785e80 795e60 001000 00 WA 0 0 1 Commit e4bae4d0b5f3 made some changes to the linker script to allow the UEFI firmware to map the decompressor with strict R-X/RW- permissions before invoking it. Unfortunately, this turns out to break the boot on some systems, because the linker now also moves the ksymtab/kcrctab sections around, resulting in .piggydata to appear misaligned. [Nr] Name Type Addr Off Size ES Flg Lk Inf Al [ 0] NULL 00000000 000000 000000 00 0 0 0 [ 1] .text PROGBITS 00000000 010000 00a93c 00 AX 0 0 4096 [ 2] .rodata PROGBITS 0000a93c 01a93c 001684 00 A 0 0 4 [ 3] __ksymtab_strings PROGBITS 0000bfc0 01bfc0 000005 00 A 0 0 1 [ 4] .piggydata PROGBITS 0000bfc5 01bfc5 77ac47 00 A 0 0 1 [ 5] .got.plt PROGBITS 00786c0c 796c0c 00000c 04 WA 0 0 4 [ 6] .got PROGBITS 00786c18 796c18 000028 00 WA 0 0 4 [ 7] .pad PROGBITS 00786c40 796c40 000008 00 WA 0 0 1 [ 8] .data PROGBITS 00787000 797000 000200 00 WA 0 0 4096 [ 9] ___ksymtab+sort PROGBITS 00787200 797200 000008 00 WA 0 0 4 [10] .bss NOBITS 00787208 797208 00001c 00 WA 0 0 4 [11] .stack NOBITS 00787228 797208 001000 00 WA 0 0 1 So let's align piggydata explicitly, and discard these sections from the binary. Cc: Russell King <linux@armlinux.org.uk> Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate ...") Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- arch/arm/boot/compressed/piggy.S | 1 + arch/arm/boot/compressed/vmlinux.lds.S | 1 + 2 files changed, 2 insertions(+)