RE: [PATCH v3] board_f.c: Insure gd->new_bootstage alignment
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.
Regards Patrick
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.
Regards Patrick
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!
Hi Tom,
From: Tom Rini trini@konsulko.com Sent: mercredi 22 janvier 2020 00:18
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!
Done with serie = "Insure 16 alignment of reserved memory in board_f.c"
http://patchwork.ozlabs.org/project/uboot/list/?series=154685
I could merge the patches 1/4 and 4/4 if it is more clear.
Regards
Patrick
participants (2)
-
Patrick DELAUNAY -
Tom Rini