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

[Patrick DELAUNAY]

Hi Patrice and Tom

> Sent: mercredi 18 décembre 2019 10:10
> 
> 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.

I am preparing this patch.... 

Do you think it is ok to merge the Patrice v3 proposal first on master branch
for v2020.04 release (he just align the reserved memory for bootstage),
and after I make my patch (16-byte align all reserved area).

or it is better to make a more generic patch v4 to replace the Patrice one.

Regards
Patrick