Re: [PATCH v18 02/17] x86/setup: Move xen_pv_domain() check and insert_resource() to setup_arch()

["Leizhen (ThunderTown)" <thunder.leizhen@huawei.com>]

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@huawei.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...

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");
> 
> 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.

> 
> 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.
>