On 2023/6/4 11:50, Baoquan He wrote:
Hi Jiahao,
On 05/11/23 at 04:51pm, Chen Jiahao wrote:
......
@@ -1300,14 +1325,34 @@ static void __init reserve_crashkernel(void)
return;
}
- ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
+ ret = parse_crashkernel(cmdline, memblock_phys_mem_size(),
&crash_size, &crash_base);
- if (ret || !crash_size)
+ if (ret == -ENOENT) {
+ /* Fallback to crashkernel=X,[high,low] */
+ ret = parse_crashkernel_high(cmdline, 0, &crash_size, &crash_base);
+ if (ret || !crash_size)
+ return;
+
+ /*
+ * crashkernel=Y,low is valid only when crashkernel=X,high
+ * is passed.
+ */
+ ret = parse_crashkernel_low(cmdline, 0, &crash_low_size, &crash_base);
+ if (ret == -ENOENT)
+ crash_low_size = DEFAULT_CRASH_KERNEL_LOW_SIZE;
+ else if (ret)
+ return;
+
+ search_end = memblock_end_of_DRAM();
+ } else if (ret || !crash_size) {
+ /* Invalid argument value specified */
return;
+ }
The parsing part looks great, while you didn't mark if it's specified
high reservation, please see later comment why it's needed.
crash_size = PAGE_ALIGN(crash_size);
if (crash_base) {
+ fixed_base = true;
search_start = crash_base;
search_end = crash_base + crash_size;
}
@@ -1320,17 +1365,31 @@ static void __init reserve_crashkernel(void)
* swiotlb can work on the crash kernel.
*/
crash_base = memblock_phys_alloc_range(crash_size, PMD_SIZE,
- search_start,
- min(search_end, (unsigned long) SZ_4G));
+ search_start, search_end);
If it's a specified high reservation, you have
search_start = memblock_start_of_DRAM();
search_end = memblock_end_of_DRAM();
Then it attempts to search top down first time here.
if (crash_base == 0) {
- /* Try again without restricting region to 32bit addressible memory */
+ if (fixed_base) {
+ pr_warn("crashkernel: allocating failed with given size@offset\n");
+ return;
+ }
+ search_end = memblock_end_of_DRAM();
+
+ /* Try again above the region of 32bit addressible memory */
crash_base = memblock_phys_alloc_range(crash_size, PMD_SIZE,
- search_start, search_end);
+ search_start, search_end);
If crashkernel=,high case, the first attempt failed, here it assigns
search_end with memblock_end_of_DRAM(). It's the exactly the same
attempt, why is that needed? Why don't you use a local variable 'high'
to mark the crashkernel=,hig, then judge when deciding how to adjsut the
reservation range.
Do I misunderstand the code?
Thanks
Baoquan
You are right. Here I use search_end = memblock_end_of_DRAM() for the
first attempt on "crashkernel=,high" case, but it will not distinct from
other cases if the first attempt fails.
I have read your latest refactor on Arm64, introducing the "high" flag
is a good choice, the logic gets more straightforward when handling
crashkernel=,high case and retrying.
Following that logic, here introducing and set "high" flag when parsing
cmdline, when the first attempt failed:
if fixed_base:
failed and return;
if set high:
search_start = memblock_start_of_DRAM();
search_end = (unsigned long)dma32_phys_limit;
else:
search_start = (unsigned long)dma32_phys_limit;
search_end = memblock_end_of_DRAM();
second attempt with new {search_start, search_end}
...
This should handle "crashkernel=,high" case correctly and avoid cross
4G reservation.
Is that logic correct, or is any other problem missed?
Thanks,
Jiahao
if (crash_base == 0) {
pr_warn("crashkernel: couldn't allocate %lldKB\n",
crash_size >> 10);
return;
}
+
+ if (!crash_low_size)
+ crash_low_size = DEFAULT_CRASH_KERNEL_LOW_SIZE;
+ }
+
+ if ((crash_base > dma32_phys_limit - crash_low_size) &&
+ crash_low_size && reserve_crashkernel_low(crash_low_size)) {
+ memblock_phys_free(crash_base, crash_size);
+ return;
}
pr_info("crashkernel: reserved 0x%016llx - 0x%016llx (%lld MB)\n",
--
2.31.1
