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