[PATCH 0/3] Fix boot regressions on at least TI am62x

[PATCH 0/3] Fix boot regressions on at least TI am62x

[Sjoerd Simons]

When booting a recent u-boot master on TI am62x with the distro boot
sequence it fails loading the FDT:

   Booting using the fdt blob at 0x88000000
Working FDT set to 88000000
   Loading Ramdisk to 8d711000, end 8ffffd95 ... OK
ERROR: reserving fdt memory region failed (addr=9e780000 size=80000 flags=4)
ERROR: reserving fdt memory region failed (addr=9e800000 size=1800000 flags=4)
ERROR: reserving fdt memory region failed (addr=9db00000 size=c00000 flags=4)
ERROR: Failed to allocate 0x8f58 bytes below 0x90000000.
device tree - allocation error
FDT creation failed!
resetting ...

It turns out that this is caused by commit 06d514d77c37 ("lmb: consider EFI
memory map") due to injecting both extra and some similar memory regions
via a different path into the lmb. The following series fixes the
apparent fallout from that ;)


Sjoerd Simons (3):
  Bump LMB_MAX_REGIONS default to 16
  lmb: Set correct lmb flags for EFI memory map entries
  lmb: Treat a region which is a subset as equal

 lib/Kconfig |  2 +-
 lib/lmb.c   | 15 +++++++++------
 2 files changed, 10 insertions(+), 7 deletions(-)

-- 
2.39.0

[PATCH 1/3] Bump LMB_MAX_REGIONS default to 16

[Sjoerd Simons]

Since commit 06d514d77c37 ("lmb: consider EFI memory map") the EFI regions
are also pushed into the lmb if EFI_LOADER is enabled (which is by
default on most system). Which can cause the number of entries to go
over the maximum as it's default is only 8.

Specifically i ran into this case on an TI am62 which has an fdt with
4 reserved regions (in practice 3 lmb entries due to adjecent ranges).

As this is likely to impact more devices bump the default max
regions to 16 so there is a bit more slack.

Signed-off-by: Sjoerd Simons <sjoerd@collabora.com>
---

 lib/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/Kconfig b/lib/Kconfig
index def36f275ce..7eb0e7addc7 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -1005,7 +1005,7 @@ config LMB_USE_MAX_REGIONS
 config LMB_MAX_REGIONS
 	int "Number of memory and reserved regions in lmb lib"
 	depends on LMB && LMB_USE_MAX_REGIONS
-	default 8
+	default 16
 	help
 	  Define the number of supported regions, memory and reserved, in the
 	  library logical memory blocks.
-- 
2.39.0

Re: [PATCH 1/3] Bump LMB_MAX_REGIONS default to 16

[Tom Rini]

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

On Thu, Jan 19, 2023 at 09:38:17AM +0100, Sjoerd Simons wrote:

> Since commit 06d514d77c37 ("lmb: consider EFI memory map") the EFI regions
> are also pushed into the lmb if EFI_LOADER is enabled (which is by
> default on most system). Which can cause the number of entries to go
> over the maximum as it's default is only 8.
> 
> Specifically i ran into this case on an TI am62 which has an fdt with
> 4 reserved regions (in practice 3 lmb entries due to adjecent ranges).
> 
> As this is likely to impact more devices bump the default max
> regions to 16 so there is a bit more slack.
> 
> Signed-off-by: Sjoerd Simons <sjoerd@collabora.com>

I prefer to use:
https://patchwork.ozlabs.org/project/uboot/patch/20230125230823.1567778-1-trini@konsulko.com/
in this case due to it being overall a smaller size increase.

-- 
Tom

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

Re: [PATCH 1/3] Bump LMB_MAX_REGIONS default to 16

[Sjoerd Simons]

On Thu, 2023-01-26 at 13:28 -0500, Tom Rini wrote:
> On Thu, Jan 19, 2023 at 09:38:17AM +0100, Sjoerd Simons wrote:
> > 
> > As this is likely to impact more devices bump the default max
> > regions to 16 so there is a bit more slack.
> > 
> > Signed-off-by: Sjoerd Simons <sjoerd@collabora.com>
> 
> I prefer to use:
> https://patchwork.ozlabs.org/project/uboot/patch/20230125230823.1567778-1-trini@konsulko.com/
> in this case due to it being overall a smaller size increase.

That's fine by me, achieves the same goal i was after for sure; The
other 2 patches in this series are still required to fix the boot
regressions i'm seeing ofcourse :)

-- 
Sjoerd Simons
Collabora Ltd.

Re: [PATCH 1/3] Bump LMB_MAX_REGIONS default to 16

[Tom Rini]

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

On Fri, Jan 27, 2023 at 08:55:42AM +0100, Sjoerd Simons wrote:
> On Thu, 2023-01-26 at 13:28 -0500, Tom Rini wrote:
> > On Thu, Jan 19, 2023 at 09:38:17AM +0100, Sjoerd Simons wrote:
> > > 
> > > As this is likely to impact more devices bump the default max
> > > regions to 16 so there is a bit more slack.
> > > 
> > > Signed-off-by: Sjoerd Simons <sjoerd@collabora.com>
> > 
> > I prefer to use:
> > https://patchwork.ozlabs.org/project/uboot/patch/20230125230823.1567778-1-trini@konsulko.com/
> > in this case due to it being overall a smaller size increase.
> 
> That's fine by me, achieves the same goal i was after for sure; The
> other 2 patches in this series are still required to fix the boot
> regressions i'm seeing ofcourse :)

Right, I'll be getting those soon.

-- 
Tom

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

Re: [PATCH 1/3] Bump LMB_MAX_REGIONS default to 16

[Tom Rini]

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

On Thu, Jan 19, 2023 at 09:38:17AM +0100, Sjoerd Simons wrote:

> Since commit 06d514d77c37 ("lmb: consider EFI memory map") the EFI regions
> are also pushed into the lmb if EFI_LOADER is enabled (which is by
> default on most system). Which can cause the number of entries to go
> over the maximum as it's default is only 8.
> 
> Specifically i ran into this case on an TI am62 which has an fdt with
> 4 reserved regions (in practice 3 lmb entries due to adjecent ranges).
> 
> As this is likely to impact more devices bump the default max
> regions to 16 so there is a bit more slack.
> 
> Signed-off-by: Sjoerd Simons <sjoerd@collabora.com>

As Philippe Schenker explained, I was mis-interpreting what my attempted
solution would do, so I've reverted that and this patch here makes the
most sense to use as a general fix. As such I've now applied this to
u-boot/master.

-- 
Tom

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

[PATCH 2/3] lmb: Set correct lmb flags for EFI memory map entries

[Sjoerd Simons]

When adding reserved memory areas from the EFI memory map set the NOMAP
flag when applicable. When this isn't done adding "no-map" flagged entries
from the fdt after receiving the same from the EFI memory map fails due
to non-matching flags.

Signed-off-by: Sjoerd Simons <sjoerd@collabora.com>
---

 lib/lmb.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/lib/lmb.c b/lib/lmb.c
index ec790760db6..f447c639a60 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -175,11 +175,14 @@ static __maybe_unused int efi_lmb_reserve(struct lmb *lmb)
 		return 1;
 
 	for (i = 0, map = memmap; i < map_size / sizeof(*map); ++map, ++i) {
-		if (map->type != EFI_CONVENTIONAL_MEMORY)
-			lmb_reserve(lmb,
-				    map_to_sysmem((void *)(uintptr_t)
-						  map->physical_start),
-				    map->num_pages * EFI_PAGE_SIZE);
+		if (map->type != EFI_CONVENTIONAL_MEMORY) {
+			lmb_reserve_flags(lmb,
+					  map_to_sysmem((void *)(uintptr_t)
+							map->physical_start),
+					  map->num_pages * EFI_PAGE_SIZE,
+					  map->type == EFI_RESERVED_MEMORY_TYPE
+					      ? LMB_NOMAP : LMB_NONE);
+		}
 	}
 	efi_free_pool(memmap);
 
-- 
2.39.0

Re: [PATCH 2/3] lmb: Set correct lmb flags for EFI memory map entries

[Tom Rini]

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

On Thu, Jan 19, 2023 at 09:38:18AM +0100, Sjoerd Simons wrote:

> When adding reserved memory areas from the EFI memory map set the NOMAP
> flag when applicable. When this isn't done adding "no-map" flagged entries
> from the fdt after receiving the same from the EFI memory map fails due
> to non-matching flags.
> 
> Signed-off-by: Sjoerd Simons <sjoerd@collabora.com>

Applied to u-boot/master, thanks!

-- 
Tom

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

[PATCH 3/3] lmb: Treat a region which is a subset as equal

[Sjoerd Simons]

In various cases logical memory blocks are coalesced; As a result doing
a strict check whether memory blocks are the same doesn't necessarily
work as a previous addition of a given block might have been merged into
a bigger block.

Fix this by considering a block is already registered if it's a pure
subset of one of the existing blocks.

Signed-off-by: Sjoerd Simons <sjoerd@collabora.com>

---

 lib/lmb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/lmb.c b/lib/lmb.c
