Skip to contentSkip to conversation

x86/setup/crash: Cleanup some code

PATCH In review Borislav Petkov 3 participants 10 messages Oct 15, 2015

Subsystems (2)
x86 kexec
lore
Share & tools
lore

Borislav Petkov

author maintainer
#0 lore raw

cover x86/setup/crash: Cleanup some code

From: Borislav Petkov <[email protected]>

Hi all,

this is ontop of Baoquan's fix from yesterday:
https://lkml.kernel.org/r/[email protected]

Looking at the crashkernel reservation code made a couple of style problems wink at me so this short cleanup should address them. Please take a look and complain if something's wrong.

Thanks.

Borislav Petkov (5):
x86/setup: Cleanup crashkernel reservation functions x86/setup/crash: Remove alignment variable x86/setup/crash: Cleanup some more x86/setup/crash: Check memblock_reserve() retval kexec/crash: Say which char is the unrecognized

 arch/x86/kernel/setup.c | 81 +++++++++++++++++++++++++------------------------
 kernel/kexec_core.c     |  6 ++--
 2 files changed, 45 insertions(+), 42 deletions(-)
Signature · 1 line
2.3.5
author maintainer
#1 lore raw

PATCH 1/5 x86/setup: Cleanup crashkernel reservation functions

From: Borislav Petkov <[email protected]>

* Shorten variable names
* Realign code, space out for better readability

No code changed:

# arch/x86/kernel/setup.o:

   text    data     bss     dec     hex filename
   4543    3096   69904   77543   12ee7 setup.o.before
   4543    3096   69904   77543   12ee7 setup.o.after

md5:
8a1b7c6738a553ca207b56bd84a8f359 setup.o.before.asm 8a1b7c6738a553ca207b56bd84a8f359 setup.o.after.asm

Cc Andrew Morton <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Dave Young <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc [email protected]
Cc Jiri Kosina <[email protected]>
Cc Joerg Roedel <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Mark Salter <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc WANG Chao <[email protected]>
Cc x86-ml <[email protected]>
Signed-off-by Borislav Petkov <[email protected]>
---
 arch/x86/kernel/setup.c | 43 +++++++++++++++++++++----------------------
 1 file changed, 21 insertions(+), 22 deletions(-)
arch/x86/kernel/setup.c +21 -22
index 1b36839e41eb..fd9e178aa890 100644
@@ -486,11 +486,11 @@ static void __init memblock_x86_reserve_range_setup_data(void)
  * On 64bit, old kexec-tools need to under 896MiB.
  */
 #ifdef CONFIG_X86_32
-# define CRASH_KERNEL_ADDR_LOW_MAX	(512 << 20)
-# define CRASH_KERNEL_ADDR_HIGH_MAX	(512 << 20)
+# define CRASH_ADDR_LOW_MAX	(512 << 20)
+# define CRASH_ADDR_HIGH_MAX	(512 << 20)
 #else
-# define CRASH_KERNEL_ADDR_LOW_MAX	(896UL<<20)
-# define CRASH_KERNEL_ADDR_HIGH_MAX	MAXMEM
+# define CRASH_ADDR_LOW_MAX	(896UL << 20)
+# define CRASH_ADDR_HIGH_MAX	MAXMEM
 #endif
 
 static int __init reserve_crashkernel_low(void)

@@ -503,10 +503,10 @@ static int __init reserve_crashkernel_low(void)
 	bool auto_set = false;
 	int ret;
 
-	total_low_mem = memblock_mem_size(1UL<<(32-PAGE_SHIFT));
+	total_low_mem = memblock_mem_size(1UL << (32 - PAGE_SHIFT));
+
 	/* crashkernel=Y,low */
-	ret = parse_crashkernel_low(boot_command_line, total_low_mem,
-						&low_size, &base);
+	ret = parse_crashkernel_low(boot_command_line, total_low_mem, &low_size, &base);
 	if (ret != 0) {
 		/*
 		 * two parts from lib/swiotlb.c:

@@ -517,7 +517,7 @@ static int __init reserve_crashkernel_low(void)
 		 * 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 = max(swiotlb_size_or_default() + (8UL << 20), 256UL << 20);
 		auto_set = true;
 	} else {
 		/* passed with crashkernel=0,low ? */

@@ -525,9 +525,7 @@ static int __init reserve_crashkernel_low(void)
 			return 0;
 	}
 
