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

[Patrick DELAUNAY]

Hi,

> From: Patrick DELAUNAY
> Sent: mardi 7 janvier 2020 13:07
> 
> 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.

I push a  serie, with my proposal:
[3/3] board_f.c: Insure 16 alignment of start_addr_sp and reserved memory

http://patchwork.ozlabs.org/project/uboot/list/?series=152226

As I found issue for ARM32 (I need to modify arch/arm/lib/crt0.S)
I think it is preferable that the Patrice Patch is merged in v2020.04, and my serie can live  independently.
But I can also squash of the 2 series.
 
Regards
Patrick