From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id ACEE7C76196 for ; Mon, 10 Apr 2023 10:05:00 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229557AbjDJKE6 (ORCPT ); Mon, 10 Apr 2023 06:04:58 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49988 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229612AbjDJKEt (ORCPT ); Mon, 10 Apr 2023 06:04:49 -0400 Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 844A9212D; Mon, 10 Apr 2023 03:04:48 -0700 (PDT) Received: from dggpemm500016.china.huawei.com (unknown [172.30.72.54]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4Pw4HZ1ymSz17Rtt; Mon, 10 Apr 2023 18:01:14 +0800 (CST) Received: from [10.67.108.26] (10.67.108.26) by dggpemm500016.china.huawei.com (7.185.36.25) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.23; Mon, 10 Apr 2023 17:52:37 +0800 Message-ID: <20dd1890-273b-3a5b-7a4e-6feaf722cf8a@huawei.com> Date: Mon, 10 Apr 2023 17:52:37 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 Subject: Re: [PATCH -next v3 1/2] riscv: kdump: Implement crashkernel=X,[high,low] Content-Language: en-US To: "Leizhen (ThunderTown)" , Simon Horman CC: , , , , , , , , , , , , , References: <20230406220206.3067006-1-chenjiahao16@huawei.com> <20230406220206.3067006-2-chenjiahao16@huawei.com> From: "chenjiahao (C)" In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.108.26] X-ClientProxiedBy: dggems705-chm.china.huawei.com (10.3.19.182) To dggpemm500016.china.huawei.com (7.185.36.25) X-CFilter-Loop: Reflected Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2023/4/8 10:00, Leizhen (ThunderTown) wrote: > > On 2023/4/7 20:58, Leizhen (ThunderTown) wrote: >> >> On 2023/4/7 20:03, Simon Horman wrote: >>> On Fri, Apr 07, 2023 at 06:02:05AM +0800, Chen Jiahao wrote: >>>> On riscv, the current crash kernel allocation logic is trying to >>>> allocate within 32bit addressible memory region by default, if >>>> failed, try to allocate without 4G restriction. >>>> >>>> In need of saving DMA zone memory while allocating a relatively large >>>> crash kernel region, allocating the reserved memory top down in >>>> high memory, without overlapping the DMA zone, is a mature solution. >>>> Here introduce the parameter option crashkernel=X,[high,low]. >>>> >>>> One can reserve the crash kernel from high memory above DMA zone range >>>> by explicitly passing "crashkernel=X,high"; or reserve a memory range >>>> below 4G with "crashkernel=X,low". >>>> >>>> Signed-off-by: Chen Jiahao >>> ... >>> >>>> @@ -1180,14 +1206,37 @@ 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) { >>>> + /* >>>> + * crashkernel=X,[high,low] can be specified or not, but >>>> + * invalid value is not allowed. >>> nit: Perhaps something like this would be easier to correlate with the >>> code that follows: >>> >>> /* Fallback to crashkernel=X,[high,low] */ >> The description "crashkernel=X,[high,low] can be specified or not" is not >> correct, because crashkernel=X,high must be specified when walking into this >> branch. So use Simon's comments or copy arm64's comments(it's written for >> parse_crashkernel_low()). > I rethink it a little bit, if it's relative to crashkernel=X[@offset], > that's also true. > > Reviewed-by: Zhen Lei Sure, The commit should not be ambiguous like this, Simon's comment above is a better option. >>> >>>> + */ >>>> + 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 and high memory is reserved successful. >>> nit: s/successful/successfully/ >> Seems like the whole "and high memory is reserved successful" needs to be deleted. >> Only the dependency between the two boot options should be described here, >> regardless of whether their memory is successfully allocated. The comment here is imprecise, since there is absolutely no check whether the allocation is successful before "parse_crashkernel_low" >> >>>> + */ >>>> + 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_start = search_low_max; >>>> + } else if (ret || !crash_size) { >>>> + /* Invalid argument value specified */ >>>> return; >>>> + } >>> ... >>> . >>> BR, Jiahao