Date:   Fri, 27 Dec 2019 19:04:23 +0800
From:   Chen Zhou <chenzhou10@...wei.com>
To:     Dave Young <dyoung@...hat.com>
CC:     <tglx@...utronix.de>, <mingo@...hat.com>,
        <catalin.marinas@....com>, <will@...nel.org>,
        <james.morse@....com>, <bhsharma@...hat.com>, <horms@...ge.net.au>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <kexec@...ts.infradead.org>,
        <linux-doc@...r.kernel.org>, <xiexiuqi@...wei.com>,
        kbuild test robot <lkp@...el.com>
Subject: Re: [PATCH v7 1/4] x86: kdump: move reserve_crashkernel_low() into
 crash_core.c

Hi Dave

On 2019/12/27 13:54, Dave Young wrote:
> Hi,
> On 12/23/19 at 11:23pm, Chen Zhou wrote:
>> In preparation for supporting reserve_crashkernel_low in arm64 as
>> x86_64 does, move reserve_crashkernel_low() into kernel/crash_core.c.
>>
>> Note, in arm64, we reserve low memory if and only if crashkernel=X,low
>> is specified. Different with x86_64, don't set low memory automatically.
> 
> Do you have any reason for the difference?  I'd expect we have same
> logic if possible and remove some of the ifdefs.

In x86_64, if we reserve crashkernel above 4G, then we call reserve_crashkernel_low()
to reserve low memory.

In arm64, to simplify, we call reserve_crashkernel_low() at the beginning of reserve_crashkernel()
and then relax the arm64_dma32_phys_limit if reserve_crashkernel_low() allocated something.
In this case, if reserve crashkernel below 4G there will be 256M low memory set automatically
and this needs extra considerations.

previous discusses:
	https://lkml.org/lkml/2019/6/5/670
	https://lkml.org/lkml/2019/6/13/229

