[PATCH v2 0/2] powerpc/fadump: support memory range syntax for fadump memory reservation

This patchset adds support to input system memory range based memory size
for fadump reservation. The crashkernel parameter already supports such
syntax. The first patch refactors the parsing code of crashkernel parameter
for reuse. The second patch uses the newly refactored parsing code to reserve
memory for fadump based on system memory size.

---

Hari Bathini (2):
      refactor code parsing size based on memory range
      powerpc/fadump: parse fadump reserve memory size based on memory range


 arch/powerpc/kernel/fadump.c |   64 ++++++++++++++++++++++++----
 include/linux/kernel.h       |    5 ++
 kernel/kexec_core.c          |   63 ++--------------------------
 kernel/params.c              |   96 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 161 insertions(+), 67 deletions(-)

--
Hari Bathini

Currently, memory for fadump can be specified with fadump_reserve_mem=size,
where only a fixed size can be specified. Add the below syntax as well, to
support conditional reservation based on system memory size:

	fadump_reserve_mem=<range1>:<size1>[,<range2>:<size2>,...]

This syntax helps using the same commandline parameter for different system
memory sizes.

Signed-off-by: Hari Bathini <redacted>
Reviewed-by: Mahesh J Salgaonkar <redacted>
---
Changes in v2:
1. Changed subject from "[PATCH v2 2/2] powerpc/fadump: add support to parse size based on memory range".
2. Rebased to latest upstream.

 arch/powerpc/kernel/fadump.c |   64 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 55 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index 3cb3b02a..e435828 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c

@@ -193,6 +193,56 @@ static unsigned long init_fadump_mem_struct(struct fadump_mem_struct *fdm,
 	return addr;
 }
 
+/*
+ * This function parses command line for fadump_reserve_mem=
+ *
+ * Supports the below two syntaxes:
+ *    1. fadump_reserve_mem=size
+ *    2. fadump_reserve_mem=ramsize-range:size[,...]
+ *
+ * Sets fw_dump.reserve_bootvar with the memory size
+ * provided, 0 otherwise
+ *
+ * The function returns -EINVAL on failure, 0 otherwise.
+ */
+static int __init parse_fadump_reserve_mem(void)
+{
+	char *name = "fadump_reserve_mem=";
+	char *fadump_cmdline = NULL, *cur;
+
+	fw_dump.reserve_bootvar = 0;
+
+	/* find fadump_reserve_mem and use the last one if there are many */
+	cur = strstr(boot_command_line, name);
+	while (cur) {
+		fadump_cmdline = cur;
+		cur = strstr(cur+1, name);
+	}
+
+	/* when no fadump_reserve_mem= cmdline option is provided */
+	if (!fadump_cmdline)
+		return 0;
+
+	fadump_cmdline += strlen(name);
+
+	/* for fadump_reserve_mem=size cmdline syntax */
+	if (!is_param_range_based(fadump_cmdline)) {
+		fw_dump.reserve_bootvar = memparse(fadump_cmdline, NULL);
+		return 0;
+	}
+
+	/* for fadump_reserve_mem=ramsize-range:size[,...] cmdline syntax */
+	cur = fadump_cmdline;
+	fw_dump.reserve_bootvar = parse_mem_range_size("fadump_reserve_mem",
+					&cur, memblock_phys_mem_size());
+	if (cur == fadump_cmdline) {
+		printk(KERN_INFO "fadump_reserve_mem: Invaild syntax!\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /**
  * fadump_calculate_reserve_size(): reserve variable boot area 5% of System RAM
  *

@@ -212,12 +262,17 @@ static inline unsigned long fadump_calculate_reserve_size(void)
 {
 	unsigned long size;
 
+	/* sets fw_dump.reserve_bootvar */
+	parse_fadump_reserve_mem();
+
 	/*
 	 * Check if the size is specified through fadump_reserve_mem= cmdline
 	 * option. If yes, then use that.
 	 */
 	if (fw_dump.reserve_bootvar)
 		return fw_dump.reserve_bootvar;
+	else
+		printk(KERN_INFO "fadump: calculating default boot size\n");
 
 	/* divide by 20 to get 5% of value */
 	size = memblock_end_of_DRAM() / 20;

@@ -348,15 +403,6 @@ static int __init early_fadump_param(char *p)
 }
 early_param("fadump", early_fadump_param);
 
-/* Look for fadump_reserve_mem= cmdline option */
-static int __init early_fadump_reserve_mem(char *p)
-{
-	if (p)
-		fw_dump.reserve_bootvar = memparse(p, &p);
-	return 0;
-}
-early_param("fadump_reserve_mem", early_fadump_reserve_mem);
-
 static void register_fw_dump(struct fadump_mem_struct *fdm)
 {
 	int rc;

Currently, crashkernel parameter supports the below syntax to parse size
based on memory range:

	crashkernel=<range1>:<size1>[,<range2>:<size2>,...]

While such parsing is implemented for crashkernel parameter, it applies to
other parameters with similar syntax. So, move this code to a more generic
place for code reuse.

Cc: Eric Biederman <redacted>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Rusty Russell <redacted>
Cc: kexec@lists.infradead.org
Signed-off-by: Hari Bathini <redacted>
---
changes in v2:
1. Rebased to latest upstream.
2. Marked few more people on cc.

 include/linux/kernel.h |    5 +++
 kernel/kexec_core.c    |   63 +++-----------------------------
 kernel/params.c        |   96 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 106 insertions(+), 58 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 94aa10f..72f55e5 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h

@@ -436,6 +436,11 @@ extern char *get_options(const char *str, int nints, int *ints);
 extern unsigned long long memparse(const char *ptr, char **retptr);
 extern bool parse_option_str(const char *str, const char *option);
 
+extern bool __init is_param_range_based(const char *cmdline);
+extern unsigned long long __init parse_mem_range_size(const char *param,
+						      char **str,
+						      unsigned long long system_ram);
+
 extern int core_kernel_text(unsigned long addr);
 extern int core_kernel_data(unsigned long addr);
 extern int __kernel_text_address(unsigned long addr);

diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index 56b3ed0..d43f5cc 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c

@@ -1083,59 +1083,9 @@ static int __init parse_crashkernel_mem(char *cmdline,
 	char *cur = cmdline, *tmp;
 
 	/* for each entry of the comma-separated list */
-	do {
-		unsigned long long start, end = ULLONG_MAX, size;
-
-		/* get the start of the range */
-		start = memparse(cur, &tmp);
-		if (cur == tmp) {
-			pr_warn("crashkernel: Memory value expected\n");
-			return -EINVAL;
-		}
-		cur = tmp;
-		if (*cur != '-') {
-			pr_warn("crashkernel: '-' expected\n");
-			return -EINVAL;
-		}
-		cur++;
-
-		/* if no ':' is here, than we read the end */
-		if (*cur != ':') {
-			end = memparse(cur, &tmp);
-			if (cur == tmp) {
-				pr_warn("crashkernel: Memory value expected\n");
-				return -EINVAL;
-			}
-			cur = tmp;
-			if (end <= start) {
-				pr_warn("crashkernel: end <= start\n");
-				return -EINVAL;
-			}
-		}
-
-		if (*cur != ':') {
-			pr_warn("crashkernel: ':' expected\n");
-			return -EINVAL;
-		}
-		cur++;
-
-		size = memparse(cur, &tmp);
-		if (cur == tmp) {
-			pr_warn("Memory value expected\n");
-			return -EINVAL;
-		}
-		cur = tmp;
-		if (size >= system_ram) {
-			pr_warn("crashkernel: invalid size\n");
-			return -EINVAL;
-		}
-
-		/* match ? */
-		if (system_ram >= start && system_ram < end) {
-			*crash_size = size;
-			break;
-		}
-	} while (*cur++ == ',');
+	*crash_size = parse_mem_range_size("crashkernel", &cur, system_ram);
+	if (cur == cmdline)
+		return -EINVAL;
 
 	if (*crash_size > 0) {
 		while (*cur && *cur != ' ' && *cur != '@')

@@ -1272,7 +1222,6 @@ static int __init __parse_crashkernel(char *cmdline,
 			     const char *name,
 			     const char *suffix)
 {
-	char	*first_colon, *first_space;
 	char	*ck_cmdline;
 
 	BUG_ON(!crash_size || !crash_base);

@@ -1290,12 +1239,10 @@ static int __init __parse_crashkernel(char *cmdline,
 		return parse_crashkernel_suffix(ck_cmdline, crash_size,
 				suffix);
 	/*
-	 * if the commandline contains a ':', then that's the extended
+	 * if the parameter is range based, 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))
+	if (is_param_range_based(ck_cmdline))
 		return parse_crashkernel_mem(ck_cmdline, system_ram,
 				crash_size, crash_base);
 

diff --git a/kernel/params.c b/kernel/params.c
index a6d6149..84e40ae 100644
--- a/kernel/params.c
+++ b/kernel/params.c

@@ -268,6 +268,102 @@ char *parse_args(const char *doing,
 	return err;
 }
 
+/*
+ * is_param_range_based - check if current parameter is range based
+ * @cmdline: points to the parameter to check
+ *
+ * Returns true when the current paramer is range based, false otherwise
+ */
+bool __init is_param_range_based(const char *cmdline)
+{
+	char    *first_colon, *first_space;
+
+	first_colon = strchr(cmdline, ':');
+	first_space = strchr(cmdline, ' ');
+	if (first_colon && (!first_space || first_colon < first_space))
+		return true;
+
+	return false;
+}
+
+/*
+ * parse_mem_range_size - parse size based on memory range
+ * @param:  the thing being parsed
+ * @str: (input)  where parse begins
+ *                expected format - <range1>:<size1>[,<range2>:<size2>,...]
+ *       (output) On success - next char after parse completes
+ *                On failure - unchanged
+ * @system_ram: system ram size to check memory range against
+ *
+ * Returns the memory size on success and 0 on failure
+ */
+unsigned long long __init parse_mem_range_size(const char *param,
+					       char **str,
+					       unsigned long long system_ram)
+{
+	char *cur = *str, *tmp;
+	unsigned long long mem_size = 0;
+
+	/* for each entry of the comma-separated list */
+	do {
+		unsigned long long start, end = ULLONG_MAX, size;
+
+		/* get the start of the range */
+		start = memparse(cur, &tmp);
+		if (cur == tmp) {
+			printk(KERN_INFO "%s: Memory value expected\n", param);
+			return mem_size;
+		}
+		cur = tmp;
+		if (*cur != '-') {
+			printk(KERN_INFO "%s: '-' expected\n", param);
+			return mem_size;
+		}
+		cur++;
+
+		/* if no ':' is here, than we read the end */
+		if (*cur != ':') {
+			end = memparse(cur, &tmp);
+			if (cur == tmp) {
+				printk(KERN_INFO "%s: Memory value expected\n",
+					param);
+				return mem_size;
+			}
+			cur = tmp;
+			if (end <= start) {
+				printk(KERN_INFO "%s: end <= start\n", param);
+				return mem_size;
+			}
+		}
+
+		if (*cur != ':') {
+			printk(KERN_INFO "%s: ':' expected\n", param);
+			return mem_size;
+		}
+		cur++;
+
+		size = memparse(cur, &tmp);
+		if (cur == tmp) {
+			printk(KERN_INFO "%s: Memory value expected\n", param);
+			return mem_size;
+		}
+		cur = tmp;
+		if (size >= system_ram) {
+			printk(KERN_INFO "%s: invalid size\n", param);
+			return mem_size;
+		}
+
+		/* match ? */
+		if (system_ram >= start && system_ram < end) {
+			mem_size = size;
+			*str = cur;
+			break;
+		}
+	} while (*cur++ == ',');
+
+	return mem_size;
+}
+
 /* Lazy bastard, eh? */
 #define STANDARD_PARAM_DEF(name, type, format, strtolfn)      		\
 	int param_set_##name(const char *val, const struct kernel_param *kp) \

On Wed, 2016-22-06 at 19:25:26 UTC, Hari Bathini wrote:

Currently, crashkernel parameter supports the below syntax to parse size
based on memory range:

	crashkernel=<range1>:<size1>[,<range2>:<size2>,...]

While such parsing is implemented for crashkernel parameter, it applies to
other parameters with similar syntax. So, move this code to a more generic
place for code reuse.

Cc: Eric Biederman <redacted>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Rusty Russell <redacted>
Cc: kexec@lists.infradead.org
Signed-off-by: Hari Bathini <redacted>

Hari, it's not immediately clear that this makes no change to the logic in the
kexec code. Can you reply with a longer change log explaining why the old & new
logic is the same for kexec.

cheers

On 06/24/2016 10:56 AM, Michael Ellerman wrote:

On Wed, 2016-22-06 at 19:25:26 UTC, Hari Bathini wrote:

quoted

Currently, crashkernel parameter supports the below syntax to parse size
based on memory range:

	crashkernel=<range1>:<size1>[,<range2>:<size2>,...]

While such parsing is implemented for crashkernel parameter, it applies to
other parameters with similar syntax. So, move this code to a more generic
place for code reuse.

Cc: Eric Biederman <redacted>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Rusty Russell <redacted>
Cc: kexec@lists.infradead.org
Signed-off-by: Hari Bathini <redacted>

Hari, it's not immediately clear that this makes no change to the logic in the
kexec code. Can you reply with a longer change log explaining why the old & new
logic is the same for kexec.

Hi Michael,

Please consider this changelog for this patch:

--
crashkernel parameter supports different syntaxes to specify the amount
of memory to be reserved for kdump kernel. Below is one of the supported
syntaxes that needs parsing to find the memory size to reserve, based on
memory range:

crashkernel=<range1>:<size1>[,<range2>:<size2>,...]

While such parsing is implemented for crashkernel parameter, it applies to
other parameters, like fadump_reserve_mem, which could use similar syntax.
So, to reuse code, moving the code that checks if the parameter syntax is as
above and also the code that parses memory size to reserve, for this syntax.
While the code is moved to kernel/params.c file, there is no change in logic
for crashkernel parameter parsing as the moved code is invoked with function
calls at appropriate places.
--

Thanks
Hari

quoted

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 94aa10f..72f55e5 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h

@@ -436,6 +436,11 @@ extern char *get_options(const char *str, int nints, int *ints);
  extern unsigned long long memparse(const char *ptr, char **retptr);
  extern bool parse_option_str(const char *str, const char *option);
  
+extern bool __init is_param_range_based(const char *cmdline);
+extern unsigned long long __init parse_mem_range_size(const char *param,
+						      char **str,
+						      unsigned long long system_ram);
+
  extern int core_kernel_text(unsigned long addr);
  extern int core_kernel_data(unsigned long addr);
  extern int __kernel_text_address(unsigned long addr);

diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index 56b3ed0..d43f5cc 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c

@@ -1083,59 +1083,9 @@ static int __init parse_crashkernel_mem(char *cmdline,
  	char *cur = cmdline, *tmp;
  
  	/* for each entry of the comma-separated list */
-	do {
-		unsigned long long start, end = ULLONG_MAX, size;
-
-		/* get the start of the range */
-		start = memparse(cur, &tmp);
-		if (cur == tmp) {
-			pr_warn("crashkernel: Memory value expected\n");
-			return -EINVAL;
-		}
-		cur = tmp;
-		if (*cur != '-') {
-			pr_warn("crashkernel: '-' expected\n");
-			return -EINVAL;
-		}
-		cur++;
-
-		/* if no ':' is here, than we read the end */
-		if (*cur != ':') {
-			end = memparse(cur, &tmp);
-			if (cur == tmp) {
-				pr_warn("crashkernel: Memory value expected\n");
-				return -EINVAL;
-			}
-			cur = tmp;
-			if (end <= start) {
-				pr_warn("crashkernel: end <= start\n");
-				return -EINVAL;
-			}
-		}
-
-		if (*cur != ':') {
-			pr_warn("crashkernel: ':' expected\n");
-			return -EINVAL;
-		}
-		cur++;
-
-		size = memparse(cur, &tmp);
-		if (cur == tmp) {
-			pr_warn("Memory value expected\n");
-			return -EINVAL;
-		}
-		cur = tmp;
-		if (size >= system_ram) {
-			pr_warn("crashkernel: invalid size\n");
-			return -EINVAL;
-		}
-
-		/* match ? */
-		if (system_ram >= start && system_ram < end) {
-			*crash_size = size;
-			break;
-		}
-	} while (*cur++ == ',');
+	*crash_size = parse_mem_range_size("crashkernel", &cur, system_ram);
+	if (cur == cmdline)
+		return -EINVAL;
  
  	if (*crash_size > 0) {
  		while (*cur && *cur != ' ' && *cur != '@')

@@ -1272,7 +1222,6 @@ static int __init __parse_crashkernel(char *cmdline,
  			     const char *name,
  			     const char *suffix)
  {
-	char	*first_colon, *first_space;
  	char	*ck_cmdline;
  
  	BUG_ON(!crash_size || !crash_base);

@@ -1290,12 +1239,10 @@ static int __init __parse_crashkernel(char *cmdline,
  		return parse_crashkernel_suffix(ck_cmdline, crash_size,
  				suffix);
  	/*
-	 * if the commandline contains a ':', then that's the extended
+	 * if the parameter is range based, 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))
+	if (is_param_range_based(ck_cmdline))
  		return parse_crashkernel_mem(ck_cmdline, system_ram,
  				crash_size, crash_base);
  

diff --git a/kernel/params.c b/kernel/params.c
index a6d6149..84e40ae 100644
--- a/kernel/params.c
+++ b/kernel/params.c

@@ -268,6 +268,102 @@ char *parse_args(const char *doing,
  	return err;
  }
  
+/*
+ * is_param_range_based - check if current parameter is range based
+ * @cmdline: points to the parameter to check
+ *
+ * Returns true when the current paramer is range based, false otherwise
+ */
+bool __init is_param_range_based(const char *cmdline)
+{
+	char    *first_colon, *first_space;
+
+	first_colon = strchr(cmdline, ':');
+	first_space = strchr(cmdline, ' ');
+	if (first_colon && (!first_space || first_colon < first_space))
+		return true;
+
+	return false;
+}
+
+/*
+ * parse_mem_range_size - parse size based on memory range
+ * @param:  the thing being parsed
+ * @str: (input)  where parse begins
+ *                expected format - <range1>:<size1>[,<range2>:<size2>,...]
+ *       (output) On success - next char after parse completes
+ *                On failure - unchanged
+ * @system_ram: system ram size to check memory range against
+ *
+ * Returns the memory size on success and 0 on failure
+ */
+unsigned long long __init parse_mem_range_size(const char *param,
+					       char **str,
+					       unsigned long long system_ram)
+{
+	char *cur = *str, *tmp;
+	unsigned long long mem_size = 0;
+
+	/* for each entry of the comma-separated list */
+	do {
+		unsigned long long start, end = ULLONG_MAX, size;
+
+		/* get the start of the range */
+		start = memparse(cur, &tmp);
+		if (cur == tmp) {
+			printk(KERN_INFO "%s: Memory value expected\n", param);
+			return mem_size;
+		}
+		cur = tmp;
+		if (*cur != '-') {
+			printk(KERN_INFO "%s: '-' expected\n", param);
+			return mem_size;
+		}
+		cur++;
+
+		/* if no ':' is here, than we read the end */
+		if (*cur != ':') {
+			end = memparse(cur, &tmp);
+			if (cur == tmp) {
+				printk(KERN_INFO "%s: Memory value expected\n",
+					param);
+				return mem_size;
+			}
+			cur = tmp;
+			if (end <= start) {
+				printk(KERN_INFO "%s: end <= start\n", param);
+				return mem_size;
+			}
+		}
+
+		if (*cur != ':') {
+			printk(KERN_INFO "%s: ':' expected\n", param);
+			return mem_size;
+		}
+		cur++;
+
+		size = memparse(cur, &tmp);
+		if (cur == tmp) {
+			printk(KERN_INFO "%s: Memory value expected\n", param);
+			return mem_size;
+		}
+		cur = tmp;
+		if (size >= system_ram) {
+			printk(KERN_INFO "%s: invalid size\n", param);
+			return mem_size;
+		}
+
+		/* match ? */
+		if (system_ram >= start && system_ram < end) {
+			mem_size = size;
+			*str = cur;
+			break;
+		}
+	} while (*cur++ == ',');
+
+	return mem_size;
+}
+
  /* Lazy bastard, eh? */
  #define STANDARD_PARAM_DEF(name, type, format, strtolfn)      		\
  	int param_set_##name(const char *val, const struct kernel_param *kp) \

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

On 06/24/2016 10:56 AM, Michael Ellerman wrote:

quoted

On Wed, 2016-22-06 at 19:25:26 UTC, Hari Bathini wrote:

...

While the code is moved to kernel/params.c file, there is no change in logic
for crashkernel parameter parsing as the moved code is invoked with function
calls at appropriate places.

Are you sure that's true?

The old code would return -EINVAL from parse_crashkernel_mem() for any
error, regardless of whether it had already parsed some of the string.

eg:

quoted

quoted

diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index 56b3ed0..d43f5cc 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c

@@ -1083,59 +1083,9 @@ static int __init parse_crashkernel_mem(char *cmdline,
  	char *cur = cmdline, *tmp;
  
  	/* for each entry of the comma-separated list */
-	do {
-		unsigned long long start, end = ULLONG_MAX, size;
-
-		/* get the start of the range */
-		start = memparse(cur, &tmp);
-		if (cur == tmp) {
-			pr_warn("crashkernel: Memory value expected\n");
-			return -EINVAL;
-		}
-		cur = tmp;
-		if (*cur != '-') {
-			pr_warn("crashkernel: '-' expected\n");
-			return -EINVAL;
-		}
-		cur++;
-
-		/* if no ':' is here, than we read the end */
-		if (*cur != ':') {
-			end = memparse(cur, &tmp);
-			if (cur == tmp) {
-				pr_warn("crashkernel: Memory value expected\n");
-				return -EINVAL;
-			}

So eg, if I give it "128M-foo" it will modify cur, and then error out here ^

You've changed that to:

quoted

quoted

+	*crash_size = parse_mem_range_size("crashkernel", &cur, system_ram);
+	if (cur == cmdline)
+		return -EINVAL;

Which only returns EINVAL if cur is not modified at all.

And looking below:

quoted

quoted

diff --git a/kernel/params.c b/kernel/params.c
index a6d6149..84e40ae 100644
--- a/kernel/params.c
+++ b/kernel/params.c

...

quoted

quoted

+unsigned long long __init parse_mem_range_size(const char *param,
+					       char **str,
+					       unsigned long long system_ram)
+{
+	char *cur = *str, *tmp;
+	unsigned long long mem_size = 0;
+
+	/* for each entry of the comma-separated list */
+	do {
+		unsigned long long start, end = ULLONG_MAX, size;
+
+		/* get the start of the range */
+		start = memparse(cur, &tmp);
+		if (cur == tmp) {
+			printk(KERN_INFO "%s: Memory value expected\n", param);
+			return mem_size;
+		}
+		cur = tmp;
+		if (*cur != '-') {
+			printk(KERN_INFO "%s: '-' expected\n", param);
+			return mem_size;
+		}
+		cur++;
+
+		/* if no ':' is here, than we read the end */
+		if (*cur != ':') {
+			end = memparse(cur, &tmp);
+			if (cur == tmp) {
+				printk(KERN_INFO "%s: Memory value expected\n",
+					param);
+				return mem_size;

If we error out here for example, we have modified cur, so the code above
*won't* return EINVAL.

Which looks like a behaviour change to me?

cheers

On 07/05/2016 10:48 AM, Michael Ellerman wrote:

quoted

On 06/24/2016 10:56 AM, Michael Ellerman wrote:

quoted

On Wed, 2016-22-06 at 19:25:26 UTC, Hari Bathini wrote:

...

quoted

While the code is moved to kernel/params.c file, there is no change in logic
for crashkernel parameter parsing as the moved code is invoked with function
calls at appropriate places.

Hi Michael,

Are you sure that's true?

Yes. I tested it.

The old code would return -EINVAL from parse_crashkernel_mem() for any
error, regardless of whether it had already parsed some of the string.

eg:

quoted

quoted

quoted

diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index 56b3ed0..d43f5cc 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c

@@ -1083,59 +1083,9 @@ static int __init parse_crashkernel_mem(char *cmdline,
   	char *cur = cmdline, *tmp;
   
   	/* for each entry of the comma-separated list */
-	do {
-		unsigned long long start, end = ULLONG_MAX, size;
-
-		/* get the start of the range */
-		start = memparse(cur, &tmp);
-		if (cur == tmp) {
-			pr_warn("crashkernel: Memory value expected\n");
-			return -EINVAL;
-		}
-		cur = tmp;
-		if (*cur != '-') {
-			pr_warn("crashkernel: '-' expected\n");
-			return -EINVAL;
-		}
-		cur++;
-
-		/* if no ':' is here, than we read the end */
-		if (*cur != ':') {
-			end = memparse(cur, &tmp);
-			if (cur == tmp) {
-				pr_warn("crashkernel: Memory value expected\n");
-				return -EINVAL;
-			}

So eg, if I give it "128M-foo" it will modify cur, and then error out here ^

It does modify cur (local variable) but that would have no bearing on 
parsing logic
as we are returning immediately..

You've changed that to:

quoted

quoted

quoted

+	*crash_size = parse_mem_range_size("crashkernel", &cur, system_ram);
+	if (cur == cmdline)
+		return -EINVAL;

Which only returns EINVAL if cur is not modified at all.

I think the confusion is with the same local variable cur in 
parse_crashkernel_mem()
& parse_mem_range_size() functions.

We modified cur (local variable) in parse_mem_range_size() but the 
output parameter (char **str)
remains unchanged unless we find a match.

Thanks
Hari

And looking below:

quoted

quoted

quoted

diff --git a/kernel/params.c b/kernel/params.c
index a6d6149..84e40ae 100644
--- a/kernel/params.c
+++ b/kernel/params.c

...

quoted

quoted

quoted

+unsigned long long __init parse_mem_range_size(const char *param,
+					       char **str,
+					       unsigned long long system_ram)
+{
+	char *cur = *str, *tmp;
+	unsigned long long mem_size = 0;
+
+	/* for each entry of the comma-separated list */
+	do {
+		unsigned long long start, end = ULLONG_MAX, size;
+
+		/* get the start of the range */
+		start = memparse(cur, &tmp);
+		if (cur == tmp) {
+			printk(KERN_INFO "%s: Memory value expected\n", param);
+			return mem_size;
+		}
+		cur = tmp;
+		if (*cur != '-') {
+			printk(KERN_INFO "%s: '-' expected\n", param);
+			return mem_size;
+		}
+		cur++;
+
+		/* if no ':' is here, than we read the end */
+		if (*cur != ':') {
+			end = memparse(cur, &tmp);
+			if (cur == tmp) {
+				printk(KERN_INFO "%s: Memory value expected\n",
+					param);
+				return mem_size;

If we error out here for example, we have modified cur, so the code above
*won't* return EINVAL.

Which looks like a behaviour change to me?

cheers
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Ping..


On Friday 24 June 2016 10:45 PM, Hari Bathini wrote:

On 06/24/2016 10:56 AM, Michael Ellerman wrote:

quoted

On Wed, 2016-22-06 at 19:25:26 UTC, Hari Bathini wrote:

quoted

Currently, crashkernel parameter supports the below syntax to parse 
size
based on memory range:

    crashkernel=<range1>:<size1>[,<range2>:<size2>,...]

While such parsing is implemented for crashkernel parameter, it 
applies to
other parameters with similar syntax. So, move this code to a more 
generic
place for code reuse.

Cc: Eric Biederman <redacted>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Rusty Russell <redacted>
Cc: kexec@lists.infradead.org
Signed-off-by: Hari Bathini <redacted>

Hari, it's not immediately clear that this makes no change to the 
logic in the
kexec code. Can you reply with a longer change log explaining why the 
old & new
logic is the same for kexec.

Hi Michael,

Please consider this changelog for this patch:

-- 
crashkernel parameter supports different syntaxes to specify the amount
of memory to be reserved for kdump kernel. Below is one of the supported
syntaxes that needs parsing to find the memory size to reserve, based on
memory range:

crashkernel=<range1>:<size1>[,<range2>:<size2>,...]

While such parsing is implemented for crashkernel parameter, it 
applies to
other parameters, like fadump_reserve_mem, which could use similar 
syntax.
So, to reuse code, moving the code that checks if the parameter syntax 
is as
above and also the code that parses memory size to reserve, for this 
syntax.
While the code is moved to kernel/params.c file, there is no change in 
logic
for crashkernel parameter parsing as the moved code is invoked with 
function
calls at appropriate places.
-- 

Thanks
Hari

quoted

quoted

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 94aa10f..72f55e5 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h

@@ -436,6 +436,11 @@ extern char *get_options(const char *str, int 

nints, int *ints);
  extern unsigned long long memparse(const char *ptr, char **retptr);
  extern bool parse_option_str(const char *str, const char *option);
  +extern bool __init is_param_range_based(const char *cmdline);
+extern unsigned long long __init parse_mem_range_size(const char 
*param,
+                              char **str,
+                              unsigned long long system_ram);
+
  extern int core_kernel_text(unsigned long addr);
  extern int core_kernel_data(unsigned long addr);
  extern int __kernel_text_address(unsigned long addr);

diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index 56b3ed0..d43f5cc 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c

@@ -1083,59 +1083,9 @@ static int __init parse_crashkernel_mem(char 

*cmdline,
      char *cur = cmdline, *tmp;
        /* for each entry of the comma-separated list */
-    do {
-        unsigned long long start, end = ULLONG_MAX, size;
-
-        /* get the start of the range */
-        start = memparse(cur, &tmp);
-        if (cur == tmp) {
-            pr_warn("crashkernel: Memory value expected\n");
-            return -EINVAL;
-        }
-        cur = tmp;
-        if (*cur != '-') {
-            pr_warn("crashkernel: '-' expected\n");
-            return -EINVAL;
-        }
-        cur++;
-
-        /* if no ':' is here, than we read the end */
-        if (*cur != ':') {
-            end = memparse(cur, &tmp);
-            if (cur == tmp) {
-                pr_warn("crashkernel: Memory value expected\n");
-                return -EINVAL;
-            }
-            cur = tmp;
-            if (end <= start) {
-                pr_warn("crashkernel: end <= start\n");
-                return -EINVAL;
-            }
-        }
-
-        if (*cur != ':') {
-            pr_warn("crashkernel: ':' expected\n");
-            return -EINVAL;
-        }
-        cur++;
-
-        size = memparse(cur, &tmp);
-        if (cur == tmp) {
-            pr_warn("Memory value expected\n");
-            return -EINVAL;
-        }
-        cur = tmp;
-        if (size >= system_ram) {
-            pr_warn("crashkernel: invalid size\n");
-            return -EINVAL;
-        }
-
-        /* match ? */
-        if (system_ram >= start && system_ram < end) {
-            *crash_size = size;
-            break;
-        }
-    } while (*cur++ == ',');
+    *crash_size = parse_mem_range_size("crashkernel", &cur, 
system_ram);
+    if (cur == cmdline)
+        return -EINVAL;
        if (*crash_size > 0) {
          while (*cur && *cur != ' ' && *cur != '@')

@@ -1272,7 +1222,6 @@ static int __init __parse_crashkernel(char 

*cmdline,
                   const char *name,
                   const char *suffix)
  {
-    char    *first_colon, *first_space;
      char    *ck_cmdline;
        BUG_ON(!crash_size || !crash_base);

@@ -1290,12 +1239,10 @@ static int __init __parse_crashkernel(char 

*cmdline,
          return parse_crashkernel_suffix(ck_cmdline, crash_size,
                  suffix);
      /*
-     * if the commandline contains a ':', then that's the extended
+     * if the parameter is range based, 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))
+    if (is_param_range_based(ck_cmdline))
          return parse_crashkernel_mem(ck_cmdline, system_ram,
                  crash_size, crash_base);
  diff --git a/kernel/params.c b/kernel/params.c
index a6d6149..84e40ae 100644

--- a/kernel/params.c
+++ b/kernel/params.c

@@ -268,6 +268,102 @@ char *parse_args(const char *doing,
      return err;
  }
  +/*
+ * is_param_range_based - check if current parameter is range based
+ * @cmdline: points to the parameter to check
+ *
+ * Returns true when the current paramer is range based, false 

otherwise
+ */
+bool __init is_param_range_based(const char *cmdline)
+{
+    char    *first_colon, *first_space;
+
+    first_colon = strchr(cmdline, ':');
+    first_space = strchr(cmdline, ' ');
+    if (first_colon && (!first_space || first_colon < first_space))
+        return true;
+
+    return false;
+}
+
+/*
+ * parse_mem_range_size - parse size based on memory range
+ * @param:  the thing being parsed
+ * @str: (input)  where parse begins
+ *                expected format - 
<range1>:<size1>[,<range2>:<size2>,...]
+ *       (output) On success - next char after parse completes
+ *                On failure - unchanged
+ * @system_ram: system ram size to check memory range against
+ *
+ * Returns the memory size on success and 0 on failure
+ */
+unsigned long long __init parse_mem_range_size(const char *param,
+                           char **str,
+                           unsigned long long system_ram)
+{
+    char *cur = *str, *tmp;
+    unsigned long long mem_size = 0;
+
+    /* for each entry of the comma-separated list */
+    do {
+        unsigned long long start, end = ULLONG_MAX, size;
+
+        /* get the start of the range */
+        start = memparse(cur, &tmp);
+        if (cur == tmp) {
+            printk(KERN_INFO "%s: Memory value expected\n", param);
+            return mem_size;
+        }
+        cur = tmp;
+        if (*cur != '-') {
+            printk(KERN_INFO "%s: '-' expected\n", param);
+            return mem_size;
+        }
+        cur++;
+
+        /* if no ':' is here, than we read the end */
+        if (*cur != ':') {
+            end = memparse(cur, &tmp);
+            if (cur == tmp) {
+                printk(KERN_INFO "%s: Memory value expected\n",
+                    param);
+                return mem_size;
+            }
+            cur = tmp;
+            if (end <= start) {
+                printk(KERN_INFO "%s: end <= start\n", param);
+                return mem_size;
+            }
+        }
+
+        if (*cur != ':') {
+            printk(KERN_INFO "%s: ':' expected\n", param);
+            return mem_size;
+        }
+        cur++;
+
+        size = memparse(cur, &tmp);
+        if (cur == tmp) {
+            printk(KERN_INFO "%s: Memory value expected\n", param);
+            return mem_size;
+        }
+        cur = tmp;
+        if (size >= system_ram) {
+            printk(KERN_INFO "%s: invalid size\n", param);
+            return mem_size;
+        }
+
+        /* match ? */
+        if (system_ram >= start && system_ram < end) {
+            mem_size = size;
+            *str = cur;
+            break;
+        }
+    } while (*cur++ == ',');
+
+    return mem_size;
+}
+
  /* Lazy bastard, eh? */
  #define STANDARD_PARAM_DEF(name, type, format, 
strtolfn)              \
      int param_set_##name(const char *val, const struct 
kernel_param *kp) \

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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