From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-pf0-x22f.google.com ([2607:f8b0:400e:c00::22f]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1a94VF-0004NL-NT for kexec@lists.infradead.org; Wed, 16 Dec 2015 05:19:55 +0000 Received: by mail-pf0-x22f.google.com with SMTP id v86so6087904pfa.2 for ; Tue, 15 Dec 2015 21:19:33 -0800 (PST) Subject: Re: [PATCH v12 11/16] arm64: kdump: reserve memory for crash dump kernel References: <20151215172953.GD353@arm.com> From: AKASHI Takahiro Message-ID: <5670F45E.7060100@linaro.org> Date: Wed, 16 Dec 2015 14:19:26 +0900 MIME-Version: 1.0 In-Reply-To: <20151215172953.GD353@arm.com> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "kexec" Errors-To: kexec-bounces+dwmw2=infradead.org@lists.infradead.org To: Will Deacon , Geoff Levand Cc: Mark Rutland , marc.zyngier@arm.com, Catalin Marinas , kexec@lists.infradead.org, christoffer.dall@linaro.org, linux-arm-kernel@lists.infradead.org On 12/16/2015 02:29 AM, Will Deacon wrote: > On Tue, Nov 24, 2015 at 10:25:34PM +0000, Geoff Levand wrote: >> From: AKASHI Takahiro >> >> On primary kernel, the memory region used by crash dump kernel must be >> specified by "crashkernel=" boot parameter. reserve_crashkernel() >> will allocate and reserve the region for later use. >> >> User space tools will be able to find the region marked as "Crash kernel" >> in /proc/iomem. >> >> Signed-off-by: AKASHI Takahiro >> --- >> arch/arm64/kernel/setup.c | 7 +++++- >> arch/arm64/mm/init.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 60 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c >> index 8119479..f9fffc9 100644 >> --- a/arch/arm64/kernel/setup.c >> +++ b/arch/arm64/kernel/setup.c >> @@ -31,7 +31,6 @@ >> #include >> #include >> #include >> -#include >> #include >> #include >> #include >> @@ -221,6 +220,12 @@ static void __init request_standard_resources(void) >> kernel_data.end <= res->end) >> request_resource(res, &kernel_data); >> } >> + >> +#ifdef CONFIG_KEXEC >> + /* User space tools will find "Crash kernel" region in /proc/iomem. */ >> + if (crashk_res.end) >> + insert_resource(&iomem_resource, &crashk_res); >> +#endif > > Why can't this bit be moved into reserved_crashkernel, at the end? Because - I want to call reserve_crashkernel() as early as possbile, (so before request_standard_resources()), and - if inserting "Crash kernel" region is done before request_standard_resources(), request_resource() for "System RAM" region which contains "Crash kernel" will fail and the entry will be lost from /proc/iomem. >> } >> >> #ifdef CONFIG_BLK_DEV_INITRD >> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c >> index 17bf39a..24f0a1c 100644 >> --- a/arch/arm64/mm/init.c >> +++ b/arch/arm64/mm/init.c >> @@ -34,6 +34,7 @@ >> #include >> #include >> #include >> +#include >> >> #include >> #include >> @@ -66,6 +67,55 @@ static int __init early_initrd(char *p) >> early_param("initrd", early_initrd); >> #endif >> >> +#ifdef CONFIG_KEXEC >> +/* >> + * reserve_crashkernel() - reserves memory for crash kernel >> + * >> + * This function reserves memory area given in "crashkernel=" kernel command >> + * line parameter. The memory reserved is used by dump capture kernel when >> + * primary kernel is crashing. >> + */ >> +static void __init reserve_crashkernel(void) >> +{ >> + unsigned long long crash_size = 0, crash_base = 0; >> + int ret; >> + >> + ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(), >> + &crash_size, &crash_base); >> + if (ret) >> + return; > > What's the point in ret? parse_crashkernel() will returns -EINVAL if - the boot command line doesn't contain "crashkernel=" parameter, or - "crashkernel=" parameter has an invalid synstax of value Otherwise returns 0. So just returning should be OK, but just in case that the syntax is OK, but that a resulting crash_size gets 0, I will add a check like: if (!crash_size) return; >> + >> + if (crash_base == 0) { >> + crash_base = memblock_alloc(crash_size, 1 << 21); > > What is this magic number? How about SZ_2M and a comment mentioning the > arm64 boot protocol? Right. I will do so. >> + if (crash_base == 0) { >> + pr_warn("Unable to allocate crashkernel (size:%llx)\n", >> + crash_size); >> + return; >> + } >> + } else { >> + /* User specifies base address explicitly. */ >> + if (!memblock_is_region_memory(crash_base, crash_size) || >> + memblock_is_region_reserved(crash_base, crash_size)) { >> + pr_warn("crashkernel has wrong address or size\n"); >> + return; >> + } >> + >> + if (crash_base & ((1 << 21) - 1)) { > > IS_ALIGNED OK. >> + pr_warn("crashkernel base address is not 2MB aligned\n"); >> + return; >> + } >> + >> + memblock_reserve(crash_base, crash_size); >> + } >> + >> + pr_info("Reserving %lldMB of memory at %lldMB for crashkernel\n", >> + crash_size >> 20, crash_base >> 20); >> + >> + crashk_res.start = crash_base; >> + crashk_res.end = crash_base + crash_size - 1; >> +} > > #else > > static void __init reserve_crashkernel(void) {}; OK. Thanks, -Takahiro AKASHI > Will > _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec