| Message ID | 20171020200231.1355569-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 20 October 2017 at 21:01, Arnd Bergmann <arnd@arndb.de> wrote: > The asm-generic/unaligned.h header provides two different implementations > for accessing unaligned variables: the access_ok.h version used when > CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is set pretends that all pointers > are in fact aligned, while the le_struct.h version convinces gcc that the > alignment of a pointer is '1', to make it issue the correct load/store > instructions depending on the architecture flags. > > On ARMv5 and older, we always use the second version, to let the compiler > use byte accesses. On ARMv6 and newer, we currently use the access_ok.h > version, so the compiler can use any instruction including stm/ldm and > ldrd/strd that will cause an alignment trap. This trap can significantly > impact performance when we have to do a lot of fixups and, worse, has > led to crashes in the LZ4 decompressor code that does not have a trap > handler. > > This adds an ARM specific version of asm/unaligned.h that uses the > le_struct.h/be_struct.h implementation unconditionally. This should lead > to essentially the same code on ARMv6+ as before, with the exception of > using regular load/store instructions instead of the trapping instructions > multi-register variants. > > The crash in the LZ4 decompressor code was probably introduced by the > patch replacing the LZ4 implementation, commit 4e1a33b105dd ("lib: update > LZ4 compressor module"), so linux-4.11 and higher would be affected most. > However, we probably want to have this backported to all older stable > kernels as well, to help with the performance issues. > > There are two follow-ups that I think we should also work on, but not > backport to stable kernels, first to change the asm-generic version of > the header to remove the ARM special case, and second to review all > other uses of CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS to see if they > might be affected by the same problem on ARM. > > Cc: stable@vger.kernel.org > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > Untested so far, please verify that this fixes all the known problems > with the alignment traps. > --- > arch/arm/include/asm/Kbuild | 1 - > arch/arm/include/asm/unaligned.h | 27 +++++++++++++++++++++++++++ > 2 files changed, 27 insertions(+), 1 deletion(-) > create mode 100644 arch/arm/include/asm/unaligned.h > > diff --git a/arch/arm/include/asm/Kbuild b/arch/arm/include/asm/Kbuild > index 721ab5ecfb9b..0f2c8a2a8131 100644 > --- a/arch/arm/include/asm/Kbuild > +++ b/arch/arm/include/asm/Kbuild > @@ -20,7 +20,6 @@ generic-y += simd.h > generic-y += sizes.h > generic-y += timex.h > generic-y += trace_clock.h > -generic-y += unaligned.h > > generated-y += mach-types.h > generated-y += unistd-nr.h > diff --git a/arch/arm/include/asm/unaligned.h b/arch/arm/include/asm/unaligned.h > new file mode 100644 > index 000000000000..ab905ffcf193 > --- /dev/null > +++ b/arch/arm/include/asm/unaligned.h > @@ -0,0 +1,27 @@ > +#ifndef __ASM_ARM_UNALIGNED_H > +#define __ASM_ARM_UNALIGNED_H > + > +/* > + * We generally want to set CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS on ARMv6+, > + * but we don't want to use linux/unaligned/access_ok.h since that can lead > + * to traps on unaligned stm/ldm or strd/ldrd. > + */ > +#include <asm/byteorder.h> > + > +#if defined(__LITTLE_ENDIAN) > +# include <linux/unaligned/le_struct.h> > +# include <linux/unaligned/be_byteshift.h> > +# include <linux/unaligned/generic.h> > +# define get_unaligned __get_unaligned_le > +# define put_unaligned __put_unaligned_le > +#elif defined(__BIG_ENDIAN) > +# include <linux/unaligned/be_struct.h> > +# include <linux/unaligned/le_byteshift.h> > +# include <linux/unaligned/generic.h> > +# define get_unaligned __get_unaligned_be > +# define put_unaligned __put_unaligned_be > +#else > +# error need to define endianess > +#endif > + > +#endif /* __ASM_ARM_UNALIGNED_H */ > -- > 2.9.0 >
2017-10-20 22:01 GMT+02:00 Arnd Bergmann <arnd@arndb.de>: > The asm-generic/unaligned.h header provides two different implementations > for accessing unaligned variables: the access_ok.h version used when > CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is set pretends that all pointers > are in fact aligned, while the le_struct.h version convinces gcc that the > alignment of a pointer is '1', to make it issue the correct load/store > instructions depending on the architecture flags. > > On ARMv5 and older, we always use the second version, to let the compiler > use byte accesses. On ARMv6 and newer, we currently use the access_ok.h > version, so the compiler can use any instruction including stm/ldm and > ldrd/strd that will cause an alignment trap. This trap can significantly > impact performance when we have to do a lot of fixups and, worse, has > led to crashes in the LZ4 decompressor code that does not have a trap > handler. > > This adds an ARM specific version of asm/unaligned.h that uses the > le_struct.h/be_struct.h implementation unconditionally. This should lead > to essentially the same code on ARMv6+ as before, with the exception of > using regular load/store instructions instead of the trapping instructions > multi-register variants. > > The crash in the LZ4 decompressor code was probably introduced by the > patch replacing the LZ4 implementation, commit 4e1a33b105dd ("lib: update > LZ4 compressor module"), so linux-4.11 and higher would be affected most. > However, we probably want to have this backported to all older stable > kernels as well, to help with the performance issues. > > There are two follow-ups that I think we should also work on, but not > backport to stable kernels, first to change the asm-generic version of > the header to remove the ARM special case, and second to review all > other uses of CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS to see if they > might be affected by the same problem on ARM. > > Cc: stable@vger.kernel.org > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Tested-by: Romain.Izard <romain.izard.pro@gmail.com> Without this patch, the LZ4 decompression code is invalid in v4.14-rc6, when compiled with arm-linux-gnueabihf-gcc, version 7.2. The zImage does not start. With this patch, the decompression code is valid, and the kernel boots without a problem. No fixups are reported in /proc/cpu/alignment.
Hi Arnd, On ven., oct. 20 2017, Arnd Bergmann <arnd@arndb.de> wrote: > The asm-generic/unaligned.h header provides two different implementations > for accessing unaligned variables: the access_ok.h version used when > CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is set pretends that all pointers > are in fact aligned, while the le_struct.h version convinces gcc that the > alignment of a pointer is '1', to make it issue the correct load/store > instructions depending on the architecture flags. > > On ARMv5 and older, we always use the second version, to let the compiler > use byte accesses. On ARMv6 and newer, we currently use the access_ok.h > version, so the compiler can use any instruction including stm/ldm and > ldrd/strd that will cause an alignment trap. This trap can significantly > impact performance when we have to do a lot of fixups and, worse, has > led to crashes in the LZ4 decompressor code that does not have a trap > handler. > > This adds an ARM specific version of asm/unaligned.h that uses the > le_struct.h/be_struct.h implementation unconditionally. This should lead > to essentially the same code on ARMv6+ as before, with the exception of > using regular load/store instructions instead of the trapping instructions > multi-register variants. > > The crash in the LZ4 decompressor code was probably introduced by the > patch replacing the LZ4 implementation, commit 4e1a33b105dd ("lib: update > LZ4 compressor module"), so linux-4.11 and higher would be affected most. > However, we probably want to have this backported to all older stable > kernels as well, to help with the performance issues. > > There are two follow-ups that I think we should also work on, but not > backport to stable kernels, first to change the asm-generic version of > the header to remove the ARM special case, and second to review all > other uses of CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS to see if they > might be affected by the same problem on ARM. > > Cc: stable@vger.kernel.org > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > Untested so far, please verify that this fixes all the known problems > with the alignment traps. I think Russell already find this conclusion but this patch didn't solve my boot issue with dtb append. I tested this patch onto a v4.14-rc6. Then at least with the patch from Ard: "efi/libstub: arm: omit sorting of the UEFI memory map", it didn't prevent booting. Gregory > --- > arch/arm/include/asm/Kbuild | 1 - > arch/arm/include/asm/unaligned.h | 27 +++++++++++++++++++++++++++ > 2 files changed, 27 insertions(+), 1 deletion(-) > create mode 100644 arch/arm/include/asm/unaligned.h > > diff --git a/arch/arm/include/asm/Kbuild b/arch/arm/include/asm/Kbuild > index 721ab5ecfb9b..0f2c8a2a8131 100644 > --- a/arch/arm/include/asm/Kbuild > +++ b/arch/arm/include/asm/Kbuild > @@ -20,7 +20,6 @@ generic-y += simd.h > generic-y += sizes.h > generic-y += timex.h > generic-y += trace_clock.h > -generic-y += unaligned.h > > generated-y += mach-types.h > generated-y += unistd-nr.h > diff --git a/arch/arm/include/asm/unaligned.h b/arch/arm/include/asm/unaligned.h > new file mode 100644 > index 000000000000..ab905ffcf193 > --- /dev/null > +++ b/arch/arm/include/asm/unaligned.h > @@ -0,0 +1,27 @@ > +#ifndef __ASM_ARM_UNALIGNED_H > +#define __ASM_ARM_UNALIGNED_H > + > +/* > + * We generally want to set CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS on ARMv6+, > + * but we don't want to use linux/unaligned/access_ok.h since that can lead > + * to traps on unaligned stm/ldm or strd/ldrd. > + */ > +#include <asm/byteorder.h> > + > +#if defined(__LITTLE_ENDIAN) > +# include <linux/unaligned/le_struct.h> > +# include <linux/unaligned/be_byteshift.h> > +# include <linux/unaligned/generic.h> > +# define get_unaligned __get_unaligned_le > +# define put_unaligned __put_unaligned_le > +#elif defined(__BIG_ENDIAN) > +# include <linux/unaligned/be_struct.h> > +# include <linux/unaligned/le_byteshift.h> > +# include <linux/unaligned/generic.h> > +# define get_unaligned __get_unaligned_be > +# define put_unaligned __put_unaligned_be > +#else > +# error need to define endianess > +#endif > + > +#endif /* __ASM_ARM_UNALIGNED_H */ > -- > 2.9.0 >
On Fri, Oct 27, 2017 at 05:19:55PM +0200, Gregory CLEMENT wrote: > Hi Arnd, > > On ven., oct. 20 2017, Arnd Bergmann <arnd@arndb.de> wrote: > > > The asm-generic/unaligned.h header provides two different implementations > > for accessing unaligned variables: the access_ok.h version used when > > CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is set pretends that all pointers > > are in fact aligned, while the le_struct.h version convinces gcc that the > > alignment of a pointer is '1', to make it issue the correct load/store > > instructions depending on the architecture flags. > > > > On ARMv5 and older, we always use the second version, to let the compiler > > use byte accesses. On ARMv6 and newer, we currently use the access_ok.h > > version, so the compiler can use any instruction including stm/ldm and > > ldrd/strd that will cause an alignment trap. This trap can significantly > > impact performance when we have to do a lot of fixups and, worse, has > > led to crashes in the LZ4 decompressor code that does not have a trap > > handler. > > > > This adds an ARM specific version of asm/unaligned.h that uses the > > le_struct.h/be_struct.h implementation unconditionally. This should lead > > to essentially the same code on ARMv6+ as before, with the exception of > > using regular load/store instructions instead of the trapping instructions > > multi-register variants. > > > > The crash in the LZ4 decompressor code was probably introduced by the > > patch replacing the LZ4 implementation, commit 4e1a33b105dd ("lib: update > > LZ4 compressor module"), so linux-4.11 and higher would be affected most. > > However, we probably want to have this backported to all older stable > > kernels as well, to help with the performance issues. > > > > There are two follow-ups that I think we should also work on, but not > > backport to stable kernels, first to change the asm-generic version of > > the header to remove the ARM special case, and second to review all > > other uses of CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS to see if they > > might be affected by the same problem on ARM. > > > > Cc: stable@vger.kernel.org > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > --- > > Untested so far, please verify that this fixes all the known problems > > with the alignment traps. > > I think Russell already find this conclusion but this patch didn't solve > my boot issue with dtb append. > > I tested this patch onto a v4.14-rc6. > > Then at least with the patch from Ard: "efi/libstub: arm: omit sorting > of the UEFI memory map", it didn't prevent booting. There's three things wrong, all of which I have patches to address: 1. The decompressor code reading the image data sometimes issues unaligned reads. Some compilers get this wrong and cause an abort. Arnds patch addresses this. 2. Additional sections can appear in the zImage binary which adds extra bytes on the end of the image. Concatenating the zImage with the extra bytes onto a DTB is the same thing as doing this: cat zImage extrabytes foo.dtb > image and the decompressor tolerates no additional bytes between the _official_ end of the zImage and the DTB. I've added a patch which detects this situation and fails the kernel build when it happens. 3. Ard's patch "efi/libstub: arm: omit sorting of the UEFI memory map" gets rid of the additional sections that (a) change the alignment of the compressed data, and (b) add additional unexpected bytes on the end of zImage. So, merely applying Ard's patch will appear to fix the problem, but leave the actual underlying issues - so next time we have an additional section appear in the zImage, we'd go through all this pain again. These other patches are required to either make the decompressor tolerant or catch the problem cases while they're still in the developer's tree.
Hi Russell, On ven., oct. 27 2017, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Fri, Oct 27, 2017 at 05:19:55PM +0200, Gregory CLEMENT wrote: >> Hi Arnd, >> >> On ven., oct. 20 2017, Arnd Bergmann <arnd@arndb.de> wrote: >> >> > The asm-generic/unaligned.h header provides two different implementations >> > for accessing unaligned variables: the access_ok.h version used when >> > CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is set pretends that all pointers >> > are in fact aligned, while the le_struct.h version convinces gcc that the >> > alignment of a pointer is '1', to make it issue the correct load/store >> > instructions depending on the architecture flags. >> > >> > On ARMv5 and older, we always use the second version, to let the compiler >> > use byte accesses. On ARMv6 and newer, we currently use the access_ok.h >> > version, so the compiler can use any instruction including stm/ldm and >> > ldrd/strd that will cause an alignment trap. This trap can significantly >> > impact performance when we have to do a lot of fixups and, worse, has >> > led to crashes in the LZ4 decompressor code that does not have a trap >> > handler. >> > >> > This adds an ARM specific version of asm/unaligned.h that uses the >> > le_struct.h/be_struct.h implementation unconditionally. This should lead >> > to essentially the same code on ARMv6+ as before, with the exception of >> > using regular load/store instructions instead of the trapping instructions >> > multi-register variants. >> > >> > The crash in the LZ4 decompressor code was probably introduced by the >> > patch replacing the LZ4 implementation, commit 4e1a33b105dd ("lib: update >> > LZ4 compressor module"), so linux-4.11 and higher would be affected most. >> > However, we probably want to have this backported to all older stable >> > kernels as well, to help with the performance issues. >> > >> > There are two follow-ups that I think we should also work on, but not >> > backport to stable kernels, first to change the asm-generic version of >> > the header to remove the ARM special case, and second to review all >> > other uses of CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS to see if they >> > might be affected by the same problem on ARM. >> > >> > Cc: stable@vger.kernel.org >> > Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> > --- >> > Untested so far, please verify that this fixes all the known problems >> > with the alignment traps. >> >> I think Russell already find this conclusion but this patch didn't solve >> my boot issue with dtb append. >> >> I tested this patch onto a v4.14-rc6. >> >> Then at least with the patch from Ard: "efi/libstub: arm: omit sorting >> of the UEFI memory map", it didn't prevent booting. > > There's three things wrong, all of which I have patches to address: > > 1. The decompressor code reading the image data sometimes issues unaligned > reads. Some compilers get this wrong and cause an abort. Arnds patch > addresses this. > > 2. Additional sections can appear in the zImage binary which adds extra > bytes on the end of the image. Concatenating the zImage with the > extra bytes onto a DTB is the same thing as doing this: > > cat zImage extrabytes foo.dtb > image > > and the decompressor tolerates no additional bytes between the > _official_ end of the zImage and the DTB. I've added a patch which > detects this situation and fails the kernel build when it happens. So I tested the branch fixes in your git tree. After doing a "make multi_v7_defconfig; make zImage", I got the message "arm-linux-gnueabi-ld: error: zImage file size is incorrect" you added in the commit "ARM: verify size of zImage". It is the same with mvebu_v7_defconfig, so I wonder wich with configuration this patch was tested ? Gregory > > 3. Ard's patch "efi/libstub: arm: omit sorting of the UEFI memory map" > gets rid of the additional sections that (a) change the alignment > of the compressed data, and (b) add additional unexpected bytes on > the end of zImage. > > So, merely applying Ard's patch will appear to fix the problem, but leave > the actual underlying issues - so next time we have an additional section > appear in the zImage, we'd go through all this pain again. > > These other patches are required to either make the decompressor tolerant > or catch the problem cases while they're still in the developer's tree. > > -- > 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 30 October 2017 at 13:48, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > Hi Russell, > > On ven., oct. 27 2017, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > >> On Fri, Oct 27, 2017 at 05:19:55PM +0200, Gregory CLEMENT wrote: >>> Hi Arnd, >>> >>> On ven., oct. 20 2017, Arnd Bergmann <arnd@arndb.de> wrote: >>> >>> > The asm-generic/unaligned.h header provides two different implementations >>> > for accessing unaligned variables: the access_ok.h version used when >>> > CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is set pretends that all pointers >>> > are in fact aligned, while the le_struct.h version convinces gcc that the >>> > alignment of a pointer is '1', to make it issue the correct load/store >>> > instructions depending on the architecture flags. >>> > >>> > On ARMv5 and older, we always use the second version, to let the compiler >>> > use byte accesses. On ARMv6 and newer, we currently use the access_ok.h >>> > version, so the compiler can use any instruction including stm/ldm and >>> > ldrd/strd that will cause an alignment trap. This trap can significantly >>> > impact performance when we have to do a lot of fixups and, worse, has >>> > led to crashes in the LZ4 decompressor code that does not have a trap >>> > handler. >>> > >>> > This adds an ARM specific version of asm/unaligned.h that uses the >>> > le_struct.h/be_struct.h implementation unconditionally. This should lead >>> > to essentially the same code on ARMv6+ as before, with the exception of >>> > using regular load/store instructions instead of the trapping instructions >>> > multi-register variants. >>> > >>> > The crash in the LZ4 decompressor code was probably introduced by the >>> > patch replacing the LZ4 implementation, commit 4e1a33b105dd ("lib: update >>> > LZ4 compressor module"), so linux-4.11 and higher would be affected most. >>> > However, we probably want to have this backported to all older stable >>> > kernels as well, to help with the performance issues. >>> > >>> > There are two follow-ups that I think we should also work on, but not >>> > backport to stable kernels, first to change the asm-generic version of >>> > the header to remove the ARM special case, and second to review all >>> > other uses of CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS to see if they >>> > might be affected by the same problem on ARM. >>> > >>> > Cc: stable@vger.kernel.org >>> > Signed-off-by: Arnd Bergmann <arnd@arndb.de> >>> > --- >>> > Untested so far, please verify that this fixes all the known problems >>> > with the alignment traps. >>> >>> I think Russell already find this conclusion but this patch didn't solve >>> my boot issue with dtb append. >>> >>> I tested this patch onto a v4.14-rc6. >>> >>> Then at least with the patch from Ard: "efi/libstub: arm: omit sorting >>> of the UEFI memory map", it didn't prevent booting. >> >> There's three things wrong, all of which I have patches to address: >> >> 1. The decompressor code reading the image data sometimes issues unaligned >> reads. Some compilers get this wrong and cause an abort. Arnds patch >> addresses this. >> >> 2. Additional sections can appear in the zImage binary which adds extra >> bytes on the end of the image. Concatenating the zImage with the >> extra bytes onto a DTB is the same thing as doing this: >> >> cat zImage extrabytes foo.dtb > image >> >> and the decompressor tolerates no additional bytes between the >> _official_ end of the zImage and the DTB. I've added a patch which >> detects this situation and fails the kernel build when it happens. > > So I tested the branch fixes in your git tree. > > After doing a "make multi_v7_defconfig; make zImage", I got the message > "arm-linux-gnueabi-ld: error: zImage file size is incorrect" you added > in the commit "ARM: verify size of zImage". > > It is the same with mvebu_v7_defconfig, so I wonder wich with > configuration this patch was tested ? > Could you please share the output of 'readelf -S' for those vmlinux decompressor images?
Hi Ard, On lun., oct. 30 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 30 October 2017 at 13:48, Gregory CLEMENT > <gregory.clement@free-electrons.com> wrote: >> Hi Russell, >> >> On ven., oct. 27 2017, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: >> >>> On Fri, Oct 27, 2017 at 05:19:55PM +0200, Gregory CLEMENT wrote: >>>> Hi Arnd, >>>> >>>> On ven., oct. 20 2017, Arnd Bergmann <arnd@arndb.de> wrote: >>>> >>>> > The asm-generic/unaligned.h header provides two different implementations >>>> > for accessing unaligned variables: the access_ok.h version used when >>>> > CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is set pretends that all pointers >>>> > are in fact aligned, while the le_struct.h version convinces gcc that the >>>> > alignment of a pointer is '1', to make it issue the correct load/store >>>> > instructions depending on the architecture flags. >>>> > >>>> > On ARMv5 and older, we always use the second version, to let the compiler >>>> > use byte accesses. On ARMv6 and newer, we currently use the access_ok.h >>>> > version, so the compiler can use any instruction including stm/ldm and >>>> > ldrd/strd that will cause an alignment trap. This trap can significantly >>>> > impact performance when we have to do a lot of fixups and, worse, has >>>> > led to crashes in the LZ4 decompressor code that does not have a trap >>>> > handler. >>>> > >>>> > This adds an ARM specific version of asm/unaligned.h that uses the >>>> > le_struct.h/be_struct.h implementation unconditionally. This should lead >>>> > to essentially the same code on ARMv6+ as before, with the exception of >>>> > using regular load/store instructions instead of the trapping instructions >>>> > multi-register variants. >>>> > >>>> > The crash in the LZ4 decompressor code was probably introduced by the >>>> > patch replacing the LZ4 implementation, commit 4e1a33b105dd ("lib: update >>>> > LZ4 compressor module"), so linux-4.11 and higher would be affected most. >>>> > However, we probably want to have this backported to all older stable >>>> > kernels as well, to help with the performance issues. >>>> > >>>> > There are two follow-ups that I think we should also work on, but not >>>> > backport to stable kernels, first to change the asm-generic version of >>>> > the header to remove the ARM special case, and second to review all >>>> > other uses of CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS to see if they >>>> > might be affected by the same problem on ARM. >>>> > >>>> > Cc: stable@vger.kernel.org >>>> > Signed-off-by: Arnd Bergmann <arnd@arndb.de> >>>> > --- >>>> > Untested so far, please verify that this fixes all the known problems >>>> > with the alignment traps. >>>> >>>> I think Russell already find this conclusion but this patch didn't solve >>>> my boot issue with dtb append. >>>> >>>> I tested this patch onto a v4.14-rc6. >>>> >>>> Then at least with the patch from Ard: "efi/libstub: arm: omit sorting >>>> of the UEFI memory map", it didn't prevent booting. >>> >>> There's three things wrong, all of which I have patches to address: >>> >>> 1. The decompressor code reading the image data sometimes issues unaligned >>> reads. Some compilers get this wrong and cause an abort. Arnds patch >>> addresses this. >>> >>> 2. Additional sections can appear in the zImage binary which adds extra >>> bytes on the end of the image. Concatenating the zImage with the >>> extra bytes onto a DTB is the same thing as doing this: >>> >>> cat zImage extrabytes foo.dtb > image >>> >>> and the decompressor tolerates no additional bytes between the >>> _official_ end of the zImage and the DTB. I've added a patch which >>> detects this situation and fails the kernel build when it happens. >> >> So I tested the branch fixes in your git tree. >> >> After doing a "make multi_v7_defconfig; make zImage", I got the message >> "arm-linux-gnueabi-ld: error: zImage file size is incorrect" you added >> in the commit "ARM: verify size of zImage". >> >> It is the same with mvebu_v7_defconfig, so I wonder wich with >> configuration this patch was tested ? >> > > Could you please share the output of 'readelf -S' for those vmlinux > decompressor images? Here it is: In the meantime I also used an arm-linux-gnueabihf- in case it could be related to the toolchain, I had te same issue and here it is the readelf output: arm-linux-gnueabi-readelf -S ../build/vmlinux There are 40 section headers, starting at offset 0x7cc06bc: Section Headers: [Nr] Name Type Addr Off Size ES Flg Lk Inf Al [ 0] NULL 00000000 000000 000000 00 0 0 0 [ 1] .head.text PROGBITS c0008000 008000 00026c 00 AX 0 0 4 [ 2] .text PROGBITS c0100000 010000 6bd280 00 AX 0 0 64 [ 3] .fixup PROGBITS c07bd280 6cd280 00001c 00 AX 0 0 4 [ 4] .rodata PROGBITS c0800000 6d0000 168321 00 WA 0 0 64 [ 5] .pci_fixup PROGBITS c0968324 838324 001e40 00 A 0 0 4 [ 6] __ksymtab PROGBITS c096a164 83a164 007c28 00 A 0 0 4 [ 7] __ksymtab_gpl PROGBITS c0971d8c 841d8c 0081c8 00 A 0 0 4 [ 8] __ksymtab_strings PROGBITS c0979f54 849f54 0266e6 00 A 0 0 1 [ 9] __param PROGBITS c09a063c 87063c 00102c 00 A 0 0 4 [10] __modver PROGBITS c09a1668 871668 000998 00 A 0 0 4 [11] __ex_table PROGBITS c09a2000 872000 000ef0 00 A 0 0 8 [12] .ARM.unwind_idx ARM_EXIDX c09a2ef0 872ef0 02f348 00 AL 17 0 4 [13] .ARM.unwind_tab PROGBITS c09d2238 8a2238 001458 00 A 0 0 4 [14] .notes NOTE c09d3690 8a3690 000024 00 A 0 0 4 [15] .vectors PROGBITS ffff0000 8b0000 000020 00 AX 0 0 4 [16] .stubs PROGBITS ffff1000 8b1000 0002ac 00 AX 0 0 32 [17] .init.text PROGBITS c0a002e0 8c02e0 04ee58 00 AX 0 0 32 [18] .exit.text PROGBITS c0a4f138 90f138 0018cc 00 AX 0 0 4 [19] .init.arch.info PROGBITS c0a50a04 910a04 000270 00 A 0 0 4 [20] .init.tagtable PROGBITS c0a50c74 910c74 000040 00 A 0 0 4 [21] .init.smpalt PROGBITS c0a50cb4 910cb4 00d070 00 A 0 0 4 [22] .init.pv_table PROGBITS c0a5dd24 91dd24 0001ec 00 A 0 0 1 [23] .init.data PROGBITS c0a5e000 91e000 011e7c 00 WA 0 0 4096 [24] .data..percpu PROGBITS c0a70000 930000 008f0c 00 WA 0 0 64 [25] .data PROGBITS c0b00000 940000 07f5e8 00 WA 0 0 64 [26] .bss NOBITS c0b7f600 9bf5e8 02be6c 00 WA 0 0 64 [27] .comment PROGBITS 00000000 9bf5e8 00001c 01 MS 0 0 1 [28] .ARM.attributes ARM_ATTRIBUTES 00000000 9bf604 000031 00 0 0 1 [29] .debug_line PROGBITS 00000000 9bf635 706427 00 0 0 1 [30] .debug_info PROGBITS 00000000 10c5a5c 5c11e67 00 0 0 1 [31] .debug_abbrev PROGBITS 00000000 6cd78c3 2efd55 00 0 0 1 [32] .debug_aranges PROGBITS 00000000 6fc7618 011340 00 0 0 8 [33] .debug_str PROGBITS 00000000 6fd8958 24bc04 01 MS 0 0 1 [34] .debug_ranges PROGBITS 00000000 7224560 1f2bd0 00 0 0 8 [35] .debug_frame PROGBITS 00000000 7417130 14fa24 00 0 0 4 [36] .debug_loc PROGBITS 00000000 7566b54 4562b5 00 0 0 1 [37] .symtab SYMTAB 00000000 79bce0c 1b8f60 10 38 95872 4 [38] .strtab STRTAB 00000000 7b75d6c 14a7a2 00 0 0 1 [39] .shstrtab STRTAB 00000000 7cc050e 0001ab 00 0 0 1 Key to Flags: W (write), A (alloc), X (execute), M (merge), S (strings), I (info), L (link order), O (extra OS processing required), G (group), T (TLS), C (compressed), x (unknown), o (OS specific), E (exclude), y (purecode), p (processor specific) arm-linux-gnueabihf-readelf -S ../build/vmlinux There are 40 section headers, starting at offset 0x7cc853c: Section Headers: [Nr] Name Type Addr Off Size ES Flg Lk Inf Al [ 0] NULL 00000000 000000 000000 00 0 0 0 [ 1] .head.text PROGBITS c0008000 008000 00026c 00 AX 0 0 4 [ 2] .text PROGBITS c0100000 010000 6bd860 00 AX 0 0 64 [ 3] .fixup PROGBITS c07bd860 6cd860 00001c 00 AX 0 0 4 [ 4] .rodata PROGBITS c0800000 6d0000 168361 00 WA 0 0 64 [ 5] .pci_fixup PROGBITS c0968364 838364 001e40 00 A 0 0 4 [ 6] __ksymtab PROGBITS c096a1a4 83a1a4 007c28 00 A 0 0 4 [ 7] __ksymtab_gpl PROGBITS c0971dcc 841dcc 0081c8 00 A 0 0 4 [ 8] __ksymtab_strings PROGBITS c0979f94 849f94 0266e6 00 A 0 0 1 [ 9] __param PROGBITS c09a067c 87067c 00102c 00 A 0 0 4 [10] __modver PROGBITS c09a16a8 8716a8 000958 00 A 0 0 4 [11] __ex_table PROGBITS c09a2000 872000 000ef0 00 A 0 0 8 [12] .ARM.unwind_idx ARM_EXIDX c09a2ef0 872ef0 02f368 00 AL 17 0 4 [13] .ARM.unwind_tab PROGBITS c09d2258 8a2258 001458 00 A 0 0 4 [14] .notes NOTE c09d36b0 8a36b0 000024 00 A 0 0 4 [15] .vectors PROGBITS ffff0000 8b0000 000020 00 AX 0 0 4 [16] .stubs PROGBITS ffff1000 8b1000 0002ac 00 AX 0 0 32 [17] .init.text PROGBITS c0a002e0 8c02e0 04ee58 00 AX 0 0 32 [18] .exit.text PROGBITS c0a4f138 90f138 0018cc 00 AX 0 0 4 [19] .init.arch.info PROGBITS c0a50a04 910a04 000270 00 A 0 0 4 [20] .init.tagtable PROGBITS c0a50c74 910c74 000040 00 A 0 0 4 [21] .init.smpalt PROGBITS c0a50cb4 910cb4 00d070 00 A 0 0 4 [22] .init.pv_table PROGBITS c0a5dd24 91dd24 0001ec 00 A 0 0 1 [23] .init.data PROGBITS c0a5e000 91e000 011e7c 00 WA 0 0 4096 [24] .data..percpu PROGBITS c0a70000 930000 008f0c 00 WA 0 0 64 [25] .data PROGBITS c0b00000 940000 07f5e8 00 WA 0 0 64 [26] .bss NOBITS c0b7f600 9bf5e8 02be6c 00 WA 0 0 64 [27] .comment PROGBITS 00000000 9bf5e8 00001c 01 MS 0 0 1 [28] .ARM.attributes ARM_ATTRIBUTES 00000000 9bf604 000031 00 0 0 1 [29] .debug_line PROGBITS 00000000 9bf635 70533b 00 0 0 1 [30] .debug_info PROGBITS 00000000 10c4970 5c18a6b 00 0 0 1 [31] .debug_abbrev PROGBITS 00000000 6cdd3db 2eff72 00 0 0 1 [32] .debug_aranges PROGBITS 00000000 6fcd350 011340 00 0 0 8 [33] .debug_str PROGBITS 00000000 6fde690 24bc92 01 MS 0 0 1 [34] .debug_ranges PROGBITS 00000000 722a328 1f41c8 00 0 0 8 [35] .debug_frame PROGBITS 00000000 741e4f0 14fa78 00 0 0 4 [36] .debug_loc PROGBITS 00000000 756df68 456ca3 00 0 0 1 [37] .symtab SYMTAB 00000000 79c4c0c 1b8f90 10 38 95875 4 [38] .strtab STRTAB 00000000 7b7db9c 14a7f5 00 0 0 1 [39] .shstrtab STRTAB 00000000 7cc8391 0001ab 00 0 0 1 Key to Flags: W (write), A (alloc), X (execute), M (merge), S (strings), I (info), L (link order), O (extra OS processing required), G (group), T (TLS), C (compressed), x (unknown), o (OS specific), E (exclude), y (purecode), p (processor specific) Gregory
On 30 October 2017 at 15:05, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > Hi Ard, > > On lun., oct. 30 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > ... >> >> Could you please share the output of 'readelf -S' for those vmlinux >> decompressor images? > > Here it is: > > In the meantime I also used an arm-linux-gnueabihf- in case it could be > related to the toolchain, I had te same issue and here it is the readelf > output: > arm-linux-gnueabi-readelf -S ../build/vmlinux Actually, I meant the 'vmlinux' file in arch/arm/boot/compressed, not the main one. > There are 40 section headers, starting at offset 0x7cc06bc: > > Section Headers: > [Nr] Name Type Addr Off Size ES Flg Lk Inf Al > [ 0] NULL 00000000 000000 000000 00 0 0 0 > [ 1] .head.text PROGBITS c0008000 008000 00026c 00 AX 0 0 4 > [ 2] .text PROGBITS c0100000 010000 6bd280 00 AX 0 0 64 > [ 3] .fixup PROGBITS c07bd280 6cd280 00001c 00 AX 0 0 4 > [ 4] .rodata PROGBITS c0800000 6d0000 168321 00 WA 0 0 64 > [ 5] .pci_fixup PROGBITS c0968324 838324 001e40 00 A 0 0 4 > [ 6] __ksymtab PROGBITS c096a164 83a164 007c28 00 A 0 0 4 > [ 7] __ksymtab_gpl PROGBITS c0971d8c 841d8c 0081c8 00 A 0 0 4 > [ 8] __ksymtab_strings PROGBITS c0979f54 849f54 0266e6 00 A 0 0 1 > [ 9] __param PROGBITS c09a063c 87063c 00102c 00 A 0 0 4 > [10] __modver PROGBITS c09a1668 871668 000998 00 A 0 0 4 > [11] __ex_table PROGBITS c09a2000 872000 000ef0 00 A 0 0 8 > [12] .ARM.unwind_idx ARM_EXIDX c09a2ef0 872ef0 02f348 00 AL 17 0 4 > [13] .ARM.unwind_tab PROGBITS c09d2238 8a2238 001458 00 A 0 0 4 > [14] .notes NOTE c09d3690 8a3690 000024 00 A 0 0 4 > [15] .vectors PROGBITS ffff0000 8b0000 000020 00 AX 0 0 4 > [16] .stubs PROGBITS ffff1000 8b1000 0002ac 00 AX 0 0 32 > [17] .init.text PROGBITS c0a002e0 8c02e0 04ee58 00 AX 0 0 32 > [18] .exit.text PROGBITS c0a4f138 90f138 0018cc 00 AX 0 0 4 > [19] .init.arch.info PROGBITS c0a50a04 910a04 000270 00 A 0 0 4 > [20] .init.tagtable PROGBITS c0a50c74 910c74 000040 00 A 0 0 4 > [21] .init.smpalt PROGBITS c0a50cb4 910cb4 00d070 00 A 0 0 4 > [22] .init.pv_table PROGBITS c0a5dd24 91dd24 0001ec 00 A 0 0 1 > [23] .init.data PROGBITS c0a5e000 91e000 011e7c 00 WA 0 0 4096 > [24] .data..percpu PROGBITS c0a70000 930000 008f0c 00 WA 0 0 64 > [25] .data PROGBITS c0b00000 940000 07f5e8 00 WA 0 0 64 > [26] .bss NOBITS c0b7f600 9bf5e8 02be6c 00 WA 0 0 64 > [27] .comment PROGBITS 00000000 9bf5e8 00001c 01 MS 0 0 1 > [28] .ARM.attributes ARM_ATTRIBUTES 00000000 9bf604 000031 00 0 0 1 > [29] .debug_line PROGBITS 00000000 9bf635 706427 00 0 0 1 > [30] .debug_info PROGBITS 00000000 10c5a5c 5c11e67 00 0 0 1 > [31] .debug_abbrev PROGBITS 00000000 6cd78c3 2efd55 00 0 0 1 > [32] .debug_aranges PROGBITS 00000000 6fc7618 011340 00 0 0 8 > [33] .debug_str PROGBITS 00000000 6fd8958 24bc04 01 MS 0 0 1 > [34] .debug_ranges PROGBITS 00000000 7224560 1f2bd0 00 0 0 8 > [35] .debug_frame PROGBITS 00000000 7417130 14fa24 00 0 0 4 > [36] .debug_loc PROGBITS 00000000 7566b54 4562b5 00 0 0 1 > [37] .symtab SYMTAB 00000000 79bce0c 1b8f60 10 38 95872 4 > [38] .strtab STRTAB 00000000 7b75d6c 14a7a2 00 0 0 1 > [39] .shstrtab STRTAB 00000000 7cc050e 0001ab 00 0 0 1 > Key to Flags: > W (write), A (alloc), X (execute), M (merge), S (strings), I (info), > L (link order), O (extra OS processing required), G (group), T (TLS), > C (compressed), x (unknown), o (OS specific), E (exclude), > y (purecode), p (processor specific) > > > arm-linux-gnueabihf-readelf -S ../build/vmlinux > There are 40 section headers, starting at offset 0x7cc853c: > > Section Headers: > [Nr] Name Type Addr Off Size ES Flg Lk Inf Al > [ 0] NULL 00000000 000000 000000 00 0 0 0 > [ 1] .head.text PROGBITS c0008000 008000 00026c 00 AX 0 0 4 > [ 2] .text PROGBITS c0100000 010000 6bd860 00 AX 0 0 64 > [ 3] .fixup PROGBITS c07bd860 6cd860 00001c 00 AX 0 0 4 > [ 4] .rodata PROGBITS c0800000 6d0000 168361 00 WA 0 0 64 > [ 5] .pci_fixup PROGBITS c0968364 838364 001e40 00 A 0 0 4 > [ 6] __ksymtab PROGBITS c096a1a4 83a1a4 007c28 00 A 0 0 4 > [ 7] __ksymtab_gpl PROGBITS c0971dcc 841dcc 0081c8 00 A 0 0 4 > [ 8] __ksymtab_strings PROGBITS c0979f94 849f94 0266e6 00 A 0 0 1 > [ 9] __param PROGBITS c09a067c 87067c 00102c 00 A 0 0 4 > [10] __modver PROGBITS c09a16a8 8716a8 000958 00 A 0 0 4 > [11] __ex_table PROGBITS c09a2000 872000 000ef0 00 A 0 0 8 > [12] .ARM.unwind_idx ARM_EXIDX c09a2ef0 872ef0 02f368 00 AL 17 0 4 > [13] .ARM.unwind_tab PROGBITS c09d2258 8a2258 001458 00 A 0 0 4 > [14] .notes NOTE c09d36b0 8a36b0 000024 00 A 0 0 4 > [15] .vectors PROGBITS ffff0000 8b0000 000020 00 AX 0 0 4 > [16] .stubs PROGBITS ffff1000 8b1000 0002ac 00 AX 0 0 32 > [17] .init.text PROGBITS c0a002e0 8c02e0 04ee58 00 AX 0 0 32 > [18] .exit.text PROGBITS c0a4f138 90f138 0018cc 00 AX 0 0 4 > [19] .init.arch.info PROGBITS c0a50a04 910a04 000270 00 A 0 0 4 > [20] .init.tagtable PROGBITS c0a50c74 910c74 000040 00 A 0 0 4 > [21] .init.smpalt PROGBITS c0a50cb4 910cb4 00d070 00 A 0 0 4 > [22] .init.pv_table PROGBITS c0a5dd24 91dd24 0001ec 00 A 0 0 1 > [23] .init.data PROGBITS c0a5e000 91e000 011e7c 00 WA 0 0 4096 > [24] .data..percpu PROGBITS c0a70000 930000 008f0c 00 WA 0 0 64 > [25] .data PROGBITS c0b00000 940000 07f5e8 00 WA 0 0 64 > [26] .bss NOBITS c0b7f600 9bf5e8 02be6c 00 WA 0 0 64 > [27] .comment PROGBITS 00000000 9bf5e8 00001c 01 MS 0 0 1 > [28] .ARM.attributes ARM_ATTRIBUTES 00000000 9bf604 000031 00 0 0 1 > [29] .debug_line PROGBITS 00000000 9bf635 70533b 00 0 0 1 > [30] .debug_info PROGBITS 00000000 10c4970 5c18a6b 00 0 0 1 > [31] .debug_abbrev PROGBITS 00000000 6cdd3db 2eff72 00 0 0 1 > [32] .debug_aranges PROGBITS 00000000 6fcd350 011340 00 0 0 8 > [33] .debug_str PROGBITS 00000000 6fde690 24bc92 01 MS 0 0 1 > [34] .debug_ranges PROGBITS 00000000 722a328 1f41c8 00 0 0 8 > [35] .debug_frame PROGBITS 00000000 741e4f0 14fa78 00 0 0 4 > [36] .debug_loc PROGBITS 00000000 756df68 456ca3 00 0 0 1 > [37] .symtab SYMTAB 00000000 79c4c0c 1b8f90 10 38 95875 4 > [38] .strtab STRTAB 00000000 7b7db9c 14a7f5 00 0 0 1 > [39] .shstrtab STRTAB 00000000 7cc8391 0001ab 00 0 0 1 > Key to Flags: > W (write), A (alloc), X (execute), M (merge), S (strings), I (info), > L (link order), O (extra OS processing required), G (group), T (TLS), > C (compressed), x (unknown), o (OS specific), E (exclude), > y (purecode), p (processor specific) > > Gregory > > -- > Gregory Clement, Free Electrons > Kernel, drivers, real-time and embedded Linux > development, consulting, training and support. > http://free-electrons.com
Hi Ard, On lun., oct. 30 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 30 October 2017 at 15:05, Gregory CLEMENT > <gregory.clement@free-electrons.com> wrote: >> Hi Ard, >> >> On lun., oct. 30 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> > ... >>> >>> Could you please share the output of 'readelf -S' for those vmlinux >>> decompressor images? >> >> Here it is: >> >> In the meantime I also used an arm-linux-gnueabihf- in case it could be >> related to the toolchain, I had te same issue and here it is the readelf >> output: >> arm-linux-gnueabi-readelf -S ../build/vmlinux > > Actually, I meant the 'vmlinux' file in arch/arm/boot/compressed, not > the main one. Here it is then: arm-linux-gnueabi-readelf -S ../build/arch/arm/boot/compressed/vmlinux There are 22 section headers, starting at offset 0x4b3e14: Section Headers: [Nr] Name Type Addr Off Size ES Flg Lk Inf Al [ 0] NULL 00000000 000000 000000 00 0 0 0 [ 1] .text PROGBITS 00000000 010000 005ef8 00 AX 0 0 32 [ 2] .rodata PROGBITS 00005ef8 015ef8 000d4c 00 A 0 0 4 [ 3] .piggydata PROGBITS 00006c44 016c44 48a33a 00 A 0 0 1 [ 4] .got.plt PROGBITS 00490f80 4a0f80 00000c 04 WA 0 0 4 [ 5] .got PROGBITS 00490f8c 4a0f8c 000028 00 WA 0 0 4 [ 6] .pad PROGBITS 00490fb4 4a0fb4 000004 00 WA 0 0 1 [ 7] .bss NOBITS 00490fb8 4a0fb8 00001c 00 WA 0 0 4 [ 8] .stack NOBITS 00490fd8 4a0fb8 001000 00 WA 0 0 1 [ 9] .comment PROGBITS 00000000 4a0fb8 00001c 01 MS 0 0 1 [10] .ARM.attributes ARM_ATTRIBUTES 00000000 4a0fd4 00002d 00 0 0 1 [11] .debug_line PROGBITS 00000000 4a1001 00281b 00 0 0 1 [12] .debug_info PROGBITS 00000000 4a381c 0066cb 00 0 0 1 [13] .debug_abbrev PROGBITS 00000000 4a9ee7 0013ea 00 0 0 1 [14] .debug_aranges PROGBITS 00000000 4ab2d8 0001a8 00 0 0 8 [15] .debug_str PROGBITS 00000000 4ab480 0019b4 01 MS 0 0 1 [16] .debug_ranges PROGBITS 00000000 4ace38 000640 00 0 0 8 [17] .debug_frame PROGBITS 00000000 4ad478 001010 00 0 0 4 [18] .debug_loc PROGBITS 00000000 4ae488 003643 00 0 0 1 [19] .symtab SYMTAB 00000000 4b1acc 0015b0 10 20 225 4 [20] .strtab STRTAB 00000000 4b307c 000cc5 00 0 0 1 [21] .shstrtab STRTAB 00000000 4b3d41 0000d2 00 0 0 1 Key to Flags: W (write), A (alloc), X (execute), M (merge), S (strings), I (info), L (link order), O (extra OS processing required), G (group), T (TLS), C (compressed), x (unknown), o (OS specific), E (exclude), y (purecode), p (processor specific) Gregory > > > >> There are 40 section headers, starting at offset 0x7cc06bc: >> >> Section Headers: >> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >> [ 0] NULL 00000000 000000 000000 00 0 0 0 >> [ 1] .head.text PROGBITS c0008000 008000 00026c 00 AX 0 0 4 >> [ 2] .text PROGBITS c0100000 010000 6bd280 00 AX 0 0 64 >> [ 3] .fixup PROGBITS c07bd280 6cd280 00001c 00 AX 0 0 4 >> [ 4] .rodata PROGBITS c0800000 6d0000 168321 00 WA 0 0 64 >> [ 5] .pci_fixup PROGBITS c0968324 838324 001e40 00 A 0 0 4 >> [ 6] __ksymtab PROGBITS c096a164 83a164 007c28 00 A 0 0 4 >> [ 7] __ksymtab_gpl PROGBITS c0971d8c 841d8c 0081c8 00 A 0 0 4 >> [ 8] __ksymtab_strings PROGBITS c0979f54 849f54 0266e6 00 A 0 0 1 >> [ 9] __param PROGBITS c09a063c 87063c 00102c 00 A 0 0 4 >> [10] __modver PROGBITS c09a1668 871668 000998 00 A 0 0 4 >> [11] __ex_table PROGBITS c09a2000 872000 000ef0 00 A 0 0 8 >> [12] .ARM.unwind_idx ARM_EXIDX c09a2ef0 872ef0 02f348 00 AL 17 0 4 >> [13] .ARM.unwind_tab PROGBITS c09d2238 8a2238 001458 00 A 0 0 4 >> [14] .notes NOTE c09d3690 8a3690 000024 00 A 0 0 4 >> [15] .vectors PROGBITS ffff0000 8b0000 000020 00 AX 0 0 4 >> [16] .stubs PROGBITS ffff1000 8b1000 0002ac 00 AX 0 0 32 >> [17] .init.text PROGBITS c0a002e0 8c02e0 04ee58 00 AX 0 0 32 >> [18] .exit.text PROGBITS c0a4f138 90f138 0018cc 00 AX 0 0 4 >> [19] .init.arch.info PROGBITS c0a50a04 910a04 000270 00 A 0 0 4 >> [20] .init.tagtable PROGBITS c0a50c74 910c74 000040 00 A 0 0 4 >> [21] .init.smpalt PROGBITS c0a50cb4 910cb4 00d070 00 A 0 0 4 >> [22] .init.pv_table PROGBITS c0a5dd24 91dd24 0001ec 00 A 0 0 1 >> [23] .init.data PROGBITS c0a5e000 91e000 011e7c 00 WA 0 0 4096 >> [24] .data..percpu PROGBITS c0a70000 930000 008f0c 00 WA 0 0 64 >> [25] .data PROGBITS c0b00000 940000 07f5e8 00 WA 0 0 64 >> [26] .bss NOBITS c0b7f600 9bf5e8 02be6c 00 WA 0 0 64 >> [27] .comment PROGBITS 00000000 9bf5e8 00001c 01 MS 0 0 1 >> [28] .ARM.attributes ARM_ATTRIBUTES 00000000 9bf604 000031 00 0 0 1 >> [29] .debug_line PROGBITS 00000000 9bf635 706427 00 0 0 1 >> [30] .debug_info PROGBITS 00000000 10c5a5c 5c11e67 00 0 0 1 >> [31] .debug_abbrev PROGBITS 00000000 6cd78c3 2efd55 00 0 0 1 >> [32] .debug_aranges PROGBITS 00000000 6fc7618 011340 00 0 0 8 >> [33] .debug_str PROGBITS 00000000 6fd8958 24bc04 01 MS 0 0 1 >> [34] .debug_ranges PROGBITS 00000000 7224560 1f2bd0 00 0 0 8 >> [35] .debug_frame PROGBITS 00000000 7417130 14fa24 00 0 0 4 >> [36] .debug_loc PROGBITS 00000000 7566b54 4562b5 00 0 0 1 >> [37] .symtab SYMTAB 00000000 79bce0c 1b8f60 10 38 95872 4 >> [38] .strtab STRTAB 00000000 7b75d6c 14a7a2 00 0 0 1 >> [39] .shstrtab STRTAB 00000000 7cc050e 0001ab 00 0 0 1 >> Key to Flags: >> W (write), A (alloc), X (execute), M (merge), S (strings), I (info), >> L (link order), O (extra OS processing required), G (group), T (TLS), >> C (compressed), x (unknown), o (OS specific), E (exclude), >> y (purecode), p (processor specific) >> >> >> arm-linux-gnueabihf-readelf -S ../build/vmlinux >> There are 40 section headers, starting at offset 0x7cc853c: >> >> Section Headers: >> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >> [ 0] NULL 00000000 000000 000000 00 0 0 0 >> [ 1] .head.text PROGBITS c0008000 008000 00026c 00 AX 0 0 4 >> [ 2] .text PROGBITS c0100000 010000 6bd860 00 AX 0 0 64 >> [ 3] .fixup PROGBITS c07bd860 6cd860 00001c 00 AX 0 0 4 >> [ 4] .rodata PROGBITS c0800000 6d0000 168361 00 WA 0 0 64 >> [ 5] .pci_fixup PROGBITS c0968364 838364 001e40 00 A 0 0 4 >> [ 6] __ksymtab PROGBITS c096a1a4 83a1a4 007c28 00 A 0 0 4 >> [ 7] __ksymtab_gpl PROGBITS c0971dcc 841dcc 0081c8 00 A 0 0 4 >> [ 8] __ksymtab_strings PROGBITS c0979f94 849f94 0266e6 00 A 0 0 1 >> [ 9] __param PROGBITS c09a067c 87067c 00102c 00 A 0 0 4 >> [10] __modver PROGBITS c09a16a8 8716a8 000958 00 A 0 0 4 >> [11] __ex_table PROGBITS c09a2000 872000 000ef0 00 A 0 0 8 >> [12] .ARM.unwind_idx ARM_EXIDX c09a2ef0 872ef0 02f368 00 AL 17 0 4 >> [13] .ARM.unwind_tab PROGBITS c09d2258 8a2258 001458 00 A 0 0 4 >> [14] .notes NOTE c09d36b0 8a36b0 000024 00 A 0 0 4 >> [15] .vectors PROGBITS ffff0000 8b0000 000020 00 AX 0 0 4 >> [16] .stubs PROGBITS ffff1000 8b1000 0002ac 00 AX 0 0 32 >> [17] .init.text PROGBITS c0a002e0 8c02e0 04ee58 00 AX 0 0 32 >> [18] .exit.text PROGBITS c0a4f138 90f138 0018cc 00 AX 0 0 4 >> [19] .init.arch.info PROGBITS c0a50a04 910a04 000270 00 A 0 0 4 >> [20] .init.tagtable PROGBITS c0a50c74 910c74 000040 00 A 0 0 4 >> [21] .init.smpalt PROGBITS c0a50cb4 910cb4 00d070 00 A 0 0 4 >> [22] .init.pv_table PROGBITS c0a5dd24 91dd24 0001ec 00 A 0 0 1 >> [23] .init.data PROGBITS c0a5e000 91e000 011e7c 00 WA 0 0 4096 >> [24] .data..percpu PROGBITS c0a70000 930000 008f0c 00 WA 0 0 64 >> [25] .data PROGBITS c0b00000 940000 07f5e8 00 WA 0 0 64 >> [26] .bss NOBITS c0b7f600 9bf5e8 02be6c 00 WA 0 0 64 >> [27] .comment PROGBITS 00000000 9bf5e8 00001c 01 MS 0 0 1 >> [28] .ARM.attributes ARM_ATTRIBUTES 00000000 9bf604 000031 00 0 0 1 >> [29] .debug_line PROGBITS 00000000 9bf635 70533b 00 0 0 1 >> [30] .debug_info PROGBITS 00000000 10c4970 5c18a6b 00 0 0 1 >> [31] .debug_abbrev PROGBITS 00000000 6cdd3db 2eff72 00 0 0 1 >> [32] .debug_aranges PROGBITS 00000000 6fcd350 011340 00 0 0 8 >> [33] .debug_str PROGBITS 00000000 6fde690 24bc92 01 MS 0 0 1 >> [34] .debug_ranges PROGBITS 00000000 722a328 1f41c8 00 0 0 8 >> [35] .debug_frame PROGBITS 00000000 741e4f0 14fa78 00 0 0 4 >> [36] .debug_loc PROGBITS 00000000 756df68 456ca3 00 0 0 1 >> [37] .symtab SYMTAB 00000000 79c4c0c 1b8f90 10 38 95875 4 >> [38] .strtab STRTAB 00000000 7b7db9c 14a7f5 00 0 0 1 >> [39] .shstrtab STRTAB 00000000 7cc8391 0001ab 00 0 0 1 >> Key to Flags: >> W (write), A (alloc), X (execute), M (merge), S (strings), I (info), >> L (link order), O (extra OS processing required), G (group), T (TLS), >> C (compressed), x (unknown), o (OS specific), E (exclude), >> y (purecode), p (processor specific) >> >> Gregory >> >> -- >> Gregory Clement, Free Electrons >> Kernel, drivers, real-time and embedded Linux >> development, consulting, training and support. >> http://free-electrons.com > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Oct 27, 2017 at 5:27 PM, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > > There's three things wrong, all of which I have patches to address: > > 1. The decompressor code reading the image data sometimes issues unaligned > reads. Some compilers get this wrong and cause an abort. Arnds patch > addresses this. > > 2. Additional sections can appear in the zImage binary which adds extra > bytes on the end of the image. Concatenating the zImage with the > extra bytes onto a DTB is the same thing as doing this: > > cat zImage extrabytes foo.dtb > image > > and the decompressor tolerates no additional bytes between the > _official_ end of the zImage and the DTB. I've added a patch which > detects this situation and fails the kernel build when it happens. > > 3. Ard's patch "efi/libstub: arm: omit sorting of the UEFI memory map" > gets rid of the additional sections that (a) change the alignment > of the compressed data, and (b) add additional unexpected bytes on > the end of zImage. It's possible that we still need yet another patch to address the gcc bug that Alex Graf found, i.e. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82445 Without the latest gcc, we might still get into a situation in which we get an unaligned strd when compiling for armv5te or armv6 with gcc-7.[012]. As someone mentioned in the bug report, that problem doesn't seem to happen with gcc-6 or -mtune=xscale, or with gcc-7 -march=armv7-a. Arnd
On 30 October 2017 at 15:09, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > Hi Ard, > > On lun., oct. 30 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >> On 30 October 2017 at 15:05, Gregory CLEMENT >> <gregory.clement@free-electrons.com> wrote: >>> Hi Ard, >>> >>> On lun., oct. 30 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >>> >> ... >>>> >>>> Could you please share the output of 'readelf -S' for those vmlinux >>>> decompressor images? >>> >>> Here it is: >>> >>> In the meantime I also used an arm-linux-gnueabihf- in case it could be >>> related to the toolchain, I had te same issue and here it is the readelf >>> output: >>> arm-linux-gnueabi-readelf -S ../build/vmlinux >> >> Actually, I meant the 'vmlinux' file in arch/arm/boot/compressed, not >> the main one. > > Here it is then: > > arm-linux-gnueabi-readelf -S ../build/arch/arm/boot/compressed/vmlinux > There are 22 section headers, starting at offset 0x4b3e14: > > Section Headers: > [Nr] Name Type Addr Off Size ES Flg Lk Inf Al > [ 0] NULL 00000000 000000 000000 00 0 0 0 > [ 1] .text PROGBITS 00000000 010000 005ef8 00 AX 0 0 32 > [ 2] .rodata PROGBITS 00005ef8 015ef8 000d4c 00 A 0 0 4 > [ 3] .piggydata PROGBITS 00006c44 016c44 48a33a 00 A 0 0 1 > [ 4] .got.plt PROGBITS 00490f80 4a0f80 00000c 04 WA 0 0 4 > [ 5] .got PROGBITS 00490f8c 4a0f8c 000028 00 WA 0 0 4 > [ 6] .pad PROGBITS 00490fb4 4a0fb4 000004 00 WA 0 0 1 > [ 7] .bss NOBITS 00490fb8 4a0fb8 00001c 00 WA 0 0 4 > [ 8] .stack NOBITS 00490fd8 4a0fb8 001000 00 WA 0 0 1 > [ 9] .comment PROGBITS 00000000 4a0fb8 00001c 01 MS 0 0 1 > [10] .ARM.attributes ARM_ATTRIBUTES 00000000 4a0fd4 00002d 00 0 0 1 > [11] .debug_line PROGBITS 00000000 4a1001 00281b 00 0 0 1 > [12] .debug_info PROGBITS 00000000 4a381c 0066cb 00 0 0 1 > [13] .debug_abbrev PROGBITS 00000000 4a9ee7 0013ea 00 0 0 1 > [14] .debug_aranges PROGBITS 00000000 4ab2d8 0001a8 00 0 0 8 > [15] .debug_str PROGBITS 00000000 4ab480 0019b4 01 MS 0 0 1 > [16] .debug_ranges PROGBITS 00000000 4ace38 000640 00 0 0 8 > [17] .debug_frame PROGBITS 00000000 4ad478 001010 00 0 0 4 > [18] .debug_loc PROGBITS 00000000 4ae488 003643 00 0 0 1 > [19] .symtab SYMTAB 00000000 4b1acc 0015b0 10 20 225 4 > [20] .strtab STRTAB 00000000 4b307c 000cc5 00 0 0 1 > [21] .shstrtab STRTAB 00000000 4b3d41 0000d2 00 0 0 1 > Key to Flags: > W (write), A (alloc), X (execute), M (merge), S (strings), I (info), > L (link order), O (extra OS processing required), G (group), T (TLS), > C (compressed), x (unknown), o (OS specific), E (exclude), > y (purecode), p (processor specific) > And this is from the build the build that generated the linker assert? Could you check the value of _edata in the output of 'nm vmlinux' please?
Hi Ard, On lun., oct. 30 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 30 October 2017 at 15:09, Gregory CLEMENT > <gregory.clement@free-electrons.com> wrote: >> Hi Ard, >> >> On lun., oct. 30 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> >>> On 30 October 2017 at 15:05, Gregory CLEMENT >>> <gregory.clement@free-electrons.com> wrote: >>>> Hi Ard, >>>> >>>> On lun., oct. 30 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >>>> >>> ... >>>>> >>>>> Could you please share the output of 'readelf -S' for those vmlinux >>>>> decompressor images? >>>> >>>> Here it is: >>>> >>>> In the meantime I also used an arm-linux-gnueabihf- in case it could be >>>> related to the toolchain, I had te same issue and here it is the readelf >>>> output: >>>> arm-linux-gnueabi-readelf -S ../build/vmlinux >>> >>> Actually, I meant the 'vmlinux' file in arch/arm/boot/compressed, not >>> the main one. >> >> Here it is then: >> >> arm-linux-gnueabi-readelf -S ../build/arch/arm/boot/compressed/vmlinux >> There are 22 section headers, starting at offset 0x4b3e14: >> >> Section Headers: >> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >> [ 0] NULL 00000000 000000 000000 00 0 0 0 >> [ 1] .text PROGBITS 00000000 010000 005ef8 00 AX 0 0 32 >> [ 2] .rodata PROGBITS 00005ef8 015ef8 000d4c 00 A 0 0 4 >> [ 3] .piggydata PROGBITS 00006c44 016c44 48a33a 00 A 0 0 1 >> [ 4] .got.plt PROGBITS 00490f80 4a0f80 00000c 04 WA 0 0 4 >> [ 5] .got PROGBITS 00490f8c 4a0f8c 000028 00 WA 0 0 4 >> [ 6] .pad PROGBITS 00490fb4 4a0fb4 000004 00 WA 0 0 1 >> [ 7] .bss NOBITS 00490fb8 4a0fb8 00001c 00 WA 0 0 4 >> [ 8] .stack NOBITS 00490fd8 4a0fb8 001000 00 WA 0 0 1 >> [ 9] .comment PROGBITS 00000000 4a0fb8 00001c 01 MS 0 0 1 >> [10] .ARM.attributes ARM_ATTRIBUTES 00000000 4a0fd4 00002d 00 0 0 1 >> [11] .debug_line PROGBITS 00000000 4a1001 00281b 00 0 0 1 >> [12] .debug_info PROGBITS 00000000 4a381c 0066cb 00 0 0 1 >> [13] .debug_abbrev PROGBITS 00000000 4a9ee7 0013ea 00 0 0 1 >> [14] .debug_aranges PROGBITS 00000000 4ab2d8 0001a8 00 0 0 8 >> [15] .debug_str PROGBITS 00000000 4ab480 0019b4 01 MS 0 0 1 >> [16] .debug_ranges PROGBITS 00000000 4ace38 000640 00 0 0 8 >> [17] .debug_frame PROGBITS 00000000 4ad478 001010 00 0 0 4 >> [18] .debug_loc PROGBITS 00000000 4ae488 003643 00 0 0 1 >> [19] .symtab SYMTAB 00000000 4b1acc 0015b0 10 20 225 4 >> [20] .strtab STRTAB 00000000 4b307c 000cc5 00 0 0 1 >> [21] .shstrtab STRTAB 00000000 4b3d41 0000d2 00 0 0 1 >> Key to Flags: >> W (write), A (alloc), X (execute), M (merge), S (strings), I (info), >> L (link order), O (extra OS processing required), G (group), T (TLS), >> C (compressed), x (unknown), o (OS specific), E (exclude), >> y (purecode), p (processor specific) >> > > And this is from the build the build that generated the linker assert? Humm no, actually it was with the wrong branch. If I have the patch "ARM: verify size of zImage" then arch/arm/boot/compressed/vmlinux is not generated. But if I remove this patch then I can generate this file and so: arm-linux-gnueabi-readelf -S ../build/arch/arm/boot/compressed/vmlinux There are 22 section headers, starting at offset 0x4b402c: Section Headers: [Nr] Name Type Addr Off Size ES Flg Lk Inf Al [ 0] NULL 00000000 000000 000000 00 0 0 0 [ 1] .text PROGBITS 00000000 010000 005ef8 00 AX 0 0 32 [ 2] .rodata PROGBITS 00005ef8 015ef8 000d4c 00 A 0 0 4 [ 3] .piggydata PROGBITS 00006c44 016c44 48a552 00 A 0 0 1 [ 4] .got.plt PROGBITS 00491198 4a1198 00000c 04 WA 0 0 4 [ 5] .got PROGBITS 004911a4 4a11a4 000028 00 WA 0 0 4 [ 6] .pad PROGBITS 004911cc 4a11cc 000004 00 WA 0 0 1 [ 7] .bss NOBITS 004911d0 4a11d0 00001c 00 WA 0 0 4 [ 8] .stack NOBITS 004911f0 4a11d0 001000 00 WA 0 0 1 [ 9] .comment PROGBITS 00000000 4a11d0 00001c 01 MS 0 0 1 [10] .ARM.attributes ARM_ATTRIBUTES 00000000 4a11ec 00002d 00 0 0 1 [11] .debug_line PROGBITS 00000000 4a1219 00281b 00 0 0 1 [12] .debug_info PROGBITS 00000000 4a3a34 0066cb 00 0 0 1 [13] .debug_abbrev PROGBITS 00000000 4aa0ff 0013ea 00 0 0 1 [14] .debug_aranges PROGBITS 00000000 4ab4f0 0001a8 00 0 0 8 [15] .debug_str PROGBITS 00000000 4ab698 0019b4 01 MS 0 0 1 [16] .debug_ranges PROGBITS 00000000 4ad050 000640 00 0 0 8 [17] .debug_frame PROGBITS 00000000 4ad690 001010 00 0 0 4 [18] .debug_loc PROGBITS 00000000 4ae6a0 003643 00 0 0 1 [19] .symtab SYMTAB 00000000 4b1ce4 0015b0 10 20 225 4 [20] .strtab STRTAB 00000000 4b3294 000cc5 00 0 0 1 [21] .shstrtab STRTAB 00000000 4b3f59 0000d2 00 0 0 1 > > Could you check the value of _edata in the output of 'nm vmlinux' > please? With this kernel: arm-linux-gnueabi-nm ../build/arch/arm/boot/compressed/vmlinux | grep _edata 004911d0 D _edata Gregory > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 30 October 2017 at 15:33, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > Hi Ard, > > On lun., oct. 30 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >> On 30 October 2017 at 15:09, Gregory CLEMENT >> <gregory.clement@free-electrons.com> wrote: >>> Hi Ard, >>> >>> On lun., oct. 30 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >>> >>>> On 30 October 2017 at 15:05, Gregory CLEMENT >>>> <gregory.clement@free-electrons.com> wrote: >>>>> Hi Ard, >>>>> >>>>> On lun., oct. 30 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >>>>> >>>> ... >>>>>> >>>>>> Could you please share the output of 'readelf -S' for those vmlinux >>>>>> decompressor images? >>>>> >>>>> Here it is: >>>>> >>>>> In the meantime I also used an arm-linux-gnueabihf- in case it could be >>>>> related to the toolchain, I had te same issue and here it is the readelf >>>>> output: >>>>> arm-linux-gnueabi-readelf -S ../build/vmlinux >>>> >>>> Actually, I meant the 'vmlinux' file in arch/arm/boot/compressed, not >>>> the main one. >>> >>> Here it is then: >>> >>> arm-linux-gnueabi-readelf -S ../build/arch/arm/boot/compressed/vmlinux >>> There are 22 section headers, starting at offset 0x4b3e14: >>> >>> Section Headers: >>> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >>> [ 0] NULL 00000000 000000 000000 00 0 0 0 >>> [ 1] .text PROGBITS 00000000 010000 005ef8 00 AX 0 0 32 >>> [ 2] .rodata PROGBITS 00005ef8 015ef8 000d4c 00 A 0 0 4 >>> [ 3] .piggydata PROGBITS 00006c44 016c44 48a33a 00 A 0 0 1 >>> [ 4] .got.plt PROGBITS 00490f80 4a0f80 00000c 04 WA 0 0 4 >>> [ 5] .got PROGBITS 00490f8c 4a0f8c 000028 00 WA 0 0 4 >>> [ 6] .pad PROGBITS 00490fb4 4a0fb4 000004 00 WA 0 0 1 >>> [ 7] .bss NOBITS 00490fb8 4a0fb8 00001c 00 WA 0 0 4 >>> [ 8] .stack NOBITS 00490fd8 4a0fb8 001000 00 WA 0 0 1 >>> [ 9] .comment PROGBITS 00000000 4a0fb8 00001c 01 MS 0 0 1 >>> [10] .ARM.attributes ARM_ATTRIBUTES 00000000 4a0fd4 00002d 00 0 0 1 >>> [11] .debug_line PROGBITS 00000000 4a1001 00281b 00 0 0 1 >>> [12] .debug_info PROGBITS 00000000 4a381c 0066cb 00 0 0 1 >>> [13] .debug_abbrev PROGBITS 00000000 4a9ee7 0013ea 00 0 0 1 >>> [14] .debug_aranges PROGBITS 00000000 4ab2d8 0001a8 00 0 0 8 >>> [15] .debug_str PROGBITS 00000000 4ab480 0019b4 01 MS 0 0 1 >>> [16] .debug_ranges PROGBITS 00000000 4ace38 000640 00 0 0 8 >>> [17] .debug_frame PROGBITS 00000000 4ad478 001010 00 0 0 4 >>> [18] .debug_loc PROGBITS 00000000 4ae488 003643 00 0 0 1 >>> [19] .symtab SYMTAB 00000000 4b1acc 0015b0 10 20 225 4 >>> [20] .strtab STRTAB 00000000 4b307c 000cc5 00 0 0 1 >>> [21] .shstrtab STRTAB 00000000 4b3d41 0000d2 00 0 0 1 >>> Key to Flags: >>> W (write), A (alloc), X (execute), M (merge), S (strings), I (info), >>> L (link order), O (extra OS processing required), G (group), T (TLS), >>> C (compressed), x (unknown), o (OS specific), E (exclude), >>> y (purecode), p (processor specific) >>> >> >> And this is from the build the build that generated the linker assert? > > Humm no, actually it was with the wrong branch. If I have the patch > "ARM: verify size of zImage" then arch/arm/boot/compressed/vmlinux is > not generated. > > But if I remove this patch then I can generate this file and so: > arm-linux-gnueabi-readelf -S ../build/arch/arm/boot/compressed/vmlinux > There are 22 section headers, starting at offset 0x4b402c: > > Section Headers: > [Nr] Name Type Addr Off Size ES Flg Lk Inf Al > [ 0] NULL 00000000 000000 000000 00 0 0 0 > [ 1] .text PROGBITS 00000000 010000 005ef8 00 AX 0 0 32 > [ 2] .rodata PROGBITS 00005ef8 015ef8 000d4c 00 A 0 0 4 > [ 3] .piggydata PROGBITS 00006c44 016c44 48a552 00 A 0 0 1 > [ 4] .got.plt PROGBITS 00491198 4a1198 00000c 04 WA 0 0 4 > [ 5] .got PROGBITS 004911a4 4a11a4 000028 00 WA 0 0 4 > [ 6] .pad PROGBITS 004911cc 4a11cc 000004 00 WA 0 0 1 > [ 7] .bss NOBITS 004911d0 4a11d0 00001c 00 WA 0 0 4 > [ 8] .stack NOBITS 004911f0 4a11d0 001000 00 WA 0 0 1 > [ 9] .comment PROGBITS 00000000 4a11d0 00001c 01 MS 0 0 1 > [10] .ARM.attributes ARM_ATTRIBUTES 00000000 4a11ec 00002d 00 0 0 1 > [11] .debug_line PROGBITS 00000000 4a1219 00281b 00 0 0 1 > [12] .debug_info PROGBITS 00000000 4a3a34 0066cb 00 0 0 1 > [13] .debug_abbrev PROGBITS 00000000 4aa0ff 0013ea 00 0 0 1 > [14] .debug_aranges PROGBITS 00000000 4ab4f0 0001a8 00 0 0 8 > [15] .debug_str PROGBITS 00000000 4ab698 0019b4 01 MS 0 0 1 > [16] .debug_ranges PROGBITS 00000000 4ad050 000640 00 0 0 8 > [17] .debug_frame PROGBITS 00000000 4ad690 001010 00 0 0 4 > [18] .debug_loc PROGBITS 00000000 4ae6a0 003643 00 0 0 1 > [19] .symtab SYMTAB 00000000 4b1ce4 0015b0 10 20 225 4 > [20] .strtab STRTAB 00000000 4b3294 000cc5 00 0 0 1 > [21] .shstrtab STRTAB 00000000 4b3f59 0000d2 00 0 0 1 > >> >> Could you check the value of _edata in the output of 'nm vmlinux' >> please? > With this kernel: > > arm-linux-gnueabi-nm ../build/arch/arm/boot/compressed/vmlinux | grep _edata > 004911d0 D _edata > Well, that is disappointing. This means the ASSERT() does not work reliably, and we're back to using a bunch of shell scripts to check whether _edata appears at the end of the image. Which version of binutils are you using?
Hi Ard, On lun., oct. 30 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 30 October 2017 at 15:33, Gregory CLEMENT > <gregory.clement@free-electrons.com> wrote: >> Hi Ard, >> >> On lun., oct. 30 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> >>> On 30 October 2017 at 15:09, Gregory CLEMENT >>> <gregory.clement@free-electrons.com> wrote: >>>> Hi Ard, >>>> >>>> On lun., oct. 30 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >>>> >>>>> On 30 October 2017 at 15:05, Gregory CLEMENT >>>>> <gregory.clement@free-electrons.com> wrote: >>>>>> Hi Ard, >>>>>> >>>>>> On lun., oct. 30 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >>>>>> >>>>> ... >>>>>>> >>>>>>> Could you please share the output of 'readelf -S' for those vmlinux >>>>>>> decompressor images? >>>>>> >>>>>> Here it is: >>>>>> >>>>>> In the meantime I also used an arm-linux-gnueabihf- in case it could be >>>>>> related to the toolchain, I had te same issue and here it is the readelf >>>>>> output: >>>>>> arm-linux-gnueabi-readelf -S ../build/vmlinux >>>>> >>>>> Actually, I meant the 'vmlinux' file in arch/arm/boot/compressed, not >>>>> the main one. >>>> >>>> Here it is then: >>>> >>>> arm-linux-gnueabi-readelf -S ../build/arch/arm/boot/compressed/vmlinux >>>> There are 22 section headers, starting at offset 0x4b3e14: >>>> >>>> Section Headers: >>>> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >>>> [ 0] NULL 00000000 000000 000000 00 0 0 0 >>>> [ 1] .text PROGBITS 00000000 010000 005ef8 00 AX 0 0 32 >>>> [ 2] .rodata PROGBITS 00005ef8 015ef8 000d4c 00 A 0 0 4 >>>> [ 3] .piggydata PROGBITS 00006c44 016c44 48a33a 00 A 0 0 1 >>>> [ 4] .got.plt PROGBITS 00490f80 4a0f80 00000c 04 WA 0 0 4 >>>> [ 5] .got PROGBITS 00490f8c 4a0f8c 000028 00 WA 0 0 4 >>>> [ 6] .pad PROGBITS 00490fb4 4a0fb4 000004 00 WA 0 0 1 >>>> [ 7] .bss NOBITS 00490fb8 4a0fb8 00001c 00 WA 0 0 4 >>>> [ 8] .stack NOBITS 00490fd8 4a0fb8 001000 00 WA 0 0 1 >>>> [ 9] .comment PROGBITS 00000000 4a0fb8 00001c 01 MS 0 0 1 >>>> [10] .ARM.attributes ARM_ATTRIBUTES 00000000 4a0fd4 00002d 00 0 0 1 >>>> [11] .debug_line PROGBITS 00000000 4a1001 00281b 00 0 0 1 >>>> [12] .debug_info PROGBITS 00000000 4a381c 0066cb 00 0 0 1 >>>> [13] .debug_abbrev PROGBITS 00000000 4a9ee7 0013ea 00 0 0 1 >>>> [14] .debug_aranges PROGBITS 00000000 4ab2d8 0001a8 00 0 0 8 >>>> [15] .debug_str PROGBITS 00000000 4ab480 0019b4 01 MS 0 0 1 >>>> [16] .debug_ranges PROGBITS 00000000 4ace38 000640 00 0 0 8 >>>> [17] .debug_frame PROGBITS 00000000 4ad478 001010 00 0 0 4 >>>> [18] .debug_loc PROGBITS 00000000 4ae488 003643 00 0 0 1 >>>> [19] .symtab SYMTAB 00000000 4b1acc 0015b0 10 20 225 4 >>>> [20] .strtab STRTAB 00000000 4b307c 000cc5 00 0 0 1 >>>> [21] .shstrtab STRTAB 00000000 4b3d41 0000d2 00 0 0 1 >>>> Key to Flags: >>>> W (write), A (alloc), X (execute), M (merge), S (strings), I (info), >>>> L (link order), O (extra OS processing required), G (group), T (TLS), >>>> C (compressed), x (unknown), o (OS specific), E (exclude), >>>> y (purecode), p (processor specific) >>>> >>> >>> And this is from the build the build that generated the linker assert? >> >> Humm no, actually it was with the wrong branch. If I have the patch >> "ARM: verify size of zImage" then arch/arm/boot/compressed/vmlinux is >> not generated. >> >> But if I remove this patch then I can generate this file and so: >> arm-linux-gnueabi-readelf -S ../build/arch/arm/boot/compressed/vmlinux >> There are 22 section headers, starting at offset 0x4b402c: >> >> Section Headers: >> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >> [ 0] NULL 00000000 000000 000000 00 0 0 0 >> [ 1] .text PROGBITS 00000000 010000 005ef8 00 AX 0 0 32 >> [ 2] .rodata PROGBITS 00005ef8 015ef8 000d4c 00 A 0 0 4 >> [ 3] .piggydata PROGBITS 00006c44 016c44 48a552 00 A 0 0 1 >> [ 4] .got.plt PROGBITS 00491198 4a1198 00000c 04 WA 0 0 4 >> [ 5] .got PROGBITS 004911a4 4a11a4 000028 00 WA 0 0 4 >> [ 6] .pad PROGBITS 004911cc 4a11cc 000004 00 WA 0 0 1 >> [ 7] .bss NOBITS 004911d0 4a11d0 00001c 00 WA 0 0 4 >> [ 8] .stack NOBITS 004911f0 4a11d0 001000 00 WA 0 0 1 >> [ 9] .comment PROGBITS 00000000 4a11d0 00001c 01 MS 0 0 1 >> [10] .ARM.attributes ARM_ATTRIBUTES 00000000 4a11ec 00002d 00 0 0 1 >> [11] .debug_line PROGBITS 00000000 4a1219 00281b 00 0 0 1 >> [12] .debug_info PROGBITS 00000000 4a3a34 0066cb 00 0 0 1 >> [13] .debug_abbrev PROGBITS 00000000 4aa0ff 0013ea 00 0 0 1 >> [14] .debug_aranges PROGBITS 00000000 4ab4f0 0001a8 00 0 0 8 >> [15] .debug_str PROGBITS 00000000 4ab698 0019b4 01 MS 0 0 1 >> [16] .debug_ranges PROGBITS 00000000 4ad050 000640 00 0 0 8 >> [17] .debug_frame PROGBITS 00000000 4ad690 001010 00 0 0 4 >> [18] .debug_loc PROGBITS 00000000 4ae6a0 003643 00 0 0 1 >> [19] .symtab SYMTAB 00000000 4b1ce4 0015b0 10 20 225 4 >> [20] .strtab STRTAB 00000000 4b3294 000cc5 00 0 0 1 >> [21] .shstrtab STRTAB 00000000 4b3f59 0000d2 00 0 0 1 >> >>> >>> Could you check the value of _edata in the output of 'nm vmlinux' >>> please? >> With this kernel: >> >> arm-linux-gnueabi-nm ../build/arch/arm/boot/compressed/vmlinux | grep _edata >> 004911d0 D _edata >> > > Well, that is disappointing. This means the ASSERT() does not work > reliably, and we're back to using a bunch of shell scripts to check > whether _edata appears at the end of the image. > > Which version of binutils are you using? arm-linux-gnueabi-ld -v GNU ld (GNU Binutils for Debian) 2.29.1 And for gcc: arm-linux-gnueabi-gcc -v Using built-in specs. COLLECT_GCC=arm-linux-gnueabi-gcc COLLECT_LTO_WRAPPER=/usr/lib/gcc-cross/arm-linux-gnueabi/7/lto-wrapper Target: arm-linux-gnueabi Configured with: ../src/configure -v --with-pkgversion='Debian 7.2.0-6' --with-bugurl=file:///usr/share/doc/gcc-7/README.Bugs --enable-languages=c,ada,c++,go,d,fortran,objc,obj-c++ --prefix=/usr --with-gcc-major-version-only --program-suffix=-7 --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-libitm --disable-libquadmath --enable-plugin --enable-default-pie --with-system-zlib --with-target-system-zlib --enable-multiarch --disable-sjlj-exceptions --with-arch=armv4t --with-float=soft --disable-werror --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=arm-linux-gnueabi --program-prefix=arm-linux-gnueabi- --includedir=/usr/arm-linux-gnueabi/include Thread model: posix gcc version 7.2.0 (Debian 7.2.0-6) Gregory
On Mon, Oct 30, 2017 at 02:48:02PM +0100, Gregory CLEMENT wrote: > Hi Russell, > > So I tested the branch fixes in your git tree. > > After doing a "make multi_v7_defconfig; make zImage", I got the message > "arm-linux-gnueabi-ld: error: zImage file size is incorrect" you added > in the commit "ARM: verify size of zImage". > > It is the same with mvebu_v7_defconfig, so I wonder wich with > configuration this patch was tested ? I heard a similar report from Olof when his autobuilder produced 100% failure. I tried one of the same defconfig's that Olof tried here, and didn't see the failure. It passes my build tests here, and it also passes kernelci's build tests too. So, I _think_ it's got something to do with the toolchain versions being used, but at the moment I'm just guessing. I've no real idea, because I've no idea what's causing the failure at the moment. Olof said that he'd send me one of the build trees, but I'm still waiting. What I need is a tarball of the objects from arch/arm/boot/compressed to work out what's going on - when grabbing that, it may be a good idea to first disable the assert in the linker script, so that the resulting "vmlinux" in that directory remains for analysis. Or... someone else needs to work out what's going on and report back.
On Mon, Oct 30, 2017 at 04:09:43PM +0100, Arnd Bergmann wrote: > On Fri, Oct 27, 2017 at 5:27 PM, Russell King - ARM Linux > <linux@armlinux.org.uk> wrote: > > > > > There's three things wrong, all of which I have patches to address: > > > > 1. The decompressor code reading the image data sometimes issues unaligned > > reads. Some compilers get this wrong and cause an abort. Arnds patch > > addresses this. > > > > 2. Additional sections can appear in the zImage binary which adds extra > > bytes on the end of the image. Concatenating the zImage with the > > extra bytes onto a DTB is the same thing as doing this: > > > > cat zImage extrabytes foo.dtb > image > > > > and the decompressor tolerates no additional bytes between the > > _official_ end of the zImage and the DTB. I've added a patch which > > detects this situation and fails the kernel build when it happens. > > > > 3. Ard's patch "efi/libstub: arm: omit sorting of the UEFI memory map" > > gets rid of the additional sections that (a) change the alignment > > of the compressed data, and (b) add additional unexpected bytes on > > the end of zImage. > > It's possible that we still need yet another patch to address the gcc bug that > Alex Graf found, i.e. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82445 > > Without the latest gcc, we might still get into a situation in which we get > an unaligned strd when compiling for armv5te or armv6 with gcc-7.[012]. > As someone mentioned in the bug report, that problem doesn't seem > to happen with gcc-6 or -mtune=xscale, or with gcc-7 -march=armv7-a. There is another solution which we've used in the past - we could detect these compiler versions and refuse to build with the broken compilers.
On Mon, Oct 30, 2017 at 04:33:07PM +0100, Gregory CLEMENT wrote: > Humm no, actually it was with the wrong branch. If I have the patch > "ARM: verify size of zImage" then arch/arm/boot/compressed/vmlinux is > not generated. > > But if I remove this patch then I can generate this file and so: > arm-linux-gnueabi-readelf -S ../build/arch/arm/boot/compressed/vmlinux > There are 22 section headers, starting at offset 0x4b402c: > > Section Headers: > [Nr] Name Type Addr Off Size ES Flg Lk Inf Al > [ 0] NULL 00000000 000000 000000 00 0 0 0 > [ 1] .text PROGBITS 00000000 010000 005ef8 00 AX 0 0 32 > [ 2] .rodata PROGBITS 00005ef8 015ef8 000d4c 00 A 0 0 4 > [ 3] .piggydata PROGBITS 00006c44 016c44 48a552 00 A 0 0 1 > [ 4] .got.plt PROGBITS 00491198 4a1198 00000c 04 WA 0 0 4 > [ 5] .got PROGBITS 004911a4 4a11a4 000028 00 WA 0 0 4 > [ 6] .pad PROGBITS 004911cc 4a11cc 000004 00 WA 0 0 1 > [ 7] .bss NOBITS 004911d0 4a11d0 00001c 00 WA 0 0 4 > [ 8] .stack NOBITS 004911f0 4a11d0 001000 00 WA 0 0 1 > [ 9] .comment PROGBITS 00000000 4a11d0 00001c 01 MS 0 0 1 > [10] .ARM.attributes ARM_ATTRIBUTES 00000000 4a11ec 00002d 00 0 0 1 > [11] .debug_line PROGBITS 00000000 4a1219 00281b 00 0 0 1 > [12] .debug_info PROGBITS 00000000 4a3a34 0066cb 00 0 0 1 > [13] .debug_abbrev PROGBITS 00000000 4aa0ff 0013ea 00 0 0 1 > [14] .debug_aranges PROGBITS 00000000 4ab4f0 0001a8 00 0 0 8 > [15] .debug_str PROGBITS 00000000 4ab698 0019b4 01 MS 0 0 1 > [16] .debug_ranges PROGBITS 00000000 4ad050 000640 00 0 0 8 > [17] .debug_frame PROGBITS 00000000 4ad690 001010 00 0 0 4 > [18] .debug_loc PROGBITS 00000000 4ae6a0 003643 00 0 0 1 > [19] .symtab SYMTAB 00000000 4b1ce4 0015b0 10 20 225 4 > [20] .strtab STRTAB 00000000 4b3294 000cc5 00 0 0 1 > [21] .shstrtab STRTAB 00000000 4b3f59 0000d2 00 0 0 1 I don't like readelf's output - please can you post the output of arm-linux-objdump -h ../build/arch/arm/boot/compressed/vmlinux instead. Thanks.
Hi Russell, On lun., oct. 30 2017, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Mon, Oct 30, 2017 at 04:33:07PM +0100, Gregory CLEMENT wrote: >> Humm no, actually it was with the wrong branch. If I have the patch >> "ARM: verify size of zImage" then arch/arm/boot/compressed/vmlinux is >> not generated. >> >> But if I remove this patch then I can generate this file and so: >> arm-linux-gnueabi-readelf -S ../build/arch/arm/boot/compressed/vmlinux >> There are 22 section headers, starting at offset 0x4b402c: >> >> Section Headers: >> [Nr] Name Type Addr Off Size ES Flg Lk Inf Al >> [ 0] NULL 00000000 000000 000000 00 0 0 0 >> [ 1] .text PROGBITS 00000000 010000 005ef8 00 AX 0 0 32 >> [ 2] .rodata PROGBITS 00005ef8 015ef8 000d4c 00 A 0 0 4 >> [ 3] .piggydata PROGBITS 00006c44 016c44 48a552 00 A 0 0 1 >> [ 4] .got.plt PROGBITS 00491198 4a1198 00000c 04 WA 0 0 4 >> [ 5] .got PROGBITS 004911a4 4a11a4 000028 00 WA 0 0 4 >> [ 6] .pad PROGBITS 004911cc 4a11cc 000004 00 WA 0 0 1 >> [ 7] .bss NOBITS 004911d0 4a11d0 00001c 00 WA 0 0 4 >> [ 8] .stack NOBITS 004911f0 4a11d0 001000 00 WA 0 0 1 >> [ 9] .comment PROGBITS 00000000 4a11d0 00001c 01 MS 0 0 1 >> [10] .ARM.attributes ARM_ATTRIBUTES 00000000 4a11ec 00002d 00 0 0 1 >> [11] .debug_line PROGBITS 00000000 4a1219 00281b 00 0 0 1 >> [12] .debug_info PROGBITS 00000000 4a3a34 0066cb 00 0 0 1 >> [13] .debug_abbrev PROGBITS 00000000 4aa0ff 0013ea 00 0 0 1 >> [14] .debug_aranges PROGBITS 00000000 4ab4f0 0001a8 00 0 0 8 >> [15] .debug_str PROGBITS 00000000 4ab698 0019b4 01 MS 0 0 1 >> [16] .debug_ranges PROGBITS 00000000 4ad050 000640 00 0 0 8 >> [17] .debug_frame PROGBITS 00000000 4ad690 001010 00 0 0 4 >> [18] .debug_loc PROGBITS 00000000 4ae6a0 003643 00 0 0 1 >> [19] .symtab SYMTAB 00000000 4b1ce4 0015b0 10 20 225 4 >> [20] .strtab STRTAB 00000000 4b3294 000cc5 00 0 0 1 >> [21] .shstrtab STRTAB 00000000 4b3f59 0000d2 00 0 0 1 > > I don't like readelf's output - please can you post the output of > arm-linux-objdump -h ../build/arch/arm/boot/compressed/vmlinux > instead. Here it is: arm-linux-gnueabi-objdump -h ../build/arch/arm/boot/compressed/vmlinux ../build/arch/arm/boot/compressed/vmlinux: file format elf32-littlearm Sections: Idx Name Size VMA LMA File off Algn 0 .text 00005ef8 00000000 00000000 00010000 2**5 CONTENTS, ALLOC, LOAD, READONLY, CODE 1 .rodata 00000d4c 00005ef8 00005ef8 00015ef8 2**2 CONTENTS, ALLOC, LOAD, READONLY, DATA 2 .piggydata 0048a552 00006c44 00006c44 00016c44 2**0 CONTENTS, ALLOC, LOAD, READONLY, DATA 3 .got.plt 0000000c 00491198 00491198 004a1198 2**2 CONTENTS, ALLOC, LOAD, DATA 4 .got 00000028 004911a4 004911a4 004a11a4 2**2 CONTENTS, ALLOC, LOAD, DATA 5 .pad 00000004 004911cc 004911cc 004a11cc 2**0 CONTENTS, ALLOC, LOAD, DATA 6 .bss 0000001c 004911d0 004911d0 004a11d0 2**2 ALLOC 7 .stack 00001000 004911f0 004911f0 004a11d0 2**0 ALLOC 8 .comment 0000001c 00000000 00000000 004a11d0 2**0 CONTENTS, READONLY 9 .ARM.attributes 0000002d 00000000 00000000 004a11ec 2**0 CONTENTS, READONLY 10 .debug_line 0000281b 00000000 00000000 004a1219 2**0 CONTENTS, READONLY, DEBUGGING 11 .debug_info 000066cb 00000000 00000000 004a3a34 2**0 CONTENTS, READONLY, DEBUGGING 12 .debug_abbrev 000013ea 00000000 00000000 004aa0ff 2**0 CONTENTS, READONLY, DEBUGGING 13 .debug_aranges 000001a8 00000000 00000000 004ab4f0 2**3 CONTENTS, READONLY, DEBUGGING 14 .debug_str 000019b4 00000000 00000000 004ab698 2**0 CONTENTS, READONLY, DEBUGGING 15 .debug_ranges 00000640 00000000 00000000 004ad050 2**3 CONTENTS, READONLY, DEBUGGING 16 .debug_frame 00001010 00000000 00000000 004ad690 2**2 CONTENTS, READONLY, DEBUGGING 17 .debug_loc 00003643 00000000 00000000 004ae6a0 2**0 CONTENTS, READONLY, DEBUGGING Gregory
On Mon, Oct 30, 2017 at 05:01:44PM +0100, Gregory CLEMENT wrote: > Hi Russell King, > > On lun., oct. 30 2017, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > > > On Mon, Oct 30, 2017 at 02:48:02PM +0100, Gregory CLEMENT wrote: > >> Hi Russell, > >> > >> So I tested the branch fixes in your git tree. > >> > >> After doing a "make multi_v7_defconfig; make zImage", I got the message > >> "arm-linux-gnueabi-ld: error: zImage file size is incorrect" you added > >> in the commit "ARM: verify size of zImage". > >> > >> It is the same with mvebu_v7_defconfig, so I wonder wich with > >> configuration this patch was tested ? > > > > I heard a similar report from Olof when his autobuilder produced 100% > > failure. I tried one of the same defconfig's that Olof tried here, > > and didn't see the failure. It passes my build tests here, and it > > also passes kernelci's build tests too. > > > > So, I _think_ it's got something to do with the toolchain versions > > being used, but at the moment I'm just guessing. I've no real idea, > > because I've no idea what's causing the failure at the moment. > > > > Olof said that he'd send me one of the build trees, but I'm still > > waiting. > > > > What I need is a tarball of the objects from arch/arm/boot/compressed > > to work out what's going on - when grabbing that, it may be a good > > I've just attached this tarball to this email. It might be rejected by > the mailing list, but as you are also in the "To" field you should > receive it. > > Actually the archive is the full content of arch/arm/boot/compressed > from my build directory and I removed vmlinux and all the piggy files to > have a small archive. > > If they are also needed then I can provide an url for it. Thanks. Unfortunately, I can't reproduce the issue using your objects and my linker. If I modify your vmlinux.lds to add the assert in, and then link using: $ arm-linux-ld -o vmlinux -T vmlinux.lds *.o --defsym _kernel_bss_size=0 \ --defsym input_data_end=0 --defsym input_data=0 Then it links successfully. If I objcopy that: $ arm-linux-objcopy -O binary -R .comment -S vmlinux zImage $ vdir zImage -rwxrwxr-x 1 rmk rmk 27784 Oct 30 16:10 zImage $ arm-linux-nm vmlinux |grep _edata 00006c88 D _edata $ echo $((0x6c88)) 27784 So it all looks sane here. So my hunch is that whatever's going wrong, it's going wrong at the final link, which means I need the vmlinux as generated by your toolchain.
Hi Russell King, On lun., oct. 30 2017, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Mon, Oct 30, 2017 at 05:01:44PM +0100, Gregory CLEMENT wrote: >> Hi Russell King, >> >> On lun., oct. 30 2017, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: >> >> > On Mon, Oct 30, 2017 at 02:48:02PM +0100, Gregory CLEMENT wrote: >> >> Hi Russell, >> >> >> >> So I tested the branch fixes in your git tree. >> >> >> >> After doing a "make multi_v7_defconfig; make zImage", I got the message >> >> "arm-linux-gnueabi-ld: error: zImage file size is incorrect" you added >> >> in the commit "ARM: verify size of zImage". >> >> >> >> It is the same with mvebu_v7_defconfig, so I wonder wich with >> >> configuration this patch was tested ? >> > >> > I heard a similar report from Olof when his autobuilder produced 100% >> > failure. I tried one of the same defconfig's that Olof tried here, >> > and didn't see the failure. It passes my build tests here, and it >> > also passes kernelci's build tests too. >> > >> > So, I _think_ it's got something to do with the toolchain versions >> > being used, but at the moment I'm just guessing. I've no real idea, >> > because I've no idea what's causing the failure at the moment. >> > >> > Olof said that he'd send me one of the build trees, but I'm still >> > waiting. >> > >> > What I need is a tarball of the objects from arch/arm/boot/compressed >> > to work out what's going on - when grabbing that, it may be a good >> >> I've just attached this tarball to this email. It might be rejected by >> the mailing list, but as you are also in the "To" field you should >> receive it. >> >> Actually the archive is the full content of arch/arm/boot/compressed >> from my build directory and I removed vmlinux and all the piggy files to >> have a small archive. >> >> If they are also needed then I can provide an url for it. > > Thanks. > > Unfortunately, I can't reproduce the issue using your objects and my > linker. > > If I modify your vmlinux.lds to add the assert in, and then link using: > > $ arm-linux-ld -o vmlinux -T vmlinux.lds *.o --defsym _kernel_bss_size=0 \ > --defsym input_data_end=0 --defsym input_data=0 > > Then it links successfully. If I objcopy that: > > $ arm-linux-objcopy -O binary -R .comment -S vmlinux zImage > $ vdir zImage > -rwxrwxr-x 1 rmk rmk 27784 Oct 30 16:10 zImage > $ arm-linux-nm vmlinux |grep _edata > 00006c88 D _edata > $ echo $((0x6c88)) > 27784 > > So it all looks sane here. So my hunch is that whatever's going wrong, > it's going wrong at the final link, which means I need the vmlinux as > generated by your toolchain. Here you will find all the objects included the vmlinux: http://free-electrons.com/~gregory/pub/compressed.tgz Gregory > > -- > 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 > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Oct 30, 2017 at 05:24:34PM +0100, Gregory CLEMENT wrote: > Hi Russell King, > > Here you will find all the objects included the vmlinux: > > http://free-electrons.com/~gregory/pub/compressed.tgz Thanks. Unfortunately, nothing stands out, but I do see a difference between the output of your linker from mine. Yours: Idx Name Size VMA LMA File off Algn 0 .text 00005ef8 00000000 00000000 00010000 2**5 CONTENTS, ALLOC, LOAD, READONLY, CODE Mine: Idx Name Size VMA LMA File off Algn 0 .text 00005f00 00000000 00000000 00010000 2**5 CONTENTS, ALLOC, LOAD, READONLY, CODE That has the effect of moving the addresses of the following sections in your vmlinux down by 8 bytes. However, I don't think that's the cause of this - but it does hint at something being different in binutils in the way sections are processed in the linker. Please add to your linker script after the assignment of _edata: .image_end (NOLOAD) : { _edata_foo = .; } relink the decompressor, and see what value _edata_foo ends up with compared to _edata? They should be the same, but I suspect using your linker, they will be different. Also try adding BYTE(0); after the _edata_foo assignment as a separate test, and see whether that makes any difference - with that you should end up with the .image_end section in the output image.
On Mon, Oct 30, 2017 at 03:35:38PM +0000, Ard Biesheuvel wrote: > Well, that is disappointing. This means the ASSERT() does not work > reliably, and we're back to using a bunch of shell scripts to check > whether _edata appears at the end of the image. That didn't work too well here. While it did correctly detect some instances: zImage size (8008200) disagrees with linked size (8008192) it also misdetected others: zImage size (13348808) disagrees with linked size (-928003328) It seems to suggest that _magic_end - _magic_start = 0xc8afcb00. zImage size is 0xcbafc8. That points at the addresses output by "nm" being dependent on the endian-ness of the image, which to me seems utterly insane. I wouldn't be surprised if that is toolchain version dependent as well. IMHO, our toolchain is a mess!
On Mon, Oct 30, 2017 at 4:50 PM, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Mon, Oct 30, 2017 at 04:09:43PM +0100, Arnd Bergmann wrote: >> On Fri, Oct 27, 2017 at 5:27 PM, Russell King - ARM Linux >> <linux@armlinux.org.uk> wrote: >> >> > >> > There's three things wrong, all of which I have patches to address: >> > >> > 1. The decompressor code reading the image data sometimes issues unaligned >> > reads. Some compilers get this wrong and cause an abort. Arnds patch >> > addresses this. >> > >> > 2. Additional sections can appear in the zImage binary which adds extra >> > bytes on the end of the image. Concatenating the zImage with the >> > extra bytes onto a DTB is the same thing as doing this: >> > >> > cat zImage extrabytes foo.dtb > image >> > >> > and the decompressor tolerates no additional bytes between the >> > _official_ end of the zImage and the DTB. I've added a patch which >> > detects this situation and fails the kernel build when it happens. >> > >> > 3. Ard's patch "efi/libstub: arm: omit sorting of the UEFI memory map" >> > gets rid of the additional sections that (a) change the alignment >> > of the compressed data, and (b) add additional unexpected bytes on >> > the end of zImage. >> >> It's possible that we still need yet another patch to address the gcc bug that >> Alex Graf found, i.e. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82445 >> >> Without the latest gcc, we might still get into a situation in which we get >> an unaligned strd when compiling for armv5te or armv6 with gcc-7.[012]. >> As someone mentioned in the bug report, that problem doesn't seem >> to happen with gcc-6 or -mtune=xscale, or with gcc-7 -march=armv7-a. > > There is another solution which we've used in the past - we could detect > these compiler versions and refuse to build with the broken compilers. Right, either fail the build or use the workaround from the gcc bugzilla, passing -fno-store-merging to the broken compilers avoids the problem reliably at the cost of slightly worse code. For the decompressor, we might also get away with passing -march=armv5t (not armv5te), which has experimentally been found to avoid the problem and produce better code than "-march=armv6 -fno-store-merging" as it avoids the strd instruction but not other store merging. For normal kernel access (after the decompressor), relying on the fixup in the kernel is probably good enough, we already have problems with a couple of functiosn that check for CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS before doing accesses that can result in ldm/stm on any compiler. I'm still trying to figure out exactly what architectures and gcc versions are affected, the gcc-6 branch contains a fix for the problem but I have not been able to reproduce it on that version or earlier. However, I have now reproduced an strd with gcc-7.1 on this code on all architectures that support strd, with this test case from the gcc testsuite: void foo(int a, int b, int* p) { p[1] = a; p[2] = b; } Arnd
On Mon, Oct 30, 2017 at 06:01:44PM +0100, Arnd Bergmann wrote: > Right, either fail the build or use the workaround from the gcc bugzilla, > passing -fno-store-merging to the broken compilers avoids the problem > reliably at the cost of slightly worse code. For the decompressor, we might > also get away with passing -march=armv5t (not armv5te), which has > experimentally been found to avoid the problem and produce better code > than "-march=armv6 -fno-store-merging" as it avoids the strd instruction > but not other store merging. Depends how important the compilers are... We currently refuse to build using these compilers: gcc = 3.x where x < 3 gcc = 4.x where x < 3 if unwinding is enabled gcc = 4.8.x where x < 3
On Mon, Oct 30, 2017 at 04:38:17PM +0000, Russell King - ARM Linux wrote: > On Mon, Oct 30, 2017 at 05:24:34PM +0100, Gregory CLEMENT wrote: > > Hi Russell King, > > > > Here you will find all the objects included the vmlinux: > > > > http://free-electrons.com/~gregory/pub/compressed.tgz > > Thanks. Unfortunately, nothing stands out, but I do see a difference > between the output of your linker from mine. > > Yours: > > Idx Name Size VMA LMA File off Algn > 0 .text 00005ef8 00000000 00000000 00010000 2**5 > CONTENTS, ALLOC, LOAD, READONLY, CODE > > Mine: > > Idx Name Size VMA LMA File off Algn > 0 .text 00005f00 00000000 00000000 00010000 2**5 > CONTENTS, ALLOC, LOAD, READONLY, CODE > > That has the effect of moving the addresses of the following > sections in your vmlinux down by 8 bytes. However, I don't think > that's the cause of this - but it does hint at something being > different in binutils in the way sections are processed in the > linker. > > Please add to your linker script after the assignment of _edata: > > .image_end (NOLOAD) : { > _edata_foo = .; > } > > relink the decompressor, and see what value _edata_foo ends up with > compared to _edata? They should be the same, but I suspect using > your linker, they will be different. > > Also try adding > BYTE(0); > > after the _edata_foo assignment as a separate test, and see whether > that makes any difference - with that you should end up with the > .image_end section in the output image. Gregory sent me has new url... for _both_ changes, which gives me: $ arm-linux-nm vmlinux |grep _edata 00491160 D _edata 00491160 D _edata_foo So there's no reason that ASSERT() should be failing! However, as I don't have the intermediate step, I can't say whether the addition of the BYTE() affected it in some way - sorry, but I asked for _both_ to be tested above because I wanted to speed up the process, and clearly that's backfired. Given how close we potentially are to 4.14, I don't think we're going to get to the bottom of this to make 4.14. I'd want to get this sorted by Wednesday so linux-next (which is resuming this evening) can grab a copy of my tree with it in, and we have another day to sort out any remaining issues, but I'm basically out of time to do anything further with this as of now. So, 4.14 will likely be released without any of this being fixed.
On 31 October 2017 at 12:47, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Mon, Oct 30, 2017 at 04:38:17PM +0000, Russell King - ARM Linux wrote: >> On Mon, Oct 30, 2017 at 05:24:34PM +0100, Gregory CLEMENT wrote: >> > Hi Russell King, >> > >> > Here you will find all the objects included the vmlinux: >> > >> > http://free-electrons.com/~gregory/pub/compressed.tgz >> >> Thanks. Unfortunately, nothing stands out, but I do see a difference >> between the output of your linker from mine. >> >> Yours: >> >> Idx Name Size VMA LMA File off Algn >> 0 .text 00005ef8 00000000 00000000 00010000 2**5 >> CONTENTS, ALLOC, LOAD, READONLY, CODE >> >> Mine: >> >> Idx Name Size VMA LMA File off Algn >> 0 .text 00005f00 00000000 00000000 00010000 2**5 >> CONTENTS, ALLOC, LOAD, READONLY, CODE >> >> That has the effect of moving the addresses of the following >> sections in your vmlinux down by 8 bytes. However, I don't think >> that's the cause of this - but it does hint at something being >> different in binutils in the way sections are processed in the >> linker. >> >> Please add to your linker script after the assignment of _edata: >> >> .image_end (NOLOAD) : { >> _edata_foo = .; >> } >> >> relink the decompressor, and see what value _edata_foo ends up with >> compared to _edata? They should be the same, but I suspect using >> your linker, they will be different. >> >> Also try adding >> BYTE(0); >> >> after the _edata_foo assignment as a separate test, and see whether >> that makes any difference - with that you should end up with the >> .image_end section in the output image. > > Gregory sent me has new url... for _both_ changes, which gives me: > > $ arm-linux-nm vmlinux |grep _edata > 00491160 D _edata > 00491160 D _edata_foo > > So there's no reason that ASSERT() should be failing! However, as I > don't have the intermediate step, I can't say whether the addition > of the BYTE() affected it in some way - sorry, but I asked for _both_ > to be tested above because I wanted to speed up the process, and > clearly that's backfired. > > Given how close we potentially are to 4.14, I don't think we're going > to get to the bottom of this to make 4.14. I'd want to get this > sorted by Wednesday so linux-next (which is resuming this evening) > can grab a copy of my tree with it in, and we have another day to > sort out any remaining issues, but I'm basically out of time to do > anything further with this as of now. > > So, 4.14 will likely be released without any of this being fixed. > IIUC, the current issue is limited to the ASSERT() itself, which is there to prevent future regressions, while the other two patches deal with severe and difficult to diagnose known issues. So why can't we apply those two patches as fixes, and revisit the patch that helps us prevent this from regressing in the future for v4.15?
Hi Ard, On mar., oct. 31 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 31 October 2017 at 12:47, Russell King - ARM Linux > <linux@armlinux.org.uk> wrote: >> On Mon, Oct 30, 2017 at 04:38:17PM +0000, Russell King - ARM Linux wrote: >>> On Mon, Oct 30, 2017 at 05:24:34PM +0100, Gregory CLEMENT wrote: >>> > Hi Russell King, >>> > >>> > Here you will find all the objects included the vmlinux: >>> > >>> > http://free-electrons.com/~gregory/pub/compressed.tgz >>> >>> Thanks. Unfortunately, nothing stands out, but I do see a difference >>> between the output of your linker from mine. >>> >>> Yours: >>> >>> Idx Name Size VMA LMA File off Algn >>> 0 .text 00005ef8 00000000 00000000 00010000 2**5 >>> CONTENTS, ALLOC, LOAD, READONLY, CODE >>> >>> Mine: >>> >>> Idx Name Size VMA LMA File off Algn >>> 0 .text 00005f00 00000000 00000000 00010000 2**5 >>> CONTENTS, ALLOC, LOAD, READONLY, CODE >>> >>> That has the effect of moving the addresses of the following >>> sections in your vmlinux down by 8 bytes. However, I don't think >>> that's the cause of this - but it does hint at something being >>> different in binutils in the way sections are processed in the >>> linker. >>> >>> Please add to your linker script after the assignment of _edata: >>> >>> .image_end (NOLOAD) : { >>> _edata_foo = .; >>> } >>> >>> relink the decompressor, and see what value _edata_foo ends up with >>> compared to _edata? They should be the same, but I suspect using >>> your linker, they will be different. >>> >>> Also try adding >>> BYTE(0); >>> >>> after the _edata_foo assignment as a separate test, and see whether >>> that makes any difference - with that you should end up with the >>> .image_end section in the output image. >> >> Gregory sent me has new url... for _both_ changes, which gives me: If needed I can provide this url. >> >> $ arm-linux-nm vmlinux |grep _edata >> 00491160 D _edata >> 00491160 D _edata_foo >> >> So there's no reason that ASSERT() should be failing! However, as I >> don't have the intermediate step, I can't say whether the addition >> of the BYTE() affected it in some way - sorry, but I asked for _both_ >> to be tested above because I wanted to speed up the process, and >> clearly that's backfired. >> >> Given how close we potentially are to 4.14, I don't think we're going >> to get to the bottom of this to make 4.14. I'd want to get this >> sorted by Wednesday so linux-next (which is resuming this evening) >> can grab a copy of my tree with it in, and we have another day to >> sort out any remaining issues, but I'm basically out of time to do >> anything further with this as of now. > >> So, 4.14 will likely be released without any of this being fixed. >> > > IIUC, the current issue is limited to the ASSERT() itself, which is > there to prevent future regressions, while the other two patches deal > with severe and difficult to diagnose known issues. I confirm that whithout the last commit (adding the ASSERT()) in the fixes branch it worked well. > > So why can't we apply those two patches as fixes, and revisit the > patch that helps us prevent this from regressing in the future for > v4.15? I also agree with this. Gregory
On 31 October 2017 at 13:22, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > Hi Ard, > > On mar., oct. 31 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >> On 31 October 2017 at 12:47, Russell King - ARM Linux >> <linux@armlinux.org.uk> wrote: >>> On Mon, Oct 30, 2017 at 04:38:17PM +0000, Russell King - ARM Linux wrote: >>>> On Mon, Oct 30, 2017 at 05:24:34PM +0100, Gregory CLEMENT wrote: >>>> > Hi Russell King, >>>> > >>>> > Here you will find all the objects included the vmlinux: >>>> > >>>> > http://free-electrons.com/~gregory/pub/compressed.tgz >>>> >>>> Thanks. Unfortunately, nothing stands out, but I do see a difference >>>> between the output of your linker from mine. >>>> >>>> Yours: >>>> >>>> Idx Name Size VMA LMA File off Algn >>>> 0 .text 00005ef8 00000000 00000000 00010000 2**5 >>>> CONTENTS, ALLOC, LOAD, READONLY, CODE >>>> >>>> Mine: >>>> >>>> Idx Name Size VMA LMA File off Algn >>>> 0 .text 00005f00 00000000 00000000 00010000 2**5 >>>> CONTENTS, ALLOC, LOAD, READONLY, CODE >>>> >>>> That has the effect of moving the addresses of the following >>>> sections in your vmlinux down by 8 bytes. However, I don't think >>>> that's the cause of this - but it does hint at something being >>>> different in binutils in the way sections are processed in the >>>> linker. >>>> >>>> Please add to your linker script after the assignment of _edata: >>>> >>>> .image_end (NOLOAD) : { >>>> _edata_foo = .; >>>> } >>>> >>>> relink the decompressor, and see what value _edata_foo ends up with >>>> compared to _edata? They should be the same, but I suspect using >>>> your linker, they will be different. >>>> >>>> Also try adding >>>> BYTE(0); >>>> >>>> after the _edata_foo assignment as a separate test, and see whether >>>> that makes any difference - with that you should end up with the >>>> .image_end section in the output image. >>> >>> Gregory sent me has new url... for _both_ changes, which gives me: > > If needed I can provide this url. > >>> >>> $ arm-linux-nm vmlinux |grep _edata >>> 00491160 D _edata >>> 00491160 D _edata_foo >>> >>> So there's no reason that ASSERT() should be failing! However, as I >>> don't have the intermediate step, I can't say whether the addition >>> of the BYTE() affected it in some way - sorry, but I asked for _both_ >>> to be tested above because I wanted to speed up the process, and >>> clearly that's backfired. >>> >>> Given how close we potentially are to 4.14, I don't think we're going >>> to get to the bottom of this to make 4.14. I'd want to get this >>> sorted by Wednesday so linux-next (which is resuming this evening) >>> can grab a copy of my tree with it in, and we have another day to >>> sort out any remaining issues, but I'm basically out of time to do >>> anything further with this as of now. >> >>> So, 4.14 will likely be released without any of this being fixed. >>> >> >> IIUC, the current issue is limited to the ASSERT() itself, which is >> there to prevent future regressions, while the other two patches deal >> with severe and difficult to diagnose known issues. > > I confirm that whithout the last commit (adding the ASSERT()) in the > fixes branch it worked well. > >> >> So why can't we apply those two patches as fixes, and revisit the >> patch that helps us prevent this from regressing in the future for >> v4.15? > > I also agree with this. > Russell, Please drop the EFI patch from your tree. I will forward it myself. Thanks, Ard.
On Wed, Nov 01, 2017 at 03:57:36PM +0000, Ard Biesheuvel wrote: > On 31 October 2017 at 13:22, Gregory CLEMENT > <gregory.clement@free-electrons.com> wrote: > > Hi Ard, > > > > On mar., oct. 31 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > > >> On 31 October 2017 at 12:47, Russell King - ARM Linux > >> <linux@armlinux.org.uk> wrote: > >>> On Mon, Oct 30, 2017 at 04:38:17PM +0000, Russell King - ARM Linux wrote: > >>>> On Mon, Oct 30, 2017 at 05:24:34PM +0100, Gregory CLEMENT wrote: > >>>> > Hi Russell King, > >>>> > > >>>> > Here you will find all the objects included the vmlinux: > >>>> > > >>>> > http://free-electrons.com/~gregory/pub/compressed.tgz > >>>> > >>>> Thanks. Unfortunately, nothing stands out, but I do see a difference > >>>> between the output of your linker from mine. > >>>> > >>>> Yours: > >>>> > >>>> Idx Name Size VMA LMA File off Algn > >>>> 0 .text 00005ef8 00000000 00000000 00010000 2**5 > >>>> CONTENTS, ALLOC, LOAD, READONLY, CODE > >>>> > >>>> Mine: > >>>> > >>>> Idx Name Size VMA LMA File off Algn > >>>> 0 .text 00005f00 00000000 00000000 00010000 2**5 > >>>> CONTENTS, ALLOC, LOAD, READONLY, CODE > >>>> > >>>> That has the effect of moving the addresses of the following > >>>> sections in your vmlinux down by 8 bytes. However, I don't think > >>>> that's the cause of this - but it does hint at something being > >>>> different in binutils in the way sections are processed in the > >>>> linker. > >>>> > >>>> Please add to your linker script after the assignment of _edata: > >>>> > >>>> .image_end (NOLOAD) : { > >>>> _edata_foo = .; > >>>> } > >>>> > >>>> relink the decompressor, and see what value _edata_foo ends up with > >>>> compared to _edata? They should be the same, but I suspect using > >>>> your linker, they will be different. > >>>> > >>>> Also try adding > >>>> BYTE(0); > >>>> > >>>> after the _edata_foo assignment as a separate test, and see whether > >>>> that makes any difference - with that you should end up with the > >>>> .image_end section in the output image. > >>> > >>> Gregory sent me has new url... for _both_ changes, which gives me: > > > > If needed I can provide this url. > > > >>> > >>> $ arm-linux-nm vmlinux |grep _edata > >>> 00491160 D _edata > >>> 00491160 D _edata_foo > >>> > >>> So there's no reason that ASSERT() should be failing! However, as I > >>> don't have the intermediate step, I can't say whether the addition > >>> of the BYTE() affected it in some way - sorry, but I asked for _both_ > >>> to be tested above because I wanted to speed up the process, and > >>> clearly that's backfired. > >>> > >>> Given how close we potentially are to 4.14, I don't think we're going > >>> to get to the bottom of this to make 4.14. I'd want to get this > >>> sorted by Wednesday so linux-next (which is resuming this evening) > >>> can grab a copy of my tree with it in, and we have another day to > >>> sort out any remaining issues, but I'm basically out of time to do > >>> anything further with this as of now. > >> > >>> So, 4.14 will likely be released without any of this being fixed. > >>> > >> > >> IIUC, the current issue is limited to the ASSERT() itself, which is > >> there to prevent future regressions, while the other two patches deal > >> with severe and difficult to diagnose known issues. > > > > I confirm that whithout the last commit (adding the ASSERT()) in the > > fixes branch it worked well. > > > >> > >> So why can't we apply those two patches as fixes, and revisit the > >> patch that helps us prevent this from regressing in the future for > >> v4.15? > > > > I also agree with this. > > > > Russell, > > Please drop the EFI patch from your tree. I will forward it myself. Really, no, I'm not going to. I've enough to do than chase around playing political games about who should send what patches. You asked me to merge it, and I will merge it.
On 1 November 2017 at 18:00, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Wed, Nov 01, 2017 at 03:57:36PM +0000, Ard Biesheuvel wrote: >> On 31 October 2017 at 13:22, Gregory CLEMENT >> <gregory.clement@free-electrons.com> wrote: >> > Hi Ard, >> > >> > On mar., oct. 31 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> > >> >> On 31 October 2017 at 12:47, Russell King - ARM Linux >> >> <linux@armlinux.org.uk> wrote: >> >>> On Mon, Oct 30, 2017 at 04:38:17PM +0000, Russell King - ARM Linux wrote: >> >>>> On Mon, Oct 30, 2017 at 05:24:34PM +0100, Gregory CLEMENT wrote: >> >>>> > Hi Russell King, >> >>>> > >> >>>> > Here you will find all the objects included the vmlinux: >> >>>> > >> >>>> > http://free-electrons.com/~gregory/pub/compressed.tgz >> >>>> >> >>>> Thanks. Unfortunately, nothing stands out, but I do see a difference >> >>>> between the output of your linker from mine. >> >>>> >> >>>> Yours: >> >>>> >> >>>> Idx Name Size VMA LMA File off Algn >> >>>> 0 .text 00005ef8 00000000 00000000 00010000 2**5 >> >>>> CONTENTS, ALLOC, LOAD, READONLY, CODE >> >>>> >> >>>> Mine: >> >>>> >> >>>> Idx Name Size VMA LMA File off Algn >> >>>> 0 .text 00005f00 00000000 00000000 00010000 2**5 >> >>>> CONTENTS, ALLOC, LOAD, READONLY, CODE >> >>>> >> >>>> That has the effect of moving the addresses of the following >> >>>> sections in your vmlinux down by 8 bytes. However, I don't think >> >>>> that's the cause of this - but it does hint at something being >> >>>> different in binutils in the way sections are processed in the >> >>>> linker. >> >>>> >> >>>> Please add to your linker script after the assignment of _edata: >> >>>> >> >>>> .image_end (NOLOAD) : { >> >>>> _edata_foo = .; >> >>>> } >> >>>> >> >>>> relink the decompressor, and see what value _edata_foo ends up with >> >>>> compared to _edata? They should be the same, but I suspect using >> >>>> your linker, they will be different. >> >>>> >> >>>> Also try adding >> >>>> BYTE(0); >> >>>> >> >>>> after the _edata_foo assignment as a separate test, and see whether >> >>>> that makes any difference - with that you should end up with the >> >>>> .image_end section in the output image. >> >>> >> >>> Gregory sent me has new url... for _both_ changes, which gives me: >> > >> > If needed I can provide this url. >> > >> >>> >> >>> $ arm-linux-nm vmlinux |grep _edata >> >>> 00491160 D _edata >> >>> 00491160 D _edata_foo >> >>> >> >>> So there's no reason that ASSERT() should be failing! However, as I >> >>> don't have the intermediate step, I can't say whether the addition >> >>> of the BYTE() affected it in some way - sorry, but I asked for _both_ >> >>> to be tested above because I wanted to speed up the process, and >> >>> clearly that's backfired. >> >>> >> >>> Given how close we potentially are to 4.14, I don't think we're going >> >>> to get to the bottom of this to make 4.14. I'd want to get this >> >>> sorted by Wednesday so linux-next (which is resuming this evening) >> >>> can grab a copy of my tree with it in, and we have another day to >> >>> sort out any remaining issues, but I'm basically out of time to do >> >>> anything further with this as of now. >> >> >> >>> So, 4.14 will likely be released without any of this being fixed. >> >>> >> >> >> >> IIUC, the current issue is limited to the ASSERT() itself, which is >> >> there to prevent future regressions, while the other two patches deal >> >> with severe and difficult to diagnose known issues. >> > >> > I confirm that whithout the last commit (adding the ASSERT()) in the >> > fixes branch it worked well. >> > >> >> >> >> So why can't we apply those two patches as fixes, and revisit the >> >> patch that helps us prevent this from regressing in the future for >> >> v4.15? >> > >> > I also agree with this. >> > >> >> Russell, >> >> Please drop the EFI patch from your tree. I will forward it myself. > > Really, no, I'm not going to. I've enough to do than chase around > playing political games about who should send what patches. You > asked me to merge it, and I will merge it. > Fine, then merge it. I am not the one who is playing games here, I just want to get stuff fixed. I don't understand why you are dragging your feet like this.
On Wed, Nov 01, 2017 at 06:02:24PM +0000, Ard Biesheuvel wrote: > On 1 November 2017 at 18:00, Russell King - ARM Linux > <linux@armlinux.org.uk> wrote: > > On Wed, Nov 01, 2017 at 03:57:36PM +0000, Ard Biesheuvel wrote: > >> On 31 October 2017 at 13:22, Gregory CLEMENT > >> <gregory.clement@free-electrons.com> wrote: > >> > Hi Ard, > >> > > >> > On mar., oct. 31 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >> > > >> >> On 31 October 2017 at 12:47, Russell King - ARM Linux > >> >> <linux@armlinux.org.uk> wrote: > >> >>> On Mon, Oct 30, 2017 at 04:38:17PM +0000, Russell King - ARM Linux wrote: > >> >>>> On Mon, Oct 30, 2017 at 05:24:34PM +0100, Gregory CLEMENT wrote: > >> >>>> > Hi Russell King, > >> >>>> > > >> >>>> > Here you will find all the objects included the vmlinux: > >> >>>> > > >> >>>> > http://free-electrons.com/~gregory/pub/compressed.tgz > >> >>>> > >> >>>> Thanks. Unfortunately, nothing stands out, but I do see a difference > >> >>>> between the output of your linker from mine. > >> >>>> > >> >>>> Yours: > >> >>>> > >> >>>> Idx Name Size VMA LMA File off Algn > >> >>>> 0 .text 00005ef8 00000000 00000000 00010000 2**5 > >> >>>> CONTENTS, ALLOC, LOAD, READONLY, CODE > >> >>>> > >> >>>> Mine: > >> >>>> > >> >>>> Idx Name Size VMA LMA File off Algn > >> >>>> 0 .text 00005f00 00000000 00000000 00010000 2**5 > >> >>>> CONTENTS, ALLOC, LOAD, READONLY, CODE > >> >>>> > >> >>>> That has the effect of moving the addresses of the following > >> >>>> sections in your vmlinux down by 8 bytes. However, I don't think > >> >>>> that's the cause of this - but it does hint at something being > >> >>>> different in binutils in the way sections are processed in the > >> >>>> linker. > >> >>>> > >> >>>> Please add to your linker script after the assignment of _edata: > >> >>>> > >> >>>> .image_end (NOLOAD) : { > >> >>>> _edata_foo = .; > >> >>>> } > >> >>>> > >> >>>> relink the decompressor, and see what value _edata_foo ends up with > >> >>>> compared to _edata? They should be the same, but I suspect using > >> >>>> your linker, they will be different. > >> >>>> > >> >>>> Also try adding > >> >>>> BYTE(0); > >> >>>> > >> >>>> after the _edata_foo assignment as a separate test, and see whether > >> >>>> that makes any difference - with that you should end up with the > >> >>>> .image_end section in the output image. > >> >>> > >> >>> Gregory sent me has new url... for _both_ changes, which gives me: > >> > > >> > If needed I can provide this url. > >> > > >> >>> > >> >>> $ arm-linux-nm vmlinux |grep _edata > >> >>> 00491160 D _edata > >> >>> 00491160 D _edata_foo > >> >>> > >> >>> So there's no reason that ASSERT() should be failing! However, as I > >> >>> don't have the intermediate step, I can't say whether the addition > >> >>> of the BYTE() affected it in some way - sorry, but I asked for _both_ > >> >>> to be tested above because I wanted to speed up the process, and > >> >>> clearly that's backfired. > >> >>> > >> >>> Given how close we potentially are to 4.14, I don't think we're going > >> >>> to get to the bottom of this to make 4.14. I'd want to get this > >> >>> sorted by Wednesday so linux-next (which is resuming this evening) > >> >>> can grab a copy of my tree with it in, and we have another day to > >> >>> sort out any remaining issues, but I'm basically out of time to do > >> >>> anything further with this as of now. > >> >> > >> >>> So, 4.14 will likely be released without any of this being fixed. > >> >>> > >> >> > >> >> IIUC, the current issue is limited to the ASSERT() itself, which is > >> >> there to prevent future regressions, while the other two patches deal > >> >> with severe and difficult to diagnose known issues. > >> > > >> > I confirm that whithout the last commit (adding the ASSERT()) in the > >> > fixes branch it worked well. > >> > > >> >> > >> >> So why can't we apply those two patches as fixes, and revisit the > >> >> patch that helps us prevent this from regressing in the future for > >> >> v4.15? > >> > > >> > I also agree with this. > >> > > >> > >> Russell, > >> > >> Please drop the EFI patch from your tree. I will forward it myself. > > > > Really, no, I'm not going to. I've enough to do than chase around > > playing political games about who should send what patches. You > > asked me to merge it, and I will merge it. > > > > Fine, then merge it. I am not the one who is playing games here, I > just want to get stuff fixed. I don't understand why you are dragging > your feet like this. You think I'm playing games? I most certainly am not. I'm not going to send it tonight because there's further fixes in the pipeline from Nicolas, and I don't have time to merge those tonight _and_ test them. And I certainly do not want to be sending multiple pull requests to Linus, because that annoys Linus and I've been flamed for doing that. I haven't had time to drop the ASSERT() patch from my tree either since Tuesday morning. I guess you have very little patience and you have no appreciation of the fact that when someone states that they want to get an issue sorted quickly because of lack of time, they actually really do mean that they have very little availability... I said on Monday that time was short, and that I had precious little available time. That was because I was out on Monday evening. I was out on Tuesday afternoon for a medical appointment. I was out Tuesday evening. I've been out Wednesday afternoon. This is not "games", this is reality, this is health issues. Have some patience and give your fellow developers some breathing space. Remember, many people have been at ELC and have been unavailable for much of last week. Those who weren't at ELC were available, willing and able to try and address these issues, but it was impossible because of everyone else being away. Now they're back, it seems they're banging on about getting some action. That's really unfair - _they're_ the ones who've held up progress for a week.
On 1 November 2017 at 18:11, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Wed, Nov 01, 2017 at 06:02:24PM +0000, Ard Biesheuvel wrote: >> On 1 November 2017 at 18:00, Russell King - ARM Linux >> <linux@armlinux.org.uk> wrote: >> > On Wed, Nov 01, 2017 at 03:57:36PM +0000, Ard Biesheuvel wrote: >> >> On 31 October 2017 at 13:22, Gregory CLEMENT >> >> <gregory.clement@free-electrons.com> wrote: >> >> > Hi Ard, >> >> > >> >> > On mar., oct. 31 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> >> > >> >> >> On 31 October 2017 at 12:47, Russell King - ARM Linux >> >> >> <linux@armlinux.org.uk> wrote: >> >> >>> On Mon, Oct 30, 2017 at 04:38:17PM +0000, Russell King - ARM Linux wrote: >> >> >>>> On Mon, Oct 30, 2017 at 05:24:34PM +0100, Gregory CLEMENT wrote: >> >> >>>> > Hi Russell King, >> >> >>>> > >> >> >>>> > Here you will find all the objects included the vmlinux: >> >> >>>> > >> >> >>>> > http://free-electrons.com/~gregory/pub/compressed.tgz >> >> >>>> >> >> >>>> Thanks. Unfortunately, nothing stands out, but I do see a difference >> >> >>>> between the output of your linker from mine. >> >> >>>> >> >> >>>> Yours: >> >> >>>> >> >> >>>> Idx Name Size VMA LMA File off Algn >> >> >>>> 0 .text 00005ef8 00000000 00000000 00010000 2**5 >> >> >>>> CONTENTS, ALLOC, LOAD, READONLY, CODE >> >> >>>> >> >> >>>> Mine: >> >> >>>> >> >> >>>> Idx Name Size VMA LMA File off Algn >> >> >>>> 0 .text 00005f00 00000000 00000000 00010000 2**5 >> >> >>>> CONTENTS, ALLOC, LOAD, READONLY, CODE >> >> >>>> >> >> >>>> That has the effect of moving the addresses of the following >> >> >>>> sections in your vmlinux down by 8 bytes. However, I don't think >> >> >>>> that's the cause of this - but it does hint at something being >> >> >>>> different in binutils in the way sections are processed in the >> >> >>>> linker. >> >> >>>> >> >> >>>> Please add to your linker script after the assignment of _edata: >> >> >>>> >> >> >>>> .image_end (NOLOAD) : { >> >> >>>> _edata_foo = .; >> >> >>>> } >> >> >>>> >> >> >>>> relink the decompressor, and see what value _edata_foo ends up with >> >> >>>> compared to _edata? They should be the same, but I suspect using >> >> >>>> your linker, they will be different. >> >> >>>> >> >> >>>> Also try adding >> >> >>>> BYTE(0); >> >> >>>> >> >> >>>> after the _edata_foo assignment as a separate test, and see whether >> >> >>>> that makes any difference - with that you should end up with the >> >> >>>> .image_end section in the output image. >> >> >>> >> >> >>> Gregory sent me has new url... for _both_ changes, which gives me: >> >> > >> >> > If needed I can provide this url. >> >> > >> >> >>> >> >> >>> $ arm-linux-nm vmlinux |grep _edata >> >> >>> 00491160 D _edata >> >> >>> 00491160 D _edata_foo >> >> >>> >> >> >>> So there's no reason that ASSERT() should be failing! However, as I >> >> >>> don't have the intermediate step, I can't say whether the addition >> >> >>> of the BYTE() affected it in some way - sorry, but I asked for _both_ >> >> >>> to be tested above because I wanted to speed up the process, and >> >> >>> clearly that's backfired. >> >> >>> >> >> >>> Given how close we potentially are to 4.14, I don't think we're going >> >> >>> to get to the bottom of this to make 4.14. I'd want to get this >> >> >>> sorted by Wednesday so linux-next (which is resuming this evening) >> >> >>> can grab a copy of my tree with it in, and we have another day to >> >> >>> sort out any remaining issues, but I'm basically out of time to do >> >> >>> anything further with this as of now. >> >> >> >> >> >>> So, 4.14 will likely be released without any of this being fixed. >> >> >>> >> >> >> >> >> >> IIUC, the current issue is limited to the ASSERT() itself, which is >> >> >> there to prevent future regressions, while the other two patches deal >> >> >> with severe and difficult to diagnose known issues. >> >> > >> >> > I confirm that whithout the last commit (adding the ASSERT()) in the >> >> > fixes branch it worked well. >> >> > >> >> >> >> >> >> So why can't we apply those two patches as fixes, and revisit the >> >> >> patch that helps us prevent this from regressing in the future for >> >> >> v4.15? >> >> > >> >> > I also agree with this. >> >> > >> >> >> >> Russell, >> >> >> >> Please drop the EFI patch from your tree. I will forward it myself. >> > >> > Really, no, I'm not going to. I've enough to do than chase around >> > playing political games about who should send what patches. You >> > asked me to merge it, and I will merge it. >> > >> >> Fine, then merge it. I am not the one who is playing games here, I >> just want to get stuff fixed. I don't understand why you are dragging >> your feet like this. > > You think I'm playing games? I most certainly am not. I'm not going > to send it tonight because there's further fixes in the pipeline from > Nicolas, and I don't have time to merge those tonight _and_ test them. > And I certainly do not want to be sending multiple pull requests to > Linus, because that annoys Linus and I've been flamed for doing that. > > I haven't had time to drop the ASSERT() patch from my tree either since > Tuesday morning. > > I guess you have very little patience and you have no appreciation of > the fact that when someone states that they want to get an issue sorted > quickly because of lack of time, they actually really do mean that they > have very little availability... > > I said on Monday that time was short, and that I had precious little > available time. That was because I was out on Monday evening. I was > out on Tuesday afternoon for a medical appointment. I was out Tuesday > evening. I've been out Wednesday afternoon. This is not "games", this > is reality, this is health issues. > > Have some patience and give your fellow developers some breathing space. > Apologies if that sounded rude, but the first fix I proposed for Gregory's issue was sent on September 8th, i.e., almost two months ago. > Remember, many people have been at ELC and have been unavailable for > much of last week. Those who weren't at ELC were available, willing > and able to try and address these issues, but it was impossible because > of everyone else being away. Now they're back, it seems they're banging > on about getting some action. That's really unfair - _they're_ the ones > who've held up progress for a week. >
On Wed, Nov 01, 2017 at 06:20:25PM +0000, Ard Biesheuvel wrote: > On 1 November 2017 at 18:11, Russell King - ARM Linux > <linux@armlinux.org.uk> wrote: > > Have some patience and give your fellow developers some breathing space. > > > > Apologies if that sounded rude, but the first fix I proposed for > Gregory's issue was sent on September 8th, i.e., almost two months > ago. I still do not agree that the patch you came up with on the 8th September was reasonable. It seemed to be a case of "oh, we have these extra sections, let's get rid of them" and "let's align the piggy data". There was no investigation _why_ and no justification for any of it other than "it seems to fix a problem". Sorry, that's way too vague, and hacky. Having waited those two months, we now understand what is really going on, why things have broken, and we have a completely different set of fixes for it. More importantly, we have the necessary understanding to prevent a reoccurance in the future by detecting it. Had your original patch on the 8th September been merged, we wouldn't be in this position, and we wouldn't have this additional understanding. So, IMHO the wait has been /well/ worth it. Non-boot problems are normally the hardest to solve, and it's always worth properly understanding them rather than applying sticky plasters.
diff --git a/arch/arm/include/asm/Kbuild b/arch/arm/include/asm/Kbuild index 721ab5ecfb9b..0f2c8a2a8131 100644 --- a/arch/arm/include/asm/Kbuild +++ b/arch/arm/include/asm/Kbuild @@ -20,7 +20,6 @@ generic-y += simd.h generic-y += sizes.h generic-y += timex.h generic-y += trace_clock.h -generic-y += unaligned.h generated-y += mach-types.h generated-y += unistd-nr.h diff --git a/arch/arm/include/asm/unaligned.h b/arch/arm/include/asm/unaligned.h new file mode 100644 index 000000000000..ab905ffcf193 --- /dev/null +++ b/arch/arm/include/asm/unaligned.h @@ -0,0 +1,27 @@ +#ifndef __ASM_ARM_UNALIGNED_H +#define __ASM_ARM_UNALIGNED_H + +/* + * We generally want to set CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS on ARMv6+, + * but we don't want to use linux/unaligned/access_ok.h since that can lead + * to traps on unaligned stm/ldm or strd/ldrd. + */ +#include <asm/byteorder.h> + +#if defined(__LITTLE_ENDIAN) +# include <linux/unaligned/le_struct.h> +# include <linux/unaligned/be_byteshift.h> +# include <linux/unaligned/generic.h> +# define get_unaligned __get_unaligned_le +# define put_unaligned __put_unaligned_le +#elif defined(__BIG_ENDIAN) +# include <linux/unaligned/be_struct.h> +# include <linux/unaligned/le_byteshift.h> +# include <linux/unaligned/generic.h> +# define get_unaligned __get_unaligned_be +# define put_unaligned __put_unaligned_be +#else +# error need to define endianess +#endif + +#endif /* __ASM_ARM_UNALIGNED_H */
The asm-generic/unaligned.h header provides two different implementations for accessing unaligned variables: the access_ok.h version used when CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is set pretends that all pointers are in fact aligned, while the le_struct.h version convinces gcc that the alignment of a pointer is '1', to make it issue the correct load/store instructions depending on the architecture flags. On ARMv5 and older, we always use the second version, to let the compiler use byte accesses. On ARMv6 and newer, we currently use the access_ok.h version, so the compiler can use any instruction including stm/ldm and ldrd/strd that will cause an alignment trap. This trap can significantly impact performance when we have to do a lot of fixups and, worse, has led to crashes in the LZ4 decompressor code that does not have a trap handler. This adds an ARM specific version of asm/unaligned.h that uses the le_struct.h/be_struct.h implementation unconditionally. This should lead to essentially the same code on ARMv6+ as before, with the exception of using regular load/store instructions instead of the trapping instructions multi-register variants. The crash in the LZ4 decompressor code was probably introduced by the patch replacing the LZ4 implementation, commit 4e1a33b105dd ("lib: update LZ4 compressor module"), so linux-4.11 and higher would be affected most. However, we probably want to have this backported to all older stable kernels as well, to help with the performance issues. There are two follow-ups that I think we should also work on, but not backport to stable kernels, first to change the asm-generic version of the header to remove the ARM special case, and second to review all other uses of CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS to see if they might be affected by the same problem on ARM. Cc: stable@vger.kernel.org Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- Untested so far, please verify that this fixes all the known problems with the alignment traps. --- arch/arm/include/asm/Kbuild | 1 - arch/arm/include/asm/unaligned.h | 27 +++++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 arch/arm/include/asm/unaligned.h