[PATCH] configs: ast2600: Move SPL bss section to DRAM space

[PATCH] configs: ast2600: Move SPL bss section to DRAM space

[Chia-Wei Wang]

The commit b583348ca8c8 ("image: fit: Align hash output buffers") places
the hash output buffer at the .bss section. However, AST2600 by default
executes SPL in the NOR flash XIP way. This results in the hash output
cannot be written to the buffer as it is located at the R/X only region.

We need to move the .bss section out of the SPL body to the DRAM space,
where hash output can be written to. This patch includes:
 - Define the .bss section base and size
 - A new SPL linker script is added with a separate .bss region specified
 - Enable CONFIG_SPL_SEPARATE_BSS kconfig option

Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>
---
 arch/arm/mach-aspeed/ast2600/u-boot-spl.lds | 94 +++++++++++++++++++++
 configs/evb-ast2600_defconfig               |  3 +
 include/configs/evb_ast2600.h               |  3 +
 3 files changed, 100 insertions(+)
 create mode 100644 arch/arm/mach-aspeed/ast2600/u-boot-spl.lds

diff --git a/arch/arm/mach-aspeed/ast2600/u-boot-spl.lds b/arch/arm/mach-aspeed/ast2600/u-boot-spl.lds
new file mode 100644
index 0000000000..22b4e16d35
--- /dev/null
+++ b/arch/arm/mach-aspeed/ast2600/u-boot-spl.lds
@@ -0,0 +1,94 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (c) 2004-2008 Texas Instruments
+ *
+ * (C) Copyright 2002
+ * Gary Jennejohn, DENX Software Engineering, <garyj@denx.de>
+ *
+ * (C) Copyright 2022
+ * Chia-Wei Wang <chiawei_wang@aspeedtech.com>
+ */
+
+MEMORY { .nor : ORIGIN = CONFIG_SPL_TEXT_BASE,
+		LENGTH = CONFIG_SPL_SIZE_LIMIT }
+MEMORY { .bss : ORIGIN = CONFIG_SPL_BSS_START_ADDR,
+		LENGTH = CONFIG_SPL_BSS_MAX_SIZE }
+
+OUTPUT_FORMAT("elf32-littlearm", "elf32-littlearm", "elf32-littlearm")
+OUTPUT_ARCH(arm)
+ENTRY(_start)
+SECTIONS
+{
+	. = 0x00000000;
+
+	. = ALIGN(4);
+	.text :
+	{
+		__image_copy_start = .;
+		*(.vectors)
+		CPUDIR/start.o (.text*)
+		*(.text*)
+		*(.glue*)
+	} > .nor
+
+	. = ALIGN(4);
+	.rodata : { *(SORT_BY_ALIGNMENT(SORT_BY_NAME(.rodata*))) } > .nor
+
+	. = ALIGN(4);
+	.data : {
+		*(.data*)
+	} > .nor
+
+	. = ALIGN(4);
+	.u_boot_list : {
+		KEEP(*(SORT(.u_boot_list*)));
+	} > .nor
+
+	. = ALIGN(4);
+	.binman_sym_table : {
+		__binman_sym_start = .;
+		KEEP(*(SORT(.binman_sym*)));
+		__binman_sym_end = .;
+	} > .nor
+
+	. = ALIGN(4);
+
+	__image_copy_end = .;
+
+	.rel.dyn : {
+		__rel_dyn_start = .;
+		*(.rel*)
+		__rel_dyn_end = .;
+	} > .nor
+
+	.end :
+	{
+		*(.__end)
+	} > .nor
+
+	_image_binary_end = .;
+
+	.bss : {
+		__bss_start = .;
+		*(.bss*)
+		 . = ALIGN(4);
+		__bss_end = .;
+	} > .bss
+
+	__bss_size = __bss_end - __bss_start;
+}
+
+#if defined(IMAGE_MAX_SIZE)
+ASSERT(__image_copy_end - __image_copy_start <= (IMAGE_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
+
+#if defined(CONFIG_SPL_MAX_FOOTPRINT)
+ASSERT(__bss_end - _start <= (CONFIG_SPL_MAX_FOOTPRINT), \
+	"SPL image plus BSS too big");
+#endif
diff --git a/configs/evb-ast2600_defconfig b/configs/evb-ast2600_defconfig
index f84b723bbb..d19e1d79ec 100644
--- a/configs/evb-ast2600_defconfig
+++ b/configs/evb-ast2600_defconfig
@@ -1,6 +1,7 @@
 CONFIG_ARM=y
 CONFIG_SYS_DCACHE_OFF=y
 CONFIG_SPL_SYS_THUMB_BUILD=y
+CONFIG_SPL_LDSCRIPT="arch/arm/mach-aspeed/ast2600/u-boot-spl.lds"
 CONFIG_ARCH_ASPEED=y
 CONFIG_SYS_TEXT_BASE=0x80000000
 CONFIG_SYS_MALLOC_LEN=0x2000000
@@ -35,6 +36,8 @@ CONFIG_SPL_SIZE_LIMIT_SUBTRACT_MALLOC=y
 CONFIG_SPL_SYS_MALLOC_SIMPLE=y
 CONFIG_SPL_STACK_R=y
 CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x2000000
+CONFIG_SPL_SEPARATE_BSS=y
+# CONFIG_TPL_SEPARATE_BSS is not set
 CONFIG_SPL_FIT_IMAGE_TINY=y
 CONFIG_SPL_DM_RESET=y
 CONFIG_SPL_RAM_SUPPORT=y
diff --git a/include/configs/evb_ast2600.h b/include/configs/evb_ast2600.h
index 3c2155da46..54abb29df9 100644
--- a/include/configs/evb_ast2600.h
+++ b/include/configs/evb_ast2600.h
@@ -10,6 +10,9 @@
 
 #define CONFIG_SYS_UBOOT_BASE		CONFIG_SYS_TEXT_BASE
 
+#define CONFIG_SPL_BSS_START_ADDR	0x83000000
+#define CONFIG_SPL_BSS_MAX_SIZE		0x01000000
+
 /* Misc */
 #define STR_HELPER(s)	#s
 #define STR(s)		STR_HELPER(s)
-- 
2.25.1

RE: [PATCH] configs: ast2600: Move SPL bss section to DRAM space

[Neal Liu]

> The commit b583348ca8c8 ("image: fit: Align hash output buffers") places the
> hash output buffer at the .bss section. However, AST2600 by default executes
> SPL in the NOR flash XIP way. This results in the hash output cannot be written
> to the buffer as it is located at the R/X only region.
> 
> We need to move the .bss section out of the SPL body to the DRAM space,
> where hash output can be written to. This patch includes:
>  - Define the .bss section base and size
>  - A new SPL linker script is added with a separate .bss region specified
>  - Enable CONFIG_SPL_SEPARATE_BSS kconfig option
> 
> Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>

LGTM.

Reviewed-by: Neal Liu <neal_liu@aspeedtech.com>

Best Regards,
-Neal

> ---
>  arch/arm/mach-aspeed/ast2600/u-boot-spl.lds | 94
> +++++++++++++++++++++
>  configs/evb-ast2600_defconfig               |  3 +
>  include/configs/evb_ast2600.h               |  3 +
>  3 files changed, 100 insertions(+)
>  create mode 100644 arch/arm/mach-aspeed/ast2600/u-boot-spl.lds

Re: [PATCH] configs: ast2600: Move SPL bss section to DRAM space

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 844 bytes --]

On Wed, Jun 01, 2022 at 04:21:15PM +0800, Chia-Wei Wang wrote:

> The commit b583348ca8c8 ("image: fit: Align hash output buffers") places
> the hash output buffer at the .bss section. However, AST2600 by default
> executes SPL in the NOR flash XIP way. This results in the hash output
> cannot be written to the buffer as it is located at the R/X only region.
> 
> We need to move the .bss section out of the SPL body to the DRAM space,
> where hash output can be written to. This patch includes:
>  - Define the .bss section base and size
>  - A new SPL linker script is added with a separate .bss region specified
>  - Enable CONFIG_SPL_SEPARATE_BSS kconfig option
> 
> Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>
> Reviewed-by: Neal Liu <neal_liu@aspeedtech.com>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] configs: ast2600: Move SPL bss section to DRAM space

