On Wed, Feb 01, 2017 at 03:26:09PM +0000, Mark Rutland wrote:
> Hi,
>
> On Wed, Feb 01, 2017 at 09:46:22PM +0900, AKASHI Takahiro wrote:
> > +#ifdef CONFIG_KEXEC_CORE
> > +/*
> > + * 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_base, crash_size;
> > + int ret;
> > +
> > + ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
> > + &crash_size, &crash_base);
> > + /* no crashkernel= or invalid value specified */
> > + if (ret || !crash_size)
> > + return;
> > +
> > + crash_size = PAGE_ALIGN(crash_size);
> > +
> > + if (crash_base == 0) {
> > + /* Current arm64 boot protocol requires 2MB alignment */
> > + crash_base = memblock_find_in_range(0, ARCH_LOW_ADDRESS_LIMIT,
> > + crash_size, SZ_2M);
> > + if (crash_base == 0) {
> > + pr_warn("Unable to allocate crashkernel (size:%llx)\n",
> > + crash_size);
>
> Nit: can we please make that "size: 0x%llx", so that it's always clearly
> a hex number? e.g.
>
> pr_warn("cannot allocate 0x%llx bytes for crashkernel\n",
> crash_size);
OK
> > + 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;
> > + }
>
> To better report the error, can we please split these up:
>
> if (!memblock_is_region_memory(crash_base, crash_size)) {
> pr_warn("cannot reserve crashkernel: region is not memory\n");
> return;
> }
>
> if (!memblock_is_region_memory(crash_base, crash_size)) {
> pr_warn("cannot reserve crashkernel: region overlaps reserved memory\n");
> return;
> }
OK, and
> > + if (!IS_ALIGNED(crash_base, SZ_2M)) {
> > + pr_warn("crashkernel base address is not 2MB aligned\n");
pr_warn("cannot reserve 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);
>
> We only page-align the size, so the MB will be a little off, but that's
> probably OK. However, it would also be nicer to log the base as an
> address.
You might notice that the exact same message is used by all the other
architectures, but
> Could we dump this as we do for the kernel memory layout? e.g.
>
> pr_info("crashkernel reserved: 0x%016lx - 0x%016lx (%lld MB)\n",
> crash_base, crash_base + crash_size, crash_size >> 20);
We can go either way.
> With those:
>
> Acked-by: Mark Rutland <mark.rutland at arm.com>
Thanks,
-Takahiro AKASHI
> Thanks,
> Mark.
