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

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

[Tom Rini]

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.

Reported-by: Marek Vasut <marex@denx.de>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
Cc: Pali Rohár <pali@kernel.org>
Signed-off-by: Tom Rini <trini@konsulko.com>
---
 common/memsize.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/common/memsize.c b/common/memsize.c
index 3c80ad2c8346..31884acca0df 100644
--- a/common/memsize.c
+++ b/common/memsize.c
@@ -94,23 +94,11 @@ long get_ram_size(long *base, long maxsize)
 
 phys_size_t __weak get_effective_memsize(void)
 {
-	phys_size_t ram_size = gd->ram_size;
-
-	/*
-	 * Check for overflow and limit ram size to some representable value.
-	 * It is required that ram_base + ram_size must be representable by
-	 * phys_size_t type and must be aligned by direct access, therefore
-	 * calculate it from last 4kB sector which should work as alignment
-	 * on any platform.
-	 */
-	if (gd->ram_base + ram_size < gd->ram_base)
-		ram_size = ((phys_size_t)~0xfffULL) - gd->ram_base;
-
 #ifndef CONFIG_MAX_MEM_MAPPED
-	return ram_size;
+	return gd->ram_size;
 #else
 	/* limit stack to what we can reasonable map */
-	return ((ram_size > CONFIG_MAX_MEM_MAPPED) ?
-		CONFIG_MAX_MEM_MAPPED : ram_size);
+	return ((gd->ram_size > CONFIG_MAX_MEM_MAPPED) ?
+		CONFIG_MAX_MEM_MAPPED : gd->ram_size);
 #endif
 }
-- 
2.25.1

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

[Pali Rohár]

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.

> Reported-by: Marek Vasut <marex@denx.de>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> Cc: Pali Rohár <pali@kernel.org>
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
>  common/memsize.c | 18 +++---------------
>  1 file changed, 3 insertions(+), 15 deletions(-)
> 
> diff --git a/common/memsize.c b/common/memsize.c
> index 3c80ad2c8346..31884acca0df 100644
> --- a/common/memsize.c
> +++ b/common/memsize.c
> @@ -94,23 +94,11 @@ long get_ram_size(long *base, long maxsize)
>  
>  phys_size_t __weak get_effective_memsize(void)
>  {
> -	phys_size_t ram_size = gd->ram_size;
> -
> -	/*
> -	 * Check for overflow and limit ram size to some representable value.
> -	 * It is required that ram_base + ram_size must be representable by
> -	 * phys_size_t type and must be aligned by direct access, therefore
> -	 * calculate it from last 4kB sector which should work as alignment
> -	 * on any platform.
> -	 */
> -	if (gd->ram_base + ram_size < gd->ram_base)
> -		ram_size = ((phys_size_t)~0xfffULL) - gd->ram_base;
> -
>  #ifndef CONFIG_MAX_MEM_MAPPED
> -	return ram_size;
> +	return gd->ram_size;
>  #else
>  	/* limit stack to what we can reasonable map */
> -	return ((ram_size > CONFIG_MAX_MEM_MAPPED) ?
> -		CONFIG_MAX_MEM_MAPPED : ram_size);
> +	return ((gd->ram_size > CONFIG_MAX_MEM_MAPPED) ?
> +		CONFIG_MAX_MEM_MAPPED : gd->ram_size);
>  #endif
>  }
> -- 
> 2.25.1
> 

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1182 bytes --]

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?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Pali Rohár]

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

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.

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.

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

[Fabio Estevam]

On Fri, Jan 6, 2023 at 12:51 PM Tom Rini <trini@konsulko.com> 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.
>
> Reported-by: Marek Vasut <marex@denx.de>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> Cc: Pali Rohár <pali@kernel.org>
> Signed-off-by: Tom Rini <trini@konsulko.com>

Peng also reported an imx issue caused by 777aaaa706bc
(""common/memsize.c: Fix get_effective_memsize() to check for
overflow""):

https://patchwork.ozlabs.org/project/uboot/patch/20221107080016.1304-2-peng.fan@oss.nxp.com/

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2316 bytes --]

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?

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

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2714 bytes --]

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

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Pali Rohár]

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.

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5549 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [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...

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 6753 bytes --]

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 ?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Pali Rohár]

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.

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 7862 bytes --]

On Sat, Jan 07, 2023 at 06:38:58PM +0100, Pali Rohár wrote:
> 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.

Right, so which option for the release on Monday do you prefer at this
point? We should indeed sort this mess out, for v2023.04.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Pali Rohár]

On Saturday 07 January 2023 12:40:00 Tom Rini wrote:
> On Sat, Jan 07, 2023 at 06:38:58PM +0100, Pali Rohár wrote:
> > 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.
> 
> Right, so which option for the release on Monday do you prefer at this
> point? We should indeed sort this mess out, for v2023.04.

