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

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

[Hari Bathini]

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):
      kexec: 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(-)

[RESEND][PATCH v2 1/2] kexec: refactor code parsing size based on memory range

[Hari Bathini]

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. This patch moves crashkernel's parsing code for above syntax to
to kernel/params.c file for reuse. Two functions is_param_range_based()
and parse_mem_range_size() are added to kernel/params.c file for this
purpose.

Any parameter that uses the above syntax can use is_param_range_based()
function to validate the syntax and parse_mem_range_size() function to
get the parsed memory size. While some code is moved to kernel/params.c
file, there is no change functionality wise in parsing the crashkernel
parameter.

Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com>
---

Changes from v1:
1. Updated changelog

 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 d96a611..2df7ba2 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -435,6 +435,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 5616755..3a74024 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -1104,59 +1104,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 != '@')
@@ -1293,7 +1243,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);
@@ -1311,12 +1260,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) \

[RESEND][PATCH v2 2/2] powerpc/fadump: parse fadump reserve memory size based on memory range

[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 <hbathini@linux.vnet.ibm.com>
Reviewed-by: Mahesh J Salgaonkar <mahesh@linux.vnet.ibm.com>
---
 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 b3a6633..4661ae6 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;

Re: [RESEND][PATCH v2 1/2] kexec: refactor code parsing size based on memory range

[Dave Young]

Hi Hari,

On 08/04/16 at 01:03am, Hari Bathini wrote:
> 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. This patch moves crashkernel's parsing code for above syntax to
> to kernel/params.c file for reuse. Two functions is_param_range_based()
> and parse_mem_range_size() are added to kernel/params.c file for this
> purpose.
> 
> Any parameter that uses the above syntax can use is_param_range_based()
> function to validate the syntax and parse_mem_range_size() function to
> get the parsed memory size. While some code is moved to kernel/params.c
> file, there is no change functionality wise in parsing the crashkernel
> parameter.
> 
> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com>
> ---
> 
> Changes from v1:
> 1. Updated changelog
> 
>  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 d96a611..2df7ba2 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -435,6 +435,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 5616755..3a74024 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -1104,59 +1104,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 != '@')
> @@ -1293,7 +1243,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);
> @@ -1311,12 +1260,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

/lib/cmdline.c is a better place for this?

> @@ -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
> + */

It is not necessary to be range specific? Maybe is_colon_in_param or
something else. If it is range based we need know what is the range
like.

> +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>,...]

There should be detail example about the range format, especially about
the "-" separator.

> + *       (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",

s/Memory/memory?

> +					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);

Ditto.

> +			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) \
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

Thanks
Dave

Re: [RESEND][PATCH v2 2/2] powerpc/fadump: parse fadump reserve memory size based on memory range

[Michael Ellerman]

Hari Bathini <hbathini@linux.vnet.ibm.com> writes:
...
>  /**
>   * 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;

The code already knows how to reserve 5% based on the size of the machine's
memory, as long as no commandline parameter is passed. So why can't we
just use that logic?

cheers

Re: [RESEND][PATCH v2 2/2] powerpc/fadump: parse fadump reserve memory size based on memory range

[Hari Bathini]

On Thursday 04 August 2016 03:15 PM, Michael Ellerman wrote:
> Hari Bathini <hbathini@linux.vnet.ibm.com> writes:
> ...
>>   /**
>>    * 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;
> The code already knows how to reserve 5% based on the size of the machine's
> memory, as long as no commandline parameter is passed. So why can't we
> just use that logic?

Hi Michael,

That is the default value reserved but not a good enough value for
every case. It is a bit difficult to come up with a robust formula
that works for every case as new kernel changes could make the
values obsolete. But it won't be all that difficult to find values that
work for different memory ranges for a given kernel version.
Passing that as range based input with "fadump_reserve_mem"
parameter would work for every memory configuration on a
given system, which is what this patch is trying to provide..

Thanks
Hari


> cheers
>

Re: [RESEND][PATCH v2 1/2] kexec: refactor code parsing size based on memory range

[Hari Bathini]

Hi Dave


Thanks for the review..


On Thursday 04 August 2016 02:56 PM, Dave Young wrote:
> Hi Hari,
>
> On 08/04/16 at 01:03am, Hari Bathini wrote:
>> 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. This patch moves crashkernel's parsing code for above syntax to
>> to kernel/params.c file for reuse. Two functions is_param_range_based()
>> and parse_mem_range_size() are added to kernel/params.c file for this
>> purpose.
>>
>> Any parameter that uses the above syntax can use is_param_range_based()
>> function to validate the syntax and parse_mem_range_size() function to
>> get the parsed memory size. While some code is moved to kernel/params.c
>> file, there is no change functionality wise in parsing the crashkernel
>> parameter.
>>
>> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com>
>> ---
>>
>> Changes from v1:
>> 1. Updated changelog
>>
>>   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 d96a611..2df7ba2 100644
>> --- a/include/linux/kernel.h
>> +++ b/include/linux/kernel.h
>> @@ -435,6 +435,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 5616755..3a74024 100644
>> --- a/kernel/kexec_core.c
>> +++ b/kernel/kexec_core.c
>> @@ -1104,59 +1104,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 != '@')
>> @@ -1293,7 +1243,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);
>> @@ -1311,12 +1260,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
> /lib/cmdline.c is a better place for this?

True. Will change accordingly..

>> @@ -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
>> + */
> It is not necessary to be range specific? Maybe is_colon_in_param or
> something else. If it is range based we need know what is the range
> like.

Hmmm.. I prefer to rename it to is_colon_in_param() and avoid
adding anymore checks here..

>> +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>,...]
> There should be detail example about the range format, especially about
> the "-" separator.
>

Sure..

>> + *       (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",
> s/Memory/memory?

I haven't changed the content of print messages while moving them around.
Probably I should..

- Hari

>> +					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);
> Ditto.
>
>> +			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) \
>>
>>
>> _______________________________________________
>> kexec mailing list
>> kexec@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/kexec
> Thanks
> Dave
>

Re: [RESEND][PATCH v2 2/2] powerpc/fadump: parse fadump reserve memory size based on memory range

[Hari Bathini]

On Friday 05 August 2016 12:23 AM, Hari Bathini wrote:
>
> On Thursday 04 August 2016 03:15 PM, Michael Ellerman wrote:
>> Hari Bathini <hbathini@linux.vnet.ibm.com> writes:
>> ...
>>>   /**
>>>    * 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;
>> The code already knows how to reserve 5% based on the size of the 
>> machine's
>> memory, as long as no commandline parameter is passed. So why can't we
>> just use that logic?
>
> Hi Michael,
>
> That is the default value reserved but not a good enough value for
> every case. It is a bit difficult to come up with a robust formula
> that works for every case as new kernel changes could make the
> values obsolete. But it won't be all that difficult to find values that
> work for different memory ranges for a given kernel version.
> Passing that as range based input with "fadump_reserve_mem"
> parameter would work for every memory configuration on a
> given system, which is what this patch is trying to provide..
>

Hi Michael,

You want me to add this to the changelog on respin?

Thanks
Hari

> Thanks
> Hari
>
>
>> cheers
>>
>

Re: [RESEND][PATCH v2 2/2] powerpc/fadump: parse fadump reserve memory size based on memory range

[Michael Ellerman]

Hari Bathini <hbathini@linux.vnet.ibm.com> writes:
> On Friday 05 August 2016 12:23 AM, Hari Bathini wrote:
>> On Thursday 04 August 2016 03:15 PM, Michael Ellerman wrote:
>>> The code already knows how to reserve 5% based on the size of the 
>>> machine's
>>> memory, as long as no commandline parameter is passed. So why can't we
>>> just use that logic?
>>
>> That is the default value reserved but not a good enough value for
>> every case. It is a bit difficult to come up with a robust formula
>> that works for every case as new kernel changes could make the
>> values obsolete. But it won't be all that difficult to find values that
>> work for different memory ranges for a given kernel version.
>> Passing that as range based input with "fadump_reserve_mem"
>> parameter would work for every memory configuration on a
>> given system, which is what this patch is trying to provide..
>
> You want me to add this to the changelog on respin?

I'm not really convinced.

Distros are going to want to specify a fixed set of values for different
memory sizes, at least that's what I've seen in the past with kdump. So
I don't see why we can't just do that in the kernel with a formula based
on memory size, and maybe some other information.

Maybe the formula is more complicated than 5% of RAM, but it shouldn't
be *that* much more complicated.

cheers

Re: [RESEND][PATCH v2 2/2] powerpc/fadump: parse fadump reserve memory size based on memory range

[Hari Bathini]

On Monday 08 August 2016 02:26 PM, Michael Ellerman wrote:
> Hari Bathini <hbathini@linux.vnet.ibm.com> writes:
>> On Friday 05 August 2016 12:23 AM, Hari Bathini wrote:
>>> On Thursday 04 August 2016 03:15 PM, Michael Ellerman wrote:
>>>> The code already knows how to reserve 5% based on the size of the
>>>> machine's
>>>> memory, as long as no commandline parameter is passed. So why can't we
>>>> just use that logic?
>>> That is the default value reserved but not a good enough value for
>>> every case. It is a bit difficult to come up with a robust formula
>>> that works for every case as new kernel changes could make the
>>> values obsolete. But it won't be all that difficult to find values that
>>> work for different memory ranges for a given kernel version.
>>> Passing that as range based input with "fadump_reserve_mem"
>>> parameter would work for every memory configuration on a
>>> given system, which is what this patch is trying to provide..
>> You want me to add this to the changelog on respin?

Hi Michael,

> I'm not really convinced.
>
> Distros are going to want to specify a fixed set of values for different
> memory sizes, at least that's what I've seen in the past with kdump. So
> I don't see why we can't just do that in the kernel with a formula based
> on memory size, and maybe some other information.

Agreed. Such support would be great but this patch is adding support
for a new syntax for an existing parameter which should still be good
to have?

> Maybe the formula is more complicated than 5% of RAM, but it shouldn't
> be *that* much more complicated.

Depending on what all kernel versions that need support, this can
get ugly? I could be completely wrong though..

Thanks
Hari

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