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@xxxxxxxxxxxxx>
> ----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@xxxxxxxxxxxx):
> > > * More details in commit text
> > >
> > > Changes from v1 (https://lore.kernel.org/r/20210520012731.3731314-1-swboyd@xxxxxxxxxxxx):
> > > * 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.