-	low_base = memblock_find_in_range(low_size, (1ULL<<32),
-					low_size, alignment);
-
+	low_base = memblock_find_in_range(low_size, 1ULL << 32, low_size, alignment);
 	if (!low_base) {
 		pr_err("Cannot reserve %ldMB crashkernel low memory, please try smaller size.\n",
 		       (unsigned long)(low_size >> 20));

@@ -535,10 +533,12 @@ static int __init reserve_crashkernel_low(void)
 	}
 
 	memblock_reserve(low_base, low_size);
+
 	pr_info("Reserving %ldMB of low memory at %ldMB for crashkernel (System low RAM: %ldMB)\n",
-			(unsigned long)(low_size >> 20),
-			(unsigned long)(low_base >> 20),
-			(unsigned long)(total_low_mem >> 20));
+		(unsigned long)(low_size >> 20),
+		(unsigned long)(low_base >> 20),
+		(unsigned long)(total_low_mem >> 20));
+
 	crashk_low_res.start = low_base;
 	crashk_low_res.end   = low_base + low_size - 1;
 	insert_resource(&iomem_resource, &crashk_low_res);

@@ -557,12 +557,11 @@ static void __init reserve_crashkernel(void)
 	total_mem = memblock_phys_mem_size();
 
 	/* crashkernel=XM */
-	ret = parse_crashkernel(boot_command_line, total_mem,
-			&crash_size, &crash_base);
+	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);
+					     &crash_size, &crash_base);
 		if (ret != 0 || crash_size <= 0)
 			return;
 		high = true;

@@ -574,10 +573,9 @@ static void __init reserve_crashkernel(void)
 		 *  kexec want bzImage is below CRASH_KERNEL_ADDR_MAX
 		 */
 		crash_base = memblock_find_in_range(alignment,
-					high ? CRASH_KERNEL_ADDR_HIGH_MAX :
-					       CRASH_KERNEL_ADDR_LOW_MAX,
-					crash_size, alignment);
-
+						    high ? CRASH_ADDR_HIGH_MAX
+							 : CRASH_ADDR_LOW_MAX,
+						    crash_size, alignment);
 		if (!crash_base) {
 			pr_info("crashkernel reservation failed - No suitable area found.\n");
 			return;

@@ -587,7 +585,8 @@ static void __init reserve_crashkernel(void)
 		unsigned long long start;
 
 		start = memblock_find_in_range(crash_base,
-				 crash_base + crash_size, crash_size, 1<<20);
+					       crash_base + crash_size,
+					       crash_size, 1 << 20);
 		if (start != crash_base) {
 			pr_info("crashkernel reservation failed - memory is in use.\n");
 			return;
Signature · 1 line
2.3.5
author maintainer
#2 lore raw

PATCH 2/5 x86/setup/crash: Remove alignment variable

In reply to Borislav Petkov

From: Borislav Petkov <[email protected]>

Use a macro instead. No functionality change.

Cc Andrew Morton <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Dave Young <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc [email protected]
Cc Jiri Kosina <[email protected]>
Cc Joerg Roedel <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Mark Salter <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc WANG Chao <[email protected]>
Cc x86-ml <[email protected]>
Signed-off-by Borislav Petkov <[email protected]>
---
 arch/x86/kernel/setup.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
arch/x86/kernel/setup.c +6 -5
index fd9e178aa890..ea086dd8e821 100644
@@ -480,6 +480,9 @@ static void __init memblock_x86_reserve_range_setup_data(void)
 
 #ifdef CONFIG_KEXEC_CORE
 
+/* 16M alignment for crash kernel regions */
+#define CRASH_ALIGN		(16 << 20)
+
 /*
  * Keep the crash kernel below this limit.  On 32 bits earlier kernels
  * would limit the kernel to the low 512 MiB due to mapping restrictions.

@@ -496,7 +499,6 @@ static void __init memblock_x86_reserve_range_setup_data(void)
 static int __init reserve_crashkernel_low(void)
 {
 #ifdef CONFIG_X86_64
-	const unsigned long long alignment = 16<<20;	/* 16M */
 	unsigned long long low_base = 0, low_size = 0;
 	unsigned long total_low_mem;
 	unsigned long long base;

@@ -525,7 +527,7 @@ static int __init reserve_crashkernel_low(void)
 			return 0;
 	}
 
