[U-Boot] [PATCH 3/4] fix memory corruption on versatile

[U-Boot] [PATCH 3/4] fix memory corruption on versatile

[Marek Vasut]

Dear Pavel Herrmann,

> ARM board.c doesnt respect CONFIG_SYS_GBL_DATA_OFFSET, nor do all boards
> set it, so reorganize the memory a bit to avoid overlaps.
> 
> Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>

Ccing Albert

> ---
>  include/configs/versatile.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/configs/versatile.h b/include/configs/versatile.h
> index c9aed72..27ffffe 100644
> --- a/include/configs/versatile.h
> +++ b/include/configs/versatile.h
> @@ -35,7 +35,7 @@
>  #define CONFIG_DM_BLOCK
>  #define CONFIG_SYS_EARLY_MALLOC
>  #define CONFIG_SYS_EARLY_HEAP_ADDR	(CONFIG_SYS_INIT_RAM_ADDR + \
> -						CONFIG_SYS_GBL_DATA_OFFSET - \
> +						CONFIG_SYS_INIT_RAM_SIZE - \
>  						CONFIG_SYS_EARLY_HEAP_SIZE - 64)
>  #define CONFIG_SYS_EARLY_HEAP_SIZE	1024

Best regards,
Marek Vasut

[U-Boot] [PATCH 3/4] fix memory corruption on versatile

[Albert ARIBAUD]

Hi Pavel,

On Mon, 24 Dec 2012 02:27:53 +0100, Marek Vasut <marex@denx.de> wrote:
> Dear Pavel Herrmann,
> 
> > ARM board.c doesnt respect CONFIG_SYS_GBL_DATA_OFFSET, nor do all boards
> > set it, so reorganize the memory a bit to avoid overlaps.
> > 
> > Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
> 
> Ccing Albert
> 
> > ---
> >  include/configs/versatile.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/configs/versatile.h b/include/configs/versatile.h
> > index c9aed72..27ffffe 100644
> > --- a/include/configs/versatile.h
> > +++ b/include/configs/versatile.h
> > @@ -35,7 +35,7 @@
> >  #define CONFIG_DM_BLOCK
> >  #define CONFIG_SYS_EARLY_MALLOC
> >  #define CONFIG_SYS_EARLY_HEAP_ADDR	(CONFIG_SYS_INIT_RAM_ADDR + \
> > -						CONFIG_SYS_GBL_DATA_OFFSET - \
> > +						CONFIG_SYS_INIT_RAM_SIZE - \
> >  						CONFIG_SYS_EARLY_HEAP_SIZE - 64)
> >  #define CONFIG_SYS_EARLY_HEAP_SIZE	1024
> 
> Best regards,
> Marek Vasut

I can't seem to find this patch in my inbox or in the list archives.

Pavel, can you please repost it?

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 3/4] fix memory corruption on versatile

[Pavel Herrmann]

Hi,

On Monday 24 December 2012 14:56:03 Albert ARIBAUD wrote:
> Hi Pavel,
> 
> On Mon, 24 Dec 2012 02:27:53 +0100, Marek Vasut <marex@denx.de> wrote:
> > Dear Pavel Herrmann,
> > 
> > > ARM board.c doesnt respect CONFIG_SYS_GBL_DATA_OFFSET, nor do all boards
> > > set it, so reorganize the memory a bit to avoid overlaps.
> > > 
> > > Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
> > 
> > Ccing Albert
> > 
> > > ---
> > > 
> > >  include/configs/versatile.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/include/configs/versatile.h b/include/configs/versatile.h
> > > index c9aed72..27ffffe 100644
> > > --- a/include/configs/versatile.h
> > > +++ b/include/configs/versatile.h
> > > @@ -35,7 +35,7 @@
> > > 
> > >  #define CONFIG_DM_BLOCK
> > >  #define CONFIG_SYS_EARLY_MALLOC
> > >  #define CONFIG_SYS_EARLY_HEAP_ADDR	(CONFIG_SYS_INIT_RAM_ADDR + \
> > > 
> > > -						CONFIG_SYS_GBL_DATA_OFFSET - \
> > > +						CONFIG_SYS_INIT_RAM_SIZE - \
> > > 
> > >  						CONFIG_SYS_EARLY_HEAP_SIZE - 64)
> > >  
> > >  #define CONFIG_SYS_EARLY_HEAP_SIZE	1024
> > 
> > Best regards,
> > Marek Vasut
> 
> I can't seem to find this patch in my inbox or in the list archives.
> 
> Pavel, can you please repost it?
> 
> Amicalement,

