[PATCH v19 00/13] support reserving crashkernel above 4G on arm64 kdump

[PATCH v19 00/13] support reserving crashkernel above 4G on arm64 kdump

[Zhen Lei]

There are following issues in arm64 kdump:
1. We use crashkernel=X to reserve crashkernel below 4G, which
will fail when there is no enough low memory.
2. If reserving crashkernel above 4G, in this case, crash dump
kernel will boot failure because there is no low memory available
for allocation.

To solve these issues, change the behavior of crashkernel=X.
crashkernel=X tries low allocation in DMA zone and fall back to high
allocation if it fails.

We can also use "crashkernel=X,high" to select a high region above
DMA zone, which also tries to allocate at least 256M low memory in
DMA zone automatically and "crashkernel=Y,low" can be used to allocate
specified size low memory.

When reserving crashkernel in high memory, some low memory is reserved
for crash dump kernel devices. So there may be two regions reserved for
crash dump kernel.
In order to distinct from the high region and make no effect to the use
of existing kexec-tools, rename the low region as "Crash kernel (low)",
and pass the low region by reusing DT property
"linux,usable-memory-range". We made the low memory region as the last
range of "linux,usable-memory-range" to keep compatibility with existing
user-space and older kdump kernels.

Besides, we need to modify kexec-tools:
arm64: support more than one crash kernel regions(see [1])

Another update is document about DT property 'linux,usable-memory-range':
schemas: update 'linux,usable-memory-range' node schema(see [2])

Changes since [v17]: v17 --> v19
1. Patch 0001-0004
   Introduce generic parse_crashkernel_high_low() to bring the parsing of
   "crashkernel=X,high" and the parsing of "crashkernel=X,low" together,
   then use it instead of the call to parse_crashkernel_{high|low}(). Two
   confusing parameters of parse_crashkernel_{high|low}() are deleted.

   I previously sent these four patches separately:
   [1] https://lkml.org/lkml/2021/12/25/40
2. Patch 0005-0009
   Introduce generic reserve_crashkernel_mem[_low](), the implementation of
   these two functions is based on function reserve_crashkernel[_low]() in
   arch/x86/kernel/setup.c. There is no functional change for x86.
   1) The check position of xen_pv_domain() does not change.
   2) Still 1M alignment for crash kernel fixed region, when 'base' is specified.

   To avoid compilation problems on other architectures: patch 0004 moves
   the definition of global variable crashk[_low]_res from kexec_core.c to
   crash_core.c, and provide default definitions for all macros involved, a
   particular platform can redefine these macros to override the default
   values.
3. 0010, only one line of comment was changed.
4. 0011
   1) crashk_low_res may also a valid reserved memory, should be checked
      in crash_is_nosave(), see arch/arm64/kernel/machine_kexec.
   2) Drop memblock_mark_nomap() for crashk_low_res, because of:
      2687275a5843 arm64: Force NO_BLOCK_MAPPINGS if crashkernel reservation is required
   3) Also call kmemleak_ignore_phys() for crashk_low_res, because of:
      85f58eb18898 arm64: kdump: Skip kmemleak scan reserved memory for kdump
5. 0012, slightly rebased, because the following patch is applied in advance. 
   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/commit/?h=dt/linus&id=8347b41748c3019157312fbe7f8a6792ae396eb7
6. 0013, no change.

Others:
1. Discard add ARCH_WANT_RESERVE_CRASH_KERNEL
2. When allocating crash low memory, the start address still starts from 0.
   low_base = memblock_phys_alloc_range(low_size, CRASH_ALIGN, 0, CRASH_ADDR_LOW_MAX);
3. Discard change (1ULL << 32) to CRASH_ADDR_LOW_MAX.
4. Ensure the check position of xen_pv_domain() have no change.
5. Except patch 0010 and 0012, all "Tested-by", "Reviewed-by", "Acked-by" are removed.
6. Update description.



Changes since [v16]
- Because no functional changes in this version, so add
  "Tested-by: Dave Kleikamp <dave.kleikamp@oracle.com>" for patch 1-9
- Add "Reviewed-by: Rob Herring <robh@kernel.org>" for patch 8
- Update patch 9 based on the review comments of Rob Herring
- As Catalin Marinas's suggestion, merge the implementation of
  ARCH_WANT_RESERVE_CRASH_KERNEL into patch 5. Ensure that the
  contents of X86 and ARM64 do not overlap, and reduce unnecessary
  temporary differences.

Changes since [v15]
-  Aggregate the processing of "linux,usable-memory-range" into one function.
   Only patch 9-10 have been updated.

Changes since [v14]
- Recovering the requirement that the CrashKernel memory regions on X86
  only requires 1 MiB alignment.
- Combine patches 5 and 6 in v14 into one. The compilation warning fixed
  by patch 6 was introduced by patch 5 in v14.
- As with crashk_res, crashk_low_res is also processed by
  crash_exclude_mem_range() in patch 7.
- Due to commit b261dba2fdb2 ("arm64: kdump: Remove custom linux,usable-memory-range handling")
  has removed the architecture-specific code, extend the property "linux,usable-memory-range"
  in the platform-agnostic FDT core code. See patch 9.
- Discard the x86 description update in the document, because the description
  has been updated by commit b1f4c363666c ("Documentation: kdump: update kdump guide").
- Change "arm64" to "ARM64" in Doc.


Changes since [v13]
- Rebased on top of 5.11-rc5.
- Introduce config CONFIG_ARCH_WANT_RESERVE_CRASH_KERNEL.
Since reserve_crashkernel[_low]() implementations are quite similar on
other architectures, so have CONFIG_ARCH_WANT_RESERVE_CRASH_KERNEL in
arch/Kconfig and select this by X86 and ARM64.
- Some minor cleanup.

Changes since [v12]
- Rebased on top of 5.10-rc1.
- Keep CRASH_ALIGN as 16M suggested by Dave.
- Drop patch "kdump: add threshold for the required memory".
- Add Tested-by from John.

Changes since [v11]
- Rebased on top of 5.9-rc4.
- Make the function reserve_crashkernel() of x86 generic.
Suggested by Catalin, make the function reserve_crashkernel() of x86 generic
and arm64 use the generic version to reimplement crashkernel=X.

Changes since [v10]
- Reimplement crashkernel=X suggested by Catalin, Many thanks to Catalin.

Changes since [v9]
- Patch 1 add Acked-by from Dave.
- Update patch 5 according to Dave's comments.
- Update chosen schema.

Changes since [v8]
- Reuse DT property "linux,usable-memory-range".
Suggested by Rob, reuse DT property "linux,usable-memory-range" to pass the low
memory region.
- Fix kdump broken with ZONE_DMA reintroduced.
- Update chosen schema.

Changes since [v7]
- Move x86 CRASH_ALIGN to 2M
Suggested by Dave and do some test, move x86 CRASH_ALIGN to 2M.
- Update Documentation/devicetree/bindings/chosen.txt.
Add corresponding documentation to Documentation/devicetree/bindings/chosen.txt
suggested by Arnd.
- Add Tested-by from Jhon and pk.

Changes since [v6]
- Fix build errors reported by kbuild test robot.

Changes since [v5]
- Move reserve_crashkernel_low() into kernel/crash_core.c.
- Delete crashkernel=X,high.
- Modify crashkernel=X,low.
If crashkernel=X,low is specified simultaneously, reserve spcified size low
memory for crash kdump kernel devices firstly and then reserve memory above 4G.
In addition, rename crashk_low_res as "Crash kernel (low)" for arm64, and then
pass to crash dump kernel by DT property "linux,low-memory-range".
- Update Documentation/admin-guide/kdump/kdump.rst.

Changes since [v4]
- Reimplement memblock_cap_memory_ranges for multiple ranges by Mike.

Changes since [v3]
- Add memblock_cap_memory_ranges back for multiple ranges.
- Fix some compiling warnings.

Changes since [v2]
- Split patch "arm64: kdump: support reserving crashkernel above 4G" as
two. Put "move reserve_crashkernel_low() into kexec_core.c" in a separate
patch.

Changes since [v1]:
- Move common reserve_crashkernel_low() code into kernel/kexec_core.c.
- Remove memblock_cap_memory_ranges() i added in v1 and implement that
in fdt_enforce_memory_region().
There are at most two crash kernel regions, for two crash kernel regions
case, we cap the memory range [min(regs[*].start), max(regs[*].end)]
and then remove the memory range in the middle.

[1]: http://lists.infradead.org/pipermail/kexec/2020-June/020737.html
[2]: https://github.com/robherring/dt-schema/pull/19 
[v1]: https://lkml.org/lkml/2019/4/2/1174
[v2]: https://lkml.org/lkml/2019/4/9/86
[v3]: https://lkml.org/lkml/2019/4/9/306
[v4]: https://lkml.org/lkml/2019/4/15/273
[v5]: https://lkml.org/lkml/2019/5/6/1360
[v6]: https://lkml.org/lkml/2019/8/30/142
[v7]: https://lkml.org/lkml/2019/12/23/411
[v8]: https://lkml.org/lkml/2020/5/21/213
[v9]: https://lkml.org/lkml/2020/6/28/73
[v10]: https://lkml.org/lkml/2020/7/2/1443
[v11]: https://lkml.org/lkml/2020/8/1/150
[v12]: https://lkml.org/lkml/2020/9/7/1037
[v13]: https://lkml.org/lkml/2020/10/31/34
[v14]: https://lkml.org/lkml/2021/1/30/53
[v15]: https://lkml.org/lkml/2021/10/19/1405
[v16]: https://lkml.org/lkml/2021/11/23/435
[v17]: https://lkml.org/lkml/2021/12/10/38
[v18]: https://lkml.org/lkml/2021/12/22/424


Chen Zhou (6):
  kexec: move crashk[_low]_res to crash_core module
  x86/setup: Move CRASH[_BASE]_ALIGN and CRASH_ADDR_{LOW|HIGH}_MAX to
    asm/kexec.h
  arm64: kdump: introduce some macros for crash kernel reservation
  arm64: kdump: reimplement crashkernel=X
  of: fdt: Add memory for devices by DT property
    "linux,usable-memory-range"
  kdump: update Documentation about crashkernel

Zhen Lei (7):
  kdump: add helper parse_crashkernel_high_low()
  x86/setup: Use parse_crashkernel_high_low() to simplify code
  kdump: make parse_crashkernel_{high|low}() static
  kdump: reduce unnecessary parameters of parse_crashkernel_{high|low}()
  x86/setup: Add and use CRASH_BASE_ALIGN
  kdump: Add helper reserve_crashkernel_mem[_low]()
  x86/setup: Use generic reserve_crashkernel_mem[_low]()

 Documentation/admin-guide/kdump/kdump.rst     |  11 +-
 .../admin-guide/kernel-parameters.txt         |  11 +-
 arch/arm64/include/asm/kexec.h                |   6 +
 arch/arm64/kernel/machine_kexec.c             |   5 +-
 arch/arm64/kernel/machine_kexec_file.c        |  12 +-
 arch/arm64/kernel/setup.c                     |  13 +-
 arch/arm64/mm/init.c                          |  41 ++--
 arch/x86/include/asm/kexec.h                  |  27 +++
 arch/x86/kernel/setup.c                       | 128 +---------
 drivers/of/fdt.c                              |  33 ++-
 include/linux/crash_core.h                    |  17 +-
 include/linux/kexec.h                         |   4 -
 kernel/crash_core.c                           | 224 +++++++++++++++++-
 kernel/kexec_core.c                           |  17 --
 14 files changed, 350 insertions(+), 199 deletions(-)

-- 
2.25.1

[PATCH v19 01/13] kdump: add helper parse_crashkernel_high_low()

[Zhen Lei]

The bootup command line option crashkernel=Y,low is valid only when
crashkernel=X,high is specified. Putting their parsing into a separate
function makes the code logic clearer and easier to understand the strong
dependencies between them.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 include/linux/crash_core.h |  3 +++
 kernel/crash_core.c        | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index de62a722431e7db..2d3a64761d18998 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -83,5 +83,8 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,
 		unsigned long long *crash_size, unsigned long long *crash_base);
 int parse_crashkernel_low(char *cmdline, unsigned long long system_ram,
 		unsigned long long *crash_size, unsigned long long *crash_base);
+int __init parse_crashkernel_high_low(char *cmdline,
+				      unsigned long long *high_size,
+				      unsigned long long *low_size);
 
 #endif /* LINUX_CRASH_CORE_H */
diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index eb53f5ec62c900f..8966beaf7c4fd52 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -295,6 +295,41 @@ int __init parse_crashkernel_low(char *cmdline,
 				"crashkernel=", suffix_tbl[SUFFIX_LOW]);
 }
 
+/**
+ * parse_crashkernel_high_low - Parsing "crashkernel=X,high" and possible
+ *				"crashkernel=Y,low".
+ * @cmdline:	The bootup command line.
+ * @high_size:	Save the memory size specified by "crashkernel=X,high".
+ * @low_size:	Save the memory size specified by "crashkernel=Y,low" or "-1"
+ *		if it's not specified.
+ *
+ * Returns 0 on success, else a negative status code.
+ */
+int __init parse_crashkernel_high_low(char *cmdline,
+				      unsigned long long *high_size,
+				      unsigned long long *low_size)
+{
+	int ret;
+	unsigned long long base;
+
+	BUG_ON(!high_size || !low_size);
+
+	/* crashkernel=X,high */
+	ret = parse_crashkernel_high(cmdline, 0, high_size, &base);
+	if (ret)
+		return ret;
+
+	if (*high_size <= 0)
+		return -EINVAL;
+
+	/* crashkernel=Y,low */
+	ret = parse_crashkernel_low(cmdline, 0, low_size, &base);
+	if (ret)
+		*low_size = -1;
+
+	return 0;
+}
+
 Elf_Word *append_elf_note(Elf_Word *buf, char *name, unsigned int type,
 			  void *data, size_t data_len)
 {
-- 
2.25.1

[PATCH v19 02/13] x86/setup: Use parse_crashkernel_high_low() to simplify code

[Zhen Lei]

Use parse_crashkernel_high_low() to bring the parsing of
"crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they
are strongly dependent, make code logic clear and more readable.

Suggested-by: Borislav Petkov <bp@alien8.de>
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 arch/x86/kernel/setup.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 6a190c7f4d71b05..93d78aae1937db3 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -416,18 +416,16 @@ static void __init memblock_x86_reserve_range_setup_data(void)
 # define CRASH_ADDR_HIGH_MAX	SZ_64T
 #endif
 
-static int __init reserve_crashkernel_low(void)
+static int __init reserve_crashkernel_low(unsigned long long low_size)
 {
 #ifdef CONFIG_X86_64
-	unsigned long long base, low_base = 0, low_size = 0;
+	unsigned long long low_base = 0;
 	unsigned long low_mem_limit;
-	int ret;
 
 	low_mem_limit = min(memblock_phys_mem_size(), CRASH_ADDR_LOW_MAX);
 
-	/* crashkernel=Y,low */
-	ret = parse_crashkernel_low(boot_command_line, low_mem_limit, &low_size, &base);
-	if (ret) {
+	/* crashkernel=Y,low is not specified */
+	if ((long)low_size < 0) {
 		/*
 		 * two parts from kernel/dma/swiotlb.c:
 		 * -swiotlb size: user-specified with swiotlb= or default.
@@ -465,7 +463,7 @@ static int __init reserve_crashkernel_low(void)
 
 static void __init reserve_crashkernel(void)
 {
-	unsigned long long crash_size, crash_base, total_mem;
+	unsigned long long crash_size, crash_base, total_mem, low_size;
 	bool high = false;
 	int ret;
 
@@ -474,10 +472,9 @@ static void __init reserve_crashkernel(void)
 	/* crashkernel=XM */
 	ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base);
 	if (ret != 0 || crash_size <= 0) {
-		/* crashkernel=X,high */
-		ret = parse_crashkernel_high(boot_command_line, total_mem,
-					     &crash_size, &crash_base);
-		if (ret != 0 || crash_size <= 0)
+		/* crashkernel=X,high and possible crashkernel=Y,low */
+		ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size);
+		if (ret)
 			return;
 		high = true;
 	}
@@ -520,7 +517,7 @@ static void __init reserve_crashkernel(void)
 		}
 	}
 
-	if (crash_base >= (1ULL << 32) && reserve_crashkernel_low()) {
+	if (crash_base >= (1ULL << 32) && reserve_crashkernel_low(low_size)) {
 		memblock_phys_free(crash_base, crash_size);
 		return;
 	}
-- 
2.25.1

[PATCH v19 03/13] kdump: make parse_crashkernel_{high|low}() static

[Zhen Lei]

Make parse_crashkernel_{high|low}() static, they are only referenced by
parse_crashkernel_high_low() in the same file. The latter is recommended.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 include/linux/crash_core.h | 4 ----
 kernel/crash_core.c        | 4 ++--
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index 2d3a64761d18998..598fd55d83c169e 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -79,10 +79,6 @@ void final_note(Elf_Word *buf);
 
 int __init parse_crashkernel(char *cmdline, unsigned long long system_ram,
 		unsigned long long *crash_size, unsigned long long *crash_base);
-int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,
-		unsigned long long *crash_size, unsigned long long *crash_base);
-int parse_crashkernel_low(char *cmdline, unsigned long long system_ram,
-		unsigned long long *crash_size, unsigned long long *crash_base);
 int __init parse_crashkernel_high_low(char *cmdline,
 				      unsigned long long *high_size,
 				      unsigned long long *low_size);
diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index 8966beaf7c4fd52..3b9e01fc450b2a4 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -277,7 +277,7 @@ int __init parse_crashkernel(char *cmdline,
 					"crashkernel=", NULL);
 }
 
-int __init parse_crashkernel_high(char *cmdline,
+static int __init parse_crashkernel_high(char *cmdline,
 			     unsigned long long system_ram,
 			     unsigned long long *crash_size,
 			     unsigned long long *crash_base)
@@ -286,7 +286,7 @@ int __init parse_crashkernel_high(char *cmdline,
 				"crashkernel=", suffix_tbl[SUFFIX_HIGH]);
 }
 
-int __init parse_crashkernel_low(char *cmdline,
+static int __init parse_crashkernel_low(char *cmdline,
 			     unsigned long long system_ram,
 			     unsigned long long *crash_size,
 			     unsigned long long *crash_base)
-- 
2.25.1

[PATCH v19 04/13] kdump: reduce unnecessary parameters of parse_crashkernel_{high|low}()

[Zhen Lei]

Delete confusing parameters 'system_ram' and 'crash_base' of
parse_crashkernel_{high|low}(), they are only needed by the case of
"crashkernel=X@[offset]".

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 kernel/crash_core.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index 3b9e01fc450b2a4..b7d024eb464d0ae 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -278,20 +278,20 @@ int __init parse_crashkernel(char *cmdline,
 }
 
 static int __init parse_crashkernel_high(char *cmdline,
-			     unsigned long long system_ram,
-			     unsigned long long *crash_size,
-			     unsigned long long *crash_base)
+					 unsigned long long *crash_size)
 {
-	return __parse_crashkernel(cmdline, system_ram, crash_size, crash_base,
+	unsigned long long base;
+
+	return __parse_crashkernel(cmdline, 0, crash_size, &base,
 				"crashkernel=", suffix_tbl[SUFFIX_HIGH]);
 }
 
 static int __init parse_crashkernel_low(char *cmdline,
-			     unsigned long long system_ram,
-			     unsigned long long *crash_size,
-			     unsigned long long *crash_base)
+					unsigned long long *crash_size)
 {
-	return __parse_crashkernel(cmdline, system_ram, crash_size, crash_base,
+	unsigned long long base;
+
+	return __parse_crashkernel(cmdline, 0, crash_size, &base,
 				"crashkernel=", suffix_tbl[SUFFIX_LOW]);
 }
 
@@ -310,12 +310,11 @@ int __init parse_crashkernel_high_low(char *cmdline,
 				      unsigned long long *low_size)
 {
 	int ret;
-	unsigned long long base;
 
 	BUG_ON(!high_size || !low_size);
 
 	/* crashkernel=X,high */
-	ret = parse_crashkernel_high(cmdline, 0, high_size, &base);
+	ret = parse_crashkernel_high(cmdline, high_size);
 	if (ret)
 		return ret;
 
@@ -323,7 +322,7 @@ int __init parse_crashkernel_high_low(char *cmdline,
 		return -EINVAL;
 
 	/* crashkernel=Y,low */
-	ret = parse_crashkernel_low(cmdline, 0, low_size, &base);
+	ret = parse_crashkernel_low(cmdline, low_size);
 	if (ret)
 		*low_size = -1;
 
-- 
2.25.1

[PATCH v19 05/13] x86/setup: Add and use CRASH_BASE_ALIGN

[Zhen Lei]

Add macro CRASH_BASE_ALIGN to indicate the alignment for crash kernel
fixed region, in preparation for making partial implementation of
reserve_crashkernel[_low]() generic.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 arch/x86/kernel/setup.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 93d78aae1937db3..cb7f237a2ae0dfa 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -392,9 +392,12 @@ static void __init memblock_x86_reserve_range_setup_data(void)
 
 #ifdef CONFIG_KEXEC_CORE
 
-/* 16M alignment for crash kernel regions */
+/* alignment for crash kernel dynamic regions */
 #define CRASH_ALIGN		SZ_16M
 
+/* alignment for crash kernel fixed region */
+#define CRASH_BASE_ALIGN	SZ_1M
+
 /*
  * Keep the crash kernel below this limit.
  *
@@ -509,7 +512,7 @@ static void __init reserve_crashkernel(void)
 	} else {
 		unsigned long long start;
 
-		start = memblock_phys_alloc_range(crash_size, SZ_1M, crash_base,
+		start = memblock_phys_alloc_range(crash_size, CRASH_BASE_ALIGN, crash_base,
 						  crash_base + crash_size);
 		if (start != crash_base) {
 			pr_info("crashkernel reservation failed - memory is in use.\n");
-- 
2.25.1

[PATCH v19 06/13] kexec: move crashk[_low]_res to crash_core module

[Zhen Lei]

From: Chen Zhou <chenzhou10@huawei.com>

Move the definition and declaration of global variable crashk[_low]_res
from kexec module to crash_core module, in preparation of adding generic
reserve_crashkernel_mem[_low]() to crash_core.c, the latter refers to
variable crashk[_low]_res. Due to the config KEXEC automatically selects
CRASH_CORE, and the header crash_core.h is included by kexec.h, so there
is no functional change.

Signed-off-by: Chen Zhou <chenzhou10@huawei.com>
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 include/linux/crash_core.h |  4 ++++
 include/linux/kexec.h      |  4 ----
 kernel/crash_core.c        | 16 ++++++++++++++++
 kernel/kexec_core.c        | 17 -----------------
 4 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index 598fd55d83c169e..f5437c9c9411fce 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -73,6 +73,10 @@ extern unsigned char *vmcoreinfo_data;
 extern size_t vmcoreinfo_size;
 extern u32 *vmcoreinfo_note;
 
+/* Location of a reserved region to hold the crash kernel. */
+extern struct resource crashk_res;
+extern struct resource crashk_low_res;
+
 Elf_Word *append_elf_note(Elf_Word *buf, char *name, unsigned int type,
 			  void *data, size_t data_len);
 void final_note(Elf_Word *buf);
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 0c994ae37729e1e..47e784d66ea8645 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -350,10 +350,6 @@ extern int kexec_load_disabled;
 #define KEXEC_FILE_FLAGS	(KEXEC_FILE_UNLOAD | KEXEC_FILE_ON_CRASH | \
 				 KEXEC_FILE_NO_INITRAMFS)
 
-/* Location of a reserved region to hold the crash kernel.
- */
-extern struct resource crashk_res;
-extern struct resource crashk_low_res;
 extern note_buf_t __percpu *crash_notes;
 
 /* flag to track if kexec reboot is in progress */
diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index b7d024eb464d0ae..686d8a65e12a337 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -22,6 +22,22 @@ u32 *vmcoreinfo_note;
 /* trusted vmcoreinfo, e.g. we can make a copy in the crash memory */
 static unsigned char *vmcoreinfo_data_safecopy;
 
+/* Location of the reserved area for the crash kernel */
+struct resource crashk_res = {
+	.name  = "Crash kernel",
+	.start = 0,
+	.end   = 0,
+	.flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM,
+	.desc  = IORES_DESC_CRASH_KERNEL
+};
+struct resource crashk_low_res = {
+	.name  = "Crash kernel",
+	.start = 0,
+	.end   = 0,
+	.flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM,
+	.desc  = IORES_DESC_CRASH_KERNEL
+};
+
 /*
  * parsing the "crashkernel" commandline
  *
diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index 5a5d192a89ac307..1e0d4909bbb6b77 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -54,23 +54,6 @@ note_buf_t __percpu *crash_notes;
 /* Flag to indicate we are going to kexec a new kernel */
 bool kexec_in_progress = false;
 
-
-/* Location of the reserved area for the crash kernel */
-struct resource crashk_res = {
-	.name  = "Crash kernel",
-	.start = 0,
-	.end   = 0,
-	.flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM,
-	.desc  = IORES_DESC_CRASH_KERNEL
-};
-struct resource crashk_low_res = {
-	.name  = "Crash kernel",
-	.start = 0,
-	.end   = 0,
-	.flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM,
-	.desc  = IORES_DESC_CRASH_KERNEL
-};
-
 int kexec_should_crash(struct task_struct *p)
 {
 	/*
-- 
2.25.1

[PATCH v19 07/13] kdump: Add helper reserve_crashkernel_mem[_low]()

[Zhen Lei]

Add helper reserve_crashkernel_mem[_low]() to reserve high and low memory
for crash kernel. The implementation of these two functions is based on
function reserve_crashkernel[_low]() in arch/x86/kernel/setup.c. But the
following adaptations are made:
1. To avoid compilation problems on other architectures, provide default
   values for macro CRASH[_BASE]_ALIGN, CRASH_ADDR_{LOW|HIGH}_MAX, and add
   new macro CRASH_LOW_SIZE_MIN.
2. Only functions that reserve crash memory are extracted from
   reserve_crashkernel(), excluding functions such as parse_crashkernel()
   and insert_resource().
3. Change "return;" in reserve_crashkernel() to "return -ENOMEM;".
4. Change call reserve_crashkernel_low() to call
   reserve_crashkernel_mem_low().
5. Change CONFIG_X86_64 to CONFIG_64BIT.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 include/linux/crash_core.h |   6 ++
 kernel/crash_core.c        | 154 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 159 insertions(+), 1 deletion(-)

diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index f5437c9c9411fce..2e19632f8d45a60 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -86,5 +86,11 @@ int __init parse_crashkernel(char *cmdline, unsigned long long system_ram,
 int __init parse_crashkernel_high_low(char *cmdline,
 				      unsigned long long *high_size,
 				      unsigned long long *low_size);
+int __init reserve_crashkernel_mem_low(unsigned long long low_size);
+int __init reserve_crashkernel_mem(unsigned long long system_ram,
+				   unsigned long long crash_size,
+				   unsigned long long crash_base,
+				   unsigned long long low_size,
+				   bool high);
 
 #endif /* LINUX_CRASH_CORE_H */
diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index 686d8a65e12a337..4bd30098534a184 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -5,7 +5,9 @@
  */
 
 #include <linux/buildid.h>
-#include <linux/crash_core.h>
+#include <linux/kexec.h>
+#include <linux/memblock.h>
+#include <linux/swiotlb.h>
 #include <linux/utsname.h>
 #include <linux/vmalloc.h>
 
@@ -345,6 +347,156 @@ int __init parse_crashkernel_high_low(char *cmdline,
 	return 0;
 }
 
+/* alignment for crash kernel dynamic regions */
+#ifndef CRASH_ALIGN
+#define CRASH_ALIGN		SZ_2M
+#endif
+
+/* alignment for crash kernel fixed region */
+#ifndef CRASH_BASE_ALIGN
+#define CRASH_BASE_ALIGN	SZ_2M
+#endif
+
+/* upper bound for crash low memory */
+#ifndef CRASH_ADDR_LOW_MAX
+#ifdef CONFIG_PHYS_ADDR_T_64BIT
+#define CRASH_ADDR_LOW_MAX	SZ_4G
+#else
+#define CRASH_ADDR_LOW_MAX	MEMBLOCK_ALLOC_ACCESSIBLE
+#endif
+#endif
+
+/* upper bound for crash high memory */
+#ifndef CRASH_ADDR_HIGH_MAX
+#define CRASH_ADDR_HIGH_MAX	MEMBLOCK_ALLOC_ACCESSIBLE
+#endif
+
+#ifdef CONFIG_SWIOTLB
+/*
+ * two parts from kernel/dma/swiotlb.c:
+ * -swiotlb size: user-specified with swiotlb= or default.
+ *
+ * -swiotlb overflow buffer: now hardcoded to 32k. We round it
+ * to 8M for other buffers that may need to stay low too. Also
+ * make sure we allocate enough extra low memory so that we
+ * don't run out of DMA buffers for 32-bit devices.
+ */
+#define CRASH_LOW_SIZE_MIN	max(swiotlb_size_or_default() + (8UL << 20), 256UL << 20)
+#else
+#define CRASH_LOW_SIZE_MIN	(256UL << 20)
+#endif
+
+/**
+ * reserve_crashkernel_mem_low - Reserve crash kernel low memory.
+ *
+ * @low_size:	The memory size specified by "crashkernel=Y,low" or "-1"
+ *		if it's not specified.
+ *
+ * Returns 0 on success, else a negative status code.
+ */
+int __init reserve_crashkernel_mem_low(unsigned long long low_size)
+{
+#ifdef CONFIG_64BIT
+	unsigned long long low_base = 0;
+	unsigned long low_mem_limit;
+
+	low_mem_limit = min(memblock_phys_mem_size(), CRASH_ADDR_LOW_MAX);
+
+	/* crashkernel=Y,low is not specified */
+	if ((long)low_size < 0) {
+		low_size = CRASH_LOW_SIZE_MIN;
+	} else {
+		/* passed with crashkernel=0,low ? */
+		if (!low_size)
+			return 0;
+	}
+
+	low_base = memblock_phys_alloc_range(low_size, CRASH_ALIGN, 0, CRASH_ADDR_LOW_MAX);
+	if (!low_base) {
+		pr_err("Cannot reserve %ldMB crashkernel low memory, please try smaller size.\n",
+		       (unsigned long)(low_size >> 20));
+		return -ENOMEM;
+	}
+
+	pr_info("Reserving %ldMB of low memory at %ldMB for crashkernel (low RAM limit: %ldMB)\n",
+		(unsigned long)(low_size >> 20),
+		(unsigned long)(low_base >> 20),
+		(unsigned long)(low_mem_limit >> 20));
+
+	crashk_low_res.start = low_base;
+	crashk_low_res.end   = low_base + low_size - 1;
+#endif
+	return 0;
+}
+
+/**
+ * reserve_crashkernel_mem - Reserve crash kernel memory.
+ *
+ * @system_ram: Total system memory size.
+ * @crash_size: The memory size specified by "crashkernel=X[@offset]" or
+ *		"crashkernel=X,high".
+ * @crash_base: The base address specified by "crashkernel=X@offset"
+ * @low_size:	The memory size specified by "crashkernel=Y,low" or "-1"
+ *		if it's not specified.
+ * @high:	Whether "crashkernel=X,high" is specified.
+ *
+ * Returns 0 on success, else a negative status code.
+ */
+int __init reserve_crashkernel_mem(unsigned long long system_ram,
+				   unsigned long long crash_size,
+				   unsigned long long crash_base,
+				   unsigned long long low_size,
+				   bool high)
+{
+	/* 0 means: find the address automatically */
+	if (!crash_base) {
+		/*
+		 * Set CRASH_ADDR_LOW_MAX upper bound for crash memory,
+		 * crashkernel=x,high reserves memory over 4G, also allocates
+		 * 256M extra low memory for DMA buffers and swiotlb.
+		 * But the extra memory is not required for all machines.
+		 * So try low memory first and fall back to high memory
+		 * unless "crashkernel=size[KMG],high" is specified.
+		 */
+		if (!high)
+			crash_base = memblock_phys_alloc_range(crash_size,
+						CRASH_ALIGN, CRASH_ALIGN,
+						CRASH_ADDR_LOW_MAX);
+		if (!crash_base)
+			crash_base = memblock_phys_alloc_range(crash_size,
+						CRASH_ALIGN, CRASH_ALIGN,
+						CRASH_ADDR_HIGH_MAX);
+		if (!crash_base) {
+			pr_info("crashkernel reservation failed - No suitable area found.\n");
+			return -ENOMEM;
+		}
+	} else {
+		unsigned long long start;
+
+		start = memblock_phys_alloc_range(crash_size, CRASH_BASE_ALIGN, crash_base,
+						  crash_base + crash_size);
+		if (start != crash_base) {
+			pr_info("crashkernel reservation failed - memory is in use.\n");
+			return -ENOMEM;
+		}
+	}
+
+	if (crash_base >= (1ULL << 32) && reserve_crashkernel_mem_low(low_size)) {
+		memblock_phys_free(crash_base, crash_size);
+		return -ENOMEM;
+	}
+
+	pr_info("Reserving %ldMB of memory at %ldMB for crashkernel (System RAM: %ldMB)\n",
+		(unsigned long)(crash_size >> 20),
+		(unsigned long)(crash_base >> 20),
+		(unsigned long)(system_ram >> 20));
+
+	crashk_res.start = crash_base;
+	crashk_res.end   = crash_base + crash_size - 1;
+
+	return 0;
+}
+
 Elf_Word *append_elf_note(Elf_Word *buf, char *name, unsigned int type,
 			  void *data, size_t data_len)
 {
-- 
2.25.1

[PATCH v19 08/13] x86/setup: Move CRASH[_BASE]_ALIGN and CRASH_ADDR_{LOW|HIGH}_MAX to asm/kexec.h

[Zhen Lei]

From: Chen Zhou <chenzhou10@huawei.com>

Move CRASH[_BASE]_ALIGN and CRASH_ADDR_{LOW|HIGH}_MAX to the arch-specific
header in preparation of using generic reserve_crashkernel_mem[_low]().

Signed-off-by: Chen Zhou <chenzhou10@huawei.com>
Co-developed-by: Zhen Lei <thunder.leizhen@huawei.com>
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 arch/x86/include/asm/kexec.h | 27 +++++++++++++++++++++++++++
 arch/x86/kernel/setup.c      | 27 ---------------------------
 2 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
index 11b7c06e2828c30..452c35ce3e3fc54 100644
--- a/arch/x86/include/asm/kexec.h
+++ b/arch/x86/include/asm/kexec.h
@@ -18,6 +18,33 @@
 
 # define KEXEC_CONTROL_CODE_MAX_SIZE	2048
 
+/* alignment for crash kernel dynamic regions */
+#define CRASH_ALIGN		SZ_16M
+
+/* alignment for crash kernel fixed region */
+#define CRASH_BASE_ALIGN	SZ_1M
+
+/*
+ * Keep the crash kernel below this limit.
+ *
+ * Earlier 32-bits kernels would limit the kernel to the low 512 MB range
+ * due to mapping restrictions.
+ *
+ * 64-bit kdump kernels need to be restricted to be under 64 TB, which is
+ * the upper limit of system RAM in 4-level paging mode. Since the kdump
+ * jump could be from 5-level paging to 4-level paging, the jump will fail if
+ * the kernel is put above 64 TB, and during the 1st kernel bootup there's
+ * no good way to detect the paging mode of the target kernel which will be
+ * loaded for dumping.
+ */
+#ifdef CONFIG_X86_32
+# define CRASH_ADDR_LOW_MAX	SZ_512M
+# define CRASH_ADDR_HIGH_MAX	SZ_512M
+#else
+# define CRASH_ADDR_LOW_MAX	SZ_4G
+# define CRASH_ADDR_HIGH_MAX	SZ_64T
+#endif
+
 #ifndef __ASSEMBLY__
 
 #include <linux/string.h>
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index cb7f237a2ae0dfa..22d63dbf5db0a58 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -392,33 +392,6 @@ static void __init memblock_x86_reserve_range_setup_data(void)
 
 #ifdef CONFIG_KEXEC_CORE
 
-/* alignment for crash kernel dynamic regions */
-#define CRASH_ALIGN		SZ_16M
-
-/* alignment for crash kernel fixed region */
-#define CRASH_BASE_ALIGN	SZ_1M
-
-/*
- * Keep the crash kernel below this limit.
- *
- * Earlier 32-bits kernels would limit the kernel to the low 512 MB range
- * due to mapping restrictions.
- *
- * 64-bit kdump kernels need to be restricted to be under 64 TB, which is
- * the upper limit of system RAM in 4-level paging mode. Since the kdump
- * jump could be from 5-level paging to 4-level paging, the jump will fail if
- * the kernel is put above 64 TB, and during the 1st kernel bootup there's
- * no good way to detect the paging mode of the target kernel which will be
- * loaded for dumping.
- */
-#ifdef CONFIG_X86_32
-# define CRASH_ADDR_LOW_MAX	SZ_512M
-# define CRASH_ADDR_HIGH_MAX	SZ_512M
-#else
-# define CRASH_ADDR_LOW_MAX	SZ_4G
-# define CRASH_ADDR_HIGH_MAX	SZ_64T
-#endif
-
 static int __init reserve_crashkernel_low(unsigned long long low_size)
 {
 #ifdef CONFIG_X86_64
-- 
2.25.1

[PATCH v19 09/13] x86/setup: Use generic reserve_crashkernel_mem[_low]()

[Zhen Lei]

Use generic reserve_crashkernel_mem[_low]() to replace arch-specific
reserve_crashkernel_low() and a partial implementation of
reserve_crashkernel(). The only difference is that
"insert_resource(&iomem_resource, &crashk_low_res);" is moved into
reserve_crashkernel(), no functional change.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 arch/x86/kernel/setup.c | 93 ++---------------------------------------
 1 file changed, 4 insertions(+), 89 deletions(-)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 22d63dbf5db0a58..ee2606b3b9da662 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -391,52 +391,6 @@ static void __init memblock_x86_reserve_range_setup_data(void)
  */
 
 #ifdef CONFIG_KEXEC_CORE
-
-static int __init reserve_crashkernel_low(unsigned long long low_size)
-{
-#ifdef CONFIG_X86_64
-	unsigned long long low_base = 0;
-	unsigned long low_mem_limit;
-
-	low_mem_limit = min(memblock_phys_mem_size(), CRASH_ADDR_LOW_MAX);
-
-	/* crashkernel=Y,low is not specified */
-	if ((long)low_size < 0) {
-		/*
-		 * two parts from kernel/dma/swiotlb.c:
-		 * -swiotlb size: user-specified with swiotlb= or default.
-		 *
-		 * -swiotlb overflow buffer: now hardcoded to 32k. We round it
-		 * to 8M for other buffers that may need to stay low too. Also
-		 * make sure we allocate enough extra low memory so that we
-		 * don't run out of DMA buffers for 32-bit devices.
-		 */
-		low_size = max(swiotlb_size_or_default() + (8UL << 20), 256UL << 20);
-	} else {
-		/* passed with crashkernel=0,low ? */
-		if (!low_size)
-			return 0;
-	}
-
-	low_base = memblock_phys_alloc_range(low_size, CRASH_ALIGN, 0, CRASH_ADDR_LOW_MAX);
-	if (!low_base) {
-		pr_err("Cannot reserve %ldMB crashkernel low memory, please try smaller size.\n",
-		       (unsigned long)(low_size >> 20));
-		return -ENOMEM;
-	}
-
-	pr_info("Reserving %ldMB of low memory at %ldMB for crashkernel (low RAM limit: %ldMB)\n",
-		(unsigned long)(low_size >> 20),
-		(unsigned long)(low_base >> 20),
-		(unsigned long)(low_mem_limit >> 20));
-
-	crashk_low_res.start = low_base;
-	crashk_low_res.end   = low_base + low_size - 1;
-	insert_resource(&iomem_resource, &crashk_low_res);
-#endif
-	return 0;
-}
-
 static void __init reserve_crashkernel(void)
 {
 	unsigned long long crash_size, crash_base, total_mem, low_size;
@@ -460,51 +414,12 @@ static void __init reserve_crashkernel(void)
 		return;
 	}
 
-	/* 0 means: find the address automatically */
-	if (!crash_base) {
-		/*
-		 * Set CRASH_ADDR_LOW_MAX upper bound for crash memory,
-		 * crashkernel=x,high reserves memory over 4G, also allocates
-		 * 256M extra low memory for DMA buffers and swiotlb.
-		 * But the extra memory is not required for all machines.
-		 * So try low memory first and fall back to high memory
-		 * unless "crashkernel=size[KMG],high" is specified.
-		 */
-		if (!high)
-			crash_base = memblock_phys_alloc_range(crash_size,
-						CRASH_ALIGN, CRASH_ALIGN,
-						CRASH_ADDR_LOW_MAX);
-		if (!crash_base)
-			crash_base = memblock_phys_alloc_range(crash_size,
-						CRASH_ALIGN, CRASH_ALIGN,
-						CRASH_ADDR_HIGH_MAX);
-		if (!crash_base) {
-			pr_info("crashkernel reservation failed - No suitable area found.\n");
-			return;
-		}
-	} else {
-		unsigned long long start;
-
-		start = memblock_phys_alloc_range(crash_size, CRASH_BASE_ALIGN, crash_base,
-						  crash_base + crash_size);
-		if (start != crash_base) {
-			pr_info("crashkernel reservation failed - memory is in use.\n");
-			return;
-		}
-	}
-
-	if (crash_base >= (1ULL << 32) && reserve_crashkernel_low(low_size)) {
-		memblock_phys_free(crash_base, crash_size);
+	ret = reserve_crashkernel_mem(total_mem, crash_size, crash_base, low_size, high);
+	if (ret)
 		return;
-	}
-
-	pr_info("Reserving %ldMB of memory at %ldMB for crashkernel (System RAM: %ldMB)\n",
-		(unsigned long)(crash_size >> 20),
-		(unsigned long)(crash_base >> 20),
-		(unsigned long)(total_mem >> 20));
 
-	crashk_res.start = crash_base;
-	crashk_res.end   = crash_base + crash_size - 1;
+	if (crashk_low_res.end > crashk_low_res.start)
+		insert_resource(&iomem_resource, &crashk_low_res);
 	insert_resource(&iomem_resource, &crashk_res);
 }
 #else
-- 
2.25.1

[PATCH v19 10/13] arm64: kdump: introduce some macros for crash kernel reservation

[Zhen Lei]

From: Chen Zhou <chenzhou10@huawei.com>

Introduce macro CRASH_ALIGN for alignment, macro CRASH_ADDR_LOW_MAX
for upper bound of low crash memory, macro CRASH_ADDR_HIGH_MAX for
upper bound of high crash memory, use macros instead.

Signed-off-by: Chen Zhou <chenzhou10@huawei.com>
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
Tested-by: John Donnelly <John.p.donnelly@oracle.com>
Tested-by: Dave Kleikamp <dave.kleikamp@oracle.com>
---
 arch/arm64/include/asm/kexec.h | 6 ++++++
 arch/arm64/mm/init.c           | 4 ++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
index 9839bfc163d7147..f019e78dede02dc 100644
--- a/arch/arm64/include/asm/kexec.h
+++ b/arch/arm64/include/asm/kexec.h
@@ -25,6 +25,12 @@
 
 #define KEXEC_ARCH KEXEC_ARCH_AARCH64
 
+/* alignment for crash kernel dynamic regions */
+#define CRASH_ALIGN		SZ_2M
+
+#define CRASH_ADDR_LOW_MAX	arm64_dma_phys_limit
+#define CRASH_ADDR_HIGH_MAX	MEMBLOCK_ALLOC_ACCESSIBLE
+
 #ifndef __ASSEMBLY__
 
 /**
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index a8834434af99ae0..be4595dc7459115 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -75,7 +75,7 @@ phys_addr_t arm64_dma_phys_limit __ro_after_init;
 static void __init reserve_crashkernel(void)
 {
 	unsigned long long crash_base, crash_size;
-	unsigned long long crash_max = arm64_dma_phys_limit;
+	unsigned long long crash_max = CRASH_ADDR_LOW_MAX;
 	int ret;
 
 	ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
@@ -91,7 +91,7 @@ static void __init reserve_crashkernel(void)
 		crash_max = crash_base + crash_size;
 
 	/* Current arm64 boot protocol requires 2MB alignment */
-	crash_base = memblock_phys_alloc_range(crash_size, SZ_2M,
+	crash_base = memblock_phys_alloc_range(crash_size, CRASH_ALIGN,
 					       crash_base, crash_max);
 	if (!crash_base) {
 		pr_warn("cannot allocate crashkernel (size:0x%llx)\n",
-- 
2.25.1

[PATCH v19 11/13] arm64: kdump: reimplement crashkernel=X

[Zhen Lei]

From: Chen Zhou <chenzhou10@huawei.com>

There are following issues in arm64 kdump:
1. We use crashkernel=X to reserve crashkernel below 4G, which
will fail when there is no enough low memory.
2. If reserving crashkernel above 4G, in this case, crash dump
kernel will boot failure because there is no low memory available
for allocation.

To solve these issues, change the behavior of crashkernel=X and
introduce crashkernel=X,[high,low]. crashkernel=X tries low allocation
in DMA zone, and fall back to high allocation if it fails.
We can also use "crashkernel=X,high" to select a region above DMA zone,
which also tries to allocate at least 256M in DMA zone automatically.
"crashkernel=Y,low" can be used to allocate specified size low memory.

Another minor change, there may be two regions reserved for crash
dump kernel, in order to distinct from the high region and make no
effect to the use of existing kexec-tools, rename the low region as
"Crash kernel (low)".

Signed-off-by: Chen Zhou <chenzhou10@huawei.com>
Co-developed-by: Zhen Lei <thunder.leizhen@huawei.com>
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 arch/arm64/kernel/machine_kexec.c      |  5 +++-
 arch/arm64/kernel/machine_kexec_file.c | 12 ++++++--
 arch/arm64/kernel/setup.c              | 13 +++++++-
 arch/arm64/mm/init.c                   | 41 ++++++++++----------------
 4 files changed, 42 insertions(+), 29 deletions(-)

diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c
index 6fb31c117ebe08c..6665bf31f6b6a19 100644
--- a/arch/arm64/kernel/machine_kexec.c
+++ b/arch/arm64/kernel/machine_kexec.c
@@ -327,7 +327,10 @@ bool crash_is_nosave(unsigned long pfn)
 
 	/* in reserved memory? */
 	addr = __pfn_to_phys(pfn);
-	if ((addr < crashk_res.start) || (crashk_res.end < addr))
+	if (((addr < crashk_res.start) || (crashk_res.end < addr)) && !crashk_low_res.end)
+		return false;
+
+	if ((addr < crashk_low_res.start) || (crashk_low_res.end < addr))
 		return false;
 
 	if (!kexec_crash_image)
diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
index 59c648d51848886..889951291cc0f9c 100644
--- a/arch/arm64/kernel/machine_kexec_file.c
+++ b/arch/arm64/kernel/machine_kexec_file.c
@@ -65,10 +65,18 @@ static int prepare_elf_headers(void **addr, unsigned long *sz)
 
 	/* Exclude crashkernel region */
 	ret = crash_exclude_mem_range(cmem, crashk_res.start, crashk_res.end);
+	if (ret)
+		goto out;
+
+	if (crashk_low_res.end) {
+		ret = crash_exclude_mem_range(cmem, crashk_low_res.start, crashk_low_res.end);
+		if (ret)
+			goto out;
+	}
 
-	if (!ret)
-		ret =  crash_prepare_elf64_headers(cmem, true, addr, sz);
+	ret = crash_prepare_elf64_headers(cmem, true, addr, sz);
 
+out:
 	kfree(cmem);
 	return ret;
 }
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index be5f85b0a24de69..4bb2e55366be64d 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -248,7 +248,18 @@ static void __init request_standard_resources(void)
 		    kernel_data.end <= res->end)
 			request_resource(res, &kernel_data);
 #ifdef CONFIG_KEXEC_CORE
-		/* Userspace will find "Crash kernel" region in /proc/iomem. */
+		/*
+		 * Userspace will find "Crash kernel" or "Crash kernel (low)"
+		 * region in /proc/iomem.
+		 * In order to distinct from the high region and make no effect
+		 * to the use of existing kexec-tools, rename the low region as
+		 * "Crash kernel (low)".
+		 */
+		if (crashk_low_res.end && crashk_low_res.start >= res->start &&
+				crashk_low_res.end <= res->end) {
+			crashk_low_res.name = "Crash kernel (low)";
+			request_resource(res, &crashk_low_res);
+		}
 		if (crashk_res.end && crashk_res.start >= res->start &&
 		    crashk_res.end <= res->end)
 			request_resource(res, &crashk_res);
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index be4595dc7459115..91b8038a1529068 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -74,41 +74,32 @@ phys_addr_t arm64_dma_phys_limit __ro_after_init;
  */
 static void __init reserve_crashkernel(void)
 {
-	unsigned long long crash_base, crash_size;
-	unsigned long long crash_max = CRASH_ADDR_LOW_MAX;
+	unsigned long long crash_size, crash_base, total_mem, low_size;
+	bool high = false;
 	int ret;
 
-	ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
-				&crash_size, &crash_base);
-	/* no crashkernel= or invalid value specified */
-	if (ret || !crash_size)
-		return;
-
-	crash_size = PAGE_ALIGN(crash_size);
-
-	/* User specifies base address explicitly. */
-	if (crash_base)
-		crash_max = crash_base + crash_size;
+	total_mem = memblock_phys_mem_size();
 
-	/* Current arm64 boot protocol requires 2MB alignment */
-	crash_base = memblock_phys_alloc_range(crash_size, CRASH_ALIGN,
-					       crash_base, crash_max);
-	if (!crash_base) {
-		pr_warn("cannot allocate crashkernel (size:0x%llx)\n",
-			crash_size);
-		return;
+	ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base);
+	if (ret != 0 || crash_size <= 0) {
+		/* crashkernel=X,high and possible crashkernel=Y,low */
+		ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size);
+		if (ret)
+			return;
+		high = true;
 	}
 
-	pr_info("crashkernel reserved: 0x%016llx - 0x%016llx (%lld MB)\n",
-		crash_base, crash_base + crash_size, crash_size >> 20);
+	ret = reserve_crashkernel_mem(total_mem, crash_size, crash_base, low_size, high);
+	if (ret)
+		return;
 
 	/*
 	 * The crashkernel memory will be removed from the kernel linear
 	 * map. Inform kmemleak so that it won't try to access it.
 	 */
-	kmemleak_ignore_phys(crash_base);
-	crashk_res.start = crash_base;
-	crashk_res.end = crash_base + crash_size - 1;
+	kmemleak_ignore_phys(crashk_res.start);
+	if (crashk_low_res.end)
+		kmemleak_ignore_phys(crashk_low_res.start);
 }
 #else
 static void __init reserve_crashkernel(void)
-- 
2.25.1

[PATCH v19 12/13] of: fdt: Add memory for devices by DT property "linux,usable-memory-range"

[Zhen Lei]

From: Chen Zhou <chenzhou10@huawei.com>

When reserving crashkernel in high memory, some low memory is reserved
for crash dump kernel devices and never mapped by the first kernel.
This memory range is advertised to crash dump kernel via DT property
under /chosen,
        linux,usable-memory-range = <BASE1 SIZE1 [BASE2 SIZE2]>

We reused the DT property linux,usable-memory-range and made the low
memory region as the second range "BASE2 SIZE2", which keeps compatibility
with existing user-space and older kdump kernels.

Crash dump kernel reads this property at boot time and call memblock_add()
to add the low memory region after memblock_cap_memory_range() has been
called.

Signed-off-by: Chen Zhou <chenzhou10@huawei.com>
Co-developed-by: Zhen Lei <thunder.leizhen@huawei.com>
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Tested-by: Dave Kleikamp <dave.kleikamp@oracle.com>
---
 drivers/of/fdt.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 65af475dfa9508f..20e6281b2201ff5 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -967,16 +967,24 @@ static void __init early_init_dt_check_for_elfcorehdr(unsigned long node)
 
 static unsigned long chosen_node_offset = -FDT_ERR_NOTFOUND;
 
+/*
+ * The main usage of linux,usable-memory-range is for crash dump kernel.
+ * Originally, the number of usable-memory regions is one. Now there may
+ * be two regions, low region and high region.
+ * To make compatibility with existing user-space and older kdump, the low
+ * region is always the last range of linux,usable-memory-range if exist.
+ */
+#define MAX_USABLE_RANGES		2
+
 /**
  * early_init_dt_check_for_usable_mem_range - Decode usable memory range
  * location from flat tree
  */
 void __init early_init_dt_check_for_usable_mem_range(void)
 {
-	const __be32 *prop;
-	int len;
-	phys_addr_t cap_mem_addr;
-	phys_addr_t cap_mem_size;
+	struct memblock_region rgn[MAX_USABLE_RANGES] = {0};
+	const __be32 *prop, *endp;
+	int len, i;
 	unsigned long node = chosen_node_offset;
 
 	if ((long)node < 0)
@@ -985,16 +993,21 @@ void __init early_init_dt_check_for_usable_mem_range(void)
 	pr_debug("Looking for usable-memory-range property... ");
 
 	prop = of_get_flat_dt_prop(node, "linux,usable-memory-range", &len);
-	if (!prop || (len < (dt_root_addr_cells + dt_root_size_cells)))
+	if (!prop || (len % (dt_root_addr_cells + dt_root_size_cells)))
 		return;
 
-	cap_mem_addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
-	cap_mem_size = dt_mem_next_cell(dt_root_size_cells, &prop);
+	endp = prop + (len / sizeof(__be32));
+	for (i = 0; i < MAX_USABLE_RANGES && prop < endp; i++) {
+		rgn[i].base = dt_mem_next_cell(dt_root_addr_cells, &prop);
+		rgn[i].size = dt_mem_next_cell(dt_root_size_cells, &prop);
 
-	pr_debug("cap_mem_start=%pa cap_mem_size=%pa\n", &cap_mem_addr,
-		 &cap_mem_size);
+		pr_debug("cap_mem_regions[%d]: base=%pa, size=%pa\n",
+			 i, &rgn[i].base, &rgn[i].size);
+	}
 
-	memblock_cap_memory_range(cap_mem_addr, cap_mem_size);
+	memblock_cap_memory_range(rgn[0].base, rgn[0].size);
+	for (i = 1; i < MAX_USABLE_RANGES && rgn[i].size; i++)
+		memblock_add(rgn[i].base, rgn[i].size);
 }
 
 #ifdef CONFIG_SERIAL_EARLYCON
-- 
2.25.1

[PATCH v19 13/13] kdump: update Documentation about crashkernel

[Zhen Lei]

From: Chen Zhou <chenzhou10@huawei.com>

For arm64, the behavior of crashkernel=X has been changed, which
tries low allocation in DMA zone and fall back to high allocation
if it fails.

We can also use "crashkernel=X,high" to select a high region above
DMA zone, which also tries to allocate at least 256M low memory in
DMA zone automatically and "crashkernel=Y,low" can be used to allocate
specified size low memory.

So update the Documentation.

Signed-off-by: Chen Zhou <chenzhou10@huawei.com>
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 Documentation/admin-guide/kdump/kdump.rst       | 11 +++++++++--
 Documentation/admin-guide/kernel-parameters.txt | 11 +++++++++--
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/kdump/kdump.rst b/Documentation/admin-guide/kdump/kdump.rst
index cb30ca3df27c9b2..d4c287044be0c70 100644
--- a/Documentation/admin-guide/kdump/kdump.rst
+++ b/Documentation/admin-guide/kdump/kdump.rst
@@ -361,8 +361,15 @@ Boot into System Kernel
    kernel will automatically locate the crash kernel image within the
    first 512MB of RAM if X is not given.
 
-   On arm64, use "crashkernel=Y[@X]".  Note that the start address of
-   the kernel, X if explicitly specified, must be aligned to 2MiB (0x200000).
+   On arm64, use "crashkernel=X" to try low allocation in DMA zone and
+   fall back to high allocation if it fails.
+   We can also use "crashkernel=X,high" to select a high region above
+   DMA zone, which also tries to allocate at least 256M low memory in
+   DMA zone automatically.
+   "crashkernel=Y,low" can be used to allocate specified size low memory.
+   Use "crashkernel=Y@X" if you really have to reserve memory from
+   specified start address X. Note that the start address of the kernel,
+   X if explicitly specified, must be aligned to 2MiB (0x200000).
 
 Load the Dump-capture Kernel
 ============================
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index fc34332c8d9a6df..5fafeea70f8f14d 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -783,6 +783,9 @@
 			[KNL, X86-64] Select a region under 4G first, and
 			fall back to reserve region above 4G when '@offset'
 			hasn't been specified.
+			[KNL, ARM64] Try low allocation in DMA zone and fall back
+			to high allocation if it fails when '@offset' hasn't been
+			specified.
 			See Documentation/admin-guide/kdump/kdump.rst for further details.
 
 	crashkernel=range1:size1[,range2:size2,...][@offset]
@@ -799,6 +802,8 @@
 			Otherwise memory region will be allocated below 4G, if
 			available.
 			It will be ignored if crashkernel=X is specified.
+			[KNL, ARM64] range in high memory.
+			Allow kernel to allocate physical memory region from top.
 	crashkernel=size[KMG],low
 			[KNL, X86-64] range under 4G. When crashkernel=X,high
 			is passed, kernel could allocate physical memory region
@@ -807,13 +812,15 @@
 			requires at least 64M+32K low memory, also enough extra
 			low memory is needed to make sure DMA buffers for 32-bit
 			devices won't run out. Kernel would try to allocate at
-			at least 256M below 4G automatically.
+			least 256M below 4G automatically.
 			This one let user to specify own low range under 4G
 			for second kernel instead.
 			0: to disable low allocation.
 			It will be ignored when crashkernel=X,high is not used
 			or memory reserved is below 4G.
-
+			[KNL, ARM64] range in low memory.
+			This one let user to specify a low range in DMA zone for
+			crash dump kernel.
 	cryptomgr.notests
 			[KNL] Disable crypto self-tests
 
-- 
2.25.1

Re: [PATCH v19 02/13] x86/setup: Use parse_crashkernel_high_low() to simplify code

[Borislav Petkov]

On Tue, Dec 28, 2021 at 09:26:01PM +0800, Zhen Lei wrote:
> Use parse_crashkernel_high_low() to bring the parsing of
> "crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they
> are strongly dependent, make code logic clear and more readable.
> 
> Suggested-by: Borislav Petkov <bp@alien8.de>

Yeah, doesn't look like something I suggested...

> @@ -474,10 +472,9 @@ static void __init reserve_crashkernel(void)
>  	/* crashkernel=XM */
>  	ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base);
>  	if (ret != 0 || crash_size <= 0) {
> -		/* crashkernel=X,high */
> -		ret = parse_crashkernel_high(boot_command_line, total_mem,
> -					     &crash_size, &crash_base);
> -		if (ret != 0 || crash_size <= 0)
> +		/* crashkernel=X,high and possible crashkernel=Y,low */
> +		ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size);

So this calls parse_crashkernel() and when that one fails, it calls this
new weird parse high/low helper you added.

But then all three end up in the same __parse_crashkernel() worker
function which seems to do the actual parsing.

What I suggested and what would be real clean is if the arches would
simply call a *single* 

	parse_crashkernel()

function and when that one returns, *all* crashkernel= options would
have been parsed properly, low, high, middle crashkernel, whatever...
and the caller would know what crash kernel needs to be allocated.

Then each arch can do its memory allocations and checks based on that
parsed data and decide to allocate or bail.

So it is getting there but it needs more surgery...

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v19 02/13] x86/setup: Use parse_crashkernel_high_low() to simplify code

[Leizhen (ThunderTown)]

On 2021/12/29 0:13, Borislav Petkov wrote:
> On Tue, Dec 28, 2021 at 09:26:01PM +0800, Zhen Lei wrote:
>> Use parse_crashkernel_high_low() to bring the parsing of
>> "crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they
>> are strongly dependent, make code logic clear and more readable.
>>
>> Suggested-by: Borislav Petkov <bp@alien8.de>
> 
> Yeah, doesn't look like something I suggested...
> 
>> @@ -474,10 +472,9 @@ static void __init reserve_crashkernel(void)
>>  	/* crashkernel=XM */
>>  	ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base);
>>  	if (ret != 0 || crash_size <= 0) {
>> -		/* crashkernel=X,high */
>> -		ret = parse_crashkernel_high(boot_command_line, total_mem,
>> -					     &crash_size, &crash_base);
>> -		if (ret != 0 || crash_size <= 0)
>> +		/* crashkernel=X,high and possible crashkernel=Y,low */
>> +		ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size);
> 
> So this calls parse_crashkernel() and when that one fails, it calls this
> new weird parse high/low helper you added.
> 
> But then all three end up in the same __parse_crashkernel() worker
> function which seems to do the actual parsing.
> 
> What I suggested and what would be real clean is if the arches would
> simply call a *single* 
> 
> 	parse_crashkernel()
> 
> function and when that one returns, *all* crashkernel= options would
> have been parsed properly, low, high, middle crashkernel, whatever...
> and the caller would know what crash kernel needs to be allocated.
> 
> Then each arch can do its memory allocations and checks based on that
> parsed data and decide to allocate or bail.

