Patchwork [U-Boot,4/4] ARM: fix CONFIG_SPL_MAX_SIZE semantics

login
register
mail settings
Submitter Albert ARIBAUD
Date April 8, 2013, 7:58 p.m.
Message ID <1365451109-22030-5-git-send-email-albert.u.boot@aribaud.net>
Download mbox | patch
Permalink /patch/234878/
State Superseded
Headers show

Comments

Albert ARIBAUD - April 8, 2013, 7:58 p.m.
The ASSERT() in arch/arm/cpu/u-boot.lds is unneeded as
this linker file is not used for SPL builds. Remove it.

The ASSERT() in arch/arm/cpu/u-boot-spl.lds is wrong
as it compares image+BSS size to max image size only.
Split it in two distinct ASSERT()s, one for image size,
one for BSS size.

Finally, update README regarding the (now homogeneous)
semantics of the CONFIG_SPL_MAX_SIZE and
CONFIG_SPL_BSS_MAX_SIZE macros.

Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
---
 README                      |   17 +++++++++++++++--
 arch/arm/cpu/u-boot-spl.lds |   10 ++++++++--
 arch/arm/cpu/u-boot.lds     |    4 ----
 3 files changed, 23 insertions(+), 8 deletions(-)
Benoît Thébaudeau - April 8, 2013, 9:43 p.m.
Hi Albert,