I dont really understand why Marek CCed you, this patch is meant to go in the 
DM tree, and i dont see why it should be considered for upstream.

The problem this is trying to solve is that some boards (like the versatile i 
use, as it has upstream qemu support) do define a suspicious looking 
CONFIG_SYS_GBL_DATA_OFFSET, but that macro is not used in the board_init_f. 
instead the GD is placed under the bottom of stack.

This is perfectly fine when we only have GD and stack in memory, but I would 
like to have the early heap in there somewhere, ideally between GD and stack. 
When I set the macros (not knowing that it is actually ignored) to represent 
this layout, the boot failed because the stack was overlapping the GD, so this 
patch changed the layout to [stack][GD][heap][end].

the "optimal" solution would be to have universal (as in that all archs 
board_init_f()s would respect them, and all board configs would have them) 
macros for address of GD, early heap and stack independently (with the obvious 
exception that for stack you have the highest address, while for other you 
have the lowest), so that we can have the originally intended layout ([stack]
[heap][GD][end]), or any other layout for that matter.

This, of course, may not be possible due to various limitations of various 
archs, so I did not attempt to make any such patch, instead just quick-fix the 
board i am using to test early DM initialization.

Hope this clears the confusion

Pavel Herrmann

[U-Boot] [PATCH 3/4] fix memory corruption on versatile

[Albert ARIBAUD]

Hi Pavel,

On Mon, 24 Dec 2012 15:57:30 +0100, Pavel Herrmann
<morpheus.ibis@gmail.com> wrote:
> Hi,
> 
> On Monday 24 December 2012 14:56:03 Albert ARIBAUD wrote:
> > Hi Pavel,
> > 
> > On Mon, 24 Dec 2012 02:27:53 +0100, Marek Vasut <marex@denx.de> wrote:
> > > Dear Pavel Herrmann,
> > > 
> > > > ARM board.c doesnt respect CONFIG_SYS_GBL_DATA_OFFSET, nor do all boards
> > > > set it, so reorganize the memory a bit to avoid overlaps.
> > > > 
> > > > Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
> > > 
> > > Ccing Albert
> > > 
> > > > ---
> > > > 
> > > >  include/configs/versatile.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/include/configs/versatile.h b/include/configs/versatile.h
> > > > index c9aed72..27ffffe 100644
> > > > --- a/include/configs/versatile.h
> > > > +++ b/include/configs/versatile.h
> > > > @@ -35,7 +35,7 @@
> > > > 
> > > >  #define CONFIG_DM_BLOCK
> > > >  #define CONFIG_SYS_EARLY_MALLOC
> > > >  #define CONFIG_SYS_EARLY_HEAP_ADDR	(CONFIG_SYS_INIT_RAM_ADDR + \
> > > > 
> > > > -						CONFIG_SYS_GBL_DATA_OFFSET - \
> > > > +						CONFIG_SYS_INIT_RAM_SIZE - \
> > > > 
> > > >  						CONFIG_SYS_EARLY_HEAP_SIZE - 64)
> > > >  
> > > >  #define CONFIG_SYS_EARLY_HEAP_SIZE	1024
> > > 
> > > Best regards,
> > > Marek Vasut
> > 
> > I can't seem to find this patch in my inbox or in the list archives.
> > 
> > Pavel, can you please repost it?
> > 
> > Amicalement,
> 
> I dont really understand why Marek CCed you, this patch is meant to go in the 
> DM tree, and i dont see why it should be considered for upstream.

Patches should not be meant for a tree in the first place; they should
be meant for the tree where they make most sense. IIUC, this patch
fixes an issue related not to DM per se but to ARM, which is why Marek
CC:ed me (and the list).

> The problem this is trying to solve is that some boards (like the versatile i 
> use, as it has upstream qemu support) do define a suspicious looking 
> CONFIG_SYS_GBL_DATA_OFFSET, but that macro is not used in the board_init_f. 
> instead the GD is placed under the bottom of stack.
> 
> This is perfectly fine when we only have GD and stack in memory, but I would 
> like to have the early heap in there somewhere, ideally between GD and stack. 
> When I set the macros (not knowing that it is actually ignored) to represent 
> this layout, the boot failed because the stack was overlapping the GD, so this 
> patch changed the layout to [stack][GD][heap][end].
> 
> the "optimal" solution would be to have universal (as in that all archs 
> board_init_f()s would respect them, and all board configs would have them) 
> macros for address of GD, early heap and stack independently (with the obvious 
> exception that for stack you have the highest address, while for other you 
> have the lowest), so that we can have the originally intended layout ([stack]
> [heap][GD][end]), or any other layout for that matter.
> 
> This, of course, may not be possible due to various limitations of various 
> archs, so I did not attempt to make any such patch, instead just quick-fix the 
> board i am using to test early DM initialization.
> 
> Hope this clears the confusion

