Re: [PATCH -next v5 1/2] riscv: kdump: Implement crashkernel=X,[high,low]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 2023/6/15 17:49, chenjiahao (C) wrote:

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

I have sent v6 patches, implementing the logic above. That fixes the retrying

logic and should be aligned with Arm64 code.


Please let me know if there is any problem remains.


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


_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux