armv8: calculate __bss_size in u-boot-spl.lds

PATCH New Oded Gabbay 4 participants 8 messages Dec 26, 2016 – Jan 26, 2017 Arm (32-bit)

Share & tools

Oded Gabbay

author

2016-12-26 14:20 UTC #0 lore raw

PATCH 1/2 armv8: calculate __bss_size in u-boot-spl.lds

This patch adds a missing __bss_size symbol to the default armv8 u-boot-spl.lds file.
Makefile.spl relies on __bss_size to be present when it creates the SPL image. It uses that symbol to create a pad file that will be used to place the dtb after the bss section.
In ARMv8 default u-boot-spl.lds, __bss_size was missing and therefore, the pad file was always 0. As a result, the dtb was placed after _image_binary_end, which caused a failure when loading it inside the SPL.

Signed-off-by Oded Gabbay <[email protected]>

Cc Albert Aribaud <[email protected]>

---
 arch/arm/cpu/armv8/u-boot-spl.lds | 1 +
 1 file changed, 1 insertion(+)

arch/arm/cpu/armv8/u-boot-spl.lds +1 -0

index cc427c3..e7799cc 100644
@@ -70,6 +70,7 @@ SECTIONS
 		KEEP(*(.__bss_end));
 	} >.sdram
 
+	__bss_size = __bss_end - _image_binary_end;
 	/DISCARD/ : { *(.dynsym) }
 	/DISCARD/ : { *(.dynstr*) }
 	/DISCARD/ : { *(.dynamic*) }

Signature · 1 line

2.7.4

Oded Gabbay

author

2017-01-17 09:27 UTC #1 lore raw

PATCH 1/2 armv8: calculate __bss_size in u-boot-spl.lds

Hi Mashiro,
Could you please also take a look at the patch I sent to add the missing __bss_size ?

Thanks,
Oded

Quoted 30 lines · Oded Gabbay

This patch adds a missing __bss_size symbol to the default armv8 u-boot-spl.lds file.
Makefile.spl relies on __bss_size to be present when it creates the SPL image. It uses that symbol to create a pad file that will be used to place the dtb after the bss section.
In ARMv8 default u-boot-spl.lds, __bss_size was missing and therefore, the pad file was always 0. As a result, the dtb was placed after _image_binary_end, which caused a failure when loading it inside the SPL.
Signed-off-by Oded Gabbay <[email protected]>
Cc Albert Aribaud <[email protected]>
---
 arch/arm/cpu/armv8/u-boot-spl.lds | 1 +
 1 file changed, 1 insertion(+)
arch/arm/cpu/armv8/u-boot-spl.lds +1 -1
index cc427c3..e7799cc 100644
@@ -70,6 +70,7 @@ SECTIONS
                KEEP(*(.__bss_end));
        } >.sdram
 
+       __bss_size = __bss_end - _image_binary_end;
        /DISCARD/ : { *(.dynsym) }
        /DISCARD/ : { *(.dynstr*) }
        /DISCARD/ : { *(.dynamic*) }
--
2.7.4

Masahiro Yamada

2017-01-19 11:28 UTC #2 lore raw

PATCH 1/2 armv8: calculate __bss_size in u-boot-spl.lds

Hi.

(CCing Simon because he wrote the code addressed in this discussion.)

2017-01-17 18:27 GMT+09:00 Oded Gabbay <[email protected]>:

Quoted 31 lines

Hi Mashiro,
Could you please also take a look at the patch I sent to add the missing __bss_size ?
Thanks,
Oded
Quoted 23 lines · Oded Gabbay
This patch adds a missing __bss_size symbol to the default armv8 u-boot-spl.lds file.
Makefile.spl relies on __bss_size to be present when it creates the SPL image. It uses that symbol to create a pad file that will be used to place the dtb after the bss section.
In ARMv8 default u-boot-spl.lds, __bss_size was missing and therefore, the pad file was always 0. As a result, the dtb was placed after _image_binary_end, which caused a failure when loading it inside the SPL.
Signed-off-by Oded Gabbay <[email protected]>
Cc Albert Aribaud <[email protected]>
---
 arch/arm/cpu/armv8/u-boot-spl.lds | 1 +
 1 file changed, 1 insertion(+)
arch/arm/cpu/armv8/u-boot-spl.lds +0 -0
index cc427c3..e7799cc 100644
@@ -70,6 +70,7 @@ SECTIONS
                KEEP(*(.__bss_end));
        } >.sdram
 

Quoted 4 lines

arch/arm/cpu/armv8/u-boot-spl.lds SECTIONS · line 76
Quoted 4 lines
arch/arm/cpu/armv8/u-boot-spl.lds +1 -0
+       __bss_size = __bss_end - _image_binary_end;
        /DISCARD/ : { *(.dynsym) }
        /DISCARD/ : { *(.dynstr*) }
        /DISCARD/ : { *(.dynamic*) }

This patch seems wrong.

This linker script seems to support separate two memory regions.

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 }

_image_binary_end is the end of the first region (.sram), __bss_end is the end of the second region (.sdram).

So, with your patch,

__bss_size = (gap between .sram and .sdram) + (size of .bss)

This will append really big padding there is a gap between the two regions. (at least, it will break my boards.)

If you take a look at fdtdec_setup(),

    #  ifdef CONFIG_SPL_BUILD
    /* FDT is at end of BSS unless it is in a different memory region */
    if (IS_ENABLED(CONFIG_SPL_SEPARATE_BSS))
             gd->fdt_blob = (ulong *)&_image_binary_end;
    else
             gd->fdt_blob = (ulong *)&__bss_end;

I think you are supposed to enable CONFIG_SPL_SEPARATE_BSS if you want to use SPL_OF_CONTROL on your arm64 boards.

Checking the following commit will help you.

commit 10172962479ddd6609101fdb83bde66c0719852c

Author: Simon Glass <[email protected]>
Date:   Sat Oct 17 19:41:19 2015 -0600

dm: spl: Support device tree when BSS is in a different section

At present in SPL we place the device tree immediately after BSS. This avoids needing to copy it out of the way before BSS can be used. However on some boards BSS is not placed with the image - e.g. it can be in RAM if available.

Add an option to tell U-Boot that the device tree should be placed at the end of the image binary (_image_binary_end) instead of at the end of BSS.

Note: A common reason to place BSS in RAM is to support the FAT filesystem. We should update the code so that it does not use so much BSS.

Signed-off-by Simon Glass <[email protected]>

Signed-off-by Michal Simek <[email protected]>

Signature · 2 lines

Best Regards
Masahiro Yamada

Simon Glass

2017-01-26 14:24 UTC #3 lore raw

PATCH 1/2 armv8: calculate __bss_size in u-boot-spl.lds

Hi,

Quoted 94 lines · Masahiro Yamada

Hi.
(CCing Simon because he wrote the code addressed in this discussion.)
2017-01-17 18:27 GMT+09:00 Oded Gabbay <[email protected]>:
Quoted 31 lines
Hi Mashiro,
Could you please also take a look at the patch I sent to add the missing __bss_size ?
Thanks,
Oded
Quoted 23 lines · Oded Gabbay
This patch adds a missing __bss_size symbol to the default armv8 u-boot-spl.lds file.
Makefile.spl relies on __bss_size to be present when it creates the SPL image. It uses that symbol to create a pad file that will be used to place the dtb after the bss section.
In ARMv8 default u-boot-spl.lds, __bss_size was missing and therefore, the pad file was always 0. As a result, the dtb was placed after _image_binary_end, which caused a failure when loading it inside the SPL.
Signed-off-by Oded Gabbay <[email protected]>
Cc Albert Aribaud <[email protected]>
---
 arch/arm/cpu/armv8/u-boot-spl.lds | 1 +
 1 file changed, 1 insertion(+)
arch/arm/cpu/armv8/u-boot-spl.lds +0 -0
index cc427c3..e7799cc 100644
@@ -70,6 +70,7 @@ SECTIONS
                KEEP(*(.__bss_end));
        } >.sdram
 
Quoted 4 lines
arch/arm/cpu/armv8/u-boot-spl.lds SECTIONS · line 76
Quoted 4 lines
arch/arm/cpu/armv8/u-boot-spl.lds +1 -0
+       __bss_size = __bss_end - _image_binary_end;
        /DISCARD/ : { *(.dynsym) }
        /DISCARD/ : { *(.dynstr*) }
        /DISCARD/ : { *(.dynamic*) }
This patch seems wrong.
This linker script seems to support separate two memory regions.
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 }
_image_binary_end is the end of the first region (.sram), __bss_end is the end of the second region (.sdram).
So, with your patch,
__bss_size = (gap between .sram and .sdram) + (size of .bss)
This will append really big padding there is a gap between the two regions. (at least, it will break my boards.)
If you take a look at fdtdec_setup(),
    #  ifdef CONFIG_SPL_BUILD
    /* FDT is at end of BSS unless it is in a different memory region */
    if (IS_ENABLED(CONFIG_SPL_SEPARATE_BSS))
             gd->fdt_blob = (ulong *)&_image_binary_end;
    else
             gd->fdt_blob = (ulong *)&__bss_end;
