Re: [PATCH -next v5 1/2] riscv: kdump: Implement crashkernel=X,[high,low]

From: "chenjiahao (C)" <chenjiahao16@huawei.com>
To: Baoquan He <bhe@redhat.com>
Cc: <linux-kernel@vger.kernel.org>, <linux-riscv@lists.infradead.org>,
	<kexec@lists.infradead.org>, <linux-doc@vger.kernel.org>,
	<paul.walmsley@sifive.com>, <palmer@dabbelt.com>,
	<conor.dooley@microchip.com>, <guoren@kernel.org>,
	<heiko@sntech.de>, <bjorn@rivosinc.com>, <alex@ghiti.fr>,
	<akpm@linux-foundation.org>, <atishp@rivosinc.com>,
	<thunder.leizhen@huawei.com>, <horms@kernel.org>
Subject: Re: [PATCH -next v5 1/2] riscv: kdump: Implement crashkernel=X,[high,low]
Date: Sat, 1 Jul 2023 17:51:26 +0800	[thread overview]
Message-ID: <5c80666c-e6e2-8fa6-50b6-89536315925e@huawei.com> (raw)
In-Reply-To: <852b8777-3c6e-f76b-0413-1c66629f33cd@huawei.com>

On 2023/6/15 17:49, chenjiahao (C) wrote:
>
> 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@offset\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

I have sent v6 patches, implementing the logic above. That fixes the 
retrying

logic and should be aligned with Arm64 code.

Please let me know if there is any problem remains.

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
>>>
>
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec