Patchwork [U-Boot] ARM: Fix __bss_start and __bss_end in linker scripts

login
register
mail settings
Submitter Albert ARIBAUD
Date April 4, 2013, 10:13 p.m.
Message ID <1365113633-31281-1-git-send-email-albert.u.boot@aribaud.net>
Download mbox | patch
Permalink /patch/233964/
State Changes Requested
Delegated to: Albert ARIBAUD
Headers show

Comments

Albert ARIBAUD - April 4, 2013, 10:13 p.m.
Commit 3ebd1cbc introduced compiler-generated __bss_start
and __bss_end__ and commit c23561e7 rewrote all __bss_end__
as __bss_end. Their merge caused silent and harmless but
potentially bug-inducing clashes between compiler- and linker-
enerated __bss_end symbols.

Make __bss_end and __bss_start compiler-only, and create
__bss_base and __bss_limit for linker-only use.

Reported-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
---
 arch/arm/cpu/ixp/u-boot.lds        |   14 ++++++++++----
 arch/arm/cpu/u-boot.lds            |   14 ++++++++++----
 board/actux1/u-boot.lds            |   14 ++++++++++----
 board/actux2/u-boot.lds            |   14 ++++++++++----
 board/actux3/u-boot.lds            |   14 ++++++++++----
 board/dvlhost/u-boot.lds           |   14 ++++++++++----
 board/freescale/mx31ads/u-boot.lds |   14 ++++++++++----
 7 files changed, 70 insertions(+), 28 deletions(-)
Benoît Thébaudeau - April 4, 2013, 11:05 p.m.
Hi Albert,

On Friday, April 5, 2013 12:13:53 AM, Albert ARIBAUD wrote:
> Subject: [PATCH] ARM: Fix __bss_start and __bss_end in linker scripts
> 
> Commit 3ebd1cbc introduced compiler-generated __bss_start
> and __bss_end__ and commit c23561e7 rewrote all __bss_end__
> as __bss_end. Their merge caused silent and harmless but
> potentially bug-inducing clashes between compiler- and linker-
> enerated __bss_end symbols.
> 
> Make __bss_end and __bss_start compiler-only, and create
> __bss_base and __bss_limit for linker-only use.
> 
> Reported-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
> Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> ---
>  arch/arm/cpu/ixp/u-boot.lds        |   14 ++++++++++----
>  arch/arm/cpu/u-boot.lds            |   14 ++++++++++----
>  board/actux1/u-boot.lds            |   14 ++++++++++----
>  board/actux2/u-boot.lds            |   14 ++++++++++----
>  board/actux3/u-boot.lds            |   14 ++++++++++----
>  board/dvlhost/u-boot.lds           |   14 ++++++++++----
>  board/freescale/mx31ads/u-boot.lds |   14 ++++++++++----
>  7 files changed, 70 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/arm/cpu/ixp/u-boot.lds b/arch/arm/cpu/ixp/u-boot.lds
> index 8345b55..c999829 100644
> --- a/arch/arm/cpu/ixp/u-boot.lds
> +++ b/arch/arm/cpu/ixp/u-boot.lds
> @@ -67,17 +67,23 @@ SECTIONS
>  
>  	_end = .;
>  
> +/*
> + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> + * __bss_base and __bss_limit are for linker only (overlay ordering)
> + */
> +
>  	.bss_start __rel_dyn_start (OVERLAY) : {
>  		KEEP(*(.__bss_start));
> +		HIDDEN(__bss_base = .);
>  	}
>  
> -	.bss __bss_start (OVERLAY) : {
> +	.bss __bss_base (OVERLAY) : {
>  		*(.bss*)
>  		 . = ALIGN(4);
> -		 __bss_end = .;
> +		 HIDDEN(__bss_limit = .);
>  	}
> -	.bss_end __bss_end (OVERLAY) : {
> -		KEEP(*(__bss_end));
> +	.bss_end __bss_limit (OVERLAY) : {
> +		KEEP(*(.__bss_end));
>  	}
>  
>  	/DISCARD/ : { *(.dynstr*) }
> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> index 3a1083d..0543b06 100644
> --- a/arch/arm/cpu/u-boot.lds
> +++ b/arch/arm/cpu/u-boot.lds
> @@ -81,18 +81,24 @@ SECTIONS
>  		*(.mmutable)
>  	}
>  
> +/*
> + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> + * __bss_base and __bss_limit are for linker only (overlay ordering)
> + */
> +
>  	.bss_start __rel_dyn_start (OVERLAY) : {
>  		KEEP(*(.__bss_start));
> +		HIDDEN(__bss_base = .);
>  	}
>  
> -	.bss __bss_start (OVERLAY) : {
> +	.bss __bss_base (OVERLAY) : {
>  		*(.bss*)
>  		 . = ALIGN(4);
> -		 __bss_end = .;
> +		 HIDDEN(__bss_limit = .);
>  	}
>  
> -	.bss_end __bss_end (OVERLAY) : {
> -		KEEP(*(__bss_end));
> +	.bss_end __bss_limit (OVERLAY) : {
> +		KEEP(*(.__bss_end));
>  	}
>  
>  	/DISCARD/ : { *(.dynstr*) }
> diff --git a/board/actux1/u-boot.lds b/board/actux1/u-boot.lds
> index c76728a..7e5c4d8 100644
> --- a/board/actux1/u-boot.lds
> +++ b/board/actux1/u-boot.lds
> @@ -74,17 +74,23 @@ SECTIONS
>  
>  	_end = .;
>  
> +/*
> + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> + * __bss_base and __bss_limit are for linker only (overlay ordering)
> + */
> +
>  	.bss_start __rel_dyn_start (OVERLAY) : {
>  		KEEP(*(.__bss_start));
> +		HIDDEN(__bss_base = .);
>  	}
>  
> -	.bss __bss_start (OVERLAY) : {
> +	.bss __bss_base (OVERLAY) : {
>  		*(.bss*)
>  		 . = ALIGN(4);
> -		 __bss_end = .;
> +		 HIDDEN(__bss_limit = .);
>  	}
> -	.bss_end __bss_end (OVERLAY) : {
> -		KEEP(*(__bss_end));
> +	.bss_end __bss_limit (OVERLAY) : {
> +		KEEP(*(.__bss_end));
>  	}
>  
>  	/DISCARD/ : { *(.dynstr*) }
> diff --git a/board/actux2/u-boot.lds b/board/actux2/u-boot.lds
> index 984f70e..ce1b7c9 100644
> --- a/board/actux2/u-boot.lds
> +++ b/board/actux2/u-boot.lds
> @@ -74,17 +74,23 @@ SECTIONS
>  
>  	_end = .;
>  
> +/*
> + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> + * __bss_base and __bss_limit are for linker only (overlay ordering)
> + */
> +
>  	.bss_start __rel_dyn_start (OVERLAY) : {
>  		KEEP(*(.__bss_start));
> +		HIDDEN(__bss_base = .);
>  	}
>  
> -	.bss __bss_start (OVERLAY) : {
> +	.bss __bss_base (OVERLAY) : {
>  		*(.bss*)
>  		 . = ALIGN(4);
> -		 __bss_end = .;
> +		 HIDDEN(__bss_limit = .);
>  	}
> -	.bss_end __bss_end (OVERLAY) : {
> -		KEEP(*(__bss_end));
> +	.bss_end __bss_limit (OVERLAY) : {
> +		KEEP(*(.__bss_end));
>  	}
>  
>  	/DISCARD/ : { *(.dynstr*) }
> diff --git a/board/actux3/u-boot.lds b/board/actux3/u-boot.lds
> index fc48cf0..3e091dd 100644
> --- a/board/actux3/u-boot.lds
> +++ b/board/actux3/u-boot.lds
> @@ -74,17 +74,23 @@ SECTIONS
>  
>  	_end = .;
>  
> +/*
> + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> + * __bss_base and __bss_limit are for linker only (overlay ordering)
> + */
> +
>  	.bss_start __rel_dyn_start (OVERLAY) : {
>  		KEEP(*(.__bss_start));
> +		HIDDEN(__bss_base = .);
>  	}
>  
> -	.bss __bss_start (OVERLAY) : {
> +	.bss __bss_base (OVERLAY) : {
>  		*(.bss*)
>  		 . = ALIGN(4);
> -		 __bss_end = .;
> +		HIDDEN(__bss_limit = .);
>  	}
> -	.bss_end __bss_end (OVERLAY) : {
> -		KEEP(*(__bss_end));
> +	.bss_end __bss_limit (OVERLAY) : {
> +		KEEP(*(.__bss_end));
>  	}
>  
>  	/DISCARD/ : { *(.dynstr*) }
> diff --git a/board/dvlhost/u-boot.lds b/board/dvlhost/u-boot.lds
> index b13d3e1..fe3c21b 100644
> --- a/board/dvlhost/u-boot.lds
> +++ b/board/dvlhost/u-boot.lds
> @@ -74,17 +74,23 @@ SECTIONS
>  
>  	_end = .;
>  
> +/*
> + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> + * __bss_base and __bss_limit are for linker only (overlay ordering)
> + */
> +
>  	.bss_start __rel_dyn_start (OVERLAY) : {
>  		KEEP(*(.__bss_start));
> +		HIDDEN(__bss_base = .);
>  	}
>  
> -	.bss __bss_start (OVERLAY) : {
> +	.bss __bss_base (OVERLAY) : {
>  		*(.bss*)
>  		 . = ALIGN(4);
> -		 __bss_end = .;
> +		 HIDDEN(__bss_limit = .);
>  	}
> -	.bss_end __bss_end (OVERLAY) : {
> -		KEEP(*(__bss_end));
> +	.bss_end __bss_limit (OVERLAY) : {
> +		KEEP(*(.__bss_end));
>  	}
>  
>  	/DISCARD/ : { *(.dynstr*) }
> diff --git a/board/freescale/mx31ads/u-boot.lds
> b/board/freescale/mx31ads/u-boot.lds
> index 264c4e8..e6885a5 100644
> --- a/board/freescale/mx31ads/u-boot.lds
> +++ b/board/freescale/mx31ads/u-boot.lds
> @@ -80,17 +80,23 @@ SECTIONS
>  
>  	_end = .;
>  
> +/*
> + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> + * __bss_base and __bss_limit are for linker only (overlay ordering)
> + */
> +
>  	.bss_start __rel_dyn_start (OVERLAY) : {
>  		KEEP(*(.__bss_start));
> +		HIDDEN(__bss_base = .);
>  	}
>  
> -	.bss __bss_start (OVERLAY) : {
> +	.bss __bss_base (OVERLAY) : {
>  		*(.bss*)
>  		 . = ALIGN(4);
> -		 __bss_end = .;
> +		 HIDDEN(__bss_limit = .);
>  	}
> -	.bss_end __bss_end (OVERLAY) : {
> -		KEEP(*(__bss_end));
> +	.bss_end __bss_limit (OVERLAY) : {
> +		KEEP(*(.__bss_end));
>  	}
>  
>  	/DISCARD/ : { *(.bss*) }
> --
> 1.7.10.4
> 
> 

Looks good, but what about the __bss_end in
ASSERT(__bss_end < (CONFIG_SPL_TEXT_BASE + CONFIG_SPL_MAX_SIZE), "SPL image too big");
?

Shouldn't it be replaced with __bss_limit, or even better, with __image_copy_end
(why should BSS be taken into account for SPL image size?)?

Best regards,
Benoît
Albert ARIBAUD - April 4, 2013, 11:12 p.m.
Hi Albert,

On Fri,  5 Apr 2013 00:13:53 +0200, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:

> Commit 3ebd1cbc introduced compiler-generated __bss_start
> and __bss_end__ and commit c23561e7 rewrote all __bss_end__
> as __bss_end. Their merge caused silent and harmless but
> potentially bug-inducing clashes between compiler- and linker-
> enerated __bss_end symbols.
  ~~~~~~~~

make that "generated". If there is no request for changes, I'll fix
this typo directly when applying the patch.

Amicalement,
Benoît Thébaudeau - April 4, 2013, 11:13 p.m.
On Friday, April 5, 2013 1:05:53 AM, Benoît Thébaudeau wrote:
> Hi Albert,
> 
> On Friday, April 5, 2013 12:13:53 AM, Albert ARIBAUD wrote:
> > Subject: [PATCH] ARM: Fix __bss_start and __bss_end in linker scripts
> > 
> > Commit 3ebd1cbc introduced compiler-generated __bss_start
> > and __bss_end__ and commit c23561e7 rewrote all __bss_end__
> > as __bss_end. Their merge caused silent and harmless but
> > potentially bug-inducing clashes between compiler- and linker-
> > enerated __bss_end symbols.
> > 
> > Make __bss_end and __bss_start compiler-only, and create
> > __bss_base and __bss_limit for linker-only use.
> > 
> > Reported-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
> > Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> > ---
> >  arch/arm/cpu/ixp/u-boot.lds        |   14 ++++++++++----
> >  arch/arm/cpu/u-boot.lds            |   14 ++++++++++----
> >  board/actux1/u-boot.lds            |   14 ++++++++++----
> >  board/actux2/u-boot.lds            |   14 ++++++++++----
> >  board/actux3/u-boot.lds            |   14 ++++++++++----
> >  board/dvlhost/u-boot.lds           |   14 ++++++++++----
> >  board/freescale/mx31ads/u-boot.lds |   14 ++++++++++----
> >  7 files changed, 70 insertions(+), 28 deletions(-)
> > 
> > diff --git a/arch/arm/cpu/ixp/u-boot.lds b/arch/arm/cpu/ixp/u-boot.lds
> > index 8345b55..c999829 100644
> > --- a/arch/arm/cpu/ixp/u-boot.lds
> > +++ b/arch/arm/cpu/ixp/u-boot.lds
> > @@ -67,17 +67,23 @@ SECTIONS
> >  
> >  	_end = .;
> >  
> > +/*
> > + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > + */
> > +
> >  	.bss_start __rel_dyn_start (OVERLAY) : {
> >  		KEEP(*(.__bss_start));
> > +		HIDDEN(__bss_base = .);
> >  	}
> >  
> > -	.bss __bss_start (OVERLAY) : {
> > +	.bss __bss_base (OVERLAY) : {
> >  		*(.bss*)
> >  		 . = ALIGN(4);
> > -		 __bss_end = .;
> > +		 HIDDEN(__bss_limit = .);
> >  	}
> > -	.bss_end __bss_end (OVERLAY) : {
> > -		KEEP(*(__bss_end));
> > +	.bss_end __bss_limit (OVERLAY) : {
> > +		KEEP(*(.__bss_end));
> >  	}
> >  
> >  	/DISCARD/ : { *(.dynstr*) }
> > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> > index 3a1083d..0543b06 100644
> > --- a/arch/arm/cpu/u-boot.lds
> > +++ b/arch/arm/cpu/u-boot.lds
> > @@ -81,18 +81,24 @@ SECTIONS
> >  		*(.mmutable)
> >  	}
> >  
> > +/*
> > + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > + */
> > +
> >  	.bss_start __rel_dyn_start (OVERLAY) : {
> >  		KEEP(*(.__bss_start));
> > +		HIDDEN(__bss_base = .);
> >  	}
> >  
> > -	.bss __bss_start (OVERLAY) : {
> > +	.bss __bss_base (OVERLAY) : {
> >  		*(.bss*)
> >  		 . = ALIGN(4);
> > -		 __bss_end = .;
> > +		 HIDDEN(__bss_limit = .);
> >  	}
> >  
> > -	.bss_end __bss_end (OVERLAY) : {
> > -		KEEP(*(__bss_end));
> > +	.bss_end __bss_limit (OVERLAY) : {
> > +		KEEP(*(.__bss_end));
> >  	}
> >  
> >  	/DISCARD/ : { *(.dynstr*) }
> > diff --git a/board/actux1/u-boot.lds b/board/actux1/u-boot.lds
> > index c76728a..7e5c4d8 100644
> > --- a/board/actux1/u-boot.lds
> > +++ b/board/actux1/u-boot.lds
> > @@ -74,17 +74,23 @@ SECTIONS
> >  
> >  	_end = .;
> >  
> > +/*
> > + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > + */
> > +
> >  	.bss_start __rel_dyn_start (OVERLAY) : {
> >  		KEEP(*(.__bss_start));
> > +		HIDDEN(__bss_base = .);
> >  	}
> >  
> > -	.bss __bss_start (OVERLAY) : {
> > +	.bss __bss_base (OVERLAY) : {
> >  		*(.bss*)
> >  		 . = ALIGN(4);
> > -		 __bss_end = .;
> > +		 HIDDEN(__bss_limit = .);
> >  	}
> > -	.bss_end __bss_end (OVERLAY) : {
> > -		KEEP(*(__bss_end));
> > +	.bss_end __bss_limit (OVERLAY) : {
> > +		KEEP(*(.__bss_end));
> >  	}
> >  
> >  	/DISCARD/ : { *(.dynstr*) }
> > diff --git a/board/actux2/u-boot.lds b/board/actux2/u-boot.lds
> > index 984f70e..ce1b7c9 100644
> > --- a/board/actux2/u-boot.lds
> > +++ b/board/actux2/u-boot.lds
> > @@ -74,17 +74,23 @@ SECTIONS
> >  
> >  	_end = .;
> >  
> > +/*
> > + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > + */
> > +
> >  	.bss_start __rel_dyn_start (OVERLAY) : {
> >  		KEEP(*(.__bss_start));
> > +		HIDDEN(__bss_base = .);
> >  	}
> >  
> > -	.bss __bss_start (OVERLAY) : {
> > +	.bss __bss_base (OVERLAY) : {
> >  		*(.bss*)
> >  		 . = ALIGN(4);
> > -		 __bss_end = .;
> > +		 HIDDEN(__bss_limit = .);
> >  	}
> > -	.bss_end __bss_end (OVERLAY) : {
> > -		KEEP(*(__bss_end));
> > +	.bss_end __bss_limit (OVERLAY) : {
> > +		KEEP(*(.__bss_end));
> >  	}
> >  
> >  	/DISCARD/ : { *(.dynstr*) }
> > diff --git a/board/actux3/u-boot.lds b/board/actux3/u-boot.lds
> > index fc48cf0..3e091dd 100644
> > --- a/board/actux3/u-boot.lds
> > +++ b/board/actux3/u-boot.lds
> > @@ -74,17 +74,23 @@ SECTIONS
> >  
> >  	_end = .;
> >  
> > +/*
> > + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > + */
> > +
> >  	.bss_start __rel_dyn_start (OVERLAY) : {
> >  		KEEP(*(.__bss_start));
> > +		HIDDEN(__bss_base = .);
> >  	}
> >  
> > -	.bss __bss_start (OVERLAY) : {
> > +	.bss __bss_base (OVERLAY) : {
> >  		*(.bss*)
> >  		 . = ALIGN(4);
> > -		 __bss_end = .;
> > +		HIDDEN(__bss_limit = .);
> >  	}
> > -	.bss_end __bss_end (OVERLAY) : {
> > -		KEEP(*(__bss_end));
> > +	.bss_end __bss_limit (OVERLAY) : {
> > +		KEEP(*(.__bss_end));
> >  	}
> >  
> >  	/DISCARD/ : { *(.dynstr*) }
> > diff --git a/board/dvlhost/u-boot.lds b/board/dvlhost/u-boot.lds
> > index b13d3e1..fe3c21b 100644
> > --- a/board/dvlhost/u-boot.lds
> > +++ b/board/dvlhost/u-boot.lds
> > @@ -74,17 +74,23 @@ SECTIONS
> >  
> >  	_end = .;
> >  
> > +/*
> > + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > + */
> > +
> >  	.bss_start __rel_dyn_start (OVERLAY) : {
> >  		KEEP(*(.__bss_start));
> > +		HIDDEN(__bss_base = .);
> >  	}
> >  
> > -	.bss __bss_start (OVERLAY) : {
> > +	.bss __bss_base (OVERLAY) : {
> >  		*(.bss*)
> >  		 . = ALIGN(4);
> > -		 __bss_end = .;
> > +		 HIDDEN(__bss_limit = .);
> >  	}
> > -	.bss_end __bss_end (OVERLAY) : {
> > -		KEEP(*(__bss_end));
> > +	.bss_end __bss_limit (OVERLAY) : {
> > +		KEEP(*(.__bss_end));
> >  	}
> >  
> >  	/DISCARD/ : { *(.dynstr*) }
> > diff --git a/board/freescale/mx31ads/u-boot.lds
> > b/board/freescale/mx31ads/u-boot.lds
> > index 264c4e8..e6885a5 100644
> > --- a/board/freescale/mx31ads/u-boot.lds
> > +++ b/board/freescale/mx31ads/u-boot.lds
> > @@ -80,17 +80,23 @@ SECTIONS
> >  
> >  	_end = .;
> >  
> > +/*
> > + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > + */
> > +
> >  	.bss_start __rel_dyn_start (OVERLAY) : {
> >  		KEEP(*(.__bss_start));
> > +		HIDDEN(__bss_base = .);
> >  	}
> >  
> > -	.bss __bss_start (OVERLAY) : {
> > +	.bss __bss_base (OVERLAY) : {
> >  		*(.bss*)
> >  		 . = ALIGN(4);
> > -		 __bss_end = .;
> > +		 HIDDEN(__bss_limit = .);
> >  	}
> > -	.bss_end __bss_end (OVERLAY) : {
> > -		KEEP(*(__bss_end));
> > +	.bss_end __bss_limit (OVERLAY) : {
> > +		KEEP(*(.__bss_end));
> >  	}
> >  
> >  	/DISCARD/ : { *(.bss*) }
> > --
> > 1.7.10.4
> > 
> > 
> 
> Looks good, but what about the __bss_end in
> ASSERT(__bss_end < (CONFIG_SPL_TEXT_BASE + CONFIG_SPL_MAX_SIZE), "SPL image
> too big");
> ?
> 
> Shouldn't it be replaced with __bss_limit, or even better, with
> __image_copy_end
> (why should BSS be taken into account for SPL image size?)?

I meant __bss_base, not __image_copy_end.

Best regards,
Benoît
Albert ARIBAUD - April 4, 2013, 11:54 p.m.
Hi Benoît,

On Fri, 5 Apr 2013 01:13:51 +0200 (CEST), Benoît Thébaudeau
<benoit.thebaudeau@advansee.com> wrote:

> On Friday, April 5, 2013 1:05:53 AM, Benoît Thébaudeau wrote:
> > Hi Albert,
> > 
> > On Friday, April 5, 2013 12:13:53 AM, Albert ARIBAUD wrote:
> > > Subject: [PATCH] ARM: Fix __bss_start and __bss_end in linker scripts
> > > 
> > > Commit 3ebd1cbc introduced compiler-generated __bss_start
> > > and __bss_end__ and commit c23561e7 rewrote all __bss_end__
> > > as __bss_end. Their merge caused silent and harmless but
> > > potentially bug-inducing clashes between compiler- and linker-
> > > enerated __bss_end symbols.
> > > 
> > > Make __bss_end and __bss_start compiler-only, and create
> > > __bss_base and __bss_limit for linker-only use.
> > > 
> > > Reported-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
> > > Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> > > ---
> > >  arch/arm/cpu/ixp/u-boot.lds        |   14 ++++++++++----
> > >  arch/arm/cpu/u-boot.lds            |   14 ++++++++++----
> > >  board/actux1/u-boot.lds            |   14 ++++++++++----
> > >  board/actux2/u-boot.lds            |   14 ++++++++++----
> > >  board/actux3/u-boot.lds            |   14 ++++++++++----
> > >  board/dvlhost/u-boot.lds           |   14 ++++++++++----
> > >  board/freescale/mx31ads/u-boot.lds |   14 ++++++++++----
> > >  7 files changed, 70 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/arch/arm/cpu/ixp/u-boot.lds b/arch/arm/cpu/ixp/u-boot.lds
> > > index 8345b55..c999829 100644
> > > --- a/arch/arm/cpu/ixp/u-boot.lds
> > > +++ b/arch/arm/cpu/ixp/u-boot.lds
> > > @@ -67,17 +67,23 @@ SECTIONS
> > >  
> > >  	_end = .;
> > >  
> > > +/*
> > > + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> > > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > > + */
> > > +
> > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > >  		KEEP(*(.__bss_start));
> > > +		HIDDEN(__bss_base = .);
> > >  	}
> > >  
> > > -	.bss __bss_start (OVERLAY) : {
> > > +	.bss __bss_base (OVERLAY) : {
> > >  		*(.bss*)
> > >  		 . = ALIGN(4);
> > > -		 __bss_end = .;
> > > +		 HIDDEN(__bss_limit = .);
> > >  	}
> > > -	.bss_end __bss_end (OVERLAY) : {
> > > -		KEEP(*(__bss_end));
> > > +	.bss_end __bss_limit (OVERLAY) : {
> > > +		KEEP(*(.__bss_end));
> > >  	}
> > >  
> > >  	/DISCARD/ : { *(.dynstr*) }
> > > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> > > index 3a1083d..0543b06 100644
> > > --- a/arch/arm/cpu/u-boot.lds
> > > +++ b/arch/arm/cpu/u-boot.lds
> > > @@ -81,18 +81,24 @@ SECTIONS
> > >  		*(.mmutable)
> > >  	}
> > >  
> > > +/*
> > > + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> > > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > > + */
> > > +
> > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > >  		KEEP(*(.__bss_start));
> > > +		HIDDEN(__bss_base = .);
> > >  	}
> > >  
> > > -	.bss __bss_start (OVERLAY) : {
> > > +	.bss __bss_base (OVERLAY) : {
> > >  		*(.bss*)
> > >  		 . = ALIGN(4);
> > > -		 __bss_end = .;
> > > +		 HIDDEN(__bss_limit = .);
> > >  	}
> > >  
> > > -	.bss_end __bss_end (OVERLAY) : {
> > > -		KEEP(*(__bss_end));
> > > +	.bss_end __bss_limit (OVERLAY) : {
> > > +		KEEP(*(.__bss_end));
> > >  	}
> > >  
> > >  	/DISCARD/ : { *(.dynstr*) }
> > > diff --git a/board/actux1/u-boot.lds b/board/actux1/u-boot.lds
> > > index c76728a..7e5c4d8 100644
> > > --- a/board/actux1/u-boot.lds
> > > +++ b/board/actux1/u-boot.lds
> > > @@ -74,17 +74,23 @@ SECTIONS
> > >  
> > >  	_end = .;
> > >  
> > > +/*
> > > + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> > > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > > + */
> > > +
> > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > >  		KEEP(*(.__bss_start));
> > > +		HIDDEN(__bss_base = .);
> > >  	}
> > >  
> > > -	.bss __bss_start (OVERLAY) : {
> > > +	.bss __bss_base (OVERLAY) : {
> > >  		*(.bss*)
> > >  		 . = ALIGN(4);
> > > -		 __bss_end = .;
> > > +		 HIDDEN(__bss_limit = .);
> > >  	}
> > > -	.bss_end __bss_end (OVERLAY) : {
> > > -		KEEP(*(__bss_end));
> > > +	.bss_end __bss_limit (OVERLAY) : {
> > > +		KEEP(*(.__bss_end));
> > >  	}
> > >  
> > >  	/DISCARD/ : { *(.dynstr*) }
> > > diff --git a/board/actux2/u-boot.lds b/board/actux2/u-boot.lds
> > > index 984f70e..ce1b7c9 100644
> > > --- a/board/actux2/u-boot.lds
> > > +++ b/board/actux2/u-boot.lds
> > > @@ -74,17 +74,23 @@ SECTIONS
> > >  
> > >  	_end = .;
> > >  
> > > +/*
> > > + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> > > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > > + */
> > > +
> > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > >  		KEEP(*(.__bss_start));
> > > +		HIDDEN(__bss_base = .);
> > >  	}
> > >  
> > > -	.bss __bss_start (OVERLAY) : {
> > > +	.bss __bss_base (OVERLAY) : {
> > >  		*(.bss*)
> > >  		 . = ALIGN(4);
> > > -		 __bss_end = .;
> > > +		 HIDDEN(__bss_limit = .);
> > >  	}
> > > -	.bss_end __bss_end (OVERLAY) : {
> > > -		KEEP(*(__bss_end));
> > > +	.bss_end __bss_limit (OVERLAY) : {
> > > +		KEEP(*(.__bss_end));
> > >  	}
> > >  
> > >  	/DISCARD/ : { *(.dynstr*) }
> > > diff --git a/board/actux3/u-boot.lds b/board/actux3/u-boot.lds
> > > index fc48cf0..3e091dd 100644
> > > --- a/board/actux3/u-boot.lds
> > > +++ b/board/actux3/u-boot.lds
> > > @@ -74,17 +74,23 @@ SECTIONS
> > >  
> > >  	_end = .;
> > >  
> > > +/*
> > > + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> > > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > > + */
> > > +
> > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > >  		KEEP(*(.__bss_start));
> > > +		HIDDEN(__bss_base = .);
> > >  	}
> > >  
> > > -	.bss __bss_start (OVERLAY) : {
> > > +	.bss __bss_base (OVERLAY) : {
> > >  		*(.bss*)
> > >  		 . = ALIGN(4);
> > > -		 __bss_end = .;
> > > +		HIDDEN(__bss_limit = .);
> > >  	}
> > > -	.bss_end __bss_end (OVERLAY) : {
> > > -		KEEP(*(__bss_end));
> > > +	.bss_end __bss_limit (OVERLAY) : {
> > > +		KEEP(*(.__bss_end));
> > >  	}
> > >  
> > >  	/DISCARD/ : { *(.dynstr*) }
> > > diff --git a/board/dvlhost/u-boot.lds b/board/dvlhost/u-boot.lds
> > > index b13d3e1..fe3c21b 100644
> > > --- a/board/dvlhost/u-boot.lds
> > > +++ b/board/dvlhost/u-boot.lds
> > > @@ -74,17 +74,23 @@ SECTIONS
> > >  
> > >  	_end = .;
> > >  
> > > +/*
> > > + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> > > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > > + */
> > > +
> > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > >  		KEEP(*(.__bss_start));
> > > +		HIDDEN(__bss_base = .);
> > >  	}
> > >  
> > > -	.bss __bss_start (OVERLAY) : {
> > > +	.bss __bss_base (OVERLAY) : {
> > >  		*(.bss*)
> > >  		 . = ALIGN(4);
> > > -		 __bss_end = .;
> > > +		 HIDDEN(__bss_limit = .);
> > >  	}
> > > -	.bss_end __bss_end (OVERLAY) : {
> > > -		KEEP(*(__bss_end));
> > > +	.bss_end __bss_limit (OVERLAY) : {
> > > +		KEEP(*(.__bss_end));
> > >  	}
> > >  
> > >  	/DISCARD/ : { *(.dynstr*) }
> > > diff --git a/board/freescale/mx31ads/u-boot.lds
> > > b/board/freescale/mx31ads/u-boot.lds
> > > index 264c4e8..e6885a5 100644
> > > --- a/board/freescale/mx31ads/u-boot.lds
> > > +++ b/board/freescale/mx31ads/u-boot.lds
> > > @@ -80,17 +80,23 @@ SECTIONS
> > >  
> > >  	_end = .;
> > >  
> > > +/*
> > > + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> > > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > > + */
> > > +
> > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > >  		KEEP(*(.__bss_start));
> > > +		HIDDEN(__bss_base = .);
> > >  	}
> > >  
> > > -	.bss __bss_start (OVERLAY) : {
> > > +	.bss __bss_base (OVERLAY) : {
> > >  		*(.bss*)
> > >  		 . = ALIGN(4);
> > > -		 __bss_end = .;
> > > +		 HIDDEN(__bss_limit = .);
> > >  	}
> > > -	.bss_end __bss_end (OVERLAY) : {
> > > -		KEEP(*(__bss_end));
> > > +	.bss_end __bss_limit (OVERLAY) : {
> > > +		KEEP(*(.__bss_end));
> > >  	}
> > >  
> > >  	/DISCARD/ : { *(.bss*) }
> > > --
> > > 1.7.10.4
> > > 
> > > 
> > 
> > Looks good, but what about the __bss_end in
> > ASSERT(__bss_end < (CONFIG_SPL_TEXT_BASE + CONFIG_SPL_MAX_SIZE), "SPL image
> > too big");
> > ?
> > 
> > Shouldn't it be replaced with __bss_limit, or even better, with
> > __image_copy_end
> > (why should BSS be taken into account for SPL image size?)?
> 
> I meant __bss_base, not __image_copy_end.

Part of SPL can use BSS, once board_init_f() has handed things over to
board_init_r(), and this test is meant to ensure that .text+.data+.bss
fits in the RAM available to SPL. This ASSERT() compares the upper
limit of this RAM to the upper limit of the SPL needs, i.e. __bss_end.

IOW, this ASSERT is about how much memory SPL will use when it runs,
while OTOH, image_copy_end is about how much code from SPL need to
becopied from e.g. NAND to RAM to make it run.

As for bss_limit, as indicated, it is only for linker convenience, so
that overlaid sections are mapped properly; it should not be used to
signify something about the image itself)

Hope this clarifies. If it doesn't, don't hesitate to ask more
questions.

> Best regards,
> Benoît

Amicalement,
Benoît Thébaudeau - April 5, 2013, 3:44 a.m.
Hi Albert,

On Friday, April 5, 2013 1:54:38 AM, Albert ARIBAUD wrote:
> Hi Benoît,
> 
> On Fri, 5 Apr 2013 01:13:51 +0200 (CEST), Benoît Thébaudeau
> <benoit.thebaudeau@advansee.com> wrote:
> 
> > On Friday, April 5, 2013 1:05:53 AM, Benoît Thébaudeau wrote:
> > > Hi Albert,
> > > 
> > > On Friday, April 5, 2013 12:13:53 AM, Albert ARIBAUD wrote:
> > > > Subject: [PATCH] ARM: Fix __bss_start and __bss_end in linker scripts
> > > > 
> > > > Commit 3ebd1cbc introduced compiler-generated __bss_start
> > > > and __bss_end__ and commit c23561e7 rewrote all __bss_end__
> > > > as __bss_end. Their merge caused silent and harmless but
> > > > potentially bug-inducing clashes between compiler- and linker-
> > > > enerated __bss_end symbols.
> > > > 
> > > > Make __bss_end and __bss_start compiler-only, and create
> > > > __bss_base and __bss_limit for linker-only use.
> > > > 
> > > > Reported-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
> > > > Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> > > > ---
> > > >  arch/arm/cpu/ixp/u-boot.lds        |   14 ++++++++++----
> > > >  arch/arm/cpu/u-boot.lds            |   14 ++++++++++----
> > > >  board/actux1/u-boot.lds            |   14 ++++++++++----
> > > >  board/actux2/u-boot.lds            |   14 ++++++++++----
> > > >  board/actux3/u-boot.lds            |   14 ++++++++++----
> > > >  board/dvlhost/u-boot.lds           |   14 ++++++++++----
> > > >  board/freescale/mx31ads/u-boot.lds |   14 ++++++++++----
> > > >  7 files changed, 70 insertions(+), 28 deletions(-)
> > > > 
> > > > diff --git a/arch/arm/cpu/ixp/u-boot.lds b/arch/arm/cpu/ixp/u-boot.lds
> > > > index 8345b55..c999829 100644
> > > > --- a/arch/arm/cpu/ixp/u-boot.lds
> > > > +++ b/arch/arm/cpu/ixp/u-boot.lds
> > > > @@ -67,17 +67,23 @@ SECTIONS
> > > >  
> > > >  	_end = .;
> > > >  
> > > > +/*
> > > > + * Compiler-generated __bss_start and __bss_end, see
> > > > arch/arm/lib/bss.c
> > > > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > > > + */
> > > > +
> > > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > > >  		KEEP(*(.__bss_start));
> > > > +		HIDDEN(__bss_base = .);
> > > >  	}
> > > >  
> > > > -	.bss __bss_start (OVERLAY) : {
> > > > +	.bss __bss_base (OVERLAY) : {
> > > >  		*(.bss*)
> > > >  		 . = ALIGN(4);
> > > > -		 __bss_end = .;
> > > > +		 HIDDEN(__bss_limit = .);
> > > >  	}
> > > > -	.bss_end __bss_end (OVERLAY) : {
> > > > -		KEEP(*(__bss_end));
> > > > +	.bss_end __bss_limit (OVERLAY) : {
> > > > +		KEEP(*(.__bss_end));
> > > >  	}
> > > >  
> > > >  	/DISCARD/ : { *(.dynstr*) }
> > > > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> > > > index 3a1083d..0543b06 100644
> > > > --- a/arch/arm/cpu/u-boot.lds
> > > > +++ b/arch/arm/cpu/u-boot.lds
> > > > @@ -81,18 +81,24 @@ SECTIONS
> > > >  		*(.mmutable)
> > > >  	}
> > > >  
> > > > +/*
> > > > + * Compiler-generated __bss_start and __bss_end, see
> > > > arch/arm/lib/bss.c
> > > > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > > > + */
> > > > +
> > > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > > >  		KEEP(*(.__bss_start));
> > > > +		HIDDEN(__bss_base = .);
> > > >  	}
> > > >  
> > > > -	.bss __bss_start (OVERLAY) : {
> > > > +	.bss __bss_base (OVERLAY) : {
> > > >  		*(.bss*)
> > > >  		 . = ALIGN(4);
> > > > -		 __bss_end = .;
> > > > +		 HIDDEN(__bss_limit = .);
> > > >  	}
> > > >  
> > > > -	.bss_end __bss_end (OVERLAY) : {
> > > > -		KEEP(*(__bss_end));
> > > > +	.bss_end __bss_limit (OVERLAY) : {
> > > > +		KEEP(*(.__bss_end));
> > > >  	}
> > > >  
> > > >  	/DISCARD/ : { *(.dynstr*) }
> > > > diff --git a/board/actux1/u-boot.lds b/board/actux1/u-boot.lds
> > > > index c76728a..7e5c4d8 100644
> > > > --- a/board/actux1/u-boot.lds
> > > > +++ b/board/actux1/u-boot.lds
> > > > @@ -74,17 +74,23 @@ SECTIONS
> > > >  
> > > >  	_end = .;
> > > >  
> > > > +/*
> > > > + * Compiler-generated __bss_start and __bss_end, see
> > > > arch/arm/lib/bss.c
> > > > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > > > + */
> > > > +
> > > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > > >  		KEEP(*(.__bss_start));
> > > > +		HIDDEN(__bss_base = .);
> > > >  	}
> > > >  
> > > > -	.bss __bss_start (OVERLAY) : {
> > > > +	.bss __bss_base (OVERLAY) : {
> > > >  		*(.bss*)
> > > >  		 . = ALIGN(4);
> > > > -		 __bss_end = .;
> > > > +		 HIDDEN(__bss_limit = .);
> > > >  	}
> > > > -	.bss_end __bss_end (OVERLAY) : {
> > > > -		KEEP(*(__bss_end));
> > > > +	.bss_end __bss_limit (OVERLAY) : {
> > > > +		KEEP(*(.__bss_end));
> > > >  	}
> > > >  
> > > >  	/DISCARD/ : { *(.dynstr*) }
> > > > diff --git a/board/actux2/u-boot.lds b/board/actux2/u-boot.lds
> > > > index 984f70e..ce1b7c9 100644
> > > > --- a/board/actux2/u-boot.lds
> > > > +++ b/board/actux2/u-boot.lds
> > > > @@ -74,17 +74,23 @@ SECTIONS
> > > >  
> > > >  	_end = .;
> > > >  
> > > > +/*
> > > > + * Compiler-generated __bss_start and __bss_end, see
> > > > arch/arm/lib/bss.c
> > > > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > > > + */
> > > > +
> > > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > > >  		KEEP(*(.__bss_start));
> > > > +		HIDDEN(__bss_base = .);
> > > >  	}
> > > >  
> > > > -	.bss __bss_start (OVERLAY) : {
> > > > +	.bss __bss_base (OVERLAY) : {
> > > >  		*(.bss*)
> > > >  		 . = ALIGN(4);
> > > > -		 __bss_end = .;
> > > > +		 HIDDEN(__bss_limit = .);
> > > >  	}
> > > > -	.bss_end __bss_end (OVERLAY) : {
> > > > -		KEEP(*(__bss_end));
> > > > +	.bss_end __bss_limit (OVERLAY) : {
> > > > +		KEEP(*(.__bss_end));
> > > >  	}
> > > >  
> > > >  	/DISCARD/ : { *(.dynstr*) }
> > > > diff --git a/board/actux3/u-boot.lds b/board/actux3/u-boot.lds
> > > > index fc48cf0..3e091dd 100644
> > > > --- a/board/actux3/u-boot.lds
> > > > +++ b/board/actux3/u-boot.lds
> > > > @@ -74,17 +74,23 @@ SECTIONS
> > > >  
> > > >  	_end = .;
> > > >  
> > > > +/*
> > > > + * Compiler-generated __bss_start and __bss_end, see
> > > > arch/arm/lib/bss.c
> > > > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > > > + */
> > > > +
> > > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > > >  		KEEP(*(.__bss_start));
> > > > +		HIDDEN(__bss_base = .);
> > > >  	}
> > > >  
> > > > -	.bss __bss_start (OVERLAY) : {
> > > > +	.bss __bss_base (OVERLAY) : {
> > > >  		*(.bss*)
> > > >  		 . = ALIGN(4);
> > > > -		 __bss_end = .;
> > > > +		HIDDEN(__bss_limit = .);
> > > >  	}
> > > > -	.bss_end __bss_end (OVERLAY) : {
> > > > -		KEEP(*(__bss_end));
> > > > +	.bss_end __bss_limit (OVERLAY) : {
> > > > +		KEEP(*(.__bss_end));
> > > >  	}
> > > >  
> > > >  	/DISCARD/ : { *(.dynstr*) }
> > > > diff --git a/board/dvlhost/u-boot.lds b/board/dvlhost/u-boot.lds
> > > > index b13d3e1..fe3c21b 100644
> > > > --- a/board/dvlhost/u-boot.lds
> > > > +++ b/board/dvlhost/u-boot.lds
> > > > @@ -74,17 +74,23 @@ SECTIONS
> > > >  
> > > >  	_end = .;
> > > >  
> > > > +/*
> > > > + * Compiler-generated __bss_start and __bss_end, see
> > > > arch/arm/lib/bss.c
> > > > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > > > + */
> > > > +
> > > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > > >  		KEEP(*(.__bss_start));
> > > > +		HIDDEN(__bss_base = .);
> > > >  	}
> > > >  
> > > > -	.bss __bss_start (OVERLAY) : {
> > > > +	.bss __bss_base (OVERLAY) : {
> > > >  		*(.bss*)
> > > >  		 . = ALIGN(4);
> > > > -		 __bss_end = .;
> > > > +		 HIDDEN(__bss_limit = .);
> > > >  	}
> > > > -	.bss_end __bss_end (OVERLAY) : {
> > > > -		KEEP(*(__bss_end));
> > > > +	.bss_end __bss_limit (OVERLAY) : {
> > > > +		KEEP(*(.__bss_end));
> > > >  	}
> > > >  
> > > >  	/DISCARD/ : { *(.dynstr*) }
> > > > diff --git a/board/freescale/mx31ads/u-boot.lds
> > > > b/board/freescale/mx31ads/u-boot.lds
> > > > index 264c4e8..e6885a5 100644
> > > > --- a/board/freescale/mx31ads/u-boot.lds
> > > > +++ b/board/freescale/mx31ads/u-boot.lds
> > > > @@ -80,17 +80,23 @@ SECTIONS
> > > >  
> > > >  	_end = .;
> > > >  
> > > > +/*
> > > > + * Compiler-generated __bss_start and __bss_end, see
> > > > arch/arm/lib/bss.c
> > > > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > > > + */
> > > > +
> > > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > > >  		KEEP(*(.__bss_start));
> > > > +		HIDDEN(__bss_base = .);
> > > >  	}
> > > >  
> > > > -	.bss __bss_start (OVERLAY) : {
> > > > +	.bss __bss_base (OVERLAY) : {
> > > >  		*(.bss*)
> > > >  		 . = ALIGN(4);
> > > > -		 __bss_end = .;
> > > > +		 HIDDEN(__bss_limit = .);
> > > >  	}
> > > > -	.bss_end __bss_end (OVERLAY) : {
> > > > -		KEEP(*(__bss_end));
> > > > +	.bss_end __bss_limit (OVERLAY) : {
> > > > +		KEEP(*(.__bss_end));
> > > >  	}
> > > >  
> > > >  	/DISCARD/ : { *(.bss*) }
> > > > --
> > > > 1.7.10.4
> > > > 
> > > > 
> > > 
> > > Looks good, but what about the __bss_end in
> > > ASSERT(__bss_end < (CONFIG_SPL_TEXT_BASE + CONFIG_SPL_MAX_SIZE), "SPL
> > > image
> > > too big");
> > > ?
> > > 
> > > Shouldn't it be replaced with __bss_limit, or even better, with
> > > __image_copy_end
> > > (why should BSS be taken into account for SPL image size?)?
> > 
> > I meant __bss_base, not __image_copy_end.
> 
> Part of SPL can use BSS, once board_init_f() has handed things over to
> board_init_r(),

I agree with that.

> and this test is meant to ensure that .text+.data+.bss
> fits in the RAM available to SPL. This ASSERT() compares the upper
> limit of this RAM to the upper limit of the SPL needs, i.e. __bss_end.
> 
> IOW, this ASSERT is about how much memory SPL will use when it runs,

This is where there might be an issue with the definition / usage of
CONFIG_SPL_MAX_SIZE.

I had understood that this is the purpose of this assert, but this usage of
CONFIG_SPL_MAX_SIZE, while frequent on arm, conflicts with its definition in
README, as well as with some arm and other usages, e.g. in
include/configs/am335x_evm.h or include/configs/p1_p2_rdb_pc.h.

The README's definition is also how I understood this config when discussing it
with Scott about my 16/30, which led to the #error test that was added at some
point to this patch.

According to README, CONFIG_SPL_MAX_SIZE is for text + data + rodata (i.e. for
the size of the binary image file to be programmed on the target device), while
CONFIG_SPL_BSS_MAX_SIZE is dedicated to the BSS. Hence, in order to comply with
this definition, that assert should either use __bss_base/__bss_start, or
compare against CONFIG_SPL_MAX_SIZE + CONFIG_SPL_BSS_MAX_SIZE, depending on
whether it's supposed to test the binary image size or the memory footprint.
There could even be asserts testing both. This would anyway be a new patch
separate from this one.

If you decided not to change that, CONFIG_SPL_MAX_SIZE would have different
meanings depending on the board, and CONFIG_SPL_PAD_TO could be smaller than it
in some cases, so the #error test in my 16/30 could be removed, but no such case
exists so far, making this 16/30 change not strictly necessary for now.

> while OTOH, image_copy_end is about how much code from SPL need to
> becopied from e.g. NAND to RAM to make it run.

Right.

> As for bss_limit, as indicated, it is only for linker convenience, so
> that overlaid sections are mapped properly; it should not be used to
> signify something about the image itself)

OK, I'm fine with that. They have the same value, so it's only a matter of
taste. I spotted that just because you said __bss_limit is for linker stuff, and
it is an assert for the linker, but __bss_end is just fine too, all the more the
resulting assert is then identical to the one in arch/arm/cpu/u-boot-spl.lds.

Best regards,
Benoît
Albert ARIBAUD - April 5, 2013, 6 a.m.
Hi Benoît,

CC:ing Stephen Warren who wrote commit 2b7818d4 which git blame tells
me added the ASSERT() to arch/arm/cpu/u-boot.lds, and Tom Rini to help
decide what to do.

On Fri, 5 Apr 2013 05:44:55 +0200 (CEST), Benoît Thébaudeau
<benoit.thebaudeau@advansee.com> wrote:

> Hi Albert,
> 
> On Friday, April 5, 2013 1:54:38 AM, Albert ARIBAUD wrote:
> > Hi Benoît,
> > 
> > On Fri, 5 Apr 2013 01:13:51 +0200 (CEST), Benoît Thébaudeau
> > <benoit.thebaudeau@advansee.com> wrote:
> > 
> > > On Friday, April 5, 2013 1:05:53 AM, Benoît Thébaudeau wrote:
> > > > Hi Albert,
> > > > 
> > > > On Friday, April 5, 2013 12:13:53 AM, Albert ARIBAUD wrote:
> > > > > Subject: [PATCH] ARM: Fix __bss_start and __bss_end in linker scripts
> > > > > 
> > > > > Commit 3ebd1cbc introduced compiler-generated __bss_start
> > > > > and __bss_end__ and commit c23561e7 rewrote all __bss_end__
> > > > > as __bss_end. Their merge caused silent and harmless but
> > > > > potentially bug-inducing clashes between compiler- and linker-
> > > > > enerated __bss_end symbols.
> > > > > 
> > > > > Make __bss_end and __bss_start compiler-only, and create
> > > > > __bss_base and __bss_limit for linker-only use.
> > > > > 
> > > > > Reported-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
> > > > > Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> > > > > ---
> > > > >  arch/arm/cpu/ixp/u-boot.lds        |   14 ++++++++++----
> > > > >  arch/arm/cpu/u-boot.lds            |   14 ++++++++++----
> > > > >  board/actux1/u-boot.lds            |   14 ++++++++++----
> > > > >  board/actux2/u-boot.lds            |   14 ++++++++++----
> > > > >  board/actux3/u-boot.lds            |   14 ++++++++++----
> > > > >  board/dvlhost/u-boot.lds           |   14 ++++++++++----
> > > > >  board/freescale/mx31ads/u-boot.lds |   14 ++++++++++----
> > > > >  7 files changed, 70 insertions(+), 28 deletions(-)
> > > > > 
> > > > > diff --git a/arch/arm/cpu/ixp/u-boot.lds b/arch/arm/cpu/ixp/u-boot.lds
> > > > > index 8345b55..c999829 100644
> > > > > --- a/arch/arm/cpu/ixp/u-boot.lds
> > > > > +++ b/arch/arm/cpu/ixp/u-boot.lds
> > > > > @@ -67,17 +67,23 @@ SECTIONS
> > > > >  
> > > > >  	_end = .;
> > > > >  
> > > > > +/*
> > > > > + * Compiler-generated __bss_start and __bss_end, see
> > > > > arch/arm/lib/bss.c
> > > > > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > > > > + */
> > > > > +
> > > > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > > > >  		KEEP(*(.__bss_start));
> > > > > +		HIDDEN(__bss_base = .);
> > > > >  	}
> > > > >  
> > > > > -	.bss __bss_start (OVERLAY) : {
> > > > > +	.bss __bss_base (OVERLAY) : {
> > > > >  		*(.bss*)
> > > > >  		 . = ALIGN(4);
> > > > > -		 __bss_end = .;
> > > > > +		 HIDDEN(__bss_limit = .);
> > > > >  	}
> > > > > -	.bss_end __bss_end (OVERLAY) : {
> > > > > -		KEEP(*(__bss_end));
> > > > > +	.bss_end __bss_limit (OVERLAY) : {
> > > > > +		KEEP(*(.__bss_end));
> > > > >  	}
> > > > >  
> > > > >  	/DISCARD/ : { *(.dynstr*) }
> > > > > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> > > > > index 3a1083d..0543b06 100644
> > > > > --- a/arch/arm/cpu/u-boot.lds
> > > > > +++ b/arch/arm/cpu/u-boot.lds
> > > > > @@ -81,18 +81,24 @@ SECTIONS
> > > > >  		*(.mmutable)
> > > > >  	}
> > > > >  
> > > > > +/*
> > > > > + * Compiler-generated __bss_start and __bss_end, see
> > > > > arch/arm/lib/bss.c
> > > > > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > > > > + */
> > > > > +
> > > > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > > > >  		KEEP(*(.__bss_start));
> > > > > +		HIDDEN(__bss_base = .);
> > > > >  	}
> > > > >  
> > > > > -	.bss __bss_start (OVERLAY) : {
> > > > > +	.bss __bss_base (OVERLAY) : {
> > > > >  		*(.bss*)
> > > > >  		 . = ALIGN(4);
> > > > > -		 __bss_end = .;
> > > > > +		 HIDDEN(__bss_limit = .);
> > > > >  	}
> > > > >  
> > > > > -	.bss_end __bss_end (OVERLAY) : {
> > > > > -		KEEP(*(__bss_end));
> > > > > +	.bss_end __bss_limit (OVERLAY) : {
> > > > > +		KEEP(*(.__bss_end));
> > > > >  	}
> > > > >  
> > > > >  	/DISCARD/ : { *(.dynstr*) }
> > > > > diff --git a/board/actux1/u-boot.lds b/board/actux1/u-boot.lds
> > > > > index c76728a..7e5c4d8 100644
> > > > > --- a/board/actux1/u-boot.lds
> > > > > +++ b/board/actux1/u-boot.lds
> > > > > @@ -74,17 +74,23 @@ SECTIONS
> > > > >  
> > > > >  	_end = .;
> > > > >  
> > > > > +/*
> > > > > + * Compiler-generated __bss_start and __bss_end, see
> > > > > arch/arm/lib/bss.c
> > > > > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > > > > + */
> > > > > +
> > > > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > > > >  		KEEP(*(.__bss_start));
> > > > > +		HIDDEN(__bss_base = .);
> > > > >  	}
> > > > >  
> > > > > -	.bss __bss_start (OVERLAY) : {
> > > > > +	.bss __bss_base (OVERLAY) : {
> > > > >  		*(.bss*)
> > > > >  		 . = ALIGN(4);
> > > > > -		 __bss_end = .;
> > > > > +		 HIDDEN(__bss_limit = .);
> > > > >  	}
> > > > > -	.bss_end __bss_end (OVERLAY) : {
> > > > > -		KEEP(*(__bss_end));
> > > > > +	.bss_end __bss_limit (OVERLAY) : {
> > > > > +		KEEP(*(.__bss_end));
> > > > >  	}
> > > > >  
> > > > >  	/DISCARD/ : { *(.dynstr*) }
> > > > > diff --git a/board/actux2/u-boot.lds b/board/actux2/u-boot.lds
> > > > > index 984f70e..ce1b7c9 100644
> > > > > --- a/board/actux2/u-boot.lds
> > > > > +++ b/board/actux2/u-boot.lds
> > > > > @@ -74,17 +74,23 @@ SECTIONS
> > > > >  
> > > > >  	_end = .;
> > > > >  
> > > > > +/*
> > > > > + * Compiler-generated __bss_start and __bss_end, see
> > > > > arch/arm/lib/bss.c
> > > > > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > > > > + */
> > > > > +
> > > > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > > > >  		KEEP(*(.__bss_start));
> > > > > +		HIDDEN(__bss_base = .);
> > > > >  	}
> > > > >  
> > > > > -	.bss __bss_start (OVERLAY) : {
> > > > > +	.bss __bss_base (OVERLAY) : {
> > > > >  		*(.bss*)
> > > > >  		 . = ALIGN(4);
> > > > > -		 __bss_end = .;
> > > > > +		 HIDDEN(__bss_limit = .);
> > > > >  	}
> > > > > -	.bss_end __bss_end (OVERLAY) : {
> > > > > -		KEEP(*(__bss_end));
> > > > > +	.bss_end __bss_limit (OVERLAY) : {
> > > > > +		KEEP(*(.__bss_end));
> > > > >  	}
> > > > >  
> > > > >  	/DISCARD/ : { *(.dynstr*) }
> > > > > diff --git a/board/actux3/u-boot.lds b/board/actux3/u-boot.lds
> > > > > index fc48cf0..3e091dd 100644
> > > > > --- a/board/actux3/u-boot.lds
> > > > > +++ b/board/actux3/u-boot.lds
> > > > > @@ -74,17 +74,23 @@ SECTIONS
> > > > >  
> > > > >  	_end = .;
> > > > >  
> > > > > +/*
> > > > > + * Compiler-generated __bss_start and __bss_end, see
> > > > > arch/arm/lib/bss.c
> > > > > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > > > > + */
> > > > > +
> > > > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > > > >  		KEEP(*(.__bss_start));
> > > > > +		HIDDEN(__bss_base = .);
> > > > >  	}
> > > > >  
> > > > > -	.bss __bss_start (OVERLAY) : {
> > > > > +	.bss __bss_base (OVERLAY) : {
> > > > >  		*(.bss*)
> > > > >  		 . = ALIGN(4);
> > > > > -		 __bss_end = .;
> > > > > +		HIDDEN(__bss_limit = .);
> > > > >  	}
> > > > > -	.bss_end __bss_end (OVERLAY) : {
> > > > > -		KEEP(*(__bss_end));
> > > > > +	.bss_end __bss_limit (OVERLAY) : {
> > > > > +		KEEP(*(.__bss_end));
> > > > >  	}
> > > > >  
> > > > >  	/DISCARD/ : { *(.dynstr*) }
> > > > > diff --git a/board/dvlhost/u-boot.lds b/board/dvlhost/u-boot.lds
> > > > > index b13d3e1..fe3c21b 100644
> > > > > --- a/board/dvlhost/u-boot.lds
> > > > > +++ b/board/dvlhost/u-boot.lds
> > > > > @@ -74,17 +74,23 @@ SECTIONS
> > > > >  
> > > > >  	_end = .;
> > > > >  
> > > > > +/*
> > > > > + * Compiler-generated __bss_start and __bss_end, see
> > > > > arch/arm/lib/bss.c
> > > > > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > > > > + */
> > > > > +
> > > > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > > > >  		KEEP(*(.__bss_start));
> > > > > +		HIDDEN(__bss_base = .);
> > > > >  	}
> > > > >  
> > > > > -	.bss __bss_start (OVERLAY) : {
> > > > > +	.bss __bss_base (OVERLAY) : {
> > > > >  		*(.bss*)
> > > > >  		 . = ALIGN(4);
> > > > > -		 __bss_end = .;
> > > > > +		 HIDDEN(__bss_limit = .);
> > > > >  	}
> > > > > -	.bss_end __bss_end (OVERLAY) : {
> > > > > -		KEEP(*(__bss_end));
> > > > > +	.bss_end __bss_limit (OVERLAY) : {
> > > > > +		KEEP(*(.__bss_end));
> > > > >  	}
> > > > >  
> > > > >  	/DISCARD/ : { *(.dynstr*) }
> > > > > diff --git a/board/freescale/mx31ads/u-boot.lds
> > > > > b/board/freescale/mx31ads/u-boot.lds
> > > > > index 264c4e8..e6885a5 100644
> > > > > --- a/board/freescale/mx31ads/u-boot.lds
> > > > > +++ b/board/freescale/mx31ads/u-boot.lds
> > > > > @@ -80,17 +80,23 @@ SECTIONS
> > > > >  
> > > > >  	_end = .;
> > > > >  
> > > > > +/*
> > > > > + * Compiler-generated __bss_start and __bss_end, see
> > > > > arch/arm/lib/bss.c
> > > > > + * __bss_base and __bss_limit are for linker only (overlay ordering)
> > > > > + */
> > > > > +
> > > > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > > > >  		KEEP(*(.__bss_start));
> > > > > +		HIDDEN(__bss_base = .);
> > > > >  	}
> > > > >  
> > > > > -	.bss __bss_start (OVERLAY) : {
> > > > > +	.bss __bss_base (OVERLAY) : {
> > > > >  		*(.bss*)
> > > > >  		 . = ALIGN(4);
> > > > > -		 __bss_end = .;
> > > > > +		 HIDDEN(__bss_limit = .);
> > > > >  	}
> > > > > -	.bss_end __bss_end (OVERLAY) : {
> > > > > -		KEEP(*(__bss_end));
> > > > > +	.bss_end __bss_limit (OVERLAY) : {
> > > > > +		KEEP(*(.__bss_end));
> > > > >  	}
> > > > >  
> > > > >  	/DISCARD/ : { *(.bss*) }
> > > > > --
> > > > > 1.7.10.4
> > > > > 
> > > > > 
> > > > 
> > > > Looks good, but what about the __bss_end in
> > > > ASSERT(__bss_end < (CONFIG_SPL_TEXT_BASE + CONFIG_SPL_MAX_SIZE), "SPL
> > > > image
> > > > too big");
> > > > ?
> > > > 
> > > > Shouldn't it be replaced with __bss_limit, or even better, with
> > > > __image_copy_end
> > > > (why should BSS be taken into account for SPL image size?)?
> > > 
> > > I meant __bss_base, not __image_copy_end.
> > 
> > Part of SPL can use BSS, once board_init_f() has handed things over to
> > board_init_r(),
> 
> I agree with that.
> 
> > and this test is meant to ensure that .text+.data+.bss
> > fits in the RAM available to SPL. This ASSERT() compares the upper
> > limit of this RAM to the upper limit of the SPL needs, i.e. __bss_end.
> > 
> > IOW, this ASSERT is about how much memory SPL will use when it runs,
> 
> This is where there might be an issue with the definition / usage of
> CONFIG_SPL_MAX_SIZE.
> 
> I had understood that this is the purpose of this assert, but this usage of
> CONFIG_SPL_MAX_SIZE, while frequent on arm, conflicts with its definition in
> README, as well as with some arm and other usages, e.g. in
> include/configs/am335x_evm.h or include/configs/p1_p2_rdb_pc.h.
> 
> The README's definition is also how I understood this config when discussing it
> with Scott about my 16/30, which led to the #error test that was added at some
> point to this patch.

Sorry, I'd missed that. Adding Stephen for any comment regarding the
ASSERT() and CONFIG_SPL_MAX_SIZE semantics.

> According to README, CONFIG_SPL_MAX_SIZE is for text + data + rodata (i.e. for
> the size of the binary image file to be programmed on the target device), while
> CONFIG_SPL_BSS_MAX_SIZE is dedicated to the BSS. Hence, in order to comply with
> this definition, that assert should either use __bss_base/__bss_start, or
> compare against CONFIG_SPL_MAX_SIZE + CONFIG_SPL_BSS_MAX_SIZE, depending on
> whether it's supposed to test the binary image size or the memory footprint.
> There could even be asserts testing both. This would anyway be a new patch
> separate from this one.

It would indeed be a different patch.

IIUC, this future patch would increase the limit for SPL run-time size,
as the constant against which the ASS tests __bss_end for would
necessarily be greater than it is now. Correct? If so, this future
patch should not break any target, as it would loosen the constraint,
not tighten it.

Tom, would such a patch, if posted today, be acceptable for 2013.04?

> If you decided not to change that, CONFIG_SPL_MAX_SIZE would have different
> meanings depending on the board, and CONFIG_SPL_PAD_TO could be smaller than it
> in some cases, so the #error test in my 16/30 could be removed, but no such case
> exists so far, making this 16/30 change not strictly necessary for now.

IF a patch is posted, it should be done above your series, and take
care of fixing 16/30 as necessary.

Now, pending Tom's approval, the question is: who does this future
patch?

Benoît, You obviously have most of the necessary inputs, but I might
be more available to prepare a patch now. Your pick -- and in any case,
you earn a second Reported-By: badge. :)

> Best regards,
> Benoît

Amicalement,
Tom Rini - April 5, 2013, 1:53 p.m.
On Fri, Apr 05, 2013 at 08:00:43AM +0200, Albert ARIBAUD wrote:
> Hi Beno??t,
> 
> CC:ing Stephen Warren who wrote commit 2b7818d4 which git blame tells
> me added the ASSERT() to arch/arm/cpu/u-boot.lds, and Tom Rini to help
> decide what to do.
[snip]
> > > > > Looks good, but what about the __bss_end in
> > > > > ASSERT(__bss_end < (CONFIG_SPL_TEXT_BASE + CONFIG_SPL_MAX_SIZE), "SPL
> > > > > image
> > > > > too big");
> > > > > ?
> > > > > 
> > > > > Shouldn't it be replaced with __bss_limit, or even better, with
> > > > > __image_copy_end
> > > > > (why should BSS be taken into account for SPL image size?)?
> > > > 
> > > > I meant __bss_base, not __image_copy_end.
> > > 
> > > Part of SPL can use BSS, once board_init_f() has handed things over to
> > > board_init_r(),
> > 
> > I agree with that.
> > 
> > > and this test is meant to ensure that .text+.data+.bss
> > > fits in the RAM available to SPL. This ASSERT() compares the upper
> > > limit of this RAM to the upper limit of the SPL needs, i.e. __bss_end.
> > > 
> > > IOW, this ASSERT is about how much memory SPL will use when it runs,
> > 
> > This is where there might be an issue with the definition / usage of
> > CONFIG_SPL_MAX_SIZE.
> > 
> > I had understood that this is the purpose of this assert, but this usage of
> > CONFIG_SPL_MAX_SIZE, while frequent on arm, conflicts with its definition in
> > README, as well as with some arm and other usages, e.g. in
> > include/configs/am335x_evm.h or include/configs/p1_p2_rdb_pc.h.
> > 
> > The README's definition is also how I understood this config when discussing it
> > with Scott about my 16/30, which led to the #error test that was added at some
> > point to this patch.
> 
> Sorry, I'd missed that. Adding Stephen for any comment regarding the
> ASSERT() and CONFIG_SPL_MAX_SIZE semantics.

Yes.  I'm a little confused about this one as well now.

> > According to README, CONFIG_SPL_MAX_SIZE is for text + data + rodata (i.e. for
> > the size of the binary image file to be programmed on the target device), while
> > CONFIG_SPL_BSS_MAX_SIZE is dedicated to the BSS. Hence, in order to comply with
> > this definition, that assert should either use __bss_base/__bss_start, or
> > compare against CONFIG_SPL_MAX_SIZE + CONFIG_SPL_BSS_MAX_SIZE, depending on
> > whether it's supposed to test the binary image size or the memory footprint.
> > There could even be asserts testing both. This would anyway be a new patch
> > separate from this one.
> 
> It would indeed be a different patch.
> 
> IIUC, this future patch would increase the limit for SPL run-time size,
> as the constant against which the ASS tests __bss_end for would
> necessarily be greater than it is now. Correct? If so, this future
> patch should not break any target, as it would loosen the constraint,
> not tighten it.
> 
> Tom, would such a patch, if posted today, be acceptable for 2013.04?

So, lets talk things out.  On (setting aside davinci) TI platforms,
SPL's text/data/rodata is sitting in SRAM (starting at 0x4...) and we
say BSS shall live in DDR (starting at 0x8...) and we configure DDR
before clearing and using BSS.  The "max" we set is mostly about trying
to not possibly step on ourselves later on (SPL malloc, full U-Boot,
etc).

If I read all of the tegra files correctly, the way they work is that
SPL and U-Boot are both loaded into memory, and thus they're ensuring
that the BSS won't overlap with the full U-Boot that's right behind.

So yes, a patch to correct the behavior is fine, and we can build test
that easily enough as well.
Benoît Thébaudeau - April 5, 2013, 1:56 p.m.
Hi Albert,

On Friday, April 5, 2013 8:00:43 AM, Albert ARIBAUD wrote:
> Hi Benoît,
> 
> CC:ing Stephen Warren who wrote commit 2b7818d4 which git blame tells
> me added the ASSERT() to arch/arm/cpu/u-boot.lds, and Tom Rini to help
> decide what to do.
> 
> On Fri, 5 Apr 2013 05:44:55 +0200 (CEST), Benoît Thébaudeau
> <benoit.thebaudeau@advansee.com> wrote:
> 
> > Hi Albert,
> > 
> > On Friday, April 5, 2013 1:54:38 AM, Albert ARIBAUD wrote:
> > > Hi Benoît,
> > > 
> > > On Fri, 5 Apr 2013 01:13:51 +0200 (CEST), Benoît Thébaudeau
> > > <benoit.thebaudeau@advansee.com> wrote:
> > > 
> > > > On Friday, April 5, 2013 1:05:53 AM, Benoît Thébaudeau wrote:
> > > > > Hi Albert,
> > > > > 
> > > > > On Friday, April 5, 2013 12:13:53 AM, Albert ARIBAUD wrote:
> > > > > > Subject: [PATCH] ARM: Fix __bss_start and __bss_end in linker
> > > > > > scripts
> > > > > > 
> > > > > > Commit 3ebd1cbc introduced compiler-generated __bss_start
> > > > > > and __bss_end__ and commit c23561e7 rewrote all __bss_end__
> > > > > > as __bss_end. Their merge caused silent and harmless but
> > > > > > potentially bug-inducing clashes between compiler- and linker-
> > > > > > enerated __bss_end symbols.
> > > > > > 
> > > > > > Make __bss_end and __bss_start compiler-only, and create
> > > > > > __bss_base and __bss_limit for linker-only use.
> > > > > > 
> > > > > > Reported-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
> > > > > > Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> > > > > > ---
> > > > > >  arch/arm/cpu/ixp/u-boot.lds        |   14 ++++++++++----
> > > > > >  arch/arm/cpu/u-boot.lds            |   14 ++++++++++----
> > > > > >  board/actux1/u-boot.lds            |   14 ++++++++++----
> > > > > >  board/actux2/u-boot.lds            |   14 ++++++++++----
> > > > > >  board/actux3/u-boot.lds            |   14 ++++++++++----
> > > > > >  board/dvlhost/u-boot.lds           |   14 ++++++++++----
> > > > > >  board/freescale/mx31ads/u-boot.lds |   14 ++++++++++----
> > > > > >  7 files changed, 70 insertions(+), 28 deletions(-)
> > > > > > 
> > > > > > diff --git a/arch/arm/cpu/ixp/u-boot.lds
> > > > > > b/arch/arm/cpu/ixp/u-boot.lds
> > > > > > index 8345b55..c999829 100644
> > > > > > --- a/arch/arm/cpu/ixp/u-boot.lds
> > > > > > +++ b/arch/arm/cpu/ixp/u-boot.lds
> > > > > > @@ -67,17 +67,23 @@ SECTIONS
> > > > > >  
> > > > > >  	_end = .;
> > > > > >  
> > > > > > +/*
> > > > > > + * Compiler-generated __bss_start and __bss_end, see
> > > > > > arch/arm/lib/bss.c
> > > > > > + * __bss_base and __bss_limit are for linker only (overlay
> > > > > > ordering)
> > > > > > + */
> > > > > > +
> > > > > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > > > > >  		KEEP(*(.__bss_start));
> > > > > > +		HIDDEN(__bss_base = .);
> > > > > >  	}
> > > > > >  
> > > > > > -	.bss __bss_start (OVERLAY) : {
> > > > > > +	.bss __bss_base (OVERLAY) : {
> > > > > >  		*(.bss*)
> > > > > >  		 . = ALIGN(4);
> > > > > > -		 __bss_end = .;
> > > > > > +		 HIDDEN(__bss_limit = .);
> > > > > >  	}
> > > > > > -	.bss_end __bss_end (OVERLAY) : {
> > > > > > -		KEEP(*(__bss_end));
> > > > > > +	.bss_end __bss_limit (OVERLAY) : {
> > > > > > +		KEEP(*(.__bss_end));
> > > > > >  	}
> > > > > >  
> > > > > >  	/DISCARD/ : { *(.dynstr*) }
> > > > > > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> > > > > > index 3a1083d..0543b06 100644
> > > > > > --- a/arch/arm/cpu/u-boot.lds
> > > > > > +++ b/arch/arm/cpu/u-boot.lds
> > > > > > @@ -81,18 +81,24 @@ SECTIONS
> > > > > >  		*(.mmutable)
> > > > > >  	}
> > > > > >  
> > > > > > +/*
> > > > > > + * Compiler-generated __bss_start and __bss_end, see
> > > > > > arch/arm/lib/bss.c
> > > > > > + * __bss_base and __bss_limit are for linker only (overlay
> > > > > > ordering)
> > > > > > + */
> > > > > > +
> > > > > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > > > > >  		KEEP(*(.__bss_start));
> > > > > > +		HIDDEN(__bss_base = .);
> > > > > >  	}
> > > > > >  
> > > > > > -	.bss __bss_start (OVERLAY) : {
> > > > > > +	.bss __bss_base (OVERLAY) : {
> > > > > >  		*(.bss*)
> > > > > >  		 . = ALIGN(4);
> > > > > > -		 __bss_end = .;
> > > > > > +		 HIDDEN(__bss_limit = .);
> > > > > >  	}
> > > > > >  
> > > > > > -	.bss_end __bss_end (OVERLAY) : {
> > > > > > -		KEEP(*(__bss_end));
> > > > > > +	.bss_end __bss_limit (OVERLAY) : {
> > > > > > +		KEEP(*(.__bss_end));
> > > > > >  	}
> > > > > >  
> > > > > >  	/DISCARD/ : { *(.dynstr*) }
> > > > > > diff --git a/board/actux1/u-boot.lds b/board/actux1/u-boot.lds
> > > > > > index c76728a..7e5c4d8 100644
> > > > > > --- a/board/actux1/u-boot.lds
> > > > > > +++ b/board/actux1/u-boot.lds
> > > > > > @@ -74,17 +74,23 @@ SECTIONS
> > > > > >  
> > > > > >  	_end = .;
> > > > > >  
> > > > > > +/*
> > > > > > + * Compiler-generated __bss_start and __bss_end, see
> > > > > > arch/arm/lib/bss.c
> > > > > > + * __bss_base and __bss_limit are for linker only (overlay
> > > > > > ordering)
> > > > > > + */
> > > > > > +
> > > > > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > > > > >  		KEEP(*(.__bss_start));
> > > > > > +		HIDDEN(__bss_base = .);
> > > > > >  	}
> > > > > >  
> > > > > > -	.bss __bss_start (OVERLAY) : {
> > > > > > +	.bss __bss_base (OVERLAY) : {
> > > > > >  		*(.bss*)
> > > > > >  		 . = ALIGN(4);
> > > > > > -		 __bss_end = .;
> > > > > > +		 HIDDEN(__bss_limit = .);
> > > > > >  	}
> > > > > > -	.bss_end __bss_end (OVERLAY) : {
> > > > > > -		KEEP(*(__bss_end));
> > > > > > +	.bss_end __bss_limit (OVERLAY) : {
> > > > > > +		KEEP(*(.__bss_end));
> > > > > >  	}
> > > > > >  
> > > > > >  	/DISCARD/ : { *(.dynstr*) }
> > > > > > diff --git a/board/actux2/u-boot.lds b/board/actux2/u-boot.lds
> > > > > > index 984f70e..ce1b7c9 100644
> > > > > > --- a/board/actux2/u-boot.lds
> > > > > > +++ b/board/actux2/u-boot.lds
> > > > > > @@ -74,17 +74,23 @@ SECTIONS
> > > > > >  
> > > > > >  	_end = .;
> > > > > >  
> > > > > > +/*
> > > > > > + * Compiler-generated __bss_start and __bss_end, see
> > > > > > arch/arm/lib/bss.c
> > > > > > + * __bss_base and __bss_limit are for linker only (overlay
> > > > > > ordering)
> > > > > > + */
> > > > > > +
> > > > > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > > > > >  		KEEP(*(.__bss_start));
> > > > > > +		HIDDEN(__bss_base = .);
> > > > > >  	}
> > > > > >  
> > > > > > -	.bss __bss_start (OVERLAY) : {
> > > > > > +	.bss __bss_base (OVERLAY) : {
> > > > > >  		*(.bss*)
> > > > > >  		 . = ALIGN(4);
> > > > > > -		 __bss_end = .;
> > > > > > +		 HIDDEN(__bss_limit = .);
> > > > > >  	}
> > > > > > -	.bss_end __bss_end (OVERLAY) : {
> > > > > > -		KEEP(*(__bss_end));
> > > > > > +	.bss_end __bss_limit (OVERLAY) : {
> > > > > > +		KEEP(*(.__bss_end));
> > > > > >  	}
> > > > > >  
> > > > > >  	/DISCARD/ : { *(.dynstr*) }
> > > > > > diff --git a/board/actux3/u-boot.lds b/board/actux3/u-boot.lds
> > > > > > index fc48cf0..3e091dd 100644
> > > > > > --- a/board/actux3/u-boot.lds
> > > > > > +++ b/board/actux3/u-boot.lds
> > > > > > @@ -74,17 +74,23 @@ SECTIONS
> > > > > >  
> > > > > >  	_end = .;
> > > > > >  
> > > > > > +/*
> > > > > > + * Compiler-generated __bss_start and __bss_end, see
> > > > > > arch/arm/lib/bss.c
> > > > > > + * __bss_base and __bss_limit are for linker only (overlay
> > > > > > ordering)
> > > > > > + */
> > > > > > +
> > > > > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > > > > >  		KEEP(*(.__bss_start));
> > > > > > +		HIDDEN(__bss_base = .);
> > > > > >  	}
> > > > > >  
> > > > > > -	.bss __bss_start (OVERLAY) : {
> > > > > > +	.bss __bss_base (OVERLAY) : {
> > > > > >  		*(.bss*)
> > > > > >  		 . = ALIGN(4);
> > > > > > -		 __bss_end = .;
> > > > > > +		HIDDEN(__bss_limit = .);
> > > > > >  	}
> > > > > > -	.bss_end __bss_end (OVERLAY) : {
> > > > > > -		KEEP(*(__bss_end));
> > > > > > +	.bss_end __bss_limit (OVERLAY) : {
> > > > > > +		KEEP(*(.__bss_end));
> > > > > >  	}
> > > > > >  
> > > > > >  	/DISCARD/ : { *(.dynstr*) }
> > > > > > diff --git a/board/dvlhost/u-boot.lds b/board/dvlhost/u-boot.lds
> > > > > > index b13d3e1..fe3c21b 100644
> > > > > > --- a/board/dvlhost/u-boot.lds
> > > > > > +++ b/board/dvlhost/u-boot.lds
> > > > > > @@ -74,17 +74,23 @@ SECTIONS
> > > > > >  
> > > > > >  	_end = .;
> > > > > >  
> > > > > > +/*
> > > > > > + * Compiler-generated __bss_start and __bss_end, see
> > > > > > arch/arm/lib/bss.c
> > > > > > + * __bss_base and __bss_limit are for linker only (overlay
> > > > > > ordering)
> > > > > > + */
> > > > > > +
> > > > > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > > > > >  		KEEP(*(.__bss_start));
> > > > > > +		HIDDEN(__bss_base = .);
> > > > > >  	}
> > > > > >  
> > > > > > -	.bss __bss_start (OVERLAY) : {
> > > > > > +	.bss __bss_base (OVERLAY) : {
> > > > > >  		*(.bss*)
> > > > > >  		 . = ALIGN(4);
> > > > > > -		 __bss_end = .;
> > > > > > +		 HIDDEN(__bss_limit = .);
> > > > > >  	}
> > > > > > -	.bss_end __bss_end (OVERLAY) : {
> > > > > > -		KEEP(*(__bss_end));
> > > > > > +	.bss_end __bss_limit (OVERLAY) : {
> > > > > > +		KEEP(*(.__bss_end));
> > > > > >  	}
> > > > > >  
> > > > > >  	/DISCARD/ : { *(.dynstr*) }
> > > > > > diff --git a/board/freescale/mx31ads/u-boot.lds
> > > > > > b/board/freescale/mx31ads/u-boot.lds
> > > > > > index 264c4e8..e6885a5 100644
> > > > > > --- a/board/freescale/mx31ads/u-boot.lds
> > > > > > +++ b/board/freescale/mx31ads/u-boot.lds
> > > > > > @@ -80,17 +80,23 @@ SECTIONS
> > > > > >  
> > > > > >  	_end = .;
> > > > > >  
> > > > > > +/*
> > > > > > + * Compiler-generated __bss_start and __bss_end, see
> > > > > > arch/arm/lib/bss.c
> > > > > > + * __bss_base and __bss_limit are for linker only (overlay
> > > > > > ordering)
> > > > > > + */
> > > > > > +
> > > > > >  	.bss_start __rel_dyn_start (OVERLAY) : {
> > > > > >  		KEEP(*(.__bss_start));
> > > > > > +		HIDDEN(__bss_base = .);
> > > > > >  	}
> > > > > >  
> > > > > > -	.bss __bss_start (OVERLAY) : {
> > > > > > +	.bss __bss_base (OVERLAY) : {
> > > > > >  		*(.bss*)
> > > > > >  		 . = ALIGN(4);
> > > > > > -		 __bss_end = .;
> > > > > > +		 HIDDEN(__bss_limit = .);
> > > > > >  	}
> > > > > > -	.bss_end __bss_end (OVERLAY) : {
> > > > > > -		KEEP(*(__bss_end));
> > > > > > +	.bss_end __bss_limit (OVERLAY) : {
> > > > > > +		KEEP(*(.__bss_end));
> > > > > >  	}
> > > > > >  
> > > > > >  	/DISCARD/ : { *(.bss*) }
> > > > > > --
> > > > > > 1.7.10.4
> > > > > > 
> > > > > > 
> > > > > 
> > > > > Looks good, but what about the __bss_end in
> > > > > ASSERT(__bss_end < (CONFIG_SPL_TEXT_BASE + CONFIG_SPL_MAX_SIZE), "SPL
> > > > > image
> > > > > too big");
> > > > > ?
> > > > > 
> > > > > Shouldn't it be replaced with __bss_limit, or even better, with
> > > > > __image_copy_end
> > > > > (why should BSS be taken into account for SPL image size?)?
> > > > 
> > > > I meant __bss_base, not __image_copy_end.
> > > 
> > > Part of SPL can use BSS, once board_init_f() has handed things over to
> > > board_init_r(),
> > 
> > I agree with that.
> > 
> > > and this test is meant to ensure that .text+.data+.bss
> > > fits in the RAM available to SPL. This ASSERT() compares the upper
> > > limit of this RAM to the upper limit of the SPL needs, i.e. __bss_end.
> > > 
> > > IOW, this ASSERT is about how much memory SPL will use when it runs,
> > 
> > This is where there might be an issue with the definition / usage of
> > CONFIG_SPL_MAX_SIZE.
> > 
> > I had understood that this is the purpose of this assert, but this usage of
> > CONFIG_SPL_MAX_SIZE, while frequent on arm, conflicts with its definition
> > in
> > README, as well as with some arm and other usages, e.g. in
> > include/configs/am335x_evm.h or include/configs/p1_p2_rdb_pc.h.

It's not even that frequent on ARM. I've looked at a few other ARM linker
scripts (arm1136, omap), and they don't count bss in CONFIG_SPL_MAX_SIZE.

> > The README's definition is also how I understood this config when
> > discussing it
> > with Scott about my 16/30, which led to the #error test that was added at
> > some
> > point to this patch.
> 
> Sorry, I'd missed that. Adding Stephen for any comment regarding the
> ASSERT() and CONFIG_SPL_MAX_SIZE semantics.
> 
> > According to README, CONFIG_SPL_MAX_SIZE is for text + data + rodata (i.e.
> > for
> > the size of the binary image file to be programmed on the target device),
> > while
> > CONFIG_SPL_BSS_MAX_SIZE is dedicated to the BSS. Hence, in order to comply
> > with
> > this definition, that assert should either use __bss_base/__bss_start, or
> > compare against CONFIG_SPL_MAX_SIZE + CONFIG_SPL_BSS_MAX_SIZE, depending on
> > whether it's supposed to test the binary image size or the memory
> > footprint.
> > There could even be asserts testing both. This would anyway be a new patch
> > separate from this one.
> 
> It would indeed be a different patch.
> 
> IIUC, this future patch would increase the limit for SPL run-time size,
> as the constant against which the ASS tests __bss_end for would
> necessarily be greater than it is now. Correct? If so, this future
> patch should not break any target, as it would loosen the constraint,
> not tighten it.

Yes, it would either be the same or relaxed a bit, depending on the chosen
option:
 - Define CONFIG_SPL_BSS_MAX_SIZE and test against CONFIG_SPL_MAX_SIZE +
   CONFIG_SPL_BSS_MAX_SIZE, the sum remaining the same as or being larger than
   currently, depending on the new values for CONFIG_SPL_MAX_SIZE and
   CONFIG_SPL_BSS_MAX_SIZE.
 - Define a new config meaning text + data + rodata + bss (e.g.
   CONFIG_SPL_MAX_RAM_SIZE or CONFIG_SPL_MAX_MEM_FOOTPRINT), and just replace
   CONFIG_SPL_MAX_SIZE with it for the users of arch/arm/cpu/u-boot*.lds, taking
   care that this was the only meaning those users were giving to
   CONFIG_SPL_MAX_SIZE.

The first option would probably be preferable, using the same value for
CONFIG_SPL_MAX_SIZE, and a non-zero value for CONFIG_SPL_BSS_MAX_SIZE.

> Tom, would such a patch, if posted today, be acceptable for 2013.04?
> 
> > If you decided not to change that, CONFIG_SPL_MAX_SIZE would have different
> > meanings depending on the board, and CONFIG_SPL_PAD_TO could be smaller
> > than it
> > in some cases, so the #error test in my 16/30 could be removed, but no such
> > case
> > exists so far, making this 16/30 change not strictly necessary for now.
> 
> IF a patch is posted, it should be done above your series, and take
> care of fixing 16/30 as necessary.

If this assert is fixed according to the README definition, nothing needs to be
changed in 16/30.

> Now, pending Tom's approval, the question is: who does this future
> patch?
> 
> Benoît, You obviously have most of the necessary inputs, but I might
> be more available to prepare a patch now. Your pick -- and in any case,
> you earn a second Reported-By: badge. :)

I let you handle this, and approved for my RB badge. :)

Best regards,
Benoît
Stephen Warren - April 5, 2013, 3:59 p.m.
On 04/05/2013 07:53 AM, Tom Rini wrote:
> On Fri, Apr 05, 2013 at 08:00:43AM +0200, Albert ARIBAUD wrote:
>> Hi Beno??t,
>> 
>> CC:ing Stephen Warren who wrote commit 2b7818d4 which git blame
>> tells me added the ASSERT() to arch/arm/cpu/u-boot.lds, and Tom
>> Rini to help decide what to do.
> [snip]
>>>>>> Looks good, but what about the __bss_end in 
>>>>>> ASSERT(__bss_end < (CONFIG_SPL_TEXT_BASE +
>>>>>> CONFIG_SPL_MAX_SIZE), "SPL image too big"); ?
>>>>>> 
>>>>>> Shouldn't it be replaced with __bss_limit, or even
>>>>>> better, with __image_copy_end (why should BSS be taken
>>>>>> into account for SPL image size?)?
>>>>> 
>>>>> I meant __bss_base, not __image_copy_end.
>>>> 
>>>> Part of SPL can use BSS, once board_init_f() has handed
>>>> things over to board_init_r(),
>>> 
>>> I agree with that.
>>> 
>>>> and this test is meant to ensure that .text+.data+.bss fits
>>>> in the RAM available to SPL. This ASSERT() compares the
>>>> upper limit of this RAM to the upper limit of the SPL needs,
>>>> i.e. __bss_end.
>>>> 
>>>> IOW, this ASSERT is about how much memory SPL will use when
>>>> it runs,
>>> 
>>> This is where there might be an issue with the definition /
>>> usage of CONFIG_SPL_MAX_SIZE.
>>> 
>>> I had understood that this is the purpose of this assert, but
>>> this usage of CONFIG_SPL_MAX_SIZE, while frequent on arm,
>>> conflicts with its definition in README, as well as with some
>>> arm and other usages, e.g. in include/configs/am335x_evm.h or
>>> include/configs/p1_p2_rdb_pc.h.
>>> 
>>> The README's definition is also how I understood this config
>>> when discussing it with Scott about my 16/30, which led to the
>>> #error test that was added at some point to this patch.
>> 
>> Sorry, I'd missed that. Adding Stephen for any comment regarding
>> the ASSERT() and CONFIG_SPL_MAX_SIZE semantics.
> 
> Yes.  I'm a little confused about this one as well now.
...
> If I read all of the tegra files correctly, the way they work is
> that SPL and U-Boot are both loaded into memory, and thus they're
> ensuring that the BSS won't overlap with the full U-Boot that's
> right behind.

Yes exactly.

I'll explain the whole situation just so there's something to refer to
in the archives.

Tegra has two different types of CPU. As far as boot process goes, the
AVP (Audio/Video Processor) is what is released from reset at system
boot time, and what runs the built-in Tegra boot ROM and the U-Boot
SPL. The AVP is an ARM7TDMI. The SPL initializes the main CPU complex
(A9/A15), and causes it to execute the main U-Boot.

The boot process is:

* Entire U-Boot binary (SPL+pad+U-Boot) sits in boot flash.
* Boot ROM code running on AVP:
** Initializes DRAM controller.
** Copies entire SPL+pad+U-Boot to DRAM byte-for-byte.
** Jumps to U-Boot SPL.
* U-Boot SPL running on AVP:
** Initializes clocks/... required to boot A9 CPUs.
** Set up the A9 reset vector to point at the main U-Boot code,
   and releases A9 CPUs from reset.
* Main U-Boot running on A9:
** Runs in the typical fashion, including regular relocation.

Thus, the in-flash layout of SPL+U-Boot must match the in-DRAM layout,
since the boot ROM just copies it byte-for-byte, and the SPL does
nothing to move the appended main U-Boot out-of-the-way before executing.

The gap between SPL and concatenated U-Boot must be large enough to
hold the BSS (since SPL will use BSS without moving U-boot), but is
also larger so that we can hard-code the TEXT_BASE of the main U-Boot
to some stable value, without having to move it about often if SPL
changes size.

So the U-Boot binary layout in flash/DRAM is:

+---------------------+
| SPL                 |
+---------------------+
| Pad, including      |
| space for BSS       |
+---------------------+
| Main U-Boot         |
+---------------------+

The assert in question ensures that the start of the "Main U-Boot" in
the diagram above is at a large enough address that it doesn't overlap
the SPL's BSS usage in RAM.

It's possible that I chose the wrong defines to validate when writing
that assert. As long as the assert continues to ensure the above image
layout in the generated u-boot*.bin, I have no problems with it being
changed.
Tom Rini - April 5, 2013, 4 p.m.
On Fri, Apr 05, 2013 at 03:56:46PM +0200, Beno??t Th??baudeau wrote:
> Hi Albert,
> 
> On Friday, April 5, 2013 8:00:43 AM, Albert ARIBAUD wrote:
> > Hi Beno??t,
[snip]
> > IIUC, this future patch would increase the limit for SPL run-time size,
> > as the constant against which the ASS tests __bss_end for would
> > necessarily be greater than it is now. Correct? If so, this future
> > patch should not break any target, as it would loosen the constraint,
> > not tighten it.
> 
> Yes, it would either be the same or relaxed a bit, depending on the chosen
> option:
>  - Define CONFIG_SPL_BSS_MAX_SIZE and test against CONFIG_SPL_MAX_SIZE +
>    CONFIG_SPL_BSS_MAX_SIZE, the sum remaining the same as or being larger than
>    currently, depending on the new values for CONFIG_SPL_MAX_SIZE and
>    CONFIG_SPL_BSS_MAX_SIZE.
>  - Define a new config meaning text + data + rodata + bss (e.g.
>    CONFIG_SPL_MAX_RAM_SIZE or CONFIG_SPL_MAX_MEM_FOOTPRINT), and just replace
>    CONFIG_SPL_MAX_SIZE with it for the users of arch/arm/cpu/u-boot*.lds, taking
>    care that this was the only meaning those users were giving to
>    CONFIG_SPL_MAX_SIZE.
> 
> The first option would probably be preferable, using the same value for
> CONFIG_SPL_MAX_SIZE, and a non-zero value for CONFIG_SPL_BSS_MAX_SIZE.

I think the problem is that Tegra really needs the second case as their
constraint is "must fit below next part of payload".  We can assume the
users of that linker script today care about footprint and update their
define I believe.  da850evm and the rest of the davinci platforms would
also be a case to convert to this, but the omap*/am3* platforms would
not.
Benoît Thébaudeau - April 5, 2013, 5:32 p.m.
Hi Tom,

On Friday, April 5, 2013 6:00:30 PM, Tom Rini wrote:
> On Fri, Apr 05, 2013 at 03:56:46PM +0200, Beno??t Th??baudeau wrote:
> > Hi Albert,
> > 
> > On Friday, April 5, 2013 8:00:43 AM, Albert ARIBAUD wrote:
> > > Hi Beno??t,
> [snip]
> > > IIUC, this future patch would increase the limit for SPL run-time size,
> > > as the constant against which the ASS tests __bss_end for would
> > > necessarily be greater than it is now. Correct? If so, this future
> > > patch should not break any target, as it would loosen the constraint,
> > > not tighten it.
> > 
> > Yes, it would either be the same or relaxed a bit, depending on the chosen
> > option:
> >  - Define CONFIG_SPL_BSS_MAX_SIZE and test against CONFIG_SPL_MAX_SIZE +
> >    CONFIG_SPL_BSS_MAX_SIZE, the sum remaining the same as or being larger
> >    than
> >    currently, depending on the new values for CONFIG_SPL_MAX_SIZE and
> >    CONFIG_SPL_BSS_MAX_SIZE.
> >  - Define a new config meaning text + data + rodata + bss (e.g.
> >    CONFIG_SPL_MAX_RAM_SIZE or CONFIG_SPL_MAX_MEM_FOOTPRINT), and just
> >    replace
> >    CONFIG_SPL_MAX_SIZE with it for the users of arch/arm/cpu/u-boot*.lds,
> >    taking
> >    care that this was the only meaning those users were giving to
> >    CONFIG_SPL_MAX_SIZE.
> > 
> > The first option would probably be preferable, using the same value for
> > CONFIG_SPL_MAX_SIZE, and a non-zero value for CONFIG_SPL_BSS_MAX_SIZE.
> 
> I think the problem is that Tegra really needs the second case as their
> constraint is "must fit below next part of payload".  We can assume the
> users of that linker script today care about footprint and update their
> define I believe.  da850evm and the rest of the davinci platforms would
> also be a case to convert to this, but the omap*/am3* platforms would
> not.

Yes, then let's have an assert in arch/arm/cpu/u-boot*.lds with a different
config name (as in option 2 above) just for Tegra, and another assert for
CONFIG_SPL_MAX_SIZE against __bss_start.

And all users of CONFIG_SPL_MAX_SIZE should be checked to make sure that there
is not another special case somewhere.

Best regards,
Benoît
Tom Rini - April 5, 2013, 5:55 p.m.
On Fri, Apr 05, 2013 at 07:32:54PM +0200, Beno??t Th??baudeau wrote:
> Hi Tom,
> 
> On Friday, April 5, 2013 6:00:30 PM, Tom Rini wrote:
> > On Fri, Apr 05, 2013 at 03:56:46PM +0200, Beno??t Th??baudeau wrote:
> > > Hi Albert,
> > > 
> > > On Friday, April 5, 2013 8:00:43 AM, Albert ARIBAUD wrote:
> > > > Hi Beno??t,
> > [snip]
> > > > IIUC, this future patch would increase the limit for SPL run-time size,
> > > > as the constant against which the ASS tests __bss_end for would
> > > > necessarily be greater than it is now. Correct? If so, this future
> > > > patch should not break any target, as it would loosen the constraint,
> > > > not tighten it.
> > > 
> > > Yes, it would either be the same or relaxed a bit, depending on the chosen
> > > option:
> > >  - Define CONFIG_SPL_BSS_MAX_SIZE and test against CONFIG_SPL_MAX_SIZE +
> > >    CONFIG_SPL_BSS_MAX_SIZE, the sum remaining the same as or being larger
> > >    than
> > >    currently, depending on the new values for CONFIG_SPL_MAX_SIZE and
> > >    CONFIG_SPL_BSS_MAX_SIZE.
> > >  - Define a new config meaning text + data + rodata + bss (e.g.
> > >    CONFIG_SPL_MAX_RAM_SIZE or CONFIG_SPL_MAX_MEM_FOOTPRINT), and just
> > >    replace
> > >    CONFIG_SPL_MAX_SIZE with it for the users of arch/arm/cpu/u-boot*.lds,
> > >    taking
> > >    care that this was the only meaning those users were giving to
> > >    CONFIG_SPL_MAX_SIZE.
> > > 
> > > The first option would probably be preferable, using the same value for
> > > CONFIG_SPL_MAX_SIZE, and a non-zero value for CONFIG_SPL_BSS_MAX_SIZE.
> > 
> > I think the problem is that Tegra really needs the second case as their
> > constraint is "must fit below next part of payload".  We can assume the
> > users of that linker script today care about footprint and update their
> > define I believe.  da850evm and the rest of the davinci platforms would
> > also be a case to convert to this, but the omap*/am3* platforms would
> > not.
> 
> Yes, then let's have an assert in arch/arm/cpu/u-boot*.lds with a
> different config name (as in option 2 above) just for Tegra, and
> another assert for CONFIG_SPL_MAX_SIZE against __bss_start.
> 
> And all users of CONFIG_SPL_MAX_SIZE should be checked to make sure
> that there is not another special case somewhere.

I didn't audit the PowerPC targets, but on ARM we have, roughly:
- Tegra (covered in Stephen's email, and in short, must include BSS in
  size check) which uses SPL_MAX_SIZE to include BSS
- OMAP*/AM3* which does not constrain BSS to SPL_MAX_SIZE
- DaVinci which must also constrain BSS to the initial RAM, but for
  different reasons.
- iMX which also uses SPL_BSS_MAX to cover the BSS separate from the
  rest of the program.
Albert ARIBAUD - April 5, 2013, 7:17 p.m.
Hi Tom,

On Fri, 5 Apr 2013 13:55:21 -0400, Tom Rini <trini@ti.com> wrote:

> On Fri, Apr 05, 2013 at 07:32:54PM +0200, Beno??t Th??baudeau wrote:
> > Hi Tom,
> > 
> > On Friday, April 5, 2013 6:00:30 PM, Tom Rini wrote:
> > > On Fri, Apr 05, 2013 at 03:56:46PM +0200, Beno??t Th??baudeau wrote:
> > > > Hi Albert,
> > > > 
> > > > On Friday, April 5, 2013 8:00:43 AM, Albert ARIBAUD wrote:
> > > > > Hi Beno??t,
> > > [snip]
> > > > > IIUC, this future patch would increase the limit for SPL run-time size,
> > > > > as the constant against which the ASS tests __bss_end for would
> > > > > necessarily be greater than it is now. Correct? If so, this future
> > > > > patch should not break any target, as it would loosen the constraint,
> > > > > not tighten it.
> > > > 
> > > > Yes, it would either be the same or relaxed a bit, depending on the chosen
> > > > option:
> > > >  - Define CONFIG_SPL_BSS_MAX_SIZE and test against CONFIG_SPL_MAX_SIZE +
> > > >    CONFIG_SPL_BSS_MAX_SIZE, the sum remaining the same as or being larger
> > > >    than
> > > >    currently, depending on the new values for CONFIG_SPL_MAX_SIZE and
> > > >    CONFIG_SPL_BSS_MAX_SIZE.
> > > >  - Define a new config meaning text + data + rodata + bss (e.g.
> > > >    CONFIG_SPL_MAX_RAM_SIZE or CONFIG_SPL_MAX_MEM_FOOTPRINT), and just
> > > >    replace
> > > >    CONFIG_SPL_MAX_SIZE with it for the users of arch/arm/cpu/u-boot*.lds,
> > > >    taking
> > > >    care that this was the only meaning those users were giving to
> > > >    CONFIG_SPL_MAX_SIZE.
> > > > 
> > > > The first option would probably be preferable, using the same value for
> > > > CONFIG_SPL_MAX_SIZE, and a non-zero value for CONFIG_SPL_BSS_MAX_SIZE.
> > > 
> > > I think the problem is that Tegra really needs the second case as their
> > > constraint is "must fit below next part of payload".  We can assume the
> > > users of that linker script today care about footprint and update their
> > > define I believe.  da850evm and the rest of the davinci platforms would
> > > also be a case to convert to this, but the omap*/am3* platforms would
> > > not.
> > 
> > Yes, then let's have an assert in arch/arm/cpu/u-boot*.lds with a
> > different config name (as in option 2 above) just for Tegra, and
> > another assert for CONFIG_SPL_MAX_SIZE against __bss_start.
> > 
> > And all users of CONFIG_SPL_MAX_SIZE should be checked to make sure
> > that there is not another special case somewhere.
> 
> I didn't audit the PowerPC targets, but on ARM we have, roughly:
> - Tegra (covered in Stephen's email, and in short, must include BSS in
>   size check) which uses SPL_MAX_SIZE to include BSS
> - OMAP*/AM3* which does not constrain BSS to SPL_MAX_SIZE
> - DaVinci which must also constrain BSS to the initial RAM, but for
>   different reasons.
> - iMX which also uses SPL_BSS_MAX to cover the BSS separate from the
>   rest of the program.

How about this?

1. In the u-boot*.lds files, doing separate asserts for SPL and SPL BSS
   max size, with the SPL assert being further divided in two cases
   depending on BSS max size being defined or not:

   #if defined(CONFIG_SPL_MAX_SIZE)
   #if   defined(CONFIG_SPL_BSS_MAX_SIZE)
   ASSERT( __bss_end - __image_copy_start < (CONFIG_SPL_TEXT_BASE + \
       CONFIG_SPL_MAX_SIZE), "SPL image code+BSS too big");
   #else
   ASSERT( __bss_end - __image_copy_start < CONFIG_SPL_TEXT_BASE, \
       CONFIG_SPL_MAX_SIZE), "SPL image code too big");
   #endif

   #if defined(CONFIG_SPL_BSS_MAX_SIZE)
   ASSERT( __bss_end - __bss_start < CONFIG_SPL_TEXT_BASE, \
       CONFIG_SPL_BSS_MAX_SIZE), "SPL image BSS too big");
   #endif

2. Defining CONFIG_SPL_BSS_MAX_SIZE only for Tegra, Davinci, IMX (where
   CONFIG_SPL_BSS_MAX_SIZE is actually the gap size)

3. *Not* defining CONFIG_SPL_MAX_SIZE or CONFIG_SPL_BSS_MAX_SIZE for
   OMAP*/AM3*

4. Adjusting README descriptions of CONFIG_SPL_[BSS_]MAX_SIZE and 
   ensuring Makefile uses the right size for --pad-to, as well as
   the few other files which use CONFIG_SPL_MAX_SIZE.

Amicalement,
Albert ARIBAUD - April 5, 2013, 7:28 p.m.
On Fri, 5 Apr 2013 21:17:40 +0200, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:

> Hi Tom,
> 
> On Fri, 5 Apr 2013 13:55:21 -0400, Tom Rini <trini@ti.com> wrote:
> 
> > On Fri, Apr 05, 2013 at 07:32:54PM +0200, Beno??t Th??baudeau wrote:
> > > Hi Tom,
> > > 
> > > On Friday, April 5, 2013 6:00:30 PM, Tom Rini wrote:
> > > > On Fri, Apr 05, 2013 at 03:56:46PM +0200, Beno??t Th??baudeau wrote:
> > > > > Hi Albert,
> > > > > 
> > > > > On Friday, April 5, 2013 8:00:43 AM, Albert ARIBAUD wrote:
> > > > > > Hi Beno??t,
> > > > [snip]
> > > > > > IIUC, this future patch would increase the limit for SPL run-time size,
> > > > > > as the constant against which the ASS tests __bss_end for would
> > > > > > necessarily be greater than it is now. Correct? If so, this future
> > > > > > patch should not break any target, as it would loosen the constraint,
> > > > > > not tighten it.
> > > > > 
> > > > > Yes, it would either be the same or relaxed a bit, depending on the chosen
> > > > > option:
> > > > >  - Define CONFIG_SPL_BSS_MAX_SIZE and test against CONFIG_SPL_MAX_SIZE +
> > > > >    CONFIG_SPL_BSS_MAX_SIZE, the sum remaining the same as or being larger
> > > > >    than
> > > > >    currently, depending on the new values for CONFIG_SPL_MAX_SIZE and
> > > > >    CONFIG_SPL_BSS_MAX_SIZE.
> > > > >  - Define a new config meaning text + data + rodata + bss (e.g.
> > > > >    CONFIG_SPL_MAX_RAM_SIZE or CONFIG_SPL_MAX_MEM_FOOTPRINT), and just
> > > > >    replace
> > > > >    CONFIG_SPL_MAX_SIZE with it for the users of arch/arm/cpu/u-boot*.lds,
> > > > >    taking
> > > > >    care that this was the only meaning those users were giving to
> > > > >    CONFIG_SPL_MAX_SIZE.
> > > > > 
> > > > > The first option would probably be preferable, using the same value for
> > > > > CONFIG_SPL_MAX_SIZE, and a non-zero value for CONFIG_SPL_BSS_MAX_SIZE.
> > > > 
> > > > I think the problem is that Tegra really needs the second case as their
> > > > constraint is "must fit below next part of payload".  We can assume the
> > > > users of that linker script today care about footprint and update their
> > > > define I believe.  da850evm and the rest of the davinci platforms would
> > > > also be a case to convert to this, but the omap*/am3* platforms would
> > > > not.
> > > 
> > > Yes, then let's have an assert in arch/arm/cpu/u-boot*.lds with a
> > > different config name (as in option 2 above) just for Tegra, and
> > > another assert for CONFIG_SPL_MAX_SIZE against __bss_start.
> > > 
> > > And all users of CONFIG_SPL_MAX_SIZE should be checked to make sure
> > > that there is not another special case somewhere.
> > 
> > I didn't audit the PowerPC targets, but on ARM we have, roughly:
> > - Tegra (covered in Stephen's email, and in short, must include BSS in
> >   size check) which uses SPL_MAX_SIZE to include BSS
> > - OMAP*/AM3* which does not constrain BSS to SPL_MAX_SIZE
> > - DaVinci which must also constrain BSS to the initial RAM, but for
> >   different reasons.
> > - iMX which also uses SPL_BSS_MAX to cover the BSS separate from the
> >   rest of the program.
> 
> How about this?
> 
> 1. In the u-boot*.lds files, doing separate asserts for SPL and SPL BSS
>    max size, with the SPL assert being further divided in two cases
>    depending on BSS max size being defined or not:
> 
>    #if defined(CONFIG_SPL_MAX_SIZE)
>    #if   defined(CONFIG_SPL_BSS_MAX_SIZE)
>    ASSERT( __bss_end - __image_copy_start < (CONFIG_SPL_TEXT_BASE + \
>        CONFIG_SPL_MAX_SIZE), "SPL image code+BSS too big");
>    #else
>    ASSERT( __bss_end - __image_copy_start < CONFIG_SPL_TEXT_BASE, \
>        CONFIG_SPL_MAX_SIZE), "SPL image code too big");
>    #endif
     #endif

(sorry)

>    #if defined(CONFIG_SPL_BSS_MAX_SIZE)
>    ASSERT( __bss_end - __bss_start < CONFIG_SPL_TEXT_BASE, \
>        CONFIG_SPL_BSS_MAX_SIZE), "SPL image BSS too big");
>    #endif
> 
> 2. Defining CONFIG_SPL_BSS_MAX_SIZE only for Tegra, Davinci, IMX (where
>    CONFIG_SPL_BSS_MAX_SIZE is actually the gap size)
> 
> 3. *Not* defining CONFIG_SPL_MAX_SIZE or CONFIG_SPL_BSS_MAX_SIZE for
>    OMAP*/AM3*
> 
> 4. Adjusting README descriptions of CONFIG_SPL_[BSS_]MAX_SIZE and 
>    ensuring Makefile uses the right size for --pad-to, as well as
>    the few other files which use CONFIG_SPL_MAX_SIZE.
> 
> Amicalement,


Amicalement,
Tom Rini - April 5, 2013, 7:44 p.m.
On Fri, Apr 05, 2013 at 09:17:40PM +0200, Albert ARIBAUD wrote:
> Hi Tom,
> 
> On Fri, 5 Apr 2013 13:55:21 -0400, Tom Rini <trini@ti.com> wrote:
> 
> > On Fri, Apr 05, 2013 at 07:32:54PM +0200, Beno??t Th??baudeau wrote:
> > > Hi Tom,
> > > 
> > > On Friday, April 5, 2013 6:00:30 PM, Tom Rini wrote:
> > > > On Fri, Apr 05, 2013 at 03:56:46PM +0200, Beno??t Th??baudeau wrote:
> > > > > Hi Albert,
> > > > > 
> > > > > On Friday, April 5, 2013 8:00:43 AM, Albert ARIBAUD wrote:
> > > > > > Hi Beno??t,
> > > > [snip]
> > > > > > IIUC, this future patch would increase the limit for SPL run-time size,
> > > > > > as the constant against which the ASS tests __bss_end for would
> > > > > > necessarily be greater than it is now. Correct? If so, this future
> > > > > > patch should not break any target, as it would loosen the constraint,
> > > > > > not tighten it.
> > > > > 
> > > > > Yes, it would either be the same or relaxed a bit, depending on the chosen
> > > > > option:
> > > > >  - Define CONFIG_SPL_BSS_MAX_SIZE and test against CONFIG_SPL_MAX_SIZE +
> > > > >    CONFIG_SPL_BSS_MAX_SIZE, the sum remaining the same as or being larger
> > > > >    than
> > > > >    currently, depending on the new values for CONFIG_SPL_MAX_SIZE and
> > > > >    CONFIG_SPL_BSS_MAX_SIZE.
> > > > >  - Define a new config meaning text + data + rodata + bss (e.g.
> > > > >    CONFIG_SPL_MAX_RAM_SIZE or CONFIG_SPL_MAX_MEM_FOOTPRINT), and just
> > > > >    replace
> > > > >    CONFIG_SPL_MAX_SIZE with it for the users of arch/arm/cpu/u-boot*.lds,
> > > > >    taking
> > > > >    care that this was the only meaning those users were giving to
> > > > >    CONFIG_SPL_MAX_SIZE.
> > > > > 
> > > > > The first option would probably be preferable, using the same value for
> > > > > CONFIG_SPL_MAX_SIZE, and a non-zero value for CONFIG_SPL_BSS_MAX_SIZE.
> > > > 
> > > > I think the problem is that Tegra really needs the second case as their
> > > > constraint is "must fit below next part of payload".  We can assume the
> > > > users of that linker script today care about footprint and update their
> > > > define I believe.  da850evm and the rest of the davinci platforms would
> > > > also be a case to convert to this, but the omap*/am3* platforms would
> > > > not.
> > > 
> > > Yes, then let's have an assert in arch/arm/cpu/u-boot*.lds with a
> > > different config name (as in option 2 above) just for Tegra, and
> > > another assert for CONFIG_SPL_MAX_SIZE against __bss_start.
> > > 
> > > And all users of CONFIG_SPL_MAX_SIZE should be checked to make sure
> > > that there is not another special case somewhere.
> > 
> > I didn't audit the PowerPC targets, but on ARM we have, roughly:
> > - Tegra (covered in Stephen's email, and in short, must include BSS in
> >   size check) which uses SPL_MAX_SIZE to include BSS
> > - OMAP*/AM3* which does not constrain BSS to SPL_MAX_SIZE
> > - DaVinci which must also constrain BSS to the initial RAM, but for
> >   different reasons.
> > - iMX which also uses SPL_BSS_MAX to cover the BSS separate from the
> >   rest of the program.
> 
> How about this?
> 
> 1. In the u-boot*.lds files, doing separate asserts for SPL and SPL BSS
>    max size, with the SPL assert being further divided in two cases
>    depending on BSS max size being defined or not:
> 
>    #if defined(CONFIG_SPL_MAX_SIZE)
>    #if   defined(CONFIG_SPL_BSS_MAX_SIZE)
>    ASSERT( __bss_end - __image_copy_start < (CONFIG_SPL_TEXT_BASE + \
>        CONFIG_SPL_MAX_SIZE), "SPL image code+BSS too big");
>    #else
>    ASSERT( __bss_end - __image_copy_start < CONFIG_SPL_TEXT_BASE, \
>        CONFIG_SPL_MAX_SIZE), "SPL image code too big");
>    #endif
> 
>    #if defined(CONFIG_SPL_BSS_MAX_SIZE)
>    ASSERT( __bss_end - __bss_start < CONFIG_SPL_TEXT_BASE, \
>        CONFIG_SPL_BSS_MAX_SIZE), "SPL image BSS too big");
>    #endif
>    #endif

I think this is too complicated.  Our cases are:
1. text/data/rodata/bss MUST fit within $size for $location
2. text/data/rodata MUST fit within $sizeA for $locationA and BSS must
   start at $locationB (which is at least $sizeB big)

The problem is that CONFIG_SPL_MAX_SIZE is defined to mean #2 but the
generic ARM SPL linker script was using it for #1.  We should correct
the generic ARM SPL linker script to test #2, and add in a new option to
cover #1 and convert tegra (as they're the user of the generic script)
to this new option.

> 2. Defining CONFIG_SPL_BSS_MAX_SIZE only for Tegra, Davinci, IMX (where
>    CONFIG_SPL_BSS_MAX_SIZE is actually the gap size)

No, this is wrong.  These do not care about the BSS size, they care
about the text/data/rodata/bss size.

> 3. *Not* defining CONFIG_SPL_MAX_SIZE or CONFIG_SPL_BSS_MAX_SIZE for
>    OMAP*/AM3*

This is wrong.  The main reason for SPL_BSS_MAX_SIZE here is that we
have the BSS at a very different location from the rest of the binary
(text/data/rodata at 0x4... BSS at 0x8...) and use MEMORY constructs to
place things correctly.  So we must(?) define a start and end, and
that's what BSS_MAX_SIZE is for.

> 4. Adjusting README descriptions of CONFIG_SPL_[BSS_]MAX_SIZE and 
>    ensuring Makefile uses the right size for --pad-to, as well as
>    the few other files which use CONFIG_SPL_MAX_SIZE.

Yes, we need to audit README and users after all of this.
Albert ARIBAUD - April 5, 2013, 8:04 p.m.
Hi Tom,

On Fri, 5 Apr 2013 15:44:08 -0400, Tom Rini <trini@ti.com> wrote:

> On Fri, Apr 05, 2013 at 09:17:40PM +0200, Albert ARIBAUD wrote:
> > Hi Tom,
> > 
> > On Fri, 5 Apr 2013 13:55:21 -0400, Tom Rini <trini@ti.com> wrote:
> > 
> > > On Fri, Apr 05, 2013 at 07:32:54PM +0200, Beno??t Th??baudeau wrote:
> > > > Hi Tom,
> > > > 
> > > > On Friday, April 5, 2013 6:00:30 PM, Tom Rini wrote:
> > > > > On Fri, Apr 05, 2013 at 03:56:46PM +0200, Beno??t Th??baudeau wrote:
> > > > > > Hi Albert,
> > > > > > 
> > > > > > On Friday, April 5, 2013 8:00:43 AM, Albert ARIBAUD wrote:
> > > > > > > Hi Beno??t,
> > > > > [snip]
> > > > > > > IIUC, this future patch would increase the limit for SPL run-time size,
> > > > > > > as the constant against which the ASS tests __bss_end for would
> > > > > > > necessarily be greater than it is now. Correct? If so, this future
> > > > > > > patch should not break any target, as it would loosen the constraint,
> > > > > > > not tighten it.
> > > > > > 
> > > > > > Yes, it would either be the same or relaxed a bit, depending on the chosen
> > > > > > option:
> > > > > >  - Define CONFIG_SPL_BSS_MAX_SIZE and test against CONFIG_SPL_MAX_SIZE +
> > > > > >    CONFIG_SPL_BSS_MAX_SIZE, the sum remaining the same as or being larger
> > > > > >    than
> > > > > >    currently, depending on the new values for CONFIG_SPL_MAX_SIZE and
> > > > > >    CONFIG_SPL_BSS_MAX_SIZE.
> > > > > >  - Define a new config meaning text + data + rodata + bss (e.g.
> > > > > >    CONFIG_SPL_MAX_RAM_SIZE or CONFIG_SPL_MAX_MEM_FOOTPRINT), and just
> > > > > >    replace
> > > > > >    CONFIG_SPL_MAX_SIZE with it for the users of arch/arm/cpu/u-boot*.lds,
> > > > > >    taking
> > > > > >    care that this was the only meaning those users were giving to
> > > > > >    CONFIG_SPL_MAX_SIZE.
> > > > > > 
> > > > > > The first option would probably be preferable, using the same value for
> > > > > > CONFIG_SPL_MAX_SIZE, and a non-zero value for CONFIG_SPL_BSS_MAX_SIZE.
> > > > > 
> > > > > I think the problem is that Tegra really needs the second case as their
> > > > > constraint is "must fit below next part of payload".  We can assume the
> > > > > users of that linker script today care about footprint and update their
> > > > > define I believe.  da850evm and the rest of the davinci platforms would
> > > > > also be a case to convert to this, but the omap*/am3* platforms would
> > > > > not.
> > > > 
> > > > Yes, then let's have an assert in arch/arm/cpu/u-boot*.lds with a
> > > > different config name (as in option 2 above) just for Tegra, and
> > > > another assert for CONFIG_SPL_MAX_SIZE against __bss_start.
> > > > 
> > > > And all users of CONFIG_SPL_MAX_SIZE should be checked to make sure
> > > > that there is not another special case somewhere.
> > > 
> > > I didn't audit the PowerPC targets, but on ARM we have, roughly:
> > > - Tegra (covered in Stephen's email, and in short, must include BSS in
> > >   size check) which uses SPL_MAX_SIZE to include BSS
> > > - OMAP*/AM3* which does not constrain BSS to SPL_MAX_SIZE
> > > - DaVinci which must also constrain BSS to the initial RAM, but for
> > >   different reasons.
> > > - iMX which also uses SPL_BSS_MAX to cover the BSS separate from the
> > >   rest of the program.
> > 
> > How about this?
> > 
> > 1. In the u-boot*.lds files, doing separate asserts for SPL and SPL BSS
> >    max size, with the SPL assert being further divided in two cases
> >    depending on BSS max size being defined or not:
> > 
> >    #if defined(CONFIG_SPL_MAX_SIZE)
> >    #if   defined(CONFIG_SPL_BSS_MAX_SIZE)
> >    ASSERT( __bss_end - __image_copy_start < (CONFIG_SPL_TEXT_BASE + \
> >        CONFIG_SPL_MAX_SIZE), "SPL image code+BSS too big");
> >    #else
> >    ASSERT( __bss_end - __image_copy_start < CONFIG_SPL_TEXT_BASE, \
> >        CONFIG_SPL_MAX_SIZE), "SPL image code too big");
> >    #endif
> > 
> >    #if defined(CONFIG_SPL_BSS_MAX_SIZE)
> >    ASSERT( __bss_end - __bss_start < CONFIG_SPL_TEXT_BASE, \
> >        CONFIG_SPL_BSS_MAX_SIZE), "SPL image BSS too big");
> >    #endif
> >    #endif
> 
> I think this is too complicated.  Our cases are:
> 1. text/data/rodata/bss MUST fit within $size for $location
> 2. text/data/rodata MUST fit within $sizeA for $locationA and BSS must
>    start at $locationB (which is at least $sizeB big)
> 
> The problem is that CONFIG_SPL_MAX_SIZE is defined to mean #2 but the
> generic ARM SPL linker script was using it for #1.  We should correct
> the generic ARM SPL linker script to test #2, and add in a new option to
> cover #1 and convert tegra (as they're the user of the generic script)
> to this new option.
> 
> > 2. Defining CONFIG_SPL_BSS_MAX_SIZE only for Tegra, Davinci, IMX (where
> >    CONFIG_SPL_BSS_MAX_SIZE is actually the gap size)
> 
> No, this is wrong.  These do not care about the BSS size, they care
> about the text/data/rodata/bss size.

Maybe I wasn't clear. I did not mean to "define only BSS max size for
those [and not image max size]", but to "define, only for those, BSS max
size [in addition to image max size being defined for most of the
boards including these]" -- IOW, while most if not all boards want to
check image size, only some boards seem to want to check BSS size in
addition, and we can differentiate with CONFIG_SPL_BSS_MAX_SIZE.

> > 3. *Not* defining CONFIG_SPL_MAX_SIZE or CONFIG_SPL_BSS_MAX_SIZE for
> >    OMAP*/AM3*
> 
> This is wrong.  The main reason for SPL_BSS_MAX_SIZE here is that we
> have the BSS at a very different location from the rest of the binary
> (text/data/rodata at 0x4... BSS at 0x8...) and use MEMORY constructs to
> place things correctly.  So we must(?) define a start and end, and
> that's what BSS_MAX_SIZE is for.

Then I misunderstood you -- you seemed to be saying OMAP*/AM3* did not
constrain BSS or SPL max size. If they do constrain image size, then
they must define CONFIG_SPL_MAX_SIZE; if they do constrain BSS, then
they must define CONFIG_SPL_BSS_MAX_SIZE.

The two general ideas of my proposal are:

1) to separate testing the image (text,data,rodata,lists) size on the
   one hand and the image BSS size on the other hand, and

2) to consider that if a target defines an image max size and a BSS
max size, then the image max size does not include the BSS size ; and
if it defined an image max size but no BSS max size, then the image max
size includes the BSS.

The first idea allows boards with disjoint image and BSS to still check
eahc part's size, a thing not feasible with the current code; the
second idea allows fewer changes, but if one wants CONFIG_SPL_MAX_SIZE
to have a strict meaning, we can drop idea #2 and still keep idea #1.

> > 4. Adjusting README descriptions of CONFIG_SPL_[BSS_]MAX_SIZE and 
> >    ensuring Makefile uses the right size for --pad-to, as well as
> >    the few other files which use CONFIG_SPL_MAX_SIZE.
> 
> Yes, we need to audit README and users after all of this.

Amicalement,
Tom Rini - April 5, 2013, 8:23 p.m.
On Fri, Apr 05, 2013 at 10:04:02PM +0200, Albert ARIBAUD wrote:

> The two general ideas of my proposal are:
> 
> 1) to separate testing the image (text,data,rodata,lists) size on the
>    one hand and the image BSS size on the other hand, and
> 
> 2) to consider that if a target defines an image max size and a BSS
> max size, then the image max size does not include the BSS size ; and
> if it defined an image max size but no BSS max size, then the image max
> size includes the BSS.
> 
> The first idea allows boards with disjoint image and BSS to still check
> eahc part's size, a thing not feasible with the current code; the
> second idea allows fewer changes, but if one wants CONFIG_SPL_MAX_SIZE
> to have a strict meaning, we can drop idea #2 and still keep idea #1.

Make it so, thanks!
Albert ARIBAUD - April 8, 2013, 7:03 p.m.
Hi Tom,

On Fri, 5 Apr 2013 16:23:30 -0400, Tom Rini <trini@ti.com> wrote:

> On Fri, Apr 05, 2013 at 10:04:02PM +0200, Albert ARIBAUD wrote:
> 
> > The two general ideas of my proposal are:
> > 
> > 1) to separate testing the image (text,data,rodata,lists) size on the
> >    one hand and the image BSS size on the other hand, and
> > 
> > 2) to consider that if a target defines an image max size and a BSS
> > max size, then the image max size does not include the BSS size ; and
> > if it defined an image max size but no BSS max size, then the image max
> > size includes the BSS.
> > 
> > The first idea allows boards with disjoint image and BSS to still check
> > eahc part's size, a thing not feasible with the current code; the
> > second idea allows fewer changes, but if one wants CONFIG_SPL_MAX_SIZE
> > to have a strict meaning, we can drop idea #2 and still keep idea #1.
> 
> Make it so, thanks!

Just a quick heads up: in the little time I could spare this week-end,
I analyzed the SPL image vs BSS max sizes issue, and the following
appeared:


1. File arch/arm/cpu/u-boot.lds has an ASSERT() regarding SPL even
   though it is never used for building SPL (tested by replacing the
   ASSERT() condition by 0 and building all of ARM).

2. Several boards use arch/arm/cpu/u-boot-spl.lds, which has an ASSERT()
   wrongly comparing image+BSS size to image-only CONFIG_SPL_MAX_SIZE.
   Fixing the linker file will fix all tegra boards as well as the
   exynos origen and smdkv310.

   Note: the tegra-common.h config file states a non-existent .lds file
   for SPL.

3. Boards cam_enc_4xx, da850evm, smdk5250 and snow all define maximum
   SPL size to include image and BSS. These must be split arbitrarily;
   I have chosen values that fit sizes from current build results.

4. Boards MPC8313ERDB_[NAND_]{33,66} hardcode their image+BSS limit. I
   have left this untouched.

5. Board a3m071 and its linker file define CONFIG_SYS_SPL_MAX_LEN
   instead of CONFIG_SPL_MAX_SIZE, but the semantics are correct. Left
   unchanged.

6. Generic p1_p2_rdb_pc board header defines CONFIG_SPL_MAX_SIZE but
   linker files do not seem to test it anyway. Left unchanged.

7. Tegra configs specify a non-existent lds file name. Build defaults
   to generic ARM lds file. Left unchanged.

Patch series to follow in a couple of hours.

Amicalement,
Tom Rini - April 8, 2013, 7:56 p.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/08/2013 03:03 PM, Albert ARIBAUD wrote:
> Hi Tom,
> 
> On Fri, 5 Apr 2013 16:23:30 -0400, Tom Rini <trini@ti.com> wrote:
> 
>> On Fri, Apr 05, 2013 at 10:04:02PM +0200, Albert ARIBAUD wrote:
>> 
>>> The two general ideas of my proposal are:
>>> 
>>> 1) to separate testing the image (text,data,rodata,lists) size
>>> on the one hand and the image BSS size on the other hand, and
>>> 
>>> 2) to consider that if a target defines an image max size and a
>>> BSS max size, then the image max size does not include the BSS
>>> size ; and if it defined an image max size but no BSS max size,
>>> then the image max size includes the BSS.
>>> 
>>> The first idea allows boards with disjoint image and BSS to
>>> still check eahc part's size, a thing not feasible with the
>>> current code; the second idea allows fewer changes, but if one
>>> wants CONFIG_SPL_MAX_SIZE to have a strict meaning, we can drop
>>> idea #2 and still keep idea #1.
>> 
>> Make it so, thanks!
> 
> Just a quick heads up: in the little time I could spare this
> week-end, I analyzed the SPL image vs BSS max sizes issue, and the
> following appeared:
> 
> 
> 1. File arch/arm/cpu/u-boot.lds has an ASSERT() regarding SPL even 
> though it is never used for building SPL (tested by replacing the 
> ASSERT() condition by 0 and building all of ARM).
> 
> 2. Several boards use arch/arm/cpu/u-boot-spl.lds, which has an
> ASSERT() wrongly comparing image+BSS size to image-only
> CONFIG_SPL_MAX_SIZE. Fixing the linker file will fix all tegra
> boards as well as the exynos origen and smdkv310.
> 
> Note: the tegra-common.h config file states a non-existent .lds
> file for SPL.

I was wondering about #1 and the tegra bit too, but thought there must
be magic I'm missing.

> 
> 3. Boards cam_enc_4xx, da850evm, smdk5250 and snow all define
> maximum SPL size to include image and BSS. These must be split
> arbitrarily; I have chosen values that fit sizes from current build
> results.

In all of the first two, it's limit on overall size
(text/data/rodata/bss).


- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRYyDcAAoJENk4IS6UOR1WzEwP/1JmhuZRwr/UZ1Pg51tqeRHW
Tk9w5JTVZoOIUFGT10JNp2y5nWIjCFlFDlZMnXTFtNpBYO7gV+qJrI+54LhSR8Ra
UxOwp3MHV/ihb/6iTY8Bx9ziEvdwU78NnoS2dCwOoY9UuskQ++M2qGgLtYnYU2Oh
9KVfDEf9/rItO/JL63ZviYPgoLBJ88OSGM/fc3cw1n89WDCl4u/AUqS3d3IR+G4U
hCVAuJfp3xbX5z/zvzj56kmJeLvxjfA5M9/M74YABU5mJ+4Rq9pZ+h93JVyttfjZ
4muDY8GQJ/uha/1FPbnukN71Gr6wdbobeU0dkz371moiPGNw3GQ2pS9EkJ5BXT/l
1KqDK7Y7B6v1jFxIIW6Uffjrd3BrX8XHxn3nkhV35CRAjOqYwSJXm/duAeYEnek4
mDTyjTxs6f1vA4neRs3rTlZI6TsrkDPSFKQJyOzjPL/9l6S672ui5tHcyeG5Um0P
T0QIL0uNf5fE/ER4nOxgY1wWHN+tHvsQyLJgiA5va61qtxqReD4D2GqmSnIkWDuT
zrnP9df9mJFVlbnrAjgjZNNbg+zo2kUfvx0cLsM+5ly/cl3CQD3CjjNocPmvBIML
r0l7oKo+yfykGzmh5M90AhD6yshGQJOabunEp5zqqYAFoG6sQMQZp0lzqgtP2mhW
LN1Eg8+xGB4KfxoI5RZn
=YZOX
-----END PGP SIGNATURE-----
Albert ARIBAUD - April 8, 2013, 8:05 p.m.
Hi Tom,

On Mon, 8 Apr 2013 15:56:12 -0400, Tom Rini <trini@ti.com> wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 04/08/2013 03:03 PM, Albert ARIBAUD wrote:
> > Hi Tom,
> > 
> > On Fri, 5 Apr 2013 16:23:30 -0400, Tom Rini <trini@ti.com> wrote:
> > 
> >> On Fri, Apr 05, 2013 at 10:04:02PM +0200, Albert ARIBAUD wrote:
> >> 
> >>> The two general ideas of my proposal are:
> >>> 
> >>> 1) to separate testing the image (text,data,rodata,lists) size
> >>> on the one hand and the image BSS size on the other hand, and
> >>> 
> >>> 2) to consider that if a target defines an image max size and a
> >>> BSS max size, then the image max size does not include the BSS
> >>> size ; and if it defined an image max size but no BSS max size,
> >>> then the image max size includes the BSS.
> >>> 
> >>> The first idea allows boards with disjoint image and BSS to
> >>> still check eahc part's size, a thing not feasible with the
> >>> current code; the second idea allows fewer changes, but if one
> >>> wants CONFIG_SPL_MAX_SIZE to have a strict meaning, we can drop
> >>> idea #2 and still keep idea #1.
> >> 
> >> Make it so, thanks!
> > 
> > Just a quick heads up: in the little time I could spare this
> > week-end, I analyzed the SPL image vs BSS max sizes issue, and the
> > following appeared:
> > 
> > 
> > 1. File arch/arm/cpu/u-boot.lds has an ASSERT() regarding SPL even 
> > though it is never used for building SPL (tested by replacing the 
> > ASSERT() condition by 0 and building all of ARM).
> > 
> > 2. Several boards use arch/arm/cpu/u-boot-spl.lds, which has an
> > ASSERT() wrongly comparing image+BSS size to image-only
> > CONFIG_SPL_MAX_SIZE. Fixing the linker file will fix all tegra
> > boards as well as the exynos origen and smdkv310.
> > 
> > Note: the tegra-common.h config file states a non-existent .lds
> > file for SPL.
> 
> I was wondering about #1 and the tegra bit too, but thought there must
> be magic I'm missing.

The tegra ones got me triple-checking, but yes, the config-specified lds
file is ignored and the build defaults to the generic ARM lds file.

> > 3. Boards cam_enc_4xx, da850evm, smdk5250 and snow all define
> > maximum SPL size to include image and BSS. These must be split
> > arbitrarily; I have chosen values that fit sizes from current build
> > results.
> 
> In all of the first two, it's limit on overall size
> (text/data/rodata/bss).

Not sure I understand you there. Anyway the patches are out, so best is
you comment directly on the changes.

Amicalement,
Albert ARIBAUD - April 11, 2013, 3:30 p.m.
On Fri,  5 Apr 2013 00:13:53 +0200, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:

> Commit 3ebd1cbc introduced compiler-generated __bss_start
> and __bss_end__ and commit c23561e7 rewrote all __bss_end__
> as __bss_end. Their merge caused silent and harmless but
> potentially bug-inducing clashes between compiler- and linker-
> enerated __bss_end symbols.
> 
> Make __bss_end and __bss_start compiler-only, and create
> __bss_base and __bss_limit for linker-only use.
> 
> Reported-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
> Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> ---
>  arch/arm/cpu/ixp/u-boot.lds        |   14 ++++++++++----
>  arch/arm/cpu/u-boot.lds            |   14 ++++++++++----
>  board/actux1/u-boot.lds            |   14 ++++++++++----
>  board/actux2/u-boot.lds            |   14 ++++++++++----
>  board/actux3/u-boot.lds            |   14 ++++++++++----
>  board/dvlhost/u-boot.lds           |   14 ++++++++++----
>  board/freescale/mx31ads/u-boot.lds |   14 ++++++++++----
>  7 files changed, 70 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/arm/cpu/ixp/u-boot.lds b/arch/arm/cpu/ixp/u-boot.lds
> index 8345b55..c999829 100644
> --- a/arch/arm/cpu/ixp/u-boot.lds
> +++ b/arch/arm/cpu/ixp/u-boot.lds
> @@ -67,17 +67,23 @@ SECTIONS
>  
>  	_end = .;
>  
> +/*
> + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> + * __bss_base and __bss_limit are for linker only (overlay ordering)
> + */
> +
>  	.bss_start __rel_dyn_start (OVERLAY) : {
>  		KEEP(*(.__bss_start));
> +		HIDDEN(__bss_base = .);
>  	}
>  
> -	.bss __bss_start (OVERLAY) : {
> +	.bss __bss_base (OVERLAY) : {
>  		*(.bss*)
>  		 . = ALIGN(4);
> -		 __bss_end = .;
> +		 HIDDEN(__bss_limit = .);
>  	}
> -	.bss_end __bss_end (OVERLAY) : {
> -		KEEP(*(__bss_end));
> +	.bss_end __bss_limit (OVERLAY) : {
> +		KEEP(*(.__bss_end));
>  	}
>  
>  	/DISCARD/ : { *(.dynstr*) }
> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> index 3a1083d..0543b06 100644
> --- a/arch/arm/cpu/u-boot.lds
> +++ b/arch/arm/cpu/u-boot.lds
> @@ -81,18 +81,24 @@ SECTIONS
>  		*(.mmutable)
>  	}
>  
> +/*
> + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> + * __bss_base and __bss_limit are for linker only (overlay ordering)
> + */
> +
>  	.bss_start __rel_dyn_start (OVERLAY) : {
>  		KEEP(*(.__bss_start));
> +		HIDDEN(__bss_base = .);
>  	}
>  
> -	.bss __bss_start (OVERLAY) : {
> +	.bss __bss_base (OVERLAY) : {
>  		*(.bss*)
>  		 . = ALIGN(4);
> -		 __bss_end = .;
> +		 HIDDEN(__bss_limit = .);
>  	}
>  
> -	.bss_end __bss_end (OVERLAY) : {
> -		KEEP(*(__bss_end));
> +	.bss_end __bss_limit (OVERLAY) : {
> +		KEEP(*(.__bss_end));
>  	}
>  
>  	/DISCARD/ : { *(.dynstr*) }
> diff --git a/board/actux1/u-boot.lds b/board/actux1/u-boot.lds
> index c76728a..7e5c4d8 100644
> --- a/board/actux1/u-boot.lds
> +++ b/board/actux1/u-boot.lds
> @@ -74,17 +74,23 @@ SECTIONS
>  
>  	_end = .;
>  
> +/*
> + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> + * __bss_base and __bss_limit are for linker only (overlay ordering)
> + */
> +
>  	.bss_start __rel_dyn_start (OVERLAY) : {
>  		KEEP(*(.__bss_start));
> +		HIDDEN(__bss_base = .);
>  	}
>  
> -	.bss __bss_start (OVERLAY) : {
> +	.bss __bss_base (OVERLAY) : {
>  		*(.bss*)
>  		 . = ALIGN(4);
> -		 __bss_end = .;
> +		 HIDDEN(__bss_limit = .);
>  	}
> -	.bss_end __bss_end (OVERLAY) : {
> -		KEEP(*(__bss_end));
> +	.bss_end __bss_limit (OVERLAY) : {
> +		KEEP(*(.__bss_end));
>  	}
>  
>  	/DISCARD/ : { *(.dynstr*) }
> diff --git a/board/actux2/u-boot.lds b/board/actux2/u-boot.lds
> index 984f70e..ce1b7c9 100644
> --- a/board/actux2/u-boot.lds
> +++ b/board/actux2/u-boot.lds
> @@ -74,17 +74,23 @@ SECTIONS
>  
>  	_end = .;
>  
> +/*
> + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> + * __bss_base and __bss_limit are for linker only (overlay ordering)
> + */
> +
>  	.bss_start __rel_dyn_start (OVERLAY) : {
>  		KEEP(*(.__bss_start));
> +		HIDDEN(__bss_base = .);
>  	}
>  
> -	.bss __bss_start (OVERLAY) : {
> +	.bss __bss_base (OVERLAY) : {
>  		*(.bss*)
>  		 . = ALIGN(4);
> -		 __bss_end = .;
> +		 HIDDEN(__bss_limit = .);
>  	}
> -	.bss_end __bss_end (OVERLAY) : {
> -		KEEP(*(__bss_end));
> +	.bss_end __bss_limit (OVERLAY) : {
> +		KEEP(*(.__bss_end));
>  	}
>  
>  	/DISCARD/ : { *(.dynstr*) }
> diff --git a/board/actux3/u-boot.lds b/board/actux3/u-boot.lds
> index fc48cf0..3e091dd 100644
> --- a/board/actux3/u-boot.lds
> +++ b/board/actux3/u-boot.lds
> @@ -74,17 +74,23 @@ SECTIONS
>  
>  	_end = .;
>  
> +/*
> + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> + * __bss_base and __bss_limit are for linker only (overlay ordering)
> + */
> +
>  	.bss_start __rel_dyn_start (OVERLAY) : {
>  		KEEP(*(.__bss_start));
> +		HIDDEN(__bss_base = .);
>  	}
>  
> -	.bss __bss_start (OVERLAY) : {
> +	.bss __bss_base (OVERLAY) : {
>  		*(.bss*)
>  		 . = ALIGN(4);
> -		 __bss_end = .;
> +		HIDDEN(__bss_limit = .);
>  	}
> -	.bss_end __bss_end (OVERLAY) : {
> -		KEEP(*(__bss_end));
> +	.bss_end __bss_limit (OVERLAY) : {
> +		KEEP(*(.__bss_end));
>  	}
>  
>  	/DISCARD/ : { *(.dynstr*) }
> diff --git a/board/dvlhost/u-boot.lds b/board/dvlhost/u-boot.lds
> index b13d3e1..fe3c21b 100644
> --- a/board/dvlhost/u-boot.lds
> +++ b/board/dvlhost/u-boot.lds
> @@ -74,17 +74,23 @@ SECTIONS
>  
>  	_end = .;
>  
> +/*
> + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> + * __bss_base and __bss_limit are for linker only (overlay ordering)
> + */
> +
>  	.bss_start __rel_dyn_start (OVERLAY) : {
>  		KEEP(*(.__bss_start));
> +		HIDDEN(__bss_base = .);
>  	}
>  
> -	.bss __bss_start (OVERLAY) : {
> +	.bss __bss_base (OVERLAY) : {
>  		*(.bss*)
>  		 . = ALIGN(4);
> -		 __bss_end = .;
> +		 HIDDEN(__bss_limit = .);
>  	}
> -	.bss_end __bss_end (OVERLAY) : {
> -		KEEP(*(__bss_end));
> +	.bss_end __bss_limit (OVERLAY) : {
> +		KEEP(*(.__bss_end));
>  	}
>  
>  	/DISCARD/ : { *(.dynstr*) }
> diff --git a/board/freescale/mx31ads/u-boot.lds b/board/freescale/mx31ads/u-boot.lds
> index 264c4e8..e6885a5 100644
> --- a/board/freescale/mx31ads/u-boot.lds
> +++ b/board/freescale/mx31ads/u-boot.lds
> @@ -80,17 +80,23 @@ SECTIONS
>  
>  	_end = .;
>  
> +/*
> + * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
> + * __bss_base and __bss_limit are for linker only (overlay ordering)
> + */
> +
>  	.bss_start __rel_dyn_start (OVERLAY) : {
>  		KEEP(*(.__bss_start));
> +		HIDDEN(__bss_base = .);
>  	}
>  
> -	.bss __bss_start (OVERLAY) : {
> +	.bss __bss_base (OVERLAY) : {
>  		*(.bss*)
>  		 . = ALIGN(4);
> -		 __bss_end = .;
> +		 HIDDEN(__bss_limit = .);
>  	}
> -	.bss_end __bss_end (OVERLAY) : {
> -		KEEP(*(__bss_end));
> +	.bss_end __bss_limit (OVERLAY) : {
> +		KEEP(*(.__bss_end));
>  	}
>  
>  	/DISCARD/ : { *(.bss*) }

Officially, HIDDEN() is known only from binutils 2.23 up, even though
Ubuntu/Linaro ARM binutils 2.22.90.20120919 knows about it. HIDDEN()
fails for ELDK 4.2 and 5.2.1 as well as Ubuntu/Linaro 2.23.52.20120713.

Amicalement,
Albert ARIBAUD - April 11, 2013, 5:59 p.m.
Hi Tom,

On Thu, 11 Apr 2013 10:52:08 -0700, Tom Warren <TWarren@nvidia.com>
wrote:

> Albert,
> 
> > -----Original Message-----
> > From: Stephen Warren [mailto:swarren@wwwdotorg.org]
> > Sent: Friday, April 05, 2013 8:59 AM
> > To: Tom Rini
> > Cc: Albert ARIBAUD; Scott Wood; u-boot@lists.denx.de;
> > Stephen@theia.denx.de; Tom Warren; Simon Glass; Allen Martin
> > Subject: Re: [U-Boot] [PATCH] ARM: Fix __bss_start and __bss_end in linker
> > scripts
> 
> I just fetched & merged TOT u-boot-arm (getting ready for a PR from u-boot-tegra -> u-boot-arm), and did a test build of all Tegra boards.
> 
> I get the following linker (ld) error on all builds due to this commit (abbecf4c8,  ARM: Fix __bss_start and __bss_end in linker scripts):
> 
>  arm-linux-gnueabi-ld.bfd:u-boot.lds:44: syntax error
> 
> This is pointing to the 'HIDDEN(__bss_base = .);' lines added in this change (in the generated u-boot.lds in the root dir).
> 
> I'm seeing this with both gcc 4.4.1/ld 2.19.51  and gcc 4.6.2/ld 2.22 tools.
> 
> What compiler/binutils did you use to build/test this?

This has been detected already, and one of the reasons why Tom and I
decided to roll back ARM. Normally, you should have u-boot-arm/master =
u-boot-arm/master = dd2445ec1b839a5ca61ff8438a5b7aebb21b7986.

> Tom

Amicalement,
Tom Warren - April 11, 2013, 6:13 p.m.
Albert,


On Thu, Apr 11, 2013 at 10:59 AM, Albert ARIBAUD
<albert.u.boot@aribaud.net>wrote:

> Hi Tom,
>
> On Thu, 11 Apr 2013 10:52:08 -0700, Tom Warren <TWarren@nvidia.com>
> wrote:
>
> > Albert,
> >
> > > -----Original Message-----
> > > From: Stephen Warren [mailto:swarren@wwwdotorg.org]
> > > Sent: Friday, April 05, 2013 8:59 AM
> > > To: Tom Rini
> > > Cc: Albert ARIBAUD; Scott Wood; u-boot@lists.denx.de;
> > > Stephen@theia.denx.de; Tom Warren; Simon Glass; Allen Martin
> > > Subject: Re: [U-Boot] [PATCH] ARM: Fix __bss_start and __bss_end in
> linker
> > > scripts
> >
> > I just fetched & merged TOT u-boot-arm (getting ready for a PR from
> u-boot-tegra -> u-boot-arm), and did a test build of all Tegra boards.
> >
> > I get the following linker (ld) error on all builds due to this commit
> (abbecf4c8,  ARM: Fix __bss_start and __bss_end in linker scripts):
> >
> >  arm-linux-gnueabi-ld.bfd:u-boot.lds:44: syntax error
> >
> > This is pointing to the 'HIDDEN(__bss_base = .);' lines added in this
> change (in the generated u-boot.lds in the root dir).
> >
> > I'm seeing this with both gcc 4.4.1/ld 2.19.51  and gcc 4.6.2/ld 2.22
> tools.
> >
> > What compiler/binutils did you use to build/test this?
>
> This has been detected already, and one of the reasons why Tom and I
> decided to roll back ARM. Normally, you should have u-boot-arm/master =
> u-boot-arm/master = dd2445ec1b839a5ca61ff8438a5b7aebb21b7986.
>

>you should have u-boot-arm/master = u-boot-arm/master =
dd2445ec1b839a5ca61ff8438a5b7aebb21b7986.

I would hope that u-boot-arm/master would always == u-boot-arm/master ;)

That commit is not 5 patches back (from our other thread 'Rollback *again*
-- 5 patches), but more like 45.

I'll wait to do a fetch/rebase/PR for u-boot-tegra until the Tegra builds
are fixed.

Thanks,

Tom

>
> > Tom
>
> Amicalement,
> --
> Albert.
>

Patch

diff --git a/arch/arm/cpu/ixp/u-boot.lds b/arch/arm/cpu/ixp/u-boot.lds
index 8345b55..c999829 100644
--- a/arch/arm/cpu/ixp/u-boot.lds
+++ b/arch/arm/cpu/ixp/u-boot.lds
@@ -67,17 +67,23 @@  SECTIONS
 
 	_end = .;
 
+/*
+ * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
+ * __bss_base and __bss_limit are for linker only (overlay ordering)
+ */
+
 	.bss_start __rel_dyn_start (OVERLAY) : {
 		KEEP(*(.__bss_start));
+		HIDDEN(__bss_base = .);
 	}
 
-	.bss __bss_start (OVERLAY) : {
+	.bss __bss_base (OVERLAY) : {
 		*(.bss*)
 		 . = ALIGN(4);
-		 __bss_end = .;
+		 HIDDEN(__bss_limit = .);
 	}
-	.bss_end __bss_end (OVERLAY) : {
-		KEEP(*(__bss_end));
+	.bss_end __bss_limit (OVERLAY) : {
+		KEEP(*(.__bss_end));
 	}
 
 	/DISCARD/ : { *(.dynstr*) }
diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
index 3a1083d..0543b06 100644
--- a/arch/arm/cpu/u-boot.lds
+++ b/arch/arm/cpu/u-boot.lds
@@ -81,18 +81,24 @@  SECTIONS
 		*(.mmutable)
 	}
 
+/*
+ * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
+ * __bss_base and __bss_limit are for linker only (overlay ordering)
+ */
+
 	.bss_start __rel_dyn_start (OVERLAY) : {
 		KEEP(*(.__bss_start));
+		HIDDEN(__bss_base = .);
 	}
 
-	.bss __bss_start (OVERLAY) : {
+	.bss __bss_base (OVERLAY) : {
 		*(.bss*)
 		 . = ALIGN(4);
-		 __bss_end = .;
+		 HIDDEN(__bss_limit = .);
 	}
 
-	.bss_end __bss_end (OVERLAY) : {
-		KEEP(*(__bss_end));
+	.bss_end __bss_limit (OVERLAY) : {
+		KEEP(*(.__bss_end));
 	}
 
 	/DISCARD/ : { *(.dynstr*) }
diff --git a/board/actux1/u-boot.lds b/board/actux1/u-boot.lds
index c76728a..7e5c4d8 100644
--- a/board/actux1/u-boot.lds
+++ b/board/actux1/u-boot.lds
@@ -74,17 +74,23 @@  SECTIONS
 
 	_end = .;
 
+/*
+ * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
+ * __bss_base and __bss_limit are for linker only (overlay ordering)
+ */
+
 	.bss_start __rel_dyn_start (OVERLAY) : {
 		KEEP(*(.__bss_start));
+		HIDDEN(__bss_base = .);
 	}
 
-	.bss __bss_start (OVERLAY) : {
+	.bss __bss_base (OVERLAY) : {
 		*(.bss*)
 		 . = ALIGN(4);
-		 __bss_end = .;
+		 HIDDEN(__bss_limit = .);
 	}
-	.bss_end __bss_end (OVERLAY) : {
-		KEEP(*(__bss_end));
+	.bss_end __bss_limit (OVERLAY) : {
+		KEEP(*(.__bss_end));
 	}
 
 	/DISCARD/ : { *(.dynstr*) }
diff --git a/board/actux2/u-boot.lds b/board/actux2/u-boot.lds
index 984f70e..ce1b7c9 100644
--- a/board/actux2/u-boot.lds
+++ b/board/actux2/u-boot.lds
@@ -74,17 +74,23 @@  SECTIONS
 
 	_end = .;
 
+/*
+ * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
+ * __bss_base and __bss_limit are for linker only (overlay ordering)
+ */
+
 	.bss_start __rel_dyn_start (OVERLAY) : {
 		KEEP(*(.__bss_start));
+		HIDDEN(__bss_base = .);
 	}
 
-	.bss __bss_start (OVERLAY) : {
+	.bss __bss_base (OVERLAY) : {
 		*(.bss*)
 		 . = ALIGN(4);
-		 __bss_end = .;
+		 HIDDEN(__bss_limit = .);
 	}
-	.bss_end __bss_end (OVERLAY) : {
-		KEEP(*(__bss_end));
+	.bss_end __bss_limit (OVERLAY) : {
+		KEEP(*(.__bss_end));
 	}
 
 	/DISCARD/ : { *(.dynstr*) }
diff --git a/board/actux3/u-boot.lds b/board/actux3/u-boot.lds
index fc48cf0..3e091dd 100644
--- a/board/actux3/u-boot.lds
+++ b/board/actux3/u-boot.lds
@@ -74,17 +74,23 @@  SECTIONS
 
 	_end = .;
 
+/*
+ * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
+ * __bss_base and __bss_limit are for linker only (overlay ordering)
+ */
+
 	.bss_start __rel_dyn_start (OVERLAY) : {
 		KEEP(*(.__bss_start));
+		HIDDEN(__bss_base = .);
 	}
 
-	.bss __bss_start (OVERLAY) : {
+	.bss __bss_base (OVERLAY) : {
 		*(.bss*)
 		 . = ALIGN(4);
-		 __bss_end = .;
+		HIDDEN(__bss_limit = .);
 	}
-	.bss_end __bss_end (OVERLAY) : {
-		KEEP(*(__bss_end));
+	.bss_end __bss_limit (OVERLAY) : {
+		KEEP(*(.__bss_end));
 	}
 
 	/DISCARD/ : { *(.dynstr*) }
diff --git a/board/dvlhost/u-boot.lds b/board/dvlhost/u-boot.lds
index b13d3e1..fe3c21b 100644
--- a/board/dvlhost/u-boot.lds
+++ b/board/dvlhost/u-boot.lds
@@ -74,17 +74,23 @@  SECTIONS
 
 	_end = .;
 
+/*
+ * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
+ * __bss_base and __bss_limit are for linker only (overlay ordering)
+ */
+
 	.bss_start __rel_dyn_start (OVERLAY) : {
 		KEEP(*(.__bss_start));
+		HIDDEN(__bss_base = .);
 	}
 
-	.bss __bss_start (OVERLAY) : {
+	.bss __bss_base (OVERLAY) : {
 		*(.bss*)
 		 . = ALIGN(4);
-		 __bss_end = .;
+		 HIDDEN(__bss_limit = .);
 	}
-	.bss_end __bss_end (OVERLAY) : {
-		KEEP(*(__bss_end));
+	.bss_end __bss_limit (OVERLAY) : {
+		KEEP(*(.__bss_end));
 	}
 
 	/DISCARD/ : { *(.dynstr*) }
diff --git a/board/freescale/mx31ads/u-boot.lds b/board/freescale/mx31ads/u-boot.lds
index 264c4e8..e6885a5 100644
--- a/board/freescale/mx31ads/u-boot.lds
+++ b/board/freescale/mx31ads/u-boot.lds
@@ -80,17 +80,23 @@  SECTIONS
 
 	_end = .;
 
+/*
+ * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c
+ * __bss_base and __bss_limit are for linker only (overlay ordering)
+ */
+
 	.bss_start __rel_dyn_start (OVERLAY) : {
 		KEEP(*(.__bss_start));
+		HIDDEN(__bss_base = .);
 	}
 
-	.bss __bss_start (OVERLAY) : {
+	.bss __bss_base (OVERLAY) : {
 		*(.bss*)
 		 . = ALIGN(4);
-		 __bss_end = .;
+		 HIDDEN(__bss_limit = .);
 	}
-	.bss_end __bss_end (OVERLAY) : {
-		KEEP(*(__bss_end));
+	.bss_end __bss_limit (OVERLAY) : {
+		KEEP(*(.__bss_end));
 	}
 
 	/DISCARD/ : { *(.bss*) }