[Joel Stanley]

Hi Chai Wei,

On Wed, 1 Jun 2022 at 08:21, Chia-Wei Wang <chiawei_wang@aspeedtech.com> wrote:
>
> The commit b583348ca8c8 ("image: fit: Align hash output buffers") places
> the hash output buffer at the .bss section. However, AST2600 by default
> executes SPL in the NOR flash XIP way. This results in the hash output
> cannot be written to the buffer as it is located at the R/X only region.
>
> We need to move the .bss section out of the SPL body to the DRAM space,
> where hash output can be written to. This patch includes:
>  - Define the .bss section base and size
>  - A new SPL linker script is added with a separate .bss region specified
>  - Enable CONFIG_SPL_SEPARATE_BSS kconfig option
>
> Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>

This patch breaks booting for me.

My concern with the approach is it creates extra maintenance work.
When changes are made to the main linker script they need to be
mirrored here, or else the aspeed port will miss out. (Having the
machine tested in CI will help this somewhat, but only for the code
paths we can test under emulation).

I know the patch has been merged, but I have a few questions:

I imagine the ast2600 is not the only board that runs XIP. How do
other boards solve the problem?

What happens when a symbol that is used before DRAM training has
completed is placed in bss?

How do you plan to support systems that don't have NOR?

Cheers,

Joel


> ---
>  arch/arm/mach-aspeed/ast2600/u-boot-spl.lds | 94 +++++++++++++++++++++
>  configs/evb-ast2600_defconfig               |  3 +
>  include/configs/evb_ast2600.h               |  3 +
>  3 files changed, 100 insertions(+)
>  create mode 100644 arch/arm/mach-aspeed/ast2600/u-boot-spl.lds
>
> diff --git a/arch/arm/mach-aspeed/ast2600/u-boot-spl.lds b/arch/arm/mach-aspeed/ast2600/u-boot-spl.lds
> new file mode 100644
> index 0000000000..22b4e16d35
> --- /dev/null
> +++ b/arch/arm/mach-aspeed/ast2600/u-boot-spl.lds
> @@ -0,0 +1,94 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (c) 2004-2008 Texas Instruments
> + *
> + * (C) Copyright 2002
> + * Gary Jennejohn, DENX Software Engineering, <garyj@denx.de>
> + *
> + * (C) Copyright 2022
> + * Chia-Wei Wang <chiawei_wang@aspeedtech.com>
> + */
> +
> +MEMORY { .nor : ORIGIN = CONFIG_SPL_TEXT_BASE,
> +               LENGTH = CONFIG_SPL_SIZE_LIMIT }
> +MEMORY { .bss : ORIGIN = CONFIG_SPL_BSS_START_ADDR,
> +               LENGTH = CONFIG_SPL_BSS_MAX_SIZE }
> +
> +OUTPUT_FORMAT("elf32-littlearm", "elf32-littlearm", "elf32-littlearm")
> +OUTPUT_ARCH(arm)
> +ENTRY(_start)
> +SECTIONS
> +{
> +       . = 0x00000000;
> +
> +       . = ALIGN(4);
> +       .text :
> +       {
> +               __image_copy_start = .;
> +               *(.vectors)
> +               CPUDIR/start.o (.text*)
> +               *(.text*)
> +               *(.glue*)
> +       } > .nor
> +
> +       . = ALIGN(4);
> +       .rodata : { *(SORT_BY_ALIGNMENT(SORT_BY_NAME(.rodata*))) } > .nor
> +
> +       . = ALIGN(4);
> +       .data : {
> +               *(.data*)
> +       } > .nor
> +
> +       . = ALIGN(4);
> +       .u_boot_list : {
> +               KEEP(*(SORT(.u_boot_list*)));
> +       } > .nor
> +
> +       . = ALIGN(4);
> +       .binman_sym_table : {
> +               __binman_sym_start = .;
> +               KEEP(*(SORT(.binman_sym*)));
> +               __binman_sym_end = .;
> +       } > .nor
> +
> +       . = ALIGN(4);
> +
> +       __image_copy_end = .;
> +
> +       .rel.dyn : {
> +               __rel_dyn_start = .;
> +               *(.rel*)
> +               __rel_dyn_end = .;
> +       } > .nor
> +
> +       .end :
> +       {
> +               *(.__end)
> +       } > .nor
> +
> +       _image_binary_end = .;
> +
> +       .bss : {
> +               __bss_start = .;
> +               *(.bss*)
> +                . = ALIGN(4);
> +               __bss_end = .;
> +       } > .bss
> +
> +       __bss_size = __bss_end - __bss_start;
> +}
> +
> +#if defined(IMAGE_MAX_SIZE)
> +ASSERT(__image_copy_end - __image_copy_start <= (IMAGE_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
> +
> +#if defined(CONFIG_SPL_MAX_FOOTPRINT)
> +ASSERT(__bss_end - _start <= (CONFIG_SPL_MAX_FOOTPRINT), \
> +       "SPL image plus BSS too big");
> +#endif
> diff --git a/configs/evb-ast2600_defconfig b/configs/evb-ast2600_defconfig
> index f84b723bbb..d19e1d79ec 100644
> --- a/configs/evb-ast2600_defconfig
> +++ b/configs/evb-ast2600_defconfig
> @@ -1,6 +1,7 @@
>  CONFIG_ARM=y
>  CONFIG_SYS_DCACHE_OFF=y
>  CONFIG_SPL_SYS_THUMB_BUILD=y
> +CONFIG_SPL_LDSCRIPT="arch/arm/mach-aspeed/ast2600/u-boot-spl.lds"
>  CONFIG_ARCH_ASPEED=y
>  CONFIG_SYS_TEXT_BASE=0x80000000
>  CONFIG_SYS_MALLOC_LEN=0x2000000
> @@ -35,6 +36,8 @@ CONFIG_SPL_SIZE_LIMIT_SUBTRACT_MALLOC=y
>  CONFIG_SPL_SYS_MALLOC_SIMPLE=y
>  CONFIG_SPL_STACK_R=y
>  CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x2000000
> +CONFIG_SPL_SEPARATE_BSS=y
> +# CONFIG_TPL_SEPARATE_BSS is not set
>  CONFIG_SPL_FIT_IMAGE_TINY=y
>  CONFIG_SPL_DM_RESET=y
>  CONFIG_SPL_RAM_SUPPORT=y
> diff --git a/include/configs/evb_ast2600.h b/include/configs/evb_ast2600.h
> index 3c2155da46..54abb29df9 100644
> --- a/include/configs/evb_ast2600.h
> +++ b/include/configs/evb_ast2600.h
> @@ -10,6 +10,9 @@
>
>  #define CONFIG_SYS_UBOOT_BASE          CONFIG_SYS_TEXT_BASE
>
> +#define CONFIG_SPL_BSS_START_ADDR      0x83000000
> +#define CONFIG_SPL_BSS_MAX_SIZE                0x01000000
> +
>  /* Misc */
>  #define STR_HELPER(s)  #s
>  #define STR(s)         STR_HELPER(s)
> --
> 2.25.1
>

Re: [PATCH] configs: ast2600: Move SPL bss section to DRAM space

[Sean Anderson]

Hi Chai,

On 6/28/22 12:23 AM, Joel Stanley wrote:
> Hi Chai Wei,
> 
> On Wed, 1 Jun 2022 at 08:21, Chia-Wei Wang <chiawei_wang@aspeedtech.com> wrote:
>>
>> The commit b583348ca8c8 ("image: fit: Align hash output buffers") places
>> the hash output buffer at the .bss section. However, AST2600 by default
>> executes SPL in the NOR flash XIP way. This results in the hash output
>> cannot be written to the buffer as it is located at the R/X only region.
>>
>> We need to move the .bss section out of the SPL body to the DRAM space,
>> where hash output can be written to. This patch includes:
>>   - Define the .bss section base and size
>>   - A new SPL linker script is added with a separate .bss region specified
>>   - Enable CONFIG_SPL_SEPARATE_BSS kconfig option
>>
>> Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>
> 
> This patch breaks booting for me.

Does the patch Joel posted [1] fix your issue? It seems like I used the wrong macro
in the first place, so hopefully this patch shouldn't be necessary.

--Sean

[1] https://lore.kernel.org/u-boot/20220620070117.3443066-1-joel@jms.id.au/

RE: [PATCH] configs: ast2600: Move SPL bss section to DRAM space

[ChiaWei Wang]

Hi Sean,

> From: Sean Anderson <sean.anderson@seco.com>
> Sent: Tuesday, June 28, 2022 12:57 PM
> 
> Hi Chai,
> 
> On 6/28/22 12:23 AM, Joel Stanley wrote:
> > Hi Chai Wei,
> >
> > On Wed, 1 Jun 2022 at 08:21, Chia-Wei Wang
> <chiawei_wang@aspeedtech.com> wrote:
> >>
> >> The commit b583348ca8c8 ("image: fit: Align hash output buffers")
> >> places the hash output buffer at the .bss section. However, AST2600
> >> by default executes SPL in the NOR flash XIP way. This results in the
> >> hash output cannot be written to the buffer as it is located at the R/X only
> region.
> >>
> >> We need to move the .bss section out of the SPL body to the DRAM
> >> space, where hash output can be written to. This patch includes:
> >>   - Define the .bss section base and size
> >>   - A new SPL linker script is added with a separate .bss region specified
> >>   - Enable CONFIG_SPL_SEPARATE_BSS kconfig option
> >>
> >> Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>
> >
> > This patch breaks booting for me.
> 
> Does the patch Joel posted [1] fix your issue? It seems like I used the wrong
> macro in the first place, so hopefully this patch shouldn't be necessary.
> 
> --Sean
> 
> [1] https://lore.kernel.org/u-boot/20220620070117.3443066-1-joel@jms.id.au/

Yes. Joel's patch also solved that issue.

Relocating .bss to DRAM space can avoid similar issues.
But it do create additional maintenance work.

Chiawei

RE: [PATCH] configs: ast2600: Move SPL bss section to DRAM space

[ChiaWei Wang]

Hi Joel,

> From: Joel Stanley <joel@jms.id.au>
> Sent: Tuesday, June 28, 2022 12:24 PM
> 
> Hi Chai Wei,
> 
> On Wed, 1 Jun 2022 at 08:21, Chia-Wei Wang
> <chiawei_wang@aspeedtech.com> wrote:
> >
> > The commit b583348ca8c8 ("image: fit: Align hash output buffers")
> > places the hash output buffer at the .bss section. However, AST2600 by
> > default executes SPL in the NOR flash XIP way. This results in the
> > hash output cannot be written to the buffer as it is located at the R/X only
> region.
> >
> > We need to move the .bss section out of the SPL body to the DRAM
> > space, where hash output can be written to. This patch includes:
> >  - Define the .bss section base and size
> >  - A new SPL linker script is added with a separate .bss region
> > specified
> >  - Enable CONFIG_SPL_SEPARATE_BSS kconfig option
> >
> > Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>
> 
> This patch breaks booting for me.
> 
> My concern with the approach is it creates extra maintenance work.
> When changes are made to the main linker script they need to be mirrored
> here, or else the aspeed port will miss out. (Having the machine tested in CI
> will help this somewhat, but only for the code paths we can test under
> emulation).

The patch was trying to solve the hash buffer allocation change to common code and to avoid similar issues.
But I agree there is additional maintenance work on the customized linker script.

> 
> I know the patch has been merged, but I have a few questions:
> 
> I imagine the ast2600 is not the only board that runs XIP. How do other boards
> solve the problem?
> 
> What happens when a symbol that is used before DRAM training has
> completed is placed in bss?

Honestly, I am not sure how other platforms/boards dealing with this.
But like U-Boot REAME stated, the initial global data is read-only.
I guess we have to be careful about the global variable use before DRAM initialization or even variable relocation.

> 
> How do you plan to support systems that don't have NOR?

We would like to have another defconfig for eMMC booting.
If the NOR-based linker script is not applicable, the default one can be used.

Chiawei