On Monday, April 8, 2013 9:58:29 PM, Albert ARIBAUD wrote:
> The ASSERT() in arch/arm/cpu/u-boot.lds is unneeded as
> this linker file is not used for SPL builds. Remove it.
> 
> The ASSERT() in arch/arm/cpu/u-boot-spl.lds is wrong
> as it compares image+BSS size to max image size only.
> Split it in two distinct ASSERT()s, one for image size,
> one for BSS size.
> 
> Finally, update README regarding the (now homogeneous)
> semantics of the CONFIG_SPL_MAX_SIZE and
> CONFIG_SPL_BSS_MAX_SIZE macros.
> 
> Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> ---
>  README                      |   17 +++++++++++++++--
>  arch/arm/cpu/u-boot-spl.lds |   10 ++++++++--
>  arch/arm/cpu/u-boot.lds     |    4 ----
>  3 files changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/README b/README
> index a35ef31..c902421 100644
> --- a/README
> +++ b/README
> @@ -2784,7 +2784,14 @@ FIT uImage format:
>  		LDSCRIPT for linking the SPL binary.
>  
>  		CONFIG_SPL_MAX_SIZE
> -		Maximum binary size (text, data and rodata) of the SPL binary.
> +		Maximum size for the image (sum of the text, data, rodata
> +		and linker lists sections) of the SPL executable.
> +		When defined, linker checks that the actual image size does
> +		not exceed it.
> +		The total amount of memory used by the SPL when running is
> +		equal CONFIG_SPL_MAX_SIZE, plus CONFIG_SPL_BSS_MAX_SIZE if
> +		it exists.
> +		Note: image and BSS are disjoint for some targets.
>  
>  		CONFIG_SPL_TEXT_BASE
>  		TEXT_BASE for linking the SPL binary.
> @@ -2797,7 +2804,13 @@ FIT uImage format:
>  		Link address for the BSS within the SPL binary.
>  
>  		CONFIG_SPL_BSS_MAX_SIZE
> -		Maximum binary size of the BSS section of the SPL binary.
> +		Maximum size of the BSS section of the SPL executable.
> +		When defined, linker checks that the actual BSS size does
> +		not exceed it.
> +		The total amount of memory used by the SPL when running is
> +		equal CONFIG_SPL_MAX_SIZE, plus CONFIG_SPL_BSS_MAX_SIZE if
> +		it exists.
> +		Note: image and BSS are disjoint for some targets.
>  
>  		CONFIG_SPL_STACK
>  		Adress of the start of the stack SPL will use
> diff --git a/arch/arm/cpu/u-boot-spl.lds b/arch/arm/cpu/u-boot-spl.lds
> index 3c0d99c..89ef9ce 100644
> --- a/arch/arm/cpu/u-boot-spl.lds
> +++ b/arch/arm/cpu/u-boot-spl.lds
> @@ -88,6 +88,12 @@ SECTIONS
>  	/DISCARD/ : { *(.gnu*) }
>  }
>  
> -#if defined(CONFIG_SPL_TEXT_BASE) && defined(CONFIG_SPL_MAX_SIZE)
> -ASSERT(__bss_end < (CONFIG_SPL_TEXT_BASE + CONFIG_SPL_MAX_SIZE), "SPL image
> too big");
> +#if defined(CONFIG_SPL_MAX_SIZE)
> +ASSERT(__image_copy_end - __image_copy_start < (CONFIG_SPL_MAX_SIZE), \

The possible relocation and MMU data is also part of the binary image file, so
that would be __bss_start rather than __image_copy_end above, and README should
be updated to reflect this.

> +	"SPL image too big");
> +#endif
> +
> +#if defined(CONFIG_SPL_BSS_MAX_SIZE)
> +ASSERT(__bss_end - __bss_start < (CONFIG_SPL_BSS MAX_SIZE), \
> +	"SPL image BSS too big");
>  #endif
> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> index 3a1083d..7bbc4f5 100644
> --- a/arch/arm/cpu/u-boot.lds
> +++ b/arch/arm/cpu/u-boot.lds
> @@ -101,7 +101,3 @@ SECTIONS
>  	/DISCARD/ : { *(.interp*) }
>  	/DISCARD/ : { *(.gnu*) }
>  }
> -
> -#if defined(CONFIG_SPL_TEXT_BASE) && defined(CONFIG_SPL_MAX_SIZE)
> -ASSERT(__bss_end < (CONFIG_SPL_TEXT_BASE + CONFIG_SPL_MAX_SIZE), "SPL image
> too big");
> -#endif
> --
> 1.7.10.4
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
> 

Best regards,
Benoît
Albert ARIBAUD - April 9, 2013, 2:23 p.m.
Hi Benoît,

On Mon, 8 Apr 2013 23:43:37 +0200 (CEST), Benoît Thébaudeau
<benoit.thebaudeau@advansee.com> wrote:

> Hi Albert,

> > diff --git a/arch/arm/cpu/u-boot-spl.lds b/arch/arm/cpu/u-boot-spl.lds
> > index 3c0d99c..89ef9ce 100644
> > --- a/arch/arm/cpu/u-boot-spl.lds
> > +++ b/arch/arm/cpu/u-boot-spl.lds
> > @@ -88,6 +88,12 @@ SECTIONS
> >  	/DISCARD/ : { *(.gnu*) }
> >  }
> >  
> > -#if defined(CONFIG_SPL_TEXT_BASE) && defined(CONFIG_SPL_MAX_SIZE)
> > -ASSERT(__bss_end < (CONFIG_SPL_TEXT_BASE + CONFIG_SPL_MAX_SIZE), "SPL image
> > too big");
> > +#if defined(CONFIG_SPL_MAX_SIZE)
> > +ASSERT(__image_copy_end - __image_copy_start < (CONFIG_SPL_MAX_SIZE), \
> 
> The possible relocation and MMU data is also part of the binary image file, so
> that would be __bss_start rather than __image_copy_end above, and README should
> be updated to reflect this.

Actually, mmutable is not used in any SPL; it is used only in targets
h2200, lubbock, palmtc, pxa255_idp and xaeniax, none of which use SPL.
I have just confirmed this with a MAKEALL -a arm and a grep on all map
files.

This presence of mmutable in u-boot-spl.lds is in fact an overlook
that I missed when I created this file from u-boot.lds. I have just
finished verifying that removing the mmutable section altogether does
not change a single bit to any of the 309 ARM platforms currently built
under MAKEALL -a arm.

I'll remove mmutable entries from u-boot-spl.lds in V2.

Amicalement,
Benoît Thébaudeau - April 9, 2013, 2:24 p.m.
Hi Albert,

On Tuesday, April 9, 2013 4:23:58 PM, Albert ARIBAUD wrote:
> Hi Benoît,
> 
> On Mon, 8 Apr 2013 23:43:37 +0200 (CEST), Benoît Thébaudeau
> <benoit.thebaudeau@advansee.com> wrote:
> 
> > Hi Albert,
> 
> > > diff --git a/arch/arm/cpu/u-boot-spl.lds b/arch/arm/cpu/u-boot-spl.lds
> > > index 3c0d99c..89ef9ce 100644
> > > --- a/arch/arm/cpu/u-boot-spl.lds
> > > +++ b/arch/arm/cpu/u-boot-spl.lds
> > > @@ -88,6 +88,12 @@ SECTIONS
> > >  	/DISCARD/ : { *(.gnu*) }
> > >  }
> > >  
> > > -#if defined(CONFIG_SPL_TEXT_BASE) && defined(CONFIG_SPL_MAX_SIZE)
> > > -ASSERT(__bss_end < (CONFIG_SPL_TEXT_BASE + CONFIG_SPL_MAX_SIZE), "SPL
> > > image
> > > too big");
> > > +#if defined(CONFIG_SPL_MAX_SIZE)
> > > +ASSERT(__image_copy_end - __image_copy_start < (CONFIG_SPL_MAX_SIZE), \
> > 
> > The possible relocation and MMU data is also part of the binary image file,
> > so
> > that would be __bss_start rather than __image_copy_end above, and README
> > should
> > be updated to reflect this.
> 
> Actually, mmutable is not used in any SPL; it is used only in targets
> h2200, lubbock, palmtc, pxa255_idp and xaeniax, none of which use SPL.
> I have just confirmed this with a MAKEALL -a arm and a grep on all map
> files.
> 
> This presence of mmutable in u-boot-spl.lds is in fact an overlook
> that I missed when I created this file from u-boot.lds. I have just
> finished verifying that removing the mmutable section altogether does
> not change a single bit to any of the 309 ARM platforms currently built
> under MAKEALL -a arm.
> 
> I'll remove mmutable entries from u-boot-spl.lds in V2.

OK, that's perfect for MMU data, but what about relocation data?

Best regards,
Benoît
Benoît Thébaudeau - April 9, 2013, 5:39 p.m.
Hi Albert,

On Tuesday, April 9, 2013 7:43:06 PM, Albert ARIBAUD wrote:
> Hi Benoît,
> 
> On Tue, 9 Apr 2013 16:24:36 +0200 (CEST), Benoît Thébaudeau
> <benoit.thebaudeau@advansee.com> wrote:
> 
> > Hi Albert,
> > 
> > On Tuesday, April 9, 2013 4:23:58 PM, Albert ARIBAUD wrote:
> > > Hi Benoît,
> > > 
> > > On Mon, 8 Apr 2013 23:43:37 +0200 (CEST), Benoît Thébaudeau
> > > <benoit.thebaudeau@advansee.com> wrote:
> > > 
> > > > Hi Albert,
> > > 
> > > > > diff --git a/arch/arm/cpu/u-boot-spl.lds
> > > > > b/arch/arm/cpu/u-boot-spl.lds
> > > > > index 3c0d99c..89ef9ce 100644
> > > > > --- a/arch/arm/cpu/u-boot-spl.lds
> > > > > +++ b/arch/arm/cpu/u-boot-spl.lds
> > > > > @@ -88,6 +88,12 @@ SECTIONS
> > > > >  	/DISCARD/ : { *(.gnu*) }
> > > > >  }
> > > > >  
> > > > > -#if defined(CONFIG_SPL_TEXT_BASE) && defined(CONFIG_SPL_MAX_SIZE)
> > > > > -ASSERT(__bss_end < (CONFIG_SPL_TEXT_BASE + CONFIG_SPL_MAX_SIZE),
> > > > > "SPL
> > > > > image
> > > > > too big");
> > > > > +#if defined(CONFIG_SPL_MAX_SIZE)
> > > > > +ASSERT(__image_copy_end - __image_copy_start <
> > > > > (CONFIG_SPL_MAX_SIZE), \
> > > > 
> > > > The possible relocation and MMU data is also part of the binary image
> > > > file,
> > > > so
> > > > that would be __bss_start rather than __image_copy_end above, and
> > > > README
> > > > should
> > > > be updated to reflect this.
> > > 
> > > Actually, mmutable is not used in any SPL; it is used only in targets
> > > h2200, lubbock, palmtc, pxa255_idp and xaeniax, none of which use SPL.
> > > I have just confirmed this with a MAKEALL -a arm and a grep on all map
> > > files.
> > > 
> > > This presence of mmutable in u-boot-spl.lds is in fact an overlook
> > > that I missed when I created this file from u-boot.lds. I have just
> > > finished verifying that removing the mmutable section altogether does
> > > not change a single bit to any of the 309 ARM platforms currently built
> > > under MAKEALL -a arm.
> > > 
> > > I'll remove mmutable entries from u-boot-spl.lds in V2.
> > 
> > OK, that's perfect for MMU data, but what about relocation data?
> 
> Relocation data should not exist for SPLs, which do not relocate.
> 
> Unfortunately, most tegra and some exynos have start.S code going
> through the relocation loop even for their SPL; that's cardhu,
> colibri_t20_iris, dalmore, harmony, medcom-wide, origen, paz00, plutux,
> seaboard, smdkv310, tec, trimslice, ventana, and whistler.
> 
> Tegras do it for their arm720t; Exynos' probably do something similar.
> I am not going to try and change some start.S so close in time to
> release. :)
> 
> Fortunately, for all the SPLs that fail building if I remove .rel.dyn
> and .dynsym, these sections are actually empty, i.e. their __end is
> equal to their __image_copy_end. I have manually verified both reloc
> section emptiness and equality of _end and __image_copy_end.
> 
> Therefore I'll leave the ASSERT() as written in V2, and will provide a
> separate patch for fixing the Tegra / Exynos unneeded relocation data
> issue.

That's perfect.

Best regards,
Benoît
Albert ARIBAUD - April 9, 2013, 5:43 p.m.
Hi Benoît,

On Tue, 9 Apr 2013 16:24:36 +0200 (CEST), Benoît Thébaudeau
<benoit.thebaudeau@advansee.com> wrote:

> Hi Albert,
> 
> On Tuesday, April 9, 2013 4:23:58 PM, Albert ARIBAUD wrote:
> > Hi Benoît,
> > 
> > On Mon, 8 Apr 2013 23:43:37 +0200 (CEST), Benoît Thébaudeau
> > <benoit.thebaudeau@advansee.com> wrote:
> > 
> > > Hi Albert,
> > 
> > > > diff --git a/arch/arm/cpu/u-boot-spl.lds b/arch/arm/cpu/u-boot-spl.lds
> > > > index 3c0d99c..89ef9ce 100644
> > > > --- a/arch/arm/cpu/u-boot-spl.lds
> > > > +++ b/arch/arm/cpu/u-boot-spl.lds
> > > > @@ -88,6 +88,12 @@ SECTIONS
> > > >  	/DISCARD/ : { *(.gnu*) }
> > > >  }
> > > >  
> > > > -#if defined(CONFIG_SPL_TEXT_BASE) && defined(CONFIG_SPL_MAX_SIZE)
> > > > -ASSERT(__bss_end < (CONFIG_SPL_TEXT_BASE + CONFIG_SPL_MAX_SIZE), "SPL
> > > > image
> > > > too big");
> > > > +#if defined(CONFIG_SPL_MAX_SIZE)
> > > > +ASSERT(__image_copy_end - __image_copy_start < (CONFIG_SPL_MAX_SIZE), \
> > > 
> > > The possible relocation and MMU data is also part of the binary image file,
> > > so
> > > that would be __bss_start rather than __image_copy_end above, and README
> > > should
> > > be updated to reflect this.
> > 
> > Actually, mmutable is not used in any SPL; it is used only in targets
> > h2200, lubbock, palmtc, pxa255_idp and xaeniax, none of which use SPL.
> > I have just confirmed this with a MAKEALL -a arm and a grep on all map
> > files.
> > 
> > This presence of mmutable in u-boot-spl.lds is in fact an overlook
> > that I missed when I created this file from u-boot.lds. I have just
> > finished verifying that removing the mmutable section altogether does
> > not change a single bit to any of the 309 ARM platforms currently built
> > under MAKEALL -a arm.
> > 
> > I'll remove mmutable entries from u-boot-spl.lds in V2.
> 
> OK, that's perfect for MMU data, but what about relocation data?

Relocation data should not exist for SPLs, which do not relocate.

Unfortunately, most tegra and some exynos have start.S code going
through the relocation loop even for their SPL; that's cardhu,
colibri_t20_iris, dalmore, harmony, medcom-wide, origen, paz00, plutux,
seaboard, smdkv310, tec, trimslice, ventana, and whistler.

Tegras do it for their arm720t; Exynos' probably do something similar.
I am not going to try and change some start.S so close in time to
release. :)

Fortunately, for all the SPLs that fail building if I remove .rel.dyn
and .dynsym, these sections are actually empty, i.e. their __end is
equal to their __image_copy_end. I have manually verified both reloc
section emptiness and equality of _end and __image_copy_end.

Therefore I'll leave the ASSERT() as written in V2, and will provide a
separate patch for fixing the Tegra / Exynos unneeded relocation data
issue.

> Best regards,
> Benoît

Amicalement,

Patch

diff --git a/README b/README
index a35ef31..c902421 100644
--- a/README
+++ b/README
@@ -2784,7 +2784,14 @@  FIT uImage format:
 		LDSCRIPT for linking the SPL binary.
 
 		CONFIG_SPL_MAX_SIZE
-		Maximum binary size (text, data and rodata) of the SPL binary.
+		Maximum size for the image (sum of the text, data, rodata
+		and linker lists sections) of the SPL executable.
+		When defined, linker checks that the actual image size does
+		not exceed it.
+		The total amount of memory used by the SPL when running is
+		equal CONFIG_SPL_MAX_SIZE, plus CONFIG_SPL_BSS_MAX_SIZE if
+		it exists.
+		Note: image and BSS are disjoint for some targets.
 
 		CONFIG_SPL_TEXT_BASE
 		TEXT_BASE for linking the SPL binary.
@@ -2797,7 +2804,13 @@  FIT uImage format:
 		Link address for the BSS within the SPL binary.
 
 		CONFIG_SPL_BSS_MAX_SIZE
-		Maximum binary size of the BSS section of the SPL binary.
+		Maximum size of the BSS section of the SPL executable.
+		When defined, linker checks that the actual BSS size does
+		not exceed it.
+		The total amount of memory used by the SPL when running is
+		equal CONFIG_SPL_MAX_SIZE, plus CONFIG_SPL_BSS_MAX_SIZE if
+		it exists.
+		Note: image and BSS are disjoint for some targets.
 
 		CONFIG_SPL_STACK
 		Adress of the start of the stack SPL will use
diff --git a/arch/arm/cpu/u-boot-spl.lds b/arch/arm/cpu/u-boot-spl.lds
index 3c0d99c..89ef9ce 100644
--- a/arch/arm/cpu/u-boot-spl.lds
+++ b/arch/arm/cpu/u-boot-spl.lds
@@ -88,6 +88,12 @@  SECTIONS
 	/DISCARD/ : { *(.gnu*) }
 }
 
-#if defined(CONFIG_SPL_TEXT_BASE) && defined(CONFIG_SPL_MAX_SIZE)
-ASSERT(__bss_end < (CONFIG_SPL_TEXT_BASE + CONFIG_SPL_MAX_SIZE), "SPL image too big");
+#if defined(CONFIG_SPL_MAX_SIZE)
+ASSERT(__image_copy_end - __image_copy_start < (CONFIG_SPL_MAX_SIZE), \
+	"SPL image too big");
+#endif
+
+#if defined(CONFIG_SPL_BSS_MAX_SIZE)
+ASSERT(__bss_end - __bss_start < (CONFIG_SPL_BSS MAX_SIZE), \
+	"SPL image BSS too big");
 #endif
diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
index 3a1083d..7bbc4f5 100644
--- a/arch/arm/cpu/u-boot.lds
+++ b/arch/arm/cpu/u-boot.lds
@@ -101,7 +101,3 @@  SECTIONS
 	/DISCARD/ : { *(.interp*) }
 	/DISCARD/ : { *(.gnu*) }
 }
-
-#if defined(CONFIG_SPL_TEXT_BASE) && defined(CONFIG_SPL_MAX_SIZE)
-ASSERT(__bss_end < (CONFIG_SPL_TEXT_BASE + CONFIG_SPL_MAX_SIZE), "SPL image too big");
-#endif