[PATCH -v2 0/4] x86, kdump: Fix crashkernel high with old kexec-tools

[PATCH -v2 0/4] x86, kdump: Fix crashkernel high with old kexec-tools

[Yinghai Lu]

Vivek found some problems with old kexec-tools.

We keep the old crashkernel=X to old behavoir, so it will not break
old kexec-tools.
Add crashkernel=X,high to support new kexec-tools that supports loading high.
when high is used, memblock will search from top to low.
if the allocated one is above 4G, kernel will try to auto allocate
72M under 4G for swiotlb.
user could crashkernel=Y,low to change 72M to other value.

-v2:	reorder the patch sequences
	crashkernel=X,high, crashkernel=Y,low only handle simple form.
	crashkernel=X will override crashkernel=X;high crashkernel=Y;low

Thanks

Yinghai

[PATCH v2 1/4] x86, kdump: Set crashkernel_low automatically

[Yinghai Lu]

Chao said that kdump does does work well on his system on 3.8
without extra parameter, even iommu does not work with kdump.
And now have to append crashkernel_low=Y in first kernel to make
kdump work.

We have now modified crashkernel=X to allocate memory beyong 4G (if
available) and do not allocate low range for crashkernel if the user
does not specify that with crashkernel_low=Y.  This causes regression
if iommu is not enabled.  Without iommu, swiotlb needs to be setup in
first 4G and there is no low memory available to second kernel.

Set crashkernel_low automatically if the user does not specify that.

For system that does support IOMMU with kdump properly, user could
specify crashkernel_low=0 to save that 72M low ram.

-v3: add swiotlb_size() according to Konrad.
-v4: add comments what 8M is for according to hpa.
     also update more crashkernel_low= in kernel-parameters.txt
-v5: update changelog according to Vivek.

Reported-by: WANG Chao <chaowang@redhat.com>
Tested-by: WANG Chao <chaowang@redhat.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 Documentation/kernel-parameters.txt |   15 ++++++++++++---
 arch/x86/kernel/setup.c             |   20 +++++++++++++++++---
 include/linux/swiotlb.h             |    1 +
 lib/swiotlb.c                       |   19 +++++++++++++++----
 4 files changed, 45 insertions(+), 10 deletions(-)

Index: linux-2.6/arch/x86/kernel/setup.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/setup.c
+++ linux-2.6/arch/x86/kernel/setup.c
@@ -521,19 +521,33 @@ static void __init reserve_crashkernel_l
 	unsigned long long 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));
 	ret = parse_crashkernel_low(boot_command_line, total_low_mem,
 						&low_size, &base);
-	if (ret != 0 || low_size <= 0)
-		return;
+	if (ret != 0) {
+		/*
+		 * two parts from lib/swiotlb.c:
+		 *	swiotlb size: user specified with swiotlb= or default.
+		 *	swiotlb overflow buffer: now is hardcoded to 32k,
+		 *		round to 8M to cover more others.
+		 */
+		low_size = swiotlb_size_or_default() + (8UL<<20);
+		auto_set = true;
+	} else {
+		/* passed with crashkernel_low=0 ? */
+		if (!low_size)
+			return;
+	}
 
 	low_base = memblock_find_in_range(low_size, (1ULL<<32),
 					low_size, alignment);
 
 	if (!low_base) {
-		pr_info("crashkernel low reservation failed - No suitable area found.\n");
+		if (!auto_set)
+			pr_info("crashkernel low reservation failed - No suitable area found.\n");
 
 		return;
 	}
Index: linux-2.6/include/linux/swiotlb.h
===================================================================
--- linux-2.6.orig/include/linux/swiotlb.h
+++ linux-2.6/include/linux/swiotlb.h
@@ -25,6 +25,7 @@ extern int swiotlb_force;
 extern void swiotlb_init(int verbose);
 int swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose);
 extern unsigned long swiotlb_nr_tbl(void);
+unsigned long swiotlb_size_or_default(void);
 extern int swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs);
 
 /*
Index: linux-2.6/lib/swiotlb.c
===================================================================
--- linux-2.6.orig/lib/swiotlb.c
+++ linux-2.6/lib/swiotlb.c
@@ -105,9 +105,9 @@ setup_io_tlb_npages(char *str)
 	if (!strcmp(str, "force"))
 		swiotlb_force = 1;
 
-	return 1;
+	return 0;
 }
-__setup("swiotlb=", setup_io_tlb_npages);
+early_param("swiotlb", setup_io_tlb_npages);
 /* make io_tlb_overflow tunable too? */
 
 unsigned long swiotlb_nr_tbl(void)
@@ -115,6 +115,18 @@ unsigned long swiotlb_nr_tbl(void)
 	return io_tlb_nslabs;
 }
 EXPORT_SYMBOL_GPL(swiotlb_nr_tbl);
+
+/* default to 64MB */
+#define IO_TLB_DEFAULT_SIZE (64UL<<20)
+unsigned long swiotlb_size_or_default(void)
+{
+	unsigned long size;
+
+	size = io_tlb_nslabs << IO_TLB_SHIFT;
+
+	return size ? size : (IO_TLB_DEFAULT_SIZE);
+}
+
 /* Note that this doesn't work with highmem page */
 static dma_addr_t swiotlb_virt_to_bus(struct device *hwdev,
 				      volatile void *address)
