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

[Tom Rini <trini@konsulko.com>]

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@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.
> > 
> > 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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200121/12e6a0d7/attachment.sig>