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

Fri Dec 8 15:12:02 UTC 2017

> From: Wolfgang Denk [mailto:wd at denx.de]
> Subject: Re: [U-Boot] [PATCH] common/memsize.c: restore content of the base
> address
> 
> Dear Patrick,
> 
> In message <6daf1478e4284b8590c2862c6a504e9c at SFHDAG6NODE3.st.com>
> you wrote:
> >
> > 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 mean you are running this code from the very memory you are sizing?  This
> is fundamentally broken.  You must not do this!
>

Yes I do it, sorry if it is a error.

But it is recommended in ./doc/README.arm-relocation:
     At board level:

	dram_init(): bd pointer is now at this point not accessible, so only
	detect the real dramsize, and store it in gd->ram_size. Bst detected
	with get_ram_size().

And before to setup by board I check on many device with ARM architecture.
Most the time U-Boot loaded at beginning of the RAM, relocated at the end of the RAM 
and dram_init use get_ram_size
(I read also many debate about the end of relocation even when U-Boot in loaded in RAM on U-Boot mainling list)

Example1 :
  ./board/toradex/colibri_imx6/colibri_imx6.c
  int dram_init(void)
  {
	/* use the DDR controllers configured size */
	gd->ram_size = get_ram_size((void *)CONFIG_SYS_SDRAM_BASE,
				    (ulong)imx_ddr_size());

	return 0;
  }

./arch/arm/include/asm/arch-mx6/imx-regs.h:118:#define MMDC0_ARB_BASE_ADDR             0x10000000
./include/configs/colibri_imx6.h:249:#define PHYS_SDRAM			MMDC0_ARB_BASE_ADDR
./include/configs/colibri_imx6.h:251:#define CONFIG_SYS_SDRAM_BASE		PHYS_SDRAM
./include/configs/colibri_imx6.h:126:#define CONFIG_SYS_TEXT_BASE		0x17800000

Example 2
./arch/arm/mach-omap2/am33xx/board.c: int dram_init(void)
	/* dram_init must store complete ramsize in gd->ram_size */
	gd->ram_size = get_ram_size(
			(void *)CONFIG_SYS_SDRAM_BASE,
			CONFIG_MAX_RAM_BANK_SIZE);

./include/configs/bur_am335x_common.h:67:#define CONFIG_SYS_SDRAM_BASE		0x80000000
./include/configs/siemens-am33x-common.h:155:#define CONFIG_SYS_TEXT_BASE		0x80100000

So I just do the same for my arm/armv7 platform.

On ARM device device, I think we don't have the banks...

> get_ram_size() is supposed to run from some _different_ memory (at least a
> different memopry bank, usually better from a different memory type).

 I agree that the function is not safe in this case.
So I will change my board to remove this call....

> > 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
> 
> You write there:
> 
> | Today, I found a issue with  get_ram_size() usage in U-Boot, when
> | U-Boot is executed in the tested memory.
> 
> You must not do this.  The memory under test is supposed to be otherwise idle.
> 
> > and finally the function return max size
> 
> For the test to work correctly, max test size should be chosen at least twice as
> big as the maximum possible memory configuration.

This requirement seems strange to me

I have 32 bits ARM platform, with addressable RAM area 1GiB
DDR start => 0xC0000000
DDR end => 0xFFFFFFFF (size = 0x40000000)

If I need to test with 2Gib, the code will test access to 0xC0000000 + max_size / 2 = 0x100000000 
=> greater than 32bits access to 0x0 address and crash (not allowed to access to 0x0 = bootrom)

For me the function is working correctly with  max test size chosen at the maximum memory size
=> no overlap detected, so retrun max_size

> 
> > Perhaps other board failed because size of function code increase (not
> > fully inside instruction cache page)
> 
> No.  We are talking of boards that were implemented properly, i. e.
> the tested RAM was NOT the memory where get_ram_size() was running from.
> 
> > or it  is the same issue indicated previously: the address of function change and
> so code change during execution.
> 
> Code location or size does not play a role here.  The code was running from
> parallel NOR flash, and testing the actual RAM size, so thsi is totally
> independent.

OK, sorry
> 
> > 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.
> 
> I agree there appears to be a problem,  but it should be understood..
> 
> In your case you use the code in an illegal way, so I tend to argument you are
> provoking undefined behaviour.

> 
> Best regards,
> 
> Wolfgang Denk
>
Ok, I will remove the call of the code on my board, even if over arm platform use this function in the same context.
So it should be no more a problem for me.

But I was still convince that the test don't restore properly the DDR properly, 
So I dig deeper on the algorithm when size < max size and in fact, 
it is working with overlap (I miss it in first review):

For example :
RAM start at 0xC0000000, max available size in 1 GB = 0x40000000

Effective size = 256Mib

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 = 0x1000000 => 0xC4000000 = save[3]
	cnt = 0x2000000 => 0xC8000000 = save[2]
	cnt = 0x4000000 => 0xD0000000 = 0 overlap detected !
		size = cnt * 4 = 0x10000000
		and restore  original data
		cnt = 0x4000000 => 0xD0000000 = 0x00000000 = save[1] (with overlap)
		cnt = 0x8000000 => 0xE0000000 = 0x10000000 = save[0] ](with overlap)

So the function don't restore *base only if no overlap is detected , for the case return (maxsize)

But it is the case that you indicate it is not working:
	max test size should be chosen at least twice as big as the maximum possible memory configuration.

Even it is not indicated in function header  and if the size is correctly detected :
 * Check memory range for valid RAM. A simple memory test determines
 * the actually available RAM size between addresses `base' and
 * `base + maxsize'

Here it I not indicated maxsize / 2
I now, I am doubtfully...

Do you think this behavior is correct ?
	When no overlap is detected, return maxsize but don't restore *base

Regards

Patrick