Thanks James for the feedback. Please find replies inline. Regards Poonam > -----Original Message----- > From: James Morse [mailto:james.morse@xxxxxxx] > Sent: Friday, January 05, 2018 4:37 PM > To: Poonam Aggrwal <poonam.aggrwal@xxxxxxx>; linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx > Cc: takahiro.akashi@xxxxxxxxxx; will.deacon@xxxxxxx; G.h. Gao > <guanhua.gao@xxxxxxx>; Abhimanyu Saini <abhimanyu.saini@xxxxxxx> > Subject: Re: [PATCH][v2] arm64: Allocate elfcorehdr & crashkernel mem before > any reservation > > Hi Poonam, > > On 04/01/18 15:34, Poonam Aggrwal wrote: > > Address for both crashkernel memory and elfcorehdr can be assigned > > statically. For crashkernel memory it is via crashkernel=SIZE@ADDRESS > > and elfcorehdr_addr via by kexec-utils in dump kernel device tree. > > There are some crashkernel=SIZE@ADDRESS values that the user can supply that > we must reject. The obvious one is if it overlaps with the kernel text. (this patch > won't spot this). We need to read the hardware's reserved regions from the DT > before we allocate the crashkernel region, for example if the bootloader > reserved a chunk of memory for a frame-buffer, I shouldn't be able to use that > as crashkernel memory. > > (you shouldn't need to specify an address, doing so prevents the kernel from > picking memory it can use) > > > > So memory should be reserved for both the above areas before any other > > memory reservations are done. Otherwise overlaps may happen and memory > > allocations will fail leading to failure in booting the dump capture > > kernel. > > > Signed-off-by: Guanhua <guanhua.gao@xxxxxxx> > > The first signed-off-by should be the patch author. > If you want to credit your colleagues you can use 'CC', or they can give > reviewed-by/acked-by to the patch. > > > > Signed-off-by: Poonam Aggrwal <poonam.aggrwal@xxxxxxx> > > Signed-off-by: Abhimanyu Saini <abhimanyu.saini@xxxxxxx> > > And the last signed-of-by should be from the person posting to the mailing list. > Sure will correct this. > > > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c index > > 00e7b90..24ce15d 100644 > > --- a/arch/arm64/mm/init.c > > +++ b/arch/arm64/mm/init.c > > @@ -453,6 +453,14 @@ void __init arm64_memblock_init(void) > > * Register the kernel text, kernel data, initrd, and initial > > * pagetables with memblock. > > */ > > Please don't insert this code between 'memblock_reserve(__pa_symbol(_text), > _end > - _text);' and the comment that describes it. Thanks for catching this, will fix it. > > > > + > > + /* make these the first reservations to avoid chances of > > + * overlap > > + */ > > Nit: comment style Right, will fix it. > > > > + reserve_elfcorehdr(); > > (Moving reserve_elfcorehdr() makes sense, but..) > > > > + reserve_crashkernel(); > > reserve_crashkernel() does the allocation for the crashkernels reserved memory. > I expect this to always fail in the kdump kernel because there isn't enough > memory. (fdt_enforce_memory_region() at the top of this function calls > memblock_cap_memory_range()). > > Moving this allocation above the early_init_fdt_scan_reserved_mem() below > means we may allocate memory for the crashdump that is in use by > firmware/hardware and described as reserved in the DT. Yeah, this is a good point. So ideally the address of the crash kernel should be diligently provided by the user based on the system. > > > > memblock_reserve(__pa_symbol(_text), _end - _text); #ifdef > > CONFIG_BLK_DEV_INITRD > > if (initrd_start) { > > @@ -472,10 +480,6 @@ void __init arm64_memblock_init(void) > > else > > arm64_dma_phys_limit = PHYS_MASK + 1; > > > > - reserve_crashkernel(); > > - > > - reserve_elfcorehdr(); > > - > > high_memory = __va(memblock_end_of_DRAM() - 1) + 1; > > > > dma_contiguous_reserve(arm64_dma_phys_limit); > > > > > Thanks, > > James _______________________________________________ kexec mailing list kexec@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/kexec
