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

[Pali Rohár]

On Friday 06 January 2023 17:51:56 Tom Rini wrote:
> On Fri, Jan 06, 2023 at 11:09:30PM +0100, Pali Rohár wrote:
> > 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.
> 
> OK, good, thanks.
> 
> > > > > 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.
> 
> Yes, it's very much a mess, so for this release I'd like to return to
> either:
> - Status quo of v2022.10 (revert this patch)
> - Change only CONFIG_MAX_MEM_MAPPED being set behavior (should keep
>   PowerPC 36bit working, may have unexpected impact on other platforms,
>   still, but very few at least).

Yea, it is a mess. I'm looking at the CONFIG_MAX_MEM_MAPPED again and it
is for different situation. CONFIG_MAX_MEM_MAPPED says maximal mapped
memory. For mpc85xx it is by default set to 2GB for a very long time.
And if base physical address od the RAM is at 2GB or higher then it also
hits this 4GB limit. So CONFIG_MAX_MEM_MAPPED does not help there...