Date:   Sat, 25 Dec 2021 09:53:31 +0800
From:   "Leizhen (ThunderTown)" <thunder.leizhen@...wei.com>
To:     Borislav Petkov <bp@...en8.de>
CC:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, <x86@...nel.org>,
        "H . Peter Anvin" <hpa@...or.com>, <linux-kernel@...r.kernel.org>,
        Dave Young <dyoung@...hat.com>, Baoquan He <bhe@...hat.com>,
        Vivek Goyal <vgoyal@...hat.com>,
        Eric Biederman <ebiederm@...ssion.com>,
        <kexec@...ts.infradead.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        <devicetree@...r.kernel.org>, "Jonathan Corbet" <corbet@....net>,
        <linux-doc@...r.kernel.org>, Randy Dunlap <rdunlap@...radead.org>,
        Feng Zhou <zhoufeng.zf@...edance.com>,
        Kefeng Wang <wangkefeng.wang@...wei.com>,
        Chen Zhou <dingguo.cz@...group.com>,
        "John Donnelly" <John.p.donnelly@...cle.com>
Subject: Re: [PATCH v18 02/17] x86/setup: Move xen_pv_domain() check and
 insert_resource() to setup_arch()



On 2021/12/24 14:36, Leizhen (ThunderTown) wrote:
> 
> 
> On 2021/12/24 1:26, Borislav Petkov wrote:
>> On Wed, Dec 22, 2021 at 09:08:05PM +0800, Zhen Lei wrote:
>>> From: Chen Zhou <chenzhou10@...wei.com>
>>>
>>> We will make the functions reserve_crashkernel() as generic, the
>>> xen_pv_domain() check in reserve_crashkernel() is relevant only to
>>> x86,
>>
>> Why is that so? Is Xen-PV x86-only?
>>
>>> the same as insert_resource() in reserve_crashkernel[_low]().
>>
>> Why?
>>
>> Looking at
>>
>>   0212f9159694 ("x86: Add Crash kernel low reservation")
>>
>> it *surprisingly* explains why that resources thing is being added:
>>
>>     We need to add another range in /proc/iomem like "Crash kernel low",
>>     so kexec-tools could find that info and append to kdump kernel
>>     command line.
>>
>> Then,
>>
>>   157752d84f5d ("kexec: use Crash kernel for Crash kernel low")
>>
>> renamed it because, as it states, kexec-tools was taught to handle
>> multiple resources of the same name.
>>
>> So why does kexec-tools on arm *not* need those iomem resources? How
>> does it parse the ranges there? Questions over questions...

It's a good question worth figuring out. I'm going to dig into this.
I admire your rigorous style and sharp vision.

> 
> https://lkml.org/lkml/2019/4/4/1758
> 
> Chen Zhou has explained before, see below. I'll analyze why x86 and arm64 need
> to process iomem resources at different times.
> 
>  < This very reminds what x86 does. Any chance some of the code can be reused
>  < rather than duplicated?
> As i said in the comment, i transport reserve_crashkernel_low() from x86_64. There are minor
> differences. In arm64, we don't need to do insert_resource(), we do request_resource()
> in request_standard_resources() later.
> 
>>
>> So last time I told you to sit down and take your time with this cleanup.
>> >From reading this here, it doesn't look like it. Rather, it looks like
>> hastily done in a hurry and hurrying stuff doesn't help you one bit - it
>> actually makes it worse.
>>
>> Your commit messages need to explain *why* a change is being done and
>> why is that ok. This one doesn't.
> 
> OK, I'll do this in follow-up patches.
> 
>>
>>> @@ -1120,7 +1109,17 @@ void __init setup_arch(char **cmdline_p)
>>>  	 * Reserve memory for crash kernel after SRAT is parsed so that it
>>>  	 * won't consume hotpluggable memory.
>>>  	 */
>>> -	reserve_crashkernel();
>>> +#ifdef CONFIG_KEXEC_CORE
>>> +	if (xen_pv_domain())
>>> +		pr_info("Ignoring crashkernel for a Xen PV domain\n");

Right, these two lines of code do not need to be moved. xen_pv_domain() is
a friendly macro function.

>>
>> This is wrong - the check is currently being done inside
>> reserve_crashkernel(), *after* it has parsed a crashkernel= cmdline
>> correctly - and not before.
>>
>> Your change would print on Xen PV, regardless of whether it has received
>> crashkernel= on the cmdline or not.
> 
> Yes, you're right. There are changes in code logic, but the print doesn't
> seem to cause any misunderstanding.
> 
>>
>> This is exactly why I say that making those functions generic and shared
>> might not be such a good idea, after all, because then you'd have to
>> sprinkle around arch-specific stuff.
> 
> Yes, I'm thinking about that too. Perhaps they are not suitable for full
> code sharing, but it looks like there's some code that can be shared.
> For example, the function parse_crashkernel_in_order() that I extracted
> based on your suggestion, it could also be parse_crashkernel_high_low().
> Or the function reserve_crashkernel_low().
> 
> There are two ways to reserve memory above 4G:
> 1. Use crashkernel=X,high, with or without crashkernel=X,low
> 2. Use crashkernel=X,[offset], but try low memory first. If failed, then
>    try high memory, and retry at least 256M low memory.
> 
> I plan to only implement 2 in the next version so that there can be fewer
> changes. Then implement 1 after 2 is applied.

I tried it yesterday and it didn't work. I still have to deal with the
problem of adjusting insert_resource().

How about I isolate some cleanup patches first? Strive for them to be
merged into v5.17. This way, we can focus on the core changes in the
next version. And I can also save some repetitive rebase workload.

> 
>>
>> One of the ways how to address this particular case here would be:
>>
>> 1. Add a x86-specific wrapper around parse_crashkernel() which does
>> all the parsing. When that wrapper finishes, you should have parsed
>> everything that has crashkernel= on the cmdline.
>>
>> 2. At the end of that wrapper, you do arch-specific checks and setup
>> like the xen_pv_domain() one.
>>
>> 3. Now, you do reserve_crashkernel(), if those checks pass.
>>
>> The question is, whether the flow on arm64 can do the same. Probably but
>> it needs careful auditing.
>>

Powered by blists - more mailing lists