However, only x86 currently supports "crashkernel=X,high" and "crashkernel=Y,low", and arm64
will also support it. It is not supported on other architectures. So changing parse_crashkernel()
is not appropriate unless a new function is introduced. But naming this new function isn't easy,
and the name parse_crashkernel_in_order() that I've named before doesn't seem to be good.
Of course, we can also consider changing parse_crashkernel() to another name, then use
parse_crashkernel() to parse all possible "crashkernel=" options in order, but this will cause
other architectures to change as well.

> 
> So it is getting there but it needs more surgery...
> 
> Thx.
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH v19 02/13] x86/setup: Use parse_crashkernel_high_low() to simplify code

[Dave Young]

On 12/29/21 at 10:27am, Leizhen (ThunderTown) wrote:
> 
> 
> On 2021/12/29 0:13, Borislav Petkov wrote:
> > On Tue, Dec 28, 2021 at 09:26:01PM +0800, Zhen Lei wrote:
> >> Use parse_crashkernel_high_low() to bring the parsing of
> >> "crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they
> >> are strongly dependent, make code logic clear and more readable.
> >>
> >> Suggested-by: Borislav Petkov <bp@alien8.de>
> > 
> > Yeah, doesn't look like something I suggested...
> > 
> >> @@ -474,10 +472,9 @@ static void __init reserve_crashkernel(void)
> >>  	/* crashkernel=XM */
> >>  	ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base);
> >>  	if (ret != 0 || crash_size <= 0) {
> >> -		/* crashkernel=X,high */
> >> -		ret = parse_crashkernel_high(boot_command_line, total_mem,
> >> -					     &crash_size, &crash_base);
> >> -		if (ret != 0 || crash_size <= 0)
> >> +		/* crashkernel=X,high and possible crashkernel=Y,low */
> >> +		ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size);
> > 
> > So this calls parse_crashkernel() and when that one fails, it calls this
> > new weird parse high/low helper you added.
> > 
> > But then all three end up in the same __parse_crashkernel() worker
> > function which seems to do the actual parsing.
> > 
> > What I suggested and what would be real clean is if the arches would
> > simply call a *single* 
> > 
> > 	parse_crashkernel()
> > 
> > function and when that one returns, *all* crashkernel= options would
> > have been parsed properly, low, high, middle crashkernel, whatever...
> > and the caller would know what crash kernel needs to be allocated.
> > 
> > Then each arch can do its memory allocations and checks based on that
> > parsed data and decide to allocate or bail.
> 
> However, only x86 currently supports "crashkernel=X,high" and "crashkernel=Y,low", and arm64
> will also support it. It is not supported on other architectures. So changing parse_crashkernel()
> is not appropriate unless a new function is introduced. But naming this new function isn't easy,
> and the name parse_crashkernel_in_order() that I've named before doesn't seem to be good.
> Of course, we can also consider changing parse_crashkernel() to another name, then use
> parse_crashkernel() to parse all possible "crashkernel=" options in order, but this will cause
> other architectures to change as well.

