public inbox for u-boot@lists.denx.de
 help / color / mirror / Atom feed
* [PATCH v3] board_f.c: Insure gd->new_bootstage alignment
@ 2020-01-07 12:07 Patrick DELAUNAY
  2020-01-09 17:23 ` Patrick DELAUNAY
  0 siblings, 1 reply; 9+ messages in thread
From: Patrick DELAUNAY @ 2020-01-07 12:07 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* [U-Boot] [PATCH v3] board_f.c: Insure gd->new_bootstage alignment
@ 2019-11-27  9:11 Patrice Chotard
  2019-12-16 11:53 ` Patrick DELAUNAY
  2019-12-17 15:46 ` Simon Glass
  0 siblings, 2 replies; 9+ messages in thread
From: Patrice Chotard @ 2019-11-27  9:11 UTC (permalink / raw)
  To: u-boot

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);
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-01-22 13:57 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-01-07 12:07 [PATCH v3] board_f.c: Insure gd->new_bootstage alignment Patrick DELAUNAY
2020-01-09 17:23 ` Patrick DELAUNAY
2020-01-21 23:18   ` Tom Rini
2020-01-22 13:57     ` Patrick DELAUNAY
  -- strict thread matches above, loose matches on Subject: below --
2019-11-27  9:11 [U-Boot] " Patrice Chotard
2019-12-16 11:53 ` Patrick DELAUNAY
2019-12-17 12:52   ` Tom Rini
2019-12-18  8:26     ` Patrick DELAUNAY
2019-12-17 15:46 ` Simon Glass
2019-12-18  9:10   ` Patrick DELAUNAY

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox