diff mbox series

ARM: add a private asm/unaligned.h

Message ID 20171020200231.1355569-1-arnd@arndb.de
State New
Headers show
Series ARM: add a private asm/unaligned.h | expand

Commit Message

Arnd Bergmann Oct. 20, 2017, 8:01 p.m. UTC
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

-- 
2.9.0

Comments

Ard Biesheuvel Oct. 20, 2017, 8:22 p.m. UTC | #1
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

>
Romain Izard Oct. 23, 2017, 3:04 p.m. UTC | #2
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.

-- 
Romain Izard
Gregory CLEMENT Oct. 27, 2017, 3:19 p.m. UTC | #3
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

>


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
Russell King (Oracle) Oct. 27, 2017, 3:27 p.m. UTC | #4
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.

-- 
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
Gregory CLEMENT Oct. 30, 2017, 1:48 p.m. UTC | #5
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


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
Ard Biesheuvel Oct. 30, 2017, 2:55 p.m. UTC | #6
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?
Gregory CLEMENT Oct. 30, 2017, 3:05 p.m. UTC | #7
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

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
Ard Biesheuvel Oct. 30, 2017, 3:07 p.m. UTC | #8
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
Gregory CLEMENT Oct. 30, 2017, 3:09 p.m. UTC | #9
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


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
Arnd Bergmann Oct. 30, 2017, 3:09 p.m. UTC | #10
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
Ard Biesheuvel Oct. 30, 2017, 3:20 p.m. UTC | #11
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?
Gregory CLEMENT Oct. 30, 2017, 3:33 p.m. UTC | #12
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


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
Ard Biesheuvel Oct. 30, 2017, 3:35 p.m. UTC | #13
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?
Gregory CLEMENT Oct. 30, 2017, 3:40 p.m. UTC | #14
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

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
Russell King (Oracle) Oct. 30, 2017, 3:48 p.m. UTC | #15
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.

-- 
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
Russell King (Oracle) Oct. 30, 2017, 3:50 p.m. UTC | #16
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.

-- 
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
Russell King (Oracle) Oct. 30, 2017, 3:55 p.m. UTC | #17
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.

-- 
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
Gregory CLEMENT Oct. 30, 2017, 4:04 p.m. UTC | #18
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

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
Russell King (Oracle) Oct. 30, 2017, 4:12 p.m. UTC | #19
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.

-- 
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
Gregory CLEMENT Oct. 30, 2017, 4:24 p.m. UTC | #20
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


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
Russell King (Oracle) Oct. 30, 2017, 4:38 p.m. UTC | #21
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.

-- 
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
Russell King (Oracle) Oct. 30, 2017, 4:59 p.m. UTC | #22
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!

-- 
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
Arnd Bergmann Oct. 30, 2017, 5:01 p.m. UTC | #23
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
Russell King (Oracle) Oct. 30, 2017, 5:13 p.m. UTC | #24
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

-- 
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
Russell King (Oracle) Oct. 31, 2017, 12:47 p.m. UTC | #25
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.

-- 
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
Ard Biesheuvel Oct. 31, 2017, 12:57 p.m. UTC | #26
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?
Gregory CLEMENT Oct. 31, 2017, 1:22 p.m. UTC | #27
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

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
Ard Biesheuvel Nov. 1, 2017, 3:57 p.m. UTC | #28
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.
Russell King (Oracle) Nov. 1, 2017, 6 p.m. UTC | #29
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.

-- 
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
Ard Biesheuvel Nov. 1, 2017, 6:02 p.m. UTC | #30
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.
Russell King (Oracle) Nov. 1, 2017, 6:11 p.m. UTC | #31
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.

-- 
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
Ard Biesheuvel Nov. 1, 2017, 6:20 p.m. UTC | #32
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.

>
Russell King (Oracle) Nov. 1, 2017, 7:10 p.m. UTC | #33
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.

-- 
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
diff mbox series

Patch

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 */