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

[Patrick DELAUNAY]

Hi Tom,

> From: Tom Rini <trini at konsulko.com>
> Sent: mercredi 22 janvier 2020 00:18
> 
> On Thu, Jan 09, 2020 at 05:23:51PM +0000, Patrick DELAUNAY wrote:
> > 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.
> 
> Sorry for the delay.  Yes, please put together a single series that takes care of
> everything.  Thanks!

Done with serie = "Insure 16 alignment of reserved memory in board_f.c"

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

I could merge the patches 1/4 and 4/4 if it is more clear.
 
Regards

Patrick