[PATCH v8 1/6] powerpc/fadump: reduce memory consumption for capture kernel

[PATCH v8 1/6] powerpc/fadump: reduce memory consumption for capture kernel

[Michal Suchanek]

With fadump (dump capture) kernel booting like a regular kernel, it needs
almost the same amount of memory to boot as the production kernel, which is
unwarranted for a dump capture kernel. But with no option to disable some
of the unnecessary subsystems in fadump kernel, that much memory is wasted
on fadump, depriving the production kernel of that memory.

Introduce kernel parameter 'fadump_extra_args=' that would take regular
parameters as a space separated quoted string, to be enforced when fadump
is active. This 'fadump_extra_args=' parameter can be leveraged to pass
parameters like nr_cpus=1, cgroup_disable=memory and numa=off, to disable
unwarranted resources/subsystems.

Also, ensure the log "Firmware-assisted dump is active" is printed early
in the boot process to put the subsequent fadump messages in context.

Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com>
Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
Changes from v6:
Correct and simplify quote handling. Ideally I would like to extend
parse_args to give the length of the original quoted value to callback.
However, parse_args removes at most one doubel-quote from the start and
one from the end so that is easy to detect. Otherwise all other users
will have to be updated to trash the new argument.
Changes from v7:
Handle leading quote in parameter name.
---
 arch/powerpc/include/asm/fadump.h |   2 +
 arch/powerpc/kernel/fadump.c      | 122 +++++++++++++++++++++++++++++++++++++-
 arch/powerpc/kernel/prom.c        |   7 +++
 3 files changed, 128 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/fadump.h b/arch/powerpc/include/asm/fadump.h
index 5a23010af600..41b50b317a67 100644
--- a/arch/powerpc/include/asm/fadump.h
+++ b/arch/powerpc/include/asm/fadump.h
@@ -208,12 +208,14 @@ extern int early_init_dt_scan_fw_dump(unsigned long node,
 		const char *uname, int depth, void *data);
 extern int fadump_reserve_mem(void);
 extern int setup_fadump(void);
+extern void enforce_fadump_extra_args(char *cmdline);
 extern int is_fadump_active(void);
 extern int should_fadump_crash(void);
 extern void crash_fadump(struct pt_regs *, const char *);
 extern void fadump_cleanup(void);
 
 #else	/* CONFIG_FA_DUMP */
+static inline void enforce_fadump_extra_args(char *cmdline) { }
 static inline int is_fadump_active(void) { return 0; }
 static inline int should_fadump_crash(void) { return 0; }
 static inline void crash_fadump(struct pt_regs *regs, const char *str) { }
diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index e1431800bfb9..0e08f1a80af2 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c
@@ -78,8 +78,10 @@ int __init early_init_dt_scan_fw_dump(unsigned long node,
 	 * dump data waiting for us.
 	 */
 	fdm_active = of_get_flat_dt_prop(node, "ibm,kernel-dump", NULL);
-	if (fdm_active)
+	if (fdm_active) {
+		pr_info("Firmware-assisted dump is active.\n");
 		fw_dump.dump_active = 1;
+	}
 
 	/* Get the sizes required to store dump data for the firmware provided
 	 * dump sections.
@@ -339,8 +341,11 @@ int __init fadump_reserve_mem(void)
 {
 	unsigned long base, size, memory_boundary;
 
-	if (!fw_dump.fadump_enabled)
+	if (!fw_dump.fadump_enabled) {
+		if (fw_dump.dump_active)
+			pr_warn("Firmware-assisted dump was active but kernel booted with fadump disabled!\n");
 		return 0;
+	}
 
 	if (!fw_dump.fadump_supported) {
 		printk(KERN_INFO "Firmware-assisted dump is not supported on"
@@ -380,7 +385,6 @@ int __init fadump_reserve_mem(void)
 		memory_boundary = memblock_end_of_DRAM();
 
 	if (fw_dump.dump_active) {
-		printk(KERN_INFO "Firmware-assisted dump is active.\n");
 		/*
 		 * If last boot has crashed then reserve all the memory
 		 * above boot_memory_size so that we don't touch it until
@@ -467,6 +471,118 @@ static int __init early_fadump_reserve_mem(char *p)
 }
 early_param("fadump_reserve_mem", early_fadump_reserve_mem);
 
+#define FADUMP_EXTRA_ARGS_PARAM		"fadump_extra_args="
+#define FADUMP_EXTRA_ARGS_LEN		(strlen(FADUMP_EXTRA_ARGS_PARAM) - 1)
+
+struct param_info {
+	char		*cmdline;
+	char		*tmp_cmdline;
+	int		 shortening;
+};
+
+static void __init fadump_update_params(struct param_info *param_info,
+					char *param, char *val)
+{
+	ptrdiff_t param_offset = param - param_info->tmp_cmdline;
+	size_t vallen = val ? strlen(val) : 0;
+	char *tgt = param_info->cmdline + param_offset
+				- param_info->shortening;
+	int shortening = 0;
+	int quoted = 0;
+
+	if (!val)
+		return;
+
+	/* leading '"' removed from parameter */
+	if ((param > param_info->tmp_cmdline) && *(param - 1) == '"') {
+		quoted = 1;
+		shortening += 1;
+		tgt--;
+	}
+
+	/* next_arg removes one leading and one trailing '"' */
+	if (*(tgt + FADUMP_EXTRA_ARGS_LEN + 1) == '"')
+		shortening += 1;
+	if (*(tgt + FADUMP_EXTRA_ARGS_LEN + 1 + vallen + shortening) == '"')
+		shortening += 1;
+
+	/* remove one leading and one trailing quote if both are present */
+	if ((val[0] == '"') && (val[vallen - 1] == '"')) {
+		shortening += 2;
+		vallen -= 2;
+		val++;
+	}
+
+	/* some characters were removed - move the trailing part of cmdline */
+	if (shortening) {
+		char *src;
+
+		strncpy(tgt, FADUMP_EXTRA_ARGS_PARAM, FADUMP_EXTRA_ARGS_LEN);
+		tgt += FADUMP_EXTRA_ARGS_LEN;
+		*tgt++ = ' ';
+
+		strncpy(tgt, val, vallen);
+		tgt += vallen;
+
+		src = tgt + shortening;
+		memmove(tgt, src, strlen(src) + 1);
+	} else {
+		/* remove the '=' */
+		*(tgt + FADUMP_EXTRA_ARGS_LEN) = ' ';
+	}
+
+	param_info->shortening += shortening;
+}
+
+/*
+ * Reworks command line parameters and splits 'fadump_extra_args=' param
+ * to enforce the parameters passed through it
+ */
+static int __init fadump_rework_cmdline_params(char *param, char *val,
+					       const char *unused, void *arg)
+{
+	struct param_info *param_info = (struct param_info *)arg;
+
+	if (strncmp(param, FADUMP_EXTRA_ARGS_PARAM,
+		     strlen(FADUMP_EXTRA_ARGS_PARAM) - 1))
+		return 0;
+
+	fadump_update_params(param_info, param, val);
+
+	return 0;
+}
+
+/*
+ * Replace every occurrence of 'fadump_extra_args="param1 param2 param3"'
+ * in cmdline with 'fadump_extra_args param1 param2 param3' by stripping
+ * off '=' and quotes, if any. This ensures that the additional parameters
+ * passed with 'fadump_extra_args=' are enforced.
+ */
+void __init enforce_fadump_extra_args(char *cmdline)
+{
+	static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata;
+	static char init_cmdline[COMMAND_LINE_SIZE] __initdata;
+	struct param_info param_info;
+
+	if (strstr(cmdline, FADUMP_EXTRA_ARGS_PARAM) == NULL)
+		return;
+
+	pr_info("Modifying command line to enforce the additional parameters passed through 'fadump_extra_args='");
+
+	param_info.cmdline = cmdline;
+	param_info.tmp_cmdline = tmp_cmdline;
+	param_info.shortening = 0;
+
+	strlcpy(init_cmdline, cmdline, COMMAND_LINE_SIZE);
+
+	strlcpy(tmp_cmdline, cmdline, COMMAND_LINE_SIZE);
+	parse_args("fadump params", tmp_cmdline, NULL, 0, 0, 0,
+			&param_info, &fadump_rework_cmdline_params);
+
+	pr_info("Original command line: %s\n", init_cmdline);
+	pr_info("Modified command line: %s\n", cmdline);
+}
+
 static int register_fw_dump(struct fadump_mem_struct *fdm)
 {
 	int rc, err;
diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index f83056297441..2e6f40217266 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -693,6 +693,13 @@ void __init early_init_devtree(void *params)
 	of_scan_flat_dt(early_init_dt_scan_root, NULL);
 	of_scan_flat_dt(early_init_dt_scan_memory_ppc, NULL);
 
+	/*
+	 * Look for 'fadump_extra_args=' parameter and enfore the additional
+	 * parameters passed to it if fadump is active.
+	 */
+	if (is_fadump_active())
+		enforce_fadump_extra_args(boot_command_line);
+
 	parse_early_param();
 
 	/* make sure we've parsed cmdline for mem= before this */
-- 
2.10.2

[PATCH v8 2/6] powerpc/fadump: update documentation about 'fadump_extra_args=' parameter

[Michal Suchanek]

With the introduction of 'fadump_extra_args=' parameter to pass additional
parameters to fadump (capture) kernel, update documentation about it.

Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com>
Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 Documentation/powerpc/firmware-assisted-dump.txt | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/Documentation/powerpc/firmware-assisted-dump.txt b/Documentation/powerpc/firmware-assisted-dump.txt
index bdd344aa18d9..2df88524d2c7 100644
--- a/Documentation/powerpc/firmware-assisted-dump.txt
+++ b/Documentation/powerpc/firmware-assisted-dump.txt
@@ -162,7 +162,19 @@ How to enable firmware-assisted dump (fadump):
 
 1. Set config option CONFIG_FA_DUMP=y and build kernel.
 2. Boot into linux kernel with 'fadump=on' kernel cmdline option.
-3. Optionally, user can also set 'crashkernel=' kernel cmdline
+3. A user can pass additional command line parameters as a space
+   separated quoted list through 'fadump_extra_args=' parameter,
+   to be enforced when fadump is active. For example, parameter
+   'fadump_extra_args="nr_cpus=1 numa=off udev.children-max=2"'
+   will be changed to 'fadump_extra_args nr_cpus=1  numa=off
+   udev.children-max=2' in-place when fadump is active. This
+   parameter has no affect when fadump is not active. Multiple
+   instances of 'fadump_extra_args=' can be passed. This provision
+   can be used to reduce memory consumption during dump capture by
+   disabling unwarranted resources/subsystems like CPUs, NUMA
+   and such. Value with spaces can be passed as
+   'fadump_extra_args=""parameter="value with spaces"""'
+4. Optionally, user can also set 'crashkernel=' kernel cmdline
    to specify size of the memory to reserve for boot memory dump
    preservation.
 
@@ -172,6 +184,12 @@ NOTE: 1. 'fadump_reserve_mem=' parameter has been deprecated. Instead
       2. If firmware-assisted dump fails to reserve memory then it
          will fallback to existing kdump mechanism if 'crashkernel='
          option is set at kernel cmdline.
+      3. Special parameters like '--' passed inside fadump_extra_args are also
+         just left in-place. So, the user is advised to consider this while
+         specifying such parameters. It may be required to quote the argument
+         to fadump_extra_args when the bootloader uses double-quotes as
+         argument delimiter as well. eg
+        append = " fadump_extra_args=\"nr_cpus=1 numa=off udev.children-max=2\""
 
 Sysfs/debugfs files:
 ------------
-- 
2.10.2

[PATCH v8 3/6] lib/cmdline.c: Remove quotes symmetrically.

[Michal Suchanek]

Remove quotes from argument value only if there is qoute on both sides.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 lib/cmdline.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/lib/cmdline.c b/lib/cmdline.c
index 171c19b6888e..6d398a8b63fc 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -227,14 +227,12 @@ char *next_arg(char *args, char **param, char **val)
 		*val = args + equals + 1;
 
 		/* Don't include quotes in value. */
-		if (**val == '"') {
-			(*val)++;
-			if (args[i-1] == '"')
-				args[i-1] = '\0';
+		if ((args[i-1] == '"') && ((quoted) || (**val == '"'))) {
+			args[i-1] = '\0';
+			if (!quoted)
+				(*val)++;
 		}
 	}
-	if (quoted && args[i-1] == '"')
-		args[i-1] = '\0';
 
 	if (args[i]) {
 		args[i] = '\0';
-- 
2.10.2

[PATCH v8 4/6] powerpc/fadump: update the dequoting logic to match lib/cmdline.c

[Michal Suchanek]

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 arch/powerpc/kernel/fadump.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index 0e08f1a80af2..b214c1e333dd 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c
@@ -501,10 +501,12 @@ static void __init fadump_update_params(struct param_info *param_info,
 	}
 
 	/* next_arg removes one leading and one trailing '"' */
-	if (*(tgt + FADUMP_EXTRA_ARGS_LEN + 1) == '"')
-		shortening += 1;
-	if (*(tgt + FADUMP_EXTRA_ARGS_LEN + 1 + vallen + shortening) == '"')
+	if ((*(tgt + FADUMP_EXTRA_ARGS_LEN + 1 + vallen + shortening) == '"') &&
+	    (quoted || (*(tgt + FADUMP_EXTRA_ARGS_LEN + 1) == '"'))) {
 		shortening += 1;
+		if (!quoted)
+			shortening += 1;
+	}
 
 	/* remove one leading and one trailing quote if both are present */
 	if ((val[0] == '"') && (val[vallen - 1] == '"')) {
-- 
2.10.2

[PATCH v8 5/6] boot/param: add pointer to current and next argument to unknown parameter callback

[Michal Suchanek]

The fadump parameter processing re-does the logic of next_arg quote
stripping to determine where the argument ends. Pass pointer to the
current and next argument instead to make this more robust.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
rebase on master
split off changes to fadump.c
add pointer to current argument to detect shortening of the parameterer name
---
 arch/powerpc/kernel/fadump.c |  1 +
 include/linux/moduleparam.h  |  1 +
 init/main.c                  |  8 ++++++--
 kernel/module.c              |  5 +++--
 kernel/params.c              | 20 +++++++++++++-------
 lib/dynamic_debug.c          |  1 +
 6 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index b214c1e333dd..8778e1cc0380 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c
@@ -541,6 +541,7 @@ static void __init fadump_update_params(struct param_info *param_info,
  * to enforce the parameters passed through it
  */
 static int __init fadump_rework_cmdline_params(char *param, char *val,
+					       char *currant, char *next,
 					       const char *unused, void *arg)
 {
 	struct param_info *param_info = (struct param_info *)arg;
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 1ee7b30dafec..e86f3f830a7f 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -327,6 +327,7 @@ extern char *parse_args(const char *name,
 		      s16 level_max,
 		      void *arg,
 		      int (*unknown)(char *param, char *val,
+				     char *currant, char *next,
 				     const char *doing, void *arg));
 
 /* Called by module remove. */
diff --git a/init/main.c b/init/main.c
index 0ee9c6866ada..9381aa24bca7 100644
--- a/init/main.c
+++ b/init/main.c
@@ -240,6 +240,7 @@ early_param("loglevel", loglevel);
 
 /* Change NUL term back to "=", to make "param" the whole string. */
 static int __init repair_env_string(char *param, char *val,
+				    char *unused3, char *unused2,
 				    const char *unused, void *arg)
 {
 	if (val) {
@@ -258,6 +259,7 @@ static int __init repair_env_string(char *param, char *val,
 
 /* Anything after -- gets handed straight to init. */
 static int __init set_init_arg(char *param, char *val,
+			       char *unused3, char *unused2,
 			       const char *unused, void *arg)
 {
 	unsigned int i;
@@ -265,7 +267,7 @@ static int __init set_init_arg(char *param, char *val,
 	if (panic_later)
 		return 0;
 
-	repair_env_string(param, val, unused, NULL);
+	repair_env_string(param, val, unused3, unused2, unused, NULL);
 
 	for (i = 0; argv_init[i]; i++) {
 		if (i == MAX_INIT_ARGS) {
@@ -283,9 +285,10 @@ static int __init set_init_arg(char *param, char *val,
  * unused parameters (modprobe will find them in /proc/cmdline).
  */
 static int __init unknown_bootoption(char *param, char *val,
+				     char *unused3, char *unused2,
 				     const char *unused, void *arg)
 {
-	repair_env_string(param, val, unused, NULL);
+	repair_env_string(param, val, unused3, unused2, unused, NULL);
 
 	/* Handle obsolete-style parameters */
 	if (obsolete_checksetup(param))
@@ -437,6 +440,7 @@ static noinline void __ref rest_init(void)
 
 /* Check for early params. */
 static int __init do_early_param(char *param, char *val,
+				 char *unused3, char *unused2,
 				 const char *unused, void *arg)
 {
 	const struct obs_kernel_param *p;
diff --git a/kernel/module.c b/kernel/module.c
index 40f983cbea81..0f74718f8934 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3609,8 +3609,9 @@ static int prepare_coming_module(struct module *mod)
 	return 0;
 }
 
-static int unknown_module_param_cb(char *param, char *val, const char *modname,
-				   void *arg)
+static int unknown_module_param_cb(char *param, char *val,
+				   char *unused, char *unused2,
+				   const char *modname, void *arg)
 {
 	struct module *mod = arg;
 	int ret;
diff --git a/kernel/params.c b/kernel/params.c
index 60b2d8101355..c0e0c65f460b 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -119,6 +119,8 @@ static void param_check_unsafe(const struct kernel_param *kp)
 
 static int parse_one(char *param,
 		     char *val,
+		     char *currant,
+		     char *next,
 		     const char *doing,
 		     const struct kernel_param *params,
 		     unsigned num_params,
@@ -126,7 +128,8 @@ static int parse_one(char *param,
 		     s16 max_level,
 		     void *arg,
 		     int (*handle_unknown)(char *param, char *val,
-				     const char *doing, void *arg))
+					   char *currant, char *next,
+					   const char *doing, void *arg))
 {
 	unsigned int i;
 	int err;
@@ -153,7 +156,7 @@ static int parse_one(char *param,
 
 	if (handle_unknown) {
 		pr_debug("doing %s: %s='%s'\n", doing, param, val);
-		return handle_unknown(param, val, doing, arg);
+		return handle_unknown(param, val, currant, next, doing, arg);
 	}
 
 	pr_debug("Unknown argument '%s'\n", param);
@@ -169,9 +172,10 @@ char *parse_args(const char *doing,
 		 s16 max_level,
 		 void *arg,
 		 int (*unknown)(char *param, char *val,
+				char *currant, char *next,
 				const char *doing, void *arg))
 {
-	char *param, *val, *err = NULL;
+	char *param, *val, *next, *err = NULL;
 
 	/* Chew leading spaces */
 	args = skip_spaces(args);
@@ -179,16 +183,18 @@ char *parse_args(const char *doing,
 	if (*args)
 		pr_debug("doing %s, parsing ARGS: '%s'\n", doing, args);
 
-	while (*args) {
+	next = next_arg(args, &param, &val);
+	do {
 		int ret;
 		int irq_was_disabled;
 
-		args = next_arg(args, &param, &val);
+		args = next;
+		next = next_arg(args, &param, &val);
 		/* Stop at -- */
 		if (!val && strcmp(param, "--") == 0)
 			return err ?: args;
 		irq_was_disabled = irqs_disabled();
-		ret = parse_one(param, val, doing, params, num,
+		ret = parse_one(param, val, args, next, doing, params, num,
 				min_level, max_level, arg, unknown);
 		if (irq_was_disabled && !irqs_disabled())
 			pr_warn("%s: option '%s' enabled irq's!\n",
@@ -211,7 +217,7 @@ char *parse_args(const char *doing,
 		}
 
 		err = ERR_PTR(ret);
-	}
+	} while (*next);
 
 	return err;
 }
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index da796e2dc4f5..dec7f40c3f47 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -889,6 +889,7 @@ static int ddebug_dyndbg_param_cb(char *param, char *val,
 
 /* handle both dyndbg and $module.dyndbg params at boot */
 static int ddebug_dyndbg_boot_param_cb(char *param, char *val,
+				char *unused3, char *unused2,
 				const char *unused, void *arg)
 {
 	vpr_info("%s=\"%s\"\n", param, val);
-- 
2.10.2

[PATCH v8 6/6] powerpc/fadump: use the new parse_args callback arguments

[Michal Suchanek]

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 arch/powerpc/kernel/fadump.c | 47 ++++++++++++--------------------------------
 1 file changed, 13 insertions(+), 34 deletions(-)

diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index 8778e1cc0380..1678d99ea835 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c
@@ -481,33 +481,19 @@ struct param_info {
 };
 
 static void __init fadump_update_params(struct param_info *param_info,
-					char *param, char *val)
+					char *param, char *val,
+					char *currant, char *next)
 {
-	ptrdiff_t param_offset = param - param_info->tmp_cmdline;
+	ptrdiff_t param_offset = currant - param_info->tmp_cmdline;
 	size_t vallen = val ? strlen(val) : 0;
 	char *tgt = param_info->cmdline + param_offset
 				- param_info->shortening;
-	int shortening = 0;
-	int quoted = 0;
+	int shortening = ((next - 1) - (currant))
+		- (FADUMP_EXTRA_ARGS_LEN + 1 + vallen);
 
 	if (!val)
 		return;
 
-	/* leading '"' removed from parameter */
-	if ((param > param_info->tmp_cmdline) && *(param - 1) == '"') {
-		quoted = 1;
-		shortening += 1;
-		tgt--;
-	}
-
-	/* next_arg removes one leading and one trailing '"' */
-	if ((*(tgt + FADUMP_EXTRA_ARGS_LEN + 1 + vallen + shortening) == '"') &&
-	    (quoted || (*(tgt + FADUMP_EXTRA_ARGS_LEN + 1) == '"'))) {
-		shortening += 1;
-		if (!quoted)
-			shortening += 1;
-	}
-
 	/* remove one leading and one trailing quote if both are present */
 	if ((val[0] == '"') && (val[vallen - 1] == '"')) {
 		shortening += 2;
@@ -515,22 +501,15 @@ static void __init fadump_update_params(struct param_info *param_info,
 		val++;
 	}
 
-	/* some characters were removed - move the trailing part of cmdline */
-	if (shortening) {
-		char *src;
+	strncpy(tgt, FADUMP_EXTRA_ARGS_PARAM, FADUMP_EXTRA_ARGS_LEN);
+	tgt += FADUMP_EXTRA_ARGS_LEN;
+	*tgt++ = ' ';
+	strncpy(tgt, val, vallen);
+	tgt += vallen;
 
-		strncpy(tgt, FADUMP_EXTRA_ARGS_PARAM, FADUMP_EXTRA_ARGS_LEN);
-		tgt += FADUMP_EXTRA_ARGS_LEN;
-		*tgt++ = ' ';
-
-		strncpy(tgt, val, vallen);
-		tgt += vallen;
-
-		src = tgt + shortening;
+	if (shortening) {
+		char *src = tgt + shortening;
 		memmove(tgt, src, strlen(src) + 1);
-	} else {
-		/* remove the '=' */
-		*(tgt + FADUMP_EXTRA_ARGS_LEN) = ' ';
 	}
 
 	param_info->shortening += shortening;
@@ -550,7 +529,7 @@ static int __init fadump_rework_cmdline_params(char *param, char *val,
 		     strlen(FADUMP_EXTRA_ARGS_PARAM) - 1))
 		return 0;
 
-	fadump_update_params(param_info, param, val);
+	fadump_update_params(param_info, param, val, currant, next);
 
 	return 0;
 }
-- 
2.10.2

[PATCH 1/6] lib/cmdline.c: Add backslash support to kernel commandline parsing.

[Michal Suchanek]

This allows passing quotes in kernel arguments. It is useful for passing
fadump nested arguemnts in fadump_extra_args and might be useful if
somebody wanted to pass a double quote directly as part of an argument.

It is also useful to have quoting grammar more similar to shells and
bootloaders.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 lib/cmdline.c | 41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/lib/cmdline.c b/lib/cmdline.c
index 6d398a8b63fc..d98bdc017545 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -193,30 +193,36 @@ bool parse_option_str(const char *str, const char *option)
 
 /*
  * Parse a string to get a param value pair.
- * You can use " around spaces, but can't escape ".
+ * You can use " around spaces, and you can escape with \
  * Hyphens and underscores equivalent in parameter names.
  */
 char *next_arg(char *args, char **param, char **val)
 {
 	unsigned int i, equals = 0;
-	int in_quote = 0, quoted = 0;
+	int in_quote = 0, backslash = 0;
 	char *next;
 
-	if (*args == '"') {
-		args++;
-		in_quote = 1;
-		quoted = 1;
-	}
-
 	for (i = 0; args[i]; i++) {
-		if (isspace(args[i]) && !in_quote)
+		if (isspace(args[i]) && !in_quote && !backslash)
 			break;
-		if (equals == 0) {
-			if (args[i] == '=')
-				equals = i;
+
+		if ((equals == 0) && (args[i] == '='))
+			equals = i;
+
+		if (!backslash) {
+			if ((args[i] == '"') || (args[i] == '\\')) {
+				if (args[i] == '"')
+					in_quote = !in_quote;
+				if (args[i] == '\\')
+					backslash = 1;
+
+				memmove(args + 1, args, i);
+				args++;
+				i--;
+			}
+		} else {
+			backslash = 0;
 		}
-		if (args[i] == '"')
-			in_quote = !in_quote;
 	}
 
 	*param = args;
@@ -225,13 +231,6 @@ char *next_arg(char *args, char **param, char **val)
 	else {
 		args[equals] = '\0';
 		*val = args + equals + 1;
-
-		/* Don't include quotes in value. */
-		if ((args[i-1] == '"') && ((quoted) || (**val == '"'))) {
-			args[i-1] = '\0';
-			if (!quoted)
-				(*val)++;
-		}
 	}
 
 	if (args[i]) {
-- 
2.10.2

[PATCH 2/6] Documentation/admin-guide: backslash support in commandline.

[Michal Suchanek]

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 Documentation/admin-guide/kernel-parameters.rst | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst
index b2598cc9834c..722d3f771924 100644
--- a/Documentation/admin-guide/kernel-parameters.rst
+++ b/Documentation/admin-guide/kernel-parameters.rst
@@ -35,9 +35,9 @@ can also be entered as::
 
 	log-buf-len=1M print_fatal_signals=1
 
-Double-quotes can be used to protect spaces in values, e.g.::
+Double-quotes and backslashes can be used to protect spaces in values, e.g.::
 
-	param="spaces in here"
+	param="spaces in here" param2=spaces\ in\ here
 
 cpu lists:
 ----------
-- 
2.10.2

[PATCH 3/6] powerpc/fadump: stop removing quotes in argument parsing.

[Michal Suchanek]

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 arch/powerpc/kernel/fadump.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index 1678d99ea835..275ea42a27d5 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c
@@ -494,13 +494,6 @@ static void __init fadump_update_params(struct param_info *param_info,
 	if (!val)
 		return;
 
-	/* remove one leading and one trailing quote if both are present */
-	if ((val[0] == '"') && (val[vallen - 1] == '"')) {
-		shortening += 2;
-		vallen -= 2;
-		val++;
-	}
-
 	strncpy(tgt, FADUMP_EXTRA_ARGS_PARAM, FADUMP_EXTRA_ARGS_LEN);
 	tgt += FADUMP_EXTRA_ARGS_LEN;
 	*tgt++ = ' ';
-- 
2.10.2

[PATCH 4/6] powerpc/fadump: Update fadump ducumentation on quoting arguments.

[Michal Suchanek]

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 Documentation/powerpc/firmware-assisted-dump.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/powerpc/firmware-assisted-dump.txt b/Documentation/powerpc/firmware-assisted-dump.txt
index 2df88524d2c7..5705f55ffae4 100644
--- a/Documentation/powerpc/firmware-assisted-dump.txt
+++ b/Documentation/powerpc/firmware-assisted-dump.txt
@@ -173,7 +173,7 @@ How to enable firmware-assisted dump (fadump):
    can be used to reduce memory consumption during dump capture by
    disabling unwarranted resources/subsystems like CPUs, NUMA
    and such. Value with spaces can be passed as
-   'fadump_extra_args=""parameter="value with spaces"""'
+   'fadump_extra_args="parameter=\"value with spaces\""'
 4. Optionally, user can also set 'crashkernel=' kernel cmdline
    to specify size of the memory to reserve for boot memory dump
    preservation.
-- 
2.10.2

[PATCH 5/6] lib/cmdline.c: Implement single quotes in commandline argument parsing

[Michal Suchanek]

This brings the kernel parser about on par with bourne shell, grub, and
other tools that chew the arguments before kernel does.

This should make it easier to deal with multiple levels of
nesting/quoting. With same quoting grammar on each level there is less
room for confusion.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 lib/cmdline.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/lib/cmdline.c b/lib/cmdline.c
index d98bdc017545..c5335a79a177 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -191,34 +191,45 @@ bool parse_option_str(const char *str, const char *option)
 	return false;
 }
 
+#define squash_char { \
+	memmove(args + 1, args, i); \
+	args++; \
+	i--; \
+}
+
 /*
  * Parse a string to get a param value pair.
- * You can use " around spaces, and you can escape with \
+ * You can use " or ' around spaces, and you can escape with \
  * Hyphens and underscores equivalent in parameter names.
  */
 char *next_arg(char *args, char **param, char **val)
 {
 	unsigned int i, equals = 0;
-	int in_quote = 0, backslash = 0;
+	int in_quote = 0, backslash = 0, in_single = 0;
 	char *next;
 
 	for (i = 0; args[i]; i++) {
-		if (isspace(args[i]) && !in_quote && !backslash)
+		if (isspace(args[i]) && !in_quote && !backslash && !in_single)
 			break;
 
 		if ((equals == 0) && (args[i] == '='))
 			equals = i;
 
-		if (!backslash) {
-			if ((args[i] == '"') || (args[i] == '\\')) {
+		if (in_single) {
+			if (args[i] == '\'') {
+				in_single = 0;
+				squash_char;
+			}
+		} else if (!backslash) {
+			if ((args[i] == '"') || (args[i] == '\\') ||
+					(args[i] == '\'')) {
 				if (args[i] == '"')
 					in_quote = !in_quote;
 				if (args[i] == '\\')
 					backslash = 1;
-
-				memmove(args + 1, args, i);
-				args++;
-				i--;
+				if (args[i] == '\'')
+					in_single = 1;
+				squash_char;
 			}
 		} else {
 			backslash = 0;
-- 
2.10.2

[PATCH 6/6] Documentation/admin-guide: single quotes in kernel arguments.

[Michal Suchanek]

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 Documentation/admin-guide/kernel-parameters.rst | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst
index 722d3f771924..1f9837266417 100644
--- a/Documentation/admin-guide/kernel-parameters.rst
+++ b/Documentation/admin-guide/kernel-parameters.rst
@@ -35,9 +35,10 @@ can also be entered as::
 
 	log-buf-len=1M print_fatal_signals=1
 
-Double-quotes and backslashes can be used to protect spaces in values, e.g.::
+Double-quotes single-quaotes and backslashes can be used to protect spaces
+in values, e.g.::
 
-	param="spaces in here" param2=spaces\ in\ here
+	param="spaces in here" param2=spaces\ in\ here param3='@%# !\'
 
 cpu lists:
 ----------
-- 
2.10.2

Re: [PATCH 1/6] lib/cmdline.c: Add backslash support to kernel commandline parsing.

[Al Viro]

On Fri, Sep 15, 2017 at 07:02:46PM +0200, Michal Suchanek wrote:

>  	for (i = 0; args[i]; i++) {
> -		if (isspace(args[i]) && !in_quote)
> +		if (isspace(args[i]) && !in_quote && !backslash)
>  			break;
> -		if (equals == 0) {
> -			if (args[i] == '=')
> -				equals = i;
> +
> +		if ((equals == 0) && (args[i] == '='))
> +			equals = i;
> +
> +		if (!backslash) {
> +			if ((args[i] == '"') || (args[i] == '\\')) {
> +				if (args[i] == '"')
> +					in_quote = !in_quote;
> +				if (args[i] == '\\')
> +					backslash = 1;
> +
> +				memmove(args + 1, args, i);
> +				args++;
> +				i--;
> +			}
> +		} else {
> +			backslash = 0;
>  		}
> -		if (args[i] == '"')
> -			in_quote = !in_quote;
>  	}

... and that makes for Unidiomatic Work With Strings Award for this September.
Using memmove() for string rewrite is almost always bad taste; in this case
it's also (as usual) broken.

Re: [PATCH 1/6] lib/cmdline.c: Add backslash support to kernel commandline parsing.

[Michal Suchánek]

On Fri, 15 Sep 2017 18:14:09 +0100
Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Fri, Sep 15, 2017 at 07:02:46PM +0200, Michal Suchanek wrote:
> 
> >  	for (i = 0; args[i]; i++) {
> > -		if (isspace(args[i]) && !in_quote)
> > +		if (isspace(args[i]) && !in_quote && !backslash)
> >  			break;
> > -		if (equals == 0) {
> > -			if (args[i] == '=')
> > -				equals = i;
> > +
> > +		if ((equals == 0) && (args[i] == '='))
> > +			equals = i;
> > +
> > +		if (!backslash) {
> > +			if ((args[i] == '"') || (args[i] == '\\'))
> > {
> > +				if (args[i] == '"')
> > +					in_quote = !in_quote;
> > +				if (args[i] == '\\')
> > +					backslash = 1;
> > +
> > +				memmove(args + 1, args, i);
> > +				args++;
> > +				i--;
> > +			}
> > +		} else {
> > +			backslash = 0;
> >  		}
> > -		if (args[i] == '"')
> > -			in_quote = !in_quote;
> >  	}  
> 
> ... and that makes for Unidiomatic Work With Strings Award for this
> September. Using memmove() for string rewrite is almost always bad
> taste; in this case it's also (as usual) broken.

Care to share how it is broken?

Thanks

Michal

Re: [PATCH 6/6] Documentation/admin-guide: single quotes in kernel arguments.

[Randy Dunlap]

On 09/15/17 10:02, Michal Suchanek wrote:
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
>  Documentation/admin-guide/kernel-parameters.rst | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst
> index 722d3f771924..1f9837266417 100644
> --- a/Documentation/admin-guide/kernel-parameters.rst
> +++ b/Documentation/admin-guide/kernel-parameters.rst
> @@ -35,9 +35,10 @@ can also be entered as::
>  
>  	log-buf-len=1M print_fatal_signals=1
>  
> -Double-quotes and backslashes can be used to protect spaces in values, e.g.::
> +Double-quotes single-quaotes and backslashes can be used to protect spaces

                 single-quotes

> +in values, e.g.::
>  
> -	param="spaces in here" param2=spaces\ in\ here
> +	param="spaces in here" param2=spaces\ in\ here param3='@%# !\'
>  
>  cpu lists:
>  ----------
> 


-- 
~Randy

Re: [PATCH 1/6] lib/cmdline.c: Add backslash support to kernel commandline parsing.

[Michal Suchánek]

On Fri, 15 Sep 2017 19:28:56 +0200
Michal Suchánek <msuchanek@suse.de> wrote:

> On Fri, 15 Sep 2017 18:14:09 +0100
> Al Viro <viro@ZenIV.linux.org.uk> wrote:
> 
> > On Fri, Sep 15, 2017 at 07:02:46PM +0200, Michal Suchanek wrote:
> >   
> > >  	for (i = 0; args[i]; i++) {
> > > -		if (isspace(args[i]) && !in_quote)
> > > +		if (isspace(args[i]) && !in_quote && !backslash)
> > >  			break;
> > > -		if (equals == 0) {
> > > -			if (args[i] == '=')
> > > -				equals = i;
> > > +
> > > +		if ((equals == 0) && (args[i] == '='))
> > > +			equals = i;
> > > +
> > > +		if (!backslash) {
> > > +			if ((args[i] == '"') || (args[i] ==
> > > '\\')) {
> > > +				if (args[i] == '"')
> > > +					in_quote = !in_quote;
> > > +				if (args[i] == '\\')
> > > +					backslash = 1;
> > > +
> > > +				memmove(args + 1, args, i);
> > > +				args++;
> > > +				i--;
> > > +			}
> > > +		} else {
> > > +			backslash = 0;
> > >  		}
> > > -		if (args[i] == '"')
> > > -			in_quote = !in_quote;
> > >  	}    
> > 
> > ... and that makes for Unidiomatic Work With Strings Award for this
> > September. Using memmove() for string rewrite is almost always bad
> > taste; in this case it's also (as usual) broken.  
> 
> Care to share how it is broken?

Guess not. I will assume it is perfectly fine then. It works perfectly
fine in my testing.

Using memmove for string rewrite is not a matter of taste. It is the
only library function with sane semantics for rewrite of anything. Then
again open-coding it is always an option and maybe in better taste for
some :->

Michal

Re: [PATCH 1/6] lib/cmdline.c: Add backslash support to kernel commandline parsing.

[Michal Suchánek]

On Fri, 15 Sep 2017 19:28:56 +0200
Michal Such=C3=A1nek <msuchanek@suse.de> wrote:

> On Fri, 15 Sep 2017 18:14:09 +0100
> Al Viro <viro@ZenIV.linux.org.uk> wrote:
>=20
> > On Fri, Sep 15, 2017 at 07:02:46PM +0200, Michal Suchanek wrote:
> >  =20
> > >  	for (i =3D 0; args[i]; i++) {
> > > -		if (isspace(args[i]) && !in_quote)
> > > +		if (isspace(args[i]) && !in_quote && !backslash)
> > >  			break;
> > > -		if (equals =3D=3D 0) {
> > > -			if (args[i] =3D=3D '=3D')
> > > -				equals =3D i;
> > > +
> > > +		if ((equals =3D=3D 0) && (args[i] =3D=3D '=3D'))
> > > +			equals =3D i;
> > > +
> > > +		if (!backslash) {
> > > +			if ((args[i] =3D=3D '"') || (args[i] =3D=3D
> > > '\\')) {
> > > +				if (args[i] =3D=3D '"')
> > > +					in_quote =3D !in_quote;
> > > +				if (args[i] =3D=3D '\\')
> > > +					backslash =3D 1;
> > > +
> > > +				memmove(args + 1, args, i);
> > > +				args++;
> > > +				i--;
> > > +			}
> > > +		} else {
> > > +			backslash =3D 0;
> > >  		}
> > > -		if (args[i] =3D=3D '"')
> > > -			in_quote =3D !in_quote;
> > >  	}   =20
> >=20
> > ... and that makes for Unidiomatic Work With Strings Award for this
> > September. Using memmove() for string rewrite is almost always bad
> > taste; in this case it's also (as usual) broken. =20
>=20
> Care to share how it is broken?

Guess not. I will assume it is perfectly fine then. It works perfectly
fine in my testing.

Using memmove for string rewrite is not a matter of taste. It is the
only library function with sane semantics for rewrite of anything. Then
again open-coding it is always an option and maybe in better taste for
some :->

Michal

Re: [PATCH 6/6] Documentation/admin-guide: single quotes in kernel arguments.

[Michal Suchánek]

On Fri, 15 Sep 2017 10:59:00 -0700
Randy Dunlap <rdunlap@infradead.org> wrote:

> On 09/15/17 10:02, Michal Suchanek wrote:
> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > ---
> >  Documentation/admin-guide/kernel-parameters.rst | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/admin-guide/kernel-parameters.rst
> > b/Documentation/admin-guide/kernel-parameters.rst index
> > 722d3f771924..1f9837266417 100644 ---
> > a/Documentation/admin-guide/kernel-parameters.rst +++
> > b/Documentation/admin-guide/kernel-parameters.rst @@ -35,9 +35,10
> > @@ can also be entered as:: 
> >  	log-buf-len=1M print_fatal_signals=1
> >  
> > -Double-quotes and backslashes can be used to protect spaces in
> > values, e.g.:: +Double-quotes single-quaotes and backslashes can be
> > used to protect spaces  
> 
>                  single-quotes
> 

Thanks for catching the typo

Michal

Re: [PATCH v8 1/6] powerpc/fadump: reduce memory consumption for capture kernel

[Hari Bathini]

[-- Attachment #1: Type: text/plain, Size: 8498 bytes --]



On Tuesday 12 September 2017 09:31 PM, Michal Suchanek wrote:
> With fadump (dump capture) kernel booting like a regular kernel, it needs
> almost the same amount of memory to boot as the production kernel, which is
> unwarranted for a dump capture kernel. But with no option to disable some
> of the unnecessary subsystems in fadump kernel, that much memory is wasted
> on fadump, depriving the production kernel of that memory.
>
> Introduce kernel parameter 'fadump_extra_args=' that would take regular
> parameters as a space separated quoted string, to be enforced when fadump
> is active. This 'fadump_extra_args=' parameter can be leveraged to pass
> parameters like nr_cpus=1, cgroup_disable=memory and numa=off, to disable
> unwarranted resources/subsystems.
>
> Also, ensure the log "Firmware-assisted dump is active" is printed early
> in the boot process to put the subsequent fadump messages in context.
>
> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>

For the series...

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

> ---
> Changes from v6:
> Correct and simplify quote handling. Ideally I would like to extend
> parse_args to give the length of the original quoted value to callback.
> However, parse_args removes at most one doubel-quote from the start and
> one from the end so that is easy to detect. Otherwise all other users
> will have to be updated to trash the new argument.
> Changes from v7:
> Handle leading quote in parameter name.
> ---
>   arch/powerpc/include/asm/fadump.h |   2 +
>   arch/powerpc/kernel/fadump.c      | 122 +++++++++++++++++++++++++++++++++++++-
>   arch/powerpc/kernel/prom.c        |   7 +++
>   3 files changed, 128 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/fadump.h b/arch/powerpc/include/asm/fadump.h
> index 5a23010af600..41b50b317a67 100644
> --- a/arch/powerpc/include/asm/fadump.h
> +++ b/arch/powerpc/include/asm/fadump.h
> @@ -208,12 +208,14 @@ extern int early_init_dt_scan_fw_dump(unsigned long node,
>   		const char *uname, int depth, void *data);
>   extern int fadump_reserve_mem(void);
>   extern int setup_fadump(void);
> +extern void enforce_fadump_extra_args(char *cmdline);
>   extern int is_fadump_active(void);
>   extern int should_fadump_crash(void);
>   extern void crash_fadump(struct pt_regs *, const char *);
>   extern void fadump_cleanup(void);
>
>   #else	/* CONFIG_FA_DUMP */
> +static inline void enforce_fadump_extra_args(char *cmdline) { }
>   static inline int is_fadump_active(void) { return 0; }
>   static inline int should_fadump_crash(void) { return 0; }
>   static inline void crash_fadump(struct pt_regs *regs, const char *str) { }
> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
> index e1431800bfb9..0e08f1a80af2 100644
> --- a/arch/powerpc/kernel/fadump.c
> +++ b/arch/powerpc/kernel/fadump.c
> @@ -78,8 +78,10 @@ int __init early_init_dt_scan_fw_dump(unsigned long node,
>   	 * dump data waiting for us.
>   	 */
>   	fdm_active = of_get_flat_dt_prop(node, "ibm,kernel-dump", NULL);
> -	if (fdm_active)
> +	if (fdm_active) {
> +		pr_info("Firmware-assisted dump is active.\n");
>   		fw_dump.dump_active = 1;
> +	}
>
>   	/* Get the sizes required to store dump data for the firmware provided
>   	 * dump sections.
> @@ -339,8 +341,11 @@ int __init fadump_reserve_mem(void)
>   {
>   	unsigned long base, size, memory_boundary;
>
> -	if (!fw_dump.fadump_enabled)
> +	if (!fw_dump.fadump_enabled) {
> +		if (fw_dump.dump_active)
> +			pr_warn("Firmware-assisted dump was active but kernel booted with fadump disabled!\n");
>   		return 0;
> +	}
>
>   	if (!fw_dump.fadump_supported) {
>   		printk(KERN_INFO "Firmware-assisted dump is not supported on"
> @@ -380,7 +385,6 @@ int __init fadump_reserve_mem(void)
>   		memory_boundary = memblock_end_of_DRAM();
>
>   	if (fw_dump.dump_active) {
> -		printk(KERN_INFO "Firmware-assisted dump is active.\n");
>   		/*
>   		 * If last boot has crashed then reserve all the memory
>   		 * above boot_memory_size so that we don't touch it until
> @@ -467,6 +471,118 @@ static int __init early_fadump_reserve_mem(char *p)
>   }
>   early_param("fadump_reserve_mem", early_fadump_reserve_mem);
>
> +#define FADUMP_EXTRA_ARGS_PARAM		"fadump_extra_args="
> +#define FADUMP_EXTRA_ARGS_LEN		(strlen(FADUMP_EXTRA_ARGS_PARAM) - 1)
> +
> +struct param_info {
> +	char		*cmdline;
> +	char		*tmp_cmdline;
> +	int		 shortening;
> +};
> +
> +static void __init fadump_update_params(struct param_info *param_info,
> +					char *param, char *val)
> +{
> +	ptrdiff_t param_offset = param - param_info->tmp_cmdline;
> +	size_t vallen = val ? strlen(val) : 0;
> +	char *tgt = param_info->cmdline + param_offset
> +				- param_info->shortening;
> +	int shortening = 0;
> +	int quoted = 0;
> +
> +	if (!val)
> +		return;
> +
> +	/* leading '"' removed from parameter */
> +	if ((param > param_info->tmp_cmdline) && *(param - 1) == '"') {
> +		quoted = 1;
> +		shortening += 1;
> +		tgt--;
> +	}
> +
> +	/* next_arg removes one leading and one trailing '"' */
> +	if (*(tgt + FADUMP_EXTRA_ARGS_LEN + 1) == '"')
> +		shortening += 1;
> +	if (*(tgt + FADUMP_EXTRA_ARGS_LEN + 1 + vallen + shortening) == '"')
> +		shortening += 1;
> +
> +	/* remove one leading and one trailing quote if both are present */
> +	if ((val[0] == '"') && (val[vallen - 1] == '"')) {
> +		shortening += 2;
> +		vallen -= 2;
> +		val++;
> +	}
> +
> +	/* some characters were removed - move the trailing part of cmdline */
> +	if (shortening) {
> +		char *src;
> +
> +		strncpy(tgt, FADUMP_EXTRA_ARGS_PARAM, FADUMP_EXTRA_ARGS_LEN);
> +		tgt += FADUMP_EXTRA_ARGS_LEN;
> +		*tgt++ = ' ';
> +
> +		strncpy(tgt, val, vallen);
> +		tgt += vallen;
> +
> +		src = tgt + shortening;
> +		memmove(tgt, src, strlen(src) + 1);
> +	} else {
> +		/* remove the '=' */
> +		*(tgt + FADUMP_EXTRA_ARGS_LEN) = ' ';
> +	}
> +
> +	param_info->shortening += shortening;
> +}
> +
> +/*
> + * Reworks command line parameters and splits 'fadump_extra_args=' param
> + * to enforce the parameters passed through it
> + */
> +static int __init fadump_rework_cmdline_params(char *param, char *val,
> +					       const char *unused, void *arg)
> +{
> +	struct param_info *param_info = (struct param_info *)arg;
> +
> +	if (strncmp(param, FADUMP_EXTRA_ARGS_PARAM,
> +		     strlen(FADUMP_EXTRA_ARGS_PARAM) - 1))
> +		return 0;
> +
> +	fadump_update_params(param_info, param, val);
> +
> +	return 0;
> +}
> +
> +/*
> + * Replace every occurrence of 'fadump_extra_args="param1 param2 param3"'
> + * in cmdline with 'fadump_extra_args param1 param2 param3' by stripping
> + * off '=' and quotes, if any. This ensures that the additional parameters
> + * passed with 'fadump_extra_args=' are enforced.
> + */
> +void __init enforce_fadump_extra_args(char *cmdline)
> +{
> +	static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata;
> +	static char init_cmdline[COMMAND_LINE_SIZE] __initdata;
> +	struct param_info param_info;
> +
> +	if (strstr(cmdline, FADUMP_EXTRA_ARGS_PARAM) == NULL)
> +		return;
> +
> +	pr_info("Modifying command line to enforce the additional parameters passed through 'fadump_extra_args='");
> +
> +	param_info.cmdline = cmdline;
> +	param_info.tmp_cmdline = tmp_cmdline;
> +	param_info.shortening = 0;
> +
> +	strlcpy(init_cmdline, cmdline, COMMAND_LINE_SIZE);
> +
> +	strlcpy(tmp_cmdline, cmdline, COMMAND_LINE_SIZE);
> +	parse_args("fadump params", tmp_cmdline, NULL, 0, 0, 0,
> +			&param_info, &fadump_rework_cmdline_params);
> +
> +	pr_info("Original command line: %s\n", init_cmdline);
> +	pr_info("Modified command line: %s\n", cmdline);
> +}
> +
>   static int register_fw_dump(struct fadump_mem_struct *fdm)
>   {
>   	int rc, err;
> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index f83056297441..2e6f40217266 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -693,6 +693,13 @@ void __init early_init_devtree(void *params)
>   	of_scan_flat_dt(early_init_dt_scan_root, NULL);
>   	of_scan_flat_dt(early_init_dt_scan_memory_ppc, NULL);
>
> +	/*
> +	 * Look for 'fadump_extra_args=' parameter and enfore the additional
> +	 * parameters passed to it if fadump is active.
> +	 */
> +	if (is_fadump_active())
> +		enforce_fadump_extra_args(boot_command_line);
> +
>   	parse_early_param();
>
>   	/* make sure we've parsed cmdline for mem= before this */


[-- Attachment #2: Type: text/html, Size: 9188 bytes --]

Re: [PATCH v8 1/6] powerpc/fadump: reduce memory consumption for capture kernel

[Hari Bathini]

[-- Attachment #1: Type: text/plain, Size: 8496 bytes --]



On Tuesday 12 September 2017 09:31 PM, Michal Suchanek wrote:
> With fadump (dump capture) kernel booting like a regular kernel, it needs
> almost the same amount of memory to boot as the production kernel, which is
> unwarranted for a dump capture kernel. But with no option to disable some
> of the unnecessary subsystems in fadump kernel, that much memory is wasted
> on fadump, depriving the production kernel of that memory.
>
> Introduce kernel parameter 'fadump_extra_args=' that would take regular
> parameters as a space separated quoted string, to be enforced when fadump
> is active. This 'fadump_extra_args=' parameter can be leveraged to pass
> parameters like nr_cpus=1, cgroup_disable=memory and numa=off, to disable
> unwarranted resources/subsystems.
>
> Also, ensure the log "Firmware-assisted dump is active" is printed early
> in the boot process to put the subsequent fadump messages in context.
>
> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>

For the series..
Tested-by: Hari Bathini <hbathini@linux.vnet.ibm.com>

> ---
> Changes from v6:
> Correct and simplify quote handling. Ideally I would like to extend
> parse_args to give the length of the original quoted value to callback.
> However, parse_args removes at most one doubel-quote from the start and
> one from the end so that is easy to detect. Otherwise all other users
> will have to be updated to trash the new argument.
> Changes from v7:
> Handle leading quote in parameter name.
> ---
>   arch/powerpc/include/asm/fadump.h |   2 +
>   arch/powerpc/kernel/fadump.c      | 122 +++++++++++++++++++++++++++++++++++++-
>   arch/powerpc/kernel/prom.c        |   7 +++
>   3 files changed, 128 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/fadump.h b/arch/powerpc/include/asm/fadump.h
> index 5a23010af600..41b50b317a67 100644
> --- a/arch/powerpc/include/asm/fadump.h
> +++ b/arch/powerpc/include/asm/fadump.h
> @@ -208,12 +208,14 @@ extern int early_init_dt_scan_fw_dump(unsigned long node,
>   		const char *uname, int depth, void *data);
>   extern int fadump_reserve_mem(void);
>   extern int setup_fadump(void);
> +extern void enforce_fadump_extra_args(char *cmdline);
>   extern int is_fadump_active(void);
>   extern int should_fadump_crash(void);
>   extern void crash_fadump(struct pt_regs *, const char *);
>   extern void fadump_cleanup(void);
>
>   #else	/* CONFIG_FA_DUMP */
> +static inline void enforce_fadump_extra_args(char *cmdline) { }
>   static inline int is_fadump_active(void) { return 0; }
>   static inline int should_fadump_crash(void) { return 0; }
>   static inline void crash_fadump(struct pt_regs *regs, const char *str) { }
> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
> index e1431800bfb9..0e08f1a80af2 100644
> --- a/arch/powerpc/kernel/fadump.c
> +++ b/arch/powerpc/kernel/fadump.c
> @@ -78,8 +78,10 @@ int __init early_init_dt_scan_fw_dump(unsigned long node,
>   	 * dump data waiting for us.
>   	 */
>   	fdm_active = of_get_flat_dt_prop(node, "ibm,kernel-dump", NULL);
> -	if (fdm_active)
> +	if (fdm_active) {
> +		pr_info("Firmware-assisted dump is active.\n");
>   		fw_dump.dump_active = 1;
> +	}
>
>   	/* Get the sizes required to store dump data for the firmware provided
>   	 * dump sections.
> @@ -339,8 +341,11 @@ int __init fadump_reserve_mem(void)
>   {
>   	unsigned long base, size, memory_boundary;
>
> -	if (!fw_dump.fadump_enabled)
> +	if (!fw_dump.fadump_enabled) {
> +		if (fw_dump.dump_active)
> +			pr_warn("Firmware-assisted dump was active but kernel booted with fadump disabled!\n");
>   		return 0;
> +	}
>
>   	if (!fw_dump.fadump_supported) {
>   		printk(KERN_INFO "Firmware-assisted dump is not supported on"
> @@ -380,7 +385,6 @@ int __init fadump_reserve_mem(void)
>   		memory_boundary = memblock_end_of_DRAM();
>
>   	if (fw_dump.dump_active) {
> -		printk(KERN_INFO "Firmware-assisted dump is active.\n");
>   		/*
>   		 * If last boot has crashed then reserve all the memory
>   		 * above boot_memory_size so that we don't touch it until
> @@ -467,6 +471,118 @@ static int __init early_fadump_reserve_mem(char *p)
>   }
>   early_param("fadump_reserve_mem", early_fadump_reserve_mem);
>
> +#define FADUMP_EXTRA_ARGS_PARAM		"fadump_extra_args="
> +#define FADUMP_EXTRA_ARGS_LEN		(strlen(FADUMP_EXTRA_ARGS_PARAM) - 1)
> +
> +struct param_info {
> +	char		*cmdline;
> +	char		*tmp_cmdline;
> +	int		 shortening;
> +};
> +
> +static void __init fadump_update_params(struct param_info *param_info,
> +					char *param, char *val)
> +{
> +	ptrdiff_t param_offset = param - param_info->tmp_cmdline;
> +	size_t vallen = val ? strlen(val) : 0;
> +	char *tgt = param_info->cmdline + param_offset
> +				- param_info->shortening;
> +	int shortening = 0;
> +	int quoted = 0;
> +
> +	if (!val)
> +		return;
> +
> +	/* leading '"' removed from parameter */
> +	if ((param > param_info->tmp_cmdline) && *(param - 1) == '"') {
> +		quoted = 1;
> +		shortening += 1;
> +		tgt--;
> +	}
> +
> +	/* next_arg removes one leading and one trailing '"' */
> +	if (*(tgt + FADUMP_EXTRA_ARGS_LEN + 1) == '"')
> +		shortening += 1;
> +	if (*(tgt + FADUMP_EXTRA_ARGS_LEN + 1 + vallen + shortening) == '"')
> +		shortening += 1;
> +
> +	/* remove one leading and one trailing quote if both are present */
> +	if ((val[0] == '"') && (val[vallen - 1] == '"')) {
> +		shortening += 2;
> +		vallen -= 2;
> +		val++;
> +	}
> +
> +	/* some characters were removed - move the trailing part of cmdline */
> +	if (shortening) {
> +		char *src;
> +
> +		strncpy(tgt, FADUMP_EXTRA_ARGS_PARAM, FADUMP_EXTRA_ARGS_LEN);
> +		tgt += FADUMP_EXTRA_ARGS_LEN;
> +		*tgt++ = ' ';
> +
> +		strncpy(tgt, val, vallen);
> +		tgt += vallen;
> +
> +		src = tgt + shortening;
> +		memmove(tgt, src, strlen(src) + 1);
> +	} else {
> +		/* remove the '=' */
> +		*(tgt + FADUMP_EXTRA_ARGS_LEN) = ' ';
> +	}
> +
> +	param_info->shortening += shortening;
> +}
> +
> +/*
> + * Reworks command line parameters and splits 'fadump_extra_args=' param
> + * to enforce the parameters passed through it
> + */
> +static int __init fadump_rework_cmdline_params(char *param, char *val,
> +					       const char *unused, void *arg)
> +{
> +	struct param_info *param_info = (struct param_info *)arg;
> +
> +	if (strncmp(param, FADUMP_EXTRA_ARGS_PARAM,
> +		     strlen(FADUMP_EXTRA_ARGS_PARAM) - 1))
> +		return 0;
> +
> +	fadump_update_params(param_info, param, val);
> +
> +	return 0;
> +}
> +
> +/*
> + * Replace every occurrence of 'fadump_extra_args="param1 param2 param3"'
> + * in cmdline with 'fadump_extra_args param1 param2 param3' by stripping
> + * off '=' and quotes, if any. This ensures that the additional parameters
> + * passed with 'fadump_extra_args=' are enforced.
> + */
> +void __init enforce_fadump_extra_args(char *cmdline)
> +{
> +	static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata;
> +	static char init_cmdline[COMMAND_LINE_SIZE] __initdata;
> +	struct param_info param_info;
> +
> +	if (strstr(cmdline, FADUMP_EXTRA_ARGS_PARAM) == NULL)
> +		return;
> +
> +	pr_info("Modifying command line to enforce the additional parameters passed through 'fadump_extra_args='");
> +
> +	param_info.cmdline = cmdline;
> +	param_info.tmp_cmdline = tmp_cmdline;
> +	param_info.shortening = 0;
> +
> +	strlcpy(init_cmdline, cmdline, COMMAND_LINE_SIZE);
> +
> +	strlcpy(tmp_cmdline, cmdline, COMMAND_LINE_SIZE);
> +	parse_args("fadump params", tmp_cmdline, NULL, 0, 0, 0,
> +			&param_info, &fadump_rework_cmdline_params);
> +
> +	pr_info("Original command line: %s\n", init_cmdline);
> +	pr_info("Modified command line: %s\n", cmdline);
> +}
> +
>   static int register_fw_dump(struct fadump_mem_struct *fdm)
>   {
>   	int rc, err;
> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index f83056297441..2e6f40217266 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -693,6 +693,13 @@ void __init early_init_devtree(void *params)
>   	of_scan_flat_dt(early_init_dt_scan_root, NULL);
>   	of_scan_flat_dt(early_init_dt_scan_memory_ppc, NULL);
>
> +	/*
> +	 * Look for 'fadump_extra_args=' parameter and enfore the additional
> +	 * parameters passed to it if fadump is active.
> +	 */
> +	if (is_fadump_active())
> +		enforce_fadump_extra_args(boot_command_line);
> +
>   	parse_early_param();
>
>   	/* make sure we've parsed cmdline for mem= before this */


[-- Attachment #2: Type: text/html, Size: 9162 bytes --]

Re: [PATCH v8 6/6] powerpc/fadump: use the new parse_args callback arguments

[Hari Bathini]

In case, someone wishes for a changelog:


With fadump_rework_cmdline_params() function, parse_args() callback 
function,

taking new arguments - current & next, use them to process 
'fadump_extra_args='

parmeter, in enforcing the parameters passed through it for fadump kernel.


On Tuesday 12 September 2017 09:31 PM, Michal Suchanek wrote:
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
>   arch/powerpc/kernel/fadump.c | 47 ++++++++++++--------------------------------
>   1 file changed, 13 insertions(+), 34 deletions(-)
>
> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
> index 8778e1cc0380..1678d99ea835 100644
> --- a/arch/powerpc/kernel/fadump.c
> +++ b/arch/powerpc/kernel/fadump.c
> @@ -481,33 +481,19 @@ struct param_info {
>   };
>
>   static void __init fadump_update_params(struct param_info *param_info,
> -					char *param, char *val)
> +					char *param, char *val,
> +					char *currant, char *next)
>   {
> -	ptrdiff_t param_offset = param - param_info->tmp_cmdline;
> +	ptrdiff_t param_offset = currant - param_info->tmp_cmdline;
>   	size_t vallen = val ? strlen(val) : 0;
>   	char *tgt = param_info->cmdline + param_offset
>   				- param_info->shortening;
> -	int shortening = 0;
> -	int quoted = 0;
> +	int shortening = ((next - 1) - (currant))
> +		- (FADUMP_EXTRA_ARGS_LEN + 1 + vallen);
>
>   	if (!val)
>   		return;
>
> -	/* leading '"' removed from parameter */
> -	if ((param > param_info->tmp_cmdline) && *(param - 1) == '"') {
> -		quoted = 1;
> -		shortening += 1;
> -		tgt--;
> -	}
> -
> -	/* next_arg removes one leading and one trailing '"' */
> -	if ((*(tgt + FADUMP_EXTRA_ARGS_LEN + 1 + vallen + shortening) == '"') &&
> -	    (quoted || (*(tgt + FADUMP_EXTRA_ARGS_LEN + 1) == '"'))) {
> -		shortening += 1;
> -		if (!quoted)
> -			shortening += 1;
> -	}
> -
>   	/* remove one leading and one trailing quote if both are present */
>   	if ((val[0] == '"') && (val[vallen - 1] == '"')) {
>   		shortening += 2;
> @@ -515,22 +501,15 @@ static void __init fadump_update_params(struct param_info *param_info,
>   		val++;
>   	}
>
> -	/* some characters were removed - move the trailing part of cmdline */
> -	if (shortening) {
> -		char *src;
> +	strncpy(tgt, FADUMP_EXTRA_ARGS_PARAM, FADUMP_EXTRA_ARGS_LEN);
> +	tgt += FADUMP_EXTRA_ARGS_LEN;
> +	*tgt++ = ' ';
> +	strncpy(tgt, val, vallen);
> +	tgt += vallen;
>
> -		strncpy(tgt, FADUMP_EXTRA_ARGS_PARAM, FADUMP_EXTRA_ARGS_LEN);
> -		tgt += FADUMP_EXTRA_ARGS_LEN;
> -		*tgt++ = ' ';
> -
> -		strncpy(tgt, val, vallen);
> -		tgt += vallen;
> -
> -		src = tgt + shortening;
> +	if (shortening) {
> +		char *src = tgt + shortening;
>   		memmove(tgt, src, strlen(src) + 1);
> -	} else {
> -		/* remove the '=' */
> -		*(tgt + FADUMP_EXTRA_ARGS_LEN) = ' ';
>   	}
>
>   	param_info->shortening += shortening;
> @@ -550,7 +529,7 @@ static int __init fadump_rework_cmdline_params(char *param, char *val,
>   		     strlen(FADUMP_EXTRA_ARGS_PARAM) - 1))
>   		return 0;
>
> -	fadump_update_params(param_info, param, val);
> +	fadump_update_params(param_info, param, val, currant, next);
>
>   	return 0;
>   }


Thanks
Hari