[U-Boot] [PATCH v3] board_f.c: Insure gd->new_bootstage alignment

[U-Boot] [PATCH v3] board_f.c: Insure gd->new_bootstage alignment

[Patrice Chotard]

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>

---

Changes in v3:
  - Add Patrick Reviewed-by and Tested-by.

Changes in v2:
  - Update comment to explain the ALIGN_DOWN() usage.

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

diff --git a/common/board_f.c b/common/board_f.c
index e3591cbaeb..d367f6b044 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -559,6 +559,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 v3] board_f.c: Insure gd->new_bootstage alignment

[Patrick DELAUNAY]

Hi Tom,

> From: Patrice CHOTARD <patrice.chotard@st.com>
> Sent: mercredi 27 novembre 2019 10:12
> 
> 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>
> 

Do you plan to merge this fixe for the next rc for v2020.01 ?
Or do you expect some change / review.

This patch is mandatory for stm32mp1 (ARM plaform with bootstage feature activated).
Without this patch, the boot failed (at least for v2020.01-rc3 : crash has struct pointer new_bootstage is not aligned).

Or I will deactivate the BOOTSTAGE feature...

Thanks 

Patrick

> ---
> 
> Changes in v3:
>   - Add Patrick Reviewed-by and Tested-by.
> 
> Changes in v2:
>   - Update comment to explain the ALIGN_DOWN() usage.
> 
>  common/board_f.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/common/board_f.c b/common/board_f.c index e3591cbaeb..d367f6b044
> 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -559,6 +559,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 v3] board_f.c: Insure gd->new_bootstage alignment

[Tom Rini]

On Mon, Dec 16, 2019 at 11:53:48AM +0000, Patrick DELAUNAY wrote:
> Hi Tom,
> 
> > From: Patrice CHOTARD <patrice.chotard@st.com>
> > Sent: mercredi 27 novembre 2019 10:12
> > 
> > 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>
> > 
> 
> Do you plan to merge this fixe for the next rc for v2020.01 ?
> Or do you expect some change / review.
>
> This patch is mandatory for stm32mp1 (ARM plaform with bootstage
> feature activated).
> Without this patch, the boot failed (at least for v2020.01-rc3 : crash
> has struct pointer new_bootstage is not aligned).
> 
> Or I will deactivate the BOOTSTAGE feature...
 
I think at this point I would prefer dropping BOOTSTAGE from those
boards for the release.  There's already been more than one "I think
this is safe" followed by "this broke ..." changes I've tried.  Sorry!

-- 
Tom

[PATCH v3] board_f.c: Insure gd->new_bootstage alignment

[Simon Glass]

Hi Patrice,

On Wed, 27 Nov 2019 at 02:11, Patrice Chotard <patrice.chotard@st.com> wrote:
>
> 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>

For this patch I think it would be better to update reserve_fdt() to
keep things aligned, assuming that is the problem.

At some point we should also document that reservations must keep
things aligned.

Perhaps this should be handled by a separate function called from all
these places, which subtracts gd->start_addr_sp and ensures 16-byte
alignment.

Regards,
Simon

[PATCH v3] board_f.c: Insure gd->new_bootstage alignment

[Patrick DELAUNAY]

Hi Tom,

> From: Tom Rini <trini@konsulko.com>
> Sent: mardi 17 décembre 2019 13:52
> 
> On Mon, Dec 16, 2019 at 11:53:48AM +0000, Patrick DELAUNAY wrote:
> > Hi Tom,
> >
> > > From: Patrice CHOTARD <patrice.chotard@st.com>
> > > Sent: mercredi 27 novembre 2019 10:12
> > >
> > > 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>
> > >
> >
> > Do you plan to merge this fixe for the next rc for v2020.01 ?
> > Or do you expect some change / review.
> >
> > This patch is mandatory for stm32mp1 (ARM plaform with bootstage
> > feature activated).
> > Without this patch, the boot failed (at least for v2020.01-rc3 : crash
> > has struct pointer new_bootstage is not aligned).
> >
> > Or I will deactivate the BOOTSTAGE feature...
> 
> I think at this point I would prefer dropping BOOTSTAGE from those boards for
> the release.  There's already been more than one "I think this is safe" followed by
> "this broke ..." changes I've tried.  Sorry!

I know the number of issue with  "it should work".... and I understood.
I sill push a patch to deactivate bootstatge (remove 2 "imply" in fact) and a new pull request.


> --
> Tom

BR
Patrick

[PATCH v3] board_f.c: Insure gd->new_bootstage alignment

[Patrick DELAUNAY]

Hi Simon,

> From: Simon Glass <sjg@chromium.org>
> Sent: mardi 17 décembre 2019 16:46
> 
> Hi Patrice,
> 
> On Wed, 27 Nov 2019 at 02:11, Patrice Chotard <patrice.chotard@st.com> wrote:
> >
> > 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>

For information, Patrice is absent for personal reason up to beginning next year.
Don't wait a fast answer.

> For this patch I think it would be better to update reserve_fdt() to keep things
> aligned, assuming that is the problem.

I don't sure that solve the issue, 
for me the problem is only for the bootstage struct (gd->bootstage)
And reserve_fdt() already alligne size on 32 bytes

If I remember the Patrice analysis:

1- bootstage_get_size return a odd value (or at least not 16 bytes aligned I don't remember).

2- In reserve_bootstage()
	int size = bootstage_get_size();
	gd->start_addr_sp -= size
	=> it is a unaligned address even if gd->start_addr_sp is 32 bytes alligned

	gd->new_bootstage = map_sysmem(gd->start_addr_sp, size);
	=> also unaligned

3- Then during relocation, in reloc_bootstage()
	gd->bootstage = gd->new_bootstage;


4- crash occur because in next bootstage function beaucse the boostage address don't respect pointer to struct allignement...

	struct bootstage_data *data = gd->bootstage


> At some point we should also document that reservations must keep things
> aligned.
> 
> Perhaps this should be handled by a separate function called from all these
> places, which subtracts gd->start_addr_sp and ensures 16-byte alignment.

Yes that can be a improvement,  but perhaps ia a second step / second serie....

Do you think about a function called in all reversed_ functions (when start_addr_sp is modified)...

static int reserved_allign_check(void)
{
	/* make stack pointer 16-byte aligned */
	if (gd->start_addr_sp & 0xf) {
		gd->start_addr_sp -= 16;
		gd->start_addr_sp &= ~0xf;
	 }
}

I prefer a function to reserved a size wich replace in any reserve_ function  the line : 
	gd->start_addr_sp -= ...

/* reserve size and make stack pointer 16-byte aligned */
static int reserve(size_t size)
{
	gd->start_addr_sp -= size;
	/* make stack pointer 16-byte aligned */
	gd->start_addr_sp = ALIGN_DOWN(gd->start_addr_sp, 16);
}

I think I will push it, when the patrice patch will be accepted.

> Regards,
> Simon

Regards
Patrick