[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <852b8777-3c6e-f76b-0413-1c66629f33cd@huawei.com>
Date: Thu, 15 Jun 2023 17:49:10 +0800
From: "chenjiahao (C)" <chenjiahao16@...wei.com>
To: Baoquan He <bhe@...hat.com>
CC: <linux-kernel@...r.kernel.org>, <linux-riscv@...ts.infradead.org>,
<kexec@...ts.infradead.org>, <linux-doc@...r.kernel.org>,
<paul.walmsley@...ive.com>, <palmer@...belt.com>,
<conor.dooley@...rochip.com>, <guoren@...nel.org>,
<heiko@...ech.de>, <bjorn@...osinc.com>, <alex@...ti.fr>,
<akpm@...ux-foundation.org>, <atishp@...osinc.com>,
<thunder.leizhen@...wei.com>, <horms@...nel.org>
Subject: Re: [PATCH -next v5 1/2] riscv: kdump: Implement
crashkernel=X,[high,low]
On 2023/6/4 11:50, Baoquan He wrote:
> Hi Jiahao,
>
> On 05/11/23 at 04:51pm, Chen Jiahao wrote:
> ......
>> @@ -1300,14 +1325,34 @@ static void __init reserve_crashkernel(void)
>> return;
>> }
>>
>> - ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
>> + ret = parse_crashkernel(cmdline, memblock_phys_mem_size(),
>> &crash_size, &crash_base);
>> - if (ret || !crash_size)
>> + if (ret == -ENOENT) {
>> + /* Fallback to crashkernel=X,[high,low] */
>> + ret = parse_crashkernel_high(cmdline, 0, &crash_size, &crash_base);
>> + if (ret || !crash_size)
>> + return;
>> +
>> + /*
>> + * crashkernel=Y,low is valid only when crashkernel=X,high
>> + * is passed.
>> + */
>> + ret = parse_crashkernel_low(cmdline, 0, &crash_low_size, &crash_base);
>> + if (ret == -ENOENT)
>> + crash_low_size = DEFAULT_CRASH_KERNEL_LOW_SIZE;
>> + else if (ret)
>> + return;
>> +
>> + search_end = memblock_end_of_DRAM();
>> + } else if (ret || !crash_size) {
>> + /* Invalid argument value specified */
>> return;
>> + }
> The parsing part looks great, while you didn't mark if it's specified
> high reservation, please see later comment why it's needed.
>
>>
>> crash_size = PAGE_ALIGN(crash_size);
>>
>> if (crash_base) {
>> + fixed_base = true;
>> search_start = crash_base;
>> search_end = crash_base + crash_size;
>> }
>> @@ -1320,17 +1365,31 @@ static void __init reserve_crashkernel(void)
>> * swiotlb can work on the crash kernel.
>> */
>> crash_base = memblock_phys_alloc_range(crash_size, PMD_SIZE,
>> - search_start,
>> - min(search_end, (unsigned long) SZ_4G));
>> + search_start, search_end);
> If it's a specified high reservation, you have
> search_start = memblock_start_of_DRAM();
> search_end = memblock_end_of_DRAM();
>
> Then it attempts to search top down first time here.
>
>> if (crash_base == 0) {
>> - /* Try again without restricting region to 32bit addressible memory */
>> + if (fixed_base) {
>> + pr_warn("crashkernel: allocating failed with given size@...set\n");
>> + return;
>> + }
>> + search_end = memblock_end_of_DRAM();
>> +
>> + /* Try again above the region of 32bit addressible memory */
>> crash_base = memblock_phys_alloc_range(crash_size, PMD_SIZE,
>> - search_start, search_end);
>> + search_start, search_end);
> If crashkernel=,high case, the first attempt failed, here it assigns
> search_end with memblock_end_of_DRAM(). It's the exactly the same
> attempt, why is that needed? Why don't you use a local variable 'high'
> to mark the crashkernel=,hig, then judge when deciding how to adjsut the
> reservation range.
>
> Do I misunderstand the code?
>
> Thanks
> Baoquan
You are right. Here I use search_end = memblock_end_of_DRAM() for the
first attempt on "crashkernel=,high" case, but it will not distinct from
other cases if the first attempt fails.
I have read your latest refactor on Arm64, introducing the "high" flag
is a good choice, the logic gets more straightforward when handling
crashkernel=,high case and retrying.
Following that logic, here introducing and set "high" flag when parsing
cmdline, when the first attempt failed:
if fixed_base:
failed and return;
if set high:
search_start = memblock_start_of_DRAM();
search_end = (unsigned long)dma32_phys_limit;
else:
search_start = (unsigned long)dma32_phys_limit;
search_end = memblock_end_of_DRAM();
second attempt with new {search_start, search_end}
...
This should handle "crashkernel=,high" case correctly and avoid cross
4G reservation.
Is that logic correct, or is any other problem missed?
Thanks,
Jiahao
>
>> if (crash_base == 0) {
>> pr_warn("crashkernel: couldn't allocate %lldKB\n",
>> crash_size >> 10);
>> return;
>> }
>> +
>> + if (!crash_low_size)
>> + crash_low_size = DEFAULT_CRASH_KERNEL_LOW_SIZE;
>> + }
>> +
>> + if ((crash_base > dma32_phys_limit - crash_low_size) &&
>> + crash_low_size && reserve_crashkernel_low(crash_low_size)) {
>> + memblock_phys_free(crash_base, crash_size);
>> + return;
>> }
>>
>> pr_info("crashkernel: reserved 0x%016llx - 0x%016llx (%lld MB)\n",
>> --
>> 2.31.1
>>
Powered by blists - more mailing lists