* [PATCH v2] of/fdt: Don't worry about non-memory region overlap for no-map
@ 2021-12-15 7:20 Stephen Boyd
2021-12-15 9:49 ` Mike Rapoport
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2021-12-15 7:20 UTC (permalink / raw)
To: Rob Herring
Cc: linux-kernel, devicetree, Mike Rapoport, Douglas Anderson,
Nicolas Boichat, Quentin Perret, Jan Kiszka
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
OF: fdt: Reserved memory: failed to reserve memory for node 'memory@80900000': base 0x0000000080900000, size 2 MiB
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 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] of/fdt: Don't worry about non-memory region overlap for no-map
2021-12-15 7:20 [PATCH v2] of/fdt: Don't worry about non-memory region overlap for no-map Stephen Boyd
@ 2021-12-15 9:49 ` Mike Rapoport
2021-12-15 10:01 ` Mike Rapoport
2021-12-15 19:28 ` Stephen Boyd
0 siblings, 2 replies; 6+ messages in thread
From: Mike Rapoport @ 2021-12-15 9:49 UTC (permalink / raw)
To: Stephen Boyd
Cc: Rob Herring, linux-kernel, devicetree, Douglas Anderson,
Nicolas Boichat, Quentin Perret, Jan Kiszka
Hi,
On Tue, Dec 14, 2021 at 11:20:11PM -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 have no objections for this patch, but I afraid that this is a never
ending story of reservation vs nomap ordering and this won't be the last
fix in the area.
I was toying with the idea to use flags in memblock.reserved to have
clearer view of how the reserved memory was used and then we won't need
to guess firmware intentions.
Thoughts?
> 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.
Do you mind adding the relevant pats of the device tree to the changelog?
> For example
>
> OF: fdt: Reserved memory: failed to reserve memory for node 'memory@80900000': base 0x0000000080900000, size 2 MiB
>
> 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 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
>
--
Sincerely yours,
Mike.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] of/fdt: Don't worry about non-memory region overlap for no-map
2021-12-15 9:49 ` Mike Rapoport
@ 2021-12-15 10:01 ` Mike Rapoport
2021-12-15 19:24 ` Stephen Boyd
2021-12-15 19:28 ` Stephen Boyd
1 sibling, 1 reply; 6+ messages in thread
From: Mike Rapoport @ 2021-12-15 10:01 UTC (permalink / raw)
To: Stephen Boyd
Cc: Rob Herring, linux-kernel, devicetree, Douglas Anderson,
Nicolas Boichat, Quentin Perret, Jan Kiszka
On Wed, Dec 15, 2021 at 11:49:37AM +0200, Mike Rapoport wrote:
>
> On Tue, Dec 14, 2021 at 11:20:11PM -0800, Stephen Boyd wrote:
> > @@ -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))
One more small thing. Maybe add pr_warn() here?
> > return -EBUSY;
> >
> > return memblock_mark_nomap(base, size);
--
Sincerely yours,
Mike.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] of/fdt: Don't worry about non-memory region overlap for no-map
2021-12-15 10:01 ` Mike Rapoport
@ 2021-12-15 19:24 ` Stephen Boyd
0 siblings, 0 replies; 6+ messages in thread
From: Stephen Boyd @ 2021-12-15 19:24 UTC (permalink / raw)
To: Mike Rapoport
Cc: Rob Herring, linux-kernel, devicetree, Douglas Anderson,
Nicolas Boichat, Quentin Perret, Jan Kiszka
Quoting Mike Rapoport (2021-12-15 02:01:59)
> On Wed, Dec 15, 2021 at 11:49:37AM +0200, Mike Rapoport wrote:
> >
> > On Tue, Dec 14, 2021 at 11:20:11PM -0800, Stephen Boyd wrote:
> > > @@ -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))
>
> One more small thing. Maybe add pr_warn() here?
>
There's already a print in __reserved_mem_reserve_reg() when this
function returns an error so it doesn't seem very useful to add another
print.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] of/fdt: Don't worry about non-memory region overlap for no-map
2021-12-15 9:49 ` Mike Rapoport
2021-12-15 10:01 ` Mike Rapoport
@ 2021-12-15 19:28 ` Stephen Boyd
2021-12-16 16:43 ` Mike Rapoport
1 sibling, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2021-12-15 19:28 UTC (permalink / raw)
To: Mike Rapoport
Cc: Rob Herring, linux-kernel, devicetree, Douglas Anderson,
Nicolas Boichat, Quentin Perret, Jan Kiszka
Quoting Mike Rapoport (2021-12-15 01:49:29)
> Hi,
>
> On Tue, Dec 14, 2021 at 11:20:11PM -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 have no objections for this patch, but I afraid that this is a never
> ending story of reservation vs nomap ordering and this won't be the last
> fix in the area.
Ugh ok
>
> I was toying with the idea to use flags in memblock.reserved to have
> clearer view of how the reserved memory was used and then we won't need
> to guess firmware intentions.
> Thoughts?
My understanding of the commit being fixed was that it tried to detect
bad DT where two reserved regions overlapped and different reserved
memory regions stomped on each other. It certainly seems like that could
be improved by recording what reserved memory region it belongs to, but
within memblock I don't know if it cares. I thought memblock just cared
to find out what is memory and what is supposed to be mapped into the
page tables.
>
> > 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.
>
> Do you mind adding the relevant pats of the device tree to the changelog?
Sure. Let me add the reserved memory snippet.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] of/fdt: Don't worry about non-memory region overlap for no-map
2021-12-15 19:28 ` Stephen Boyd
@ 2021-12-16 16:43 ` Mike Rapoport
0 siblings, 0 replies; 6+ messages in thread
From: Mike Rapoport @ 2021-12-16 16:43 UTC (permalink / raw)
To: Stephen Boyd
Cc: Rob Herring, linux-kernel, devicetree, Douglas Anderson,
Nicolas Boichat, Quentin Perret, Jan Kiszka
On Wed, Dec 15, 2021 at 11:28:14AM -0800, Stephen Boyd wrote:
> Quoting Mike Rapoport (2021-12-15 01:49:29)
> > Hi,
> >
> > On Tue, Dec 14, 2021 at 11:20:11PM -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 have no objections for this patch, but I afraid that this is a never
> > ending story of reservation vs nomap ordering and this won't be the last
> > fix in the area.
>
> Ugh ok
>
> >
> > I was toying with the idea to use flags in memblock.reserved to have
> > clearer view of how the reserved memory was used and then we won't need
> > to guess firmware intentions.
> > Thoughts?
>
> My understanding of the commit being fixed was that it tried to detect
> bad DT where two reserved regions overlapped and different reserved
> memory regions stomped on each other. It certainly seems like that could
> be improved by recording what reserved memory region it belongs to, but
> within memblock I don't know if it cares. I thought memblock just cared
> to find out what is memory and what is supposed to be mapped into the
> page tables.
Except the creation of page tables, data in memblock is used to populate
the memory map and the free lists, so it's important to know where there is
memory, where there are holes and what memory is in use.
For now, any used memory will be listed in memblock.reserved, no matter if
it's used by firwmare, marked reserved in DT, occupied by the kernel code
or allocated early during boot before "real" mm is setup.
If we track the types of the memory reservations in memblock.reserved we'll
know which regions are /memreserve/, which are nomap and which are used by
kernel itself and so we'll have more deterministic view on what can be
allowed and what not. E.g it's ok to set nomap for a region that is already
reserved by firmware, but we cannot set nomap on memory used by the kernel.
For now this is a theory, I haven't even got to experiment with this.
> > > 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.
> >
> > Do you mind adding the relevant pats of the device tree to the changelog?
>
> Sure. Let me add the reserved memory snippet.
--
Sincerely yours,
Mike.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-12-16 16:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-15 7:20 [PATCH v2] of/fdt: Don't worry about non-memory region overlap for no-map Stephen Boyd
2021-12-15 9:49 ` Mike Rapoport
2021-12-15 10:01 ` Mike Rapoport
2021-12-15 19:24 ` Stephen Boyd
2021-12-15 19:28 ` Stephen Boyd
2021-12-16 16:43 ` Mike Rapoport
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).