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

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

On Saturday 07 January 2023 12:32:12 Tom Rini wrote:
> On Sat, Jan 07, 2023 at 05:26:45PM +0100, Pali Rohár wrote:
> > 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...
> 
> Right, it's a mess. So, to try and end up with the least number of
> broken platforms for the coming release, would you rather a full revert,
> or just moving your changes under CONFIG_MAX_MEM_MAPPED being set, which
> I believe you're saying is the case for 36bit PowerPC ?

CONFIG_MAX_MEM_MAPPED is by default set for all powerpc boards
(see arch/powerpc/include/asm/config.h) and also for some ARM plat.

As I suggested above, rather move ram_size modification under
CONFIG_MPC85xx which is set only for powerpc mpc85xx platform which
I tested.

CONFIG_MAX_MEM_MAPPED option is not too strict as CONFIG_MPC85xx.