[U-Boot] [PATCH] common/memsize.c: restore content of the base address

Thu Dec 7 15:48:54 UTC 2017

Dear Wolfgang,

> 
> Dear Patrick,
> 
> In message <1512575263-23010-1-git-send-email-patrick.delaunay at st.com>
> you wrote:
> > In function get_ram_size() and for 2 last cases the content of the
> > base address (*base)  is not restored even it is correctly saved in
> > stack (in save[i]).
> >
> > This patch solved this issue.
> > The content of the base address is saved in new variable in stack
> > (save_base) to avoid the need of other information (value of i) and
> > restored in all the cases.
> 
> What exactly is the problem you are trying to fix?  How exactly does it manifest
> for you?

My issue is that the base address (*base)  in not restored at the end of the test.
I will explain the sequence in details after.

I know that this code is sensible because I already debug a spurious issue on this function some month ago.
I had abort during get_ram_size() execution but problem disappear when I change just a little the code.

After investigation, I found an potential issue when the current code of get_ram_size() 
is loaded near of power of 2 offset (just before an address modified by the code)... 
In fact the content of the RAM is modified during the code execution just after the
PC address, and the code is not yet in instruction cache, so this the code crash.
I try to found a good solution (limit the min offset to be tested, 
to avoid to modify the address used by U-Boot code)

You can found analysis in :
  [U-Boot] questions about get_ram_size usage in U-Boot = DDR modification during code execution
  From Patrick DELAUNAY Mon Apr 24 16:11:41 UTC 2017
  https://lists.denx.de/pipermail/u-boot/2017-April/288709.html

Without answer, I prefer don't modify the code and finally I avoid the issue just by loading U-boot at 1MB offset 
from the beginning of the DDR, so get_ram_size() is no more loaded at power of 2 offset...

But I suspect this case can also occur on other board when U-Boot is loaded at address==base, 
when the address of get_ram_size() change (even a little) after any modification.

> 
> On which boards/architectures did you observe this problem, and on which did
> you actually test your patch?

I use a custom board, with armv7 cortex A7 chip....
But it is not yet up-streamed, I plan to upstream this board next year when the validation
will be finished and when this board will be available outside ST.

Unfortunately I test my patch only on my board:
The preloader (I don't use not SPL for this test but a custom loader) load a file with checksum 
at RAM start and U-Boot at 1MiB offset:
- without my patch, the loaded file is corrupted, memory dump indicate than *base = 0x0
   and not the magic for the loaded file, all the  other DDR content is OK
- with the patch, I have no more issue (magic is OK and checksum is OK)

> 
> How exactly is your memory mapped and tested on the boards where your patch
> fixes a problem?

I am on ARM platform, before relocation, so dcache not yet activated.

RAM start at 0xC0000000, available size in 1 GB = 0x40000000

U-Boot loaded /executed on 0xC1000000

In first loop U-Boot save the RAM and the update the power of 2 address
	cnt = 0x8000000 => 0xE0000000 = 0xE0000000 (save[0])
	cnt = 0x4000000 => 0xD0000000 = 0xD0000000 (save[1])
	cnt = 0x2000000 => 0xC8000000 = 0xC8000000 (save[2]) 
	cnt = 0x1000000 => 0xC4000000 = 0xC4000000 (save[3]) 
	....
	cnt = 0x0000040 => 0xC4000100 = 0xC0000100 (save[21]) 
	cnt = 0x0000020 => 0xC4000080 = 0xC0000080 (save[22]) 
	cnt = 0x0000010 => 0xC4000040 = 0xC0000040 (save[23]) 
	cnt = 0x0000008 => 0xC4000020 = 0xC0000020 (save[24]) 
	cnt = 0x0000004 => 0xC4000010 = 0xC0000010 (save[25]) 
	cnt = 0x0000002 => 0xC4000008 = 0xC0000008 (save[26]) 
	cnt = 0x0000001 => 0xC4000004 = 0xC0000004 (save[27])

Then update base to 0, i = 28
	0xC0000000 = 0x00000000 (save[28])

Then the second loop is executed, the content of DDR is restored of each power of 2 offset
As the test if (val != ~cnt)  is always false

	cnt = 0x0000001 => 0xC4000004 = save[27] 
	cnt = 0x0000002 => 0xC4000008 = save[26]
	... 
	cnt = 0x4000000 => 0xD0000000 = save[1]
	cnt = 0x8000000 => 0xE0000000 = save[0]

and finally the function return max size

at this point the saved value in save[28) is NOT restored and 
0xC0000000 is still stay at 0x00000000

> 
> The thing is, that this "fix" comes up again and again wevery coplu of months /
> years, and IIRC so far all these patches broke some system, while the code as is
> has been working fine of many systems.
> 
> See for example commit b8496cce and revert in 3ab270d5 in 2012, or commit
> 8e7cba04 and revert in cc8d698f in 2016.
> 
> See also the threads starting at
> 
> Subject: [U-Boot-Users] memsize.c patch
> From: "Sangmoon Kim" <dogoil at etinsys.com>
> Date: Fri, 2 Apr 2004 13:08:50 +0900
> 
> and
> 
> Subject: [PATCH v2] memsize: Fix for bug in memory sizing code
> From: Gerd Hoffmann <kraxel at redhat.com>
> Date: Tue, 21 Oct 2014 18:49:13 +0200

I agree than the second patch is exactly the same and I don't understood why this patch broke after board .
The only difference is that I also reduce the size of the array save[] (as base in no more saved in this array), 
so I don't increase the stack usage. 

Perhaps other board failed because size of function code increase (not fully inside instruction cache page) 
or it  is the same issue indicated previously: the address of function change and so code change during execution.

If the same patch come again I think that this function have really a problem and we could
solve the issue (with my patch for exmale) and then understood the regression on other the boards.

> Thanks.
> 
> Best regards,
> 
> Wolfgang Denk
> 
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de A
> supercomputer is a machine that runs an endless loop in 2 seconds.

If you think it is this risk is too high, I can also manage the issue on by board :
	save *base before to call and restore it after
But it only a workaround, so I prefer a generic correction.

Regards,

Patrick Delaunay