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

[Albert ARIBAUD]

Hi Tom,

On Fri, 5 Apr 2013 15:44:08 -0400, Tom Rini <trini at 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 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.

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,
-- 
Albert.