Message ID | 20210520012731.3731314-1-swboyd@chromium.org |
---|---|
State | Accepted |
Commit | da17d6905d29ddcdc04b2fdc37ed8cf1e8437cc8 |
Headers | show |
Series | of/fdt: Don't worry about non-memory region overlap for no-map | expand |
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.
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);
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(-) base-commit: 6efb943b8616ec53a5e444193dccf1af9ad627b5