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 <s...@chromium.org>
> > > > Sent: mardi 17 décembre 2019 16:46
> > > >
> > > > Hi Patrice,
> > > >
> > > > On Wed, 27 Nov 2019 at 02:11, Patrice Chotard
> > > > <patrice.chot...@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.chot...@st.com>
> > > > > Reviewed-by: Vikas MANOCHA <vikas.mano...@st.com>
> > > > > Reviewed-by: Patrick Delaunay <patrick.delau...@st.com>
> > > > > Tested-by: Patrick Delaunay <patrick.delau...@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
signature.asc
Description: PGP signature
