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

[Tom Rini]

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 at 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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130405/11bc39e3/attachment.pgp>