Re: [PATCH] Revert "common/memsize.c: Fix get_effective_memsize() to check for overflow"

["Pali Rohár" <pali@kernel.org>]

On Friday 06 January 2023 16:45:41 Tom Rini wrote:
> On Fri, Jan 06, 2023 at 04:14:08PM -0500, Tom Rini wrote:
> > On Fri, Jan 06, 2023 at 09:22:56PM +0100, Pali Rohár wrote:
> > > On Friday 06 January 2023 12:25:24 Tom Rini wrote:
> > > > On Fri, Jan 06, 2023 at 05:45:43PM +0100, Pali Rohár wrote:
> > > > > On Friday 06 January 2023 10:51:43 Tom Rini wrote:
> > > > > > This reverts commit 777aaaa706bcfe08c284aed06886db7d482af3f8.
> > > > > > 
> > > > > > The changes to this generic function, which is intended to help with
> > > > > > 32bit platforms with large amounts of memory has unintended side effects
> > > > > > (which in turn lead to boot failures) on other platforms which were
> > > > > > previously functional.
> > > > > 
> > > > > As mentioned previously, unfortunately this revert breaks 32-bit u-boot
> > > > > on 36-bit mpc85xx boards with 32-bit e500v2 cores and 4GB DDR module.
> > > > > 
> > > > > Which platforms currently have broken u-boot without this revert? The
> > > > > only one which was reported is stm32mp but for it there different
> > > > > workaround patch waiting in the queue.
> > > > 
> > > > Are you able to test on one of these PowerPC platforms currently?  As
> > > > the stm32 problem shows, not everything is getting tested frequently
> > > > enough, so how many other cases are lurking out there.  And, I think
> > > > overall issue is that the overflow check-and-change you introduce here
> > > > should just be in the CONFIG_MAX_MEM_MAPPED==true case.  As that's the
> > > > case you're dealing with, yes?
> > > 
> > > I was planning to do big retest again after all powerpc patches are
> > > reviewed and merged...
> > 
> > Yes, but can you test one of them now, to see if my idea works?

Ok, I will try to look at during the weekend.

> > > Anyway, if the issue here is with ram_size and its reduction was needed
> > > for mpc85xx (at the time of introduction of that patch), what about
> > > putting mpc85xx ifdef around ram_size reduction? For arm boards it would
> > > have same behavior as revert of that commit and for mpc85xx it would be
> > > no change.

This is what I mean:

#ifdef CONFIG_MPC85xx
    if (gd->ram_base + ram_size < gd->ram_base)
        ram_size = ...;
#endif

> > > I agree that this code needs to be revisited, together with ram_top
> > > issue and also code which fills DDR banks. Because really mapped memory
> > > for u-boot and real size of DDR are two different things here.
> > 
> > The issue here is that we see two now (given Fabio's reminder about
> > another thread I had forgotten) of unintended consequences, on 32bit
> > platforms trying to normally have 2GB of memory, which does not require
> > special treatment.

Running git grep get_effective_memsize and git grep 'gd->ram_top' shows
the root of this issue: Different platforms, boards and common code use
these things differently. This needs to be "fixed" = unified in whole
codebase. We need a function which returns mappable memory for u-boot
(intptr_t type is enough) and another function (or structure or
whatever) which says total size of RAM as u64 type (to ensure that it
would work also for 4GB SODIMM modules on pure 32-bit platforms). And
then each place in u-boot code has to be modified to use the correct
function.

Second issue is then gd->ram_top. Either say 4GB for 32-bit ram_top type
is not supported or say that zero value is special and represents 4GB.
And also every place in u-boot code needs to be adjusted by this
decision / code.

Fixing both issues make easily break lot of boards (if done improperly)
as it touches whole u-boot code base. So not easy task.

> What I'm leaning towards right now even, is that since it's hard to test
> the non-36bit platforms that do set CONFIG_MAX_MEM_MAPPED, to see if
> their behavior also changed here, the 36bit platforms should just be
> overriding get_effective_memsize.

There are (at least) 3 situation:
1) if RAM is mapped to the end of physical address space (possibly just
   small e.g. 1GB)
2) if platform is >32-bit but running in 32-bit mode (so physical
   address is u64 because we do not have e.g. int36_t; but void* is
   32-bit)
3) if RAM is exactly 4GB and u-boot is 32-bit

And every one has different edge cases and cause different problems.
Now, as I pointed above that every platform / board is using
get_effective_memsize() differently, plus we have CONFIG_MAX_MEM_MAPPED
option, it means that number of test matrix is really huge.

It looks like that ARM issues are caused by the fact that RAM is mapped
to the end of the physical address space (so it does not matter how big
or small it is). And powerpc issue is 4GB of RAM together with running
in 32-bit mode.

Anyway, has U-Boot support for 32-bit x86 CPUs with PAE support? If yes,
then I bet that there can be other edge cases when e.g. 8GB of DDR is
connected.