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

[Patrick DELAUNAY]

Hi Simon,

> From: Simon Glass <sjg at chromium.org>
> Sent: mardi 17 décembre 2019 16:46
> 
> Hi Patrice,
> 
> On Wed, 27 Nov 2019 at 02:11, Patrice Chotard <patrice.chotard at 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 at st.com>
> > Reviewed-by: Vikas MANOCHA <vikas.manocha at st.com>
> > Reviewed-by: Patrick Delaunay <patrick.delaunay at st.com>
> > Tested-by: Patrick Delaunay <patrick.delaunay at 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