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.
> Regards,
> Simon
Regards
Patrick