-	low_base = memblock_find_in_range(low_size, 1ULL << 32, low_size, alignment);
+	low_base = memblock_find_in_range(low_size, 1ULL << 32, low_size, CRASH_ALIGN);
 	if (!low_base) {
 		pr_err("Cannot reserve %ldMB crashkernel low memory, please try smaller size.\n",
 		       (unsigned long)(low_size >> 20));

@@ -548,7 +550,6 @@ static int __init reserve_crashkernel_low(void)
 
 static void __init reserve_crashkernel(void)
 {
-	const unsigned long long alignment = 16<<20;	/* 16M */
 	unsigned long long total_mem;
 	unsigned long long crash_size, crash_base;
 	bool high = false;

@@ -572,10 +573,10 @@ static void __init reserve_crashkernel(void)
 		/*
 		 *  kexec want bzImage is below CRASH_KERNEL_ADDR_MAX
 		 */
-		crash_base = memblock_find_in_range(alignment,
+		crash_base = memblock_find_in_range(CRASH_ALIGN,
 						    high ? CRASH_ADDR_HIGH_MAX
 							 : CRASH_ADDR_LOW_MAX,
-						    crash_size, alignment);
+						    crash_size, CRASH_ALIGN);
 		if (!crash_base) {
 			pr_info("crashkernel reservation failed - No suitable area found.\n");
 			return;
Signature · 1 line
2.3.5
author maintainer
#3 lore raw

PATCH 3/5 x86/setup/crash: Cleanup some more

In reply to Borislav Petkov

From: Borislav Petkov <[email protected]>

* Remove unused auto_set variable
* Cleanup local function variable declarations
* Reformat printk string and use pr_info()

No functionality change.

Cc Andrew Morton <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Dave Young <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc [email protected]
Cc Jiri Kosina <[email protected]>
Cc Joerg Roedel <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Mark Salter <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc WANG Chao <[email protected]>
Cc x86-ml <[email protected]>
Signed-off-by Borislav Petkov <[email protected]>
---
 arch/x86/kernel/setup.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)
arch/x86/kernel/setup.c +7 -12
index ea086dd8e821..d4788719a1e2 100644
@@ -499,17 +499,15 @@ static void __init memblock_x86_reserve_range_setup_data(void)
 static int __init reserve_crashkernel_low(void)
 {
 #ifdef CONFIG_X86_64
-	unsigned long long low_base = 0, low_size = 0;
+	unsigned long long base, low_base = 0, low_size = 0;
 	unsigned long total_low_mem;
-	unsigned long long base;
-	bool auto_set = false;
 	int ret;
 
 	total_low_mem = memblock_mem_size(1UL << (32 - PAGE_SHIFT));
 
 	/* crashkernel=Y,low */
 	ret = parse_crashkernel_low(boot_command_line, total_low_mem, &low_size, &base);
-	if (ret != 0) {
+	if (ret) {
 		/*
 		 * two parts from lib/swiotlb.c:
 		 * -swiotlb size: user-specified with swiotlb= or default.

@@ -520,7 +518,6 @@ static int __init reserve_crashkernel_low(void)
 		 * don't run out of DMA buffers for 32-bit devices.
 		 */
 		low_size = max(swiotlb_size_or_default() + (8UL << 20), 256UL << 20);
-		auto_set = true;
 	} else {
 		/* passed with crashkernel=0,low ? */
 		if (!low_size)

@@ -550,8 +547,7 @@ static int __init reserve_crashkernel_low(void)
 
 static void __init reserve_crashkernel(void)
 {
-	unsigned long long total_mem;
-	unsigned long long crash_size, crash_base;
+	unsigned long long crash_size, crash_base, total_mem;
 	bool high = false;
 	int ret;
 

@@ -600,11 +596,10 @@ static void __init reserve_crashkernel(void)
 		return;
 	}
 
-	printk(KERN_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));
+	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;
Signature · 1 line
2.3.5
author maintainer
#4 lore raw

PATCH 4/5 x86/setup/crash: Check memblock_reserve() retval

In reply to Borislav Petkov

From: Borislav Petkov <[email protected]>

memblock_reserve() can fail but the crashkernel reservation code doesn't check that and this can lead the user into believing that the crashkernel region was actually reserved. Make sure we check that return value and we exit early with a failure message in the error case.

Cc Andrew Morton <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Dave Young <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc [email protected]
Cc Jiri Kosina <[email protected]>
Cc Joerg Roedel <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Mark Salter <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc WANG Chao <[email protected]>
Cc x86-ml <[email protected]>
Signed-off-by Borislav Petkov <[email protected]>
---
 arch/x86/kernel/setup.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
arch/x86/kernel/setup.c +10 -2
index d4788719a1e2..3f75297d5fd0 100644
@@ -531,7 +531,11 @@ static int __init reserve_crashkernel_low(void)
 		return -ENOMEM;
 	}
 
-	memblock_reserve(low_base, low_size);
+	ret = memblock_reserve(low_base, low_size);
+	if (ret) {
+		pr_err("%s: Error reserving crashkernel low memblock.\n", __func__);
+		return ret;
+	}
 
 	pr_info("Reserving %ldMB of low memory at %ldMB for crashkernel (System low RAM: %ldMB)\n",
 		(unsigned long)(low_size >> 20),

@@ -589,7 +593,11 @@ static void __init reserve_crashkernel(void)
 			return;
 		}
 	}
-	memblock_reserve(crash_base, crash_size);
+	ret = memblock_reserve(crash_base, crash_size);
+	if (ret) {
+		pr_err("%s: Error reserving crashkernel memblock.\n", __func__);
+		return;
+	}
 
 	if (crash_base >= (1ULL << 32) && reserve_crashkernel_low()) {
 		memblock_free(crash_base, crash_size);
Signature · 1 line
2.3.5
#5 lore raw

Re: [PATCH 4/5] x86/setup/crash: Check memblock_reserve() retval

Quoted 30 lines · Borislav Petkov

From: Borislav Petkov <[email protected]>

memblock_reserve() can fail but the crashkernel reservation code doesn't check that and this can lead the user into believing that the crashkernel region was actually reserved. Make sure we check that return value and we exit early with a failure message in the error case.

Cc Andrew Morton <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Dave Young <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc [email protected]
Cc Jiri Kosina <[email protected]>
Cc Joerg Roedel <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Mark Salter <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc WANG Chao <[email protected]>
Cc x86-ml <[email protected]>
Signed-off-by Borislav Petkov <[email protected]>
---
 arch/x86/kernel/setup.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
arch/x86/kernel/setup.c +0 -0
index d4788719a1e2..3f75297d5fd0 100644

arch/x86/kernel/setup.c static int __init reserve_crashkernel_low(void) · line 539

Quoted 10 lines
arch/x86/kernel/setup.c +5 -1
@@ -531,7 +531,11 @@ static int __init reserve_crashkernel_low(void)
 		return -ENOMEM;
 	}
 
-	memblock_reserve(low_base, low_size);
+	ret = memblock_reserve(low_base, low_size);
+	if (ret) {
+		pr_err("%s: Error reserving crashkernel low memblock.\n", __func__);
+		return ret;
+	}

Seems there's no checking for other callback to memblock_reserve in setup.c Need another cleanup?

BTW, a further cleanup is reasonable to me, there's a lot of below patter:
memblock_find_in_range
error checking
memblock_reserve
error checking

So a new function memblock_reserve_in_range is reasonable.

Thanks
Dave

author maintainer
#6 lore raw

Re: [PATCH 4/5] x86/setup/crash: Check memblock_reserve() retval

Quoted 2 lines · Dave Young

Seems there's no checking for other callback to memblock_reserve in setup.c Need another cleanup?

True story. It sure does.

Quoted 7 lines

BTW, a further cleanup is reasonable to me, there's a lot of below patter:
memblock_find_in_range
error checking
memblock_reserve
error checking

So a new function memblock_reserve_in_range is reasonable.

Well, in some of the callsites, the first "error checking" issues a specific message, depending on the subsystem. The following memblock_reserve() is mostly unchecked though. At least that should be fixed...

Signature · 4 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
author maintainer
#7 lore raw

PATCH 5/5 kexec/crash: Say which char is the unrecognized

In reply to Borislav Petkov

From: Borislav Petkov <[email protected]>

It is helpful when the crashkernel cmdline parsing routines actually say which character is the unrecognized one. Make them do so.

Cc Andrew Morton <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Baoquan He <[email protected]>
Cc Dave Young <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc [email protected]
Cc Jiri Kosina <[email protected]>
Cc Joerg Roedel <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Mark Salter <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Vivek Goyal <[email protected]>
Cc WANG Chao <[email protected]>
Cc x86-ml <[email protected]>
Signed-off-by Borislav Petkov <[email protected]>
---
 kernel/kexec_core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
kernel/kexec_core.c +3 -3
index 201b45327804..bd9f8a03cefa 100644
@@ -1149,7 +1149,7 @@ static int __init parse_crashkernel_simple(char *cmdline,
 	if (*cur == '@')
 		*crash_base = memparse(cur+1, &cur);
 	else if (*cur != ' ' && *cur != '\0') {
-		pr_warn("crashkernel: unrecognized char\n");
+		pr_warn("crashkernel: unrecognized char: %c\n", *cur);
 		return -EINVAL;
 	}
 

@@ -1186,12 +1186,12 @@ static int __init parse_crashkernel_suffix(char *cmdline,
 
 	/* check with suffix */
 	if (strncmp(cur, suffix, strlen(suffix))) {
-		pr_warn("crashkernel: unrecognized char\n");
+		pr_warn("crashkernel: unrecognized char: %c\n", *cur);
 		return -EINVAL;
 	}
 	cur += strlen(suffix);
 	if (*cur != ' ' && *cur != '\0') {
-		pr_warn("crashkernel: unrecognized char\n");
+		pr_warn("crashkernel: unrecognized char: %c\n", *cur);
 		return -EINVAL;
 	}
Signature · 1 line
2.3.5
Reviewed-by on the v1 series
#8 lore raw
In reply to Borislav Petkov
Quoted 20 lines · Borislav Petkov

From: Borislav Petkov <[email protected]>

Hi all,

this is ontop of Baoquan's fix from yesterday:
https://lkml.kernel.org/r/[email protected]

Looking at the crashkernel reservation code made a couple of style problems wink at me so this short cleanup should address them. Please take a look and complain if something's wrong.

Thanks.

Borislav Petkov (5):
x86/setup: Cleanup crashkernel reservation functions x86/setup/crash: Remove alignment variable x86/setup/crash: Cleanup some more x86/setup/crash: Check memblock_reserve() retval kexec/crash: Say which char is the unrecognized

Quoted 8 lines
 arch/x86/kernel/setup.c | 81 +++++++++++++++++++++++++------------------------
 kernel/kexec_core.c     |  6 ++--
 2 files changed, 45 insertions(+), 42 deletions(-)
Signature · 1 line
2.3.5
Reviewed-by Dave Young <[email protected]>

Thanks
Dave

Reviewed-by on the v1 series
#9 lore raw
In reply to Borislav Petkov
Quoted 16 lines · Borislav Petkov

this is ontop of Baoquan's fix from yesterday:
https://lkml.kernel.org/r/[email protected]

Looking at the crashkernel reservation code made a couple of style problems wink at me so this short cleanup should address them. Please take a look and complain if something's wrong.

Thanks.

Borislav Petkov (5):
x86/setup: Cleanup crashkernel reservation functions x86/setup/crash: Remove alignment variable x86/setup/crash: Cleanup some more x86/setup/crash: Check memblock_reserve() retval kexec/crash: Say which char is the unrecognized

Quoted 3 lines
 arch/x86/kernel/setup.c | 81 +++++++++++++++++++++++++------------------------
 kernel/kexec_core.c     |  6 ++--
 2 files changed, 45 insertions(+), 42 deletions(-)

Looks good to me.

Reviewed-by Joerg Roedel <[email protected]>