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

[Mike Rapoport]

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.