> 
>>
>> Reported-by: kbuild test robot <lkp@...el.com>
>> Signed-off-by: Chen Zhou <chenzhou10@...wei.com>
>> ---
>>  arch/x86/kernel/setup.c    | 62 ++++-----------------------------
>>  include/linux/crash_core.h |  3 ++
>>  include/linux/kexec.h      |  2 --
>>  kernel/crash_core.c        | 87 ++++++++++++++++++++++++++++++++++++++++++++++
>>  kernel/kexec_core.c        | 17 ---------
>>  5 files changed, 96 insertions(+), 75 deletions(-)
>>
>> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
>> index cedfe20..5f38942 100644
>> --- a/arch/x86/kernel/setup.c
>> +++ b/arch/x86/kernel/setup.c
>> @@ -486,59 +486,6 @@ static void __init memblock_x86_reserve_range_setup_data(void)
>>  # define CRASH_ADDR_HIGH_MAX	SZ_64T
>>  #endif
>>  
>> -static int __init reserve_crashkernel_low(void)
>> -{
>> -#ifdef CONFIG_X86_64
>> -	unsigned long long base, low_base = 0, low_size = 0;
>> -	unsigned long total_low_mem;
>> -	int ret;
>> -
>> -	total_low_mem = memblock_mem_size(1UL << (32 - PAGE_SHIFT));
>> -
>> -	/* crashkernel=Y,low */
>> -	ret = parse_crashkernel_low(boot_command_line, total_low_mem, &low_size, &base);
>> -	if (ret) {
>> -		/*
>> -		 * two parts from kernel/dma/swiotlb.c:
>> -		 * -swiotlb size: user-specified with swiotlb= or default.
>> -		 *
>> -		 * -swiotlb overflow buffer: now hardcoded to 32k. We round it
>> -		 * to 8M for other buffers that may need to stay low too. Also
>> -		 * make sure we allocate enough extra low memory so that we
>> -		 * don't run out of DMA buffers for 32-bit devices.
>> -		 */
>> -		low_size = max(swiotlb_size_or_default() + (8UL << 20), 256UL << 20);
>> -	} else {
>> -		/* passed with crashkernel=0,low ? */
>> -		if (!low_size)
>> -			return 0;
>> -	}
>> -
>> -	low_base = memblock_find_in_range(0, 1ULL << 32, low_size, CRASH_ALIGN);
>> -	if (!low_base) {
>> -		pr_err("Cannot reserve %ldMB crashkernel low memory, please try smaller size.\n",
>> -		       (unsigned long)(low_size >> 20));
>> -		return -ENOMEM;
>> -	}
>> -
>> -	ret = memblock_reserve(low_base, low_size);
>> -	if (ret) {
>> -		pr_err("%s: Error reserving crashkernel low memblock.\n", __func__);
>> -		return ret;
>> -	}
>> -
>> -	pr_info("Reserving %ldMB of low memory at %ldMB for crashkernel (System low RAM: %ldMB)\n",
>> -		(unsigned long)(low_size >> 20),
>> -		(unsigned long)(low_base >> 20),
>> -		(unsigned long)(total_low_mem >> 20));
>> -
>> -	crashk_low_res.start = low_base;
>> -	crashk_low_res.end   = low_base + low_size - 1;
>> -	insert_resource(&iomem_resource, &crashk_low_res);
>> -#endif
>> -	return 0;
>> -}
>> -
>>  static void __init reserve_crashkernel(void)
>>  {
>>  	unsigned long long crash_size, crash_base, total_mem;
>> @@ -602,9 +549,12 @@ static void __init reserve_crashkernel(void)
>>  		return;
>>  	}
>>  
>> -	if (crash_base >= (1ULL << 32) && reserve_crashkernel_low()) {
>> -		memblock_free(crash_base, crash_size);
>> -		return;
>> +	if (crash_base >= (1ULL << 32)) {
>> +		if (reserve_crashkernel_low()) {
>> +			memblock_free(crash_base, crash_size);
>> +			return;
>> +		}
>> +		insert_resource(&iomem_resource, &crashk_low_res);
> 
> Some specific reason to move insert_resouce out of the
> reserve_crashkernel_low function?

No specific reason.
I just exposed arm64 "Crash kernel low" in request_standard_resources() as other resources,
so did this change.

> 
>>  	}
>>  
>>  	pr_info("Reserving %ldMB of memory at %ldMB for crashkernel (System RAM: %ldMB)\n",
>> diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
>> index 525510a..4df8c0b 100644
>> --- a/include/linux/crash_core.h
>> +++ b/include/linux/crash_core.h
>> @@ -63,6 +63,8 @@ phys_addr_t paddr_vmcoreinfo_note(void);
>>  extern unsigned char *vmcoreinfo_data;
>>  extern size_t vmcoreinfo_size;
>>  extern u32 *vmcoreinfo_note;
>> +extern struct resource crashk_res;
>> +extern struct resource crashk_low_res;
>>  
>>  Elf_Word *append_elf_note(Elf_Word *buf, char *name, unsigned int type,
>>  			  void *data, size_t data_len);
>> @@ -74,5 +76,6 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,
>>  		unsigned long long *crash_size, unsigned long long *crash_base);
>>  int parse_crashkernel_low(char *cmdline, unsigned long long system_ram,
>>  		unsigned long long *crash_size, unsigned long long *crash_base);
>> +int __init reserve_crashkernel_low(void);
>>  
>>  #endif /* LINUX_CRASH_CORE_H */
>> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
>> index 1776eb2..5d5d963 100644
>> --- a/include/linux/kexec.h
>> +++ b/include/linux/kexec.h
>> @@ -330,8 +330,6 @@ extern int kexec_load_disabled;
>>  
>>  /* Location of a reserved region to hold the crash kernel.
>>   */
>> -extern struct resource crashk_res;
>> -extern struct resource crashk_low_res;
>>  extern note_buf_t __percpu *crash_notes;
>>  
>>  /* flag to track if kexec reboot is in progress */
>> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
>> index 9f1557b..eb72fd6 100644
>> --- a/kernel/crash_core.c
>> +++ b/kernel/crash_core.c
>> @@ -7,6 +7,8 @@
>>  #include <linux/crash_core.h>
>>  #include <linux/utsname.h>
>>  #include <linux/vmalloc.h>
>> +#include <linux/memblock.h>
>> +#include <linux/swiotlb.h>
>>  
>>  #include <asm/page.h>
>>  #include <asm/sections.h>
>> @@ -19,6 +21,22 @@ u32 *vmcoreinfo_note;
>>  /* trusted vmcoreinfo, e.g. we can make a copy in the crash memory */
>>  static unsigned char *vmcoreinfo_data_safecopy;
>>  
>> +/* Location of the reserved area for the crash kernel */
>> +struct resource crashk_res = {
>> +	.name  = "Crash kernel",
>> +	.start = 0,
>> +	.end   = 0,
>> +	.flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM,
>> +	.desc  = IORES_DESC_CRASH_KERNEL
>> +};
>> +struct resource crashk_low_res = {
>> +	.name  = "Crash kernel",
>> +	.start = 0,
>> +	.end   = 0,
>> +	.flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM,
>> +	.desc  = IORES_DESC_CRASH_KERNEL
>> +};
>> +
>>  /*
>>   * parsing the "crashkernel" commandline
>>   *
>> @@ -292,6 +310,75 @@ int __init parse_crashkernel_low(char *cmdline,
>>  				"crashkernel=", suffix_tbl[SUFFIX_LOW]);
>>  }
>>  
>> +#if defined(CONFIG_X86_64)
>> +#define CRASH_ALIGN		SZ_16M
>> +#elif defined(CONFIG_ARM64)
>> +#define CRASH_ALIGN		SZ_2M
>> +#endif
> 
> I think no need to have the #ifdef, although I can not think out of
> reason we have 16M for X86, maybe move it to 2M as well if no other
> objections.  Then it will be easier to reserve crashkernel successfully
> considering nowadays we have KASLR and other stuff it becomes harder.

I also don't figure out why it is 16M in x86.

> 
>> +
>> +int __init reserve_crashkernel_low(void)
>> +{
>> +#if defined(CONFIG_X86_64) || defined(CONFIG_ARM64)
>> +	unsigned long long base, low_base = 0, low_size = 0;
>> +	unsigned long total_low_mem;
>> +	int ret;
>> +
>> +	total_low_mem = memblock_mem_size(1UL << (32 - PAGE_SHIFT));
>> +
>> +	/* crashkernel=Y,low */
>> +	ret = parse_crashkernel_low(boot_command_line, total_low_mem, &low_size,
>> +			&base);
>> +	if (ret) {
>> +#ifdef CONFIG_X86_64
>> +		/*
>> +		 * two parts from lib/swiotlb.c:
>> +		 * -swiotlb size: user-specified with swiotlb= or default.
>> +		 *
>> +		 * -swiotlb overflow buffer: now hardcoded to 32k. We round it
>> +		 * to 8M for other buffers that may need to stay low too. Also
>> +		 * make sure we allocate enough extra low memory so that we
>> +		 * don't run out of DMA buffers for 32-bit devices.
>> +		 */
>> +		low_size = max(swiotlb_size_or_default() + (8UL << 20),
>> +				256UL << 20);
>> +#else
>> +		/*
>> +		 * in arm64, reserve low memory if and only if crashkernel=X,low
>> +		 * specified.
>> +		 */
>> +		return -EINVAL;
>> +#endif
> 
> As said before, can you explore about why it needs different logic, it
> would be good to keep two arches same.
> 
>> +	} else {
>> +		/* passed with crashkernel=0,low ? */
>> +		if (!low_size)
>> +			return 0;
>> +	}
>> +
>> +	low_base = memblock_find_in_range(0, 1ULL << 32, low_size, CRASH_ALIGN);
>> +	if (!low_base) {
>> +		pr_err("Cannot reserve %ldMB crashkernel low memory, please try smaller size.\n",
>> +		       (unsigned long)(low_size >> 20));
>> +		return -ENOMEM;
>> +	}
>> +
>> +	ret = memblock_reserve(low_base, low_size);
>> +	if (ret) {
>> +		pr_err("%s: Error reserving crashkernel low memblock.\n",
>> +				__func__);
>> +		return ret;
>> +	}
>> +
>> +	pr_info("Reserving %ldMB of low memory at %ldMB for crashkernel (System low RAM: %ldMB)\n",
>> +		(unsigned long)(low_size >> 20),
>> +		(unsigned long)(low_base >> 20),
>> +		(unsigned long)(total_low_mem >> 20));
>> +
>> +	crashk_low_res.start = low_base;
>> +	crashk_low_res.end   = low_base + low_size - 1;
>> +#endif
>> +	return 0;
>> +}
>> +
>>  Elf_Word *append_elf_note(Elf_Word *buf, char *name, unsigned int type,
>>  			  void *data, size_t data_len)
>>  {
>> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
>> index 15d70a9..458d093 100644
>> --- a/kernel/kexec_core.c
>> +++ b/kernel/kexec_core.c
>> @@ -53,23 +53,6 @@ note_buf_t __percpu *crash_notes;
>>  /* Flag to indicate we are going to kexec a new kernel */
>>  bool kexec_in_progress = false;
>>  
>> -
>> -/* Location of the reserved area for the crash kernel */
>> -struct resource crashk_res = {
>> -	.name  = "Crash kernel",
>> -	.start = 0,
>> -	.end   = 0,
>> -	.flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM,
>> -	.desc  = IORES_DESC_CRASH_KERNEL
>> -};
>> -struct resource crashk_low_res = {
>> -	.name  = "Crash kernel",
>> -	.start = 0,
>> -	.end   = 0,
>> -	.flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM,
>> -	.desc  = IORES_DESC_CRASH_KERNEL
>> -};
>> -
>>  int kexec_should_crash(struct task_struct *p)
>>  {
>>  	/*
>> -- 
>> 2.7.4
>>
> 
> Thanks
> Dave
> 
> 
> .
> 
Thanks,
Chen Zhou

Powered by blists - more mailing lists