Add extended crashkernel command line syntax

kernel/kexec.c +61 -0

+
+/*
+ * parsing the "crashkernel" commandline
+ *
+ * this code is intended to be called from architecture specific code
+ */
+
+
+/*
+ * This function parses command lines in the format
+ *
+ *   crashkernel=<ramsize-range>:<size>[,...][@<base>]
+ *
+ * The function returns 0 on success and -EINVAL on failure.
+ */
+static int parse_crashkernel_mem(char 			*cmdline,
+				 unsigned long long 	*crash_size,
+				 unsigned long long 	*crash_base,
+				 unsigned long 		system_ram)
+{
+	char *cur = cmdline;
+
+	*crash_size = 0;
+	*crash_base = 0;
+
+	/* for each entry of the comma-separated list */
+	do {
+		unsigned long long start = 0, end = ULLONG_MAX;
+		unsigned long long size = -1;
+
+		/* get the start of the range */
+		start = memparse(cur, &cur);
+		if (*cur != '-') {
+			printk(KERN_WARNING "crashkernel: '-' expected\n");
+			return -EINVAL;
+		}
+		cur++;
+
+		/* if no ':' is here, than we read the end */
+		if (*cur != ':') {
+			end = memparse(cur, &cur);
+			if (end <= start) {
+				printk(KERN_WARNING "crashkernel: end <= start\n");
+				return -EINVAL;
+			}
+		}
+
+		if (*cur != ':') {
+			printk(KERN_WARNING "crashkernel: ':' expected\n");
+			return -EINVAL;
+		}
+		cur++;
+
+		size = memparse(cur, &cur);
+		if (size < 0) {
+			printk(KERN_WARNING "crashkernel: invalid size\n");
+			return -EINVAL;
+		}
+
+		/* match ? */
+		if (system_ram >= start  && system_ram <= end) {

"offset" seems to be optional in the new syntax. What happens if user does not specify offset. I think crash_base will be set to zero and system will try to reserve x amount of memory start at zero? That would fail?

I think we should add some intelligence for automatic selection of "offset" if user has not specified one. Automatically choose a chunk of free memory. This takes away the headache from user for selecting a right place. In fact one "offset" might not be valid for all the systems. I remember, somebody had reported that ACPI tables were mapped lower in the address space and reserving memory for kdump had failed.

Quoted 4 lines · Vivek Goyal

"offset" seems to be optional in the new syntax. What happens if user does not specify offset. I think crash_base will be set to zero and system will try to reserve x amount of memory start at zero? That would fail?

That's handled in the architecture specific code -- because it's different on each architecture and the architecture specific code does memory reservation. IA64 already can handle this case (on IA64, specifying 0 is the same than leaving out the base address, and that's why I wanted to keep that semantics). I think it doesn't also make sense on i386/x86_64 to choose 0 as real base address, because the value below 1 MB is special for booting ...

Quoted 6 lines · Bernhard Walle

Quoted 4 lines · Vivek Goyal

"offset" seems to be optional in the new syntax. What happens if user does not specify offset. I think crash_base will be set to zero and system will try to reserve x amount of memory start at zero? That would fail?

Quoted 8 lines

That's handled in the architecture specific code -- because it's different on each architecture and the architecture specific code does memory reservation. IA64 already can handle this case (on IA64, specifying 0 is the same than leaving out the base address, and that's why I wanted to keep that semantics). I think it doesn't also make sense on i386/x86_64 to choose 0 as real base address, because the value below 1 MB is special for booting ...

Ok. I see IA64 is handling this case. But in current patchset, i386 and x86_64 will try to reserve memory starting at zero? So we still got to handle this case in i386 and x86_64?

If there are multiple occurrences, then the last one is taken. The advantage is that if you have a bootloader like lilo or elilo which allows you to append a command line parameter but not to remove one (like in GRUB), then you can add another crashkernel value for testing at the boot command line and this one overwrites the command line in the configuration then.

Signed-off-by Bernhard Walle <[email protected]>

---
 include/linux/kexec.h |    2 
 kernel/kexec.c        |  139 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 141 insertions(+)

include/linux/kexec.h +7 -0

@@ -179,6 +179,8 @@ extern note_buf_t *crash_notes;
 extern u32 vmcoreinfo_note[VMCOREINFO_NOTE_SIZE/4];
 extern unsigned int vmcoreinfo_size;
 extern unsigned int vmcoreinfo_max_size;
+int parse_crashkernel(char *cmdline, unsigned long long system_ram,
+		unsigned long long *crash_size, unsigned long long *crash_base);
 
[...]
 
+int parse_crashkernel(char 		 *cmdline,
+		      unsigned long long system_ram,
+		      unsigned long long *crash_size,
+		      unsigned long long *crash_base)
+{

include/linux/kexec.h +12 -0

+	if (!ck_cmdline)
+		return -EINVAL;
+
+	ck_cmdline += 12; /* strlen("crashkernel=") */
+
+	/*
+	 * if the commandline contains a ':', then that's the extended
+	 * syntax -- if not, it must be the classic syntax
+	 */
+	first_colon = strchr(ck_cmdline, ':');
+	first_space = strchr(ck_cmdline, ' ');
+	if (first_colon && (!first_space || first_colon < first_space))
 
[...]

Quoted 5 lines · Bernhard Walle

If there are multiple occurrences, then the last one is taken. The advantage is that if you have a bootloader like lilo or elilo which allows you to append a command line parameter but not to remove one (like in GRUB), then you can add another crashkernel value for testing at the boot command line and this one overwrites the command line in the configuration then.

AFAIK it is the other way round: the lilo configuration-parameter "append" appends the kernel parameters in the configuration to the parameters given by the user on the kernel command line.

So the parameters set by the user during boot appear first on the command line.

lilo:
append=<string>

Appends the options specified to the parameter line passed to the kernel.

This patch removes the crashkernel parsing from arch/x86_64/kernel/machine_kexec.c and calls the generic function, introduced in the last patch, in setup_bootmem_allocator().

This is necessary because the amount of System RAM must be known in this function now because of the new syntax.

Signed-off-by Bernhard Walle <[email protected]>

---
 arch/x86_64/kernel/e820.c          |    3 ++-
 arch/x86_64/kernel/machine_kexec.c |   27 ---------------------------
 arch/x86_64/kernel/setup.c         |   35 ++++++++++++++++++++++++++++-------
 3 files changed, 30 insertions(+), 35 deletions(-)

arch/x86_64/kernel/e820.c +0 -0

@@ -226,7 +226,8 @@ void __init e820_reserve_resources(void)
                        request_resource(res, &code_resource);
                        request_resource(res, &data_resource);
 #ifdef CONFIG_KEXEC

arch/x86_64/kernel/machine_kexec.c +0 -27

@@ -231,33 +231,6 @@ NORET_TYPE void machine_kexec(struct kim
                        image->start);
 }
 
-/* crashkernel=size@addr specifies the location to reserve for
- * a crash kernel.  By reserving this memory we guarantee
- * that linux never set's it up as a DMA target.
- * Useful for holding code to do something appropriate
- * after a kernel panic.
- */
-static int __init setup_crashkernel(char *arg)
-{
-       unsigned long size, base;
-       char *p;
-       if (!arg)
-               return -EINVAL;
-       size = memparse(arg, &p);
-       if (arg == p)
-               return -EINVAL;
-       if (*p == '@') {
-               base = memparse(p+1, &p);
-               /* FIXME: Do I want a sanity check to validate the
-                * memory range?  Yes you do, but it's too early for
-                * e820 -AK */
-               crashk_res.start = base;
-               crashk_res.end   = base + size - 1;
-       }
-       return 0;
-}
-early_param("crashkernel", setup_crashkernel);
-
 void arch_crash_save_vmcoreinfo(void)
 {
 #ifdef CONFIG_ARCH_DISCONTIGMEM_ENABLE

Quoted 2 lines

+#ifdef CONFIG_KEXEC

...

CONFIG_KEXEC or CONFIG_CRASH_DUMP?

Quoted 4 lines · Yinghai Lu

Quoted 2 lines

+#ifdef CONFIG_KEXEC

...

Quoted 1 line

CONFIG_KEXEC or CONFIG_CRASH_DUMP?

Good question. The crashkernel parameter was CONFIG_KEXEC before, and I also wondered why, but I didn't change this because maybe there's some reason I don't know.

Vivek, do you know why this was CONFIG_KEXEC?

Quoted 4 lines · Yinghai Lu

Quoted 2 lines

+#ifdef CONFIG_KEXEC

...

Quoted 1 line

CONFIG_KEXEC or CONFIG_CRASH_DUMP?

Good question. The crashkernel parameter was CONFIG_KEXEC before, and I also wondered why, but I didn't change this because maybe there's some reason I don't know.

Vivek, do you know why this was CONFIG_KEXEC?

Quoted 5 lines · Bernhard Walle

Quoted 4 lines · Yinghai Lu

Quoted 2 lines

+#ifdef CONFIG_KEXEC

...

Quoted 7 lines

Quoted 1 line

CONFIG_KEXEC or CONFIG_CRASH_DUMP?

Good question. The crashkernel parameter was CONFIG_KEXEC before, and I also wondered why, but I didn't change this because maybe there's some reason I don't know.

Vivek, do you know why this was CONFIG_KEXEC?

As Eric mentioned, CONFIG_CRASH_DUMP has been used for all dump capturing infrastructure and rest of the kexec and kexec on panic functionality has been put under CONFIG_KEXEC.

Keeping memory reservation under CONFIG_KEXEC helps in a sense when somebody is not using a relocatable kernel and uses a custom kernel for dump capture. In that case he does not have to enable CONFIG_CRASH_DUMP in the first kernel.

Because the total amount of System RAM must be known when calling this function, efi_memmap_init() is modified to return its comulated

total_memory variable in a pointer value. IMO that's better than making the variable global.

Quoted 2 lines · Bernhard Walle

Because the total amount of System RAM must be known when calling this function, efi_memmap_init() is modified to return its comulated

accumulated?

Quoted 2 lines

total_memory variable in a pointer value. IMO that's better than making the variable global.

Why not make efi_memmap_init() return total_memory instead of void?

This patch adds a extended crashkernel syntax that makes the value of reserved system RAM dependent on the system RAM itself:

    crashkernel=<range1>:<size1>[,<range2>:<size2>,...][@offset]
    range=start-[end]

For example:

    crashkernel=512M-2G:64M,2G-:128M

The motivation comes from distributors that configure their crashkernel command line automatically with some configuration tool (YaST, you know ;)). Of course that tool knows the value of System RAM, but if the user removes RAM, then the system becomes unbootable or at least unusable and error handling is very difficult.

This series implements this change for i386, x86_64 and IA64. However, if the patch is accepted, I can also add all other architectures that support Kdump (when my grepping was correct, this is only PPC64 and SH in addition). The "simple" syntax is of course still supported.

How often does this scenario happen? Anyway, if you run into these issues and if it helps, I don't mind additional command line syntax for crashkernel=

Quoted 4 lines

This series implements this change for i386, x86_64 and IA64. However, if the patch is accepted, I can also add all other architectures that support Kdump (when my grepping was correct, this is only PPC64 and SH in addition). The "simple" syntax is of course still supported.

Please make the changes for ppc64 and sh too.