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

[PATCH] 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 for me on sc7180-trogdor.dtsi
boards that have /memreserve/ coming from the bootloader and those
reserved regions overlap with the carveouts that we want to use in DT
for other purposes like communicating with remote processors.

Cc: Douglas Anderson <dianders@chromium.org>
Cc: Nicolas Boichat <drinkcat@chromium.org>
Cc: Quentin Perret <qperret@google.com>
Fixes: 8a5a75e5e9e5 ("of/fdt: Make sure no-map does not remove already reserved regions")
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/of/fdt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index ba17a80b8c79..be13b4b6c2d8 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -1161,7 +1161,8 @@ int __init __weak early_init_dt_reserve_memory_arch(phys_addr_t base,
 		 * If the memory is already reserved (by another region), we
 		 * should not allow it to be marked nomap.
 		 */
-		if (memblock_is_region_reserved(base, size))
+		if (memblock_is_region_memory(base, size) &&
+		    memblock_is_region_reserved(base, size))
 			return -EBUSY;
 
 		return memblock_mark_nomap(base, size);

base-commit: 6efb943b8616ec53a5e444193dccf1af9ad627b5
-- 
https://chromeos.dev

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

[Doug Anderson]

Hi,

On Wed, May 19, 2021 at 6:27 PM Stephen Boyd <swboyd@chromium.org> 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.
>
> This silences a warning seen at boot for me on sc7180-trogdor.dtsi
> boards that have /memreserve/ coming from the bootloader and those
> reserved regions overlap with the carveouts that we want to use in DT
> for other purposes like communicating with remote processors.
>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Nicolas Boichat <drinkcat@chromium.org>
> Cc: Quentin Perret <qperret@google.com>
> Fixes: 8a5a75e5e9e5 ("of/fdt: Make sure no-map does not remove already reserved regions")
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/of/fdt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index ba17a80b8c79..be13b4b6c2d8 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -1161,7 +1161,8 @@ int __init __weak early_init_dt_reserve_memory_arch(phys_addr_t base,
>                  * If the memory is already reserved (by another region), we
>                  * should not allow it to be marked nomap.
>                  */
> -               if (memblock_is_region_reserved(base, size))
> +               if (memblock_is_region_memory(base, size) &&
> +                   memblock_is_region_reserved(base, size))
>                         return -EBUSY;

I'm not an expert on this code, so take review comments w/ a grain of salt.

That being said, while the change looks right on the surface, I'm not
sure it's 100% right when I dig. The names of
memblock_is_region_memory() and memblock_is_region_reserved() make
them sound more similar than they actually are. One of the two tests
for intersection and the other for "subset of". I think if
memblock_is_region_memory() used "intersects" instead of "subset of"
then your patch would be correct.

Specifically if you've got memory regions:

0x1000 - 0x2000 - memory
0x3000 - 0x4000 - memory

Then you check memblock_is_region_memory(0x2800, 0x1000) or
memblock_is_region_memory(0x1800, 0x1000) then I think it will return
false, right? Because those aren't _subsets_ of memory even though
they intersect memory.

I don't know if cases like that show up in practice, but it seems
better to be safe?

-Doug

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

[Stephen Boyd]

Quoting Doug Anderson (2021-05-20 09:17:39)
> Hi,
>
> On Wed, May 19, 2021 at 6:27 PM Stephen Boyd <swboyd@chromium.org> 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.
> >
> > This silences a warning seen at boot for me on sc7180-trogdor.dtsi
> > boards that have /memreserve/ coming from the bootloader and those
> > reserved regions overlap with the carveouts that we want to use in DT
> > for other purposes like communicating with remote processors.
> >
> > Cc: Douglas Anderson <dianders@chromium.org>
> > Cc: Nicolas Boichat <drinkcat@chromium.org>
> > Cc: Quentin Perret <qperret@google.com>
> > Fixes: 8a5a75e5e9e5 ("of/fdt: Make sure no-map does not remove already reserved regions")
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> >  drivers/of/fdt.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index ba17a80b8c79..be13b4b6c2d8 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -1161,7 +1161,8 @@ int __init __weak early_init_dt_reserve_memory_arch(phys_addr_t base,
> >                  * If the memory is already reserved (by another region), we
> >                  * should not allow it to be marked nomap.
> >                  */
> > -               if (memblock_is_region_reserved(base, size))
> > +               if (memblock_is_region_memory(base, size) &&
> > +                   memblock_is_region_reserved(base, size))
> >                         return -EBUSY;
>
> I'm not an expert on this code, so take review comments w/ a grain of salt.
>
> That being said, while the change looks right on the surface, I'm not
> sure it's 100% right when I dig. The names of
> memblock_is_region_memory() and memblock_is_region_reserved() make
> them sound more similar than they actually are. One of the two tests
> for intersection and the other for "subset of". I think if
> memblock_is_region_memory() used "intersects" instead of "subset of"
> then your patch would be correct.
>
> Specifically if you've got memory regions:
>
> 0x1000 - 0x2000 - memory
> 0x3000 - 0x4000 - memory
>
> Then you check memblock_is_region_memory(0x2800, 0x1000) or
> memblock_is_region_memory(0x1800, 0x1000) then I think it will return
> false, right? Because those aren't _subsets_ of memory even though
> they intersect memory.
>
> I don't know if cases like that show up in practice, but it seems
> better to be safe?
>

Good point. If the reserved region intersects with memory and "not
memory" then the check won't be attempted to see if the region
intersects with a reserved region. I was trying to match the spirit of
the commit that this is fixing, i.e. trying to only care if the region
is for kernel memory. At least my read of that commit text led me to
believe that it was trying to print a warning if we have a big kernel
image reservation that overlaps with the reserved memory regions in DT.
Before that patch it would silently allow a no-map region to overlap
with the kernel image (text/data/heap/bss) and then unmap the kernel,
ouch! Now it will return -EBUSY and not let the kernel unmap itself.

In my case I have a /memreserve/ region from DT that marks a large
region as reserved, and then I have a few reserved memory nodes in DT
that are marked no-map and they fit entirely inside the /memreserve/
region so they're a "subset of". Let me see if I can devise some way to
fix both scenarios. My understanding is /memreserve/ is essentially the
same as no-map in that the kernel doesn't consider the region as memory
and doesn't map it in the kernel page tables, so this silences a warning
for me but functionally doesn't change anything.