It does; I think I understand the problem you are stating. The only
problem I have is that I am still unable to tell if Marek did quote the
whole patch or not. :)

Based on the assumption that this patch is complete as quoted, and on
your comments above, my comment would be (to both Marek and you) why do
you nead a heap during flash-based inits?

> Pavel Herrmann

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 3/4] fix memory corruption on versatile

[Pavel Herrmann]

Hi,

On Tuesday 25 December 2012 12:37:55 Albert ARIBAUD wrote:
> Hi Pavel,
> 
> On Mon, 24 Dec 2012 15:57:30 +0100, Pavel Herrmann
> 
> <morpheus.ibis@gmail.com> wrote:
> > Hi,
> > 
> > On Monday 24 December 2012 14:56:03 Albert ARIBAUD wrote:
> > > Hi Pavel,
> > > 
> > > On Mon, 24 Dec 2012 02:27:53 +0100, Marek Vasut <marex@denx.de> wrote:
> > > > Dear Pavel Herrmann,
> > > > 
> > > > > ARM board.c doesnt respect CONFIG_SYS_GBL_DATA_OFFSET, nor do all
> > > > > boards
> > > > > set it, so reorganize the memory a bit to avoid overlaps.
> > > > > 
> > > > > Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
> > > > 
> > > > Ccing Albert
> > > > 
> > > > > ---
> > > > > 
> > > > >  include/configs/versatile.h | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/include/configs/versatile.h
> > > > > b/include/configs/versatile.h
> > > > > index c9aed72..27ffffe 100644
> > > > > --- a/include/configs/versatile.h
> > > > > +++ b/include/configs/versatile.h
> > > > > @@ -35,7 +35,7 @@
> > > > > 
> > > > >  #define CONFIG_DM_BLOCK
> > > > >  #define CONFIG_SYS_EARLY_MALLOC
> > > > >  #define CONFIG_SYS_EARLY_HEAP_ADDR	(CONFIG_SYS_INIT_RAM_ADDR + \
> > > > > 
> > > > > -						CONFIG_SYS_GBL_DATA_OFFSET - \
> > > > > +						CONFIG_SYS_INIT_RAM_SIZE - \
> > > > > 
> > > > >  						CONFIG_SYS_EARLY_HEAP_SIZE - 64)
> > > > >  
> > > > >  #define CONFIG_SYS_EARLY_HEAP_SIZE	1024
> > > > 
> > > > Best regards,
> > > > Marek Vasut
> > > 
> > > I can't seem to find this patch in my inbox or in the list archives.
> > > 
> > > Pavel, can you please repost it?
> > > 
> > > Amicalement,
> > 
> > I dont really understand why Marek CCed you, this patch is meant to go in
> > the DM tree, and i dont see why it should be considered for upstream.
> 
> Patches should not be meant for a tree in the first place; they should
> be meant for the tree where they make most sense. IIUC, this patch
> fixes an issue related not to DM per se but to ARM, which is why Marek
> CC:ed me (and the list).

More like it applies to me being stupid, and not checking whether the 
CONFIG_SYS_GBL_DATA_OFFSET is actually used in the code before trying to use 
it to modify the memory layout

> > The problem this is trying to solve is that some boards (like the
> > versatile i use, as it has upstream qemu support) do define a suspicious
> > looking CONFIG_SYS_GBL_DATA_OFFSET, but that macro is not used in the
> > board_init_f. instead the GD is placed under the bottom of stack.
> > 
> > This is perfectly fine when we only have GD and stack in memory, but I
> > would like to have the early heap in there somewhere, ideally between GD
> > and stack. When I set the macros (not knowing that it is actually
> > ignored) to represent this layout, the boot failed because the stack was
> > overlapping the GD, so this patch changed the layout to
> > [stack][GD][heap][end].
> > 
> > the "optimal" solution would be to have universal (as in that all archs
> > board_init_f()s would respect them, and all board configs would have them)
> > macros for address of GD, early heap and stack independently (with the
> > obvious exception that for stack you have the highest address, while for
> > other you have the lowest), so that we can have the originally intended
> > layout ([stack] [heap][GD][end]), or any other layout for that matter.
> > 
> > This, of course, may not be possible due to various limitations of various
> > archs, so I did not attempt to make any such patch, instead just quick-fix
> > the board i am using to test early DM initialization.
> > 
> > Hope this clears the confusion
> 
> It does; I think I understand the problem you are stating. The only
> problem I have is that I am still unable to tell if Marek did quote the
> whole patch or not. :)

