Messages in this thread | | | Subject | Re: [PATCH v18 02/17] x86/setup: Move xen_pv_domain() check and insert_resource() to setup_arch() | From | "Leizhen (ThunderTown)" <> | Date | Sat, 25 Dec 2021 18:16:55 +0800 |
| |
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
| |