Hi, I did not follow up all discussions, but if the only difference is
about the low -> high fallback, I think you can add another argument in
parse_crashkernel(..., *fallback_high),  and doing some changes in
__parse_crashkernel() before it returns.  But since there are two
many arguments, you could need a wrapper struct for crashkernel_param if
needed.

If you do not want to touch other arches, another function maybe
something like parse_crashkernel_fallback() for x86 and arm64 to use.

But I may not get all the context, please ignore if this is not the
case.  I agree that calling parse_crash_kernel* in the
reserve_crashkernel funtions looks not good though. 

OTOH there are bunch of other logics in param parsing code,
eg. determin the final size and offset etc. To split the logic out more
things need to be done, eg. firstly parsing function just get all the
inputs from cmdline string eg. an array of struct crashkernel_param with
mem_range, expected size, expected offset, high, or low, and do not do
any other things.   Then pass these parsed inputs to another function to
determine the final crash_size and crash_base, that part can be arch
specific somehow. 

So I think you can unify the parse_crashkernel* in x86 first with just
one function.  And leave the further improvements to later work. But
let's see how Boris think about this.

> 
> > 
> > So it is getting there but it needs more surgery...
> > 
> > Thx.
> > 
> 
> -- 
> Regards,
>   Zhen Lei
> 

Thanks
Dave

Re: [PATCH v19 02/13] x86/setup: Use parse_crashkernel_high_low() to simplify code

[Dave Young]

On 12/29/21 at 03:27pm, Dave Young wrote:
> On 12/29/21 at 10:27am, Leizhen (ThunderTown) wrote:
> > 
> > 
> > On 2021/12/29 0:13, Borislav Petkov wrote:
> > > On Tue, Dec 28, 2021 at 09:26:01PM +0800, Zhen Lei wrote:
> > >> Use parse_crashkernel_high_low() to bring the parsing of
> > >> "crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they
> > >> are strongly dependent, make code logic clear and more readable.
> > >>
> > >> Suggested-by: Borislav Petkov <bp@alien8.de>
> > > 
> > > Yeah, doesn't look like something I suggested...
> > > 
> > >> @@ -474,10 +472,9 @@ static void __init reserve_crashkernel(void)
> > >>  	/* crashkernel=XM */
> > >>  	ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base);
> > >>  	if (ret != 0 || crash_size <= 0) {
> > >> -		/* crashkernel=X,high */
> > >> -		ret = parse_crashkernel_high(boot_command_line, total_mem,
> > >> -					     &crash_size, &crash_base);
> > >> -		if (ret != 0 || crash_size <= 0)
> > >> +		/* crashkernel=X,high and possible crashkernel=Y,low */
> > >> +		ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size);
> > > 
> > > So this calls parse_crashkernel() and when that one fails, it calls this
> > > new weird parse high/low helper you added.
> > > 
> > > But then all three end up in the same __parse_crashkernel() worker
> > > function which seems to do the actual parsing.
> > > 
> > > What I suggested and what would be real clean is if the arches would
> > > simply call a *single* 
> > > 
> > > 	parse_crashkernel()
> > > 
> > > function and when that one returns, *all* crashkernel= options would
> > > have been parsed properly, low, high, middle crashkernel, whatever...
> > > and the caller would know what crash kernel needs to be allocated.
> > > 
> > > Then each arch can do its memory allocations and checks based on that
> > > parsed data and decide to allocate or bail.
> > 
> > However, only x86 currently supports "crashkernel=X,high" and "crashkernel=Y,low", and arm64
> > will also support it. It is not supported on other architectures. So changing parse_crashkernel()
> > is not appropriate unless a new function is introduced. But naming this new function isn't easy,
> > and the name parse_crashkernel_in_order() that I've named before doesn't seem to be good.
> > Of course, we can also consider changing parse_crashkernel() to another name, then use
> > parse_crashkernel() to parse all possible "crashkernel=" options in order, but this will cause
> > other architectures to change as well.
> 
> Hi, I did not follow up all discussions, but if the only difference is
> about the low -> high fallback, I think you can add another argument in
> parse_crashkernel(..., *fallback_high),  and doing some changes in
> __parse_crashkernel() before it returns.  But since there are two
> many arguments, you could need a wrapper struct for crashkernel_param if
> needed.
> 
> If you do not want to touch other arches, another function maybe
> something like parse_crashkernel_fallback() for x86 and arm64 to use.
> 
> But I may not get all the context, please ignore if this is not the
> case.  I agree that calling parse_crash_kernel* in the
> reserve_crashkernel funtions looks not good though. 
> 
> OTOH there are bunch of other logics in param parsing code,
> eg. determin the final size and offset etc. To split the logic out more
> things need to be done, eg. firstly parsing function just get all the
> inputs from cmdline string eg. an array of struct crashkernel_param with
> mem_range, expected size, expected offset, high, or low, and do not do
> any other things.   Then pass these parsed inputs to another function to
> determine the final crash_size and crash_base, that part can be arch
> specific somehow. 
> 
> So I think you can unify the parse_crashkernel* in x86 first with just
> one function.  And leave the further improvements to later work. But
> let's see how Boris think about this.
> 

BTW, I would suggest to wait for reviewers to response (eg. one week at
least, or more due to the holidays) before updating another version

