Re: [PATCH v7 1/4] x86: kdump: move reserve_crashkernel_low() into crash_core.c

From: Chen Zhou
Date: Mon Dec 30 2019 - 20:40:11 EST

Next message: Rob Landley: "Re: Why is CONFIG_VT forced on?"
Previous message: YueHaibing: "[PATCH v2 -next] mtd: rawnand: macronix: Use match_string() helper to simplify the code"
In reply to: Dave Young: "Re: [PATCH v7 1/4] x86: kdump: move reserve_crashkernel_low() into crash_core.c"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Dave,

On 2019/12/28 17:32, Dave Young wrote:
> On 12/27/19 at 07:04pm, Chen Zhou wrote:
>> 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.
>
> Sorry that I did not read the old thread details and thought that is
> arch dependent. But rethink about that, it would be better that we can
> have same semantic about crashkernel parameters across arches. If we
> make them different then it causes confusion, especially for
> distributions.
>
> OTOH, I thought if we reserve high memory then the low memory should be
> needed. There might be some exceptions, but I do not know the exact
> one, can we make the behavior same, and special case those systems which
> do not need low memory reservation.
>
I thought like this and did implement with crashkernel parameters arch independent.
This is my v4: https://lkml.org/lkml/2019/5/6/1361, i implemented according to x86_64's
behavior.

>>
>> previous discusses:
>> https://lkml.org/lkml/2019/6/5/670
>> https://lkml.org/lkml/2019/6/13/229
>
> Another concern from James:
> "
> With both crashk_low_res and crashk_res, we end up with two entries in /proc/iomem called
> "Crash kernel". Because its sorted by address, and kexec-tools stops searching when it
> find "Crash kernel", you are always going to get the kernel placed in the lower portion.
> "
>
> The kexec-tools code is iterating all "Crash kernel" ranges and add them
> in an array. In X86 code, it uses the higher range to locate memory.

We also discussed about this: https://lkml.org/lkml/2019/6/13/227.
I guess James's opinion is that kexec-tools should take forward compatibility into account.
"But we can't rely on people updating user-space when they update the kernel!" -- James

>
>>
>>>
>>>>
>>>> Reported-by: kbuild test robot <lkp@xxxxxxxxx>
>>>> Signed-off-by: Chen Zhou <chenzhou10@xxxxxxxxxx>
>>>> ---
>>>> 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.
>
> Ok.
>
>>
>>>
>>>> }
>>>>
>>>> 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.
>
> IMHO, if we do not know why and in theory it should work with 2M, can
> you do some basic testing and move it to 2M?
>
> We can easily move back to 16M if someone really report something, but
> if we do not change it will always stay there but we do not know why.

Ok. I will do some test later.

>
>>
>>>
>>>> +
>>>> +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
>>
>
> Thanks
> Dave
>
>

Thanks,
Chen Zhou

Next message: Rob Landley: "Re: Why is CONFIG_VT forced on?"
Previous message: YueHaibing: "[PATCH v2 -next] mtd: rawnand: macronix: Use match_string() helper to simplify the code"
In reply to: Dave Young: "Re: [PATCH v7 1/4] x86: kdump: move reserve_crashkernel_low() into crash_core.c"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]