[PATCH v3] of/fdt: Don't worry about non-memory region overlap for no-map

[PATCH v3] of/fdt: Don't worry about non-memory region overlap for no-map

[Stephen Boyd]

In commit 8a5a75e5e9e5 ("of/fdt: Make sure no-map does not remove
already reserved regions") we returned -EBUSY when trying to mark
regions as no-map when they're in the reserved memory node. This if
condition will still trigger though if the DT has a /memreserve/ that
completely subsumes the no-map memory carveouts in the reserved memory
node. Let's only consider this to be a problem if we're trying to mark a
region as no-map and it is actually memory. If it isn't memory,
presumably it was removed from the memory map via /memreserve/ and thus
can't be mapped anyway.

This silences a warning seen at boot on sc7180-trogdor.dtsi boards that
have /memreserve/ populated by the bootloader where those reserved
regions overlap with the reserved-memory carveouts that we have in DT
for other purposes like communicating with remote processors.

For example in sc7180.dtsi we have the following reserved-memory
node:

	smem_mem: memory@80900000 {
		reg = <0x0 0x80900000 0x0 0x200000>;
		no-map;
	};

and the memreserve injected by the bootloader is

	/memreserve/ 0x80800000 0x400000;

so the reserved memory region overlaps with the no-map carveouts and we
get the following warning message printed at boot:

 OF: fdt: Reserved memory: failed to reserve memory for node 'memory@80900000': base 0x0000000080900000, size 2 MiB

Everything keeps working, just the no-map property is being ignored in
__reserved_mem_reserve_reg() because the region we're trying to avoid
mapping has already been removed from the memory via the memreserve.

Cc: Mike Rapoport <rppt@kernel.org>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Nicolas Boichat <drinkcat@chromium.org>
Cc: Quentin Perret <qperret@google.com>
Cc: Jan Kiszka <jan.kiszka@siemens.com>
Fixes: 8a5a75e5e9e5 ("of/fdt: Make sure no-map does not remove already reserved regions")
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---

Changes from v2 (https://lore.kernel.org/r/20211215072011.496998-1-swboyd@chromium.org):
 * More details in commit text

Changes from v1 (https://lore.kernel.org/r/20210520012731.3731314-1-swboyd@chromium.org):
 * Use memblock_overlaps_region instead of memblock_is_region_memory()
 * Add more details to commit text 

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

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index bdca35284ceb..c736e5bcc2f6 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -482,9 +482,11 @@ static int __init early_init_dt_reserve_memory_arch(phys_addr_t base,
 	if (nomap) {
 		/*
 		 * If the memory is already reserved (by another region), we
-		 * should not allow it to be marked nomap.
+		 * should not allow it to be marked nomap, but don't worry
+		 * if the region isn't memory as it won't be mapped.
 		 */
-		if (memblock_is_region_reserved(base, size))
+		if (memblock_overlaps_region(&memblock.memory, base, size) &&
+		    memblock_is_region_reserved(base, size))
 			return -EBUSY;
 
 		return memblock_mark_nomap(base, size);

base-commit: 136057256686de39cc3a07c2e39ef6bc43003ff6
-- 
https://chromeos.dev

Re: [PATCH v3] of/fdt: Don't worry about non-memory region overlap for no-map

[Mike Rapoport]

On Wed, Dec 15, 2021 at 11:53:54AM -0800, Stephen Boyd wrote:
> In commit 8a5a75e5e9e5 ("of/fdt: Make sure no-map does not remove
> already reserved regions") we returned -EBUSY when trying to mark
> regions as no-map when they're in the reserved memory node. This if
> condition will still trigger though if the DT has a /memreserve/ that
> completely subsumes the no-map memory carveouts in the reserved memory
> node. Let's only consider this to be a problem if we're trying to mark a
> region as no-map and it is actually memory. If it isn't memory,
> presumably it was removed from the memory map via /memreserve/ and thus
> can't be mapped anyway.

I don't see /memreserve/ removing memory from anywhere. What do you
mean here?

> This silences a warning seen at boot on sc7180-trogdor.dtsi boards that
> have /memreserve/ populated by the bootloader where those reserved
> regions overlap with the reserved-memory carveouts that we have in DT
> for other purposes like communicating with remote processors.
> 
> For example in sc7180.dtsi we have the following reserved-memory
> node:
> 
> 	smem_mem: memory@80900000 {
> 		reg = <0x0 0x80900000 0x0 0x200000>;
> 		no-map;
> 	};
> 
> and the memreserve injected by the bootloader is
> 
> 	/memreserve/ 0x80800000 0x400000;
> 
> so the reserved memory region overlaps with the no-map carveouts and we
> get the following warning message printed at boot:
> 
>  OF: fdt: Reserved memory: failed to reserve memory for node 'memory@80900000': base 0x0000000080900000, size 2 MiB
> 
> Everything keeps working, just the no-map property is being ignored in
> __reserved_mem_reserve_reg() because the region we're trying to avoid
> mapping has already been removed from the memory via the memreserve.
> 
> Cc: Mike Rapoport <rppt@kernel.org>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Nicolas Boichat <drinkcat@chromium.org>
> Cc: Quentin Perret <qperret@google.com>
> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> Fixes: 8a5a75e5e9e5 ("of/fdt: Make sure no-map does not remove already reserved regions")
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
> 
> Changes from v2 (https://lore.kernel.org/r/20211215072011.496998-1-swboyd@chromium.org):
>  * More details in commit text
> 
> Changes from v1 (https://lore.kernel.org/r/20210520012731.3731314-1-swboyd@chromium.org):
>  * Use memblock_overlaps_region instead of memblock_is_region_memory()
>  * Add more details to commit text 
> 
>  drivers/of/fdt.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index bdca35284ceb..c736e5bcc2f6 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -482,9 +482,11 @@ static int __init early_init_dt_reserve_memory_arch(phys_addr_t base,
>  	if (nomap) {
>  		/*
>  		 * If the memory is already reserved (by another region), we
> -		 * should not allow it to be marked nomap.
> +		 * should not allow it to be marked nomap, but don't worry
> +		 * if the region isn't memory as it won't be mapped.
>  		 */
> -		if (memblock_is_region_reserved(base, size))
> +		if (memblock_overlaps_region(&memblock.memory, base, size) &&
> +		    memblock_is_region_reserved(base, size))

Apparently I'm missing something, but sc7180.dtsi has memory @80000000 and I
cannot find anything that calls memblock_remove() in DT processing.

How is that memory@80900000 does not overlap with memblock.memory?

>  			return -EBUSY;
>  
>  		return memblock_mark_nomap(base, size);
> 
> base-commit: 136057256686de39cc3a07c2e39ef6bc43003ff6
> -- 
> https://chromeos.dev
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH v3] of/fdt: Don't worry about non-memory region overlap for no-map

[Stephen Boyd]

Quoting Mike Rapoport (2021-12-16 09:23:34)
> On Wed, Dec 15, 2021 at 11:53:54AM -0800, Stephen Boyd wrote:
> > In commit 8a5a75e5e9e5 ("of/fdt: Make sure no-map does not remove
> > already reserved regions") we returned -EBUSY when trying to mark
> > regions as no-map when they're in the reserved memory node. This if
> > condition will still trigger though if the DT has a /memreserve/ that
> > completely subsumes the no-map memory carveouts in the reserved memory
> > node. Let's only consider this to be a problem if we're trying to mark a
> > region as no-map and it is actually memory. If it isn't memory,
> > presumably it was removed from the memory map via /memreserve/ and thus
> > can't be mapped anyway.
>
> I don't see /memreserve/ removing memory from anywhere. What do you
> mean here?

I mean that memory in /memreserve/ is marked as reserved via
early_init_fdt_scan_reserved_mem() calling
early_init_dt_reserve_memory_arch(). I failed to mention that this
region isn't part of the memory the DT tells us exists in the /memory
node. That's the real problem. My bootloader is trying to be helpful and
removing a range of memory that shouldn't be mapped from the /memory
node.

 localhost ~ # hexdump /sys/firmware/devicetree/base/memory/reg
 0000000 0000 0000 0080 0000 0000 0000 8000 0000
 0000010 0000 0000 c080 0000 0000 0000 207f 0000
 0000020 0000 0100 0000 0000 0000 0100 0080 0000

Another solution would be to remove 'no-map' from the reserved memory
nodes that overlap with the /memreserve/ ranges. I'd rather not do that
though in case the bootloader that injects the /memreserve/ and fills in
the 'reg' property of the /memory node decides to stop doing that. It
also doesn't really make sense that no-map would care if the region
isn't memory to start with because the property is telling us to skip
mapping that region of memory into the kernel's direct mapping. By
definition if it isn't in /memory it won't be mapped anyway.

Let me reword this to be more precise. How about this?

----8<----

In commit 8a5a75e5e9e5 ("of/fdt: Make sure no-map does not remove
already reserved regions") we returned -EBUSY when trying to mark
regions as no-map when they intersect with reserved memory. The goal was
to find bad no-map reserved memory DT nodes that would unmap the kernel
text/data sections.

The problem is the reserved memory check will still trigger if the DT
has a /memreserve/ that completely subsumes the no-map memory carveouts
in the reserved memory node _and_ that region is also not part of the
memory reg property. For example in sc7180.dtsi we have the following
reserved-memory and memory node:

      memory@80000000 {
          /* We expect the bootloader to fill in the size */
          reg = <0 0x80000000 0 0>;
      };

      smem_mem: memory@80900000 {
              reg = <0x0 0x80900000 0x0 0x200000>;
              no-map;
      };

and the memreserve filled in by the bootloader is

      /memreserve/ 0x80800000 0x400000;

while the /memory node is transformed into

      memory@80000000 {
          /* The bootloader fills in the size, and adds another region */
          reg = <0 0x80000000 0 0x00800000>,
	        <0 0x80c00000 0 0x7f200000>;
      };

The smem region is doubly reserved via /memreserve/ and by not being
part of the /memory reg property. This leads to the following warning
printed at boot.

 OF: fdt: Reserved memory: failed to reserve memory for node
'memory@80900000': base 0x0000000080900000, size 2 MiB

Otherwise nothing really goes wrong because the smem region is not going
to be mapped by the kernel's direct linear mapping given that it isn't
part of the memory node. Therefore, let's only consider this to be a
problem if we're trying to mark a region as no-map and it is actually
memory that we're intending to keep out of the kernel's direct mapping
but it's already been reserved.

---8<----

> >
> > Changes from v2 (https://lore.kernel.org/r/20211215072011.496998-1-swboyd@chromium.org):
> >  * More details in commit text
> >
> > Changes from v1 (https://lore.kernel.org/r/20210520012731.3731314-1-swboyd@chromium.org):
> >  * Use memblock_overlaps_region instead of memblock_is_region_memory()
> >  * Add more details to commit text
> >
> >  drivers/of/fdt.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index bdca35284ceb..c736e5bcc2f6 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -482,9 +482,11 @@ static int __init early_init_dt_reserve_memory_arch(phys_addr_t base,
> >       if (nomap) {
> >               /*
> >                * If the memory is already reserved (by another region), we
> > -              * should not allow it to be marked nomap.
> > +              * should not allow it to be marked nomap, but don't worry
> > +              * if the region isn't memory as it won't be mapped.
> >                */
> > -             if (memblock_is_region_reserved(base, size))
> > +             if (memblock_overlaps_region(&memblock.memory, base, size) &&
> > +                 memblock_is_region_reserved(base, size))
>
> Apparently I'm missing something, but sc7180.dtsi has memory @80000000 and I
> cannot find anything that calls memblock_remove() in DT processing.
>
> How is that memory@80900000 does not overlap with memblock.memory?
>

There's no size filled in for the sc7180.dtsi file.

Re: [PATCH v3] of/fdt: Don't worry about non-memory region overlap for no-map

[Mike Rapoport]

On Thu, Jan 06, 2022 at 02:41:50PM -0800, Stephen Boyd wrote:
> Quoting Mike Rapoport (2021-12-16 09:23:34)
> > On Wed, Dec 15, 2021 at 11:53:54AM -0800, Stephen Boyd wrote:
> > > In commit 8a5a75e5e9e5 ("of/fdt: Make sure no-map does not remove
> > > already reserved regions") we returned -EBUSY when trying to mark
> > > regions as no-map when they're in the reserved memory node. This if
> > > condition will still trigger though if the DT has a /memreserve/ that
> > > completely subsumes the no-map memory carveouts in the reserved memory
> > > node. Let's only consider this to be a problem if we're trying to mark a
> > > region as no-map and it is actually memory. If it isn't memory,
> > > presumably it was removed from the memory map via /memreserve/ and thus
> > > can't be mapped anyway.
> >
> > I don't see /memreserve/ removing memory from anywhere. What do you
> > mean here?
> 
> I mean that memory in /memreserve/ is marked as reserved via
> early_init_fdt_scan_reserved_mem() calling
> early_init_dt_reserve_memory_arch(). I failed to mention that this
> region isn't part of the memory the DT tells us exists in the /memory
> node. That's the real problem. My bootloader is trying to be helpful and
> removing a range of memory that shouldn't be mapped from the /memory
> node.

That piece is what I was missing. 
 
>  localhost ~ # hexdump /sys/firmware/devicetree/base/memory/reg
>  0000000 0000 0000 0080 0000 0000 0000 8000 0000
>  0000010 0000 0000 c080 0000 0000 0000 207f 0000
>  0000020 0000 0100 0000 0000 0000 0100 0080 0000
> 
> Another solution would be to remove 'no-map' from the reserved memory
> nodes that overlap with the /memreserve/ ranges. I'd rather not do that
> though in case the bootloader that injects the /memreserve/ and fills in
> the 'reg' property of the /memory node decides to stop doing that. It
> also doesn't really make sense that no-map would care if the region
> isn't memory to start with because the property is telling us to skip
> mapping that region of memory into the kernel's direct mapping. By
> definition if it isn't in /memory it won't be mapped anyway.
> 
> Let me reword this to be more precise. How about this?

Works for me, thanks!

Acked-by: Mike Rapoport <rppt@linux.ibm.com>
 
> ----8<----
> 
> In commit 8a5a75e5e9e5 ("of/fdt: Make sure no-map does not remove
> already reserved regions") we returned -EBUSY when trying to mark
> regions as no-map when they intersect with reserved memory. The goal was
> to find bad no-map reserved memory DT nodes that would unmap the kernel
> text/data sections.
> 
> The problem is the reserved memory check will still trigger if the DT
> has a /memreserve/ that completely subsumes the no-map memory carveouts
> in the reserved memory node _and_ that region is also not part of the
> memory reg property. For example in sc7180.dtsi we have the following
> reserved-memory and memory node:
> 
>       memory@80000000 {
>           /* We expect the bootloader to fill in the size */
>           reg = <0 0x80000000 0 0>;
>       };
> 
>       smem_mem: memory@80900000 {
>               reg = <0x0 0x80900000 0x0 0x200000>;
>               no-map;
>       };
> 
> and the memreserve filled in by the bootloader is
> 
>       /memreserve/ 0x80800000 0x400000;
> 
> while the /memory node is transformed into
> 
>       memory@80000000 {
>           /* The bootloader fills in the size, and adds another region */
>           reg = <0 0x80000000 0 0x00800000>,
> 	        <0 0x80c00000 0 0x7f200000>;
>       };
> 
> The smem region is doubly reserved via /memreserve/ and by not being
> part of the /memory reg property. This leads to the following warning
> printed at boot.
> 
>  OF: fdt: Reserved memory: failed to reserve memory for node
> 'memory@80900000': base 0x0000000080900000, size 2 MiB
> 
> Otherwise nothing really goes wrong because the smem region is not going
> to be mapped by the kernel's direct linear mapping given that it isn't
> part of the memory node. Therefore, let's only consider this to be a
> problem if we're trying to mark a region as no-map and it is actually
> memory that we're intending to keep out of the kernel's direct mapping
> but it's already been reserved.
> 
> ---8<----
> 
> > >
> > > Changes from v2 (https://lore.kernel.org/r/20211215072011.496998-1-swboyd@chromium.org):
> > >  * More details in commit text
> > >
> > > Changes from v1 (https://lore.kernel.org/r/20210520012731.3731314-1-swboyd@chromium.org):
> > >  * Use memblock_overlaps_region instead of memblock_is_region_memory()
> > >  * Add more details to commit text
> > >
> > >  drivers/of/fdt.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > index bdca35284ceb..c736e5bcc2f6 100644
> > > --- a/drivers/of/fdt.c
> > > +++ b/drivers/of/fdt.c
> > > @@ -482,9 +482,11 @@ static int __init early_init_dt_reserve_memory_arch(phys_addr_t base,
> > >       if (nomap) {
> > >               /*
> > >                * If the memory is already reserved (by another region), we
> > > -              * should not allow it to be marked nomap.
> > > +              * should not allow it to be marked nomap, but don't worry
> > > +              * if the region isn't memory as it won't be mapped.
> > >                */
> > > -             if (memblock_is_region_reserved(base, size))
> > > +             if (memblock_overlaps_region(&memblock.memory, base, size) &&
> > > +                 memblock_is_region_reserved(base, size))
> >
> > Apparently I'm missing something, but sc7180.dtsi has memory @80000000 and I
> > cannot find anything that calls memblock_remove() in DT processing.
> >
> > How is that memory@80900000 does not overlap with memblock.memory?
> >
> 
> There's no size filled in for the sc7180.dtsi file.

-- 
Sincerely yours,
Mike.