@@ -188,8 +200,7 @@ int __init swiotlb_init_with_tbl(char *t
 void  __init
 swiotlb_init(int verbose)
 {
-	/* default to 64MB */
-	size_t default_size = 64UL<<20;
+	size_t default_size = IO_TLB_DEFAULT_SIZE;
 	unsigned char *vstart;
 	unsigned long bytes;
 
Index: linux-2.6/Documentation/kernel-parameters.txt
===================================================================
--- linux-2.6.orig/Documentation/kernel-parameters.txt
+++ linux-2.6/Documentation/kernel-parameters.txt
@@ -596,9 +596,6 @@ bytes respectively. Such letter suffixes
 			is selected automatically. Check
 			Documentation/kdump/kdump.txt for further details.
 
-	crashkernel_low=size[KMG]
-			[KNL, x86] parts under 4G.
-
 	crashkernel=range1:size1[,range2:size2,...][@offset]
 			[KNL] Same as above, but depends on the memory
 			in the running system. The syntax of range is
@@ -606,6 +603,18 @@ bytes respectively. Such letter suffixes
 			a memory unit (amount[KMG]). See also
 			Documentation/kdump/kdump.txt for an example.
 
+	crashkernel_low=size[KMG]
+			[KNL, x86_64] range under 4G. When crashkernel= is
+			passed, kernel allocate physical memory region
+			above 4G, that cause second kernel crash on system
+			that need swiotlb later. Kernel would try to allocate
+			some region below 4G automatically. This one let
+			user to specify own low range under 4G for second
+			kernel instead.
+			0: to disable low allocation on systems that do not
+			need swiotlb, that will save 72M low ram in first
+			kernel.
+
 	cs89x0_dma=	[HW,NET]
 			Format: <dma>
 

[PATCH v2 3/4] kexec: use Crash kernel for Crash kernel low

[Yinghai Lu]

We can extend kexec-tools to support multiple "Crash kernel" in /proc/iomem
instead.

So we can use "Crash kernel" instead of "Crash kernel low" in /proc/iomem.

Suggested-by: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 kernel/kexec.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/kernel/kexec.c
===================================================================
--- linux-2.6.orig/kernel/kexec.c
+++ linux-2.6/kernel/kexec.c
@@ -55,7 +55,7 @@ struct resource crashk_res = {
 	.flags = IORESOURCE_BUSY | IORESOURCE_MEM
 };
 struct resource crashk_low_res = {
-	.name  = "Crash kernel low",
+	.name  = "Crash kernel",
 	.start = 0,
 	.end   = 0,
 	.flags = IORESOURCE_BUSY | IORESOURCE_MEM

[PATCH v2 2/4] x86, kdump: Retore crashkernel= to allocate low

[Yinghai Lu]

Vivek found old kexec-tools does not work new kernel anymore.

So change back crashkernel= back to old behavoir, and add crashkernel_high=
to let user decide if buffer could be above 4G, and also new kexec-tools will
be needed.

v2: let crashkernel=X override crashkernel_high=
    update description about _high will be ignored by crashkernel=X

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 Documentation/kernel-parameters.txt |   11 +++++++++--
 arch/x86/kernel/setup.c             |   24 +++++++++++++++++++-----
 include/linux/kexec.h               |    2 ++
 kernel/kexec.c                      |    9 +++++++++
 4 files changed, 39 insertions(+), 7 deletions(-)

Index: linux-2.6/Documentation/kernel-parameters.txt
===================================================================
--- linux-2.6.orig/Documentation/kernel-parameters.txt
+++ linux-2.6/Documentation/kernel-parameters.txt
@@ -603,9 +603,14 @@ bytes respectively. Such letter suffixes
 			a memory unit (amount[KMG]). See also
 			Documentation/kdump/kdump.txt for an example.
 
+	crashkernel_high=size[KMG]
+			[KNL, x86_64] range could be above 4G. Allow kernel
+			to allocate physical memory region from top, so could
+			be above 4G if system have more than 4G ram installed.
+			It will be ignored if crashkernel=X is specified.
 	crashkernel_low=size[KMG]
-			[KNL, x86_64] range under 4G. When crashkernel= is
-			passed, kernel allocate physical memory region
+			[KNL, x86_64] range under 4G. When crashkernel_high= is
+			passed, kernel could allocate physical memory region
 			above 4G, that cause second kernel crash on system
 			that need swiotlb later. Kernel would try to allocate
 			some region below 4G automatically. This one let
@@ -614,6 +619,8 @@ bytes respectively. Such letter suffixes
 			0: to disable low allocation on systems that do not
 			need swiotlb, that will save 72M low ram in first
 			kernel.
+			It will be ignored when crashkernel_high=X is not used
+			or return from that is below 4G.
 
 	cs89x0_dma=	[HW,NET]
 			Format: <dma>
Index: linux-2.6/arch/x86/kernel/setup.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/setup.c
+++ linux-2.6/arch/x86/kernel/setup.c
@@ -505,11 +505,14 @@ static void __init memblock_x86_reserve_
 /*
  * 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.
+ * On 64bit, old kexec-tools need to under 896MiB.
  */
 #ifdef CONFIG_X86_32
-# define CRASH_KERNEL_ADDR_MAX	(512 << 20)
+# define CRASH_KERNEL_ADDR_LOW_MAX	(512 << 20)
+# define CRASH_KERNEL_ADDR_HIGH_MAX	(512 << 20)
 #else
-# define CRASH_KERNEL_ADDR_MAX	MAXMEM
+# define CRASH_KERNEL_ADDR_LOW_MAX	(896UL<<20)
+# define CRASH_KERNEL_ADDR_HIGH_MAX	MAXMEM
 #endif
 
 static void __init reserve_crashkernel_low(void)
@@ -523,6 +526,7 @@ static void __init reserve_crashkernel_l
 	int ret;
 
 	total_low_mem = memblock_mem_size(1UL<<(32-PAGE_SHIFT));
+	/* crashkernel_low=YM */
 	ret = parse_crashkernel_low(boot_command_line, total_low_mem,
 						&low_size, &base);
 	if (ret != 0) {
@@ -566,14 +570,22 @@ static void __init reserve_crashkernel(v
 	const unsigned long long alignment = 16<<20;	/* 16M */
 	unsigned long long total_mem;
 	unsigned long long crash_size, crash_base;
+	bool high = false;
 	int ret;
 
 	total_mem = memblock_phys_mem_size();
 
+	/* crashkernel=XM */
 	ret = parse_crashkernel(boot_command_line, total_mem,
 			&crash_size, &crash_base);
-	if (ret != 0 || crash_size <= 0)
-		return;
+	if (ret != 0 || crash_size <= 0) {
+		/* crashkernel_high=XM */
+		ret = parse_crashkernel_high(boot_command_line, total_mem,
+				&crash_size, &crash_base);
+		if (ret != 0 || crash_size <= 0)
+			return;
+		high = true;
+	}
 
 	/* 0 means: find the address automatically */
 	if (crash_base <= 0) {
@@ -581,7 +593,9 @@ static void __init reserve_crashkernel(v
 		 *  kexec want bzImage is below CRASH_KERNEL_ADDR_MAX
 		 */
 		crash_base = memblock_find_in_range(alignment,
-			       CRASH_KERNEL_ADDR_MAX, crash_size, alignment);
+					high ? CRASH_KERNEL_ADDR_HIGH_MAX :
+					       CRASH_KERNEL_ADDR_LOW_MAX,
+					crash_size, alignment);
 
 		if (!crash_base) {
 			pr_info("crashkernel reservation failed - No suitable area found.\n");
Index: linux-2.6/include/linux/kexec.h
===================================================================
--- linux-2.6.orig/include/linux/kexec.h
+++ linux-2.6/include/linux/kexec.h
@@ -200,6 +200,8 @@ extern size_t vmcoreinfo_max_size;
 
 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 crash_shrink_memory(unsigned long new_size);
Index: linux-2.6/kernel/kexec.c
===================================================================
--- linux-2.6.orig/kernel/kexec.c
+++ linux-2.6/kernel/kexec.c
@@ -1422,6 +1422,15 @@ int __init parse_crashkernel(char *cmdli
 					"crashkernel=");
 }
 
+int __init parse_crashkernel_high(char *cmdline,
+			     unsigned long long system_ram,
+			     unsigned long long *crash_size,
+			     unsigned long long *crash_base)
+{
+	return __parse_crashkernel(cmdline, system_ram, crash_size, crash_base,
+					"crashkernel_high=");
+}
+
 int __init parse_crashkernel_low(char *cmdline,
 			     unsigned long long system_ram,
 			     unsigned long long *crash_size,

[PATCH v2 4/4] x86, kdump: Change crashkernel_high/low= to crashkernel=;high/low

[Yinghai Lu]

Per hpa, use crashkernel=XM;high crashkernel=YM;low instead of
crashkernel_hign=XM crashkernel_low=YM. As that could be extensible.

-v2: according to Vivek, change delimiter to ;
-v3: let hign and low only handle simple form and it conforms to
	description in kernel-parameters.txt
     still keep crashkernel=X override any crashkernel=X,high
        crashkernel=Y,low

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 Documentation/kernel-parameters.txt |   10 +--
 arch/x86/kernel/setup.c             |    6 +-
 kernel/kexec.c                      |   94 +++++++++++++++++++++++++++---------
 3 files changed, 80 insertions(+), 30 deletions(-)

Index: linux-2.6/Documentation/kernel-parameters.txt
===================================================================
--- linux-2.6.orig/Documentation/kernel-parameters.txt
+++ linux-2.6/Documentation/kernel-parameters.txt
@@ -603,14 +603,14 @@ bytes respectively. Such letter suffixes
 			a memory unit (amount[KMG]). See also
 			Documentation/kdump/kdump.txt for an example.
 
-	crashkernel_high=size[KMG]
+	crashkernel=size[KMG];high
 			[KNL, x86_64] range could be above 4G. Allow kernel
 			to allocate physical memory region from top, so could
 			be above 4G if system have more than 4G ram installed.
 			It will be ignored if crashkernel=X is specified.
-	crashkernel_low=size[KMG]
-			[KNL, x86_64] range under 4G. When crashkernel_high= is
-			passed, kernel could allocate physical memory region
+	crashkernel=size[KMG];low
+			[KNL, x86_64] range under 4G. When crashkernel=X;high
+			is passed, kernel could allocate physical memory region
 			above 4G, that cause second kernel crash on system
 			that need swiotlb later. Kernel would try to allocate
 			some region below 4G automatically. This one let
@@ -619,7 +619,7 @@ bytes respectively. Such letter suffixes
 			0: to disable low allocation on systems that do not
 			need swiotlb, that will save 72M low ram in first
 			kernel.
-			It will be ignored when crashkernel_high=X is not used
+			It will be ignored when crashkernel=X;high is not used
 			or return from that is below 4G.
 
 	cs89x0_dma=	[HW,NET]
Index: linux-2.6/arch/x86/kernel/setup.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/setup.c
+++ linux-2.6/arch/x86/kernel/setup.c
@@ -526,7 +526,7 @@ static void __init reserve_crashkernel_l
 	int ret;
 
 	total_low_mem = memblock_mem_size(1UL<<(32-PAGE_SHIFT));
-	/* crashkernel_low=YM */
+	/* crashkernel=YM;low */
 	ret = parse_crashkernel_low(boot_command_line, total_low_mem,
 						&low_size, &base);
 	if (ret != 0) {
@@ -539,7 +539,7 @@ static void __init reserve_crashkernel_l
 		low_size = swiotlb_size_or_default() + (8UL<<20);
 		auto_set = true;
 	} else {
-		/* passed with crashkernel_low=0 ? */
+		/* passed with crashkernel=0;low ? */
 		if (!low_size)
 			return;
 	}
@@ -579,7 +579,7 @@ static void __init reserve_crashkernel(v
 	ret = parse_crashkernel(boot_command_line, total_mem,
 			&crash_size, &crash_base);
 	if (ret != 0 || crash_size <= 0) {
-		/* crashkernel_high=XM */
+		/* crashkernel=XM;high */
 		ret = parse_crashkernel_high(boot_command_line, total_mem,
 				&crash_size, &crash_base);
 		if (ret != 0 || crash_size <= 0)
Index: linux-2.6/kernel/kexec.c
===================================================================
--- linux-2.6.orig/kernel/kexec.c
+++ linux-2.6/kernel/kexec.c
@@ -1360,7 +1360,7 @@ static int __init parse_crashkernel_simp
 
 	if (*cur == '@')
 		*crash_base = memparse(cur+1, &cur);
-	else if (*cur != ' ' && *cur != '\0') {
+	else if (*cur != ' ' && *cur != ';' && *cur != '\0') {
 		pr_warning("crashkernel: unrecognized char\n");
 		return -EINVAL;
 	}
@@ -1368,58 +1368,108 @@ static int __init parse_crashkernel_simp
 	return 0;
 }
 
-/*
- * That function is the entry point for command line parsing and should be
- * called from the arch-specific code.
- */
+#define SUFFIX_HIGH 0
+#define SUFFIX_LOW  1
+#define SUFFIX_NULL 2
+static __initdata char *suffix_tbl[] = {
+	[SUFFIX_HIGH] = ";high",
+	[SUFFIX_LOW]  = ";low",
+	[SUFFIX_NULL] = NULL,
+};
+
+static __init char *get_last_crashkernel(char *cmdline,
+			     const char *name,
+			     const char *suffix)
+{
+	char *p = cmdline, *ck_cmdline = NULL;
+
+	/* find crashkernel and use the last one if there are more */
+	p = strstr(p, name);
+	while (p) {
+		char *end_p = strchr(p, ' ');
+		char *q;
+
+		if (!end_p)
+			end_p = p + strlen(p);
+
+		if (!suffix) {
+			int i;
+
+			/* skip the one with any known suffix */
+			for (i = 0; suffix_tbl[i]; i++) {
+				q = end_p - strlen(suffix_tbl[i]);
+				if (!strncmp(q, suffix_tbl[i],
+					     strlen(suffix_tbl[i])))
+					goto next;
+			}
+			ck_cmdline = p;
+		} else {
+			q = end_p - strlen(suffix);
+			if (!strncmp(q, suffix, strlen(suffix)))
+				ck_cmdline = p;
+		}
+next:
+		p = strstr(p+1, name);
+	}
+
+	if (!ck_cmdline)
+		return NULL;
+
+	ck_cmdline += strlen(name);
+
+	return ck_cmdline;
+}
+
 static int __init __parse_crashkernel(char *cmdline,
 			     unsigned long long system_ram,
 			     unsigned long long *crash_size,
 			     unsigned long long *crash_base,
-				const char *name)
+			     const char *name,
+			     const char *suffix,
+			     bool simple_only)
 {
-	char 	*p = cmdline, *ck_cmdline = NULL;
 	char	*first_colon, *first_space;
+	char	*ck_cmdline;
 
 	BUG_ON(!crash_size || !crash_base);
 	*crash_size = 0;
 	*crash_base = 0;
 
-	/* find crashkernel and use the last one if there are more */
-	p = strstr(p, name);
-	while (p) {
-		ck_cmdline = p;
-		p = strstr(p+1, name);
-	}
+	ck_cmdline = get_last_crashkernel(cmdline, name, suffix);
 
 	if (!ck_cmdline)
 		return -EINVAL;
 
-	ck_cmdline += strlen(name);
-
 	/*
 	 * if the commandline contains a ':', then that's the extended
 	 * syntax -- if not, it must be the classic syntax
 	 */
 	first_colon = strchr(ck_cmdline, ':');
 	first_space = strchr(ck_cmdline, ' ');
-	if (first_colon && (!first_space || first_colon < first_space))
-		return parse_crashkernel_mem(ck_cmdline, system_ram,
-				crash_size, crash_base);
-	else
+	if (first_colon && (!first_space || first_colon < first_space)) {
+		if (simple_only)
+			return -EINVAL;
+		else
+			return parse_crashkernel_mem(ck_cmdline, system_ram,
+					crash_size, crash_base);
+	} else
 		return parse_crashkernel_simple(ck_cmdline, crash_size,
 				crash_base);
 
 	return 0;
 }
 
+/*
+ * That function is the entry point for command line parsing and should be
+ * called from the arch-specific code.
+ */
 int __init parse_crashkernel(char *cmdline,
 			     unsigned long long system_ram,
 			     unsigned long long *crash_size,
 			     unsigned long long *crash_base)
 {
 	return __parse_crashkernel(cmdline, system_ram, crash_size, crash_base,
-					"crashkernel=");
+					"crashkernel=", NULL, false);
 }
 
 int __init parse_crashkernel_high(char *cmdline,
@@ -1428,7 +1478,7 @@ int __init parse_crashkernel_high(char *
 			     unsigned long long *crash_base)
 {
 	return __parse_crashkernel(cmdline, system_ram, crash_size, crash_base,
-					"crashkernel_high=");
+				"crashkernel=", suffix_tbl[SUFFIX_HIGH], true);
 }
 
 int __init parse_crashkernel_low(char *cmdline,
@@ -1437,7 +1487,7 @@ int __init parse_crashkernel_low(char *c
 			     unsigned long long *crash_base)
 {
 	return __parse_crashkernel(cmdline, system_ram, crash_size, crash_base,
-					"crashkernel_low=");
+				"crashkernel=", suffix_tbl[SUFFIX_LOW], true);
 }
 
 static void update_vmcoreinfo_note(void)

Re: [PATCH v2 4/4] x86, kdump: Change crashkernel_high/low= to crashkernel=;high/low

[HATAYAMA Daisuke]

(2013/04/04 9:38), Yinghai Lu wrote:

> Index: linux-2.6/kernel/kexec.c
> ===================================================================
> --- linux-2.6.orig/kernel/kexec.c
> +++ linux-2.6/kernel/kexec.c
> @@ -1360,7 +1360,7 @@ static int __init parse_crashkernel_simp
>   
>   	if (*cur == '@')
>   		*crash_base = memparse(cur+1, &cur);
> -	else if (*cur != ' ' && *cur != '\0') {
> +	else if (*cur != ' ' && *cur != ';' && *cur != '\0') {
>   		pr_warning("crashkernel: unrecognized char\n");
>   		return -EINVAL;
>   	}

As I said below, ";high" or ";low" check should be here. It would be
enough to replace the condition *cur != ';' by strncmp(cur, ";high", 5)
|| strncmp(cur, ";low", 4).

> @@ -1368,58 +1368,108 @@ static int __init parse_crashkernel_simp
>   	return 0;
>   }
>   
> -/*
> - * That function is the entry point for command line parsing and should be
> - * called from the arch-specific code.
> - */
> +#define SUFFIX_HIGH 0
> +#define SUFFIX_LOW  1
> +#define SUFFIX_NULL 2
> +static __initdata char *suffix_tbl[] = {
> +	[SUFFIX_HIGH] = ";high",
> +	[SUFFIX_LOW]  = ";low",
> +	[SUFFIX_NULL] = NULL,
> +};
> +
> +static __init char *get_last_crashkernel(char *cmdline,
> +			     const char *name,
> +			     const char *suffix)
> +{
> +	char *p = cmdline, *ck_cmdline = NULL;
> +
> +	/* find crashkernel and use the last one if there are more */

Why did you choose the last one? Is there any reason you didn't choose
the first one?

Also, it's better to describe this bahaviour in
Documentations/kernel-parameter.txt.

> +	p = strstr(p, name);
> +	while (p) {
> +		char *end_p = strchr(p, ' ');
> +		char *q;
> +
> +		if (!end_p)
> +			end_p = p + strlen(p);
> +
> +		if (!suffix) {
> +			int i;
> +
> +			/* skip the one with any known suffix */
> +			for (i = 0; suffix_tbl[i]; i++) {
> +				q = end_p - strlen(suffix_tbl[i]);
> +				if (!strncmp(q, suffix_tbl[i],
> +					     strlen(suffix_tbl[i])))
> +					goto next;
> +			}
> +			ck_cmdline = p;
> +		} else {
> +			q = end_p - strlen(suffix);
> +			if (!strncmp(q, suffix, strlen(suffix)))
> +				ck_cmdline = p;
> +		}

It looks to me that this function does more than its name suggests. It
seems to me enough to get the last occurence of "crashkernel=<some
value>" and to leave the "<some value>" unknown for now.

The current code of yours checks if each "crashkernel=<some value>"
detected by strstr() ends with each of ";high" or ";low", but doesn't
check the formeter letters at all; e.g, "crashkernel=foobar;high" is passed.

Also, this function can be called in different contexts: from a variant
of parse_crashkernel_*(). Is it better to move this function in
reserve_crashkernel() and then pass the obtained "crashkernel=<some
value>" to a variant of parse_crashkernel_*() functions?

Thanks.
HATAYAMA, Daisuke

Re: [PATCH v2 1/4] x86, kdump: Set crashkernel_low automatically

[HATAYAMA Daisuke]

(2013/04/04 9:38), Yinghai Lu wrote:

> Index: linux-2.6/lib/swiotlb.c
> ===================================================================
> --- linux-2.6.orig/lib/swiotlb.c
> +++ linux-2.6/lib/swiotlb.c
> @@ -105,9 +105,9 @@ setup_io_tlb_npages(char *str)
>   	if (!strcmp(str, "force"))
>   		swiotlb_force = 1;
>   
> -	return 1;
> +	return 0;
>   }
> -__setup("swiotlb=", setup_io_tlb_npages);
> +early_param("swiotlb", setup_io_tlb_npages);
>   /* make io_tlb_overflow tunable too? */
>   
>   unsigned long swiotlb_nr_tbl(void)
> @@ -115,6 +115,18 @@ unsigned long swiotlb_nr_tbl(void)
>   	return io_tlb_nslabs;
>   }
>   EXPORT_SYMBOL_GPL(swiotlb_nr_tbl);
> +
> +/* default to 64MB */
> +#define IO_TLB_DEFAULT_SIZE (64UL<<20)
> +unsigned long swiotlb_size_or_default(void)
> +{
> +	unsigned long size;
> +
> +	size = io_tlb_nslabs << IO_TLB_SHIFT;
> +
> +	return size ? size : (IO_TLB_DEFAULT_SIZE);
> +}
> +
>   /* Note that this doesn't work with highmem page */
>   static dma_addr_t swiotlb_virt_to_bus(struct device *hwdev,
>   				      volatile void *address)
> @@ -188,8 +200,7 @@ int __init swiotlb_init_with_tbl(char *t
>   void  __init
>   swiotlb_init(int verbose)
>   {
> -	/* default to 64MB */
> -	size_t default_size = 64UL<<20;
> +	size_t default_size = IO_TLB_DEFAULT_SIZE;
>   	unsigned char *vstart;
>   	unsigned long bytes;

The variable default_size seems no longer necessary. IO_TLB_DEFAULT_SIZE
should be used directly.

>   
> Index: linux-2.6/Documentation/kernel-parameters.txt
> ===================================================================
> --- linux-2.6.orig/Documentation/kernel-parameters.txt
> +++ linux-2.6/Documentation/kernel-parameters.txt
> @@ -596,9 +596,6 @@ bytes respectively. Such letter suffixes
>   			is selected automatically. Check
>   			Documentation/kdump/kdump.txt for further details.
>   
> -	crashkernel_low=size[KMG]
> -			[KNL, x86] parts under 4G.
> -
>   	crashkernel=range1:size1[,range2:size2,...][@offset]
>   			[KNL] Same as above, but depends on the memory
>   			in the running system. The syntax of range is
> @@ -606,6 +603,18 @@ bytes respectively. Such letter suffixes
>   			a memory unit (amount[KMG]). See also
>   			Documentation/kdump/kdump.txt for an example.
>   
> +	crashkernel_low=size[KMG]
> +			[KNL, x86_64] range under 4G. When crashkernel= is
> +			passed, kernel allocate physical memory region
> +			above 4G, that cause second kernel crash on system
> +			that need swiotlb later. Kernel would try to allocate
> +			some region below 4G automatically. This one let
> +			user to specify own low range under 4G for second
> +			kernel instead.

In fact, swiotlb caused the 2nd kernel to fail to boot this time, but in
general, other components that require low memory can cause the same
situation. I think it better to avoid the description that only swiotlb
can cause the 2nd kernel to fail to boot.

So, how about this?

... that cause second kernel crash on system that require some amount of
low memory, e.g. swiotlb that requires at least 72MB low memory at
default. Kernel would ...

> +			0: to disable low allocation on systems that do not
> +			need swiotlb, that will save 72M low ram in first
> +			kernel.
> +

Similarly, I think it better not to say swiotlb thing here. Just:

  0: to disable automatic low memory allocation.

Thanks.
HATAYAMA, Daisuke

Re: [PATCH v2 1/4] x86, kdump: Set crashkernel_low automatically

[Vivek Goyal]

On Wed, Apr 03, 2013 at 05:38:23PM -0700, Yinghai Lu wrote:

[..]
> +	if (ret != 0) {
> +		/*
> +		 * two parts from lib/swiotlb.c:
> +		 *	swiotlb size: user specified with swiotlb= or default.
> +		 *	swiotlb overflow buffer: now is hardcoded to 32k,
> +		 *		round to 8M to cover more others.
> +		 */
> +		low_size = swiotlb_size_or_default() + (8UL<<20);
> +		auto_set = true;

What is the correlation between swiotlb size of first kernel and second
kernel. They might be completely different kernel and with different
default size for swiotlb buffers.

[..]
>  
> +	crashkernel_low=size[KMG]
> +			[KNL, x86_64] range under 4G. When crashkernel= is
> +			passed, kernel allocate physical memory region
> +			above 4G,
	
This is not right. "When crashkernel=X;high is passed kernel first tries
to allocate memory above 4G". crashkernel=X does not try to allocate 
memory above 4G.

Vivek

Re: [PATCH v2 2/4] x86, kdump: Retore crashkernel= to allocate low

[Vivek Goyal]

On Wed, Apr 03, 2013 at 05:38:25PM -0700, Yinghai Lu wrote:
> Vivek found old kexec-tools does not work new kernel anymore.
> 
> So change back crashkernel= back to old behavoir, and add crashkernel_high=
> to let user decide if buffer could be above 4G, and also new kexec-tools will
> be needed.
> 
> v2: let crashkernel=X override crashkernel_high=
>     update description about _high will be ignored by crashkernel=X
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  Documentation/kernel-parameters.txt |   11 +++++++++--
>  arch/x86/kernel/setup.c             |   24 +++++++++++++++++++-----
>  include/linux/kexec.h               |    2 ++
>  kernel/kexec.c                      |    9 +++++++++
>  4 files changed, 39 insertions(+), 7 deletions(-)
> 
> Index: linux-2.6/Documentation/kernel-parameters.txt
> ===================================================================
> --- linux-2.6.orig/Documentation/kernel-parameters.txt
> +++ linux-2.6/Documentation/kernel-parameters.txt
> @@ -603,9 +603,14 @@ bytes respectively. Such letter suffixes
>  			a memory unit (amount[KMG]). See also
>  			Documentation/kdump/kdump.txt for an example.
>  
> +	crashkernel_high=size[KMG]
> +			[KNL, x86_64] range could be above 4G. Allow kernel
> +			to allocate physical memory region from top, so could
> +			be above 4G if system have more than 4G ram installed.

Also specify "otherwise memory will be allocated below 4G, if available".

> +			It will be ignored if crashkernel=X is specified.
>  	crashkernel_low=size[KMG]
> -			[KNL, x86_64] range under 4G. When crashkernel= is
> -			passed, kernel allocate physical memory region
> +			[KNL, x86_64] range under 4G. When crashkernel_high= is
> +			passed, kernel could allocate physical memory region
>  			above 4G, that cause second kernel crash on system
>  			that need swiotlb later. Kernel would try to allocate
>  			some region below 4G automatically. This one let
> @@ -614,6 +619,8 @@ bytes respectively. Such letter suffixes
>  			0: to disable low allocation on systems that do not
>  			need swiotlb, that will save 72M low ram in first
>  			kernel.
> +			It will be ignored when crashkernel_high=X is not used
> +			or return from that is below 4G.

Replace "return from that" with "memory reserved".

Vivek

Re: [PATCH v2 1/4] x86, kdump: Set crashkernel_low automatically

[Yinghai Lu]

On Thu, Apr 4, 2013 at 7:11 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Apr 03, 2013 at 05:38:23PM -0700, Yinghai Lu wrote:
>
> [..]
>> +     if (ret != 0) {
>> +             /*
>> +              * two parts from lib/swiotlb.c:
>> +              *      swiotlb size: user specified with swiotlb= or default.
>> +              *      swiotlb overflow buffer: now is hardcoded to 32k,
>> +              *              round to 8M to cover more others.
>> +              */
>> +             low_size = swiotlb_size_or_default() + (8UL<<20);
>> +             auto_set = true;
>
> What is the correlation between swiotlb size of first kernel and second
> kernel. They might be completely different kernel and with different
> default size for swiotlb buffers.
>

You are right.
That is most good guess assume they are the same.


> [..]
>>
>> +     crashkernel_low=size[KMG]
>> +                     [KNL, x86_64] range under 4G. When crashkernel= is
>> +                     passed, kernel allocate physical memory region
>> +                     above 4G,
>
> This is not right. "When crashkernel=X;high is passed kernel first tries
> to allocate memory above 4G". crashkernel=X does not try to allocate
> memory above 4G.

This is first patch, the text will be updated in second and fourth patch.

Thanks

Yinghai

Re: [PATCH v2 1/4] x86, kdump: Set crashkernel_low automatically

[Yinghai Lu]

On Thu, Apr 4, 2013 at 1:11 AM, HATAYAMA Daisuke
<d.hatayama@jp.fujitsu.com> wrote:
> (2013/04/04 9:38), Yinghai Lu wrote:
>
>> Index: linux-2.6/lib/swiotlb.c
>> ===================================================================
>> --- linux-2.6.orig/lib/swiotlb.c
>> +++ linux-2.6/lib/swiotlb.c
>> @@ -105,9 +105,9 @@ setup_io_tlb_npages(char *str)
>>       if (!strcmp(str, "force"))
>>               swiotlb_force = 1;
>>
>> -     return 1;
>> +     return 0;
>>   }
>> -__setup("swiotlb=", setup_io_tlb_npages);
>> +early_param("swiotlb", setup_io_tlb_npages);
>>   /* make io_tlb_overflow tunable too? */
>>
>>   unsigned long swiotlb_nr_tbl(void)
>> @@ -115,6 +115,18 @@ unsigned long swiotlb_nr_tbl(void)
>>       return io_tlb_nslabs;
>>   }
>>   EXPORT_SYMBOL_GPL(swiotlb_nr_tbl);
>> +
>> +/* default to 64MB */
>> +#define IO_TLB_DEFAULT_SIZE (64UL<<20)
>> +unsigned long swiotlb_size_or_default(void)
>> +{
>> +     unsigned long size;
>> +
>> +     size = io_tlb_nslabs << IO_TLB_SHIFT;
>> +
>> +     return size ? size : (IO_TLB_DEFAULT_SIZE);
>> +}
>> +
>>   /* Note that this doesn't work with highmem page */
>>   static dma_addr_t swiotlb_virt_to_bus(struct device *hwdev,
>>                                     volatile void *address)
>> @@ -188,8 +200,7 @@ int __init swiotlb_init_with_tbl(char *t
>>   void  __init
>>   swiotlb_init(int verbose)
>>   {
>> -     /* default to 64MB */
>> -     size_t default_size = 64UL<<20;
>> +     size_t default_size = IO_TLB_DEFAULT_SIZE;
>>       unsigned char *vstart;
>>       unsigned long bytes;
>
> The variable default_size seems no longer necessary. IO_TLB_DEFAULT_SIZE
> should be used directly.

Yes, but that does not hurt.

>
>>
>> Index: linux-2.6/Documentation/kernel-parameters.txt
>> ===================================================================
>> --- linux-2.6.orig/Documentation/kernel-parameters.txt
>> +++ linux-2.6/Documentation/kernel-parameters.txt
>> @@ -596,9 +596,6 @@ bytes respectively. Such letter suffixes
>>                       is selected automatically. Check
>>                       Documentation/kdump/kdump.txt for further details.
>>
>> -     crashkernel_low=size[KMG]
>> -                     [KNL, x86] parts under 4G.
>> -
>>       crashkernel=range1:size1[,range2:size2,...][@offset]
>>                       [KNL] Same as above, but depends on the memory
>>                       in the running system. The syntax of range is
>> @@ -606,6 +603,18 @@ bytes respectively. Such letter suffixes
>>                       a memory unit (amount[KMG]). See also
>>                       Documentation/kdump/kdump.txt for an example.
>>
>> +     crashkernel_low=size[KMG]
>> +                     [KNL, x86_64] range under 4G. When crashkernel= is
>> +                     passed, kernel allocate physical memory region
>> +                     above 4G, that cause second kernel crash on system
>> +                     that need swiotlb later. Kernel would try to allocate
>> +                     some region below 4G automatically. This one let
>> +                     user to specify own low range under 4G for second
>> +                     kernel instead.
>
> In fact, swiotlb caused the 2nd kernel to fail to boot this time, but in
> general, other components that require low memory can cause the same
> situation. I think it better to avoid the description that only swiotlb
> can cause the 2nd kernel to fail to boot.
>
> So, how about this?
>
> ... that cause second kernel crash on system that require some amount of
> low memory, e.g. swiotlb that requires at least 72MB low memory at
> default. Kernel would ...

ok, will change that.

>
>> +                     0: to disable low allocation on systems that do not
>> +                     need swiotlb, that will save 72M low ram in first
>> +                     kernel.
>> +
>
> Similarly, I think it better not to say swiotlb thing here. Just:
>
>   0: to disable automatic low memory allocation.

ok,

it will be:

---
        crashkernel_low=size[KMG]
                        [KNL, x86_64] range under 4G. When crashkernel= is
                        passed, kernel allocate physical memory region
                        above 4G, that cause second kernel crash on system
                        that require some amount of low memory, e.g. swiotlb
                        requires at least 64M+32K low memory.  Kernel would
                        try to allocate 72M below 4G automatically.
                        This one let user to specify own low range under 4G
                        for second kernel instead.
                        0: to disable low allocation.
---

Thanks

Yinghai

Re: [PATCH v2 2/4] x86, kdump: Retore crashkernel= to allocate low

[Yinghai Lu]

On Thu, Apr 4, 2013 at 7:16 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> +     crashkernel_high=size[KMG]
>> +                     [KNL, x86_64] range could be above 4G. Allow kernel
>> +                     to allocate physical memory region from top, so could
>> +                     be above 4G if system have more than 4G ram installed.
>
> Also specify "otherwise memory will be allocated below 4G, if available".
>
...
>> @@ -614,6 +619,8 @@ bytes respectively. Such letter suffixes
>>                       0: to disable low allocation on systems that do not
>>                       need swiotlb, that will save 72M low ram in first
>>                       kernel.
>> +                     It will be ignored when crashkernel_high=X is not used
>> +                     or return from that is below 4G.
>
> Replace "return from that" with "memory reserved".

updated.

Thanks

Yinghai

Re: [PATCH v2 4/4] x86, kdump: Change crashkernel_high/low= to crashkernel=;high/low

[Yinghai Lu]

On Wed, Apr 3, 2013 at 11:55 PM, HATAYAMA Daisuke
<d.hatayama@jp.fujitsu.com> wrote:
> (2013/04/04 9:38), Yinghai Lu wrote:
>
>> Index: linux-2.6/kernel/kexec.c
>> ===================================================================
>> --- linux-2.6.orig/kernel/kexec.c
>> +++ linux-2.6/kernel/kexec.c
>> @@ -1360,7 +1360,7 @@ static int __init parse_crashkernel_simp
>>
>>       if (*cur == '@')
>>               *crash_base = memparse(cur+1, &cur);
>> -     else if (*cur != ' ' && *cur != '\0') {
>> +     else if (*cur != ' ' && *cur != ';' && *cur != '\0') {
>>               pr_warning("crashkernel: unrecognized char\n");
>>               return -EINVAL;
>>       }
>
> As I said below, ";high" or ";low" check should be here. It would be
> enough to replace the condition *cur != ';' by strncmp(cur, ";high", 5)
> || strncmp(cur, ";low", 4).

Good catch, will use that strict checking.

>
>> @@ -1368,58 +1368,108 @@ static int __init parse_crashkernel_simp
>>       return 0;
>>   }
>>
>> -/*
>> - * That function is the entry point for command line parsing and should be
>> - * called from the arch-specific code.
>> - */
>> +#define SUFFIX_HIGH 0
>> +#define SUFFIX_LOW  1
>> +#define SUFFIX_NULL 2
>> +static __initdata char *suffix_tbl[] = {
>> +     [SUFFIX_HIGH] = ";high",
>> +     [SUFFIX_LOW]  = ";low",
>> +     [SUFFIX_NULL] = NULL,
>> +};
>> +
>> +static __init char *get_last_crashkernel(char *cmdline,
>> +                          const char *name,
>> +                          const char *suffix)
>> +{
>> +     char *p = cmdline, *ck_cmdline = NULL;
>> +
>> +     /* find crashkernel and use the last one if there are more */
>
> Why did you choose the last one? Is there any reason you didn't choose
> the first one?

I split the function out, and keep the old comments.

Also that is default behavior of early_param and __setup() handling,
last one will be take effective.

>
> Also, it's better to describe this bahaviour in
> Documentations/kernel-parameter.txt.
>
>> +     p = strstr(p, name);
>> +     while (p) {
>> +             char *end_p = strchr(p, ' ');
>> +             char *q;
>> +
>> +             if (!end_p)
>> +                     end_p = p + strlen(p);
>> +
>> +             if (!suffix) {
>> +                     int i;
>> +
>> +                     /* skip the one with any known suffix */
>> +                     for (i = 0; suffix_tbl[i]; i++) {
>> +                             q = end_p - strlen(suffix_tbl[i]);
>> +                             if (!strncmp(q, suffix_tbl[i],
>> +                                          strlen(suffix_tbl[i])))
>> +                                     goto next;
>> +                     }
>> +                     ck_cmdline = p;
>> +             } else {
>> +                     q = end_p - strlen(suffix);
>> +                     if (!strncmp(q, suffix, strlen(suffix)))
>> +                             ck_cmdline = p;
>> +             }
>
> It looks to me that this function does more than its name suggests. It
> seems to me enough to get the last occurence of "crashkernel=<some
> value>" and to leave the "<some value>" unknown for now.

ok will move
     ck_cmdline += strlen(name);

down.

>
> The current code of yours checks if each "crashkernel=<some value>"
> detected by strstr() ends with each of ";high" or ";low", but doesn't
> check the formeter letters at all; e.g, "crashkernel=foobar;high" is passed.

that should be ok, later parse...simple will report error.

>
> Also, this function can be called in different contexts: from a variant
> of parse_crashkernel_*(). Is it better to move this function in
> reserve_crashkernel() and then pass the obtained "crashkernel=<some
> value>" to a variant of parse_crashkernel_*() functions?

Do you mean calling get_last_crashkernel directly from reserve_crashkernel
directly?

I think we could keep parse_crashkernel_*() call it directly, so could have less
change in arch/x86/kernel/setup.c.

Thanks

Yinghai