| Message ID | 1365113633-31281-1-git-send-email-albert.u.boot@aribaud.net |
|---|---|
| State | Changes Requested |
| Delegated to: | Albert ARIBAUD |
| Headers | show |
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
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,
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
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,
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
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,
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.
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
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.
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.
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
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.
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,
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,
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.
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,
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!
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,
-----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-----
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,
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,
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,
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. >
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*) }
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(-)