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

[Pali Rohar]

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?

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

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

> 
> 
> +
> +	/*
> +	 * 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.

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