On 2021/12/25 9:53, Leizhen (ThunderTown) wrote:
>>> 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.
Hi Borislav and all:
Merry Christmas!
I have a new idea now. It helps us get around all the arguments and
minimizes changes to the x86 (also to arm64).
Previously, Chen Zhou and I tried to share the entire function
reserve_crashkernel(), which led to the following series of problems:
1. reserve_crashkernel() is also defined on other architectures, so we should
add build option ARCH_WANT_RESERVE_CRASH_KERNEL to avoid conflicts.
2. Move xen_pv_domain() check out of reserve_crashkernel().
3. Move insert_resource() out of reserve_crashkernel()
Others:
4. start = memblock_phys_alloc_range(crash_size, SZ_1M, crash_base,
crash_base + crash_size);
Change SZ_1M to CRASH_ALIGN, or keep it no change.
The current conclusion is no change. But I think adding a new macro
CRASH_FIXED_ALIGN is also a way. 2M alignment allows page tables to
use block mappings for most architectures.
5. if (crash_base >= (1ULL << 32) && reserve_crashkernel_low())
Change (1ULL << 32) to CRASH_ADDR_LOW_MAX, or keep it no change.
I reanalyzed it, and this doesn't need to be changed.
So for 1-3，why not add a new function reserve_crashkernel_mem() and rename
reserve_crashkernel_low() to reserve_crashkernel_mem_low().
On x86:
static void __init reserve_crashkernel(void)
{
//Parse all "crashkernel=" configurations in priority order until
//a valid combination is found. Or return upon failure.
if (xen_pv_domain()) {
pr_info("Ignoring crashkernel for a Xen PV domain\n");
return;
}
//Call reserve_crashkernel_mem() to reserve crashkernel memory, it will
//call reserve_crashkernel_mem_low() if needed.
if (crashk_low_res.end)
insert_resource(&iomem_resource, &crashk_low_res);
insert_resource(&iomem_resource, &crashk_res);
}
On arm64:
static void __init reserve_crashkernel(void)
{
//Parse all "crashkernel=" configurations in priority order until
//a valid combination is found. Or return upon failure.
//Call reserve_crashkernel_mem() to reserve crashkernel memory, it will
//call reserve_crashkernel_mem_low() if needed.
}
1. reserve_crashkernel() is still static, so that there is no
need to add ARCH_WANT_RESERVE_CRASH_KERNEL.
2. The xen_pv_domain() check have not been affected in any way.
Hi Borislav:
As you mentioned, this check may also be needed on arm64. But it may be
better not to add it until the problem is actually triggered on arm64.
3. insert_resource() is not moved outside reserve_crashkernel() on x86.
Hi Borislav:
Currently, I haven't figured out why request_resource() can't be replaced
with insert_resource() on arm64. But I have a hunch that the kexec tool may
be involved. The cost of modification on arm64 is definitely higher than that
on x86. Other architectures that want to use reserve_crashkernel_mem() may
also face the same problem. So it's probably better that function
reserve_crashkernel_mem() doesn't invoke insert_resource().
I guess you have a long Christmas holiday. So I'm going to send the next version
without waiting for your response.
>> 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.
>
--
Regards,
Zhen Lei
