[PATCH] arm: stm32mp: Fix board_get_usable_ram_top() again

Thu Jan 5 19:31:19 CET 2023

Hi Marek,

On 1/5/23 13:11, Marek Vasut wrote:
> On 1/5/23 11:03, Patrick DELAUNAY wrote:
>> Hi,
>
> Hi,
>
>> On 1/5/23 02:22, Marek Vasut wrote:
>>> Do not access gd->ram_size and assume this is actual valid RAM size. 
>>> Since commit
>>> 777aaaa706b ("common/memsize.c: Fix get_effective_memsize() to check 
>>> for overflow")
>>> the RAM size may be less than gd->ram_size , call 
>>> get_effective_memsize() to get
>>> the limited value instead.
>>>
>>> The aforementioned commit makes STM32MP15xx boards with 1 GiB of DRAM
>>> at 0xc0000000 hang on boot, which is a grave defect.
>>>
>>> Signed-off-by: Marek Vasut <marex at denx.de>
>>> ---
>>> Cc: Pali Rohar <pali at kernel.org>
>>> Cc: Patrice Chotard <patrice.chotard at foss.st.com>
>>> Cc: Patrick Delaunay <patrick.delaunay at foss.st.com>
>>> Cc: Tom Rini <trini at konsulko.com>
>>> ---
>>>   arch/arm/mach-stm32mp/dram_init.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/mach-stm32mp/dram_init.c 
>>> b/arch/arm/mach-stm32mp/dram_init.c
>>> index 9346fa8546d..80ba5c27741 100644
>>> --- a/arch/arm/mach-stm32mp/dram_init.c
>>> +++ b/arch/arm/mach-stm32mp/dram_init.c
>>> @@ -51,7 +51,7 @@ phys_size_t board_get_usable_ram_top(phys_size_t 
>>> total_size)
>>>       /* found enough not-reserved memory to relocated U-Boot */
>>>       lmb_init(&lmb);
>>> -    lmb_add(&lmb, gd->ram_base, gd->ram_size);
>>> +    lmb_add(&lmb, gd->ram_base, get_effective_memsize());
>>>       boot_fdt_add_mem_rsv_regions(&lmb, (void *)gd->fdt_blob);
>>>       /* add 8M for reserved memory for display, fdt, gd,... */
>>>       size = ALIGN(SZ_8M + CONFIG_SYS_MALLOC_LEN + total_size, 
>>> MMU_SECTION_SIZE),
>>
>>
>> Thanks for the patch
>>
>> Reviewed-by: Patrick Delaunay <patrick.delaunay at foss.st.com>
>
> Thank you.
>
>> Even if I don't uderstood where the code blocked here...
>>
>> I need to cros-check it.
>
> Please do, thank you.

Tested, see after....

>
>> But I think it is more a regression introduced by
>>
>> 777aaaa706b ("common/memsize.c: Fix get_effective_memsize() to check 
>> for overflow")
>
> I agree that commit likely breaks more systems than just STM32MP15xx , 
> and I am tempted to say, it should be reverted before release.
>
> Tom ?
>
>> in this patch I don't understood the assumption:
>>
>> * It is required that ram_base + ram_size must be representable by
>> * phys_size_t type and must be aligned by direct access
>>
>>
>> why add now this limitation ?
>>
>> why phys_size_t here and not phys_addr_t or unsigned long, the 
>> ram_base type ?
>>
>>
>> why align on 4kB and not on MMU_SECTION_SIZE ?
>>
>>
>>
>> until now on ARM 32bits U-Boot is working with
>>
>>    ram_base = 0xC0000000 (unsigned long)
>>    ram_size = 0x40000000 (phys_size_t)
>>
>> overflow for ram_top (phys_addr_t) = ram_base + 
>> get_effective_memsize() was previously managed in the code
>> as we work only on base address + size (in board_f.c, in lmb library)
>>
>> I correct in the code overflow issues, for example in commit 
>> 54be09cd8f6e ("arm: caches: manage phys
>> addr_t overflow in mmu_set_region_dcache_behaviour").
>>
>>
>> moreover for me it is strange to reduce the size of the DDR banks by 
>> default
>> (see resutl of bdinfo command) only just to avoid overflow:
>>
>> common/board_f.c:267:    gd->bd->bi_dram[0].size = 
>> get_effective_memsize();
>>
>>
>> For me the real assumption is on the end of RAM:
>>
>> "ram_base + ram_size - 1" must be representable by 'ram_base' type
>> (phys_addr_t or unsigned long ?)
>>
>>
>>
>> +
>> +    /*
>> +     * Check for overflow and limit ram size.
>> +     * It is required that ram_base + ram_size - 1 must be 
>> representable
>> +     * by ram_base type, unsigned long.
>> +     */
>> +    if (gd->ram_base - 1 + ram_size < gd->ram_base)
>> +        ram_size = ((unsigned long)~0x0ULL) - gd->ram_base;
>>
>>
>>
>> => no need to remove 4KB is here... base + size can reach the MAX 
>> value for the type + 1
>
> Note that this -4 kiB is exactly what triggers the problem , i.e. if 
> get_effective_memsize() returns 0x4000_0000 , all is fine, if it 
> returns 0x3fff0000 then the system hangs. I also tried subtracting 
> MMU_SECTION_SIZE instead of 4k and that does not help, system still 
> hangs.

ok

I tested on STM32MP157C-EV1 on my side...

with 1GiB mermory size

U-Boot is booting on next TOP (for trusted boot with TF-A and OP-TEE)

U-Boot 2023.01-rc4-00386-gb429e78942de (Jan 05 2023 - 17:44:04 +0100)

stm32_smc: Failed to exec svc=82001003 op=1 in secure mode (err = -1)
stm32_smc: Failed to exec svc=82001003 op=1 in secure mode (err = -1)
CPU: STM32MP157C?? Rev.Z
Model: STMicroelectronics STM32MP157C eval daughter on eval mother
Board: stm32mp1 in trusted mode (st,stm32mp157c-ev1)
stm32_smc: Failed to exec svc=82001003 op=1 in secure mode (err = -1)
DRAM:  1 GiB
....

STM32MP> bdinfo
boot_params = 0x00000000
DRAM bank   = 0x00000000
-> start    = 0xc0000000
-> size     = 0x3ffff000         <<<<<< the patch is applied - 4kiB here !!!
flashstart  = 0x00000000
flashsize   = 0x00000000
flashoffset = 0x00000000
baudrate    = 115200 bps
relocaddr   = 0xfdb1d000
reloc off   = 0x3da1d000
Build       = 32-bit
current eth = ethernet at 5800a000
ethaddr     = 00:80:e1:01:60:da
IP addr     = <NULL>
fdt_blob    = 0xfbb040a0
new_fdt     = 0xfbb040a0
fdt_size    = 0x00016de0
Video       = display-controller at 5a001000 inactive
lmb_dump_all:
  memory.cnt  = 0x1
  memory[0]    [0xc0000000-0xffffefff], 0x3ffff000 bytes flags: 0        
<<<<
  reserved.cnt  = 0x6
  reserved[0]    [0x10000000-0x10045fff], 0x00046000 bytes flags: 4
  reserved[1]    [0x30000000-0x3003ffff], 0x00040000 bytes flags: 4
  reserved[2]    [0x38000000-0x3800ffff], 0x00010000 bytes flags: 4
  reserved[3]    [0xe8000000-0xefffffff], 0x08000000 bytes flags: 4
  reserved[4]    [0xfbaffbb8-0xfdffffff], 0x02500448 bytes flags: 0
  reserved[5]    [0xfe000000-0xffffffff], 0x02000000 bytes flags: 4     <<<<
devicetree  = board
arch_number = 0x00000000
TLB addr    = 0xfdff0000
irq_sp      = 0xfbb03e10
sp start    = 0xfbb03e00
Early malloc usage: 2924 / 20000
STM32MP>

But we have strange LMB reserved memory [0xfe000000-0xffffffff] from DT

=> it is outside of  memory[0]    [0xc0000000-0xffffefff]

I think the -4 kiB should be removed at least....

I test also with basic boot = SPL and U-Boot, no issue on my side

Patrick