[PATCH v2 0/1] Fix Random Kernel panic from when fail to reserve memory

[PATCH v2 0/1] Fix Random Kernel panic from when fail to reserve memory

[Lucas Tanure]

I am trying to fix an issue where the kernel panics randomly on my Vim3
board. The problem happens when the ARM Trusted Firmware memory is not
removed from the available ram.

This happens because my u-boot provides /memreserve/, but it doesn't flag
it as nomap. And the kernel scan of /reserved-memory node can't map as
nomap as the region is already reserved.

The idea now is to scan /memreserve/ after /reserved-memory node, and
/memreserve/ will skip regions already reserved by /reserved-memory node.

V1 Patch series: https://lore.kernel.org/all/20230406151429.524591-1-tanure@linux.com/

Previous Threads:
#regzbot link: https://lore.kernel.org/linux-arm-kernel/40ca11f84b7cdbfb9ad2ddd480cb204a@agner.ch/#regzbot
#regzbot link: https://lore.kernel.org/all/CAJX_Q+1Tjc+-TjZ6JW9X0NxEdFe=82a9626yL63j7uVD4LpxEA@mail.gmail.com/

Change from V1:
 - Instead of allowing to mark nomap a region already reserved, give
precedence to /reserved-memory node scan.

Lucas Tanure (1):
  of: fdt: Scan /memreserve/ last

 drivers/of/fdt.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

-- 
2.40.0

[PATCH v2 1/1] of: fdt: Scan /memreserve/ last

[Lucas Tanure]

Change the order of scanning /memreserve/ and /reserved-memory node.
/reserved-memory node should go first, as it has a more updated
description of the memory regions and it can apply flags, like nomap.
Also, /memreserve/ should avoid reserving regions described in
/reserved-memory node.

Signed-off-by: Lucas Tanure <tanure@linux.com>
---
 drivers/of/fdt.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index d1a68b6d03b3..26e608d8025d 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -635,16 +635,21 @@ void __init early_init_fdt_scan_reserved_mem(void)
 	if (!initial_boot_params)
 		return;
 
+	fdt_scan_reserved_mem();
+	fdt_reserve_elfcorehdr();
+
 	/* Process header /memreserve/ fields */
 	for (n = 0; ; n++) {
 		fdt_get_mem_rsv(initial_boot_params, n, &base, &size);
 		if (!size)
 			break;
+		if (memblock_overlaps_region(&memblock.memory, base, size) &&
+		    memblock_is_region_reserved(base, size))
+			break;
+
 		memblock_reserve(base, size);
 	}
 
-	fdt_scan_reserved_mem();
-	fdt_reserve_elfcorehdr();
 	fdt_init_reserved_mem();
 }
 
-- 
2.40.0

Re: [PATCH v2 1/1] of: fdt: Scan /memreserve/ last

[Rob Herring]

On Mon, Apr 10, 2023 at 7:00 AM Lucas Tanure <tanure@linux.com> wrote:
>
> Change the order of scanning /memreserve/ and /reserved-memory node.
> /reserved-memory node should go first, as it has a more updated
> description of the memory regions and it can apply flags, like nomap.
> Also, /memreserve/ should avoid reserving regions described in
> /reserved-memory node.

Like I said on v1, I think doing this has a high chance of causing
regressions on other platforms. It should probably not go to stable
for some time after a kernel release.

> Signed-off-by: Lucas Tanure <tanure@linux.com>
> ---
>  drivers/of/fdt.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index d1a68b6d03b3..26e608d8025d 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -635,16 +635,21 @@ void __init early_init_fdt_scan_reserved_mem(void)
>         if (!initial_boot_params)
>                 return;
>
> +       fdt_scan_reserved_mem();
> +       fdt_reserve_elfcorehdr();
> +
>         /* Process header /memreserve/ fields */
>         for (n = 0; ; n++) {
>                 fdt_get_mem_rsv(initial_boot_params, n, &base, &size);
>                 if (!size)
>                         break;
> +               if (memblock_overlaps_region(&memblock.memory, base, size) &&
> +                   memblock_is_region_reserved(base, size))

Just to make sure, a partial overlap will still get reserved?

> +                       break;

Shouldn't we continue to the next entry rather than stopping.

> +
>                 memblock_reserve(base, size);
>         }
>
> -       fdt_scan_reserved_mem();
> -       fdt_reserve_elfcorehdr();
>         fdt_init_reserved_mem();
>  }
>
> --
> 2.40.0
>

Re: [PATCH v2 1/1] of: fdt: Scan /memreserve/ last

[Lucas Tanure]

On Mon, Apr 10, 2023 at 8:52 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Mon, Apr 10, 2023 at 7:00 AM Lucas Tanure <tanure@linux.com> wrote:
> >
> > Change the order of scanning /memreserve/ and /reserved-memory node.
> > /reserved-memory node should go first, as it has a more updated
> > description of the memory regions and it can apply flags, like nomap.
> > Also, /memreserve/ should avoid reserving regions described in
> > /reserved-memory node.
>
> Like I said on v1, I think doing this has a high chance of causing
> regressions on other platforms. It should probably not go to stable
> for some time after a kernel release.
>
> > Signed-off-by: Lucas Tanure <tanure@linux.com>
> > ---
> >  drivers/of/fdt.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index d1a68b6d03b3..26e608d8025d 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -635,16 +635,21 @@ void __init early_init_fdt_scan_reserved_mem(void)
> >         if (!initial_boot_params)
> >                 return;
> >
> > +       fdt_scan_reserved_mem();
> > +       fdt_reserve_elfcorehdr();
> > +
> >         /* Process header /memreserve/ fields */
> >         for (n = 0; ; n++) {
> >                 fdt_get_mem_rsv(initial_boot_params, n, &base, &size);
> >                 if (!size)
> >                         break;
> > +               if (memblock_overlaps_region(&memblock.memory, base, size) &&
> > +                   memblock_is_region_reserved(base, size))
>
> Just to make sure, a partial overlap will still get reserved?
A partial overlap will get reserved if not already reserved by the
/reserved-memory node.
>
> > +                       break;
>
> Shouldn't we continue to the next entry rather than stopping.
Yes, my mistake; I will send v3.
>
> > +
> >                 memblock_reserve(base, size);
> >         }
> >
> > -       fdt_scan_reserved_mem();
> > -       fdt_reserve_elfcorehdr();
> >         fdt_init_reserved_mem();
> >  }
> >
> > --
> > 2.40.0
> >

Re: [PATCH v2 1/1] of: fdt: Scan /memreserve/ last

[Mike Rapoport]

On Mon, Apr 10, 2023 at 08:00:17AM -0400, Lucas Tanure wrote:
> Change the order of scanning /memreserve/ and /reserved-memory node.
> /reserved-memory node should go first, as it has a more updated
> description of the memory regions and it can apply flags, like nomap.
> Also, /memreserve/ should avoid reserving regions described in
> /reserved-memory node.
> 
> Signed-off-by: Lucas Tanure <tanure@linux.com>
> ---
>  drivers/of/fdt.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index d1a68b6d03b3..26e608d8025d 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -635,16 +635,21 @@ void __init early_init_fdt_scan_reserved_mem(void)
>  	if (!initial_boot_params)
>  		return;
>  
> +	fdt_scan_reserved_mem();
> +	fdt_reserve_elfcorehdr();
> +
>  	/* Process header /memreserve/ fields */
>  	for (n = 0; ; n++) {
>  		fdt_get_mem_rsv(initial_boot_params, n, &base, &size);
>  		if (!size)
>  			break;
> +		if (memblock_overlaps_region(&memblock.memory, base, size) &&
> +		    memblock_is_region_reserved(base, size))
> +			break;

I don't think this is really needed, it's ok to reserve the same ranges
multiple times.
Both checks are not cheap, so it'll be better to just reserve everything
both from performance and simplicity points of view.

> +
>  		memblock_reserve(base, size);
>  	}
>  
> -	fdt_scan_reserved_mem();
> -	fdt_reserve_elfcorehdr();
>  	fdt_init_reserved_mem();
>  }
>  
> -- 
> 2.40.0
> 

-- 
Sincerely yours,
Mike.