On 2021/12/25 9:53, Leizhen (ThunderTown) wrote:

quoted

quoted

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.

quoted

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