| Message ID | 1482762041-8416-2-git-send-email-oded.gabbay@gmail.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
On Mon, Dec 26, 2016 at 04:20:41PM +0200, Oded Gabbay wrote: > This patch copies from arm u-boot-spl.lds some asserts that check that the > size of the SPL image and BSS doesn't violate their max size. > > Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com> > Cc: Albert Aribaud <albert.u.boot@aribaud.net> This shows a few problems with the uniphier platforms so I'm not applying this currently. Masahiro, can you take a look please? Thanks! https://travis-ci.org/trini/u-boot/jobs/190080917
2016-12-26 23:20 GMT+09:00 Oded Gabbay <oded.gabbay@gmail.com>: > This patch copies from arm u-boot-spl.lds some asserts that check that the > size of the SPL image and BSS doesn't violate their max size. > > Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com> > Cc: Albert Aribaud <albert.u.boot@aribaud.net> > --- > arch/arm/cpu/armv8/u-boot-spl.lds | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds > index e7799cc..0876a94 100644 > --- a/arch/arm/cpu/armv8/u-boot-spl.lds > +++ b/arch/arm/cpu/armv8/u-boot-spl.lds > @@ -78,3 +78,22 @@ SECTIONS > /DISCARD/ : { *(.interp*) } > /DISCARD/ : { *(.gnu*) } > } > + > +#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) && defined(CONFIG_SPL_MAX_FOOTPRINT) > +ASSERT(0, "CONFIG_SPL_BSS_MAX_SIZE and CONFIG_SPL_MAX_FOOTPRINT must not be defined together"); > +#endif > + > +#if defined(CONFIG_SPL_BSS_MAX_SIZE) > +ASSERT(__bss_end - __bss_start < (CONFIG_SPL_BSS_MAX_SIZE), \ > + "SPL image BSS too big"); > +#endif > + > +#if defined(CONFIG_SPL_MAX_FOOTPRINT) > +ASSERT(__bss_end - _start < (CONFIG_SPL_MAX_FOOTPRINT), \ > + "SPL image plus BSS too big"); > +#endif > -- > 2.7.4 > This patch is wrong in multiple ways. See the top lines of this file. MEMORY { .sram : ORIGIN = CONFIG_SPL_TEXT_BASE, LENGTH = CONFIG_SPL_MAX_SIZE } MEMORY { .sdram : ORIGIN = CONFIG_SPL_BSS_START_ADDR, LENGTH = CONFIG_SPL_BSS_MAX_SIZE } This means CONFIG_SPL_BSS_MAX_SIZE must always be defined. Your code > +#if defined(CONFIG_SPL_BSS_MAX_SIZE) && defined(CONFIG_SPL_MAX_FOOTPRINT) > +ASSERT(0, "CONFIG_SPL_BSS_MAX_SIZE and CONFIG_SPL_MAX_FOOTPRINT must not be defined together"); > +#endif ... means CONFIG_SPL_BSS_MAX_FOOTPRINT can never be defined. As a result, > +#if defined(CONFIG_SPL_BSS_MAX_SIZE) > +ASSERT(__bss_end - __bss_start < (CONFIG_SPL_BSS_MAX_SIZE), \ > + "SPL image BSS too big"); > +#endif the ifdef is always true, so meaningless. > +ASSERT(__bss_end - __bss_start < (CONFIG_SPL_BSS_MAX_SIZE), \ > + "SPL image BSS too big"); is already size-checked by the following: MEMORY { .sdram : ORIGIN = CONFIG_SPL_BSS_START_ADDR, LENGTH = CONFIG_SPL_BSS_MAX_SIZE } so, meaningless. > +#if defined(CONFIG_SPL_MAX_FOOTPRINT) > +ASSERT(__bss_end - _start < (CONFIG_SPL_MAX_FOOTPRINT), \ > + "SPL image plus BSS too big"); > +#endif This code is never enabled.
On Tue, Jan 17, 2017 at 9:55 AM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > 2016-12-26 23:20 GMT+09:00 Oded Gabbay <oded.gabbay@gmail.com>: >> This patch copies from arm u-boot-spl.lds some asserts that check that the >> size of the SPL image and BSS doesn't violate their max size. >> >> Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com> >> Cc: Albert Aribaud <albert.u.boot@aribaud.net> >> --- >> arch/arm/cpu/armv8/u-boot-spl.lds | 19 +++++++++++++++++++ >> 1 file changed, 19 insertions(+) >> >> diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds >> index e7799cc..0876a94 100644 >> --- a/arch/arm/cpu/armv8/u-boot-spl.lds >> +++ b/arch/arm/cpu/armv8/u-boot-spl.lds >> @@ -78,3 +78,22 @@ SECTIONS >> /DISCARD/ : { *(.interp*) } >> /DISCARD/ : { *(.gnu*) } >> } >> + >> +#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) && defined(CONFIG_SPL_MAX_FOOTPRINT) >> +ASSERT(0, "CONFIG_SPL_BSS_MAX_SIZE and CONFIG_SPL_MAX_FOOTPRINT must not be defined together"); >> +#endif >> + >> +#if defined(CONFIG_SPL_BSS_MAX_SIZE) >> +ASSERT(__bss_end - __bss_start < (CONFIG_SPL_BSS_MAX_SIZE), \ >> + "SPL image BSS too big"); >> +#endif >> + >> +#if defined(CONFIG_SPL_MAX_FOOTPRINT) >> +ASSERT(__bss_end - _start < (CONFIG_SPL_MAX_FOOTPRINT), \ >> + "SPL image plus BSS too big"); >> +#endif >> -- >> 2.7.4 >> > > This patch is wrong in multiple ways. > > > > See the top lines of this file. > > MEMORY { .sram : ORIGIN = CONFIG_SPL_TEXT_BASE, > LENGTH = CONFIG_SPL_MAX_SIZE } > MEMORY { .sdram : ORIGIN = CONFIG_SPL_BSS_START_ADDR, > LENGTH = CONFIG_SPL_BSS_MAX_SIZE } > > This means CONFIG_SPL_BSS_MAX_SIZE must always be defined. > > > > > Your code > >> +#if defined(CONFIG_SPL_BSS_MAX_SIZE) && defined(CONFIG_SPL_MAX_FOOTPRINT) >> +ASSERT(0, "CONFIG_SPL_BSS_MAX_SIZE and CONFIG_SPL_MAX_FOOTPRINT must not be defined together"); >> +#endif > > ... means CONFIG_SPL_BSS_MAX_FOOTPRINT can never be defined. > Correct. According to the README: "CONFIG_SPL_MAX_FOOTPRINT and CONFIG_SPL_BSS_MAX_SIZE must not be both defined at the same time." So this assert checks this. If this is wrong, then we should fix the README instead. > > As a result, > >> +#if defined(CONFIG_SPL_BSS_MAX_SIZE) >> +ASSERT(__bss_end - __bss_start < (CONFIG_SPL_BSS_MAX_SIZE), \ >> + "SPL image BSS too big"); >> +#endif > > the ifdef is always true, so meaningless. Correct, we can remove this #ifdef > >> +ASSERT(__bss_end - __bss_start < (CONFIG_SPL_BSS_MAX_SIZE), \ >> + "SPL image BSS too big"); > > is already size-checked by the following: > > MEMORY { .sdram : ORIGIN = CONFIG_SPL_BSS_START_ADDR, > LENGTH = CONFIG_SPL_BSS_MAX_SIZE } > > > so, meaningless. > > >> +#if defined(CONFIG_SPL_MAX_FOOTPRINT) >> +ASSERT(__bss_end - _start < (CONFIG_SPL_MAX_FOOTPRINT), \ >> + "SPL image plus BSS too big"); >> +#endif > > This code is never enabled. > Correct, assuming we leave the above "#if defined(CONFIG_SPL_BSS_MAX_SIZE) && defined(CONFIG_SPL_MAX_FOOTPRINT)" in place. Thanks, Oded > > > -- > Best Regards > Masahiro Yamada
diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds index e7799cc..0876a94 100644 --- a/arch/arm/cpu/armv8/u-boot-spl.lds +++ b/arch/arm/cpu/armv8/u-boot-spl.lds @@ -78,3 +78,22 @@ SECTIONS /DISCARD/ : { *(.interp*) } /DISCARD/ : { *(.gnu*) } } + +#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) && defined(CONFIG_SPL_MAX_FOOTPRINT) +ASSERT(0, "CONFIG_SPL_BSS_MAX_SIZE and CONFIG_SPL_MAX_FOOTPRINT must not be defined together"); +#endif + +#if defined(CONFIG_SPL_BSS_MAX_SIZE) +ASSERT(__bss_end - __bss_start < (CONFIG_SPL_BSS_MAX_SIZE), \ + "SPL image BSS too big"); +#endif + +#if defined(CONFIG_SPL_MAX_FOOTPRINT) +ASSERT(__bss_end - _start < (CONFIG_SPL_MAX_FOOTPRINT), \ + "SPL image plus BSS too big"); +#endif
This patch copies from arm u-boot-spl.lds some asserts that check that the size of the SPL image and BSS doesn't violate their max size. Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com> Cc: Albert Aribaud <albert.u.boot@aribaud.net> --- arch/arm/cpu/armv8/u-boot-spl.lds | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)