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

[Marek Vasut]

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.

> 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.