Technically, the patch was quoted whole, but it goes on top of another, that 
added the DM and early heap macros to the board config, so its of no use as it 
is.

> Based on the assumption that this patch is complete as quoted, and on
> your comments above, my comment would be (to both Marek and you) why do
> you nead a heap during flash-based inits?

We want the DM to replace most of the board-specific code (by providing a 
"driver" that is configured by board-specific values, essentialy encapsulating 
common init code independently on actual board and memory layout), so that the 
board-specific init only loads required drivers with correct parameters, 
instead of directly initializing the hardware (there was a thought of having a 
RAM driver, that would init the main memory when loaded, and provided 
relcation as a method/service, not sure if we want to go this far at the 
moment).
for this we need the DM to run in early (flash-based) phase. The DM itself 
cannot work without a heap, so we need one there.

Pavel Herrmann

[U-Boot] [PATCH 3/4] fix memory corruption on versatile

[Albert ARIBAUD]

Hi Pavel,

> Technically, the patch was quoted whole, but it goes on top of another,
> that added the DM and early heap macros to the board config, so its of
> no use as it is.

Ok, so IIUC this patch is not required for fixing anything in the
current mainline u-boot, and would not break anything in mainline
u-boot either, but is required to arrange pre-relocation memory mapping
for current DM code to work.

> > Based on the assumption that this patch is complete as quoted, and on
> > your comments above, my comment would be (to both Marek and you) why do
> > you nead a heap during flash-based inits?
> 
> We want the DM to replace most of the board-specific code (by providing a 
> "driver" that is configured by board-specific values, essentialy encapsulating 
> common init code independently on actual board and memory layout), so that the 
> board-specific init only loads required drivers with correct parameters, 
> instead of directly initializing the hardware (there was a thought of having a 
> RAM driver, that would init the main memory when loaded, and provided 
> relcation as a method/service, not sure if we want to go this far at the 
> moment).
> for this we need the DM to run in early (flash-based) phase. The DM itself 
> cannot work without a heap, so we need one there.

Well, for lack of understanding of DM (I know DM documentation is out
there, just haven't had time to look it up so far) I cannot tell
whether it cannot work without a heap or whether it could work without
it but hasn't been designed that way.

This leads to two questions:

1) Why does the DM need a heap in the first place? When you look at the
DM requirements (as I understand them), they basically include a full C
runtime environment, which is precisely what we do *not* provide in the
first stage of U-Boot, because this first stage is *meant* to set the C
runtime environment up.

2) Assuming these requirements can be met in a viable way, is this heap
supposed to survive through relocation? And if it is, then how will it,
and most of all, how will references to it, remain consistent without
an ugly manual relocation fixup process?

> Pavel Herrmann

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 3/4] fix memory corruption on versatile

[Pavel Herrmann]

Hi

On Thursday 27 of December 2012 09:59:07 Albert ARIBAUD wrote:
> Hi Pavel,
> 
> > Technically, the patch was quoted whole, but it goes on top of another,
> > that added the DM and early heap macros to the board config, so its of
> > no use as it is.
> 
> Ok, so IIUC this patch is not required for fixing anything in the
> current mainline u-boot, and would not break anything in mainline
> u-boot either, but is required to arrange pre-relocation memory mapping
> for current DM code to work.

Essentially yes, except it would not apply without the first patch. if you 
applied both to mainline, then nothing would hapen (some space would be 
allocated for the early heap, that would never get used), as you dont have any 
DM code there, so it would be just a useless defined macro.

> > > Based on the assumption that this patch is complete as quoted, and on
> > > your comments above, my comment would be (to both Marek and you) why do
> > > you nead a heap during flash-based inits?
> > 
> > We want the DM to replace most of the board-specific code (by providing a
> > "driver" that is configured by board-specific values, essentialy
> > encapsulating common init code independently on actual board and memory
> > layout), so that the board-specific init only loads required drivers with
> > correct parameters, instead of directly initializing the hardware (there
> > was a thought of having a RAM driver, that would init the main memory
> > when loaded, and provided relcation as a method/service, not sure if we
> > want to go this far at the moment).
> > for this we need the DM to run in early (flash-based) phase. The DM itself
> > cannot work without a heap, so we need one there.
> 
> Well, for lack of understanding of DM (I know DM documentation is out
> there, just haven't had time to look it up so far) I cannot tell
> whether it cannot work without a heap or whether it could work without
> it but hasn't been designed that way.
> 
> This leads to two questions:
> 
> 1) Why does the DM need a heap in the first place? When you look at the
> DM requirements (as I understand them), they basically include a full C
> runtime environment, which is precisely what we do *not* provide in the
> first stage of U-Boot, because this first stage is *meant* to set the C
> runtime environment up.