Do not worry to miss the 5.17.  I would say take it easy if it will
miss then let's just leave with it and continue to work on the future
improvements.  I think one reason this issue takes too long time is that it was
discussed some time but no followup and later people need to warm up
again.  Just keep it warm and continue to engage in the improvements, do
not hurry for the specific mainline release.

Thanks
Dave

Re: [PATCH v19 02/13] x86/setup: Use parse_crashkernel_high_low() to simplify code

[Borislav Petkov]

On Wed, Dec 29, 2021 at 03:27:48PM +0800, Dave Young wrote:
> So I think you can unify the parse_crashkernel* in x86 first with just
> one function.  And leave the further improvements to later work. But
> let's see how Boris think about this.

Well, I think this all unnecessary work. Why?

If the goal is to support crashkernel...high,low on arm64, then you
should simply *copy* the functionality on arm64 and be done with it.

Unification is done by looking at code which is duplicated across
architectures and which has been untouched for a while now, i.e., no
new or arch-specific changes are going to it so a unification can be
as simple as trivially switching the architectures to call a generic
function.

What this does is carve out the "generic" parts and then try not to
break existing usage.

Which is a total waste of energy and resources. And it is casting that
functionality in stone so that when x86 wants to change something there,
it should do it in a way not to break arm64. And I fail to see the
advantage of all that. Code sharing ain't it.

So what it should do is simply copy the necessary code to arm64.
Unifications can always be done later, when the dust settles.

IMNSVHO.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v19 02/13] x86/setup: Use parse_crashkernel_high_low() to simplify code

[Borislav Petkov]

On Wed, Dec 29, 2021 at 03:45:12PM +0800, Dave Young wrote:
> BTW, I would suggest to wait for reviewers to response (eg. one week at
> least, or more due to the holidays) before updating another version
> 
> Do not worry to miss the 5.17.  I would say take it easy if it will
> miss then let's just leave with it and continue to work on the future
> improvements.  I think one reason this issue takes too long time is that it was
> discussed some time but no followup and later people need to warm up
> again.  Just keep it warm and continue to engage in the improvements, do
> not hurry for the specific mainline release.

Can you tell this to *all* patch submitters please?

I can't count the times where people simply hurry to send the new
revision just to get it in the next kernel, and make silly mistakes
while doing so. Or not think things straight and misdesign it all.

And what this causes is the opposite of what they wanna achieve - pissed
maintainers and ignored threads.

And they all *know* that the next kernel is around the corner. So why
the hell does it even matter when?

What most submitters fail to realize is, the moment your code hits
upstream, it becomes the maintainers' problem and submitters can relax.

But maintainers get to deal with this code forever. So after a while
maintainers learn that they either accept ready code and it all just
works or they make the mistake to take half-baked crap in and then they
themselves get to clean it up and fix it.

So maintainers learn quickly to push back.

But it is annoying and it would help immensely if submitters would
consider this and stop hurrying the code in but try to do a *good* job
first, design-wise and code-wise by thinking hard about what they're
trying to do.

Yeah, things could be a lot simpler and easier - it only takes a little
bit of effort...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v19 02/13] x86/setup: Use parse_crashkernel_high_low() to simplify code

[Dave Young]

On 12/29/21 at 11:11am, Borislav Petkov wrote:
> On Wed, Dec 29, 2021 at 03:45:12PM +0800, Dave Young wrote:
> > BTW, I would suggest to wait for reviewers to response (eg. one week at
> > least, or more due to the holidays) before updating another version
> > 
> > Do not worry to miss the 5.17.  I would say take it easy if it will
> > miss then let's just leave with it and continue to work on the future
> > improvements.  I think one reason this issue takes too long time is that it was
> > discussed some time but no followup and later people need to warm up
> > again.  Just keep it warm and continue to engage in the improvements, do
> > not hurry for the specific mainline release.
> 
> Can you tell this to *all* patch submitters please?

I appreciate you further explanation below to describe the situation.  I do not
see how can I tell this to *all* submitters,  but I am and I will try to do this
as far as I can.  Maintainers and patch submitters, it would help for both
parties show sympathy with each other, some soft reminders will help
people to understand each other, especially for new comers.

> 
> I can't count the times where people simply hurry to send the new
> revision just to get it in the next kernel, and make silly mistakes
> while doing so. Or not think things straight and misdesign it all.
> 
> And what this causes is the opposite of what they wanna achieve - pissed
> maintainers and ignored threads.
> 
> And they all *know* that the next kernel is around the corner. So why
> the hell does it even matter when?
> 
> What most submitters fail to realize is, the moment your code hits
> upstream, it becomes the maintainers' problem and submitters can relax.
> 
> But maintainers get to deal with this code forever. So after a while
> maintainers learn that they either accept ready code and it all just
> works or they make the mistake to take half-baked crap in and then they
> themselves get to clean it up and fix it.
> 
> So maintainers learn quickly to push back.
> 
> But it is annoying and it would help immensely if submitters would
> consider this and stop hurrying the code in but try to do a *good* job
> first, design-wise and code-wise by thinking hard about what they're
> trying to do.
> 
> Yeah, things could be a lot simpler and easier - it only takes a little
> bit of effort...
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette
> 

Thanks
Dave

Re: [PATCH v19 02/13] x86/setup: Use parse_crashkernel_high_low() to simplify code

[Dave Young]

On 12/29/21 at 11:03am, Borislav Petkov wrote:
> On Wed, Dec 29, 2021 at 03:27:48PM +0800, Dave Young wrote:
> > So I think you can unify the parse_crashkernel* in x86 first with just
> > one function.  And leave the further improvements to later work. But
> > let's see how Boris think about this.
> 
> Well, I think this all unnecessary work. Why?
> 
> If the goal is to support crashkernel...high,low on arm64, then you
> should simply *copy* the functionality on arm64 and be done with it.
> 
> Unification is done by looking at code which is duplicated across
> architectures and which has been untouched for a while now, i.e., no
> new or arch-specific changes are going to it so a unification can be
> as simple as trivially switching the architectures to call a generic
> function.
> 
> What this does is carve out the "generic" parts and then try not to
> break existing usage.
> 
> Which is a total waste of energy and resources. And it is casting that
> functionality in stone so that when x86 wants to change something there,
> it should do it in a way not to break arm64. And I fail to see the
> advantage of all that. Code sharing ain't it.
> 
> So what it should do is simply copy the necessary code to arm64.
> Unifications can always be done later, when the dust settles.

I think I agree with you about the better way is to doing some
improvements so that arches can logically doing things better.  I can
leave with the way I suggested although it is not the best.  But I think
Leizhen needs a clear direction about how to do it. It is very clear
now.  See how he will handle this. 

> 
> IMNSVHO.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette
> 

Re: [PATCH v19 02/13] x86/setup: Use parse_crashkernel_high_low() to simplify code

[Borislav Petkov]

On Wed, Dec 29, 2021 at 06:38:43PM +0800, Dave Young wrote:
> I appreciate you further explanation below to describe the situation.
> I do not see how can I tell this to *all* submitters,

You don't have to - that was hypothetical. :-)

I'm typing this on a public mailing list with the hope that people will
see it.

> but I am and I will try to do this as far as I can.

Much appreciated.

> Maintainers and patch submitters, it would help for both
> parties show sympathy with each other, some soft reminders will help
> people to understand each other, especially for new comers.

Yap, that's why we keep repeating it.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v19 02/13] x86/setup: Use parse_crashkernel_high_low() to simplify code

[Leizhen (ThunderTown)]

On 2021/12/29 15:27, Dave Young wrote:
> On 12/29/21 at 10:27am, Leizhen (ThunderTown) wrote:
>>
>>
>> On 2021/12/29 0:13, Borislav Petkov wrote:
>>> On Tue, Dec 28, 2021 at 09:26:01PM +0800, Zhen Lei wrote:
>>>> Use parse_crashkernel_high_low() to bring the parsing of
>>>> "crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they
>>>> are strongly dependent, make code logic clear and more readable.
>>>>
>>>> Suggested-by: Borislav Petkov <bp@alien8.de>
>>>
>>> Yeah, doesn't look like something I suggested...
>>>
>>>> @@ -474,10 +472,9 @@ static void __init reserve_crashkernel(void)
>>>>  	/* crashkernel=XM */
>>>>  	ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base);
>>>>  	if (ret != 0 || crash_size <= 0) {
>>>> -		/* crashkernel=X,high */
>>>> -		ret = parse_crashkernel_high(boot_command_line, total_mem,
>>>> -					     &crash_size, &crash_base);
>>>> -		if (ret != 0 || crash_size <= 0)
>>>> +		/* crashkernel=X,high and possible crashkernel=Y,low */
>>>> +		ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size);
>>>
>>> So this calls parse_crashkernel() and when that one fails, it calls this
>>> new weird parse high/low helper you added.
>>>
>>> But then all three end up in the same __parse_crashkernel() worker
>>> function which seems to do the actual parsing.
>>>
>>> What I suggested and what would be real clean is if the arches would
>>> simply call a *single* 
>>>
>>> 	parse_crashkernel()
>>>
>>> function and when that one returns, *all* crashkernel= options would
>>> have been parsed properly, low, high, middle crashkernel, whatever...
>>> and the caller would know what crash kernel needs to be allocated.
>>>
>>> Then each arch can do its memory allocations and checks based on that
>>> parsed data and decide to allocate or bail.
>>
>> However, only x86 currently supports "crashkernel=X,high" and "crashkernel=Y,low", and arm64
>> will also support it. It is not supported on other architectures. So changing parse_crashkernel()
>> is not appropriate unless a new function is introduced. But naming this new function isn't easy,
>> and the name parse_crashkernel_in_order() that I've named before doesn't seem to be good.
>> Of course, we can also consider changing parse_crashkernel() to another name, then use
>> parse_crashkernel() to parse all possible "crashkernel=" options in order, but this will cause
>> other architectures to change as well.
> 
> Hi, I did not follow up all discussions, but if the only difference is
> about the low -> high fallback, I think you can add another argument in
> parse_crashkernel(..., *fallback_high),  and doing some changes in
> __parse_crashkernel() before it returns.  But since there are two
> many arguments, you could need a wrapper struct for crashkernel_param if
> needed.

A wrapper struct is needed. Because at least two new output parameters need to be added:
1. high, to indicate whether "crashkernel=X,high" is specified
2. low_size, to save the memory size specified by "crashkernel=Y,low"

> 
> If you do not want to touch other arches, another function maybe
> something like parse_crashkernel_fallback() for x86 and arm64 to use.
> 
> But I may not get all the context, please ignore if this is not the
> case.  I agree that calling parse_crash_kernel* in the
> reserve_crashkernel funtions looks not good though. 
> 
> OTOH there are bunch of other logics in param parsing code,
> eg. determin the final size and offset etc. To split the logic out more
> things need to be done, eg. firstly parsing function just get all the
> inputs from cmdline string eg. an array of struct crashkernel_param with
> mem_range, expected size, expected offset, high, or low, and do not do
> any other things.   Then pass these parsed inputs to another function to
> determine the final crash_size and crash_base, that part can be arch
> specific somehow. 

Yes, this way makes the code logic clear. But there's a bit of trouble with
"crashkernel=range1:size1[,range2:size2,...][@offset]", the array size is dynamic.

> 
> So I think you can unify the parse_crashkernel* in x86 first with just
> one function.  And leave the further improvements to later work. But
> let's see how Boris think about this.
> 
>>
>>>
>>> So it is getting there but it needs more surgery...
>>>
>>> Thx.
>>>
>>
>> -- 
>> Regards,
>>   Zhen Lei
>>
> 
> Thanks
> Dave
> 
> .
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH v19 02/13] x86/setup: Use parse_crashkernel_high_low() to simplify code

[Leizhen (ThunderTown)]

On 2021/12/29 18:38, Dave Young wrote:
> On 12/29/21 at 11:11am, Borislav Petkov wrote:
>> On Wed, Dec 29, 2021 at 03:45:12PM +0800, Dave Young wrote:
>>> BTW, I would suggest to wait for reviewers to response (eg. one week at
>>> least, or more due to the holidays) before updating another version
>>>
>>> Do not worry to miss the 5.17.  I would say take it easy if it will
>>> miss then let's just leave with it and continue to work on the future
>>> improvements.  I think one reason this issue takes too long time is that it was
>>> discussed some time but no followup and later people need to warm up
>>> again.  Just keep it warm and continue to engage in the improvements, do
>>> not hurry for the specific mainline release.
>>
>> Can you tell this to *all* patch submitters please?
> 
> I appreciate you further explanation below to describe the situation.  I do not
> see how can I tell this to *all* submitters,  but I am and I will try to do this
> as far as I can.  Maintainers and patch submitters, it would help for both
> parties show sympathy with each other, some soft reminders will help
> people to understand each other, especially for new comers.
> 
>>
>> I can't count the times where people simply hurry to send the new
>> revision just to get it in the next kernel, and make silly mistakes
>> while doing so. Or not think things straight and misdesign it all.
>>
>> And what this causes is the opposite of what they wanna achieve - pissed
>> maintainers and ignored threads.

I just hope the first 4 patches can be merged into v5.17. It seems to me
that it is quite clear. Although the goal of the final stage is to modify
function parse_crashkernel() according to the current opinion. But that's not a
lightweight change after all. The final parse_crashkernel() change may take
one version or two. In this process, it maybe OK to do a part of cleanup first.

It's like someone who wants to buy a luxury car to commute to work six months
later. He buys a cheap used car and sells it six months later. It sounds right
to me, don't you think?

>>
>> And they all *know* that the next kernel is around the corner. So why
>> the hell does it even matter when?

Because all programmers should have confidence in the code they write. I have
a new idea, and I'm free these days, so I updated v19. I can't rely on people
telling me to take a step forward, then take a step forward. Otherwise, stand
still.

>>
>> What most submitters fail to realize is, the moment your code hits
>> upstream, it becomes the maintainers' problem and submitters can relax.

Sorry, I'll make sure all the comments are collected and then send the next
edition.

>>
>> But maintainers get to deal with this code forever. So after a while
>> maintainers learn that they either accept ready code and it all just
>> works or they make the mistake to take half-baked crap in and then they
>> themselves get to clean it up and fix it.
>>
>> So maintainers learn quickly to push back.
>>
>> But it is annoying and it would help immensely if submitters would
>> consider this and stop hurrying the code in but try to do a *good* job
>> first, design-wise and code-wise by thinking hard about what they're
>> trying to do.
>>
>> Yeah, things could be a lot simpler and easier - it only takes a little
>> bit of effort...
>>
>> -- 
>> Regards/Gruss,
>>     Boris.
>>
>> https://people.kernel.org/tglx/notes-about-netiquette
>>
> 
> Thanks
> Dave
> 
> .
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH v19 02/13] x86/setup: Use parse_crashkernel_high_low() to simplify code

[Leizhen (ThunderTown)]

On 2021/12/29 18:46, Dave Young wrote:
> On 12/29/21 at 11:03am, Borislav Petkov wrote:
>> On Wed, Dec 29, 2021 at 03:27:48PM +0800, Dave Young wrote:
>>> So I think you can unify the parse_crashkernel* in x86 first with just
>>> one function.  And leave the further improvements to later work. But
>>> let's see how Boris think about this.
>>
>> Well, I think this all unnecessary work. Why?
>>
>> If the goal is to support crashkernel...high,low on arm64, then you
>> should simply *copy* the functionality on arm64 and be done with it.
>>
>> Unification is done by looking at code which is duplicated across
>> architectures and which has been untouched for a while now, i.e., no
>> new or arch-specific changes are going to it so a unification can be
>> as simple as trivially switching the architectures to call a generic
>> function.
>>
>> What this does is carve out the "generic" parts and then try not to
>> break existing usage.
>>
>> Which is a total waste of energy and resources. And it is casting that
>> functionality in stone so that when x86 wants to change something there,
>> it should do it in a way not to break arm64. And I fail to see the
>> advantage of all that. Code sharing ain't it.

It's just a worry, there's uncertainty about whether it's going to be. I think
the only thing that might change is the default value of "low_size". Of course,
the alignment size and start address may also change, but most of them can be
controlled by macros.

Chen Zhou and I tried to share the code because of a suggestion. After so many
attempts, it doesn't seem to fit to make generic. Or maybe I haven't figured
out a good solution yet.


>>
>> So what it should do is simply copy the necessary code to arm64.
>> Unifications can always be done later, when the dust settles.
> 
> I think I agree with you about the better way is to doing some
> improvements so that arches can logically doing things better.  I can
> leave with the way I suggested although it is not the best.  But I think
> Leizhen needs a clear direction about how to do it. It is very clear
> now.  See how he will handle this. 

Surviving, then pursuing ideals.

I will put the patches that make arm64 support crashkernel...high,low to
the front, then the parse_crashkernel() unification patches. Even if the
second half of the patches is not ready for v5.18, the first half of the
patches is ready.

> 
>>
>> IMNSVHO.
>>
>> -- 
>> Regards/Gruss,
>>     Boris.
>>
>> https://people.kernel.org/tglx/notes-about-netiquette
>>
> 
> .
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH v19 02/13] x86/setup: Use parse_crashkernel_high_low() to simplify code

[Borislav Petkov]

On Wed, Dec 29, 2021 at 11:04:21PM +0800, Leizhen (ThunderTown) wrote:
> Chen Zhou and I tried to share the code because of a suggestion. After so many
> attempts, it doesn't seem to fit to make generic. Or maybe I haven't figured
> out a good solution yet.

Well, you learned a very important lesson and the many attempts are not
in vain: code sharing does not make sense in every case.

> I will put the patches that make arm64 support crashkernel...high,low to
> the front, then the parse_crashkernel() unification patches. Even if the
> second half of the patches is not ready for v5.18, the first half of the
> patches is ready.

I think you should concentrate on the arm64 side which is, AFAICT, what
you're trying to achieve.

The "parse_crashkernel() unification" needs more thought because, as I
said already, that doesn't make a whole lot of sense to me.

If you want to enforce the fact that "low" makes sense only when "high"
is supplied, parse_crashkernel_high_low() is not the right thing to do.
You need to have a *single* function which does all the parsing where
you can decide what to do: "if high, parse low", "if no high supplied,
ignore low" and so on.

And if those are supported on certain architectures only, you can do
ifdeffery...

But I think I already stated that I don't like such unifications which
introduce unnecessary dependencies between architectures. Therefore, I
won't accept them into x86 unless there's a strong compelling reason.
Which I don't see ATM.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v19 02/13] x86/setup: Use parse_crashkernel_high_low() to simplify code

[Leizhen (ThunderTown)]

On 2021/12/30 0:51, Borislav Petkov wrote:
> On Wed, Dec 29, 2021 at 11:04:21PM +0800, Leizhen (ThunderTown) wrote:
>> Chen Zhou and I tried to share the code because of a suggestion. After so many
>> attempts, it doesn't seem to fit to make generic. Or maybe I haven't figured
>> out a good solution yet.
> 
> Well, you learned a very important lesson and the many attempts are not
> in vain: code sharing does not make sense in every case.
> 
>> I will put the patches that make arm64 support crashkernel...high,low to
>> the front, then the parse_crashkernel() unification patches. Even if the
>> second half of the patches is not ready for v5.18, the first half of the
>> patches is ready.
> 
> I think you should concentrate on the arm64 side which is, AFAICT, what
> you're trying to achieve.

Right, a patchset should focus on just one thing.

> 
> The "parse_crashkernel() unification" needs more thought because, as I
> said already, that doesn't make a whole lot of sense to me.

Yes, because it's not a functional improvement, it's not a performance optimization,
it's also not a fix for a known bug, it's just a programmer's artistic pursuit.

> 
> If you want to enforce the fact that "low" makes sense only when "high"
> is supplied, parse_crashkernel_high_low() is not the right thing to do.
> You need to have a *single* function which does all the parsing where
> you can decide what to do: "if high, parse low", "if no high supplied,
> ignore low" and so on.

I understand your proposal, but parse_crashkernel_high_low() is a cost-effective
and profitable change, it makes the current code a little clearer, and avoid passing
unnecessary parameters "system_ram" and "crash_base" when other architectures use
parse_crashkernel_{high|low}().

I actually followed your advice in the beginning to do "parse_crashkernel() and
parse_crashkernel_{high|low}() unification". But I found it's difficult and the
end result may not be as good as expected. So I introduced parse_crashkernel_high_low().

The parameter "system_ram" and "crash_base" of parse_crashkernel() is not need by
"crashkernel=X,[high,low]". And parameter "low_size" of parse_crashkernel_high_low()
is not need by "crashkernel=X[@offset]". The "parse_crashkernel() unification"
complicates things. For example, the parameter "crash_size" means "low or high" memory
size for "crashkernel=X[@offset]", but only means "high" memory size for "crashkernel=X,high".
So we'd better give it two names with union.

> 
> And if those are supported on certain architectures only, you can do
> ifdeffery...

I don't think so. These __init functions are small and architecture-independent, and do not
affect compilation of other architectures. There may be other architectures that use
it in the future, such as the current arm64.

> 
> But I think I already stated that I don't like such unifications which
> introduce unnecessary dependencies between architectures. Therefore, I
> won't accept them into x86 unless there's a strong compelling reason.
> Which I don't see ATM.

OK.

> 
> Thx.
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH v19 02/13] x86/setup: Use parse_crashkernel_high_low() to simplify code

[Leizhen (ThunderTown)]

On 2021/12/30 10:39, Leizhen (ThunderTown) wrote:
> 
> 
> On 2021/12/30 0:51, Borislav Petkov wrote:
>> On Wed, Dec 29, 2021 at 11:04:21PM +0800, Leizhen (ThunderTown) wrote:
>>> Chen Zhou and I tried to share the code because of a suggestion. After so many
>>> attempts, it doesn't seem to fit to make generic. Or maybe I haven't figured
>>> out a good solution yet.
>>
>> Well, you learned a very important lesson and the many attempts are not
>> in vain: code sharing does not make sense in every case.
>>
>>> I will put the patches that make arm64 support crashkernel...high,low to
>>> the front, then the parse_crashkernel() unification patches. Even if the
>>> second half of the patches is not ready for v5.18, the first half of the
>>> patches is ready.
>>
>> I think you should concentrate on the arm64 side which is, AFAICT, what
>> you're trying to achieve.
> 
> Right, a patchset should focus on just one thing.
> 
>>
>> The "parse_crashkernel() unification" needs more thought because, as I
>> said already, that doesn't make a whole lot of sense to me.
> 
> Yes, because it's not a functional improvement, it's not a performance optimization,
> it's also not a fix for a known bug, it's just a programmer's artistic pursuit.
> 
>>
>> If you want to enforce the fact that "low" makes sense only when "high"
>> is supplied, parse_crashkernel_high_low() is not the right thing to do.
>> You need to have a *single* function which does all the parsing where
>> you can decide what to do: "if high, parse low", "if no high supplied,
>> ignore low" and so on.

In fact, this is how my current function parse_crashkernel_high_low() is
implemented.

+	/* crashkernel=X,high */
+	ret = parse_crashkernel_high(cmdline, 0, high_size, &base);
+	if (ret)			//crashkernel=X,high is not specified
+		return ret;
+
+	if (*high_size <= 0)		//crashkernel=X,high is specified but the value is invalid
+		return -EINVAL;		//Sorry, the type of high_size is "unsigned long long *", so less than zero is impossible
+
+	/* crashkernel=Y,low */
+	ret = parse_crashkernel_low(cmdline, 0, low_size, &base);	//If crashkernel=Y,low is specified, the parsed value is stored in *low_size
+	if (ret)
+		*low_size = -1;		//crashkernel=Y,low is not specified


> 
> I understand your proposal, but parse_crashkernel_high_low() is a cost-effective
> and profitable change, it makes the current code a little clearer, and avoid passing
> unnecessary parameters "system_ram" and "crash_base" when other architectures use
> parse_crashkernel_{high|low}().
> 
> I actually followed your advice in the beginning to do "parse_crashkernel() and
> parse_crashkernel_{high|low}() unification". But I found it's difficult and the
> end result may not be as good as expected. So I introduced parse_crashkernel_high_low().
> 
> The parameter "system_ram" and "crash_base" of parse_crashkernel() is not need by
> "crashkernel=X,[high,low]". And parameter "low_size" of parse_crashkernel_high_low()
> is not need by "crashkernel=X[@offset]". The "parse_crashkernel() unification"
> complicates things. For example, the parameter "crash_size" means "low or high" memory
> size for "crashkernel=X[@offset]", but only means "high" memory size for "crashkernel=X,high".
> So we'd better give it two names with union.
> 
>>
>> And if those are supported on certain architectures only, you can do
>> ifdeffery...
> 
> I don't think so. These __init functions are small and architecture-independent, and do not
> affect compilation of other architectures. There may be other architectures that use
> it in the future, such as the current arm64.
> 
>>
>> But I think I already stated that I don't like such unifications which
>> introduce unnecessary dependencies between architectures. Therefore, I
>> won't accept them into x86 unless there's a strong compelling reason.
>> Which I don't see ATM.
> 
> OK.
> 
>>
>> Thx.
>>
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH v19 01/13] kdump: add helper parse_crashkernel_high_low()

[Leizhen (ThunderTown)]

Hi, Dave, Baoquan, Borislav:
  What do you think about the introduction of parse_crashkernel_high_low()? If everyone
doesn't object, I'll bring it to the next version. But I'll make some adjustments to the
patches, see below. If there's any objection, I still strongly recommend removing the
parameters "system_ram" and "crash_base" of parse_crashkernel_{high,low}().

How about splitting __parse_crashkernel() into two parts? One for parsing
"crashkernel=X[@offset]", another one for parsing "crashkernel=X,{high,low}" and other
suffixes in the future. So the parameter requirements are clear at the lowest level.


On 2021/12/28 21:26, Zhen Lei wrote:
> The bootup command line option crashkernel=Y,low is valid only when
> crashkernel=X,high is specified. Putting their parsing into a separate
> function makes the code logic clearer and easier to understand the strong
> dependencies between them.
> 
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>  include/linux/crash_core.h |  3 +++
>  kernel/crash_core.c        | 35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> index de62a722431e7db..2d3a64761d18998 100644
> --- a/include/linux/crash_core.h
> +++ b/include/linux/crash_core.h
> @@ -83,5 +83,8 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,
>  		unsigned long long *crash_size, unsigned long long *crash_base);
>  int parse_crashkernel_low(char *cmdline, unsigned long long system_ram,
>  		unsigned long long *crash_size, unsigned long long *crash_base);
> +int __init parse_crashkernel_high_low(char *cmdline,
> +				      unsigned long long *high_size,
> +				      unsigned long long *low_size);
>  
>  #endif /* LINUX_CRASH_CORE_H */
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index eb53f5ec62c900f..8966beaf7c4fd52 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -295,6 +295,41 @@ int __init parse_crashkernel_low(char *cmdline,
>  				"crashkernel=", suffix_tbl[SUFFIX_LOW]);
>  }
>  
> +/**
> + * parse_crashkernel_high_low - Parsing "crashkernel=X,high" and possible
> + *				"crashkernel=Y,low".
> + * @cmdline:	The bootup command line.
> + * @high_size:	Save the memory size specified by "crashkernel=X,high".
> + * @low_size:	Save the memory size specified by "crashkernel=Y,low" or "-1"
> + *		if it's not specified.
> + *
> + * Returns 0 on success, else a negative status code.
> + */
> +int __init parse_crashkernel_high_low(char *cmdline,
> +				      unsigned long long *high_size,
> +				      unsigned long long *low_size)
> +{
> +	int ret;
> +	unsigned long long base;
> +
> +	BUG_ON(!high_size || !low_size);
> +
> +	/* crashkernel=X,high */
> +	ret = parse_crashkernel_high(cmdline, 0, high_size, &base);
> +	if (ret)
> +		return ret;
> +
> +	if (*high_size <= 0)
> +		return -EINVAL;
> +
> +	/* crashkernel=Y,low */
> +	ret = parse_crashkernel_low(cmdline, 0, low_size, &base);
> +	if (ret)
> +		*low_size = -1;
> +
> +	return 0;
> +}
> +
>  Elf_Word *append_elf_note(Elf_Word *buf, char *name, unsigned int type,
>  			  void *data, size_t data_len)
>  {
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH v19 01/13] kdump: add helper parse_crashkernel_high_low()

[Borislav Petkov]

On Thu, Dec 30, 2021 at 06:14:59PM +0800, Leizhen (ThunderTown) wrote:
> 
> Hi, Dave, Baoquan, Borislav:
>   What do you think about the introduction of parse_crashkernel_high_low()? If everyone
> doesn't object, I'll bring it to the next version. But I'll make some adjustments to the
> patches, see below. If there's any objection, I still strongly recommend removing the
> parameters "system_ram" and "crash_base" of parse_crashkernel_{high,low}().
> 
> How about splitting __parse_crashkernel() into two parts? One for parsing
> "crashkernel=X[@offset]", another one for parsing "crashkernel=X,{high,low}" and other
> suffixes in the future. So the parameter requirements are clear at the lowest level.

First of all, please do not top post!

Now, I already explained to you what I'd like to see:

https://lore.kernel.org/r/Ycs3kpZD/vpoo1AX@zn.tnic

yet you still don't get it.

So let me make myself clear: in its current form, this is not really an
improvement so for all x86 changes:

NAKed-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v19 01/13] kdump: add helper parse_crashkernel_high_low()

[Leizhen (ThunderTown)]

On 2021/12/30 18:40, Borislav Petkov wrote:
> On Thu, Dec 30, 2021 at 06:14:59PM +0800, Leizhen (ThunderTown) wrote:
>>
>> Hi, Dave, Baoquan, Borislav:
>>   What do you think about the introduction of parse_crashkernel_high_low()? If everyone
>> doesn't object, I'll bring it to the next version. But I'll make some adjustments to the
>> patches, see below. If there's any objection, I still strongly recommend removing the
>> parameters "system_ram" and "crash_base" of parse_crashkernel_{high,low}().
>>
>> How about splitting __parse_crashkernel() into two parts? One for parsing
>> "crashkernel=X[@offset]", another one for parsing "crashkernel=X,{high,low}" and other
>> suffixes in the future. So the parameter requirements are clear at the lowest level.
> 
> First of all, please do not top post!
> 
> Now, I already explained to you what I'd like to see:
> 
> https://lore.kernel.org/r/Ycs3kpZD/vpoo1AX@zn.tnic
> 
> yet you still don't get it.
> 
> So let me make myself clear: in its current form, this is not really an
> improvement so for all x86 changes:
> 
> NAKed-by: Borislav Petkov <bp@suse.de>
> 

OK, thanks for your immediate reply, so I can take less detours.

-- 
Regards,
  Zhen Lei

Re: [PATCH v19 01/13] kdump: add helper parse_crashkernel_high_low()

[Leizhen (ThunderTown)]

On 2021/12/30 19:08, Leizhen (ThunderTown) wrote:
> 
> 
> On 2021/12/30 18:40, Borislav Petkov wrote:
>> On Thu, Dec 30, 2021 at 06:14:59PM +0800, Leizhen (ThunderTown) wrote:
>>>
>>> Hi, Dave, Baoquan, Borislav:
>>>   What do you think about the introduction of parse_crashkernel_high_low()? If everyone
>>> doesn't object, I'll bring it to the next version. But I'll make some adjustments to the
>>> patches, see below. If there's any objection, I still strongly recommend removing the
>>> parameters "system_ram" and "crash_base" of parse_crashkernel_{high,low}().
>>>
>>> How about splitting __parse_crashkernel() into two parts? One for parsing
>>> "crashkernel=X[@offset]", another one for parsing "crashkernel=X,{high,low}" and other
>>> suffixes in the future. So the parameter requirements are clear at the lowest level.
>>
>> First of all, please do not top post!
>>
>> Now, I already explained to you what I'd like to see:
>>
>> https://lore.kernel.org/r/Ycs3kpZD/vpoo1AX@zn.tnic
>>
>> yet you still don't get it.
>>
>> So let me make myself clear: in its current form, this is not really an
>> improvement so for all x86 changes:
>>
>> NAKed-by: Borislav Petkov <bp@suse.de>

Hi Borislav:
  I'm sorry to bother you again. Do you mind if I make the following changes?
I can't stand so many comments appearing twice. Even if the size needs to be
changed in the future, mode "low_size = CRASH_LOW_SIZE_MIN + <increment>" can
be used for adaptation without affecting other architectures.


diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index e04f5e6eb33f453..da485ee51a9929e 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -428,16 +428,7 @@ static int __init reserve_crashkernel_low(void)
        /* crashkernel=Y,low */
        ret = parse_crashkernel_low(boot_command_line, low_mem_limit, &low_size, &base);
        if (ret) {
-               /*
-                * two parts from kernel/dma/swiotlb.c:
-                * -swiotlb size: user-specified with swiotlb= or default.
-                *
-                * -swiotlb overflow buffer: now hardcoded to 32k. We round it
-                * to 8M for other buffers that may need to stay low too. Also
-                * make sure we allocate enough extra low memory so that we
-                * don't run out of DMA buffers for 32-bit devices.
-                */
-               low_size = max(swiotlb_size_or_default() + (8UL << 20), 256UL << 20);
+               low_size = CRASH_LOW_SIZE_MIN;
        } else {
                /* passed with crashkernel=0,low ? */
                if (!low_size)
diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index de62a722431e7db..c85b15814312b7e 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -69,6 +69,17 @@ phys_addr_t paddr_vmcoreinfo_note(void);
 #define VMCOREINFO_CONFIG(name) \
        vmcoreinfo_append_str("CONFIG_%s=y\n", #name)

+/*
+ * two parts from kernel/dma/swiotlb.c:
+ * -swiotlb size: user-specified with swiotlb= or default.
+ *
+ * -swiotlb overflow buffer: now hardcoded to 32k. We round it
+ * to 8M for other buffers that may need to stay low too. Also
+ * make sure we allocate enough extra low memory so that we
+ * don't run out of DMA buffers for 32-bit devices.
+ */
+#define CRASH_LOW_SIZE_MIN     max(swiotlb_size_or_default() + (8UL << 20), 256UL << 20)
+
 extern unsigned char *vmcoreinfo_data;
 extern size_t vmcoreinfo_size;
 extern u32 *vmcoreinfo_note;


>>
> 
> OK, thanks for your immediate reply, so I can take less detours.
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH v19 01/13] kdump: add helper parse_crashkernel_high_low()

[Leizhen (ThunderTown)]

On 2021/12/31 17:22, Leizhen (ThunderTown) wrote:
> 
> 
> On 2021/12/30 19:08, Leizhen (ThunderTown) wrote:
>>
>>
>> On 2021/12/30 18:40, Borislav Petkov wrote:
>>> On Thu, Dec 30, 2021 at 06:14:59PM +0800, Leizhen (ThunderTown) wrote:
>>>>
>>>> Hi, Dave, Baoquan, Borislav:
>>>>   What do you think about the introduction of parse_crashkernel_high_low()? If everyone
>>>> doesn't object, I'll bring it to the next version. But I'll make some adjustments to the
>>>> patches, see below. If there's any objection, I still strongly recommend removing the
>>>> parameters "system_ram" and "crash_base" of parse_crashkernel_{high,low}().
>>>>
>>>> How about splitting __parse_crashkernel() into two parts? One for parsing
>>>> "crashkernel=X[@offset]", another one for parsing "crashkernel=X,{high,low}" and other
>>>> suffixes in the future. So the parameter requirements are clear at the lowest level.
>>>
>>> First of all, please do not top post!
>>>
>>> Now, I already explained to you what I'd like to see:
>>>
>>> https://lore.kernel.org/r/Ycs3kpZD/vpoo1AX@zn.tnic
>>>
>>> yet you still don't get it.
>>>
>>> So let me make myself clear: in its current form, this is not really an
>>> improvement so for all x86 changes:
>>>
>>> NAKed-by: Borislav Petkov <bp@suse.de>
> 
> Hi Borislav:
>   I'm sorry to bother you again. Do you mind if I make the following changes?
> I can't stand so many comments appearing twice. Even if the size needs to be
> changed in the future, mode "low_size = CRASH_LOW_SIZE_MIN + <increment>" can
> be used for adaptation without affecting other architectures.

I rethink it, the default value of default_nslabs is IO_TLB_DEFAULT_SIZE=64M.
The value of default_nslabs can only be changed by swiotlb_adjust_size() and
bootup command line option "swiotlb=". Currently, swiotlb_adjust_size() is
invoked only on x86, so I can just ignore it on arm64. Then, 64M is much
smaller than 256M, the first kernel works fine with the default 64M on arm64,
and I don't think the second kernel needs to grow to 256M. Therefore, I think
swiotlb_adjust_size() is probably a pseudo requirement for arm64.

So I will directly use 256M on arm64. If anyone gets into trouble, he/she can
add it back. Besides, there is also "crashkernel=Y,low" can be used.

> 
> 
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index e04f5e6eb33f453..da485ee51a9929e 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -428,16 +428,7 @@ static int __init reserve_crashkernel_low(void)
>         /* crashkernel=Y,low */
>         ret = parse_crashkernel_low(boot_command_line, low_mem_limit, &low_size, &base);
>         if (ret) {
> -               /*
> -                * two parts from kernel/dma/swiotlb.c:
> -                * -swiotlb size: user-specified with swiotlb= or default.
> -                *
> -                * -swiotlb overflow buffer: now hardcoded to 32k. We round it
> -                * to 8M for other buffers that may need to stay low too. Also
> -                * make sure we allocate enough extra low memory so that we
> -                * don't run out of DMA buffers for 32-bit devices.
> -                */
> -               low_size = max(swiotlb_size_or_default() + (8UL << 20), 256UL << 20);
> +               low_size = CRASH_LOW_SIZE_MIN;
>         } else {
>                 /* passed with crashkernel=0,low ? */
>                 if (!low_size)
> diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> index de62a722431e7db..c85b15814312b7e 100644
> --- a/include/linux/crash_core.h
> +++ b/include/linux/crash_core.h
> @@ -69,6 +69,17 @@ phys_addr_t paddr_vmcoreinfo_note(void);
>  #define VMCOREINFO_CONFIG(name) \
>         vmcoreinfo_append_str("CONFIG_%s=y\n", #name)
> 
> +/*
> + * two parts from kernel/dma/swiotlb.c:
> + * -swiotlb size: user-specified with swiotlb= or default.
> + *
> + * -swiotlb overflow buffer: now hardcoded to 32k. We round it
> + * to 8M for other buffers that may need to stay low too. Also
> + * make sure we allocate enough extra low memory so that we
> + * don't run out of DMA buffers for 32-bit devices.
> + */
> +#define CRASH_LOW_SIZE_MIN     max(swiotlb_size_or_default() + (8UL << 20), 256UL << 20)
> +
>  extern unsigned char *vmcoreinfo_data;
>  extern size_t vmcoreinfo_size;
>  extern u32 *vmcoreinfo_note;
> 
> 
>>>
>>
>> OK, thanks for your immediate reply, so I can take less detours.
>>
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH v19 01/13] kdump: add helper parse_crashkernel_high_low()

[john.p.donnelly]

On 12/28/21 7:26 AM, Zhen Lei wrote:
> The bootup command line option crashkernel=Y,low is valid only when
> crashkernel=X,high is specified. Putting their parsing into a separate
> function makes the code logic clearer and easier to understand the strong
> dependencies between them.
> 
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
 >
Acked-by: John Donnelly  <john.p.donnelly@oracle.com>

> ---
>   include/linux/crash_core.h |  3 +++
>   kernel/crash_core.c        | 35 +++++++++++++++++++++++++++++++++++
>   2 files changed, 38 insertions(+)
> 
> diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> index de62a722431e7db..2d3a64761d18998 100644
> --- a/include/linux/crash_core.h
> +++ b/include/linux/crash_core.h
> @@ -83,5 +83,8 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,
>   		unsigned long long *crash_size, unsigned long long *crash_base);
>   int parse_crashkernel_low(char *cmdline, unsigned long long system_ram,
>   		unsigned long long *crash_size, unsigned long long *crash_base);
> +int __init parse_crashkernel_high_low(char *cmdline,
> +				      unsigned long long *high_size,
> +				      unsigned long long *low_size);
>   
>   #endif /* LINUX_CRASH_CORE_H */
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index eb53f5ec62c900f..8966beaf7c4fd52 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -295,6 +295,41 @@ int __init parse_crashkernel_low(char *cmdline,
>   				"crashkernel=", suffix_tbl[SUFFIX_LOW]);
>   }
>   
> +/**
> + * parse_crashkernel_high_low - Parsing "crashkernel=X,high" and possible
> + *				"crashkernel=Y,low".
> + * @cmdline:	The bootup command line.
> + * @high_size:	Save the memory size specified by "crashkernel=X,high".
> + * @low_size:	Save the memory size specified by "crashkernel=Y,low" or "-1"
> + *		if it's not specified.
> + *
> + * Returns 0 on success, else a negative status code.
> + */
> +int __init parse_crashkernel_high_low(char *cmdline,
> +				      unsigned long long *high_size,
> +				      unsigned long long *low_size)
> +{
> +	int ret;
> +	unsigned long long base;
> +
> +	BUG_ON(!high_size || !low_size);
> +
> +	/* crashkernel=X,high */
> +	ret = parse_crashkernel_high(cmdline, 0, high_size, &base);
> +	if (ret)
> +		return ret;
> +
> +	if (*high_size <= 0)
> +		return -EINVAL;
> +
> +	/* crashkernel=Y,low */
> +	ret = parse_crashkernel_low(cmdline, 0, low_size, &base);
> +	if (ret)
> +		*low_size = -1;
> +
> +	return 0;
> +}
> +
>   Elf_Word *append_elf_note(Elf_Word *buf, char *name, unsigned int type,
>   			  void *data, size_t data_len)
>   {

Re: [PATCH v19 02/13] x86/setup: Use parse_crashkernel_high_low() to simplify code

[john.p.donnelly]

On 12/28/21 7:26 AM, Zhen Lei wrote:
> Use parse_crashkernel_high_low() to bring the parsing of
> "crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they
> are strongly dependent, make code logic clear and more readable.
> 
> Suggested-by: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
 >
Acked-by: John Donnelly  <john.p.donnelly@oracle.com>
> ---
>   arch/x86/kernel/setup.c | 21 +++++++++------------
>   1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 6a190c7f4d71b05..93d78aae1937db3 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -416,18 +416,16 @@ static void __init memblock_x86_reserve_range_setup_data(void)
>   # define CRASH_ADDR_HIGH_MAX	SZ_64T
>   #endif
>   
> -static int __init reserve_crashkernel_low(void)
> +static int __init reserve_crashkernel_low(unsigned long long low_size)
>   {
>   #ifdef CONFIG_X86_64
> -	unsigned long long base, low_base = 0, low_size = 0;
> +	unsigned long long low_base = 0;
>   	unsigned long low_mem_limit;
> -	int ret;
>   
>   	low_mem_limit = min(memblock_phys_mem_size(), CRASH_ADDR_LOW_MAX);
>   
> -	/* crashkernel=Y,low */
> -	ret = parse_crashkernel_low(boot_command_line, low_mem_limit, &low_size, &base);
> -	if (ret) {
> +	/* crashkernel=Y,low is not specified */
> +	if ((long)low_size < 0) {
>   		/*
>   		 * two parts from kernel/dma/swiotlb.c:
>   		 * -swiotlb size: user-specified with swiotlb= or default.
> @@ -465,7 +463,7 @@ static int __init reserve_crashkernel_low(void)
>   
>   static void __init reserve_crashkernel(void)
>   {
> -	unsigned long long crash_size, crash_base, total_mem;
> +	unsigned long long crash_size, crash_base, total_mem, low_size;
>   	bool high = false;
>   	int ret;
>   
> @@ -474,10 +472,9 @@ static void __init reserve_crashkernel(void)
>   	/* crashkernel=XM */
>   	ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base);
>   	if (ret != 0 || crash_size <= 0) {
> -		/* crashkernel=X,high */
> -		ret = parse_crashkernel_high(boot_command_line, total_mem,
> -					     &crash_size, &crash_base);
> -		if (ret != 0 || crash_size <= 0)
> +		/* crashkernel=X,high and possible crashkernel=Y,low */
> +		ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size);
> +		if (ret)
>   			return;
>   		high = true;
>   	}
> @@ -520,7 +517,7 @@ static void __init reserve_crashkernel(void)
>   		}
>   	}
>   
> -	if (crash_base >= (1ULL << 32) && reserve_crashkernel_low()) {
> +	if (crash_base >= (1ULL << 32) && reserve_crashkernel_low(low_size)) {
>   		memblock_phys_free(crash_base, crash_size);
>   		return;
>   	}

Re: [PATCH v19 03/13] kdump: make parse_crashkernel_{high|low}() static

[john.p.donnelly]

On 12/28/21 7:26 AM, Zhen Lei wrote:
> Make parse_crashkernel_{high|low}() static, they are only referenced by
> parse_crashkernel_high_low() in the same file. The latter is recommended.
> 
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
 >
Acked-by: John Donnelly  <john.p.donnelly@oracle.com>

> ---
>   include/linux/crash_core.h | 4 ----
>   kernel/crash_core.c        | 4 ++--
>   2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> index 2d3a64761d18998..598fd55d83c169e 100644
> --- a/include/linux/crash_core.h
> +++ b/include/linux/crash_core.h
> @@ -79,10 +79,6 @@ void final_note(Elf_Word *buf);
>   
>   int __init parse_crashkernel(char *cmdline, unsigned long long system_ram,
>   		unsigned long long *crash_size, unsigned long long *crash_base);
> -int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,
> -		unsigned long long *crash_size, unsigned long long *crash_base);
> -int parse_crashkernel_low(char *cmdline, unsigned long long system_ram,
> -		unsigned long long *crash_size, unsigned long long *crash_base);
>   int __init parse_crashkernel_high_low(char *cmdline,
>   				      unsigned long long *high_size,
>   				      unsigned long long *low_size);
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index 8966beaf7c4fd52..3b9e01fc450b2a4 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -277,7 +277,7 @@ int __init parse_crashkernel(char *cmdline,
>   					"crashkernel=", NULL);
>   }
>   
> -int __init parse_crashkernel_high(char *cmdline,
> +static int __init parse_crashkernel_high(char *cmdline,
>   			     unsigned long long system_ram,
>   			     unsigned long long *crash_size,
>   			     unsigned long long *crash_base)
> @@ -286,7 +286,7 @@ int __init parse_crashkernel_high(char *cmdline,
>   				"crashkernel=", suffix_tbl[SUFFIX_HIGH]);
>   }
>   
> -int __init parse_crashkernel_low(char *cmdline,
> +static int __init parse_crashkernel_low(char *cmdline,
>   			     unsigned long long system_ram,
>   			     unsigned long long *crash_size,
>   			     unsigned long long *crash_base)

Re: [PATCH v19 04/13] kdump: reduce unnecessary parameters of parse_crashkernel_{high|low}()

[john.p.donnelly]

On 12/28/21 7:26 AM, Zhen Lei wrote:
> Delete confusing parameters 'system_ram' and 'crash_base' of
> parse_crashkernel_{high|low}(), they are only needed by the case of
> "crashkernel=X@[offset]".
> 
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
 >
Acked-by: John Donnelly  <john.p.donnelly@oracle.com>

> ---
>   kernel/crash_core.c | 21 ++++++++++-----------
>   1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index 3b9e01fc450b2a4..b7d024eb464d0ae 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -278,20 +278,20 @@ int __init parse_crashkernel(char *cmdline,
>   }
>   
>   static int __init parse_crashkernel_high(char *cmdline,
> -			     unsigned long long system_ram,
> -			     unsigned long long *crash_size,
> -			     unsigned long long *crash_base)
> +					 unsigned long long *crash_size)
>   {
> -	return __parse_crashkernel(cmdline, system_ram, crash_size, crash_base,
> +	unsigned long long base;
> +
> +	return __parse_crashkernel(cmdline, 0, crash_size, &base,
>   				"crashkernel=", suffix_tbl[SUFFIX_HIGH]);
>   }
>   
>   static int __init parse_crashkernel_low(char *cmdline,
> -			     unsigned long long system_ram,
> -			     unsigned long long *crash_size,
> -			     unsigned long long *crash_base)
> +					unsigned long long *crash_size)
>   {
> -	return __parse_crashkernel(cmdline, system_ram, crash_size, crash_base,
> +	unsigned long long base;
> +
> +	return __parse_crashkernel(cmdline, 0, crash_size, &base,
>   				"crashkernel=", suffix_tbl[SUFFIX_LOW]);
>   }
>   
> @@ -310,12 +310,11 @@ int __init parse_crashkernel_high_low(char *cmdline,
>   				      unsigned long long *low_size)
>   {
>   	int ret;
> -	unsigned long long base;
>   
>   	BUG_ON(!high_size || !low_size);
>   
>   	/* crashkernel=X,high */
> -	ret = parse_crashkernel_high(cmdline, 0, high_size, &base);
> +	ret = parse_crashkernel_high(cmdline, high_size);
>   	if (ret)
>   		return ret;
>   
> @@ -323,7 +322,7 @@ int __init parse_crashkernel_high_low(char *cmdline,
>   		return -EINVAL;
>   
>   	/* crashkernel=Y,low */
> -	ret = parse_crashkernel_low(cmdline, 0, low_size, &base);
> +	ret = parse_crashkernel_low(cmdline, low_size);
>   	if (ret)
>   		*low_size = -1;
>   

Re: [PATCH v19 05/13] x86/setup: Add and use CRASH_BASE_ALIGN

[john.p.donnelly]

On 12/28/21 7:26 AM, Zhen Lei wrote:
> Add macro CRASH_BASE_ALIGN to indicate the alignment for crash kernel
> fixed region, in preparation for making partial implementation of
> reserve_crashkernel[_low]() generic.
> 
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>

 >
Acked-by: John Donnelly  <john.p.donnelly@oracle.com>

> ---
>   arch/x86/kernel/setup.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 93d78aae1937db3..cb7f237a2ae0dfa 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -392,9 +392,12 @@ static void __init memblock_x86_reserve_range_setup_data(void)
>   
>   #ifdef CONFIG_KEXEC_CORE
>   
> -/* 16M alignment for crash kernel regions */
> +/* alignment for crash kernel dynamic regions */
>   #define CRASH_ALIGN		SZ_16M
>   
> +/* alignment for crash kernel fixed region */
> +#define CRASH_BASE_ALIGN	SZ_1M
> +
>   /*
>    * Keep the crash kernel below this limit.
>    *
> @@ -509,7 +512,7 @@ static void __init reserve_crashkernel(void)
>   	} else {
>   		unsigned long long start;
>   
> -		start = memblock_phys_alloc_range(crash_size, SZ_1M, crash_base,
> +		start = memblock_phys_alloc_range(crash_size, CRASH_BASE_ALIGN, crash_base,
>   						  crash_base + crash_size);
>   		if (start != crash_base) {
>   			pr_info("crashkernel reservation failed - memory is in use.\n");

Re: [PATCH v19 06/13] kexec: move crashk[_low]_res to crash_core module

[john.p.donnelly]

On 12/28/21 7:26 AM, Zhen Lei wrote:
> From: Chen Zhou <chenzhou10@huawei.com>
> 
> Move the definition and declaration of global variable crashk[_low]_res
> from kexec module to crash_core module, in preparation of adding generic
> reserve_crashkernel_mem[_low]() to crash_core.c, the latter refers to
> variable crashk[_low]_res. Due to the config KEXEC automatically selects
> CRASH_CORE, and the header crash_core.h is included by kexec.h, so there
> is no functional change.
> 
> Signed-off-by: Chen Zhou <chenzhou10@huawei.com>
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>

 >
Acked-by: John Donnelly  <john.p.donnelly@oracle.com>

> ---
>   include/linux/crash_core.h |  4 ++++
>   include/linux/kexec.h      |  4 ----
>   kernel/crash_core.c        | 16 ++++++++++++++++
>   kernel/kexec_core.c        | 17 -----------------
>   4 files changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> index 598fd55d83c169e..f5437c9c9411fce 100644
> --- a/include/linux/crash_core.h
> +++ b/include/linux/crash_core.h
> @@ -73,6 +73,10 @@ extern unsigned char *vmcoreinfo_data;
>   extern size_t vmcoreinfo_size;
>   extern u32 *vmcoreinfo_note;
>   
> +/* Location of a reserved region to hold the crash kernel. */
> +extern struct resource crashk_res;
> +extern struct resource crashk_low_res;
> +
>   Elf_Word *append_elf_note(Elf_Word *buf, char *name, unsigned int type,
>   			  void *data, size_t data_len);
>   void final_note(Elf_Word *buf);
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 0c994ae37729e1e..47e784d66ea8645 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -350,10 +350,6 @@ extern int kexec_load_disabled;
>   #define KEXEC_FILE_FLAGS	(KEXEC_FILE_UNLOAD | KEXEC_FILE_ON_CRASH | \
>   				 KEXEC_FILE_NO_INITRAMFS)
>   
> -/* Location of a reserved region to hold the crash kernel.
> - */
> -extern struct resource crashk_res;
> -extern struct resource crashk_low_res;
>   extern note_buf_t __percpu *crash_notes;
>   
>   /* flag to track if kexec reboot is in progress */
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index b7d024eb464d0ae..686d8a65e12a337 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -22,6 +22,22 @@ u32 *vmcoreinfo_note;
>   /* trusted vmcoreinfo, e.g. we can make a copy in the crash memory */
>   static unsigned char *vmcoreinfo_data_safecopy;
>   
> +/* Location of the reserved area for the crash kernel */
> +struct resource crashk_res = {
> +	.name  = "Crash kernel",
> +	.start = 0,
> +	.end   = 0,
> +	.flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM,
> +	.desc  = IORES_DESC_CRASH_KERNEL
> +};
> +struct resource crashk_low_res = {
> +	.name  = "Crash kernel",
> +	.start = 0,
> +	.end   = 0,
> +	.flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM,
> +	.desc  = IORES_DESC_CRASH_KERNEL
> +};
> +
>   /*
>    * parsing the "crashkernel" commandline
>    *
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index 5a5d192a89ac307..1e0d4909bbb6b77 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -54,23 +54,6 @@ note_buf_t __percpu *crash_notes;
>   /* Flag to indicate we are going to kexec a new kernel */
>   bool kexec_in_progress = false;
>   
> -
> -/* Location of the reserved area for the crash kernel */
> -struct resource crashk_res = {
> -	.name  = "Crash kernel",
> -	.start = 0,
> -	.end   = 0,
> -	.flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM,
> -	.desc  = IORES_DESC_CRASH_KERNEL
> -};
> -struct resource crashk_low_res = {
> -	.name  = "Crash kernel",
> -	.start = 0,
> -	.end   = 0,
> -	.flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM,
> -	.desc  = IORES_DESC_CRASH_KERNEL
> -};
> -
>   int kexec_should_crash(struct task_struct *p)
>   {
>   	/*

Re: [PATCH v19 11/13] arm64: kdump: reimplement crashkernel=X

[Dave Kleikamp]

On 12/28/21 7:26AM, Zhen Lei wrote:
> From: Chen Zhou <chenzhou10@huawei.com>
> 
> There are following issues in arm64 kdump:
> 1. We use crashkernel=X to reserve crashkernel below 4G, which
> will fail when there is no enough low memory.
> 2. If reserving crashkernel above 4G, in this case, crash dump
> kernel will boot failure because there is no low memory available
> for allocation.
> 
> To solve these issues, change the behavior of crashkernel=X and
> introduce crashkernel=X,[high,low]. crashkernel=X tries low allocation
> in DMA zone, and fall back to high allocation if it fails.
> We can also use "crashkernel=X,high" to select a region above DMA zone,
> which also tries to allocate at least 256M in DMA zone automatically.
> "crashkernel=Y,low" can be used to allocate specified size low memory.
> 
> Another minor change, there may be two regions reserved for crash
> dump kernel, in order to distinct from the high region and make no
> effect to the use of existing kexec-tools, rename the low region as
> "Crash kernel (low)".
> 
> Signed-off-by: Chen Zhou <chenzhou10@huawei.com>
> Co-developed-by: Zhen Lei <thunder.leizhen@huawei.com>
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>   arch/arm64/kernel/machine_kexec.c      |  5 +++-
>   arch/arm64/kernel/machine_kexec_file.c | 12 ++++++--
>   arch/arm64/kernel/setup.c              | 13 +++++++-
>   arch/arm64/mm/init.c                   | 41 ++++++++++----------------
>   4 files changed, 42 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c
> index 6fb31c117ebe08c..6665bf31f6b6a19 100644
> --- a/arch/arm64/kernel/machine_kexec.c
> +++ b/arch/arm64/kernel/machine_kexec.c
> @@ -327,7 +327,10 @@ bool crash_is_nosave(unsigned long pfn)
>   
>   	/* in reserved memory? */
>   	addr = __pfn_to_phys(pfn);
> -	if ((addr < crashk_res.start) || (crashk_res.end < addr))
> +	if (((addr < crashk_res.start) || (crashk_res.end < addr)) && !crashk_low_res.end)
> +		return false;
> +
> +	if ((addr < crashk_low_res.start) || (crashk_low_res.end < addr))
>   		return false;
>   
>   	if (!kexec_crash_image)
> diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
> index 59c648d51848886..889951291cc0f9c 100644
> --- a/arch/arm64/kernel/machine_kexec_file.c
> +++ b/arch/arm64/kernel/machine_kexec_file.c
> @@ -65,10 +65,18 @@ static int prepare_elf_headers(void **addr, unsigned long *sz)
>   
>   	/* Exclude crashkernel region */
>   	ret = crash_exclude_mem_range(cmem, crashk_res.start, crashk_res.end);
> +	if (ret)
> +		goto out;
> +
> +	if (crashk_low_res.end) {
> +		ret = crash_exclude_mem_range(cmem, crashk_low_res.start, crashk_low_res.end);
> +		if (ret)
> +			goto out;
> +	}
>   
> -	if (!ret)
> -		ret =  crash_prepare_elf64_headers(cmem, true, addr, sz);
> +	ret = crash_prepare_elf64_headers(cmem, true, addr, sz);
>   
> +out:
>   	kfree(cmem);
>   	return ret;
>   }
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index be5f85b0a24de69..4bb2e55366be64d 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -248,7 +248,18 @@ static void __init request_standard_resources(void)
>   		    kernel_data.end <= res->end)
>   			request_resource(res, &kernel_data);
>   #ifdef CONFIG_KEXEC_CORE
> -		/* Userspace will find "Crash kernel" region in /proc/iomem. */
> +		/*
> +		 * Userspace will find "Crash kernel" or "Crash kernel (low)"
> +		 * region in /proc/iomem.
> +		 * In order to distinct from the high region and make no effect
> +		 * to the use of existing kexec-tools, rename the low region as
> +		 * "Crash kernel (low)".
> +		 */
> +		if (crashk_low_res.end && crashk_low_res.start >= res->start &&
> +				crashk_low_res.end <= res->end) {
> +			crashk_low_res.name = "Crash kernel (low)";
> +			request_resource(res, &crashk_low_res);
> +		}
>   		if (crashk_res.end && crashk_res.start >= res->start &&
>   		    crashk_res.end <= res->end)
>   			request_resource(res, &crashk_res);
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index be4595dc7459115..91b8038a1529068 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -74,41 +74,32 @@ phys_addr_t arm64_dma_phys_limit __ro_after_init;
>    */
>   static void __init reserve_crashkernel(void)
>   {
> -	unsigned long long crash_base, crash_size;
> -	unsigned long long crash_max = CRASH_ADDR_LOW_MAX;
> +	unsigned long long crash_size, crash_base, total_mem, low_size;

low_size needs to be initialized to -1.

If parse_crashkernel() succeeds, then an uninitialized low_size will be 
passed to reserve_crashkernel_mem().

> +	bool high = false;
>   	int ret;
>   
> -	ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
> -				&crash_size, &crash_base);
> -	/* no crashkernel= or invalid value specified */
> -	if (ret || !crash_size)
> -		return;
> -
> -	crash_size = PAGE_ALIGN(crash_size);
> -
> -	/* User specifies base address explicitly. */
> -	if (crash_base)
> -		crash_max = crash_base + crash_size;
> +	total_mem = memblock_phys_mem_size();
>   
> -	/* Current arm64 boot protocol requires 2MB alignment */
> -	crash_base = memblock_phys_alloc_range(crash_size, CRASH_ALIGN,
> -					       crash_base, crash_max);
> -	if (!crash_base) {
> -		pr_warn("cannot allocate crashkernel (size:0x%llx)\n",
> -			crash_size);
> -		return;
> +	ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base);
> +	if (ret != 0 || crash_size <= 0) {
> +		/* crashkernel=X,high and possible crashkernel=Y,low */
> +		ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size);
> +		if (ret)
> +			return;
> +		high = true;
>   	}
>   
> -	pr_info("crashkernel reserved: 0x%016llx - 0x%016llx (%lld MB)\n",
> -		crash_base, crash_base + crash_size, crash_size >> 20);
> +	ret = reserve_crashkernel_mem(total_mem, crash_size, crash_base, low_size, high);
> +	if (ret)
> +		return;
>   
>   	/*
>   	 * The crashkernel memory will be removed from the kernel linear
>   	 * map. Inform kmemleak so that it won't try to access it.
>   	 */
> -	kmemleak_ignore_phys(crash_base);
> -	crashk_res.start = crash_base;
> -	crashk_res.end = crash_base + crash_size - 1;
> +	kmemleak_ignore_phys(crashk_res.start);
> +	if (crashk_low_res.end)
> +		kmemleak_ignore_phys(crashk_low_res.start);
>   }
>   #else
>   static void __init reserve_crashkernel(void)

