[U-Boot] [PATCH v3] board_f.c: Insure gd->new_bootstage alignment
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
---
Changes in v3: - Add Patrick Reviewed-by and Tested-by.
Changes in v2: - Update comment to explain the ALIGN_DOWN() usage.
common/board_f.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/common/board_f.c b/common/board_f.c index e3591cbaeb..d367f6b044 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -559,6 +559,11 @@ static int reserve_bootstage(void) int size = bootstage_get_size();
gd->start_addr_sp -= size; + /* + * Insure that start_addr_sp is aligned down to reserve enough + * space for new_bootstage + */ + gd->start_addr_sp = ALIGN_DOWN(gd->start_addr_sp, 16); gd->new_bootstage = map_sysmem(gd->start_addr_sp, size); debug("Reserving %#x Bytes for bootstage at: %08lx\n", size, gd->start_addr_sp);
Hi Tom,
From: Patrice CHOTARD patrice.chotard@st.com Sent: mercredi 27 novembre 2019 10:12
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
Do you plan to merge this fixe for the next rc for v2020.01 ? Or do you expect some change / review.
This patch is mandatory for stm32mp1 (ARM plaform with bootstage feature activated). Without this patch, the boot failed (at least for v2020.01-rc3 : crash has struct pointer new_bootstage is not aligned).
Or I will deactivate the BOOTSTAGE feature...
Thanks
Patrick
Changes in v3:
- Add Patrick Reviewed-by and Tested-by.
Changes in v2:
- Update comment to explain the ALIGN_DOWN() usage.
common/board_f.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/common/board_f.c b/common/board_f.c index e3591cbaeb..d367f6b044 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -559,6 +559,11 @@ static int reserve_bootstage(void) int size = bootstage_get_size();
gd->start_addr_sp -= size;
- /*
* Insure that start_addr_sp is aligned down to reserve enough* space for new_bootstage*/- gd->start_addr_sp = ALIGN_DOWN(gd->start_addr_sp, 16); gd->new_bootstage = map_sysmem(gd->start_addr_sp, size); debug("Reserving %#x Bytes for bootstage at: %08lx\n", size, gd->start_addr_sp);
-- 2.17.1
On Mon, Dec 16, 2019 at 11:53:48AM +0000, Patrick DELAUNAY wrote:
Hi Tom,
From: Patrice CHOTARD patrice.chotard@st.com Sent: mercredi 27 novembre 2019 10:12
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
Do you plan to merge this fixe for the next rc for v2020.01 ? Or do you expect some change / review.
This patch is mandatory for stm32mp1 (ARM plaform with bootstage feature activated). Without this patch, the boot failed (at least for v2020.01-rc3 : crash has struct pointer new_bootstage is not aligned).
Or I will deactivate the BOOTSTAGE feature...
I think at this point I would prefer dropping BOOTSTAGE from those boards for the release. There's already been more than one "I think this is safe" followed by "this broke ..." changes I've tried. Sorry!
Hi Tom,
From: Tom Rini trini@konsulko.com Sent: mardi 17 décembre 2019 13:52
On Mon, Dec 16, 2019 at 11:53:48AM +0000, Patrick DELAUNAY wrote:
Hi Tom,
From: Patrice CHOTARD patrice.chotard@st.com Sent: mercredi 27 novembre 2019 10:12
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
Do you plan to merge this fixe for the next rc for v2020.01 ? Or do you expect some change / review.
This patch is mandatory for stm32mp1 (ARM plaform with bootstage feature activated). Without this patch, the boot failed (at least for v2020.01-rc3 : crash has struct pointer new_bootstage is not aligned).
Or I will deactivate the BOOTSTAGE feature...
I think at this point I would prefer dropping BOOTSTAGE from those boards for the release. There's already been more than one "I think this is safe" followed by "this broke ..." changes I've tried. Sorry!
I know the number of issue with "it should work".... and I understood. I sill push a patch to deactivate bootstatge (remove 2 "imply" in fact) and a new pull request.
-- Tom
BR Patrick
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 this patch I think it would be better to update reserve_fdt() to keep things aligned, assuming that is the problem.
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.
Regards, Simon
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
participants (4)
-
Patrice Chotard -
Patrick DELAUNAY -
Simon Glass -
Tom Rini