Yes, there are 2 problems (or maybe more) with C in the flash phase - you dont 
have a heap, and everything except your stack is read-only.
the early heap solves the first issue, and the DM is aimed at solving the 
second problem, as we avoid using any global data (well, except for the root 
of DM driver tree, which is in GD), and have everything in per-driver-instance 
data structures, which are allocated on the heap

> 2) Assuming these requirements can be met in a viable way, is this heap
> supposed to survive through relocation? And if it is, then how will it,
> and most of all, how will references to it, remain consistent without
> an ugly manual relocation fixup process?

the heap is supposed to survive relocation, but only for a little while.
what we do is taht after setting up normal heap (and dlmalloc, instead of the 
simple malloc we have in the early heap), we order each driver to relocate to 
a new address. basically we have a "ugly manual relocation fixup process", that 
is made manageable by dividing it between the individual drivers (as only the 
driver knows what private data it created during initialization). we have an 
infrastructure in place that walks the DM driver tree and calls these 
relocation hooks in a sane order. after this is done, we forget about the 
early heap (enable caches if it was in locked cache or whatever).

Pavel Herrmann

[U-Boot] [PATCH 3/4] fix memory corruption on versatile

[Albert ARIBAUD]

Hi Pavel,

> > 1) Why does the DM need a heap in the first place? When you look at the
> > DM requirements (as I understand them), they basically include a full C
> > runtime environment, which is precisely what we do *not* provide in the
> > first stage of U-Boot, because this first stage is *meant* to set the C
> > runtime environment up.
> 
> Yes, there are 2 problems (or maybe more) with C in the flash phase - you dont 
> have a heap, and everything except your stack is read-only.

These are not problems, these are initial conditions. The Flash phase
is precisely there to set up the hardware for running C code, so one
simply cannot expect a working C runtime there; and most of the code is
read-only because that's what Flash is about.

> the early heap solves the first issue, and the DM is aimed at solving the 
> second problem, as we avoid using any global data (well, except for the root 
> of DM driver tree, which is in GD), and have everything in per-driver-instance 
> data structures, which are allocated on the heap

Apart from being somewhat simplistically expressed ("problem: there is
no heap; solution: provide a heap"), the first "issue" does not exist
currently in U-Boot: no heap is needed by the code currently running in
the flash phase, only DM apparently needs it. so this first "issue"
only appears because of the solution brought to the second "issue".

As for the second "issue", it already has a solution, the GD, used by
existing code. What makes it impossible for the DM to extend the GD
structure with whatever data the DM must pass from flash to ram phase?

> > 2) Assuming these requirements can be met in a viable way, is this heap
> > supposed to survive through relocation? And if it is, then how will it,
> > and most of all, how will references to it, remain consistent without
> > an ugly manual relocation fixup process?
> 
> the heap is supposed to survive relocation, but only for a little while.

All the more an indication that it may not be needed at all.

> what we do is taht after setting up normal heap (and dlmalloc, instead of the 
> simple malloc we have in the early heap), we order each driver to relocate to 
> a new address. basically we have a "ugly manual relocation fixup process", that 
> is made manageable by dividing it between the individual drivers (as only the 
> driver knows what private data it created during initialization). we have an 
> infrastructure in place that walks the DM driver tree and calls these 
> relocation hooks in a sane order. after this is done, we forget about the 
> early heap (enable caches if it was in locked cache or whatever).

That's awful. I really wish I'd had more time to have a look at the big
picture of DM design earlier, because right now, it makes assumptions
about the available environment that are contrary to U-Boot's and
introduces back manual relocations which I took pains to get removed in
ARM.

So, new question:

1) Why does DM need a heap exactly? Can GD not be used or
adapted instead? How does DM ensure that the flash-based part will
not need more heap than available, and if it doesn't ensure it, How does
it handle overlimit conditions?

2) What are the types of manual relocations that DM requires? Why does
it require them at all?

> Pavel Herrmann

Amicalement,
-- 
Albert.