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

[Tom Rini]

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

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20230106/da4fdf13/attachment.sig>