index f447c639a60..b09a043f4c2 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -248,7 +248,7 @@ static long lmb_add_region_flags(struct lmb_region *rgn, phys_addr_t base,
 		phys_size_t rgnsize = rgn->region[i].size;
 		phys_size_t rgnflags = rgn->region[i].flags;
 
-		if (rgnbase == base && rgnsize == size) {
+		if (rgnbase <= base && rgnbase + rgnsize >= base + size) {
 			if (flags == rgnflags)
 				/* Already have this region, so we're done */
 				return 0;
-- 
2.39.0

Re: [PATCH 3/3] lmb: Treat a region which is a subset as equal

[Tom Rini]

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

On Thu, Jan 19, 2023 at 09:38:19AM +0100, Sjoerd Simons wrote:

> In various cases logical memory blocks are coalesced; As a result doing
> a strict check whether memory blocks are the same doesn't necessarily
> work as a previous addition of a given block might have been merged into
> a bigger block.
> 
> Fix this by considering a block is already registered if it's a pure
> subset of one of the existing blocks.
> 
> Signed-off-by: Sjoerd Simons <sjoerd@collabora.com>
> ---
> 
>  lib/lmb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/lmb.c b/lib/lmb.c
> index f447c639a60..b09a043f4c2 100644
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -248,7 +248,7 @@ static long lmb_add_region_flags(struct lmb_region *rgn, phys_addr_t base,
>  		phys_size_t rgnsize = rgn->region[i].size;
>  		phys_size_t rgnflags = rgn->region[i].flags;
>  
> -		if (rgnbase == base && rgnsize == size) {
> +		if (rgnbase <= base && rgnbase + rgnsize >= base + size) {
>  			if (flags == rgnflags)
>  				/* Already have this region, so we're done */
>  				return 0;

This breaks lib_test_lmb_simple, lib_test_lmb_simple_x2 and
lib_test_lmb_alloc_addr.

-- 
Tom

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

Re: [PATCH 3/3] lmb: Treat a region which is a subset as equal

[Sjoerd Simons]

On Mon, 2023-02-06 at 13:04 -0500, Tom Rini wrote:
> On Thu, Jan 19, 2023 at 09:38:19AM +0100, Sjoerd Simons wrote:
> 
> > In various cases logical memory blocks are coalesced; As a result
> > doing
> > a strict check whether memory blocks are the same doesn't
> > necessarily
> > work as a previous addition of a given block might have been merged
> > into
> > a bigger block.
> > 
> > Fix this by considering a block is already registered if it's a
> > pure
> > subset of one of the existing blocks.
> > 
> > Signed-off-by: Sjoerd Simons <sjoerd@collabora.com>
> > ---
> > 
> >  lib/lmb.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/lmb.c b/lib/lmb.c
> > index f447c639a60..b09a043f4c2 100644
> > --- a/lib/lmb.c
> > +++ b/lib/lmb.c
> > @@ -248,7 +248,7 @@ static long lmb_add_region_flags(struct
> > lmb_region *rgn, phys_addr_t base,
> >                 phys_size_t rgnsize = rgn->region[i].size;
> >                 phys_size_t rgnflags = rgn->region[i].flags;
> >  
> > -               if (rgnbase == base && rgnsize == size) {
> > +               if (rgnbase <= base && rgnbase + rgnsize >= base +
> > size) {
> >                         if (flags == rgnflags)
> >                                 /* Already have this region, so
> > we're done */
> >                                 return 0;
> 
> This breaks lib_test_lmb_simple, lib_test_lmb_simple_x2 and
> lib_test_lmb_alloc_addr.

Oh sorry for missing that; I'll get those fixed. Does the direction at
least seem sensible to you ?


-- 
Sjoerd Simons
Collabora Ltd.

Re: [PATCH 3/3] lmb: Treat a region which is a subset as equal

[Tom Rini]

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

On Wed, Feb 08, 2023 at 08:37:15AM +0100, Sjoerd Simons wrote:
> On Mon, 2023-02-06 at 13:04 -0500, Tom Rini wrote:
> > On Thu, Jan 19, 2023 at 09:38:19AM +0100, Sjoerd Simons wrote:
> > 
> > > In various cases logical memory blocks are coalesced; As a result
> > > doing
> > > a strict check whether memory blocks are the same doesn't
> > > necessarily
> > > work as a previous addition of a given block might have been merged
> > > into
> > > a bigger block.
> > > 
> > > Fix this by considering a block is already registered if it's a
> > > pure
> > > subset of one of the existing blocks.
> > > 
> > > Signed-off-by: Sjoerd Simons <sjoerd@collabora.com>
> > > ---
> > > 
> > >  lib/lmb.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/lib/lmb.c b/lib/lmb.c
> > > index f447c639a60..b09a043f4c2 100644
> > > --- a/lib/lmb.c
> > > +++ b/lib/lmb.c
> > > @@ -248,7 +248,7 @@ static long lmb_add_region_flags(struct
> > > lmb_region *rgn, phys_addr_t base,
> > >                 phys_size_t rgnsize = rgn->region[i].size;
> > >                 phys_size_t rgnflags = rgn->region[i].flags;
> > >  
> > > -               if (rgnbase == base && rgnsize == size) {
> > > +               if (rgnbase <= base && rgnbase + rgnsize >= base +
> > > size) {
> > >                         if (flags == rgnflags)
> > >                                 /* Already have this region, so
> > > we're done */
> > >                                 return 0;
> > 
> > This breaks lib_test_lmb_simple, lib_test_lmb_simple_x2 and
> > lib_test_lmb_alloc_addr.
> 
> Oh sorry for missing that; I'll get those fixed. Does the direction at
> least seem sensible to you ?

I think so, yes.

-- 
Tom

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