Just moving/hiding those changes under mpc85xx ifdef. So ARM platforms
would be unaffected and mpc85xx platform would not break again.
Something like this (with explanation comments):

diff --git a/common/memsize.c b/common/memsize.c
index 3c80ad2c8346..54a6416717a3 100644
--- a/common/memsize.c
+++ b/common/memsize.c
@@ -96,6 +96,7 @@ phys_size_t __weak get_effective_memsize(void)
 {
 	phys_size_t ram_size = gd->ram_size;
 
+#ifdef CONFIG_MPC85xx
 	/*
 	 * Check for overflow and limit ram size to some representable value.
 	 * It is required that ram_base + ram_size must be representable by
@@ -105,6 +106,7 @@ phys_size_t __weak get_effective_memsize(void)
 	 */
 	if (gd->ram_base + ram_size < gd->ram_base)
 		ram_size = ((phys_size_t)~0xfffULL) - gd->ram_base;
+#endif
 
 #ifndef CONFIG_MAX_MEM_MAPPED
 	return ram_size;

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 9523 bytes --]

On Sat, Jan 07, 2023 at 06:44:40PM +0100, Pali Rohár wrote:
> On Saturday 07 January 2023 12:40:00 Tom Rini wrote:
> > On Sat, Jan 07, 2023 at 06:38:58PM +0100, Pali Rohár wrote:
> > > 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.
> > 
> > Right, so which option for the release on Monday do you prefer at this
> > point? We should indeed sort this mess out, for v2023.04.
> 
> Just moving/hiding those changes under mpc85xx ifdef. So ARM platforms
> would be unaffected and mpc85xx platform would not break again.
> Something like this (with explanation comments):
> 
> diff --git a/common/memsize.c b/common/memsize.c
> index 3c80ad2c8346..54a6416717a3 100644
> --- a/common/memsize.c
> +++ b/common/memsize.c
> @@ -96,6 +96,7 @@ phys_size_t __weak get_effective_memsize(void)
>  {
>  	phys_size_t ram_size = gd->ram_size;
>  
> +#ifdef CONFIG_MPC85xx
>  	/*
>  	 * Check for overflow and limit ram size to some representable value.
>  	 * It is required that ram_base + ram_size must be representable by
> @@ -105,6 +106,7 @@ phys_size_t __weak get_effective_memsize(void)
>  	 */
>  	if (gd->ram_base + ram_size < gd->ram_base)
>  		ram_size = ((phys_size_t)~0xfffULL) - gd->ram_base;
> +#endif
>  
>  #ifndef CONFIG_MAX_MEM_MAPPED
>  	return ram_size;
> 

That works for me, can you please post it as a formal patch and I'll
apply it?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Pali Rohár]

On Saturday 07 January 2023 12:46:07 Tom Rini wrote:
> On Sat, Jan 07, 2023 at 06:44:40PM +0100, Pali Rohár wrote:
> > On Saturday 07 January 2023 12:40:00 Tom Rini wrote:
> > > On Sat, Jan 07, 2023 at 06:38:58PM +0100, Pali Rohár wrote:
> > > > 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.
> > > 
> > > Right, so which option for the release on Monday do you prefer at this
> > > point? We should indeed sort this mess out, for v2023.04.
> > 
> > Just moving/hiding those changes under mpc85xx ifdef. So ARM platforms
> > would be unaffected and mpc85xx platform would not break again.
> > Something like this (with explanation comments):
> > 
> > diff --git a/common/memsize.c b/common/memsize.c
> > index 3c80ad2c8346..54a6416717a3 100644
> > --- a/common/memsize.c
> > +++ b/common/memsize.c
> > @@ -96,6 +96,7 @@ phys_size_t __weak get_effective_memsize(void)
> >  {
> >  	phys_size_t ram_size = gd->ram_size;
> >  
> > +#ifdef CONFIG_MPC85xx
> >  	/*
> >  	 * Check for overflow and limit ram size to some representable value.
> >  	 * It is required that ram_base + ram_size must be representable by
> > @@ -105,6 +106,7 @@ phys_size_t __weak get_effective_memsize(void)
> >  	 */
> >  	if (gd->ram_base + ram_size < gd->ram_base)
> >  		ram_size = ((phys_size_t)~0xfffULL) - gd->ram_base;
> > +#endif
> >  
> >  #ifndef CONFIG_MAX_MEM_MAPPED
> >  	return ram_size;
> > 
> 
> That works for me, can you please post it as a formal patch and I'll
> apply it?

I reused your commit message and I sent above patch to the list.