Re: [PATCH v19 11/13] arm64: kdump: reimplement crashkernel=X

[Leizhen (ThunderTown)]

On 2022/1/12 22:45, Dave Kleikamp wrote:
> On 12/28/21 7:26AM, Zhen Lei wrote:
>> From: Chen Zhou <chenzhou10@huawei.com>
>>
>> There are following issues in arm64 kdump:
>> 1. We use crashkernel=X to reserve crashkernel below 4G, which
>> will fail when there is no enough low memory.
>> 2. If reserving crashkernel above 4G, in this case, crash dump
>> kernel will boot failure because there is no low memory available
>> for allocation.
>>
>> To solve these issues, change the behavior of crashkernel=X and
>> introduce crashkernel=X,[high,low]. crashkernel=X tries low allocation
>> in DMA zone, and fall back to high allocation if it fails.
>> We can also use "crashkernel=X,high" to select a region above DMA zone,
>> which also tries to allocate at least 256M in DMA zone automatically.
>> "crashkernel=Y,low" can be used to allocate specified size low memory.
>>
>> Another minor change, there may be two regions reserved for crash
>> dump kernel, in order to distinct from the high region and make no
>> effect to the use of existing kexec-tools, rename the low region as
>> "Crash kernel (low)".
>>
>> Signed-off-by: Chen Zhou <chenzhou10@huawei.com>
>> Co-developed-by: Zhen Lei <thunder.leizhen@huawei.com>
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>>   arch/arm64/kernel/machine_kexec.c      |  5 +++-
>>   arch/arm64/kernel/machine_kexec_file.c | 12 ++++++--
>>   arch/arm64/kernel/setup.c              | 13 +++++++-
>>   arch/arm64/mm/init.c                   | 41 ++++++++++----------------
>>   4 files changed, 42 insertions(+), 29 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c
>> index 6fb31c117ebe08c..6665bf31f6b6a19 100644
>> --- a/arch/arm64/kernel/machine_kexec.c
>> +++ b/arch/arm64/kernel/machine_kexec.c
>> @@ -327,7 +327,10 @@ bool crash_is_nosave(unsigned long pfn)
>>         /* in reserved memory? */
>>       addr = __pfn_to_phys(pfn);
>> -    if ((addr < crashk_res.start) || (crashk_res.end < addr))
>> +    if (((addr < crashk_res.start) || (crashk_res.end < addr)) && !crashk_low_res.end)
>> +        return false;
>> +
>> +    if ((addr < crashk_low_res.start) || (crashk_low_res.end < addr))
>>           return false;
>>         if (!kexec_crash_image)
>> diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
>> index 59c648d51848886..889951291cc0f9c 100644
>> --- a/arch/arm64/kernel/machine_kexec_file.c
>> +++ b/arch/arm64/kernel/machine_kexec_file.c
>> @@ -65,10 +65,18 @@ static int prepare_elf_headers(void **addr, unsigned long *sz)
>>         /* Exclude crashkernel region */
>>       ret = crash_exclude_mem_range(cmem, crashk_res.start, crashk_res.end);
>> +    if (ret)
>> +        goto out;
>> +
>> +    if (crashk_low_res.end) {
>> +        ret = crash_exclude_mem_range(cmem, crashk_low_res.start, crashk_low_res.end);
>> +        if (ret)
>> +            goto out;
>> +    }
>>   -    if (!ret)
>> -        ret =  crash_prepare_elf64_headers(cmem, true, addr, sz);
>> +    ret = crash_prepare_elf64_headers(cmem, true, addr, sz);
>>   +out:
>>       kfree(cmem);
>>       return ret;
>>   }
>> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
>> index be5f85b0a24de69..4bb2e55366be64d 100644
>> --- a/arch/arm64/kernel/setup.c
>> +++ b/arch/arm64/kernel/setup.c
>> @@ -248,7 +248,18 @@ static void __init request_standard_resources(void)
>>               kernel_data.end <= res->end)
>>               request_resource(res, &kernel_data);
>>   #ifdef CONFIG_KEXEC_CORE
>> -        /* Userspace will find "Crash kernel" region in /proc/iomem. */
>> +        /*
>> +         * Userspace will find "Crash kernel" or "Crash kernel (low)"
>> +         * region in /proc/iomem.
>> +         * In order to distinct from the high region and make no effect
>> +         * to the use of existing kexec-tools, rename the low region as
>> +         * "Crash kernel (low)".
>> +         */
>> +        if (crashk_low_res.end && crashk_low_res.start >= res->start &&
>> +                crashk_low_res.end <= res->end) {
>> +            crashk_low_res.name = "Crash kernel (low)";
>> +            request_resource(res, &crashk_low_res);
>> +        }
>>           if (crashk_res.end && crashk_res.start >= res->start &&
>>               crashk_res.end <= res->end)
>>               request_resource(res, &crashk_res);
>> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
>> index be4595dc7459115..91b8038a1529068 100644
>> --- a/arch/arm64/mm/init.c
>> +++ b/arch/arm64/mm/init.c
>> @@ -74,41 +74,32 @@ phys_addr_t arm64_dma_phys_limit __ro_after_init;
>>    */
>>   static void __init reserve_crashkernel(void)
>>   {
>> -    unsigned long long crash_base, crash_size;
>> -    unsigned long long crash_max = CRASH_ADDR_LOW_MAX;
>> +    unsigned long long crash_size, crash_base, total_mem, low_size;
> 
> low_size needs to be initialized to -1.
> 
> If parse_crashkernel() succeeds, then an uninitialized low_size will be passed to reserve_crashkernel_mem().

Right, thanks, I noticed that too. I'm waiting for v5.17-rc1 to release v20.

In addition, I found that the current implementation on x86 was problematic in case
"crashkernel=4G crashkernel=512M,low". According to the document, "crashkernel=512M,low"
should not take effect at this case. But reserve_crashkernel_low() didn't do that well.

> 
>> +    bool high = false;
>>       int ret;
>>   -    ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
>> -                &crash_size, &crash_base);
>> -    /* no crashkernel= or invalid value specified */
>> -    if (ret || !crash_size)
>> -        return;
>> -
>> -    crash_size = PAGE_ALIGN(crash_size);
>> -
>> -    /* User specifies base address explicitly. */
>> -    if (crash_base)
>> -        crash_max = crash_base + crash_size;
>> +    total_mem = memblock_phys_mem_size();
>>   -    /* Current arm64 boot protocol requires 2MB alignment */
>> -    crash_base = memblock_phys_alloc_range(crash_size, CRASH_ALIGN,
>> -                           crash_base, crash_max);
>> -    if (!crash_base) {
>> -        pr_warn("cannot allocate crashkernel (size:0x%llx)\n",
>> -            crash_size);
>> -        return;
>> +    ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base);
>> +    if (ret != 0 || crash_size <= 0) {
>> +        /* crashkernel=X,high and possible crashkernel=Y,low */
>> +        ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size);
>> +        if (ret)
>> +            return;
>> +        high = true;
>>       }
>>   -    pr_info("crashkernel reserved: 0x%016llx - 0x%016llx (%lld MB)\n",
>> -        crash_base, crash_base + crash_size, crash_size >> 20);
>> +    ret = reserve_crashkernel_mem(total_mem, crash_size, crash_base, low_size, high);
>> +    if (ret)
>> +        return;
>>         /*
>>        * The crashkernel memory will be removed from the kernel linear
>>        * map. Inform kmemleak so that it won't try to access it.
>>        */
>> -    kmemleak_ignore_phys(crash_base);
>> -    crashk_res.start = crash_base;
>> -    crashk_res.end = crash_base + crash_size - 1;
>> +    kmemleak_ignore_phys(crashk_res.start);
>> +    if (crashk_low_res.end)
>> +        kmemleak_ignore_phys(crashk_low_res.start);
>>   }
>>   #else
>>   static void __init reserve_crashkernel(void)
> 
> .
> 

-- 
Regards,
  Zhen Lei