[PATCH v2 0/4] Insure 16 alignment of reserved memory in board_f.c

[PATCH v2 0/4] Insure 16 alignment of reserved memory in board_f.c

[Patrick Delaunay]

Hi,

It is a V2, rebased on master branch, for the untitled serie
http://patchwork.ozlabs.org/project/uboot/list/?series=152226

This serie now include the previous patch:
[U-Boot,v3] board_f.c: Insure gd->new_bootstage alignment
http://patchwork.ozlabs.org/patch/1201452/

After this first correction, I remove the stm32mp1 workaround
as the issue of bootstage alignment is solved.

The 4th patch is a complete solution to alignment
(proposed in comment 5 of
 http://patchwork.ozlabs.org/patch/1201452/#2327366)
I always align the reserved memory to 16 bytes with a new function
reserve_sp().

This patch causes an issue on ARM 32 bits, as the relocated gd pointer
is not initialized with gd->new_gd as expected in reserve_global_data()
but is hard-coded with
  relocated gd = gd->bd - GD_SIZE
  {with GD_SIZE = sizeof(struct global_data)}

This issue is solved with the 3rd patch of the serie
  arm: set the relocated gd with gd->new_gd

Only tested on STM32MP157C-EV1 board (ARM32 architecture).


Changes in v2:
- import: [U-Boot,v3] board_f.c: Insure gd->new_bootstage alignment

Patrice Chotard (1):
  board_f.c: Insure gd->new_bootstage alignment

Patrick Delaunay (3):
  Revert "stm32mp1: remove the imply BOOTSTAGE"
  arm: set the relocated gd with gd->new_gd
  board_f.c: Insure 16 alignment of start_addr_sp and reserved memory

 arch/arm/lib/crt0.S           |  3 +--
 arch/arm/mach-stm32mp/Kconfig |  2 ++
 common/board_f.c              | 27 ++++++++++++++++++---------
 3 files changed, 21 insertions(+), 11 deletions(-)

-- 
2.17.1

[PATCH v2 1/4] board_f.c: Insure gd->new_bootstage alignment

[Patrick Delaunay]

From: Patrice Chotard <patrice.chotard@st.com>

In reserve_bootstage(), in case size is odd, gd->new_bootstage
is not aligned. In bootstage_relocate(), the platform hangs when
getting access to data->record[i].name.
To avoid this issue, make gd->new_bootstage 16 byte aligned.

To insure that new_bootstage is 16 byte aligned (at least needed for
x86_64 and ARMv8) and new_bootstage starts down to get enough space,
ALIGN_DOWN macro is used.

Fixes: ac9cd4805c8b ("bootstage: Correct relocation algorithm")

Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
Reviewed-by: Vikas MANOCHA <vikas.manocha@st.com>
Reviewed-by: Patrick Delaunay <patrick.delaunay@st.com>
Tested-by: Patrick Delaunay <patrick.delaunay@st.com>
Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

Changes in v2:
- import: [U-Boot,v3] board_f.c: Insure gd->new_bootstage alignment

 common/board_f.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/common/board_f.c b/common/board_f.c
index d66afb37ca..e21f533634 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -562,6 +562,11 @@ static int reserve_bootstage(void)
 	int size = bootstage_get_size();
 
 	gd->start_addr_sp -= size;
+	/*
+	 * Insure that start_addr_sp is aligned down to reserve enough
+	 * space for new_bootstage
+	 */
+	gd->start_addr_sp = ALIGN_DOWN(gd->start_addr_sp, 16);
 	gd->new_bootstage = map_sysmem(gd->start_addr_sp, size);
 	debug("Reserving %#x Bytes for bootstage at: %08lx\n", size,
 	      gd->start_addr_sp);
-- 
2.17.1

[PATCH v2 2/4] Revert "stm32mp1: remove the imply BOOTSTAGE"

[Patrick Delaunay]

This reverts the workaround introduced by the
commit 16fec9b0bc1a ("stm32mp1: remove the imply BOOTSTAGE")
As the bootstage alignment issue is now solved.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

Changes in v2: None

 arch/arm/mach-stm32mp/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/mach-stm32mp/Kconfig b/arch/arm/mach-stm32mp/Kconfig
index ae28f6e206..e920b89ef5 100644
--- a/arch/arm/mach-stm32mp/Kconfig
+++ b/arch/arm/mach-stm32mp/Kconfig
@@ -46,7 +46,9 @@ config TARGET_STM32MP1
 	select STM32_SERIAL
 	select SYS_ARCH_TIMER
 	imply BOOTCOUNT_LIMIT
+	imply BOOTSTAGE
 	imply CMD_BOOTCOUNT
+	imply CMD_BOOTSTAGE
 	imply CMD_CLS if CMD_BMP
 	imply DISABLE_CONSOLE
 	imply PRE_CONSOLE_BUFFER
-- 
2.17.1

[PATCH v2 3/4] arm: set the relocated gd with gd->new_gd

[Patrick Delaunay]

Simplify the arm relocation behavior and get gd directly form new_gd,
as it is already done in crt0_64.S:

	ldr	x18, [x18, #GD_NEW_GD]		/* x18 <- gd->new_gd */

This patch avoid assumption on new GD location (new GD is below bd -
with #GD_SIZE offset).

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

Changes in v2: None

 arch/arm/lib/crt0.S | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/lib/crt0.S b/arch/arm/lib/crt0.S
index fb6c37cf51..df9dd83e40 100644
--- a/arch/arm/lib/crt0.S
+++ b/arch/arm/lib/crt0.S
@@ -127,8 +127,7 @@ ENTRY(_main)
 	ldr	r0, [r9, #GD_START_ADDR_SP]	/* sp = gd->start_addr_sp */
 	bic	r0, r0, #7	/* 8-byte alignment for ABI compliance */
 	mov	sp, r0
-	ldr	r9, [r9, #GD_BD]		/* r9 = gd->bd */
-	sub	r9, r9, #GD_SIZE		/* new GD is below bd */
+	ldr	r9, [r9, #GD_NEW_GD]		/* r9 <- gd->new_gd */
 
 	adr	lr, here
 	ldr	r0, [r9, #GD_RELOC_OFF]		/* r0 = gd->reloc_off */
-- 
2.17.1

[PATCH v2 4/4] board_f.c: Insure 16 alignment of start_addr_sp and reserved memory

[Patrick Delaunay]

Add a function reserve_sp() to reserved memory with 16 bits alignment
after the stack pointer (gd->start_addr_sp) and use this new function
in board_f.c to reserve all the memory area (malloc, board, gd, fdt,
bootstage, stacks).

This 16 byte alignment is needed for cast on struct pointer
for the reserved memory, for example:
+ x86_64 ABI: https://reviews.llvm.org/D30049: 16 bytes
+ ARMv8 Instruction Set Overview: quad word, 16 bytes

An other alignment value could be needed for other architecture.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

Changes in v2: None

 common/board_f.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/common/board_f.c b/common/board_f.c
index e21f533634..0302ee4a6e 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -470,6 +470,17 @@ static int reserve_uboot(void)
 	return 0;
 }
 
+/*
+ * reserve after start_addr_sp the requested size and make the stack pointer
+ * 16-byte aligned, this alignment is needed for cast on the reserved memory
+ * ref = x86_64 ABI: https://reviews.llvm.org/D30049: 16 bytes
+ *     = ARMv8 Instruction Set Overview: quad word, 16 bytes
+ */
+static unsigned long reserve_sp(size_t size)
+{
+	return ALIGN_DOWN(gd->start_addr_sp - size, 16);
+}
+
 #ifdef CONFIG_SYS_NONCACHED_MEMORY
 static int reserve_noncached(void)
 {
@@ -495,7 +506,7 @@ static int reserve_noncached(void)
 /* reserve memory for malloc() area */
 static int reserve_malloc(void)
 {
-	gd->start_addr_sp = gd->start_addr_sp - TOTAL_MALLOC_LEN;
+	gd->start_addr_sp = reserve_sp(TOTAL_MALLOC_LEN);
 	debug("Reserving %dk for malloc() at: %08lx\n",
 	      TOTAL_MALLOC_LEN >> 10, gd->start_addr_sp);
 #ifdef CONFIG_SYS_NONCACHED_MEMORY
@@ -509,7 +520,7 @@ static int reserve_malloc(void)
 static int reserve_board(void)
 {
 	if (!gd->bd) {
-		gd->start_addr_sp -= sizeof(bd_t);
+		gd->start_addr_sp = reserve_sp(sizeof(bd_t));
 		gd->bd = (bd_t *)map_sysmem(gd->start_addr_sp, sizeof(bd_t));
 		memset(gd->bd, '\0', sizeof(bd_t));
 		debug("Reserving %zu Bytes for Board Info at: %08lx\n",
@@ -528,7 +539,7 @@ static int setup_machine(void)
 
 static int reserve_global_data(void)
 {
-	gd->start_addr_sp -= sizeof(gd_t);
+	gd->start_addr_sp = reserve_sp(sizeof(gd_t));
 	gd->new_gd = (gd_t *)map_sysmem(gd->start_addr_sp, sizeof(gd_t));
 	debug("Reserving %zu Bytes for Global Data at: %08lx\n",
 	      sizeof(gd_t), gd->start_addr_sp);
@@ -546,7 +557,7 @@ static int reserve_fdt(void)
 	if (gd->fdt_blob) {
 		gd->fdt_size = ALIGN(fdt_totalsize(gd->fdt_blob) + 0x1000, 32);
 
-		gd->start_addr_sp -= gd->fdt_size;
+		gd->start_addr_sp = reserve_sp(gd->fdt_size);
 		gd->new_fdt = map_sysmem(gd->start_addr_sp, gd->fdt_size);
 		debug("Reserving %lu Bytes for FDT at: %08lx\n",
 		      gd->fdt_size, gd->start_addr_sp);
@@ -561,12 +572,7 @@ static int reserve_bootstage(void)
 #ifdef CONFIG_BOOTSTAGE
 	int size = bootstage_get_size();
 
-	gd->start_addr_sp -= size;
-	/*
-	 * Insure that start_addr_sp is aligned down to reserve enough
-	 * space for new_bootstage
-	 */
-	gd->start_addr_sp = ALIGN_DOWN(gd->start_addr_sp, 16);
+	gd->start_addr_sp = reserve_sp(size);
 	gd->new_bootstage = map_sysmem(gd->start_addr_sp, size);
 	debug("Reserving %#x Bytes for bootstage at: %08lx\n", size,
 	      gd->start_addr_sp);
@@ -583,8 +589,7 @@ __weak int arch_reserve_stacks(void)
 static int reserve_stacks(void)
 {
 	/* make stack pointer 16-byte aligned */
-	gd->start_addr_sp -= 16;
-	gd->start_addr_sp &= ~0xf;
+	gd->start_addr_sp = reserve_sp(16);
 
 	/*
 	 * let the architecture-specific code tailor gd->start_addr_sp and
@@ -596,8 +601,7 @@ static int reserve_stacks(void)
 static int reserve_bloblist(void)
 {
 #ifdef CONFIG_BLOBLIST
-	gd->start_addr_sp &= ~0xf;
-	gd->start_addr_sp -= CONFIG_BLOBLIST_SIZE;
+	gd->start_addr_sp = reserve_sp(CONFIG_BLOBLIST_SIZE);
 	gd->new_bloblist = map_sysmem(gd->start_addr_sp, CONFIG_BLOBLIST_SIZE);
 #endif
 
-- 
2.17.1

[PATCH v2 1/4] board_f.c: Insure gd->new_bootstage alignment

[Stephen Warren]

On 1/22/20 6:52 AM, Patrick Delaunay wrote:
> From: Patrice Chotard <patrice.chotard@st.com>
> 
> In reserve_bootstage(), in case size is odd, gd->new_bootstage
> is not aligned. In bootstage_relocate(), the platform hangs when
> getting access to data->record[i].name.
> To avoid this issue, make gd->new_bootstage 16 byte aligned.
> 
> To insure that new_bootstage is 16 byte aligned (at least needed for
> x86_64 and ARMv8) and new_bootstage starts down to get enough space,
> ALIGN_DOWN macro is used.

> diff --git a/common/board_f.c b/common/board_f.c
> index d66afb37ca..e21f533634 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -562,6 +562,11 @@ static int reserve_bootstage(void)
>   	int size = bootstage_get_size();
>   
>   	gd->start_addr_sp -= size;
> +	/*
> +	 * Insure that start_addr_sp is aligned down to reserve enough

Nit: Ensure not insure (a pet peeve of mine!)

> +	 * space for new_bootstage
> +	 */
> +	gd->start_addr_sp = ALIGN_DOWN(gd->start_addr_sp, 16);
>   	gd->new_bootstage = map_sysmem(gd->start_addr_sp, size);
>   	debug("Reserving %#x Bytes for bootstage at: %08lx\n", size,
>   	      gd->start_addr_sp);
> 

[PATCH v2 4/4] board_f.c: Insure 16 alignment of start_addr_sp and reserved memory

[Stephen Warren]

On 1/22/20 6:52 AM, Patrick Delaunay wrote:
> Add a function reserve_sp() to reserved memory with 16 bits alignment
> after the stack pointer (gd->start_addr_sp) and use this new function
> in board_f.c to reserve all the memory area (malloc, board, gd, fdt,
> bootstage, stacks).
> 
> This 16 byte alignment is needed for cast on struct pointer
> for the reserved memory, for example:
> + x86_64 ABI: https://reviews.llvm.org/D30049: 16 bytes
> + ARMv8 Instruction Set Overview: quad word, 16 bytes
> 
> An other alignment value could be needed for other architecture.

> diff --git a/common/board_f.c b/common/board_f.c

>   
> +/*
> + * reserve after start_addr_sp the requested size and make the stack pointer
> + * 16-byte aligned, this alignment is needed for cast on the reserved memory
> + * ref = x86_64 ABI: https://reviews.llvm.org/D30049: 16 bytes
> + *     = ARMv8 Instruction Set Overview: quad word, 16 bytes
> + */
> +static unsigned long reserve_sp(size_t size)

Bikeshed: Not sure this name is very description. Perhaps 
reserve_stack_aligned() would be better. Not a big deal though. Patches 
1,4 look fine otherwise.

[PATCH v2 4/4] board_f.c: Insure 16 alignment of start_addr_sp and reserved memory

[Patrick DELAUNAY]

Hi Stephen,

> From: Stephen Warren <swarren@wwwdotorg.org>
> Sent: mercredi 22 janvier 2020 17:54
> 
> On 1/22/20 6:52 AM, Patrick Delaunay wrote:
> > Add a function reserve_sp() to reserved memory with 16 bits alignment
> > after the stack pointer (gd->start_addr_sp) and use this new function
> > in board_f.c to reserve all the memory area (malloc, board, gd, fdt,
> > bootstage, stacks).
> >
> > This 16 byte alignment is needed for cast on struct pointer for the
> > reserved memory, for example:
> > + x86_64 ABI: https://reviews.llvm.org/D30049: 16 bytes
> > + ARMv8 Instruction Set Overview: quad word, 16 bytes
> >
> > An other alignment value could be needed for other architecture.
> 
> > diff --git a/common/board_f.c b/common/board_f.c
> 
> >
> > +/*
> > + * reserve after start_addr_sp the requested size and make the stack
> > +pointer
> > + * 16-byte aligned, this alignment is needed for cast on the reserved
> > +memory
> > + * ref = x86_64 ABI: https://reviews.llvm.org/D30049: 16 bytes
> > + *     = ARMv8 Instruction Set Overview: quad word, 16 bytes
> > + */
> > +static unsigned long reserve_sp(size_t size)
> 
> Bikeshed: Not sure this name is very description. Perhaps
> reserve_stack_aligned() would be better. Not a big deal though. Patches
> 1,4 look fine otherwise.

Yes and thanks, I will update the comment.
I am waiting few day for sending v3.

Patrick

[PATCH v2 1/4] board_f.c: Insure gd->new_bootstage alignment

[Simon Glass]

On Wed, 22 Jan 2020 at 06:52, Patrick Delaunay <patrick.delaunay@st.com> wrote:
>
> From: Patrice Chotard <patrice.chotard@st.com>
>
> In reserve_bootstage(), in case size is odd, gd->new_bootstage
> is not aligned. In bootstage_relocate(), the platform hangs when
> getting access to data->record[i].name.
> To avoid this issue, make gd->new_bootstage 16 byte aligned.
>
> To insure that new_bootstage is 16 byte aligned (at least needed for
> x86_64 and ARMv8) and new_bootstage starts down to get enough space,
> ALIGN_DOWN macro is used.
>
> Fixes: ac9cd4805c8b ("bootstage: Correct relocation algorithm")
>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> Reviewed-by: Vikas MANOCHA <vikas.manocha@st.com>
> Reviewed-by: Patrick Delaunay <patrick.delaunay@st.com>
> Tested-by: Patrick Delaunay <patrick.delaunay@st.com>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
>
> Changes in v2:
> - import: [U-Boot,v3] board_f.c: Insure gd->new_bootstage alignment
>
>  common/board_f.c | 5 +++++
>  1 file changed, 5 insertions(+)

Ensure as Stephen says

With that fixed:

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v2 2/4] Revert "stm32mp1: remove the imply BOOTSTAGE"

[Simon Glass]

On Wed, 22 Jan 2020 at 06:52, Patrick Delaunay <patrick.delaunay@st.com> wrote:
>
> This reverts the workaround introduced by the
> commit 16fec9b0bc1a ("stm32mp1: remove the imply BOOTSTAGE")
> As the bootstage alignment issue is now solved.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
>
> Changes in v2: None
>
>  arch/arm/mach-stm32mp/Kconfig | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v2 3/4] arm: set the relocated gd with gd->new_gd

[Simon Glass]

On Wed, 22 Jan 2020 at 06:52, Patrick Delaunay <patrick.delaunay@st.com> wrote:
>
> Simplify the arm relocation behavior and get gd directly form new_gd,
> as it is already done in crt0_64.S:
>
>         ldr     x18, [x18, #GD_NEW_GD]          /* x18 <- gd->new_gd */
>
> This patch avoid assumption on new GD location (new GD is below bd -
> with #GD_SIZE offset).
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
>
> Changes in v2: None
>
>  arch/arm/lib/crt0.S | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>