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

[Patrick DELAUNAY]

Hi,

On 1/5/23 20:16, Pali Rohar wrote:
> Hello!
>
> On Thursday 05 January 2023 11:03:07 Patrick DELAUNAY wrote:
>> 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>
>>
>>
>> Even if I don't uderstood where the code blocked here...
>>
>> I need to cros-check it.
>>
>>
>>
>>
>> But I think it is more a regression introduced by
>>
>> 777aaaa706b ("common/memsize.c: Fix get_effective_memsize() to check for overflow")
>>
>>
>> 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 ?
> Well, I think that it is quite confusing when it is phys_addr_t and when
> phys_size_t. Because U-Boot on more places is doing addition:
> phys_addr_t + phys_size_t.
>
> And I was confused by it too, because get_effective_memsize() function
> works only with variables of phys_size_t type.
>
> So I would agree that it should be mentioned as phys_addr_t.
>
> But is there any platform on which phys_addr_t and phys_size_t types
> have different sizes?


I don't know, it is just more clear

=> addr overflow based on address type


>
> Why it is not ram_base? Because there is no such type. Why it is not
> phys_addr_t? Because some 32-bit platforms support more than 4GB of
> physical memory and unsigned long type is small for them. Also there are
> 64-bit platforms with just 32-bit data bus (but I'm not sure if these
> platforms are handled somehow specially with just 32-bit phys_addr_t).
>
>> why align on 4kB and not on MMU_SECTION_SIZE ?
> Because of my mistake and seems that nobody spotted or reported this
> issue yet. I agree it should be some platform specific macro (I have not
> checked now if MMU_SECTION_SIZE is the correct one).


I propose MMU_SECTION_SIZE to allow MMU configuration up to end

of effective ram top.


>
>> 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();
> Yes, this really looks strange to reduce total size of DDR banks to the
> effective memory size which U-Boot can use or map. There is also config
> option CONFIG_MAX_MEM_MAPPED to allow user to reduce mappable memory in
> U-Boot to more strict value.
>
> For sure DDR bank size should be set to the real size.
>
> For example 32-bit U-Boot can map maximally 4GB of RAM, but on some
> (32-bit) platforms there can be more than 4GB of DDR and operating
> system may support to use more than 4GB of it. So DDR bank size passed
> from bootloader to OS should be precise and not reduced to size
> supported by bootloader.
>
>> 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 ?)
> You are missing there +1. That is restriction in the U-Boot as it
> requires that one byte after the RAM can be stored in both phys_addr_t
> and phys_size_t variables.
>
> Again, I wrote code comment slightly wrong. It should say that
> ram_base + get_effective_memsize() must be representable by phys_addr_t
> type.


For me it is not a problem, 0x0 for RAM TOP  should be (max of 
phys_addr_t + 1)


=> relocation address of U-Boot is (ram_top - u-boot size) is correctly 
working

but perhaps I miss something....

and I dig deeper.


Anyway on STM32MP15X platform with 1GB DDR, for basic boot

ram_base = 0xC0000000

ram_size = 0x40000000


=> and it is working (with ram_top = 0x0)


>
>>
>> +
>> +	/*
>> +	 * 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
>>
>>
>> Patrick
>>
> For example U-Boot code in common/board_f.c sets gd->ram_top to value:
>
> gd->ram_top = gd->ram_base + get_effective_memsize();
>
> So your above change with -1 just break this code.


For me ram_top = 0x0 is functional ( for u32 equivalant with overflow to 
0x1 0000 0000)


>
> I think that the best would be to fix U-Boot code so that it never use
> last byte +1 in phys_addr_t.


I see that over platform don't change this common part of U-Boot

but reduce the usable RAM by U-Boot by using the weak function:


board_get_usable_ram_top()


common/board_f.c:376:    gd->ram_top = 
board_get_usable_ram_top(gd->mon_len);


and if you see the default implementation of this function :


/* Get the top of usable RAM */
__weak phys_size_t gd->ram_base(phys_size_t total_size)
{
#if defined(CONFIG_SYS_SDRAM_BASE) && CONFIG_SYS_SDRAM_BASE > 0
     /*
      * Detect whether we have so much RAM that it goes past the end of our
      * 32-bit address space. If so, clip the usable RAM so it doesn't.
      */
     if (gd->ram_top < CONFIG_SYS_SDRAM_BASE)
         /*
          * Will wrap back to top of 32-bit space when reservations
          * are made.
          */
         return 0;
#endif
     return gd->ram_top;
}


the 32-bis address space overflow is already detected here when

gd->ram_top =  gd->ram_base +  gd->ram_size  < CONFIG_SYS_SDRAM_BASE


=> ram_top = board_get_usable_ram_top()

                     = 0


So I think you can revert your patch....

and perhaps check your implementation of this weak function if you 
overidde it


Regards


Patrick