I think you are supposed to enable CONFIG_SPL_SEPARATE_BSS if you want to use SPL_OF_CONTROL on your arm64 boards.
Checking the following commit will help you.
commit 10172962479ddd6609101fdb83bde66c0719852c
Author: Simon Glass <[email protected]>
Date:   Sat Oct 17 19:41:19 2015 -0600
dm: spl: Support device tree when BSS is in a different section
At present in SPL we place the device tree immediately after BSS. This avoids needing to copy it out of the way before BSS can be used. However on some boards BSS is not placed with the image - e.g. it can be in RAM if available.

Quoted 8 lines

Add an option to tell U-Boot that the device tree should be placed at the end of the image binary (_image_binary_end) instead of at the end of BSS.
Note: A common reason to place BSS in RAM is to support the FAT filesystem. We should update the code so that it does not use so much BSS.
Signed-off-by Simon Glass <[email protected]>
Signed-off-by Michal Simek <[email protected]>

Yes that's right. Thank you Masahiro.

Also, consider moving to use binman to generate the image.

Regards,
Simon

Oded Gabbay

author

2016-12-26 14:20 UTC #4 lore raw

PATCH 2/2 armv8: add asserts of sizes to u-boot-spl.lds

In reply to Oded Gabbay

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 <[email protected]>

Cc Albert Aribaud <[email protected]>

---
 arch/arm/cpu/armv8/u-boot-spl.lds | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

arch/arm/cpu/armv8/u-boot-spl.lds +19 -0

index e7799cc..0876a94 100644
@@ -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

Signature · 1 line

2.7.4

Tom Rini

2017-01-09 00:56 UTC #5 lore raw

PATCH 2/2 armv8: add asserts of sizes to u-boot-spl.lds

Quoted 5 lines · Oded Gabbay

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 <[email protected]>
Cc Albert Aribaud <[email protected]>

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

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170108/31a23645/attachment.sig>

Masahiro Yamada

2017-01-17 07:55 UTC #6 lore raw

PATCH 2/2 armv8: add asserts of sizes to u-boot-spl.lds

In reply to Oded Gabbay

2016-12-26 23:20 GMT+09:00 Oded Gabbay <[email protected]>:

Quoted 37 lines

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 <[email protected]>

Cc Albert Aribaud <[email protected]>

---
 arch/arm/cpu/armv8/u-boot-spl.lds | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

arch/arm/cpu/armv8/u-boot-spl.lds +19 -1

index e7799cc..0876a94 100644
@@ -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
--

Quoted 2 lines

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

arch/arm/cpu/armv8/u-boot-spl.lds SECTIONS · line 89

Quoted 3 lines

arch/arm/cpu/armv8/u-boot-spl.lds +3 -0

+#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,

arch/arm/cpu/armv8/u-boot-spl.lds SECTIONS · line 94

Quoted 4 lines

arch/arm/cpu/armv8/u-boot-spl.lds +4 -0

+#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.

arch/arm/cpu/armv8/u-boot-spl.lds SECTIONS · line 93

Quoted 2 lines

arch/arm/cpu/armv8/u-boot-spl.lds +2 -0

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

arch/arm/cpu/armv8/u-boot-spl.lds SECTIONS · line 99

Quoted 4 lines

arch/arm/cpu/armv8/u-boot-spl.lds +4 -0

+#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.

Signature · 2 lines

Best Regards
Masahiro Yamada

Oded Gabbay

author

2017-01-17 08:04 UTC #7 lore raw

PATCH 2/2 armv8: add asserts of sizes to u-boot-spl.lds

Quoted 57 lines · Masahiro Yamada

2016-12-26 23:20 GMT+09:00 Oded Gabbay <[email protected]>:
Quoted 37 lines
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 <[email protected]>
Cc Albert Aribaud <[email protected]>
---
 arch/arm/cpu/armv8/u-boot-spl.lds | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
arch/arm/cpu/armv8/u-boot-spl.lds +19 -1
index e7799cc..0876a94 100644
@@ -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
--
Quoted 2 lines
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.

Quoted 8 lines

Your code
arch/arm/cpu/armv8/u-boot-spl.lds SECTIONS · line 89
Quoted 3 lines
arch/arm/cpu/armv8/u-boot-spl.lds +3 -0
+#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.

Quoted 9 lines

As a result,
arch/arm/cpu/armv8/u-boot-spl.lds SECTIONS · line 94
Quoted 4 lines
arch/arm/cpu/armv8/u-boot-spl.lds +4 -0
+#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

Quoted 13 lines

arch/arm/cpu/armv8/u-boot-spl.lds SECTIONS · line 93
Quoted 3 lines
arch/arm/cpu/armv8/u-boot-spl.lds +2 -0
+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.

Quoted 7 lines

arch/arm/cpu/armv8/u-boot-spl.lds SECTIONS · line 99
Quoted 4 lines
arch/arm/cpu/armv8/u-boot-spl.lds +4 -0
+#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

Quoted 5 lines

--
Best Regards
Masahiro Yamada