[PATCH] arch/arm64: elfcorehdr should be the first allocation

[Poonam Aggrwal]

> -----Original Message-----
> From: AKASHI Takahiro [mailto:takahiro.akashi at linaro.org]
> Sent: Monday, December 18, 2017 11:03 AM
> To: Poonam Aggrwal <poonam.aggrwal at nxp.com>
> Cc: Abhimanyu Saini <abhimanyu.saini at nxp.com>; Will Deacon
> <will.deacon at arm.com>; Prabhakar Kushwaha
> <prabhakar.kushwaha at nxp.com>; linux-arm-kernel at lists.infradead.org; G.h.
> Gao <guanhua.gao at nxp.com>; james.morse at arm.com
> Subject: Re: [PATCH] arch/arm64: elfcorehdr should be the first allocation
> 
> On Fri, Dec 15, 2017 at 02:20:15PM +0000, Poonam Aggrwal wrote:
> > Thanks Takahiro and Will,
> >
> > Please find responses below.
> >
> > Regards
> > Poonam
> > > -----Original Message-----
> > > From: AKASHI Takahiro [mailto:takahiro.akashi at linaro.org]
> > > Sent: Wednesday, December 13, 2017 4:17 PM
> > > To: Abhimanyu Saini <abhimanyu.saini at nxp.com>
> > > Cc: Will Deacon <will.deacon at arm.com>; Prabhakar Kushwaha
> > > <prabhakar.kushwaha at nxp.com>; linux-arm-kernel at lists.infradead.org;
> > > Poonam Aggrwal <poonam.aggrwal at nxp.com>; G.h. Gao
> > > <guanhua.gao at nxp.com>; james.morse at arm.com
> > > Subject: Re: [PATCH] arch/arm64: elfcorehdr should be the first
> > > allocation
> > >
> > > On Mon, Dec 11, 2017 at 02:07:14PM +0000, Will Deacon wrote:
> > > > On Mon, Dec 11, 2017 at 11:03:32AM +0530, Prabhakar Kushwaha wrote:
> > > > > From: Abhimanyu Saini <abhimanyu.saini at nxp.com>
> > > > >
> > > > > elfcorehdr_addr is assigned by kexec-utils and device tree of
> > > > > dump kernel is fixed in chosen node with parameter "linux,elfcorehdr".
> > > > > So, memory should be first reserved for elfcorehdr, otherwise
> > > > > overlaps may happen with other memory allocations which were
> > > > > done before the allocation of elcorehdr in the crash kernel
> > > >
> > > > What happens in that case? Do you have a crash log we can include
> > > > in the commit message?
> > >
> > > In private discussions with Poonam, he said:
> > > |   The overlap here I observed was for the reserved-mem areas in the dtb.
> > > |   And they were specific to NXP device.
> > Thanks Takahiro for providing this information.
> > Yes the memory overlap happens because when the dump-capture kernel tries
> to reserve elfcoreheader at the specific address, it finds that the address has
> already been reserved for some other use by
> early_init_fdt_scan_reserved_mem().
> > Based on the "reserved-memory" node entries in the dts file , the overlap is
> seen to happen with one of the below alocations:
> 
> So the point is that reserve_elfcorehdr() should be placed before
> early_init_fdt_scan_reserved_mem() which may allocate memory dynamically,
> isn't it. I think it makes sense.
> 
> Lisewise, reserve_crashkernel(), which can also allocate memory statically in
> case of "crashkernel=SIZE at ADDRESS", should be.
> (Even if an overrap might happen, this wouldn't prevent the system from booting
> though. We just can't use kdump in this case.)
> 
Right, so probably both the allocations (elfcorehdr and crash kernel) should go before the  early_init_fdt_scan_reserved_mem.
Should I send a re-spin?
> Thanks,
> -Takahiro AKASHI
> 
> > [    0.000000] Reserved memory: created DMA memory pool at
> 0x00000000fb800000, size 4 MiB
> > [    0.000000] Reserved memory: initialized node qman-fqd, compatible id
> shared-dma-pool
> > [    0.000000] Reserved memory: created DMA memory pool at
> 0x00000000f8000000, size 32 MiB
> > [    0.000000] Reserved memory: initialized node qman-pfdr, compatible id
> shared-dma-pool
> > >
> > > Since I have not got any details since then, I'm not sure whether
> > > your patch is the way to go.
> > > (I suspect that we might better fix the issue on kexec-tools side.)
> > >
> > I may not have full understanding of kexec-tools, but I am not sure how will
> kexec tools be able to know what addresses will get  assigned for the reserved-
> memory regions in the device tree.
> > > Thanks,
> > > -Takahiro AKASHI
> > >
> > > > > Signed-off-by: Guanhua <guanhua.gao at nxp.com>
> > > > > Signed-off-by: Poonam Aggrwal <poonam.aggrwal at nxp.com>
> > > > > Signed-off-by: Abhimanyu Saini <abhimanyu.saini at nxp.com>
> > > > > ---
> > > >
> > > > Really? How on Earth did you get three people co-developing this patch?
> >  : )Contribution was from all in terms of reproducing, testing on various
> platforms including the debug experiments. Just wanted to acknowledge the
> effort.
> > > >
> > > > >  arch/arm64/mm/init.c | 6 ++++--
> > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c index
> > > > > 5960bef0170d..551048cfcfff 100644
> > > > > --- a/arch/arm64/mm/init.c
> > > > > +++ b/arch/arm64/mm/init.c
> > > > > @@ -453,6 +453,10 @@ void __init arm64_memblock_init(void)
> > > > >  	 * Register the kernel text, kernel data, initrd, and initial
> > > > >  	 * pagetables with memblock.
> > > > >  	 */
> > > > > +
> > > > > +	/* make this the first reservation so that there are no chances
> of
> > > > > +	 * overlap */
> > > > > +	reserve_elfcorehdr();
> > > > >  	memblock_reserve(__pa_symbol(_text), _end - _text);  #ifdef
> > > > > CONFIG_BLK_DEV_INITRD
> > > > >  	if (initrd_start) {
> > > > > @@ -474,8 +478,6 @@ void __init arm64_memblock_init(void)
> > > > >
> > > > >  	reserve_crashkernel();
> > > > >
> > > > > -	reserve_elfcorehdr();
> > > >
> > > > Why isn't this also a problem for reserve_crashkernel() or any
> > > > other static reservations?
> > Thanks, for raising this. Yes looking at the code flow this may also cause a
> problem, definitely when the start address of the crash kernel is specified in the
> bootargs. We mostly tested with just the size, so it was an allocation without a
> specific address.
> >
> > > >
> > > > Will