* [PATCH] Revert "common/memsize.c: Fix get_effective_memsize() to check for overflow" @ 2023-01-06 15:51 Tom Rini 2023-01-06 16:45 ` Pali Rohár 2023-01-06 20:29 ` Fabio Estevam 0 siblings, 2 replies; 16+ messages in thread From: Tom Rini @ 2023-01-06 15:51 UTC (permalink / raw) To: u-boot; +Cc: Marek Vasut, Patrick Delaunay, Pali Rohár 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] Revert "common/memsize.c: Fix get_effective_memsize() to check for overflow" 2023-01-06 15:51 [PATCH] Revert "common/memsize.c: Fix get_effective_memsize() to check for overflow" Tom Rini @ 2023-01-06 16:45 ` Pali Rohár 2023-01-06 17:25 ` Tom Rini 2023-01-06 20:29 ` Fabio Estevam 1 sibling, 1 reply; 16+ messages in thread From: Pali Rohár @ 2023-01-06 16:45 UTC (permalink / raw) To: Tom Rini; +Cc: u-boot, Marek Vasut, Patrick Delaunay 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 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Revert "common/memsize.c: Fix get_effective_memsize() to check for overflow" 2023-01-06 16:45 ` Pali Rohár @ 2023-01-06 17:25 ` Tom Rini 2023-01-06 20:22 ` Pali Rohár 0 siblings, 1 reply; 16+ messages in thread From: Tom Rini @ 2023-01-06 17:25 UTC (permalink / raw) To: Pali Rohár; +Cc: u-boot, Marek Vasut, Patrick Delaunay [-- 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 --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Revert "common/memsize.c: Fix get_effective_memsize() to check for overflow" 2023-01-06 17:25 ` Tom Rini @ 2023-01-06 20:22 ` Pali Rohár 2023-01-06 21:14 ` Tom Rini 0 siblings, 1 reply; 16+ messages in thread From: Pali Rohár @ 2023-01-06 20:22 UTC (permalink / raw) To: Tom Rini; +Cc: u-boot, Marek Vasut, Patrick Delaunay 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Revert "common/memsize.c: Fix get_effective_memsize() to check for overflow" 2023-01-06 20:22 ` Pali Rohár @ 2023-01-06 21:14 ` Tom Rini 2023-01-06 21:45 ` Tom Rini 0 siblings, 1 reply; 16+ messages in thread From: Tom Rini @ 2023-01-06 21:14 UTC (permalink / raw) To: Pali Rohár; +Cc: u-boot, Marek Vasut, Patrick Delaunay [-- 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 --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Revert "common/memsize.c: Fix get_effective_memsize() to check for overflow" 2023-01-06 21:14 ` Tom Rini @ 2023-01-06 21:45 ` Tom Rini 2023-01-06 22:09 ` Pali Rohár 0 siblings, 1 reply; 16+ messages in thread From: Tom Rini @ 2023-01-06 21:45 UTC (permalink / raw) To: Pali Rohár; +Cc: u-boot, Marek Vasut, Patrick Delaunay [-- 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 --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Revert "common/memsize.c: Fix get_effective_memsize() to check for overflow" 2023-01-06 21:45 ` Tom Rini @ 2023-01-06 22:09 ` Pali Rohár 2023-01-06 22:51 ` Tom Rini 0 siblings, 1 reply; 16+ messages in thread From: Pali Rohár @ 2023-01-06 22:09 UTC (permalink / raw) To: Tom Rini; +Cc: u-boot, Marek Vasut, Patrick Delaunay 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Revert "common/memsize.c: Fix get_effective_memsize() to check for overflow" 2023-01-06 22:09 ` Pali Rohár @ 2023-01-06 22:51 ` Tom Rini 2023-01-07 16:26 ` Pali Rohár 0 siblings, 1 reply; 16+ messages in thread From: Tom Rini @ 2023-01-06 22:51 UTC (permalink / raw) To: Pali Rohár; +Cc: u-boot, Marek Vasut, Patrick Delaunay [-- 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 --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Revert "common/memsize.c: Fix get_effective_memsize() to check for overflow" 2023-01-06 22:51 ` Tom Rini @ 2023-01-07 16:26 ` Pali Rohár 2023-01-07 17:32 ` Tom Rini 0 siblings, 1 reply; 16+ messages in thread From: Pali Rohár @ 2023-01-07 16:26 UTC (permalink / raw) To: Tom Rini; +Cc: u-boot, Marek Vasut, Patrick Delaunay 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... ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Revert "common/memsize.c: Fix get_effective_memsize() to check for overflow" 2023-01-07 16:26 ` Pali Rohár @ 2023-01-07 17:32 ` Tom Rini 2023-01-07 17:38 ` Pali Rohár 0 siblings, 1 reply; 16+ messages in thread From: Tom Rini @ 2023-01-07 17:32 UTC (permalink / raw) To: Pali Rohár; +Cc: u-boot, Marek Vasut, Patrick Delaunay [-- 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 --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Revert "common/memsize.c: Fix get_effective_memsize() to check for overflow" 2023-01-07 17:32 ` Tom Rini @ 2023-01-07 17:38 ` Pali Rohár 2023-01-07 17:40 ` Tom Rini 0 siblings, 1 reply; 16+ messages in thread From: Pali Rohár @ 2023-01-07 17:38 UTC (permalink / raw) To: Tom Rini; +Cc: u-boot, Marek Vasut, Patrick Delaunay 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Revert "common/memsize.c: Fix get_effective_memsize() to check for overflow" 2023-01-07 17:38 ` Pali Rohár @ 2023-01-07 17:40 ` Tom Rini 2023-01-07 17:44 ` Pali Rohár 0 siblings, 1 reply; 16+ messages in thread From: Tom Rini @ 2023-01-07 17:40 UTC (permalink / raw) To: Pali Rohár; +Cc: u-boot, Marek Vasut, Patrick Delaunay [-- 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 --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Revert "common/memsize.c: Fix get_effective_memsize() to check for overflow" 2023-01-07 17:40 ` Tom Rini @ 2023-01-07 17:44 ` Pali Rohár 2023-01-07 17:46 ` Tom Rini 0 siblings, 1 reply; 16+ messages in thread From: Pali Rohár @ 2023-01-07 17:44 UTC (permalink / raw) To: Tom Rini; +Cc: u-boot, Marek Vasut, Patrick Delaunay 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; ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] Revert "common/memsize.c: Fix get_effective_memsize() to check for overflow" 2023-01-07 17:44 ` Pali Rohár @ 2023-01-07 17:46 ` Tom Rini 2023-01-07 21:58 ` Pali Rohár 0 siblings, 1 reply; 16+ messages in thread From: Tom Rini @ 2023-01-07 17:46 UTC (permalink / raw) To: Pali Rohár; +Cc: u-boot, Marek Vasut, Patrick Delaunay [-- 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 --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Revert "common/memsize.c: Fix get_effective_memsize() to check for overflow" 2023-01-07 17:46 ` Tom Rini @ 2023-01-07 21:58 ` Pali Rohár 0 siblings, 0 replies; 16+ messages in thread From: Pali Rohár @ 2023-01-07 21:58 UTC (permalink / raw) To: Tom Rini; +Cc: u-boot, Marek Vasut, Patrick Delaunay 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Revert "common/memsize.c: Fix get_effective_memsize() to check for overflow" 2023-01-06 15:51 [PATCH] Revert "common/memsize.c: Fix get_effective_memsize() to check for overflow" Tom Rini 2023-01-06 16:45 ` Pali Rohár @ 2023-01-06 20:29 ` Fabio Estevam 1 sibling, 0 replies; 16+ messages in thread From: Fabio Estevam @ 2023-01-06 20:29 UTC (permalink / raw) To: Tom Rini; +Cc: u-boot, Marek Vasut, Patrick Delaunay, Pali Rohár, Peng Fan 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/ ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2023-01-07 21:58 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-06 15:51 [PATCH] Revert "common/memsize.c: Fix get_effective_memsize() to check for overflow" Tom Rini 2023-01-06 16:45 ` Pali Rohár 2023-01-06 17:25 ` Tom Rini 2023-01-06 20:22 ` Pali Rohár 2023-01-06 21:14 ` Tom Rini 2023-01-06 21:45 ` Tom Rini 2023-01-06 22:09 ` Pali Rohár 2023-01-06 22:51 ` Tom Rini 2023-01-07 16:26 ` Pali Rohár 2023-01-07 17:32 ` Tom Rini 2023-01-07 17:38 ` Pali Rohár 2023-01-07 17:40 ` Tom Rini 2023-01-07 17:44 ` Pali Rohár 2023-01-07 17:46 ` Tom Rini 2023-01-07 21:58 ` Pali Rohár 2023-01-06 20:29 ` Fabio Estevam
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.