| Message ID | 1365549295-9388-5-git-send-email-albert.u.boot@aribaud.net |
|---|---|
| State | Superseded |
| Headers | show |
On 04/09/2013 05:14 PM, Albert ARIBAUD wrote: > Remove SPL-related ASSERT() in arch/arm/cpu/u-boot.lds > as this file is never used for SPL builds. > > Rewrite the ASSERT() in arch/arm/cpu/u-boot-spl.lds > to separately test image (text,data,rodata...) size > and BSS size each against its own max. > > Also, output section mmutable is not used in SPL builds. > Remove it. > > Finally, update README regarding the (now homogeneous) > semantics of the CONFIG_SPL_MAX_SIZE and > CONFIG_SPL_BSS_MAX_SIZE macros. This still seems to have separate defines for SPL text/data/rodata size and BSS size. If I want instead to limit the total text/data/rodata/bss size, but place no specific limit on the bss size individually, can I not do that?
Hi Stephen, On Wed, 10 Apr 2013 16:21:54 -0600, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 04/09/2013 05:14 PM, Albert ARIBAUD wrote: > > Remove SPL-related ASSERT() in arch/arm/cpu/u-boot.lds > > as this file is never used for SPL builds. > > > > Rewrite the ASSERT() in arch/arm/cpu/u-boot-spl.lds > > to separately test image (text,data,rodata...) size > > and BSS size each against its own max. > > > > Also, output section mmutable is not used in SPL builds. > > Remove it. > > > > Finally, update README regarding the (now homogeneous) > > semantics of the CONFIG_SPL_MAX_SIZE and > > CONFIG_SPL_BSS_MAX_SIZE macros. > > This still seems to have separate defines for SPL text/data/rodata size > and BSS size. If I want instead to limit the total text/data/rodata/bss > size, but place no specific limit on the bss size individually, can I > not do that? This would defeat the purpose of giving CONFIG_SPL_MAX_SIZE a constant meaning -- one of the issues which prompted this patch series is that in ARM sometime CONFIG_SPL_MAX_SIZE meant BSS included, and sometime excluded, and this inconsistency had to be resolved. As in the rest of U-boot, CONFIG_SPL_MAX_SIZE was meant BSS excluded, this is the semantics that was decided. What we could do, though, is subdivide testing based on the existence or non-existence of CONFIG_SPL_BSS_START_ADDR: - if CONFIG_SPL_BSS_START_ADDR exists, then we assume SPL image and BSS are disjoint and we test each one against its max size, as this patch series does; - if CONFIG_SPL_BSS_START_ADDR does not exist, then we assume SPL image and BSS are contiguous and we test the whole of SPL against the sum of CONFIG_SPL_MAX_SIZE and CONFIG_SPL_BSS_MAX_SIZE. I guess this will be considered useless complication -- after all, once you have artificially partitioned your SPL space into image+BSS -- and you know from the build command how much should be allotted to each of them -- the worst that can happen is that a later build fails with an explicit error message forcing you to look at current image and BSS size and adjust one or both of the max values accordingly. Amicalement,
On 04/10/2013 04:50 PM, Albert ARIBAUD wrote: > On Wed, 10 Apr 2013 16:21:54 -0600, Stephen Warren wrote: >> On 04/09/2013 05:14 PM, Albert ARIBAUD wrote: ... >> This still seems to have separate defines for SPL text/data/rodata size >> and BSS size. If I want instead to limit the total text/data/rodata/bss >> size, but place no specific limit on the bss size individually, can I >> not do that? > > This would defeat the purpose of giving CONFIG_SPL_MAX_SIZE a constant > meaning -- one of the issues which prompted this patch series is that > in ARM sometime CONFIG_SPL_MAX_SIZE meant BSS included, and sometime > excluded, and this inconsistency had to be resolved. As in the rest of > U-boot, CONFIG_SPL_MAX_SIZE was meant BSS excluded, this is the > semantics that was decided. > > What we could do, though, is subdivide testing based on the existence or > non-existence of CONFIG_SPL_BSS_START_ADDR: > > - if CONFIG_SPL_BSS_START_ADDR exists, then we assume SPL image and > BSS are disjoint and we test each one against its max size, as this > patch series does; > > - if CONFIG_SPL_BSS_START_ADDR does not exist, then we assume SPL image > and BSS are contiguous and we test the whole of SPL against the sum > of CONFIG_SPL_MAX_SIZE and CONFIG_SPL_BSS_MAX_SIZE. Why not either: a) define CONFIG_SPL_MAX_SIZE to include the BSS size if CONFIG_SPL_BSS_START_ADDR is not set, but exclude it if it is. or: b) use 3 defines instead of 2, so that CONFIG_SPL_MAX_SIZE (if defined) always limits text+rodata+data, CONFIG_SPL_MAX_FOOTPRINT (if defined) always limits text+rodata+data+bss, and CONFIG_SPL_BSS_MAX_SIZE (if defined) always limits bss size. Tegra would define only CONFIG_SPL_MAX_FOOTPRINT in this scheme. Other boards would presumably define other combinations of those.
On Thu, 11 Apr 2013 00:50:01 +0200, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote: > What we could do, though, is subdivide testing based on the existence or > non-existence of CONFIG_SPL_BSS_START_ADDR: > > - if CONFIG_SPL_BSS_START_ADDR exists, then we assume SPL image and > BSS are disjoint and we test each one against its max size, as this > patch series does; > > - if CONFIG_SPL_BSS_START_ADDR does not exist, then we assume SPL image > and BSS are contiguous and we test the whole of SPL against the sum > of CONFIG_SPL_MAX_SIZE and CONFIG_SPL_BSS_MAX_SIZE. > > I guess this will be considered useless complication -- after all, > once you have artificially partitioned your SPL space into image+BSS -- > and you know from the build command how much should be allotted to each > of them -- the worst that can happen is that a later build fails with > an explicit error message forcing you to look at current image and BSS > size and adjust one or both of the max values accordingly. P.S. In any case, the proposal above will go in, if at all, as a separate patch; the current patch series is going in right now as it is. Amicalement,
On 04/10/2013 05:09 PM, Albert ARIBAUD wrote: > On Thu, 11 Apr 2013 00:50:01 +0200, Albert ARIBAUD > <albert.u.boot@aribaud.net> wrote: > >> What we could do, though, is subdivide testing based on the existence or >> non-existence of CONFIG_SPL_BSS_START_ADDR: >> >> - if CONFIG_SPL_BSS_START_ADDR exists, then we assume SPL image and >> BSS are disjoint and we test each one against its max size, as this >> patch series does; >> >> - if CONFIG_SPL_BSS_START_ADDR does not exist, then we assume SPL image >> and BSS are contiguous and we test the whole of SPL against the sum >> of CONFIG_SPL_MAX_SIZE and CONFIG_SPL_BSS_MAX_SIZE. >> >> I guess this will be considered useless complication -- after all, >> once you have artificially partitioned your SPL space into image+BSS -- >> and you know from the build command how much should be allotted to each >> of them -- the worst that can happen is that a later build fails with >> an explicit error message forcing you to look at current image and BSS >> size and adjust one or both of the max values accordingly. > > P.S. In any case, the proposal above will go in, if at all, as a > separate patch; the current patch series is going in right now as it is. I wonder what the point of code-review is if you're just going to ignore it. What's really odd here is that by my reading of the relevant threads, TomR already pointed out this exact issue earlier on, and you had agreed that you'd resolve it in a way that didn't have this issue, yet the patch has this issue???
Hi Stephen, On Wed, 10 Apr 2013 17:09:45 -0600, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 04/10/2013 04:50 PM, Albert ARIBAUD wrote: > > On Wed, 10 Apr 2013 16:21:54 -0600, Stephen Warren wrote: > >> On 04/09/2013 05:14 PM, Albert ARIBAUD wrote: > ... > >> This still seems to have separate defines for SPL text/data/rodata size > >> and BSS size. If I want instead to limit the total text/data/rodata/bss > >> size, but place no specific limit on the bss size individually, can I > >> not do that? > > > > This would defeat the purpose of giving CONFIG_SPL_MAX_SIZE a constant > > meaning -- one of the issues which prompted this patch series is that > > in ARM sometime CONFIG_SPL_MAX_SIZE meant BSS included, and sometime > > excluded, and this inconsistency had to be resolved. As in the rest of > > U-boot, CONFIG_SPL_MAX_SIZE was meant BSS excluded, this is the > > semantics that was decided. > > > > What we could do, though, is subdivide testing based on the existence or > > non-existence of CONFIG_SPL_BSS_START_ADDR: > > > > - if CONFIG_SPL_BSS_START_ADDR exists, then we assume SPL image and > > BSS are disjoint and we test each one against its max size, as this > > patch series does; > > > > - if CONFIG_SPL_BSS_START_ADDR does not exist, then we assume SPL image > > and BSS are contiguous and we test the whole of SPL against the sum > > of CONFIG_SPL_MAX_SIZE and CONFIG_SPL_BSS_MAX_SIZE. > > Why not either: > > a) define CONFIG_SPL_MAX_SIZE to include the BSS size if > CONFIG_SPL_BSS_START_ADDR is not set, but exclude it if it is. That was in my original proposals, and it was spoken against: <http://article.gmane.org/gmane.comp.boot-loaders.u-boot/158073> <http://article.gmane.org/gmane.comp.boot-loaders.u-boot/158094> > or: > > b) use 3 defines instead of 2, so that CONFIG_SPL_MAX_SIZE (if defined) > always limits text+rodata+data, CONFIG_SPL_MAX_FOOTPRINT (if defined) > always limits text+rodata+data+bss, and CONFIG_SPL_BSS_MAX_SIZE (if > defined) always limits bss size. > > Tegra would define only CONFIG_SPL_MAX_FOOTPRINT in this scheme. > Other boards would presumably define other combinations of those. That is a possibility, with the minor nitpick that we could possibly end up having conflicting values for the three symbols, whereas with the CONFIG_SPL_BSS_START_ADDR solution, there can be no value conflict. Amicalement,
Hi Stephen, On Wed, 10 Apr 2013 17:16:49 -0600, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 04/10/2013 05:09 PM, Albert ARIBAUD wrote: > > On Thu, 11 Apr 2013 00:50:01 +0200, Albert ARIBAUD > > <albert.u.boot@aribaud.net> wrote: > > > >> What we could do, though, is subdivide testing based on the existence or > >> non-existence of CONFIG_SPL_BSS_START_ADDR: > >> > >> - if CONFIG_SPL_BSS_START_ADDR exists, then we assume SPL image and > >> BSS are disjoint and we test each one against its max size, as this > >> patch series does; > >> > >> - if CONFIG_SPL_BSS_START_ADDR does not exist, then we assume SPL image > >> and BSS are contiguous and we test the whole of SPL against the sum > >> of CONFIG_SPL_MAX_SIZE and CONFIG_SPL_BSS_MAX_SIZE. > >> > >> I guess this will be considered useless complication -- after all, > >> once you have artificially partitioned your SPL space into image+BSS -- > >> and you know from the build command how much should be allotted to each > >> of them -- the worst that can happen is that a later build fails with > >> an explicit error message forcing you to look at current image and BSS > >> size and adjust one or both of the max values accordingly. > > > > P.S. In any case, the proposal above will go in, if at all, as a > > separate patch; the current patch series is going in right now as it is. > > I wonder what the point of code-review is if you're just going to ignore it. Can we please avoid this kind of talk? It has no argumentative value and can only lead to conflicts. Your account below is sufficient to convey your argumentation without any of the potential ill-effects of the above. > What's really odd here is that by my reading of the relevant threads, > TomR already pointed out this exact issue earlier on, and you had agreed > that you'd resolve it in a way that didn't have this issue, yet the > patch has this issue??? Then our reading of the thread do not agree. Here is mine: - in <http://article.gmane.org/gmane.comp.boot-loaders.u-boot/158046>, Tom clearly asks for separate text+data+rodata size on one hand and BSS size on the other hand (his #2 case, which he wants applied uniformally and a solution found for Tegra. - in <http://article.gmane.org/gmane.comp.boot-loaders.u-boot/158073> I replied to Tom with a proposal in two parts, the first implementing his #2 case strictly, the second implementing case #1 at the cost of some minor added complexity and of muddying the symbol's semantics; I suggested that if Tom really did not want the second part of my proposal, it could be dropped and only the first part implemented. - in <http://article.gmane.org/gmane.comp.boot-loaders.u-boot/158094>, Tom asked that I keep part 1 and drop part 2 -- which I did. Additionally, I did ask Tom on IRC if V2 was ok with him, and had his agreement. Amicalement,
On Thu, Apr 11, 2013 at 04:32:53PM +0200, Albert ARIBAUD wrote: > Hi Stephen, > > On Wed, 10 Apr 2013 17:16:49 -0600, Stephen Warren > <swarren@wwwdotorg.org> wrote: > > > On 04/10/2013 05:09 PM, Albert ARIBAUD wrote: > > > On Thu, 11 Apr 2013 00:50:01 +0200, Albert ARIBAUD > > > <albert.u.boot@aribaud.net> wrote: > > > > > >> What we could do, though, is subdivide testing based on the existence or > > >> non-existence of CONFIG_SPL_BSS_START_ADDR: > > >> > > >> - if CONFIG_SPL_BSS_START_ADDR exists, then we assume SPL image and > > >> BSS are disjoint and we test each one against its max size, as this > > >> patch series does; > > >> > > >> - if CONFIG_SPL_BSS_START_ADDR does not exist, then we assume SPL image > > >> and BSS are contiguous and we test the whole of SPL against the sum > > >> of CONFIG_SPL_MAX_SIZE and CONFIG_SPL_BSS_MAX_SIZE. > > >> > > >> I guess this will be considered useless complication -- after all, > > >> once you have artificially partitioned your SPL space into image+BSS -- > > >> and you know from the build command how much should be allotted to each > > >> of them -- the worst that can happen is that a later build fails with > > >> an explicit error message forcing you to look at current image and BSS > > >> size and adjust one or both of the max values accordingly. > > > > > > P.S. In any case, the proposal above will go in, if at all, as a > > > separate patch; the current patch series is going in right now as it is. > > > > I wonder what the point of code-review is if you're just going to ignore it. > > Can we please avoid this kind of talk? It has no argumentative value > and can only lead to conflicts. Your account below is sufficient to > convey your argumentation without any of the potential ill-effects of > the above. > > > What's really odd here is that by my reading of the relevant threads, > > TomR already pointed out this exact issue earlier on, and you had agreed > > that you'd resolve it in a way that didn't have this issue, yet the > > patch has this issue??? > > Then our reading of the thread do not agree. Here is mine: > > - in <http://article.gmane.org/gmane.comp.boot-loaders.u-boot/158046>, > Tom clearly asks for separate text+data+rodata size on one hand and > BSS size on the other hand (his #2 case, which he wants applied > uniformally and a solution found for Tegra. > > - in <http://article.gmane.org/gmane.comp.boot-loaders.u-boot/158073> I > replied to Tom with a proposal in two parts, the first implementing > his #2 case strictly, the second implementing case #1 at the cost of > some minor added complexity and of muddying the symbol's semantics; I > suggested that if Tom really did not want the second part of my > proposal, it could be dropped and only the first part implemented. > > - in <http://article.gmane.org/gmane.comp.boot-loaders.u-boot/158094>, > Tom asked that I keep part 1 and drop part 2 -- which I did. > > Additionally, I did ask Tom on IRC if V2 was ok with him, and had his > agreement. To be clear, I think the fake partitioning scheme we use when the case is "we care about text/data/rodata/bss fitting in $X" is not optimal, but it covers all of the cases without adding more complexity / another way to achieve the same ends. If it turns out that platforms end up needing to re-tweak this value we can come back and say it's time to add a different mechanism for this hard-limit case.
On 04/11/2013 10:08 AM, Tom Rini wrote: ... > To be clear, I think the fake partitioning scheme we use when the > case is "we care about text/data/rodata/bss fitting in $X" is not > optimal, but it covers all of the cases without adding more > complexity / another way to achieve the same ends. But it does add more complexity; somebody has to pick, and potentially keep adjusting, the split between text/rodata/data and bss, even though they don't care about that split, but it's an implementation wart. It's also trivial to fix it properly as I described. But, if it absolutely has to be that way, then OK. However, I would ask that at least the Tegra board configuration files be set up so that the previous overall restriction (bss doesn't overlap the main U-Boot) be left in tact, so we don't have to debug that problem again.
Hi Stephen, On Wed, 10 Apr 2013 17:09:45 -0600, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 04/10/2013 04:50 PM, Albert ARIBAUD wrote: > > On Wed, 10 Apr 2013 16:21:54 -0600, Stephen Warren wrote: > >> On 04/09/2013 05:14 PM, Albert ARIBAUD wrote: > ... > >> This still seems to have separate defines for SPL text/data/rodata size > >> and BSS size. If I want instead to limit the total text/data/rodata/bss > >> size, but place no specific limit on the bss size individually, can I > >> not do that? > > > > This would defeat the purpose of giving CONFIG_SPL_MAX_SIZE a constant > > meaning -- one of the issues which prompted this patch series is that > > in ARM sometime CONFIG_SPL_MAX_SIZE meant BSS included, and sometime > > excluded, and this inconsistency had to be resolved. As in the rest of > > U-boot, CONFIG_SPL_MAX_SIZE was meant BSS excluded, this is the > > semantics that was decided. > > > > What we could do, though, is subdivide testing based on the existence or > > non-existence of CONFIG_SPL_BSS_START_ADDR: > > > > - if CONFIG_SPL_BSS_START_ADDR exists, then we assume SPL image and > > BSS are disjoint and we test each one against its max size, as this > > patch series does; > > > > - if CONFIG_SPL_BSS_START_ADDR does not exist, then we assume SPL image > > and BSS are contiguous and we test the whole of SPL against the sum > > of CONFIG_SPL_MAX_SIZE and CONFIG_SPL_BSS_MAX_SIZE. > > Why not either: > > a) define CONFIG_SPL_MAX_SIZE to include the BSS size if > CONFIG_SPL_BSS_START_ADDR is not set, but exclude it if it is. > > or: > > b) use 3 defines instead of 2, so that CONFIG_SPL_MAX_SIZE (if defined) > always limits text+rodata+data, CONFIG_SPL_MAX_FOOTPRINT (if defined) > always limits text+rodata+data+bss, and CONFIG_SPL_BSS_MAX_SIZE (if > defined) always limits bss size. > > Tegra would define only CONFIG_SPL_MAX_FOOTPRINT in this scheme. Other > boards would presumably define other combinations of those. Tom R on IRC showed preference for CONFIG_SPL_MAX_FOOTPRINT -- I'll send out a V3 with this solution. Amicalement,
diff --git a/README b/README index 5c5cd18..f31ded0 100644 --- a/README +++ b/README @@ -2776,7 +2776,14 @@ FIT uImage format: LDSCRIPT for linking the SPL binary. CONFIG_SPL_MAX_SIZE - Maximum binary size (text, data and rodata) of the SPL binary. + Maximum size for the image (sum of the text, data, rodata + and linker lists sections) of the SPL executable. + When defined, linker checks that the actual image size does + not exceed it. + The total amount of memory used by the SPL when running is + equal CONFIG_SPL_MAX_SIZE, plus CONFIG_SPL_BSS_MAX_SIZE if + it exists. + Note: image and BSS are disjoint for some targets. CONFIG_SPL_TEXT_BASE TEXT_BASE for linking the SPL binary. @@ -2789,7 +2796,13 @@ FIT uImage format: Link address for the BSS within the SPL binary. CONFIG_SPL_BSS_MAX_SIZE - Maximum binary size of the BSS section of the SPL binary. + Maximum size of the BSS section of the SPL executable. + When defined, linker checks that the actual BSS size does + not exceed it. + The total amount of memory used by the SPL when running is + equal CONFIG_SPL_MAX_SIZE, plus CONFIG_SPL_BSS_MAX_SIZE if + it exists. + Note: image and BSS are disjoint for some targets. CONFIG_SPL_STACK Adress of the start of the stack SPL will use diff --git a/arch/arm/cpu/u-boot-spl.lds b/arch/arm/cpu/u-boot-spl.lds index 3c0d99c..2e86de4 100644 --- a/arch/arm/cpu/u-boot-spl.lds +++ b/arch/arm/cpu/u-boot-spl.lds @@ -65,15 +65,6 @@ SECTIONS _end = .; - /* - * Deprecated: this MMU section is used by pxa at present but - * should not be used by new boards/CPUs. - */ - . = ALIGN(4096); - .mmutable : { - *(.mmutable) - } - .bss __rel_dyn_start (OVERLAY) : { __bss_start = .; *(.bss*) @@ -88,6 +79,12 @@ SECTIONS /DISCARD/ : { *(.gnu*) } } -#if defined(CONFIG_SPL_TEXT_BASE) && defined(CONFIG_SPL_MAX_SIZE) -ASSERT(__bss_end < (CONFIG_SPL_TEXT_BASE + CONFIG_SPL_MAX_SIZE), "SPL image too big"); +#if defined(CONFIG_SPL_MAX_SIZE) +ASSERT(__image_copy_end - __image_copy_start < (CONFIG_SPL_MAX_SIZE), \ + "SPL image too big"); +#endif + +#if defined(CONFIG_SPL_BSS_MAX_SIZE) +ASSERT(__bss_end - __bss_start < (CONFIG_SPL_BSS MAX_SIZE), \ + "SPL image BSS too big"); #endif diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds index 3a1083d..7bbc4f5 100644 --- a/arch/arm/cpu/u-boot.lds +++ b/arch/arm/cpu/u-boot.lds @@ -101,7 +101,3 @@ SECTIONS /DISCARD/ : { *(.interp*) } /DISCARD/ : { *(.gnu*) } } - -#if defined(CONFIG_SPL_TEXT_BASE) && defined(CONFIG_SPL_MAX_SIZE) -ASSERT(__bss_end < (CONFIG_SPL_TEXT_BASE + CONFIG_SPL_MAX_SIZE), "SPL image too big"); -#endif
Remove SPL-related ASSERT() in arch/arm/cpu/u-boot.lds as this file is never used for SPL builds. Rewrite the ASSERT() in arch/arm/cpu/u-boot-spl.lds to separately test image (text,data,rodata...) size and BSS size each against its own max. Also, output section mmutable is not used in SPL builds. Remove it. Finally, update README regarding the (now homogeneous) semantics of the CONFIG_SPL_MAX_SIZE and CONFIG_SPL_BSS_MAX_SIZE macros. Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net> Reported-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> --- Changes in v2: - removed mmutable in SPL linker file README | 17 +++++++++++++++-- arch/arm/cpu/u-boot-spl.lds | 19 ++++++++----------- arch/arm/cpu/u-boot.lds | 4 ---- 3 files changed, 23 insertions(+), 17 deletions(-)