[PATCH v22 2/8] arm64: limit memory regions based on DT property, usable-memory-range

[takahiro.akashi@linaro.org (AKASHI Takahiro)]

On Wed, Jul 20, 2016 at 11:39:11AM +0800, Dennis Chen wrote:
> On Tue, Jul 19, 2016 at 08:01:21PM +0900, AKASHI Takahiro wrote:
> > On Tue, Jul 19, 2016 at 06:06:18PM +0800, Dennis Chen wrote:
> > > Hello AKASHI,
> > > 
> > > On Tue, Jul 19, 2016 at 05:35:55PM +0900, AKASHI Takahiro wrote:
> > > > James,
> > > >
> > > > On Mon, Jul 18, 2016 at 07:04:33PM +0100, James Morse wrote:
> > > > > Hi!
> > > > >
> > > > > (CC: Dennis Chen)
> > > > >
> > > > > On 12/07/16 06:05, AKASHI Takahiro wrote:
> > > > > > Crash dump kernel will be run with a limited range of memory as System
> > > > > > RAM.
> > > > > >
> > > > > > On arm64, we will use a device-tree property under /chosen,
> > > > > >    linux,usable-memory-range = <BASE SIZE>
> > > > > > in order for primary kernel either on uefi or non-uefi (device tree only)
> > > > > > system to hand over the information about usable memory region to crash
> > > > > > dump kernel. This property will supercede entries in uefi memory map table
> > > > > > and "memory" nodes in a device tree.
> > > > >
> > > > > > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> > > > > > index 51b1302..d8b296f 100644
> > > > > > --- a/arch/arm64/mm/init.c
> > > > > > +++ b/arch/arm64/mm/init.c
> > > > > > @@ -300,10 +300,48 @@ static int __init early_mem(char *p)
> > > > > >  }
> > > > > >  early_param("mem", early_mem);
> > > > > >
> > > > > > +static int __init early_init_dt_scan_usablemem(unsigned long node,
> > > > > > +         const char *uname, int depth, void *data)
> > > > > > +{
> > > > > > + struct memblock_region *usablemem = (struct memblock_region *)data;
> > > > > > + const __be32 *reg;
> > > > > > + int len;
> > > > > > +
> > > > > > + usablemem->size = 0;
> > > > > > +
> > > > > > + if (depth != 1 || strcmp(uname, "chosen") != 0)
> > > > > > +         return 0;
> > > > > > +
> > > > > > + reg = of_get_flat_dt_prop(node, "linux,usable-memory-range", &len);
> > > > > > + if (!reg || (len < (dt_root_addr_cells + dt_root_size_cells)))
> > > > > > +         return 1;
> > > > > > +
> > > > > > + usablemem->base = dt_mem_next_cell(dt_root_addr_cells, &reg);
> > > > > > + usablemem->size = dt_mem_next_cell(dt_root_size_cells, &reg);
> > > > > > +
> > > > > > + return 1;
> > > > > > +}
> > > > > > +
> > > > > > +static void __init fdt_enforce_memory_region(void)
> > > > > > +{
> > > > > > + struct memblock_region reg;
> > > > > > +
> > > > > > + of_scan_flat_dt(early_init_dt_scan_usablemem, &reg);
> > > > > > +
> > > > > > + if (reg.size) {
> > > > > > +         memblock_remove(0, PAGE_ALIGN(reg.base));
> > > > > > +         memblock_remove(round_down(reg.base + reg.size, PAGE_SIZE),
> > > > > > +                         ULLONG_MAX);
> > > > >
> > > 
> > > According to the panic message from James, I guess the ACPI regions are out of the range
> > > [reg.base, reg.base + reg.size] and removed by your above codes. On ARM64, those ACPI
> > > regions have been added into memblock and marked as NOMAP, so I think it should be
> > > easy to adapt my fix to retain the NOMAP regions here
> > 
> > See below.
> > 
> > > Thanks,
> > > Dennis
> > > 
> > > > > I think this is a new way to trip the problem Dennis Chen has been working on
> > > > > [0]. If I kdump with --reuse-cmdline on a kernel booted with 'acpi=on', I get
> > > > > the panic below [1]...
> > > >
> > > > Yeah, it can be.
> > > >
> > > > > It looks like Dennis's fix involves changes in mm/memblock.c, maybe they can be
> > > > > extended to support a range instead of just a limit?
> > > > >
> > > > > (It looks like x86 explicitly adds the acpi regions to the crash-kernels memory
> > > > > map in crash_setup_memmap_entries()).
> > > > >
> > > > >
> > > > >
> > > > > Is it possible for the kernel text to be outside this range? (a bug in
> > > > > kexec-tools, or another user of the DT property) If we haven't already failed in
> > > > > this case, it may be worth printing a warning, or refusing to
> > > > > restrict-memory/expose-vmcore.
> > > >
> > > > In my implementation of kdump, the usable memory for crash dump
> > > > kernel will be allocated within memblock.memory after ACPI-related
> > > > regions have been mapped. "linux,usable-memory-range" indicates
> > > > this exact memory range.
> > > > On crash dump kernel, my fdt_enforce_memory_region() in arm64_memblock_init()
> > > > will exclude all the other regions from memblock.memory.
> > > > So the kernel (with acpi=on) won't recognize ACPI-regions as
> > > > normal memory, and map them by ioremap().
> > > >
> > > > I thought that it was safe, but actually not due to unaligned accesses.
> > > > As you suggested, we will probably be able to do the same thing of
> > > > Chen's solution in fdt_enforce_memory_region().
> > 
> > memblock_isolate_range() and memblock_remove_range() are not exported.
> > So we'd better implement an unified interface like:
> >     memblock_cap_memory_range(phys_addr_t base, size_t size);
> > 
> > If base == NULL, it would behave in the exact same way as your
> > memblock_mem_limit_remove_map().
> >
> Hello AKASHI, it's not reasonable to change the prototype of an existing memblock API

I didn't say memblock_enforce_memory_limit(), but
*your* memblock_memblock_remove_limit().

> which will be used by other components as we did with memblock_enforce_memory_limit. 
> Moreover the @size in you case is to specify a memory range of the memblock, which is
> different from the @limit as an indicator of the total size of memblocks being limited. 
> But I can be help to post an new memblock API patch to cater for your case.

Thanks, but I have already prototyped the function.
If you don't agree to my opinion, I will have to submit
a patch for a quite similar but different function.
I think that nobody will accept this.

-Takahiro AKASHI

> Thanks,
> Dennis
>   
> >
> > Thanks,
> > -Takahiro AKASHI
> > 
> > > >
> > > >
> > > > Thanks,
> > > > -Takahiro AKASHI
> > > >
>