Re: [PATCH] of: fdt: Check overlap of reserved memory regions

[Marten Lindahl <martenli@axis.com>]

On Thu, Jan 13, 2022 at 11:12:11PM +0100, Stephen Boyd wrote:
> Quoting Marten Lindahl (2022-01-13 07:56:42)
> > On Tue, Jan 11, 2022 at 07:34:00PM +0100, Rob Herring wrote:
> > > On Tue, Jan 11, 2022 at 6:25 AM Mårten Lindahl <marten.lindahl@axis.com> wrote:
> >
> > Hi Rob!
> > Thanks for looking at this.
> > > >
> > > > If a DT specified reserved memory region overlaps an already registered
> > > > reserved region no notification is made. Starting the system with
> > > > overlapped memory regions can make it very hard to debug what is going
> > > > wrong. This is specifically true in case the ramoops console intersects
> > > > with initrd since the console overwrites memory that is used for initrd,
> > > > which leads to memory corruption.
> > > >
> > > > Highlight this by printing a message about overlapping memory regions.
> > >
> > > Won't this be noisy if a region is described in both /memreserve/ and
> > > /reserved-memory node?
> > >
> > Yes, it can potentially be noisy if doing so. But I think notifying this
> > can be useful. Should it perhaps be a notification instead of a warning?
> >

Hi Stephen!
> 
> Please don't print any message for /memreserve/ and /reserved-memory nodes
> overlapping. On the chromebook at my desk we have overlapping
> /memreserve/ and /reserved-memory. My understanding is that it's
> redundant to have both, especially when a reserved-memory node has
> 'no-map', but it isn't forbidden. The /memreserve/ is like a no-map
> /resreved-memory node without the phandle.
> 
> Given that initrd is special cased in drivers/of/fdt.c can the reserved
> memory handling code look to see if it overlaps with the initrd region
> and skip that /reserved-memory carveout? A warning could probably be
> printed and ramoops should fail to probe.

I understand if the check would spam on some system setups. So yes, I should
make the check less generic. The case which I describe with initrd and ramoops
is something that I think should be warned about.

But this would result in a very specific check for these two regions. So I'm
thinking, since the ramoops region is the one that will cause overwrites of
any other intersecting region, not necessarily just initrd, would it maybe
make sense to just add an extra check for ramoops and then print the warning?

And then still let ramoops run, as it depends on what memory part is
conflicting, and may not necessarily break the system.

Something like this?

if (!fdt_node_check_compatible(initial_boot_params,
			       node, "ramoops") &&
    size && memblock_is_reserved(base)) {
	pr_warn("WARNING: %s [0x%08llx+0x%08llx] overlaps reserved memory region\n",
		uname, (u64)base, (u64)size);
}

Any more thoughts?

Kind regards
Mårten