[PATCH v4 0/4] kexec-tools, x86: E820 memmap pass for kdump

[PATCH v4 0/4] kexec-tools, x86: E820 memmap pass for kdump

[WANG Chao]

Hi, All

When kaslr comes in and kdump is broken, it seems about the right time to use
E820 instead of memmap=exactmap to pass memmap for kdump for the default memmap
passing mechanism:
http://lists.infradead.org/pipermail/kexec/2014-February/011048.html

Unfortunately, saved_max_pfn still got its user out there (calgry pci, it looks
like the only one). So for backward compatibility, I'm introducing a new option
--pass-memmap-cmdline to force kexec-tools to pass memmap=exactmap, the old way.

Any comment is appreciate!

v3->v4:
Linn: check return value of malloc (use xmalloc)
me: fix dbgprintf_mem_range

v2->v3:
Linn:
 - do not free sd (setup_data) buffer
 - reuse code in setup_e820 and setup_e820_ext

v1->v2:

Vivek:
 - Use function instead of macro for dbgprint_mem_range
 - Do not pass reserved memory range for kdump. It could addressed later
   separately.

WANG Chao (4):
  cleanup: add dbgprint_mem_range function
  x86: Store memory ranges globally used for crash kernel to boot into
  x86: add --pass-memmap-cmdline option
  x86: Pass memory range via E820 for kdump

 kexec/arch/i386/crashdump-x86.c        | 157 ++++++++++++++++--------------
 kexec/arch/i386/crashdump-x86.h        |   6 +-
 kexec/arch/i386/include/arch/options.h |   2 +
 kexec/arch/i386/kexec-x86-common.c     |   6 +-
 kexec/arch/i386/kexec-x86.c            |   4 +
 kexec/arch/i386/kexec-x86.h            |   1 +
 kexec/arch/i386/x86-linux-setup.c      | 171 ++++++++++++++++++++++-----------
 kexec/arch/i386/x86-linux-setup.h      |   1 +
 kexec/arch/x86_64/kexec-x86_64.c       |   5 +
 kexec/kexec.c                          |  10 ++
 kexec/kexec.h                          |   1 +
 11 files changed, 231 insertions(+), 133 deletions(-)

-- 
1.8.5.3


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

[PATCH v4 1/4] cleanup: add dbgprint_mem_range function

[WANG Chao]

dbgprint_mem_range is used for printing the given memory range under
debugging mode.

Signed-off-by: WANG Chao <chaowang@redhat.com>
Tested-by: Linn Crosetto <linn@hp.com>
---
 kexec/arch/i386/kexec-x86-common.c |  6 +-----
 kexec/kexec.c                      | 10 ++++++++++
 kexec/kexec.h                      |  1 +
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/kexec/arch/i386/kexec-x86-common.c b/kexec/arch/i386/kexec-x86-common.c
index f55e2c2..e416177 100644
--- a/kexec/arch/i386/kexec-x86-common.c
+++ b/kexec/arch/i386/kexec-x86-common.c
@@ -374,11 +374,7 @@ int get_memory_ranges(struct memory_range **range, int *ranges,
 			mem_max = end;
 	}
 
-	dbgprintf("MEMORY RANGES\n");
-	for (i = 0; i < *ranges; i++) {
-		dbgprintf("%016Lx-%016Lx (%d)\n", (*range)[i].start,
-			  (*range)[i].end, (*range)[i].type);
-	}
+	dbgprint_mem_range("MEMORY RANGES", *range, *ranges);
 
 	return ret;
 }
diff --git a/kexec/kexec.c b/kexec/kexec.c
index 382f86a..133e622 100644
--- a/kexec/kexec.c
+++ b/kexec/kexec.c
@@ -53,6 +53,16 @@ unsigned long long mem_max = ULONG_MAX;
 static unsigned long kexec_flags = 0;
 int kexec_debug = 0;
 
+void dbgprint_mem_range(const char *prefix, struct memory_range *mr, int nr_mr)
+{
+	int i;
+	dbgprintf("%s\n", prefix);
+	for (i = 0; i < nr_mr; i++) {
+		dbgprintf("%016llx-%016llx (%d)\n", mr[i].start,
+			  mr[i].end, mr[i].type);
+	}
+}
+
 void die(const char *fmt, ...)
 {
 	va_list args;
diff --git a/kexec/kexec.h b/kexec/kexec.h
index 2bd6e96..d69bba2 100644
--- a/kexec/kexec.h
+++ b/kexec/kexec.h
@@ -232,6 +232,7 @@ extern int file_types;
 
 #define KEXEC_OPT_STR "h?vdfxluet:p"
 
+extern void dbgprint_mem_range(const char *prefix, struct memory_range *mr, int nr_mr);
 extern void die(const char *fmt, ...)
 	__attribute__ ((format (printf, 1, 2)));
 extern void *xmalloc(size_t size);
-- 
1.8.5.3


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

[PATCH v4 2/4] x86: Store memory ranges globally used for crash kernel to boot into

[WANG Chao]

Use these two variables to store the memory ranges and the number of
memory ranges for crash kernel to boot into:

struct memory_range crash_memory_range;
int crash_memory_range;

These two variables are not static now, so can be used in other file
later.

Signed-off-by: WANG Chao <chaowang@redhat.com>
Tested-by: Linn Crosetto <linn@hp.com>
---
 kexec/arch/i386/crashdump-x86.c | 134 ++++++++++++++++++++++------------------
 kexec/arch/i386/crashdump-x86.h |   5 +-
 2 files changed, 77 insertions(+), 62 deletions(-)

diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
index 979c2bd..c55a6b1 100644
--- a/kexec/arch/i386/crashdump-x86.c
+++ b/kexec/arch/i386/crashdump-x86.c
@@ -179,7 +179,8 @@ static int exclude_region(int *nr_ranges, uint64_t start, uint64_t end);
 
 /* Stores a sorted list of RAM memory ranges for which to create elf headers.
  * A separate program header is created for backup region */
-static struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
+struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
+int crash_memory_ranges;
 
 /* Memory region reserved for storing panic kernel and other data. */
 #define CRASH_RESERVED_MEM_NR	8
@@ -201,7 +202,7 @@ static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
 				   int kexec_flags, unsigned long lowmem_limit)
 {
 	const char *iomem = proc_iomem();
-	int memory_ranges = 0, gart = 0, i;
+	int gart = 0, i;
 	char line[MAX_LINE];
 	FILE *fp;
 	unsigned long long start, end;
@@ -218,7 +219,7 @@ static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
 		char *str;
 		int type, consumed, count;
 
-		if (memory_ranges >= CRASH_MAX_MEMORY_RANGES)
+		if (crash_memory_ranges >= CRASH_MAX_MEMORY_RANGES)
 			break;
 		count = sscanf(line, "%Lx-%Lx : %n",
 			&start, &end, &consumed);
@@ -250,17 +251,17 @@ static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
 			continue;
 		}
 
-		crash_memory_range[memory_ranges].start = start;
-		crash_memory_range[memory_ranges].end = end;
-		crash_memory_range[memory_ranges].type = type;
+		crash_memory_range[crash_memory_ranges].start = start;
+		crash_memory_range[crash_memory_ranges].end = end;
+		crash_memory_range[crash_memory_ranges].type = type;
 
-		segregate_lowmem_region(&memory_ranges, lowmem_limit);
+		segregate_lowmem_region(&crash_memory_ranges, lowmem_limit);
 
-		memory_ranges++;
+		crash_memory_ranges++;
 	}
 	fclose(fp);
 	if (kexec_flags & KEXEC_PRESERVE_CONTEXT) {
-		for (i = 0; i < memory_ranges; i++) {
+		for (i = 0; i < crash_memory_ranges; i++) {
 			if (crash_memory_range[i].end > 0x0009ffff) {
 				crash_reserved_mem[0].start = \
 					crash_memory_range[i].start;
@@ -278,17 +279,17 @@ static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
 	}
 
 	for (i = 0; i < crash_reserved_mem_nr; i++)
-		if (exclude_region(&memory_ranges, crash_reserved_mem[i].start,
+		if (exclude_region(&crash_memory_ranges, crash_reserved_mem[i].start,
 				crash_reserved_mem[i].end) < 0)
 			return -1;
 
 	if (gart) {
 		/* exclude GART region if the system has one */
-		if (exclude_region(&memory_ranges, gart_start, gart_end) < 0)
+		if (exclude_region(&crash_memory_ranges, gart_start, gart_end) < 0)
 			return -1;
 	}
 	*range = crash_memory_range;
-	*ranges = memory_ranges;
+	*ranges = crash_memory_ranges;
 
 	return 0;
 }
@@ -324,7 +325,7 @@ static int get_crash_memory_ranges_xen(struct memory_range **range,
 	}
 
 	*range = crash_memory_range;
-	*ranges = j;
+	*ranges = crash_memory_ranges = j;
 
 	qsort(*range, *ranges, sizeof(struct memory_range), compare_ranges);
 
@@ -417,8 +418,8 @@ static int exclude_region(int *nr_ranges, uint64_t start, uint64_t end)
 
 /* Adds a segment from list of memory regions which new kernel can use to
  * boot. Segment start and end should be aligned to 1K boundary. */
-static int add_memmap(struct memory_range *memmap_p, unsigned long long addr,
-								size_t size)
+static int add_memmap(struct memory_range *memmap_p, int *nr_range,
+					unsigned long long addr, size_t size)
 {
 	int i, j, nr_entries = 0, tidx = 0, align = 1024;
 	unsigned long long mstart, mend;
@@ -450,29 +451,23 @@ static int add_memmap(struct memory_range *memmap_p, unsigned long long addr,
 		else if (addr > mend)
 			tidx = i+1;
 	}
-		/* Insert the memory region. */
-		for (j = nr_entries-1; j >= tidx; j--)
-			memmap_p[j+1] = memmap_p[j];
-		memmap_p[tidx].start = addr;
-		memmap_p[tidx].end = addr + size - 1;
+	/* Insert the memory region. */
+	for (j = nr_entries-1; j >= tidx; j--)
+		memmap_p[j+1] = memmap_p[j];
+	memmap_p[tidx].start = addr;
+	memmap_p[tidx].end = addr + size - 1;
+	memmap_p[tidx].type = RANGE_RAM;
+	*nr_range = nr_entries + 1;
 
-	dbgprintf("Memmap after adding segment\n");
-	for (i = 0; i < CRASH_MAX_MEMMAP_NR;  i++) {
-		mstart = memmap_p[i].start;
-		mend = memmap_p[i].end;
-		if (mstart == 0 && mend == 0)
-			break;
-		dbgprintf("%016llx - %016llx\n",
-			mstart, mend);
-	}
+	dbgprint_mem_range("Memmap after adding segment", memmap_p, *nr_range);
 
 	return 0;
 }
 
 /* Removes a segment from list of memory regions which new kernel can use to
  * boot. Segment start and end should be aligned to 1K boundary. */
-static int delete_memmap(struct memory_range *memmap_p, unsigned long long addr,
-								size_t size)
+static int delete_memmap(struct memory_range *memmap_p, int *nr_range,
+					unsigned long long addr, size_t size)
 {
 	int i, j, nr_entries = 0, tidx = -1, operation = 0, align = 1024;
 	unsigned long long mstart, mend;
@@ -534,24 +529,17 @@ static int delete_memmap(struct memory_range *memmap_p, unsigned long long addr,
 		for (j = nr_entries-1; j > tidx; j--)
 			memmap_p[j+1] = memmap_p[j];
 		memmap_p[tidx+1] = temp_region;
+		*nr_range = nr_entries + 1;
 	}
 	if ((operation == -1) && tidx >=0) {
 		/* Delete the exact match memory region. */
 		for (j = i+1; j < CRASH_MAX_MEMMAP_NR; j++)
 			memmap_p[j-1] = memmap_p[j];
 		memmap_p[j-1].start = memmap_p[j-1].end = 0;
+		*nr_range = nr_entries - 1;
 	}
 
-	dbgprintf("Memmap after deleting segment\n");
-	for (i = 0; i < CRASH_MAX_MEMMAP_NR;  i++) {
-		mstart = memmap_p[i].start;
-		mend = memmap_p[i].end;
-		if (mstart == 0 && mend == 0) {
-			break;
-		}
-		dbgprintf("%016llx - %016llx\n",
-			mstart, mend);
-	}
+	dbgprint_mem_range("Memmap after deleting segment", memmap_p, *nr_range);
 
 	return 0;
 }
@@ -626,6 +614,9 @@ static int cmdline_add_memmap(char *cmdline, struct memory_range *memmap_p)
 			/* All regions traversed. */
 			break;
 
+		if (memmap_p[i].type != RANGE_RAM)
+			continue;
+
 		/* A region is not worth adding if region size < 100K. It eats
 		 * up precious command line length. */
 		if ((endk - startk) < min_sizek)
@@ -797,6 +788,25 @@ static void get_backup_area(struct kexec_info *info,
 	info->backup_src_size = BACKUP_SRC_END - BACKUP_SRC_START + 1;
 }
 
+static void exclude_ram(struct memory_range *mr, int *nr_mr)
+{
+	int ranges, i, j, m;
+
+	ranges = *nr_mr;
+	for (i = 0, j = 0; i < ranges; i++) {
+		if (mr[j].type == RANGE_RAM) {
+			dbgprintf("Remove RAM %016llx-%016llxx: (%d)\n", mr[j].start, mr[j].end, mr[j].type);
+			for (m = j; m < *nr_mr; m++)
+				mr[m] = mr[m+1];
+			(*nr_mr)--;
+		} else {
+			j++;
+		}
+	}
+
+	dbgprint_mem_range("After remove RAM", mr, *nr_mr);
+}
+
 /* Loads additional segments in case of a panic kernel is being loaded.
  * One segment for backup region, another segment for storing elf headers
  * for crash memory image.
@@ -807,7 +817,7 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
 	void *tmp;
 	unsigned long sz, bufsz, memsz, elfcorehdr;
 	int nr_ranges = 0, align = 1024, i;
-	struct memory_range *mem_range, *memmap_p;
+	struct memory_range *mem_range;
 	struct crash_elf_info elf_info;
 	unsigned kexec_arch;
 
@@ -850,10 +860,7 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
 
 	get_backup_area(info, mem_range, nr_ranges);
 
-	dbgprintf("CRASH MEMORY RANGES\n");
-
-	for(i = 0; i < nr_ranges; ++i)
-		dbgprintf("%016Lx-%016Lx\n", mem_range[i].start, mem_range[i].end);
+	dbgprint_mem_range("CRASH MEMORY RANGES", mem_range, nr_ranges);
 
 	/*
 	 * if the core type has not been set on command line, set it here
@@ -878,17 +885,6 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
 	if (get_kernel_vaddr_and_size(info, &elf_info))
 		return -1;
 
-	/* Memory regions which panic kernel can safely use to boot into */
-	sz = (sizeof(struct memory_range) * CRASH_MAX_MEMMAP_NR);
-	memmap_p = xmalloc(sz);
-	memset(memmap_p, 0, sz);
-	add_memmap(memmap_p, info->backup_src_start, info->backup_src_size);
-	for (i = 0; i < crash_reserved_mem_nr; i++) {
-		sz = crash_reserved_mem[i].end - crash_reserved_mem[i].start +1;
-		if (add_memmap(memmap_p, crash_reserved_mem[i].start, sz) < 0)
-			return ENOCRASHKERNEL;
-	}
-
 	/* Create a backup region segment to store backup data*/
 	if (!(info->kexec_flags & KEXEC_PRESERVE_CONTEXT)) {
 		sz = _ALIGN(info->backup_src_size, align);
@@ -898,8 +894,6 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
 						0, max_addr, -1);
 		dbgprintf("Created backup segment at 0x%lx\n",
 			  info->backup_start);
-		if (delete_memmap(memmap_p, info->backup_start, sz) < 0)
-			return EFAILED;
 	}
 
 	/* Create elf header segment and store crash image data. */
@@ -915,6 +909,23 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
 						ELF_CORE_HEADER_ALIGN) < 0)
 			return EFAILED;
 	}
+
+	/* Memory regions which panic kernel can safely use to boot into */
+	exclude_ram(crash_memory_range, &crash_memory_ranges);
+
+	add_memmap(crash_memory_range, &crash_memory_ranges, info->backup_src_start, info->backup_src_size);
+	for (i = 0; i < crash_reserved_mem_nr; i++) {
+		sz = crash_reserved_mem[i].end - crash_reserved_mem[i].start +1;
+		if (add_memmap(crash_memory_range, &crash_memory_ranges, crash_reserved_mem[i].start, sz) < 0)
+			return ENOCRASHKERNEL;
+	}
+
+	/* exclude backup region from crash dump memory range */
+	sz = _ALIGN(info->backup_src_size, align);
+	if (delete_memmap(crash_memory_range, &crash_memory_ranges, info->backup_start, sz) < 0) {
+		return EFAILED;
+	}
+
 	/* the size of the elf headers allocated is returned in 'bufsz' */
 
 	/* Hack: With some ld versions (GNU ld version 2.14.90.0.4 20030523),
@@ -934,9 +945,9 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
 	elfcorehdr = add_buffer(info, tmp, bufsz, memsz, align, min_base,
 							max_addr, -1);
 	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
-	if (delete_memmap(memmap_p, elfcorehdr, memsz) < 0)
+	if (delete_memmap(crash_memory_range, &crash_memory_ranges, elfcorehdr, memsz) < 0)
 		return -1;
-	cmdline_add_memmap(mod_cmdline, memmap_p);
+	cmdline_add_memmap(mod_cmdline, crash_memory_range);
 	if (!bzImage_support_efi_boot)
 		cmdline_add_efi(mod_cmdline);
 	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
@@ -951,6 +962,7 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
 		end = mem_range[i].end;
 		cmdline_add_memmap_acpi(mod_cmdline, start, end);
 	}
+
 	return 0;
 }
 
diff --git a/kexec/arch/i386/crashdump-x86.h b/kexec/arch/i386/crashdump-x86.h
index b61cf0a..633ee0e 100644
--- a/kexec/arch/i386/crashdump-x86.h
+++ b/kexec/arch/i386/crashdump-x86.h
@@ -20,7 +20,7 @@ int load_crashdump_segments(struct kexec_info *info, char *mod_cmdline,
 /* Kernel text size */
 #define X86_64_KERNEL_TEXT_SIZE  (512UL*1024*1024)
 
-#define CRASH_MAX_MEMMAP_NR	(KEXEC_MAX_SEGMENTS + 1)
+#define CRASH_MAX_MEMMAP_NR	CRASH_MAX_MEMORY_RANGES
 #define CRASH_MAX_MEMORY_RANGES	(MAX_MEMORY_RANGES + 2)
 
 /* Backup Region, First 640K of System RAM. */
@@ -28,4 +28,7 @@ int load_crashdump_segments(struct kexec_info *info, char *mod_cmdline,
 #define BACKUP_SRC_END		0x0009ffff
 #define BACKUP_SRC_SIZE	(BACKUP_SRC_END - BACKUP_SRC_START + 1)
 
+extern struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
+extern int crash_memory_ranges;
+
 #endif /* CRASHDUMP_X86_H */
-- 
1.8.5.3


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

[PATCH v4 3/4] x86: add --pass-memmap-cmdline option

[WANG Chao]

--pass-memmap-cmdline is used for pass memmap=exactmap cmdline for 2nd
kernel. Later we will use this option to disable passing E820 memmap
method but use the old exactmap method.

Signed-off-by: WANG Chao <chaowang@redhat.com>
Tested-by: Linn Crosetto <linn@hp.com>
---
 kexec/arch/i386/include/arch/options.h | 2 ++
 kexec/arch/i386/kexec-x86.c            | 4 ++++
 kexec/arch/i386/kexec-x86.h            | 1 +
 kexec/arch/i386/x86-linux-setup.h      | 1 +
 kexec/arch/x86_64/kexec-x86_64.c       | 5 +++++
 5 files changed, 13 insertions(+)

diff --git a/kexec/arch/i386/include/arch/options.h b/kexec/arch/i386/include/arch/options.h
index aaac731..e5300b5 100644
--- a/kexec/arch/i386/include/arch/options.h
+++ b/kexec/arch/i386/include/arch/options.h
@@ -30,6 +30,7 @@
 #define OPT_VGA 		(OPT_ARCH_MAX+8)
 #define OPT_REAL_MODE		(OPT_ARCH_MAX+9)
 #define OPT_ENTRY_32BIT		(OPT_ARCH_MAX+10)
+#define OPT_PASS_MEMMAP_CMDLINE	(OPT_ARCH_MAX+11)
 
 /* Options relevant to the architecture (excluding loader-specific ones): */
 #define KEXEC_ARCH_OPTIONS \
@@ -41,6 +42,7 @@
 	{ "console-serial", 0, 0, OPT_CONSOLE_SERIAL }, \
 	{ "elf32-core-headers", 0, 0, OPT_ELF32_CORE }, \
 	{ "elf64-core-headers", 0, 0, OPT_ELF64_CORE }, \
+	{ "pass-memmap-cmdline", 0, 0, OPT_PASS_MEMMAP_CMDLINE }, \
 
 #define KEXEC_ARCH_OPT_STR KEXEC_OPT_STR ""
 
diff --git a/kexec/arch/i386/kexec-x86.c b/kexec/arch/i386/kexec-x86.c
index 014ecd5..0b58dff 100644
--- a/kexec/arch/i386/kexec-x86.c
+++ b/kexec/arch/i386/kexec-x86.c
@@ -54,6 +54,7 @@ void arch_usage(void)
 		"     --console-serial          Enable the serial console\n"
 		"     --elf32-core-headers      Prepare core headers in ELF32 format\n"
 		"     --elf64-core-headers      Prepare core headers in ELF64 format\n"
+		"     --pass--memmap-cmdline    Pass memory map via command line in kexec on panic case\n"
 		);
 }
 
@@ -64,6 +65,7 @@ struct arch_options_t arch_options = {
 	.console_vga = 0,
 	.console_serial = 0,
 	.core_header_type = CORE_TYPE_UNDEF,
+	.pass_memmap_cmdline = 0,
 };
 
 int arch_process_options(int argc, char **argv)
@@ -133,6 +135,8 @@ int arch_process_options(int argc, char **argv)
 		case OPT_ELF64_CORE:
 			arch_options.core_header_type = CORE_TYPE_ELF64;
 			break;
+		case OPT_PASS_MEMMAP_CMDLINE:
+			arch_options.pass_memmap_cmdline = 1;
 		}
 	}
 	/* Reset getopt for the next pass; called in other source modules */
diff --git a/kexec/arch/i386/kexec-x86.h b/kexec/arch/i386/kexec-x86.h
index 5aa2a46..e8c9188 100644
--- a/kexec/arch/i386/kexec-x86.h
+++ b/kexec/arch/i386/kexec-x86.h
@@ -50,6 +50,7 @@ struct arch_options_t {
 	uint8_t  	console_vga;
 	uint8_t  	console_serial;
 	enum coretype	core_header_type;
+	uint8_t  	pass_memmap_cmdline;
 };
 
 int multiboot_x86_probe(const char *buf, off_t len);
diff --git a/kexec/arch/i386/x86-linux-setup.h b/kexec/arch/i386/x86-linux-setup.h
index 6fb84b4..b0ccd26 100644
--- a/kexec/arch/i386/x86-linux-setup.h
+++ b/kexec/arch/i386/x86-linux-setup.h
@@ -30,5 +30,6 @@ void setup_linux_system_parameters(struct kexec_info *info,
 /* command line parameter may be appended by purgatory */
 #define PURGATORY_CMDLINE_SIZE 64
 extern int bzImage_support_efi_boot;
+extern int pass_memmap_cmdline;
 
 #endif /* X86_LINUX_SETUP_H */
diff --git a/kexec/arch/x86_64/kexec-x86_64.c b/kexec/arch/x86_64/kexec-x86_64.c
index 5c23e01..f70851d 100644
--- a/kexec/arch/x86_64/kexec-x86_64.c
+++ b/kexec/arch/x86_64/kexec-x86_64.c
@@ -53,6 +53,7 @@ void arch_usage(void)
 		"     --serial-baud=<baud_rate> Specify the serial port baud rate\n"
 		"     --console-vga             Enable the vga console\n"
 		"     --console-serial          Enable the serial console\n"
+		"     --pass-memmap-cmdline     Pass memory map via command line in kexec on panic case\n"
 		);
 }
 
@@ -63,6 +64,7 @@ struct arch_options_t arch_options = {
 	.console_vga = 0,
 	.console_serial = 0,
 	.core_header_type = CORE_TYPE_ELF64,
+	.pass_memmap_cmdline = 0,
 };
 
 int arch_process_options(int argc, char **argv)
@@ -126,6 +128,9 @@ int arch_process_options(int argc, char **argv)
 			}
 			arch_options.serial_baud = value;
 			break;
+		case OPT_PASS_MEMMAP_CMDLINE:
+			arch_options.pass_memmap_cmdline = 1;
+			break;
 		}
 	}
 	/* Reset getopt for the next pass; called in other source modules */
-- 
1.8.5.3


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

[PATCH v4 4/4] x86: Pass memory range via E820 for kdump

[WANG Chao]

command line size is restricted by kernel, sometimes memmap=exactmap has
too many memory ranges to pass to cmdline. A better approach, to pass the
memory ranges for crash kernel to boot into, is filling the memory
ranges into E820.

boot_params only got 128 slots for E820 map to fit in, when the number of
memory map exceeds 128, use setup_data to pass the rest as extended E820
memory map.

kexec boot could also benefit from setup_data in case E820 memory map
exceeds 128.

Now this new approach becomes default instead of memmap=exactmap.
saved_max_pfn users can specify --pass-memmap-cmdline to use the
exactmap approach.

Signed-off-by: WANG Chao <chaowang@redhat.com>
Tested-by: Linn Crosetto <linn@hp.com>
Reviewed-by: Linn Crosetto <linn@hp.com>
---
 kexec/arch/i386/crashdump-x86.c   |  25 +++---
 kexec/arch/i386/crashdump-x86.h   |   1 +
 kexec/arch/i386/x86-linux-setup.c | 171 +++++++++++++++++++++++++-------------
 3 files changed, 130 insertions(+), 67 deletions(-)

diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
index c55a6b1..cb19e7d 100644
--- a/kexec/arch/i386/crashdump-x86.c
+++ b/kexec/arch/i386/crashdump-x86.c
@@ -182,6 +182,8 @@ static int exclude_region(int *nr_ranges, uint64_t start, uint64_t end);
 struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
 int crash_memory_ranges;
 
+int pass_memmap_cmdline;
+
 /* Memory region reserved for storing panic kernel and other data. */
 #define CRASH_RESERVED_MEM_NR	8
 static struct memory_range crash_reserved_mem[CRASH_RESERVED_MEM_NR];
@@ -947,20 +949,23 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
 	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
 	if (delete_memmap(crash_memory_range, &crash_memory_ranges, elfcorehdr, memsz) < 0)
 		return -1;
-	cmdline_add_memmap(mod_cmdline, crash_memory_range);
 	if (!bzImage_support_efi_boot)
 		cmdline_add_efi(mod_cmdline);
 	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
 
-	/* Inform second kernel about the presence of ACPI tables. */
-	for (i = 0; i < CRASH_MAX_MEMORY_RANGES; i++) {
-		unsigned long start, end;
-		if ( !( mem_range[i].type == RANGE_ACPI
-			|| mem_range[i].type == RANGE_ACPI_NVS) )
-			continue;
-		start = mem_range[i].start;
-		end = mem_range[i].end;
-		cmdline_add_memmap_acpi(mod_cmdline, start, end);
+	pass_memmap_cmdline = arch_options.pass_memmap_cmdline;
+	if (pass_memmap_cmdline) {
+		cmdline_add_memmap(mod_cmdline, crash_memory_range);
+		/* Inform second kernel about the presence of ACPI tables. */
+		for (i = 0; i < CRASH_MAX_MEMORY_RANGES; i++) {
+			unsigned long start, end;
+			if ( !( mem_range[i].type == RANGE_ACPI
+						|| mem_range[i].type == RANGE_ACPI_NVS) )
+				continue;
+			start = mem_range[i].start;
+			end = mem_range[i].end;
+			cmdline_add_memmap_acpi(mod_cmdline, start, end);
+		}
 	}
 
 	return 0;
diff --git a/kexec/arch/i386/crashdump-x86.h b/kexec/arch/i386/crashdump-x86.h
index 633ee0e..e68b626 100644
--- a/kexec/arch/i386/crashdump-x86.h
+++ b/kexec/arch/i386/crashdump-x86.h
@@ -30,5 +30,6 @@ int load_crashdump_segments(struct kexec_info *info, char *mod_cmdline,
 
 extern struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
 extern int crash_memory_ranges;
+extern int pass_memmap_cmdline;
 
 #endif /* CRASHDUMP_X86_H */
diff --git a/kexec/arch/i386/x86-linux-setup.c b/kexec/arch/i386/x86-linux-setup.c
index 5884f4d..e8865e1 100644
--- a/kexec/arch/i386/x86-linux-setup.c
+++ b/kexec/arch/i386/x86-linux-setup.c
@@ -35,8 +35,7 @@
 #include "kexec-x86.h"
 #include "x86-linux-setup.h"
 #include "../../kexec/kexec-syscall.h"
-
-#define SETUP_EFI	4
+#include "crashdump-x86.h"
 
 void init_linux_parameters(struct x86_linux_param_header *real_mode)
 {
@@ -502,6 +501,11 @@ struct efi_setup_data {
 struct setup_data {
 	uint64_t next;
 	uint32_t type;
+#define SETUP_NONE	0
+#define SETUP_E820_EXT	1
+#define SETUP_DTB	2
+#define SETUP_PCI	3
+#define SETUP_EFI	4
 	uint32_t len;
 	uint8_t data[0];
 } __attribute__((packed));
@@ -602,6 +606,17 @@ struct efi_info {
 	uint32_t efi_memmap_hi;
 };
 
+static void add_setup_data(struct kexec_info *info,
+			   struct x86_linux_param_header *real_mode,
+			   struct setup_data *sd)
+{
+	int sdsize = sizeof(struct setup_data) + sd->len;
+
+	sd->next = real_mode->setup_data;
+	real_mode->setup_data = add_buffer(info, sd, sdsize, sdsize, getpagesize(),
+			    0x100000, ULONG_MAX, INT_MAX);
+}
+
 /*
  * setup_efi_data will collect below data and pass them to 2nd kernel.
  * 1) SMBIOS, fw_vendor, runtime, config_table, they are passed via x86
@@ -611,11 +626,11 @@ struct efi_info {
 static int setup_efi_data(struct kexec_info *info,
 			  struct x86_linux_param_header *real_mode)
 {
-	int64_t setup_data_paddr, memmap_paddr;
+	int64_t memmap_paddr;
 	struct setup_data *sd;
 	struct efi_setup_data *esd;
 	struct efi_mem_descriptor *maps;
-	int nr_maps, size, sdsize, ret = 0;
+	int nr_maps, size, ret = 0;
 	struct efi_info *ei = (struct efi_info *)real_mode->efi_info;
 
 	ret = access("/sys/firmware/efi/systab", F_OK);
@@ -648,10 +663,8 @@ static int setup_efi_data(struct kexec_info *info,
 	sd->len = sizeof(*esd);
 	memcpy(sd->data, esd, sizeof(*esd));
 	free(esd);
-	sdsize = sd->len + sizeof(struct setup_data);
-	setup_data_paddr = add_buffer(info, sd, sdsize, sdsize, getpagesize(),
-					0x100000, ULONG_MAX, INT_MAX);
-	real_mode->setup_data = setup_data_paddr;
+
+	add_setup_data(info, real_mode, sd);
 
 	size = nr_maps * sizeof(struct efi_mem_descriptor);
 	memmap_paddr = add_buffer(info, maps, size, size, getpagesize(),
@@ -669,6 +682,98 @@ out:
 	return ret;
 }
 
+static void add_e820_map_from_mr(struct x86_linux_param_header *real_mode,
+			struct e820entry *e820, struct memory_range *range, int nr_range)
+{
+	int i;
+
+	for (i = 0; i < nr_range; i++) {
+		e820[i].addr = range[i].start;
+		e820[i].size = range[i].end - range[i].start;
+		switch (range[i].type) {
+			case RANGE_RAM:
+				e820[i].type = E820_RAM;
+				break;
+			case RANGE_ACPI:
+				e820[i].type = E820_ACPI;
+				break;
+			case RANGE_ACPI_NVS:
+				e820[i].type = E820_NVS;
+				break;
+			default:
+			case RANGE_RESERVED:
+				e820[i].type = E820_RESERVED;
+				break;
+		}
+		dbgprintf("%016lx-%016lx (%d)\n",
+				e820[i].addr,
+				e820[i].addr + e820[i].size - 1,
+				e820[i].type);
+
+		if (range[i].type != RANGE_RAM)
+			continue;
+		if ((range[i].start <= 0x100000) && range[i].end > 0x100000) {
+			unsigned long long mem_k = (range[i].end >> 10) - (0x100000 >> 10);
+			real_mode->ext_mem_k = mem_k;
+			real_mode->alt_mem_k = mem_k;
+			if (mem_k > 0xfc00) {
+				real_mode->ext_mem_k = 0xfc00; /* 64M */
+			}
+			if (mem_k > 0xffffffff) {
+				real_mode->alt_mem_k = 0xffffffff;
+			}
+		}
+	}
+}
+
+static void setup_e820_ext(struct kexec_info *info, struct x86_linux_param_header *real_mode,
+			   struct memory_range *range, int nr_range)
+{
+	struct setup_data *sd;
+	struct e820entry *e820;
+	int nr_range_ext;
+
+	nr_range_ext = nr_range - E820MAX;
+	sd = xmalloc(sizeof(struct setup_data) + nr_range_ext * sizeof(struct e820entry));
+	sd->next = 0;
+	sd->len = nr_range_ext * sizeof(struct e820entry);
+	sd->type = SETUP_E820_EXT;
+
+	e820 = (struct e820entry *) sd->data;
+	dbgprintf("Extended E820 via setup_data:\n");
+	add_e820_map_from_mr(real_mode, e820, range + E820MAX, nr_range_ext);
+	add_setup_data(info, real_mode, sd);
+}
+
+static void setup_e820(struct kexec_info *info, struct x86_linux_param_header *real_mode)
+{
+	struct memory_range *range;
+	int nr_range, nr_range_saved;
+
+
+	if (info->kexec_flags & KEXEC_ON_CRASH && !pass_memmap_cmdline) {
+		range = crash_memory_range;
+		nr_range = crash_memory_ranges;
+	} else {
+		range = info->memory_range;
+		nr_range = info->memory_ranges;
+	}
+
+	nr_range_saved = nr_range;
+	if (nr_range > E820MAX) {
+		nr_range = E820MAX;
+	}
+
+	real_mode->e820_map_nr = nr_range;
+	dbgprintf("E820 memmap:\n");
+	add_e820_map_from_mr(real_mode, real_mode->e820_map, range, nr_range);
+
+	if (nr_range_saved > E820MAX) {
+		dbgprintf("extra E820 memmap are passed via setup_data\n");
+		setup_e820_ext(info, real_mode, range, nr_range_saved);
+	}
+}
+
 static int
 get_efi_mem_desc_version(struct x86_linux_param_header *real_mode)
 {
@@ -702,10 +807,6 @@ static void setup_efi_info(struct kexec_info *info,
 void setup_linux_system_parameters(struct kexec_info *info,
 				   struct x86_linux_param_header *real_mode)
 {
-	/* Fill in information the BIOS would usually provide */
-	struct memory_range *range;
-	int i, ranges;
-
 	/* get subarch from running kernel */
 	setup_subarch(real_mode);
 	if (bzImage_support_efi_boot)
@@ -746,51 +847,7 @@ void setup_linux_system_parameters(struct kexec_info *info,
 	/* another safe default */
 	real_mode->aux_device_info = 0;
 
-	range = info->memory_range;
-	ranges = info->memory_ranges;
-	if (ranges > E820MAX) {
-		if (!(info->kexec_flags & KEXEC_ON_CRASH))
-			/*
-			 * this e820 not used for capture kernel, see
-			 * do_bzImage_load()
-			 */
-			fprintf(stderr,
-				"Too many memory ranges, truncating...\n");
-		ranges = E820MAX;
-	}
-	real_mode->e820_map_nr = ranges;
-	for(i = 0; i < ranges; i++) {
-		real_mode->e820_map[i].addr = range[i].start;
-		real_mode->e820_map[i].size = range[i].end - range[i].start;
-		switch (range[i].type) {
-		case RANGE_RAM:
-			real_mode->e820_map[i].type = E820_RAM; 
-			break;
-		case RANGE_ACPI:
-			real_mode->e820_map[i].type = E820_ACPI; 
-			break;
-		case RANGE_ACPI_NVS:
-			real_mode->e820_map[i].type = E820_NVS;
-			break;
-		default:
-		case RANGE_RESERVED:
-			real_mode->e820_map[i].type = E820_RESERVED; 
-			break;
-		}
-		if (range[i].type != RANGE_RAM)
-			continue;
-		if ((range[i].start <= 0x100000) && range[i].end > 0x100000) {
-			unsigned long long mem_k = (range[i].end >> 10) - (0x100000 >> 10);
-			real_mode->ext_mem_k = mem_k;
-			real_mode->alt_mem_k = mem_k;
-			if (mem_k > 0xfc00) {
-				real_mode->ext_mem_k = 0xfc00; /* 64M */
-			}
-			if (mem_k > 0xffffffff) {
-				real_mode->alt_mem_k = 0xffffffff;
-			}
-		}
-	}
+	setup_e820(info, real_mode);
 
 	/* fill the EDD information */
 	setup_edd_info(real_mode);
-- 
1.8.5.3


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

Re: [PATCH v4 0/4] kexec-tools, x86: E820 memmap pass for kdump

[Linn Crosetto]

On Wed, Mar 19, 2014 at 04:03:57PM +0800, WANG Chao wrote:
> Hi, All
> 
> When kaslr comes in and kdump is broken, it seems about the right time to use
> E820 instead of memmap=exactmap to pass memmap for kdump for the default memmap
> passing mechanism:
> http://lists.infradead.org/pipermail/kexec/2014-February/011048.html
> 
> Unfortunately, saved_max_pfn still got its user out there (calgry pci, it looks
> like the only one). So for backward compatibility, I'm introducing a new option
> --pass-memmap-cmdline to force kexec-tools to pass memmap=exactmap, the old way.
> 
> Any comment is appreciate!
> 
> v3->v4:
> Linn: check return value of malloc (use xmalloc)
> me: fix dbgprintf_mem_range
> 
> v2->v3:
> Linn:
>  - do not free sd (setup_data) buffer
>  - reuse code in setup_e820 and setup_e820_ext
> 
> v1->v2:
> 
> Vivek:
>  - Use function instead of macro for dbgprint_mem_range
>  - Do not pass reserved memory range for kdump. It could addressed later
>    separately.
> 
> WANG Chao (4):
>   cleanup: add dbgprint_mem_range function
>   x86: Store memory ranges globally used for crash kernel to boot into
>   x86: add --pass-memmap-cmdline option
>   x86: Pass memory range via E820 for kdump
> 
>  kexec/arch/i386/crashdump-x86.c        | 157 ++++++++++++++++--------------
>  kexec/arch/i386/crashdump-x86.h        |   6 +-
>  kexec/arch/i386/include/arch/options.h |   2 +
>  kexec/arch/i386/kexec-x86-common.c     |   6 +-
>  kexec/arch/i386/kexec-x86.c            |   4 +
>  kexec/arch/i386/kexec-x86.h            |   1 +
>  kexec/arch/i386/x86-linux-setup.c      | 171 ++++++++++++++++++++++-----------
>  kexec/arch/i386/x86-linux-setup.h      |   1 +
>  kexec/arch/x86_64/kexec-x86_64.c       |   5 +
>  kexec/kexec.c                          |  10 ++
>  kexec/kexec.h                          |   1 +
>  11 files changed, 231 insertions(+), 133 deletions(-)

Thanks for the patches. Tested (adding a fix provided by Dave Young for
http://lists.infradead.org/pipermail/kexec/2014-March/011262.html) and
successful with both kexec and kdump, both with and without
--pass-memmap-cmdline.


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

Re: [PATCH v4 1/4] cleanup: add dbgprint_mem_range function

[Simon Horman]

On Wed, Mar 19, 2014 at 04:03:58PM +0800, WANG Chao wrote:
> dbgprint_mem_range is used for printing the given memory range under
> debugging mode.
> 
> Signed-off-by: WANG Chao <chaowang@redhat.com>
> Tested-by: Linn Crosetto <linn@hp.com>

Thanks, I have applied this patch.

> ---
>  kexec/arch/i386/kexec-x86-common.c |  6 +-----
>  kexec/kexec.c                      | 10 ++++++++++
>  kexec/kexec.h                      |  1 +
>  3 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/kexec/arch/i386/kexec-x86-common.c b/kexec/arch/i386/kexec-x86-common.c
> index f55e2c2..e416177 100644
> --- a/kexec/arch/i386/kexec-x86-common.c
> +++ b/kexec/arch/i386/kexec-x86-common.c
> @@ -374,11 +374,7 @@ int get_memory_ranges(struct memory_range **range, int *ranges,
>  			mem_max = end;
>  	}
>  
> -	dbgprintf("MEMORY RANGES\n");
> -	for (i = 0; i < *ranges; i++) {
> -		dbgprintf("%016Lx-%016Lx (%d)\n", (*range)[i].start,
> -			  (*range)[i].end, (*range)[i].type);
> -	}
> +	dbgprint_mem_range("MEMORY RANGES", *range, *ranges);
>  
>  	return ret;
>  }
> diff --git a/kexec/kexec.c b/kexec/kexec.c
> index 382f86a..133e622 100644
> --- a/kexec/kexec.c
> +++ b/kexec/kexec.c
> @@ -53,6 +53,16 @@ unsigned long long mem_max = ULONG_MAX;
>  static unsigned long kexec_flags = 0;
>  int kexec_debug = 0;
>  
> +void dbgprint_mem_range(const char *prefix, struct memory_range *mr, int nr_mr)
> +{
> +	int i;
> +	dbgprintf("%s\n", prefix);
> +	for (i = 0; i < nr_mr; i++) {
> +		dbgprintf("%016llx-%016llx (%d)\n", mr[i].start,
> +			  mr[i].end, mr[i].type);
> +	}
> +}
> +
>  void die(const char *fmt, ...)
>  {
>  	va_list args;
> diff --git a/kexec/kexec.h b/kexec/kexec.h
> index 2bd6e96..d69bba2 100644
> --- a/kexec/kexec.h
> +++ b/kexec/kexec.h
> @@ -232,6 +232,7 @@ extern int file_types;
>  
>  #define KEXEC_OPT_STR "h?vdfxluet:p"
>  
> +extern void dbgprint_mem_range(const char *prefix, struct memory_range *mr, int nr_mr);
>  extern void die(const char *fmt, ...)
>  	__attribute__ ((format (printf, 1, 2)));
>  extern void *xmalloc(size_t size);
> -- 
> 1.8.5.3
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
> 

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

Re: [PATCH v4 0/4] kexec-tools, x86: E820 memmap pass for kdump

[Simon Horman]

On Wed, Mar 19, 2014 at 11:57:57AM -0600, Linn Crosetto wrote:
> On Wed, Mar 19, 2014 at 04:03:57PM +0800, WANG Chao wrote:
> > Hi, All
> > 
> > When kaslr comes in and kdump is broken, it seems about the right time to use
> > E820 instead of memmap=exactmap to pass memmap for kdump for the default memmap
> > passing mechanism:
> > http://lists.infradead.org/pipermail/kexec/2014-February/011048.html
> > 
> > Unfortunately, saved_max_pfn still got its user out there (calgry pci, it looks
> > like the only one). So for backward compatibility, I'm introducing a new option
> > --pass-memmap-cmdline to force kexec-tools to pass memmap=exactmap, the old way.
> > 
> > Any comment is appreciate!
> > 
> > v3->v4:
> > Linn: check return value of malloc (use xmalloc)
> > me: fix dbgprintf_mem_range
> > 
> > v2->v3:
> > Linn:
> >  - do not free sd (setup_data) buffer
> >  - reuse code in setup_e820 and setup_e820_ext
> > 
> > v1->v2:
> > 
> > Vivek:
> >  - Use function instead of macro for dbgprint_mem_range
> >  - Do not pass reserved memory range for kdump. It could addressed later
> >    separately.
> > 
> > WANG Chao (4):
> >   cleanup: add dbgprint_mem_range function
> >   x86: Store memory ranges globally used for crash kernel to boot into
> >   x86: add --pass-memmap-cmdline option
> >   x86: Pass memory range via E820 for kdump
> > 
> >  kexec/arch/i386/crashdump-x86.c        | 157 ++++++++++++++++--------------
> >  kexec/arch/i386/crashdump-x86.h        |   6 +-
> >  kexec/arch/i386/include/arch/options.h |   2 +
> >  kexec/arch/i386/kexec-x86-common.c     |   6 +-
> >  kexec/arch/i386/kexec-x86.c            |   4 +
> >  kexec/arch/i386/kexec-x86.h            |   1 +
> >  kexec/arch/i386/x86-linux-setup.c      | 171 ++++++++++++++++++++++-----------
> >  kexec/arch/i386/x86-linux-setup.h      |   1 +
> >  kexec/arch/x86_64/kexec-x86_64.c       |   5 +
> >  kexec/kexec.c                          |  10 ++
> >  kexec/kexec.h                          |   1 +
> >  11 files changed, 231 insertions(+), 133 deletions(-)
> 
> Thanks for the patches. Tested (adding a fix provided by Dave Young for
> http://lists.infradead.org/pipermail/kexec/2014-March/011262.html) and
> successful with both kexec and kdump, both with and without
> --pass-memmap-cmdline.

Is the patch a pre-requisite for this series?
If so, has it been posted formally?
If so, could you point it out to me?

Thanks

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

Re: [PATCH v4 0/4] kexec-tools, x86: E820 memmap pass for kdump

[WANG Chao]

On 03/20/14 at 12:50pm, Simon Horman wrote:
> On Wed, Mar 19, 2014 at 11:57:57AM -0600, Linn Crosetto wrote:
> > On Wed, Mar 19, 2014 at 04:03:57PM +0800, WANG Chao wrote:
> > > Hi, All
> > > 
> > > When kaslr comes in and kdump is broken, it seems about the right time to use
> > > E820 instead of memmap=exactmap to pass memmap for kdump for the default memmap
> > > passing mechanism:
> > > http://lists.infradead.org/pipermail/kexec/2014-February/011048.html
> > > 
> > > Unfortunately, saved_max_pfn still got its user out there (calgry pci, it looks
> > > like the only one). So for backward compatibility, I'm introducing a new option
> > > --pass-memmap-cmdline to force kexec-tools to pass memmap=exactmap, the old way.
> > > 
> > > Any comment is appreciate!
> > > 
> > > v3->v4:
> > > Linn: check return value of malloc (use xmalloc)
> > > me: fix dbgprintf_mem_range
> > > 
> > > v2->v3:
> > > Linn:
> > >  - do not free sd (setup_data) buffer
> > >  - reuse code in setup_e820 and setup_e820_ext
> > > 
> > > v1->v2:
> > > 
> > > Vivek:
> > >  - Use function instead of macro for dbgprint_mem_range
> > >  - Do not pass reserved memory range for kdump. It could addressed later
> > >    separately.
> > > 
> > > WANG Chao (4):
> > >   cleanup: add dbgprint_mem_range function
> > >   x86: Store memory ranges globally used for crash kernel to boot into
> > >   x86: add --pass-memmap-cmdline option
> > >   x86: Pass memory range via E820 for kdump
> > > 
> > >  kexec/arch/i386/crashdump-x86.c        | 157 ++++++++++++++++--------------
> > >  kexec/arch/i386/crashdump-x86.h        |   6 +-
> > >  kexec/arch/i386/include/arch/options.h |   2 +
> > >  kexec/arch/i386/kexec-x86-common.c     |   6 +-
> > >  kexec/arch/i386/kexec-x86.c            |   4 +
> > >  kexec/arch/i386/kexec-x86.h            |   1 +
> > >  kexec/arch/i386/x86-linux-setup.c      | 171 ++++++++++++++++++++++-----------
> > >  kexec/arch/i386/x86-linux-setup.h      |   1 +
> > >  kexec/arch/x86_64/kexec-x86_64.c       |   5 +
> > >  kexec/kexec.c                          |  10 ++
> > >  kexec/kexec.h                          |   1 +
> > >  11 files changed, 231 insertions(+), 133 deletions(-)
> > 
> > Thanks for the patches. Tested (adding a fix provided by Dave Young for
> > http://lists.infradead.org/pipermail/kexec/2014-March/011262.html) and
> > successful with both kexec and kdump, both with and without
> > --pass-memmap-cmdline.
> 

Hi, Simon. I'll answer the questions below.

> Is the patch a pre-requisite for this series?

No. That's a separate problem of EFI. Old kexec-tools also needs the
patch.

> If so, has it been posted formally?

Dave Young has sent to patch to kexec mail list. The subject is as
follow:

[PATCH] kexec-tools: handle 64bit efi memmap address correctly

> If so, could you point it out to me?

Dave's patch:
http://lists.infradead.org/pipermail/kexec/2014-March/011303.html

Thanks
WANG Chao

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

Re: [PATCH v4 0/4] kexec-tools, x86: E820 memmap pass for kdump

[Vivek Goyal]

On Wed, Mar 19, 2014 at 04:03:57PM +0800, WANG Chao wrote:
> Hi, All
> 
> When kaslr comes in and kdump is broken, it seems about the right time to use
> E820 instead of memmap=exactmap to pass memmap for kdump for the default memmap
> passing mechanism:
> http://lists.infradead.org/pipermail/kexec/2014-February/011048.html

What's the connection between kaslr and passing memory mappings in
bootparams?

To me this is completely independent of kaslr. This is moving towards
a better design to pass memory map to second kernel which in turn will
also solve the issue of too many memory ranges on large machines.

Also, could you get hold of a machine with calgary IOMMU and test that
--pass-memmap option makes those calgary machines work?

Thanks
Vivek

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

Re: [PATCH v4 0/4] kexec-tools, x86: E820 memmap pass for kdump

[WANG Chao]

Hi, Vivek

Sorry for my late response.

On 03/20/14 at 11:42am, Vivek Goyal wrote:
> On Wed, Mar 19, 2014 at 04:03:57PM +0800, WANG Chao wrote:
> > Hi, All
> > 
> > When kaslr comes in and kdump is broken, it seems about the right time to use
> > E820 instead of memmap=exactmap to pass memmap for kdump for the default memmap
> > passing mechanism:
> > http://lists.infradead.org/pipermail/kexec/2014-February/011048.html
> 
> What's the connection between kaslr and passing memory mappings in
> bootparams?

kaslr doesn't work with memmap=exactmap. kaslr happens before parsing
user defined memmap and it walks through E820 in boot_params to
determine 2nd kernel text area. And it would override the 1st kernel's
ram.

> 
> To me this is completely independent of kaslr. This is moving towards
> a better design to pass memory map to second kernel which in turn will
> also solve the issue of too many memory ranges on large machines.

Yes. You're right. That's what this patchset is mostly for.
kASLR is just one of the reasons.

> 
> Also, could you get hold of a machine with calgary IOMMU and test that
> --pass-memmap option makes those calgary machines work?

I couldn't find any calgary box. I tried to force calgary iommu on some
IBM-X series, but didn't work out. Those old hardwares are really hard
to find these days.

I think given the fact that the calgary code chage is simple enough and
hpa has committed to tip tree, this patchset won't risk much.

Thanks
WANG Chao

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

Re: [PATCH v4 0/4] kexec-tools, x86: E820 memmap pass for kdump

[Vivek Goyal]

On Thu, Mar 27, 2014 at 06:31:54PM +0800, WANG Chao wrote:
> Hi, Vivek
> 
> Sorry for my late response.
> 
> On 03/20/14 at 11:42am, Vivek Goyal wrote:
> > On Wed, Mar 19, 2014 at 04:03:57PM +0800, WANG Chao wrote:
> > > Hi, All
> > > 
> > > When kaslr comes in and kdump is broken, it seems about the right time to use
> > > E820 instead of memmap=exactmap to pass memmap for kdump for the default memmap
> > > passing mechanism:
> > > http://lists.infradead.org/pipermail/kexec/2014-February/011048.html
> > 
> > What's the connection between kaslr and passing memory mappings in
> > bootparams?
> 
> kaslr doesn't work with memmap=exactmap. kaslr happens before parsing
> user defined memmap and it walks through E820 in boot_params to
> determine 2nd kernel text area. And it would override the 1st kernel's
> ram.

Ok got it. I forgot about that kaslr is broken with memmap=exactmap. So
this patchset is mandatory for making kexec-tools work with kaslr.

Thanks
Vivek

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

Re: [PATCH v4 2/4] x86: Store memory ranges globally used for crash kernel to boot into

[Vivek Goyal]

On Wed, Mar 19, 2014 at 04:03:59PM +0800, WANG Chao wrote:
> Use these two variables to store the memory ranges and the number of
> memory ranges for crash kernel to boot into:
> 
> struct memory_range crash_memory_range;
> int crash_memory_range;

I know it is my fault but I now feel that "nr_crash_ranges" is much
more intutive to represent number of crash ranges.

> 
> These two variables are not static now, so can be used in other file
> later.

So one could access these arrays with the help of pointers.
get_crash_memory_ranges() returns pointer to crash_memory_range
array. So why do we need to remove static cast.

And even if we have to, then why do we need to return pointer to
crash_memory_range array?

Thanks
Vivek

> 
> Signed-off-by: WANG Chao <chaowang@redhat.com>
> Tested-by: Linn Crosetto <linn@hp.com>
> ---
>  kexec/arch/i386/crashdump-x86.c | 134 ++++++++++++++++++++++------------------
>  kexec/arch/i386/crashdump-x86.h |   5 +-
>  2 files changed, 77 insertions(+), 62 deletions(-)
> 
> diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> index 979c2bd..c55a6b1 100644
> --- a/kexec/arch/i386/crashdump-x86.c
> +++ b/kexec/arch/i386/crashdump-x86.c
> @@ -179,7 +179,8 @@ static int exclude_region(int *nr_ranges, uint64_t start, uint64_t end);
>  
>  /* Stores a sorted list of RAM memory ranges for which to create elf headers.
>   * A separate program header is created for backup region */
> -static struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
> +struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
> +int crash_memory_ranges;
>  
>  /* Memory region reserved for storing panic kernel and other data. */
>  #define CRASH_RESERVED_MEM_NR	8
> @@ -201,7 +202,7 @@ static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
>  				   int kexec_flags, unsigned long lowmem_limit)
>  {
>  	const char *iomem = proc_iomem();
> -	int memory_ranges = 0, gart = 0, i;
> +	int gart = 0, i;
>  	char line[MAX_LINE];
>  	FILE *fp;
>  	unsigned long long start, end;
> @@ -218,7 +219,7 @@ static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
>  		char *str;
>  		int type, consumed, count;
>  
> -		if (memory_ranges >= CRASH_MAX_MEMORY_RANGES)
> +		if (crash_memory_ranges >= CRASH_MAX_MEMORY_RANGES)
>  			break;
>  		count = sscanf(line, "%Lx-%Lx : %n",
>  			&start, &end, &consumed);
> @@ -250,17 +251,17 @@ static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
>  			continue;
>  		}
>  
> -		crash_memory_range[memory_ranges].start = start;
> -		crash_memory_range[memory_ranges].end = end;
> -		crash_memory_range[memory_ranges].type = type;
> +		crash_memory_range[crash_memory_ranges].start = start;
> +		crash_memory_range[crash_memory_ranges].end = end;
> +		crash_memory_range[crash_memory_ranges].type = type;
>  
> -		segregate_lowmem_region(&memory_ranges, lowmem_limit);
> +		segregate_lowmem_region(&crash_memory_ranges, lowmem_limit);
>  
> -		memory_ranges++;
> +		crash_memory_ranges++;
>  	}
>  	fclose(fp);
>  	if (kexec_flags & KEXEC_PRESERVE_CONTEXT) {
> -		for (i = 0; i < memory_ranges; i++) {
> +		for (i = 0; i < crash_memory_ranges; i++) {
>  			if (crash_memory_range[i].end > 0x0009ffff) {
>  				crash_reserved_mem[0].start = \
>  					crash_memory_range[i].start;
> @@ -278,17 +279,17 @@ static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
>  	}
>  
>  	for (i = 0; i < crash_reserved_mem_nr; i++)
> -		if (exclude_region(&memory_ranges, crash_reserved_mem[i].start,
> +		if (exclude_region(&crash_memory_ranges, crash_reserved_mem[i].start,
>  				crash_reserved_mem[i].end) < 0)
>  			return -1;
>  
>  	if (gart) {
>  		/* exclude GART region if the system has one */
> -		if (exclude_region(&memory_ranges, gart_start, gart_end) < 0)
> +		if (exclude_region(&crash_memory_ranges, gart_start, gart_end) < 0)
>  			return -1;
>  	}
>  	*range = crash_memory_range;
> -	*ranges = memory_ranges;
> +	*ranges = crash_memory_ranges;
>  
>  	return 0;
>  }
> @@ -324,7 +325,7 @@ static int get_crash_memory_ranges_xen(struct memory_range **range,
>  	}
>  
>  	*range = crash_memory_range;
> -	*ranges = j;
> +	*ranges = crash_memory_ranges = j;
>  
>  	qsort(*range, *ranges, sizeof(struct memory_range), compare_ranges);
>  
> @@ -417,8 +418,8 @@ static int exclude_region(int *nr_ranges, uint64_t start, uint64_t end)
>  
>  /* Adds a segment from list of memory regions which new kernel can use to
>   * boot. Segment start and end should be aligned to 1K boundary. */
> -static int add_memmap(struct memory_range *memmap_p, unsigned long long addr,
> -								size_t size)
> +static int add_memmap(struct memory_range *memmap_p, int *nr_range,
> +					unsigned long long addr, size_t size)
>  {
>  	int i, j, nr_entries = 0, tidx = 0, align = 1024;
>  	unsigned long long mstart, mend;
> @@ -450,29 +451,23 @@ static int add_memmap(struct memory_range *memmap_p, unsigned long long addr,
>  		else if (addr > mend)
>  			tidx = i+1;
>  	}
> -		/* Insert the memory region. */
> -		for (j = nr_entries-1; j >= tidx; j--)
> -			memmap_p[j+1] = memmap_p[j];
> -		memmap_p[tidx].start = addr;
> -		memmap_p[tidx].end = addr + size - 1;
> +	/* Insert the memory region. */
> +	for (j = nr_entries-1; j >= tidx; j--)
> +		memmap_p[j+1] = memmap_p[j];
> +	memmap_p[tidx].start = addr;
> +	memmap_p[tidx].end = addr + size - 1;
> +	memmap_p[tidx].type = RANGE_RAM;
> +	*nr_range = nr_entries + 1;
>  
> -	dbgprintf("Memmap after adding segment\n");
> -	for (i = 0; i < CRASH_MAX_MEMMAP_NR;  i++) {
> -		mstart = memmap_p[i].start;
> -		mend = memmap_p[i].end;
> -		if (mstart == 0 && mend == 0)
> -			break;
> -		dbgprintf("%016llx - %016llx\n",
> -			mstart, mend);
> -	}
> +	dbgprint_mem_range("Memmap after adding segment", memmap_p, *nr_range);
>  
>  	return 0;
>  }
>  
>  /* Removes a segment from list of memory regions which new kernel can use to
>   * boot. Segment start and end should be aligned to 1K boundary. */
> -static int delete_memmap(struct memory_range *memmap_p, unsigned long long addr,
> -								size_t size)
> +static int delete_memmap(struct memory_range *memmap_p, int *nr_range,
> +					unsigned long long addr, size_t size)
>  {
>  	int i, j, nr_entries = 0, tidx = -1, operation = 0, align = 1024;
>  	unsigned long long mstart, mend;
> @@ -534,24 +529,17 @@ static int delete_memmap(struct memory_range *memmap_p, unsigned long long addr,
>  		for (j = nr_entries-1; j > tidx; j--)
>  			memmap_p[j+1] = memmap_p[j];
>  		memmap_p[tidx+1] = temp_region;
> +		*nr_range = nr_entries + 1;
>  	}
>  	if ((operation == -1) && tidx >=0) {
>  		/* Delete the exact match memory region. */
>  		for (j = i+1; j < CRASH_MAX_MEMMAP_NR; j++)
>  			memmap_p[j-1] = memmap_p[j];
>  		memmap_p[j-1].start = memmap_p[j-1].end = 0;
> +		*nr_range = nr_entries - 1;
>  	}
>  
> -	dbgprintf("Memmap after deleting segment\n");
> -	for (i = 0; i < CRASH_MAX_MEMMAP_NR;  i++) {
> -		mstart = memmap_p[i].start;
> -		mend = memmap_p[i].end;
> -		if (mstart == 0 && mend == 0) {
> -			break;
> -		}
> -		dbgprintf("%016llx - %016llx\n",
> -			mstart, mend);
> -	}
> +	dbgprint_mem_range("Memmap after deleting segment", memmap_p, *nr_range);
>  
>  	return 0;
>  }
> @@ -626,6 +614,9 @@ static int cmdline_add_memmap(char *cmdline, struct memory_range *memmap_p)
>  			/* All regions traversed. */
>  			break;
>  
> +		if (memmap_p[i].type != RANGE_RAM)
> +			continue;
> +
>  		/* A region is not worth adding if region size < 100K. It eats
>  		 * up precious command line length. */
>  		if ((endk - startk) < min_sizek)
> @@ -797,6 +788,25 @@ static void get_backup_area(struct kexec_info *info,
>  	info->backup_src_size = BACKUP_SRC_END - BACKUP_SRC_START + 1;
>  }
>  
> +static void exclude_ram(struct memory_range *mr, int *nr_mr)
> +{
> +	int ranges, i, j, m;
> +
> +	ranges = *nr_mr;
> +	for (i = 0, j = 0; i < ranges; i++) {
> +		if (mr[j].type == RANGE_RAM) {
> +			dbgprintf("Remove RAM %016llx-%016llxx: (%d)\n", mr[j].start, mr[j].end, mr[j].type);
> +			for (m = j; m < *nr_mr; m++)
> +				mr[m] = mr[m+1];
> +			(*nr_mr)--;
> +		} else {
> +			j++;
> +		}
> +	}
> +
> +	dbgprint_mem_range("After remove RAM", mr, *nr_mr);
> +}
> +
>  /* Loads additional segments in case of a panic kernel is being loaded.
>   * One segment for backup region, another segment for storing elf headers
>   * for crash memory image.
> @@ -807,7 +817,7 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
>  	void *tmp;
>  	unsigned long sz, bufsz, memsz, elfcorehdr;
>  	int nr_ranges = 0, align = 1024, i;
> -	struct memory_range *mem_range, *memmap_p;
> +	struct memory_range *mem_range;
>  	struct crash_elf_info elf_info;
>  	unsigned kexec_arch;
>  
> @@ -850,10 +860,7 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
>  
>  	get_backup_area(info, mem_range, nr_ranges);
>  
> -	dbgprintf("CRASH MEMORY RANGES\n");
> -
> -	for(i = 0; i < nr_ranges; ++i)
> -		dbgprintf("%016Lx-%016Lx\n", mem_range[i].start, mem_range[i].end);
> +	dbgprint_mem_range("CRASH MEMORY RANGES", mem_range, nr_ranges);
>  
>  	/*
>  	 * if the core type has not been set on command line, set it here
> @@ -878,17 +885,6 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
>  	if (get_kernel_vaddr_and_size(info, &elf_info))
>  		return -1;
>  
> -	/* Memory regions which panic kernel can safely use to boot into */
> -	sz = (sizeof(struct memory_range) * CRASH_MAX_MEMMAP_NR);
> -	memmap_p = xmalloc(sz);
> -	memset(memmap_p, 0, sz);
> -	add_memmap(memmap_p, info->backup_src_start, info->backup_src_size);
> -	for (i = 0; i < crash_reserved_mem_nr; i++) {
> -		sz = crash_reserved_mem[i].end - crash_reserved_mem[i].start +1;
> -		if (add_memmap(memmap_p, crash_reserved_mem[i].start, sz) < 0)
> -			return ENOCRASHKERNEL;
> -	}
> -
>  	/* Create a backup region segment to store backup data*/
>  	if (!(info->kexec_flags & KEXEC_PRESERVE_CONTEXT)) {
>  		sz = _ALIGN(info->backup_src_size, align);
> @@ -898,8 +894,6 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
>  						0, max_addr, -1);
>  		dbgprintf("Created backup segment at 0x%lx\n",
>  			  info->backup_start);
> -		if (delete_memmap(memmap_p, info->backup_start, sz) < 0)
> -			return EFAILED;
>  	}
>  
>  	/* Create elf header segment and store crash image data. */
> @@ -915,6 +909,23 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
>  						ELF_CORE_HEADER_ALIGN) < 0)
>  			return EFAILED;
>  	}
> +
> +	/* Memory regions which panic kernel can safely use to boot into */
> +	exclude_ram(crash_memory_range, &crash_memory_ranges);
> +
> +	add_memmap(crash_memory_range, &crash_memory_ranges, info->backup_src_start, info->backup_src_size);
> +	for (i = 0; i < crash_reserved_mem_nr; i++) {
> +		sz = crash_reserved_mem[i].end - crash_reserved_mem[i].start +1;
> +		if (add_memmap(crash_memory_range, &crash_memory_ranges, crash_reserved_mem[i].start, sz) < 0)
> +			return ENOCRASHKERNEL;
> +	}
> +
> +	/* exclude backup region from crash dump memory range */
> +	sz = _ALIGN(info->backup_src_size, align);
> +	if (delete_memmap(crash_memory_range, &crash_memory_ranges, info->backup_start, sz) < 0) {
> +		return EFAILED;
> +	}
> +
>  	/* the size of the elf headers allocated is returned in 'bufsz' */
>  
>  	/* Hack: With some ld versions (GNU ld version 2.14.90.0.4 20030523),
> @@ -934,9 +945,9 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
>  	elfcorehdr = add_buffer(info, tmp, bufsz, memsz, align, min_base,
>  							max_addr, -1);
>  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
> -	if (delete_memmap(memmap_p, elfcorehdr, memsz) < 0)
> +	if (delete_memmap(crash_memory_range, &crash_memory_ranges, elfcorehdr, memsz) < 0)
>  		return -1;
> -	cmdline_add_memmap(mod_cmdline, memmap_p);
> +	cmdline_add_memmap(mod_cmdline, crash_memory_range);
>  	if (!bzImage_support_efi_boot)
>  		cmdline_add_efi(mod_cmdline);
>  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
> @@ -951,6 +962,7 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
>  		end = mem_range[i].end;
>  		cmdline_add_memmap_acpi(mod_cmdline, start, end);
>  	}
> +
>  	return 0;
>  }
>  
> diff --git a/kexec/arch/i386/crashdump-x86.h b/kexec/arch/i386/crashdump-x86.h
> index b61cf0a..633ee0e 100644
> --- a/kexec/arch/i386/crashdump-x86.h
> +++ b/kexec/arch/i386/crashdump-x86.h
> @@ -20,7 +20,7 @@ int load_crashdump_segments(struct kexec_info *info, char *mod_cmdline,
>  /* Kernel text size */
>  #define X86_64_KERNEL_TEXT_SIZE  (512UL*1024*1024)
>  
> -#define CRASH_MAX_MEMMAP_NR	(KEXEC_MAX_SEGMENTS + 1)
> +#define CRASH_MAX_MEMMAP_NR	CRASH_MAX_MEMORY_RANGES
>  #define CRASH_MAX_MEMORY_RANGES	(MAX_MEMORY_RANGES + 2)
>  
>  /* Backup Region, First 640K of System RAM. */
> @@ -28,4 +28,7 @@ int load_crashdump_segments(struct kexec_info *info, char *mod_cmdline,
>  #define BACKUP_SRC_END		0x0009ffff
>  #define BACKUP_SRC_SIZE	(BACKUP_SRC_END - BACKUP_SRC_START + 1)
>  
> +extern struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
> +extern int crash_memory_ranges;
> +
>  #endif /* CRASHDUMP_X86_H */
> -- 
> 1.8.5.3
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

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

Re: [PATCH v4 4/4] x86: Pass memory range via E820 for kdump

[Vivek Goyal]

On Wed, Mar 19, 2014 at 04:04:01PM +0800, WANG Chao wrote:
> command line size is restricted by kernel, sometimes memmap=exactmap has
> too many memory ranges to pass to cmdline. A better approach, to pass the
> memory ranges for crash kernel to boot into, is filling the memory
> ranges into E820.
> 
> boot_params only got 128 slots for E820 map to fit in, when the number of
> memory map exceeds 128, use setup_data to pass the rest as extended E820
> memory map.
> 
> kexec boot could also benefit from setup_data in case E820 memory map
> exceeds 128.
> 
> Now this new approach becomes default instead of memmap=exactmap.
> saved_max_pfn users can specify --pass-memmap-cmdline to use the
> exactmap approach.

I think it is worth to also mention that kaslr enabled kernel does not
work with memmap=exactmap.

This patch in general looks good. Two minor nits below.

> 
> Signed-off-by: WANG Chao <chaowang@redhat.com>
> Tested-by: Linn Crosetto <linn@hp.com>
> Reviewed-by: Linn Crosetto <linn@hp.com>
> ---
>  kexec/arch/i386/crashdump-x86.c   |  25 +++---
>  kexec/arch/i386/crashdump-x86.h   |   1 +
>  kexec/arch/i386/x86-linux-setup.c | 171 +++++++++++++++++++++++++-------------
>  3 files changed, 130 insertions(+), 67 deletions(-)
> 
> diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> index c55a6b1..cb19e7d 100644
> --- a/kexec/arch/i386/crashdump-x86.c
> +++ b/kexec/arch/i386/crashdump-x86.c
> @@ -182,6 +182,8 @@ static int exclude_region(int *nr_ranges, uint64_t start, uint64_t end);
>  struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
>  int crash_memory_ranges;
>  
> +int pass_memmap_cmdline;
> +
>  /* Memory region reserved for storing panic kernel and other data. */
>  #define CRASH_RESERVED_MEM_NR	8
>  static struct memory_range crash_reserved_mem[CRASH_RESERVED_MEM_NR];
> @@ -947,20 +949,23 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
>  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
>  	if (delete_memmap(crash_memory_range, &crash_memory_ranges, elfcorehdr, memsz) < 0)
>  		return -1;
> -	cmdline_add_memmap(mod_cmdline, crash_memory_range);
>  	if (!bzImage_support_efi_boot)
>  		cmdline_add_efi(mod_cmdline);
>  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
>  
> -	/* Inform second kernel about the presence of ACPI tables. */
> -	for (i = 0; i < CRASH_MAX_MEMORY_RANGES; i++) {
> -		unsigned long start, end;
> -		if ( !( mem_range[i].type == RANGE_ACPI
> -			|| mem_range[i].type == RANGE_ACPI_NVS) )
> -			continue;
> -		start = mem_range[i].start;
> -		end = mem_range[i].end;
> -		cmdline_add_memmap_acpi(mod_cmdline, start, end);
> +	pass_memmap_cmdline = arch_options.pass_memmap_cmdline;
> +	if (pass_memmap_cmdline) {
> +		cmdline_add_memmap(mod_cmdline, crash_memory_range);
> +		/* Inform second kernel about the presence of ACPI tables. */
> +		for (i = 0; i < CRASH_MAX_MEMORY_RANGES; i++) {
> +			unsigned long start, end;
> +			if ( !( mem_range[i].type == RANGE_ACPI
> +						|| mem_range[i].type == RANGE_ACPI_NVS) )
> +				continue;
> +			start = mem_range[i].start;
> +			end = mem_range[i].end;
> +			cmdline_add_memmap_acpi(mod_cmdline, start, end);
> +		}
>  	}
>  
>  	return 0;
> diff --git a/kexec/arch/i386/crashdump-x86.h b/kexec/arch/i386/crashdump-x86.h
> index 633ee0e..e68b626 100644
> --- a/kexec/arch/i386/crashdump-x86.h
> +++ b/kexec/arch/i386/crashdump-x86.h
> @@ -30,5 +30,6 @@ int load_crashdump_segments(struct kexec_info *info, char *mod_cmdline,
>  
>  extern struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
>  extern int crash_memory_ranges;
> +extern int pass_memmap_cmdline;
>  
>  #endif /* CRASHDUMP_X86_H */
> diff --git a/kexec/arch/i386/x86-linux-setup.c b/kexec/arch/i386/x86-linux-setup.c
> index 5884f4d..e8865e1 100644
> --- a/kexec/arch/i386/x86-linux-setup.c
> +++ b/kexec/arch/i386/x86-linux-setup.c
> @@ -35,8 +35,7 @@
>  #include "kexec-x86.h"
>  #include "x86-linux-setup.h"
>  #include "../../kexec/kexec-syscall.h"
> -
> -#define SETUP_EFI	4
> +#include "crashdump-x86.h"
>  
>  void init_linux_parameters(struct x86_linux_param_header *real_mode)
>  {
> @@ -502,6 +501,11 @@ struct efi_setup_data {
>  struct setup_data {
>  	uint64_t next;
>  	uint32_t type;
> +#define SETUP_NONE	0
> +#define SETUP_E820_EXT	1
> +#define SETUP_DTB	2
> +#define SETUP_PCI	3
> +#define SETUP_EFI	4
>  	uint32_t len;
>  	uint8_t data[0];
>  } __attribute__((packed));
> @@ -602,6 +606,17 @@ struct efi_info {
>  	uint32_t efi_memmap_hi;
>  };
>  
> +static void add_setup_data(struct kexec_info *info,
> +			   struct x86_linux_param_header *real_mode,
> +			   struct setup_data *sd)
> +{

What is setup_data? A little comment above function will make it easy
to read. Is it that list of elements which contains extra memory map
entries?

> +	int sdsize = sizeof(struct setup_data) + sd->len;
> +
> +	sd->next = real_mode->setup_data;
> +	real_mode->setup_data = add_buffer(info, sd, sdsize, sdsize, getpagesize(),
> +			    0x100000, ULONG_MAX, INT_MAX);
> +}
> +
>  /*
>   * setup_efi_data will collect below data and pass them to 2nd kernel.
>   * 1) SMBIOS, fw_vendor, runtime, config_table, they are passed via x86
> @@ -611,11 +626,11 @@ struct efi_info {
>  static int setup_efi_data(struct kexec_info *info,
>  			  struct x86_linux_param_header *real_mode)
>  {
> -	int64_t setup_data_paddr, memmap_paddr;
> +	int64_t memmap_paddr;
>  	struct setup_data *sd;
>  	struct efi_setup_data *esd;
>  	struct efi_mem_descriptor *maps;
> -	int nr_maps, size, sdsize, ret = 0;
> +	int nr_maps, size, ret = 0;
>  	struct efi_info *ei = (struct efi_info *)real_mode->efi_info;
>  
>  	ret = access("/sys/firmware/efi/systab", F_OK);
> @@ -648,10 +663,8 @@ static int setup_efi_data(struct kexec_info *info,
>  	sd->len = sizeof(*esd);
>  	memcpy(sd->data, esd, sizeof(*esd));
>  	free(esd);
> -	sdsize = sd->len + sizeof(struct setup_data);
> -	setup_data_paddr = add_buffer(info, sd, sdsize, sdsize, getpagesize(),
> -					0x100000, ULONG_MAX, INT_MAX);
> -	real_mode->setup_data = setup_data_paddr;
> +
> +	add_setup_data(info, real_mode, sd);
>  
>  	size = nr_maps * sizeof(struct efi_mem_descriptor);
>  	memmap_paddr = add_buffer(info, maps, size, size, getpagesize(),
> @@ -669,6 +682,98 @@ out:
>  	return ret;
>  }
>  
> +static void add_e820_map_from_mr(struct x86_linux_param_header *real_mode,
> +			struct e820entry *e820, struct memory_range *range, int nr_range)
> +{
> +	int i;
> +
> +	for (i = 0; i < nr_range; i++) {
> +		e820[i].addr = range[i].start;
> +		e820[i].size = range[i].end - range[i].start;
> +		switch (range[i].type) {
> +			case RANGE_RAM:
> +				e820[i].type = E820_RAM;
> +				break;
> +			case RANGE_ACPI:
> +				e820[i].type = E820_ACPI;
> +				break;
> +			case RANGE_ACPI_NVS:
> +				e820[i].type = E820_NVS;
> +				break;
> +			default:
> +			case RANGE_RESERVED:
> +				e820[i].type = E820_RESERVED;
> +				break;
> +		}
> +		dbgprintf("%016lx-%016lx (%d)\n",
> +				e820[i].addr,
> +				e820[i].addr + e820[i].size - 1,
> +				e820[i].type);
> +
> +		if (range[i].type != RANGE_RAM)
> +			continue;
> +		if ((range[i].start <= 0x100000) && range[i].end > 0x100000) {
> +			unsigned long long mem_k = (range[i].end >> 10) - (0x100000 >> 10);
> +			real_mode->ext_mem_k = mem_k;
> +			real_mode->alt_mem_k = mem_k;
> +			if (mem_k > 0xfc00) {
> +				real_mode->ext_mem_k = 0xfc00; /* 64M */
> +			}
> +			if (mem_k > 0xffffffff) {
> +				real_mode->alt_mem_k = 0xffffffff;
> +			}
> +		}
> +	}
> +}
> +
> +static void setup_e820_ext(struct kexec_info *info, struct x86_linux_param_header *real_mode,
> +			   struct memory_range *range, int nr_range)
> +{
> +	struct setup_data *sd;
> +	struct e820entry *e820;
> +	int nr_range_ext;
> +
> +	nr_range_ext = nr_range - E820MAX;
> +	sd = xmalloc(sizeof(struct setup_data) + nr_range_ext * sizeof(struct e820entry));
> +	sd->next = 0;
> +	sd->len = nr_range_ext * sizeof(struct e820entry);
> +	sd->type = SETUP_E820_EXT;
> +
> +	e820 = (struct e820entry *) sd->data;
> +	dbgprintf("Extended E820 via setup_data:\n");
> +	add_e820_map_from_mr(real_mode, e820, range + E820MAX, nr_range_ext);
> +	add_setup_data(info, real_mode, sd);
> +}
> +
> +static void setup_e820(struct kexec_info *info, struct x86_linux_param_header *real_mode)
> +{
> +	struct memory_range *range;
> +	int nr_range, nr_range_saved;
> +
> +
> +	if (info->kexec_flags & KEXEC_ON_CRASH && !pass_memmap_cmdline) {
> +		range = crash_memory_range;
> +		nr_range = crash_memory_ranges;

You know what, it might be a good idea to store the pointer to
crash_memory_range in kexec_info too, (like memory_range and
memory_ranges).

> +	} else {
> +		range = info->memory_range;
> +		nr_range = info->memory_ranges;
> +	}
> +
> +	nr_range_saved = nr_range;
> +	if (nr_range > E820MAX) {
> +		nr_range = E820MAX;
> +	}
> +
> +	real_mode->e820_map_nr = nr_range;
> +	dbgprintf("E820 memmap:\n");
> +	add_e820_map_from_mr(real_mode, real_mode->e820_map, range, nr_range);
> +
> +	if (nr_range_saved > E820MAX) {
> +		dbgprintf("extra E820 memmap are passed via setup_data\n");
> +		setup_e820_ext(info, real_mode, range, nr_range_saved);
> +	}
> +}
> +
>  static int
>  get_efi_mem_desc_version(struct x86_linux_param_header *real_mode)
>  {
> @@ -702,10 +807,6 @@ static void setup_efi_info(struct kexec_info *info,
>  void setup_linux_system_parameters(struct kexec_info *info,
>  				   struct x86_linux_param_header *real_mode)
>  {
> -	/* Fill in information the BIOS would usually provide */
> -	struct memory_range *range;
> -	int i, ranges;
> -
>  	/* get subarch from running kernel */
>  	setup_subarch(real_mode);
>  	if (bzImage_support_efi_boot)
> @@ -746,51 +847,7 @@ void setup_linux_system_parameters(struct kexec_info *info,
>  	/* another safe default */
>  	real_mode->aux_device_info = 0;
>  
> -	range = info->memory_range;
> -	ranges = info->memory_ranges;
> -	if (ranges > E820MAX) {
> -		if (!(info->kexec_flags & KEXEC_ON_CRASH))
> -			/*
> -			 * this e820 not used for capture kernel, see
> -			 * do_bzImage_load()
> -			 */
> -			fprintf(stderr,
> -				"Too many memory ranges, truncating...\n");
> -		ranges = E820MAX;
> -	}
> -	real_mode->e820_map_nr = ranges;
> -	for(i = 0; i < ranges; i++) {
> -		real_mode->e820_map[i].addr = range[i].start;
> -		real_mode->e820_map[i].size = range[i].end - range[i].start;
> -		switch (range[i].type) {
> -		case RANGE_RAM:
> -			real_mode->e820_map[i].type = E820_RAM; 
> -			break;
> -		case RANGE_ACPI:
> -			real_mode->e820_map[i].type = E820_ACPI; 
> -			break;
> -		case RANGE_ACPI_NVS:
> -			real_mode->e820_map[i].type = E820_NVS;
> -			break;
> -		default:
> -		case RANGE_RESERVED:
> -			real_mode->e820_map[i].type = E820_RESERVED; 
> -			break;
> -		}
> -		if (range[i].type != RANGE_RAM)
> -			continue;
> -		if ((range[i].start <= 0x100000) && range[i].end > 0x100000) {
> -			unsigned long long mem_k = (range[i].end >> 10) - (0x100000 >> 10);
> -			real_mode->ext_mem_k = mem_k;
> -			real_mode->alt_mem_k = mem_k;
> -			if (mem_k > 0xfc00) {
> -				real_mode->ext_mem_k = 0xfc00; /* 64M */
> -			}
> -			if (mem_k > 0xffffffff) {
> -				real_mode->alt_mem_k = 0xffffffff;
> -			}
> -		}
> -	}
> +	setup_e820(info, real_mode);
>  
>  	/* fill the EDD information */
>  	setup_edd_info(real_mode);
> -- 
> 1.8.5.3
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

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

Re: [PATCH v4 2/4] x86: Store memory ranges globally used for crash kernel to boot into

[Dave Young]

On 03/19/14 at 04:03pm, WANG Chao wrote:
> Use these two variables to store the memory ranges and the number of
> memory ranges for crash kernel to boot into:
> 
> struct memory_range crash_memory_range;
> int crash_memory_range;
> 
> These two variables are not static now, so can be used in other file
> later.
> 
> Signed-off-by: WANG Chao <chaowang@redhat.com>
> Tested-by: Linn Crosetto <linn@hp.com>
> ---
>  kexec/arch/i386/crashdump-x86.c | 134 ++++++++++++++++++++++------------------
>  kexec/arch/i386/crashdump-x86.h |   5 +-
>  2 files changed, 77 insertions(+), 62 deletions(-)
> 
> diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> index 979c2bd..c55a6b1 100644
> --- a/kexec/arch/i386/crashdump-x86.c
> +++ b/kexec/arch/i386/crashdump-x86.c
> @@ -179,7 +179,8 @@ static int exclude_region(int *nr_ranges, uint64_t start, uint64_t end);
>  
>  /* Stores a sorted list of RAM memory ranges for which to create elf headers.
>   * A separate program header is created for backup region */
> -static struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
> +struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
> +int crash_memory_ranges;
>  
>  /* Memory region reserved for storing panic kernel and other data. */
>  #define CRASH_RESERVED_MEM_NR	8
> @@ -201,7 +202,7 @@ static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
>  				   int kexec_flags, unsigned long lowmem_limit)

It's not necessary to replace every static vars in the function, since there's param
*ranges and **range, so how about just replace them in caller function.

>  {
>  	const char *iomem = proc_iomem();
> -	int memory_ranges = 0, gart = 0, i;
> +	int gart = 0, i;
>  	char line[MAX_LINE];
>  	FILE *fp;
>  	unsigned long long start, end;
> @@ -218,7 +219,7 @@ static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
>  		char *str;
>  		int type, consumed, count;
>  
> -		if (memory_ranges >= CRASH_MAX_MEMORY_RANGES)
> +		if (crash_memory_ranges >= CRASH_MAX_MEMORY_RANGES)
>  			break;
>  		count = sscanf(line, "%Lx-%Lx : %n",
>  			&start, &end, &consumed);
> @@ -250,17 +251,17 @@ static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
>  			continue;
>  		}
>  
> -		crash_memory_range[memory_ranges].start = start;
> -		crash_memory_range[memory_ranges].end = end;
> -		crash_memory_range[memory_ranges].type = type;
> +		crash_memory_range[crash_memory_ranges].start = start;
> +		crash_memory_range[crash_memory_ranges].end = end;
> +		crash_memory_range[crash_memory_ranges].type = type;
>  
> -		segregate_lowmem_region(&memory_ranges, lowmem_limit);
> +		segregate_lowmem_region(&crash_memory_ranges, lowmem_limit);
>  
> -		memory_ranges++;
> +		crash_memory_ranges++;
>  	}
>  	fclose(fp);
>  	if (kexec_flags & KEXEC_PRESERVE_CONTEXT) {
> -		for (i = 0; i < memory_ranges; i++) {
> +		for (i = 0; i < crash_memory_ranges; i++) {
>  			if (crash_memory_range[i].end > 0x0009ffff) {
>  				crash_reserved_mem[0].start = \
>  					crash_memory_range[i].start;
> @@ -278,17 +279,17 @@ static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
>  	}
>  
>  	for (i = 0; i < crash_reserved_mem_nr; i++)
> -		if (exclude_region(&memory_ranges, crash_reserved_mem[i].start,
> +		if (exclude_region(&crash_memory_ranges, crash_reserved_mem[i].start,
>  				crash_reserved_mem[i].end) < 0)
>  			return -1;
>  
>  	if (gart) {
>  		/* exclude GART region if the system has one */
> -		if (exclude_region(&memory_ranges, gart_start, gart_end) < 0)
> +		if (exclude_region(&crash_memory_ranges, gart_start, gart_end) < 0)
>  			return -1;
>  	}
>  	*range = crash_memory_range;
> -	*ranges = memory_ranges;
> +	*ranges = crash_memory_ranges;
>  
>  	return 0;
>  }
> @@ -324,7 +325,7 @@ static int get_crash_memory_ranges_xen(struct memory_range **range,

Ditto for get_crash_memory_ranges_xen, just fix the caller function instead of use
global variable in this function.

>  	}
>  
>  	*range = crash_memory_range;
> -	*ranges = j;
> +	*ranges = crash_memory_ranges = j;
>  
>  	qsort(*range, *ranges, sizeof(struct memory_range), compare_ranges);
>  
> @@ -417,8 +418,8 @@ static int exclude_region(int *nr_ranges, uint64_t start, uint64_t end)
>  
>  /* Adds a segment from list of memory regions which new kernel can use to
>   * boot. Segment start and end should be aligned to 1K boundary. */
> -static int add_memmap(struct memory_range *memmap_p, unsigned long long addr,
> -								size_t size)
> +static int add_memmap(struct memory_range *memmap_p, int *nr_range,
> +					unsigned long long addr, size_t size)
>  {
>  	int i, j, nr_entries = 0, tidx = 0, align = 1024;
>  	unsigned long long mstart, mend;
> @@ -450,29 +451,23 @@ static int add_memmap(struct memory_range *memmap_p, unsigned long long addr,
>  		else if (addr > mend)
>  			tidx = i+1;
>  	}
> -		/* Insert the memory region. */
> -		for (j = nr_entries-1; j >= tidx; j--)
> -			memmap_p[j+1] = memmap_p[j];
> -		memmap_p[tidx].start = addr;
> -		memmap_p[tidx].end = addr + size - 1;
> +	/* Insert the memory region. */
> +	for (j = nr_entries-1; j >= tidx; j--)
> +		memmap_p[j+1] = memmap_p[j];
> +	memmap_p[tidx].start = addr;
> +	memmap_p[tidx].end = addr + size - 1;
> +	memmap_p[tidx].type = RANGE_RAM;
> +	*nr_range = nr_entries + 1;
>  
> -	dbgprintf("Memmap after adding segment\n");
> -	for (i = 0; i < CRASH_MAX_MEMMAP_NR;  i++) {
> -		mstart = memmap_p[i].start;
> -		mend = memmap_p[i].end;
> -		if (mstart == 0 && mend == 0)
> -			break;
> -		dbgprintf("%016llx - %016llx\n",
> -			mstart, mend);
> -	}
> +	dbgprint_mem_range("Memmap after adding segment", memmap_p, *nr_range);
>  
>  	return 0;
>  }
>  
>  /* Removes a segment from list of memory regions which new kernel can use to
>   * boot. Segment start and end should be aligned to 1K boundary. */
> -static int delete_memmap(struct memory_range *memmap_p, unsigned long long addr,
> -								size_t size)
> +static int delete_memmap(struct memory_range *memmap_p, int *nr_range,
> +					unsigned long long addr, size_t size)
>  {
>  	int i, j, nr_entries = 0, tidx = -1, operation = 0, align = 1024;
>  	unsigned long long mstart, mend;
> @@ -534,24 +529,17 @@ static int delete_memmap(struct memory_range *memmap_p, unsigned long long addr,
>  		for (j = nr_entries-1; j > tidx; j--)
>  			memmap_p[j+1] = memmap_p[j];
>  		memmap_p[tidx+1] = temp_region;
> +		*nr_range = nr_entries + 1;
>  	}
>  	if ((operation == -1) && tidx >=0) {
>  		/* Delete the exact match memory region. */
>  		for (j = i+1; j < CRASH_MAX_MEMMAP_NR; j++)
>  			memmap_p[j-1] = memmap_p[j];
>  		memmap_p[j-1].start = memmap_p[j-1].end = 0;
> +		*nr_range = nr_entries - 1;
>  	}
>  
> -	dbgprintf("Memmap after deleting segment\n");
> -	for (i = 0; i < CRASH_MAX_MEMMAP_NR;  i++) {
> -		mstart = memmap_p[i].start;
> -		mend = memmap_p[i].end;
> -		if (mstart == 0 && mend == 0) {
> -			break;
> -		}
> -		dbgprintf("%016llx - %016llx\n",
> -			mstart, mend);
> -	}
> +	dbgprint_mem_range("Memmap after deleting segment", memmap_p, *nr_range);
>  
>  	return 0;
>  }
> @@ -626,6 +614,9 @@ static int cmdline_add_memmap(char *cmdline, struct memory_range *memmap_p)
>  			/* All regions traversed. */
>  			break;
>  
> +		if (memmap_p[i].type != RANGE_RAM)
> +			continue;
> +
>  		/* A region is not worth adding if region size < 100K. It eats
>  		 * up precious command line length. */
>  		if ((endk - startk) < min_sizek)
> @@ -797,6 +788,25 @@ static void get_backup_area(struct kexec_info *info,
>  	info->backup_src_size = BACKUP_SRC_END - BACKUP_SRC_START + 1;
>  }
>  
> +static void exclude_ram(struct memory_range *mr, int *nr_mr)
> +{
> +	int ranges, i, j, m;
> +
> +	ranges = *nr_mr;
> +	for (i = 0, j = 0; i < ranges; i++) {
> +		if (mr[j].type == RANGE_RAM) {
> +			dbgprintf("Remove RAM %016llx-%016llxx: (%d)\n", mr[j].start, mr[j].end, mr[j].type);
> +			for (m = j; m < *nr_mr; m++)
> +				mr[m] = mr[m+1];
> +			(*nr_mr)--;
> +		} else {
> +			j++;
> +		}
> +	}
> +
> +	dbgprint_mem_range("After remove RAM", mr, *nr_mr);
> +}
> +
>  /* Loads additional segments in case of a panic kernel is being loaded.
>   * One segment for backup region, another segment for storing elf headers
>   * for crash memory image.
> @@ -807,7 +817,7 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
>  	void *tmp;
>  	unsigned long sz, bufsz, memsz, elfcorehdr;
>  	int nr_ranges = 0, align = 1024, i;
> -	struct memory_range *mem_range, *memmap_p;
> +	struct memory_range *mem_range;
>  	struct crash_elf_info elf_info;
>  	unsigned kexec_arch;
>  
> @@ -850,10 +860,7 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
>  
>  	get_backup_area(info, mem_range, nr_ranges);
>  
> -	dbgprintf("CRASH MEMORY RANGES\n");
> -
> -	for(i = 0; i < nr_ranges; ++i)
> -		dbgprintf("%016Lx-%016Lx\n", mem_range[i].start, mem_range[i].end);
> +	dbgprint_mem_range("CRASH MEMORY RANGES", mem_range, nr_ranges);
>  
>  	/*
>  	 * if the core type has not been set on command line, set it here
> @@ -878,17 +885,6 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
>  	if (get_kernel_vaddr_and_size(info, &elf_info))
>  		return -1;
>  
> -	/* Memory regions which panic kernel can safely use to boot into */
> -	sz = (sizeof(struct memory_range) * CRASH_MAX_MEMMAP_NR);
> -	memmap_p = xmalloc(sz);
> -	memset(memmap_p, 0, sz);
> -	add_memmap(memmap_p, info->backup_src_start, info->backup_src_size);
> -	for (i = 0; i < crash_reserved_mem_nr; i++) {
> -		sz = crash_reserved_mem[i].end - crash_reserved_mem[i].start +1;
> -		if (add_memmap(memmap_p, crash_reserved_mem[i].start, sz) < 0)
> -			return ENOCRASHKERNEL;
> -	}
> -
>  	/* Create a backup region segment to store backup data*/
>  	if (!(info->kexec_flags & KEXEC_PRESERVE_CONTEXT)) {
>  		sz = _ALIGN(info->backup_src_size, align);
> @@ -898,8 +894,6 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
>  						0, max_addr, -1);
>  		dbgprintf("Created backup segment at 0x%lx\n",
>  			  info->backup_start);
> -		if (delete_memmap(memmap_p, info->backup_start, sz) < 0)
> -			return EFAILED;
>  	}
>  
>  	/* Create elf header segment and store crash image data. */
> @@ -915,6 +909,23 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
>  						ELF_CORE_HEADER_ALIGN) < 0)
>  			return EFAILED;
>  	}
> +
> +	/* Memory regions which panic kernel can safely use to boot into */
> +	exclude_ram(crash_memory_range, &crash_memory_ranges);
> +
> +	add_memmap(crash_memory_range, &crash_memory_ranges, info->backup_src_start, info->backup_src_size);
> +	for (i = 0; i < crash_reserved_mem_nr; i++) {
> +		sz = crash_reserved_mem[i].end - crash_reserved_mem[i].start +1;
> +		if (add_memmap(crash_memory_range, &crash_memory_ranges, crash_reserved_mem[i].start, sz) < 0)
> +			return ENOCRASHKERNEL;
> +	}
> +
> +	/* exclude backup region from crash dump memory range */
> +	sz = _ALIGN(info->backup_src_size, align);
> +	if (delete_memmap(crash_memory_range, &crash_memory_ranges, info->backup_start, sz) < 0) {
> +		return EFAILED;
> +	}
> +
>  	/* the size of the elf headers allocated is returned in 'bufsz' */
>  
>  	/* Hack: With some ld versions (GNU ld version 2.14.90.0.4 20030523),
> @@ -934,9 +945,9 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
>  	elfcorehdr = add_buffer(info, tmp, bufsz, memsz, align, min_base,
>  							max_addr, -1);
>  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
> -	if (delete_memmap(memmap_p, elfcorehdr, memsz) < 0)
> +	if (delete_memmap(crash_memory_range, &crash_memory_ranges, elfcorehdr, memsz) < 0)
>  		return -1;
> -	cmdline_add_memmap(mod_cmdline, memmap_p);
> +	cmdline_add_memmap(mod_cmdline, crash_memory_range);
>  	if (!bzImage_support_efi_boot)
>  		cmdline_add_efi(mod_cmdline);
>  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
> @@ -951,6 +962,7 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
>  		end = mem_range[i].end;
>  		cmdline_add_memmap_acpi(mod_cmdline, start, end);
>  	}
> +
>  	return 0;
>  }
>  
> diff --git a/kexec/arch/i386/crashdump-x86.h b/kexec/arch/i386/crashdump-x86.h
> index b61cf0a..633ee0e 100644
> --- a/kexec/arch/i386/crashdump-x86.h
> +++ b/kexec/arch/i386/crashdump-x86.h
> @@ -20,7 +20,7 @@ int load_crashdump_segments(struct kexec_info *info, char *mod_cmdline,
>  /* Kernel text size */
>  #define X86_64_KERNEL_TEXT_SIZE  (512UL*1024*1024)
>  
> -#define CRASH_MAX_MEMMAP_NR	(KEXEC_MAX_SEGMENTS + 1)
> +#define CRASH_MAX_MEMMAP_NR	CRASH_MAX_MEMORY_RANGES
>  #define CRASH_MAX_MEMORY_RANGES	(MAX_MEMORY_RANGES + 2)
>  
>  /* Backup Region, First 640K of System RAM. */
> @@ -28,4 +28,7 @@ int load_crashdump_segments(struct kexec_info *info, char *mod_cmdline,
>  #define BACKUP_SRC_END		0x0009ffff
>  #define BACKUP_SRC_SIZE	(BACKUP_SRC_END - BACKUP_SRC_START + 1)
>  
> +extern struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
> +extern int crash_memory_ranges;
> +
>  #endif /* CRASHDUMP_X86_H */
> -- 
> 1.8.5.3
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

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

Re: [PATCH v4 2/4] x86: Store memory ranges globally used for crash kernel to boot into

[Dave Young]

>  
> +static void exclude_ram(struct memory_range *mr, int *nr_mr)
> +{
> +	int ranges, i, j, m;
> +
> +	ranges = *nr_mr;
> +	for (i = 0, j = 0; i < ranges; i++) {
> +		if (mr[j].type == RANGE_RAM) {
> +			dbgprintf("Remove RAM %016llx-%016llxx: (%d)\n", mr[j].start, mr[j].end, mr[j].type);
> +			for (m = j; m < *nr_mr; m++)
> +				mr[m] = mr[m+1];
> +			(*nr_mr)--;
> +		} else {
> +			j++;
> +		}
> +	}
> +
> +	dbgprint_mem_range("After remove RAM", mr, *nr_mr);
> +}

This is probably not necessary, what I understand you are doing is below:

get_crash_memory_ranges()
 -> collect all SYSTEM_RAM, ACPI, ACPI_NVS ranges, exclude crash reserved ranges.
 -> the system ram ranges are used to create elf header
 -> the ACPI, ACPI_NVS ranges are used by cmdline_add_memmap_acpi etc.

memmap_p
 -> contains all the crash reserved ranges
 -> to be used by cmdline_add_memmap

The several memory ranges are twisted and somehow the funcions are duplicate.

So how about just keep one memory ranges array which contains all the ranges which
include system_ram, acpi, acpi_nvs, crash_reserved range.

In the crashdump-elf.c the function for creating elf headers will check the
range type, it will just skip the range which is not ram.

Ditto for other functions they can also just select what range type they need instead
of creating these different arrays which is confusing.

Thanks
Dave

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

Re: [PATCH v4 4/4] x86: Pass memory range via E820 for kdump

[Dave Young]

On 03/19/14 at 04:04pm, WANG Chao wrote:
> command line size is restricted by kernel, sometimes memmap=exactmap has
> too many memory ranges to pass to cmdline. A better approach, to pass the
> memory ranges for crash kernel to boot into, is filling the memory
> ranges into E820.
> 
> boot_params only got 128 slots for E820 map to fit in, when the number of
> memory map exceeds 128, use setup_data to pass the rest as extended E820
> memory map.
> 
> kexec boot could also benefit from setup_data in case E820 memory map
> exceeds 128.
> 
> Now this new approach becomes default instead of memmap=exactmap.
> saved_max_pfn users can specify --pass-memmap-cmdline to use the
> exactmap approach.
> 
> Signed-off-by: WANG Chao <chaowang@redhat.com>
> Tested-by: Linn Crosetto <linn@hp.com>
> Reviewed-by: Linn Crosetto <linn@hp.com>
> ---
>  kexec/arch/i386/crashdump-x86.c   |  25 +++---
>  kexec/arch/i386/crashdump-x86.h   |   1 +
>  kexec/arch/i386/x86-linux-setup.c | 171 +++++++++++++++++++++++++-------------
>  3 files changed, 130 insertions(+), 67 deletions(-)
> 
> diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> index c55a6b1..cb19e7d 100644
> --- a/kexec/arch/i386/crashdump-x86.c
> +++ b/kexec/arch/i386/crashdump-x86.c
> @@ -182,6 +182,8 @@ static int exclude_region(int *nr_ranges, uint64_t start, uint64_t end);
>  struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
>  int crash_memory_ranges;
>  
> +int pass_memmap_cmdline;
> +
>  /* Memory region reserved for storing panic kernel and other data. */
>  #define CRASH_RESERVED_MEM_NR	8
>  static struct memory_range crash_reserved_mem[CRASH_RESERVED_MEM_NR];
> @@ -947,20 +949,23 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
>  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
>  	if (delete_memmap(crash_memory_range, &crash_memory_ranges, elfcorehdr, memsz) < 0)
>  		return -1;
> -	cmdline_add_memmap(mod_cmdline, crash_memory_range);
>  	if (!bzImage_support_efi_boot)
>  		cmdline_add_efi(mod_cmdline);
>  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
>  
> -	/* Inform second kernel about the presence of ACPI tables. */
> -	for (i = 0; i < CRASH_MAX_MEMORY_RANGES; i++) {
> -		unsigned long start, end;
> -		if ( !( mem_range[i].type == RANGE_ACPI
> -			|| mem_range[i].type == RANGE_ACPI_NVS) )
> -			continue;
> -		start = mem_range[i].start;
> -		end = mem_range[i].end;
> -		cmdline_add_memmap_acpi(mod_cmdline, start, end);
> +	pass_memmap_cmdline = arch_options.pass_memmap_cmdline;
> +	if (pass_memmap_cmdline) {
> +		cmdline_add_memmap(mod_cmdline, crash_memory_range);
> +		/* Inform second kernel about the presence of ACPI tables. */
> +		for (i = 0; i < CRASH_MAX_MEMORY_RANGES; i++) {
> +			unsigned long start, end;
> +			if ( !( mem_range[i].type == RANGE_ACPI
> +						|| mem_range[i].type == RANGE_ACPI_NVS) )
> +				continue;
> +			start = mem_range[i].start;
> +			end = mem_range[i].end;
> +			cmdline_add_memmap_acpi(mod_cmdline, start, end);
> +		}
>  	}
>  
>  	return 0;
> diff --git a/kexec/arch/i386/crashdump-x86.h b/kexec/arch/i386/crashdump-x86.h
> index 633ee0e..e68b626 100644
> --- a/kexec/arch/i386/crashdump-x86.h
> +++ b/kexec/arch/i386/crashdump-x86.h
> @@ -30,5 +30,6 @@ int load_crashdump_segments(struct kexec_info *info, char *mod_cmdline,
>  
>  extern struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
>  extern int crash_memory_ranges;
> +extern int pass_memmap_cmdline;
>  
>  #endif /* CRASHDUMP_X86_H */
> diff --git a/kexec/arch/i386/x86-linux-setup.c b/kexec/arch/i386/x86-linux-setup.c
> index 5884f4d..e8865e1 100644
> --- a/kexec/arch/i386/x86-linux-setup.c
> +++ b/kexec/arch/i386/x86-linux-setup.c
> @@ -35,8 +35,7 @@
>  #include "kexec-x86.h"
>  #include "x86-linux-setup.h"
>  #include "../../kexec/kexec-syscall.h"
> -
> -#define SETUP_EFI	4
> +#include "crashdump-x86.h"
>  
>  void init_linux_parameters(struct x86_linux_param_header *real_mode)
>  {
> @@ -502,6 +501,11 @@ struct efi_setup_data {
>  struct setup_data {
>  	uint64_t next;
>  	uint32_t type;
> +#define SETUP_NONE	0
> +#define SETUP_E820_EXT	1
> +#define SETUP_DTB	2
> +#define SETUP_PCI	3
> +#define SETUP_EFI	4
>  	uint32_t len;
>  	uint8_t data[0];
>  } __attribute__((packed));
> @@ -602,6 +606,17 @@ struct efi_info {
>  	uint32_t efi_memmap_hi;
>  };
>  
> +static void add_setup_data(struct kexec_info *info,
> +			   struct x86_linux_param_header *real_mode,
> +			   struct setup_data *sd)
> +{
> +	int sdsize = sizeof(struct setup_data) + sd->len;
> +
> +	sd->next = real_mode->setup_data;
> +	real_mode->setup_data = add_buffer(info, sd, sdsize, sdsize, getpagesize(),
> +			    0x100000, ULONG_MAX, INT_MAX);
> +}
> +
>  /*
>   * setup_efi_data will collect below data and pass them to 2nd kernel.
>   * 1) SMBIOS, fw_vendor, runtime, config_table, they are passed via x86
> @@ -611,11 +626,11 @@ struct efi_info {
>  static int setup_efi_data(struct kexec_info *info,
>  			  struct x86_linux_param_header *real_mode)
>  {
> -	int64_t setup_data_paddr, memmap_paddr;
> +	int64_t memmap_paddr;
>  	struct setup_data *sd;
>  	struct efi_setup_data *esd;
>  	struct efi_mem_descriptor *maps;
> -	int nr_maps, size, sdsize, ret = 0;
> +	int nr_maps, size, ret = 0;
>  	struct efi_info *ei = (struct efi_info *)real_mode->efi_info;
>  
>  	ret = access("/sys/firmware/efi/systab", F_OK);
> @@ -648,10 +663,8 @@ static int setup_efi_data(struct kexec_info *info,
>  	sd->len = sizeof(*esd);
>  	memcpy(sd->data, esd, sizeof(*esd));
>  	free(esd);
> -	sdsize = sd->len + sizeof(struct setup_data);
> -	setup_data_paddr = add_buffer(info, sd, sdsize, sdsize, getpagesize(),
> -					0x100000, ULONG_MAX, INT_MAX);
> -	real_mode->setup_data = setup_data_paddr;
> +
> +	add_setup_data(info, real_mode, sd);

Could you split the above add_setup_data to another patch?

Thanks
Dave

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

Re: [PATCH v4 4/4] x86: Pass memory range via E820 for kdump

[WANG Chao]

On 03/27/14 at 06:50pm, Vivek Goyal wrote:
> On Wed, Mar 19, 2014 at 04:04:01PM +0800, WANG Chao wrote:
> > command line size is restricted by kernel, sometimes memmap=exactmap has
> > too many memory ranges to pass to cmdline. A better approach, to pass the
> > memory ranges for crash kernel to boot into, is filling the memory
> > ranges into E820.
> > 
> > boot_params only got 128 slots for E820 map to fit in, when the number of
> > memory map exceeds 128, use setup_data to pass the rest as extended E820
> > memory map.
> > 
> > kexec boot could also benefit from setup_data in case E820 memory map
> > exceeds 128.
> > 
> > Now this new approach becomes default instead of memmap=exactmap.
> > saved_max_pfn users can specify --pass-memmap-cmdline to use the
> > exactmap approach.
> 
> I think it is worth to also mention that kaslr enabled kernel does not
> work with memmap=exactmap.

Sure. Will do.

> 
> This patch in general looks good. Two minor nits below.
> 
> > 
> > Signed-off-by: WANG Chao <chaowang@redhat.com>
> > Tested-by: Linn Crosetto <linn@hp.com>
> > Reviewed-by: Linn Crosetto <linn@hp.com>
> > ---
> >  kexec/arch/i386/crashdump-x86.c   |  25 +++---
> >  kexec/arch/i386/crashdump-x86.h   |   1 +
> >  kexec/arch/i386/x86-linux-setup.c | 171 +++++++++++++++++++++++++-------------
> >  3 files changed, 130 insertions(+), 67 deletions(-)
> > 
> > diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> > index c55a6b1..cb19e7d 100644
> > --- a/kexec/arch/i386/crashdump-x86.c
> > +++ b/kexec/arch/i386/crashdump-x86.c
> > @@ -182,6 +182,8 @@ static int exclude_region(int *nr_ranges, uint64_t start, uint64_t end);
> >  struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
> >  int crash_memory_ranges;
> >  
> > +int pass_memmap_cmdline;
> > +
> >  /* Memory region reserved for storing panic kernel and other data. */
> >  #define CRASH_RESERVED_MEM_NR	8
> >  static struct memory_range crash_reserved_mem[CRASH_RESERVED_MEM_NR];
> > @@ -947,20 +949,23 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> >  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
> >  	if (delete_memmap(crash_memory_range, &crash_memory_ranges, elfcorehdr, memsz) < 0)
> >  		return -1;
> > -	cmdline_add_memmap(mod_cmdline, crash_memory_range);
> >  	if (!bzImage_support_efi_boot)
> >  		cmdline_add_efi(mod_cmdline);
> >  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
> >  
> > -	/* Inform second kernel about the presence of ACPI tables. */
> > -	for (i = 0; i < CRASH_MAX_MEMORY_RANGES; i++) {
> > -		unsigned long start, end;
> > -		if ( !( mem_range[i].type == RANGE_ACPI
> > -			|| mem_range[i].type == RANGE_ACPI_NVS) )
> > -			continue;
> > -		start = mem_range[i].start;
> > -		end = mem_range[i].end;
> > -		cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > +	pass_memmap_cmdline = arch_options.pass_memmap_cmdline;
> > +	if (pass_memmap_cmdline) {
> > +		cmdline_add_memmap(mod_cmdline, crash_memory_range);
> > +		/* Inform second kernel about the presence of ACPI tables. */
> > +		for (i = 0; i < CRASH_MAX_MEMORY_RANGES; i++) {
> > +			unsigned long start, end;
> > +			if ( !( mem_range[i].type == RANGE_ACPI
> > +						|| mem_range[i].type == RANGE_ACPI_NVS) )
> > +				continue;
> > +			start = mem_range[i].start;
> > +			end = mem_range[i].end;
> > +			cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > +		}
> >  	}
> >  
> >  	return 0;
> > diff --git a/kexec/arch/i386/crashdump-x86.h b/kexec/arch/i386/crashdump-x86.h
> > index 633ee0e..e68b626 100644
> > --- a/kexec/arch/i386/crashdump-x86.h
> > +++ b/kexec/arch/i386/crashdump-x86.h
> > @@ -30,5 +30,6 @@ int load_crashdump_segments(struct kexec_info *info, char *mod_cmdline,
> >  
> >  extern struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
> >  extern int crash_memory_ranges;
> > +extern int pass_memmap_cmdline;
> >  
> >  #endif /* CRASHDUMP_X86_H */
> > diff --git a/kexec/arch/i386/x86-linux-setup.c b/kexec/arch/i386/x86-linux-setup.c
> > index 5884f4d..e8865e1 100644
> > --- a/kexec/arch/i386/x86-linux-setup.c
> > +++ b/kexec/arch/i386/x86-linux-setup.c
> > @@ -35,8 +35,7 @@
> >  #include "kexec-x86.h"
> >  #include "x86-linux-setup.h"
> >  #include "../../kexec/kexec-syscall.h"
> > -
> > -#define SETUP_EFI	4
> > +#include "crashdump-x86.h"
> >  
> >  void init_linux_parameters(struct x86_linux_param_header *real_mode)
> >  {
> > @@ -502,6 +501,11 @@ struct efi_setup_data {
> >  struct setup_data {
> >  	uint64_t next;
> >  	uint32_t type;
> > +#define SETUP_NONE	0
> > +#define SETUP_E820_EXT	1
> > +#define SETUP_DTB	2
> > +#define SETUP_PCI	3
> > +#define SETUP_EFI	4
> >  	uint32_t len;
> >  	uint8_t data[0];
> >  } __attribute__((packed));
> > @@ -602,6 +606,17 @@ struct efi_info {
> >  	uint32_t efi_memmap_hi;
> >  };
> >  
> > +static void add_setup_data(struct kexec_info *info,
> > +			   struct x86_linux_param_header *real_mode,
> > +			   struct setup_data *sd)
> > +{
> 
> What is setup_data? A little comment above function will make it easy
> to read. Is it that list of elements which contains extra memory map
> entries?

Not exactly. All extra memory maps (for SETUP_E820_EXT type) are
sealed into a single setup_data structure. Different types of setup_data
are linked in a list.

setup_data can be used to pass extra data for boot, for example EFI
data (SETUP_EFI), extended E820 map (SETUP_E820_EXT), SETUP_PCI and
SETUP_DTB. These types are defined when defining struct setup_data.

It's offically documented in Documentation/x86/boot.txt.

Field name:	setup_data
Type:		write (special)
Offset/size:	0x250/8
Protocol:	2.09+

  The 64-bit physical pointer to NULL terminated single linked list of
  struct setup_data. This is used to define a more extensible boot
  parameters passing mechanism. The definition of struct setup_data
  is as follow:

  struct setup_data {
	  u64 next;
	  u32 type;
	  u32 len;
	  u8  data[0];
  };

  Where, the next is a 64-bit physical pointer to the next node of
  linked list, the next field of the last node is 0; the type is used
  to identify the contents of data; the len is the length of data
  field; the data holds the real payload.

  This list may be modified at a number of points during the bootup
  process. Therefore, when modifying this list one should always make
  sure to consider the case where the linked list already contains
  entries.

I think I would comment add_setup_data as follows:

/*
 * Added another instance to single linked list of struct setup_data.
 * Please refer to kernel Documentation/x86/boot.txt for more details
 * about setup_data structure.
 */

> 
> > +	int sdsize = sizeof(struct setup_data) + sd->len;
> > +
> > +	sd->next = real_mode->setup_data;
> > +	real_mode->setup_data = add_buffer(info, sd, sdsize, sdsize, getpagesize(),
> > +			    0x100000, ULONG_MAX, INT_MAX);
> > +}
> > +
> >  /*
> >   * setup_efi_data will collect below data and pass them to 2nd kernel.
> >   * 1) SMBIOS, fw_vendor, runtime, config_table, they are passed via x86
> > @@ -611,11 +626,11 @@ struct efi_info {
> >  static int setup_efi_data(struct kexec_info *info,
> >  			  struct x86_linux_param_header *real_mode)
> >  {
> > -	int64_t setup_data_paddr, memmap_paddr;
> > +	int64_t memmap_paddr;
> >  	struct setup_data *sd;
> >  	struct efi_setup_data *esd;
> >  	struct efi_mem_descriptor *maps;
> > -	int nr_maps, size, sdsize, ret = 0;
> > +	int nr_maps, size, ret = 0;
> >  	struct efi_info *ei = (struct efi_info *)real_mode->efi_info;
> >  
> >  	ret = access("/sys/firmware/efi/systab", F_OK);
> > @@ -648,10 +663,8 @@ static int setup_efi_data(struct kexec_info *info,
> >  	sd->len = sizeof(*esd);
> >  	memcpy(sd->data, esd, sizeof(*esd));
> >  	free(esd);
> > -	sdsize = sd->len + sizeof(struct setup_data);
> > -	setup_data_paddr = add_buffer(info, sd, sdsize, sdsize, getpagesize(),
> > -					0x100000, ULONG_MAX, INT_MAX);
> > -	real_mode->setup_data = setup_data_paddr;
> > +
> > +	add_setup_data(info, real_mode, sd);
> >  
> >  	size = nr_maps * sizeof(struct efi_mem_descriptor);
> >  	memmap_paddr = add_buffer(info, maps, size, size, getpagesize(),
> > @@ -669,6 +682,98 @@ out:
> >  	return ret;
> >  }
> >  
> > +static void add_e820_map_from_mr(struct x86_linux_param_header *real_mode,
> > +			struct e820entry *e820, struct memory_range *range, int nr_range)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < nr_range; i++) {
> > +		e820[i].addr = range[i].start;
> > +		e820[i].size = range[i].end - range[i].start;
> > +		switch (range[i].type) {
> > +			case RANGE_RAM:
> > +				e820[i].type = E820_RAM;
> > +				break;
> > +			case RANGE_ACPI:
> > +				e820[i].type = E820_ACPI;
> > +				break;
> > +			case RANGE_ACPI_NVS:
> > +				e820[i].type = E820_NVS;
> > +				break;
> > +			default:
> > +			case RANGE_RESERVED:
> > +				e820[i].type = E820_RESERVED;
> > +				break;
> > +		}
> > +		dbgprintf("%016lx-%016lx (%d)\n",
> > +				e820[i].addr,
> > +				e820[i].addr + e820[i].size - 1,
> > +				e820[i].type);
> > +
> > +		if (range[i].type != RANGE_RAM)
> > +			continue;
> > +		if ((range[i].start <= 0x100000) && range[i].end > 0x100000) {
> > +			unsigned long long mem_k = (range[i].end >> 10) - (0x100000 >> 10);
> > +			real_mode->ext_mem_k = mem_k;
> > +			real_mode->alt_mem_k = mem_k;
> > +			if (mem_k > 0xfc00) {
> > +				real_mode->ext_mem_k = 0xfc00; /* 64M */
> > +			}
> > +			if (mem_k > 0xffffffff) {
> > +				real_mode->alt_mem_k = 0xffffffff;
> > +			}
> > +		}
> > +	}
> > +}
> > +
> > +static void setup_e820_ext(struct kexec_info *info, struct x86_linux_param_header *real_mode,
> > +			   struct memory_range *range, int nr_range)
> > +{
> > +	struct setup_data *sd;
> > +	struct e820entry *e820;
> > +	int nr_range_ext;
> > +
> > +	nr_range_ext = nr_range - E820MAX;
> > +	sd = xmalloc(sizeof(struct setup_data) + nr_range_ext * sizeof(struct e820entry));
> > +	sd->next = 0;
> > +	sd->len = nr_range_ext * sizeof(struct e820entry);
> > +	sd->type = SETUP_E820_EXT;
> > +
> > +	e820 = (struct e820entry *) sd->data;
> > +	dbgprintf("Extended E820 via setup_data:\n");
> > +	add_e820_map_from_mr(real_mode, e820, range + E820MAX, nr_range_ext);
> > +	add_setup_data(info, real_mode, sd);
> > +}
> > +
> > +static void setup_e820(struct kexec_info *info, struct x86_linux_param_header *real_mode)
> > +{
> > +	struct memory_range *range;
> > +	int nr_range, nr_range_saved;
> > +
> > +
> > +	if (info->kexec_flags & KEXEC_ON_CRASH && !pass_memmap_cmdline) {
> > +		range = crash_memory_range;
> > +		nr_range = crash_memory_ranges;
> 
> You know what, it might be a good idea to store the pointer to
> crash_memory_range in kexec_info too, (like memory_range and
> memory_ranges).

Will do.

Thanks for your review.

WANG Chao
> 
> > +	} else {
> > +		range = info->memory_range;
> > +		nr_range = info->memory_ranges;
> > +	}
> > +
> > +	nr_range_saved = nr_range;
> > +	if (nr_range > E820MAX) {
> > +		nr_range = E820MAX;
> > +	}
> > +
> > +	real_mode->e820_map_nr = nr_range;
> > +	dbgprintf("E820 memmap:\n");
> > +	add_e820_map_from_mr(real_mode, real_mode->e820_map, range, nr_range);
> > +
> > +	if (nr_range_saved > E820MAX) {
> > +		dbgprintf("extra E820 memmap are passed via setup_data\n");
> > +		setup_e820_ext(info, real_mode, range, nr_range_saved);
> > +	}
> > +}
> > +
> >  static int
> >  get_efi_mem_desc_version(struct x86_linux_param_header *real_mode)
> >  {
> > @@ -702,10 +807,6 @@ static void setup_efi_info(struct kexec_info *info,
> >  void setup_linux_system_parameters(struct kexec_info *info,
> >  				   struct x86_linux_param_header *real_mode)
> >  {
> > -	/* Fill in information the BIOS would usually provide */
> > -	struct memory_range *range;
> > -	int i, ranges;
> > -
> >  	/* get subarch from running kernel */
> >  	setup_subarch(real_mode);
> >  	if (bzImage_support_efi_boot)
> > @@ -746,51 +847,7 @@ void setup_linux_system_parameters(struct kexec_info *info,
> >  	/* another safe default */
> >  	real_mode->aux_device_info = 0;
> >  
> > -	range = info->memory_range;
> > -	ranges = info->memory_ranges;
> > -	if (ranges > E820MAX) {
> > -		if (!(info->kexec_flags & KEXEC_ON_CRASH))
> > -			/*
> > -			 * this e820 not used for capture kernel, see
> > -			 * do_bzImage_load()
> > -			 */
> > -			fprintf(stderr,
> > -				"Too many memory ranges, truncating...\n");
> > -		ranges = E820MAX;
> > -	}
> > -	real_mode->e820_map_nr = ranges;
> > -	for(i = 0; i < ranges; i++) {
> > -		real_mode->e820_map[i].addr = range[i].start;
> > -		real_mode->e820_map[i].size = range[i].end - range[i].start;
> > -		switch (range[i].type) {
> > -		case RANGE_RAM:
> > -			real_mode->e820_map[i].type = E820_RAM; 
> > -			break;
> > -		case RANGE_ACPI:
> > -			real_mode->e820_map[i].type = E820_ACPI; 
> > -			break;
> > -		case RANGE_ACPI_NVS:
> > -			real_mode->e820_map[i].type = E820_NVS;
> > -			break;
> > -		default:
> > -		case RANGE_RESERVED:
> > -			real_mode->e820_map[i].type = E820_RESERVED; 
> > -			break;
> > -		}
> > -		if (range[i].type != RANGE_RAM)
> > -			continue;
> > -		if ((range[i].start <= 0x100000) && range[i].end > 0x100000) {
> > -			unsigned long long mem_k = (range[i].end >> 10) - (0x100000 >> 10);
> > -			real_mode->ext_mem_k = mem_k;
> > -			real_mode->alt_mem_k = mem_k;
> > -			if (mem_k > 0xfc00) {
> > -				real_mode->ext_mem_k = 0xfc00; /* 64M */
> > -			}
> > -			if (mem_k > 0xffffffff) {
> > -				real_mode->alt_mem_k = 0xffffffff;
> > -			}
> > -		}
> > -	}
> > +	setup_e820(info, real_mode);
> >  
> >  	/* fill the EDD information */
> >  	setup_edd_info(real_mode);
> > -- 
> > 1.8.5.3
> > 
> > 
> > _______________________________________________
> > kexec mailing list
> > kexec@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/kexec

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

Re: [PATCH v4 4/4] x86: Pass memory range via E820 for kdump

[WANG Chao]

On 03/28/14 at 11:28am, Dave Young wrote:
> On 03/19/14 at 04:04pm, WANG Chao wrote:
> > command line size is restricted by kernel, sometimes memmap=exactmap has
> > too many memory ranges to pass to cmdline. A better approach, to pass the
> > memory ranges for crash kernel to boot into, is filling the memory
> > ranges into E820.
> > 
> > boot_params only got 128 slots for E820 map to fit in, when the number of
> > memory map exceeds 128, use setup_data to pass the rest as extended E820
> > memory map.
> > 
> > kexec boot could also benefit from setup_data in case E820 memory map
> > exceeds 128.
> > 
> > Now this new approach becomes default instead of memmap=exactmap.
> > saved_max_pfn users can specify --pass-memmap-cmdline to use the
> > exactmap approach.
> > 
> > Signed-off-by: WANG Chao <chaowang@redhat.com>
> > Tested-by: Linn Crosetto <linn@hp.com>
> > Reviewed-by: Linn Crosetto <linn@hp.com>
> > ---
> >  kexec/arch/i386/crashdump-x86.c   |  25 +++---
> >  kexec/arch/i386/crashdump-x86.h   |   1 +
> >  kexec/arch/i386/x86-linux-setup.c | 171 +++++++++++++++++++++++++-------------
> >  3 files changed, 130 insertions(+), 67 deletions(-)
> > 
> > diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> > index c55a6b1..cb19e7d 100644
> > --- a/kexec/arch/i386/crashdump-x86.c
> > +++ b/kexec/arch/i386/crashdump-x86.c
> > @@ -182,6 +182,8 @@ static int exclude_region(int *nr_ranges, uint64_t start, uint64_t end);
> >  struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
> >  int crash_memory_ranges;
> >  
> > +int pass_memmap_cmdline;
> > +
> >  /* Memory region reserved for storing panic kernel and other data. */
> >  #define CRASH_RESERVED_MEM_NR	8
> >  static struct memory_range crash_reserved_mem[CRASH_RESERVED_MEM_NR];
> > @@ -947,20 +949,23 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> >  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
> >  	if (delete_memmap(crash_memory_range, &crash_memory_ranges, elfcorehdr, memsz) < 0)
> >  		return -1;
> > -	cmdline_add_memmap(mod_cmdline, crash_memory_range);
> >  	if (!bzImage_support_efi_boot)
> >  		cmdline_add_efi(mod_cmdline);
> >  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
> >  
> > -	/* Inform second kernel about the presence of ACPI tables. */
> > -	for (i = 0; i < CRASH_MAX_MEMORY_RANGES; i++) {
> > -		unsigned long start, end;
> > -		if ( !( mem_range[i].type == RANGE_ACPI
> > -			|| mem_range[i].type == RANGE_ACPI_NVS) )
> > -			continue;
> > -		start = mem_range[i].start;
> > -		end = mem_range[i].end;
> > -		cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > +	pass_memmap_cmdline = arch_options.pass_memmap_cmdline;
> > +	if (pass_memmap_cmdline) {
> > +		cmdline_add_memmap(mod_cmdline, crash_memory_range);
> > +		/* Inform second kernel about the presence of ACPI tables. */
> > +		for (i = 0; i < CRASH_MAX_MEMORY_RANGES; i++) {
> > +			unsigned long start, end;
> > +			if ( !( mem_range[i].type == RANGE_ACPI
> > +						|| mem_range[i].type == RANGE_ACPI_NVS) )
> > +				continue;
> > +			start = mem_range[i].start;
> > +			end = mem_range[i].end;
> > +			cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > +		}
> >  	}
> >  
> >  	return 0;
> > diff --git a/kexec/arch/i386/crashdump-x86.h b/kexec/arch/i386/crashdump-x86.h
> > index 633ee0e..e68b626 100644
> > --- a/kexec/arch/i386/crashdump-x86.h
> > +++ b/kexec/arch/i386/crashdump-x86.h
> > @@ -30,5 +30,6 @@ int load_crashdump_segments(struct kexec_info *info, char *mod_cmdline,
> >  
> >  extern struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
> >  extern int crash_memory_ranges;
> > +extern int pass_memmap_cmdline;
> >  
> >  #endif /* CRASHDUMP_X86_H */
> > diff --git a/kexec/arch/i386/x86-linux-setup.c b/kexec/arch/i386/x86-linux-setup.c
> > index 5884f4d..e8865e1 100644
> > --- a/kexec/arch/i386/x86-linux-setup.c
> > +++ b/kexec/arch/i386/x86-linux-setup.c
> > @@ -35,8 +35,7 @@
> >  #include "kexec-x86.h"
> >  #include "x86-linux-setup.h"
> >  #include "../../kexec/kexec-syscall.h"
> > -
> > -#define SETUP_EFI	4
> > +#include "crashdump-x86.h"
> >  
> >  void init_linux_parameters(struct x86_linux_param_header *real_mode)
> >  {
> > @@ -502,6 +501,11 @@ struct efi_setup_data {
> >  struct setup_data {
> >  	uint64_t next;
> >  	uint32_t type;
> > +#define SETUP_NONE	0
> > +#define SETUP_E820_EXT	1
> > +#define SETUP_DTB	2
> > +#define SETUP_PCI	3
> > +#define SETUP_EFI	4
> >  	uint32_t len;
> >  	uint8_t data[0];
> >  } __attribute__((packed));
> > @@ -602,6 +606,17 @@ struct efi_info {
> >  	uint32_t efi_memmap_hi;
> >  };
> >  
> > +static void add_setup_data(struct kexec_info *info,
> > +			   struct x86_linux_param_header *real_mode,
> > +			   struct setup_data *sd)
> > +{
> > +	int sdsize = sizeof(struct setup_data) + sd->len;
> > +
> > +	sd->next = real_mode->setup_data;
> > +	real_mode->setup_data = add_buffer(info, sd, sdsize, sdsize, getpagesize(),
> > +			    0x100000, ULONG_MAX, INT_MAX);
> > +}
> > +
> >  /*
> >   * setup_efi_data will collect below data and pass them to 2nd kernel.
> >   * 1) SMBIOS, fw_vendor, runtime, config_table, they are passed via x86
> > @@ -611,11 +626,11 @@ struct efi_info {
> >  static int setup_efi_data(struct kexec_info *info,
> >  			  struct x86_linux_param_header *real_mode)
> >  {
> > -	int64_t setup_data_paddr, memmap_paddr;
> > +	int64_t memmap_paddr;
> >  	struct setup_data *sd;
> >  	struct efi_setup_data *esd;
> >  	struct efi_mem_descriptor *maps;
> > -	int nr_maps, size, sdsize, ret = 0;
> > +	int nr_maps, size, ret = 0;
> >  	struct efi_info *ei = (struct efi_info *)real_mode->efi_info;
> >  
> >  	ret = access("/sys/firmware/efi/systab", F_OK);
> > @@ -648,10 +663,8 @@ static int setup_efi_data(struct kexec_info *info,
> >  	sd->len = sizeof(*esd);
> >  	memcpy(sd->data, esd, sizeof(*esd));
> >  	free(esd);
> > -	sdsize = sd->len + sizeof(struct setup_data);
> > -	setup_data_paddr = add_buffer(info, sd, sdsize, sdsize, getpagesize(),
> > -					0x100000, ULONG_MAX, INT_MAX);
> > -	real_mode->setup_data = setup_data_paddr;
> > +
> > +	add_setup_data(info, real_mode, sd);
> 
> Could you split the above add_setup_data to another patch?

OK. Will do.

Thanks for review.

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

Re: [PATCH v4 2/4] x86: Store memory ranges globally used for crash kernel to boot into

[WANG Chao]

On 03/27/14 at 06:32pm, Vivek Goyal wrote:
> On Wed, Mar 19, 2014 at 04:03:59PM +0800, WANG Chao wrote:
> > Use these two variables to store the memory ranges and the number of
> > memory ranges for crash kernel to boot into:
> > 
> > struct memory_range crash_memory_range;
> > int crash_memory_range;
> 
> I know it is my fault but I now feel that "nr_crash_ranges" is much
> more intutive to represent number of crash ranges.
> 
> > 
> > These two variables are not static now, so can be used in other file
> > later.
> 
> So one could access these arrays with the help of pointers.
> get_crash_memory_ranges() returns pointer to crash_memory_range
> array. So why do we need to remove static cast.

get_crash_memory_ranges() is static, but we want to use
crash_memory_range in x86-linux-setup.c. So we have to make
crash_memory_range and crash_memory_ranges global variables.

But as you pointed out it might be good to store crash_memory_range and
crash_memory_ranges in the global kexec_info structure as we did for
memory_range and memory_ranges, we could get rid of these two global
variables.

> 
> And even if we have to, then why do we need to return pointer to
> crash_memory_range array?

For historical reason, get_crash_memory_ranges() is not only set
crash_memory_range but also return the pointer of another copy of
crash_memory_range:

static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
				   int kexec_flags, unsigned long lowmem_limit)
{
	[..]
	crash_memory_range = xxx;
	[..]
	*range = crash_memory_range;
	*ranges = memory_range;
}

I was just trying to keep the change as minimal as possible, so that the
reviewers can be more clear of what the patch does instead of something
looks messed up. But if you have no problem review it, I can do some
clean up within this patch. However I think it's better to be addressed
the cleanup in the future, or at least as a separated patch in this
series.

Thanks
WANG Chao

> 
> Thanks
> Vivek
> 
> > 
> > Signed-off-by: WANG Chao <chaowang@redhat.com>
> > Tested-by: Linn Crosetto <linn@hp.com>
> > ---
> >  kexec/arch/i386/crashdump-x86.c | 134 ++++++++++++++++++++++------------------
> >  kexec/arch/i386/crashdump-x86.h |   5 +-
> >  2 files changed, 77 insertions(+), 62 deletions(-)
> > 
> > diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> > index 979c2bd..c55a6b1 100644
> > --- a/kexec/arch/i386/crashdump-x86.c
> > +++ b/kexec/arch/i386/crashdump-x86.c
> > @@ -179,7 +179,8 @@ static int exclude_region(int *nr_ranges, uint64_t start, uint64_t end);
> >  
> >  /* Stores a sorted list of RAM memory ranges for which to create elf headers.
> >   * A separate program header is created for backup region */
> > -static struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
> > +struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
> > +int crash_memory_ranges;
> >  
> >  /* Memory region reserved for storing panic kernel and other data. */
> >  #define CRASH_RESERVED_MEM_NR	8
> > @@ -201,7 +202,7 @@ static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
> >  				   int kexec_flags, unsigned long lowmem_limit)
> >  {
> >  	const char *iomem = proc_iomem();
> > -	int memory_ranges = 0, gart = 0, i;
> > +	int gart = 0, i;
> >  	char line[MAX_LINE];
> >  	FILE *fp;
> >  	unsigned long long start, end;
> > @@ -218,7 +219,7 @@ static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
> >  		char *str;
> >  		int type, consumed, count;
> >  
> > -		if (memory_ranges >= CRASH_MAX_MEMORY_RANGES)
> > +		if (crash_memory_ranges >= CRASH_MAX_MEMORY_RANGES)
> >  			break;
> >  		count = sscanf(line, "%Lx-%Lx : %n",
> >  			&start, &end, &consumed);
> > @@ -250,17 +251,17 @@ static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
> >  			continue;
> >  		}
> >  
> > -		crash_memory_range[memory_ranges].start = start;
> > -		crash_memory_range[memory_ranges].end = end;
> > -		crash_memory_range[memory_ranges].type = type;
> > +		crash_memory_range[crash_memory_ranges].start = start;
> > +		crash_memory_range[crash_memory_ranges].end = end;
> > +		crash_memory_range[crash_memory_ranges].type = type;
> >  
> > -		segregate_lowmem_region(&memory_ranges, lowmem_limit);
> > +		segregate_lowmem_region(&crash_memory_ranges, lowmem_limit);
> >  
> > -		memory_ranges++;
> > +		crash_memory_ranges++;
> >  	}
> >  	fclose(fp);
> >  	if (kexec_flags & KEXEC_PRESERVE_CONTEXT) {
> > -		for (i = 0; i < memory_ranges; i++) {
> > +		for (i = 0; i < crash_memory_ranges; i++) {
> >  			if (crash_memory_range[i].end > 0x0009ffff) {
> >  				crash_reserved_mem[0].start = \
> >  					crash_memory_range[i].start;
> > @@ -278,17 +279,17 @@ static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
> >  	}
> >  
> >  	for (i = 0; i < crash_reserved_mem_nr; i++)
> > -		if (exclude_region(&memory_ranges, crash_reserved_mem[i].start,
> > +		if (exclude_region(&crash_memory_ranges, crash_reserved_mem[i].start,
> >  				crash_reserved_mem[i].end) < 0)
> >  			return -1;
> >  
> >  	if (gart) {
> >  		/* exclude GART region if the system has one */
> > -		if (exclude_region(&memory_ranges, gart_start, gart_end) < 0)
> > +		if (exclude_region(&crash_memory_ranges, gart_start, gart_end) < 0)
> >  			return -1;
> >  	}
> >  	*range = crash_memory_range;
> > -	*ranges = memory_ranges;
> > +	*ranges = crash_memory_ranges;
> >  
> >  	return 0;
> >  }
> > @@ -324,7 +325,7 @@ static int get_crash_memory_ranges_xen(struct memory_range **range,
> >  	}
> >  
> >  	*range = crash_memory_range;
> > -	*ranges = j;
> > +	*ranges = crash_memory_ranges = j;
> >  
> >  	qsort(*range, *ranges, sizeof(struct memory_range), compare_ranges);
> >  
> > @@ -417,8 +418,8 @@ static int exclude_region(int *nr_ranges, uint64_t start, uint64_t end)
> >  
> >  /* Adds a segment from list of memory regions which new kernel can use to
> >   * boot. Segment start and end should be aligned to 1K boundary. */
> > -static int add_memmap(struct memory_range *memmap_p, unsigned long long addr,
> > -								size_t size)
> > +static int add_memmap(struct memory_range *memmap_p, int *nr_range,
> > +					unsigned long long addr, size_t size)
> >  {
> >  	int i, j, nr_entries = 0, tidx = 0, align = 1024;
> >  	unsigned long long mstart, mend;
> > @@ -450,29 +451,23 @@ static int add_memmap(struct memory_range *memmap_p, unsigned long long addr,
> >  		else if (addr > mend)
> >  			tidx = i+1;
> >  	}
> > -		/* Insert the memory region. */
> > -		for (j = nr_entries-1; j >= tidx; j--)
> > -			memmap_p[j+1] = memmap_p[j];
> > -		memmap_p[tidx].start = addr;
> > -		memmap_p[tidx].end = addr + size - 1;
> > +	/* Insert the memory region. */
> > +	for (j = nr_entries-1; j >= tidx; j--)
> > +		memmap_p[j+1] = memmap_p[j];
> > +	memmap_p[tidx].start = addr;
> > +	memmap_p[tidx].end = addr + size - 1;
> > +	memmap_p[tidx].type = RANGE_RAM;
> > +	*nr_range = nr_entries + 1;
> >  
> > -	dbgprintf("Memmap after adding segment\n");
> > -	for (i = 0; i < CRASH_MAX_MEMMAP_NR;  i++) {
> > -		mstart = memmap_p[i].start;
> > -		mend = memmap_p[i].end;
> > -		if (mstart == 0 && mend == 0)
> > -			break;
> > -		dbgprintf("%016llx - %016llx\n",
> > -			mstart, mend);
> > -	}
> > +	dbgprint_mem_range("Memmap after adding segment", memmap_p, *nr_range);
> >  
> >  	return 0;
> >  }
> >  
> >  /* Removes a segment from list of memory regions which new kernel can use to
> >   * boot. Segment start and end should be aligned to 1K boundary. */
> > -static int delete_memmap(struct memory_range *memmap_p, unsigned long long addr,
> > -								size_t size)
> > +static int delete_memmap(struct memory_range *memmap_p, int *nr_range,
> > +					unsigned long long addr, size_t size)
> >  {
> >  	int i, j, nr_entries = 0, tidx = -1, operation = 0, align = 1024;
> >  	unsigned long long mstart, mend;
> > @@ -534,24 +529,17 @@ static int delete_memmap(struct memory_range *memmap_p, unsigned long long addr,
> >  		for (j = nr_entries-1; j > tidx; j--)
> >  			memmap_p[j+1] = memmap_p[j];
> >  		memmap_p[tidx+1] = temp_region;
> > +		*nr_range = nr_entries + 1;
> >  	}
> >  	if ((operation == -1) && tidx >=0) {
> >  		/* Delete the exact match memory region. */
> >  		for (j = i+1; j < CRASH_MAX_MEMMAP_NR; j++)
> >  			memmap_p[j-1] = memmap_p[j];
> >  		memmap_p[j-1].start = memmap_p[j-1].end = 0;
> > +		*nr_range = nr_entries - 1;
> >  	}
> >  
> > -	dbgprintf("Memmap after deleting segment\n");
> > -	for (i = 0; i < CRASH_MAX_MEMMAP_NR;  i++) {
> > -		mstart = memmap_p[i].start;
> > -		mend = memmap_p[i].end;
> > -		if (mstart == 0 && mend == 0) {
> > -			break;
> > -		}
> > -		dbgprintf("%016llx - %016llx\n",
> > -			mstart, mend);
> > -	}
> > +	dbgprint_mem_range("Memmap after deleting segment", memmap_p, *nr_range);
> >  
> >  	return 0;
> >  }
> > @@ -626,6 +614,9 @@ static int cmdline_add_memmap(char *cmdline, struct memory_range *memmap_p)
> >  			/* All regions traversed. */
> >  			break;
> >  
> > +		if (memmap_p[i].type != RANGE_RAM)
> > +			continue;
> > +
> >  		/* A region is not worth adding if region size < 100K. It eats
> >  		 * up precious command line length. */
> >  		if ((endk - startk) < min_sizek)
> > @@ -797,6 +788,25 @@ static void get_backup_area(struct kexec_info *info,
> >  	info->backup_src_size = BACKUP_SRC_END - BACKUP_SRC_START + 1;
> >  }
> >  
> > +static void exclude_ram(struct memory_range *mr, int *nr_mr)
> > +{
> > +	int ranges, i, j, m;
> > +
> > +	ranges = *nr_mr;
> > +	for (i = 0, j = 0; i < ranges; i++) {
> > +		if (mr[j].type == RANGE_RAM) {
> > +			dbgprintf("Remove RAM %016llx-%016llxx: (%d)\n", mr[j].start, mr[j].end, mr[j].type);
> > +			for (m = j; m < *nr_mr; m++)
> > +				mr[m] = mr[m+1];
> > +			(*nr_mr)--;
> > +		} else {
> > +			j++;
> > +		}
> > +	}
> > +
> > +	dbgprint_mem_range("After remove RAM", mr, *nr_mr);
> > +}
> > +
> >  /* Loads additional segments in case of a panic kernel is being loaded.
> >   * One segment for backup region, another segment for storing elf headers
> >   * for crash memory image.
> > @@ -807,7 +817,7 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> >  	void *tmp;
> >  	unsigned long sz, bufsz, memsz, elfcorehdr;
> >  	int nr_ranges = 0, align = 1024, i;
> > -	struct memory_range *mem_range, *memmap_p;
> > +	struct memory_range *mem_range;
> >  	struct crash_elf_info elf_info;
> >  	unsigned kexec_arch;
> >  
> > @@ -850,10 +860,7 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> >  
> >  	get_backup_area(info, mem_range, nr_ranges);
> >  
> > -	dbgprintf("CRASH MEMORY RANGES\n");
> > -
> > -	for(i = 0; i < nr_ranges; ++i)
> > -		dbgprintf("%016Lx-%016Lx\n", mem_range[i].start, mem_range[i].end);
> > +	dbgprint_mem_range("CRASH MEMORY RANGES", mem_range, nr_ranges);
> >  
> >  	/*
> >  	 * if the core type has not been set on command line, set it here
> > @@ -878,17 +885,6 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> >  	if (get_kernel_vaddr_and_size(info, &elf_info))
> >  		return -1;
> >  
> > -	/* Memory regions which panic kernel can safely use to boot into */
> > -	sz = (sizeof(struct memory_range) * CRASH_MAX_MEMMAP_NR);
> > -	memmap_p = xmalloc(sz);
> > -	memset(memmap_p, 0, sz);
> > -	add_memmap(memmap_p, info->backup_src_start, info->backup_src_size);
> > -	for (i = 0; i < crash_reserved_mem_nr; i++) {
> > -		sz = crash_reserved_mem[i].end - crash_reserved_mem[i].start +1;
> > -		if (add_memmap(memmap_p, crash_reserved_mem[i].start, sz) < 0)
> > -			return ENOCRASHKERNEL;
> > -	}
> > -
> >  	/* Create a backup region segment to store backup data*/
> >  	if (!(info->kexec_flags & KEXEC_PRESERVE_CONTEXT)) {
> >  		sz = _ALIGN(info->backup_src_size, align);
> > @@ -898,8 +894,6 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> >  						0, max_addr, -1);
> >  		dbgprintf("Created backup segment at 0x%lx\n",
> >  			  info->backup_start);
> > -		if (delete_memmap(memmap_p, info->backup_start, sz) < 0)
> > -			return EFAILED;
> >  	}
> >  
> >  	/* Create elf header segment and store crash image data. */
> > @@ -915,6 +909,23 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> >  						ELF_CORE_HEADER_ALIGN) < 0)
> >  			return EFAILED;
> >  	}
> > +
> > +	/* Memory regions which panic kernel can safely use to boot into */
> > +	exclude_ram(crash_memory_range, &crash_memory_ranges);
> > +
> > +	add_memmap(crash_memory_range, &crash_memory_ranges, info->backup_src_start, info->backup_src_size);
> > +	for (i = 0; i < crash_reserved_mem_nr; i++) {
> > +		sz = crash_reserved_mem[i].end - crash_reserved_mem[i].start +1;
> > +		if (add_memmap(crash_memory_range, &crash_memory_ranges, crash_reserved_mem[i].start, sz) < 0)
> > +			return ENOCRASHKERNEL;
> > +	}
> > +
> > +	/* exclude backup region from crash dump memory range */
> > +	sz = _ALIGN(info->backup_src_size, align);
> > +	if (delete_memmap(crash_memory_range, &crash_memory_ranges, info->backup_start, sz) < 0) {
> > +		return EFAILED;
> > +	}
> > +
> >  	/* the size of the elf headers allocated is returned in 'bufsz' */
> >  
> >  	/* Hack: With some ld versions (GNU ld version 2.14.90.0.4 20030523),
> > @@ -934,9 +945,9 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> >  	elfcorehdr = add_buffer(info, tmp, bufsz, memsz, align, min_base,
> >  							max_addr, -1);
> >  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
> > -	if (delete_memmap(memmap_p, elfcorehdr, memsz) < 0)
> > +	if (delete_memmap(crash_memory_range, &crash_memory_ranges, elfcorehdr, memsz) < 0)
> >  		return -1;
> > -	cmdline_add_memmap(mod_cmdline, memmap_p);
> > +	cmdline_add_memmap(mod_cmdline, crash_memory_range);
> >  	if (!bzImage_support_efi_boot)
> >  		cmdline_add_efi(mod_cmdline);
> >  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
> > @@ -951,6 +962,7 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> >  		end = mem_range[i].end;
> >  		cmdline_add_memmap_acpi(mod_cmdline, start, end);
> >  	}
> > +
> >  	return 0;
> >  }
> >  
> > diff --git a/kexec/arch/i386/crashdump-x86.h b/kexec/arch/i386/crashdump-x86.h
> > index b61cf0a..633ee0e 100644
> > --- a/kexec/arch/i386/crashdump-x86.h
> > +++ b/kexec/arch/i386/crashdump-x86.h
> > @@ -20,7 +20,7 @@ int load_crashdump_segments(struct kexec_info *info, char *mod_cmdline,
> >  /* Kernel text size */
> >  #define X86_64_KERNEL_TEXT_SIZE  (512UL*1024*1024)
> >  
> > -#define CRASH_MAX_MEMMAP_NR	(KEXEC_MAX_SEGMENTS + 1)
> > +#define CRASH_MAX_MEMMAP_NR	CRASH_MAX_MEMORY_RANGES
> >  #define CRASH_MAX_MEMORY_RANGES	(MAX_MEMORY_RANGES + 2)
> >  
> >  /* Backup Region, First 640K of System RAM. */
> > @@ -28,4 +28,7 @@ int load_crashdump_segments(struct kexec_info *info, char *mod_cmdline,
> >  #define BACKUP_SRC_END		0x0009ffff
> >  #define BACKUP_SRC_SIZE	(BACKUP_SRC_END - BACKUP_SRC_START + 1)
> >  
> > +extern struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
> > +extern int crash_memory_ranges;
> > +
> >  #endif /* CRASHDUMP_X86_H */
> > -- 
> > 1.8.5.3
> > 
> > 
> > _______________________________________________
> > kexec mailing list
> > kexec@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/kexec

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

Re: [PATCH v4 2/4] x86: Store memory ranges globally used for crash kernel to boot into

[WANG Chao]

On 03/28/14 at 10:19am, Dave Young wrote:
> On 03/19/14 at 04:03pm, WANG Chao wrote:
> > Use these two variables to store the memory ranges and the number of
> > memory ranges for crash kernel to boot into:
> > 
> > struct memory_range crash_memory_range;
> > int crash_memory_range;
> > 
> > These two variables are not static now, so can be used in other file
> > later.
> > 
> > Signed-off-by: WANG Chao <chaowang@redhat.com>
> > Tested-by: Linn Crosetto <linn@hp.com>
> > ---
> >  kexec/arch/i386/crashdump-x86.c | 134 ++++++++++++++++++++++------------------
> >  kexec/arch/i386/crashdump-x86.h |   5 +-
> >  2 files changed, 77 insertions(+), 62 deletions(-)
> > 
> > diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> > index 979c2bd..c55a6b1 100644
> > --- a/kexec/arch/i386/crashdump-x86.c
> > +++ b/kexec/arch/i386/crashdump-x86.c
> > @@ -179,7 +179,8 @@ static int exclude_region(int *nr_ranges, uint64_t start, uint64_t end);
> >  
> >  /* Stores a sorted list of RAM memory ranges for which to create elf headers.
> >   * A separate program header is created for backup region */
> > -static struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
> > +struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
> > +int crash_memory_ranges;
> >  
> >  /* Memory region reserved for storing panic kernel and other data. */
> >  #define CRASH_RESERVED_MEM_NR	8
> > @@ -201,7 +202,7 @@ static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
> >  				   int kexec_flags, unsigned long lowmem_limit)
> 
> It's not necessary to replace every static vars in the function, since there's param
> *ranges and **range, so how about just replace them in caller function.

OK. Will do.

> 
> >  {
> >  	const char *iomem = proc_iomem();
> > -	int memory_ranges = 0, gart = 0, i;
> > +	int gart = 0, i;
> >  	char line[MAX_LINE];
> >  	FILE *fp;
> >  	unsigned long long start, end;
> > @@ -218,7 +219,7 @@ static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
> >  		char *str;
> >  		int type, consumed, count;
> >  
> > -		if (memory_ranges >= CRASH_MAX_MEMORY_RANGES)
> > +		if (crash_memory_ranges >= CRASH_MAX_MEMORY_RANGES)
> >  			break;
> >  		count = sscanf(line, "%Lx-%Lx : %n",
> >  			&start, &end, &consumed);
> > @@ -250,17 +251,17 @@ static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
> >  			continue;
> >  		}
> >  
> > -		crash_memory_range[memory_ranges].start = start;
> > -		crash_memory_range[memory_ranges].end = end;
> > -		crash_memory_range[memory_ranges].type = type;
> > +		crash_memory_range[crash_memory_ranges].start = start;
> > +		crash_memory_range[crash_memory_ranges].end = end;
> > +		crash_memory_range[crash_memory_ranges].type = type;
> >  
> > -		segregate_lowmem_region(&memory_ranges, lowmem_limit);
> > +		segregate_lowmem_region(&crash_memory_ranges, lowmem_limit);
> >  
> > -		memory_ranges++;
> > +		crash_memory_ranges++;
> >  	}
> >  	fclose(fp);
> >  	if (kexec_flags & KEXEC_PRESERVE_CONTEXT) {
> > -		for (i = 0; i < memory_ranges; i++) {
> > +		for (i = 0; i < crash_memory_ranges; i++) {
> >  			if (crash_memory_range[i].end > 0x0009ffff) {
> >  				crash_reserved_mem[0].start = \
> >  					crash_memory_range[i].start;
> > @@ -278,17 +279,17 @@ static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
> >  	}
> >  
> >  	for (i = 0; i < crash_reserved_mem_nr; i++)
> > -		if (exclude_region(&memory_ranges, crash_reserved_mem[i].start,
> > +		if (exclude_region(&crash_memory_ranges, crash_reserved_mem[i].start,
> >  				crash_reserved_mem[i].end) < 0)
> >  			return -1;
> >  
> >  	if (gart) {
> >  		/* exclude GART region if the system has one */
> > -		if (exclude_region(&memory_ranges, gart_start, gart_end) < 0)
> > +		if (exclude_region(&crash_memory_ranges, gart_start, gart_end) < 0)
> >  			return -1;
> >  	}
> >  	*range = crash_memory_range;
> > -	*ranges = memory_ranges;
> > +	*ranges = crash_memory_ranges;
> >  
> >  	return 0;
> >  }
> > @@ -324,7 +325,7 @@ static int get_crash_memory_ranges_xen(struct memory_range **range,
> 
> Ditto for get_crash_memory_ranges_xen, just fix the caller function instead of use
> global variable in this function.

Will do.

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

Re: [PATCH v4 2/4] x86: Store memory ranges globally used for crash kernel to boot into

[WANG Chao]

On 03/28/14 at 11:24am, Dave Young wrote:
> >  
> > +static void exclude_ram(struct memory_range *mr, int *nr_mr)
> > +{
> > +	int ranges, i, j, m;
> > +
> > +	ranges = *nr_mr;
> > +	for (i = 0, j = 0; i < ranges; i++) {
> > +		if (mr[j].type == RANGE_RAM) {
> > +			dbgprintf("Remove RAM %016llx-%016llxx: (%d)\n", mr[j].start, mr[j].end, mr[j].type);
> > +			for (m = j; m < *nr_mr; m++)
> > +				mr[m] = mr[m+1];
> > +			(*nr_mr)--;
> > +		} else {
> > +			j++;
> > +		}
> > +	}
> > +
> > +	dbgprint_mem_range("After remove RAM", mr, *nr_mr);
> > +}
> 
> This is probably not necessary, what I understand you are doing is below:
> 
> get_crash_memory_ranges()
>  -> collect all SYSTEM_RAM, ACPI, ACPI_NVS ranges, exclude crash reserved ranges.
>  -> the system ram ranges are used to create elf header
>  -> the ACPI, ACPI_NVS ranges are used by cmdline_add_memmap_acpi etc.

Yes.

> 
> memmap_p
>  -> contains all the crash reserved ranges
>  -> to be used by cmdline_add_memmap

There's no memmap_p. I'll reuse crash_memory_ranges structure to store
crash reserved ranges, ACPI and ACPI_NVS ranges. So after building ELF
headers for 1st kernel memory ranges, all I have to do is exclude the
SYSTEM_RAM and add crash_reserved to crash_memory_ranges. And then
crash_memory_ranges can be used as 2nd kernel memory ranges.

> 
> The several memory ranges are twisted and somehow the funcions are duplicate.

These functions look like similar but each does serve for different purpose.

> 
> So how about just keep one memory ranges array which contains all the ranges which
> include system_ram, acpi, acpi_nvs, crash_reserved range.
> 
> In the crashdump-elf.c the function for creating elf headers will check the
> range type, it will just skip the range which is not ram.

We don't build EFL headers for crash_reserved ranges. So we can't store
system ram and crash_reserved together before building EFL header.

> 
> Ditto for other functions they can also just select what range type they need instead
> of creating these different arrays which is confusing.
> 
> Thanks
> Dave

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

Re: [PATCH v4 2/4] x86: Store memory ranges globally used for crash kernel to boot into

[Dave Young]

On 03/28/14 at 02:13pm, WANG Chao wrote:
> On 03/28/14 at 11:24am, Dave Young wrote:
> > >  
> > > +static void exclude_ram(struct memory_range *mr, int *nr_mr)
> > > +{
> > > +	int ranges, i, j, m;
> > > +
> > > +	ranges = *nr_mr;
> > > +	for (i = 0, j = 0; i < ranges; i++) {
> > > +		if (mr[j].type == RANGE_RAM) {
> > > +			dbgprintf("Remove RAM %016llx-%016llxx: (%d)\n", mr[j].start, mr[j].end, mr[j].type);
> > > +			for (m = j; m < *nr_mr; m++)
> > > +				mr[m] = mr[m+1];
> > > +			(*nr_mr)--;
> > > +		} else {
> > > +			j++;
> > > +		}
> > > +	}
> > > +
> > > +	dbgprint_mem_range("After remove RAM", mr, *nr_mr);
> > > +}
> > 
> > This is probably not necessary, what I understand you are doing is below:
> > 
> > get_crash_memory_ranges()
> >  -> collect all SYSTEM_RAM, ACPI, ACPI_NVS ranges, exclude crash reserved ranges.
> >  -> the system ram ranges are used to create elf header
> >  -> the ACPI, ACPI_NVS ranges are used by cmdline_add_memmap_acpi etc.
> 
> Yes.
> 
> > 
> > memmap_p
> >  -> contains all the crash reserved ranges
> >  -> to be used by cmdline_add_memmap
> 
> There's no memmap_p. I'll reuse crash_memory_ranges structure to store
> crash reserved ranges, ACPI and ACPI_NVS ranges. So after building ELF
> headers for 1st kernel memory ranges, all I have to do is exclude the
> SYSTEM_RAM and add crash_reserved to crash_memory_ranges. And then
> crash_memory_ranges can be used as 2nd kernel memory ranges.

How about do nothing and directly use the mem_ranges:
 * skip RANGE_RAM, only add the range which is not RANGE_RAM
 * if the range is RANGE_CRASH_KERNEL (introduce a new type?) then use it as SYSTEM_RAM for 2nd kernel.

> 
> > 
> > The several memory ranges are twisted and somehow the funcions are duplicate.
> 
> These functions look like similar but each does serve for different purpose.
> 
> > 
> > So how about just keep one memory ranges array which contains all the ranges which
> > include system_ram, acpi, acpi_nvs, crash_reserved range.
> > 
> > In the crashdump-elf.c the function for creating elf headers will check the
> > range type, it will just skip the range which is not ram.
> 
> We don't build EFL headers for crash_reserved ranges. So we can't store
> system ram and crash_reserved together before building EFL header.

There's already below code which ignore the ram ranges..
        for (i = 0; i < ranges; i++, range++) {
		if (range->type != RANGE_RAM)
			continue;

Thanks
Dave

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

Re: [PATCH v4 2/4] x86: Store memory ranges globally used for crash kernel to boot into

[Dave Young]

On 03/28/14 at 02:43pm, Dave Young wrote:
> On 03/28/14 at 02:13pm, WANG Chao wrote:
> > On 03/28/14 at 11:24am, Dave Young wrote:
> > > >  
> > > > +static void exclude_ram(struct memory_range *mr, int *nr_mr)
> > > > +{
> > > > +	int ranges, i, j, m;
> > > > +
> > > > +	ranges = *nr_mr;
> > > > +	for (i = 0, j = 0; i < ranges; i++) {
> > > > +		if (mr[j].type == RANGE_RAM) {
> > > > +			dbgprintf("Remove RAM %016llx-%016llxx: (%d)\n", mr[j].start, mr[j].end, mr[j].type);
> > > > +			for (m = j; m < *nr_mr; m++)
> > > > +				mr[m] = mr[m+1];
> > > > +			(*nr_mr)--;
> > > > +		} else {
> > > > +			j++;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	dbgprint_mem_range("After remove RAM", mr, *nr_mr);
> > > > +}
> > > 
> > > This is probably not necessary, what I understand you are doing is below:
> > > 
> > > get_crash_memory_ranges()
> > >  -> collect all SYSTEM_RAM, ACPI, ACPI_NVS ranges, exclude crash reserved ranges.
> > >  -> the system ram ranges are used to create elf header
> > >  -> the ACPI, ACPI_NVS ranges are used by cmdline_add_memmap_acpi etc.
> > 
> > Yes.
> > 
> > > 
> > > memmap_p
> > >  -> contains all the crash reserved ranges
> > >  -> to be used by cmdline_add_memmap
> > 
> > There's no memmap_p. I'll reuse crash_memory_ranges structure to store
> > crash reserved ranges, ACPI and ACPI_NVS ranges. So after building ELF
> > headers for 1st kernel memory ranges, all I have to do is exclude the
> > SYSTEM_RAM and add crash_reserved to crash_memory_ranges. And then
> > crash_memory_ranges can be used as 2nd kernel memory ranges.
> 
> How about do nothing and directly use the mem_ranges:
>  * skip RANGE_RAM, only add the range which is not RANGE_RAM
>  * if the range is RANGE_CRASH_KERNEL (introduce a new type?) then use it as SYSTEM_RAM for 2nd kernel.
> 
> > 
> > > 
> > > The several memory ranges are twisted and somehow the funcions are duplicate.
> > 
> > These functions look like similar but each does serve for different purpose.
> > 
> > > 
> > > So how about just keep one memory ranges array which contains all the ranges which
> > > include system_ram, acpi, acpi_nvs, crash_reserved range.
> > > 
> > > In the crashdump-elf.c the function for creating elf headers will check the
> > > range type, it will just skip the range which is not ram.
> > 
> > We don't build EFL headers for crash_reserved ranges. So we can't store
> > system ram and crash_reserved together before building EFL header.
> 
> There's already below code which ignore the ram ranges..
>         for (i = 0; i < ranges; i++, range++) {
> 		if (range->type != RANGE_RAM)
> 			continue;

Hmm, this RANGE_RAM is converted from Crash Kernel ranges. so for what I suggested it need change
to != RANGE_CRASH_KERNEL

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

Re: [PATCH v4 2/4] x86: Store memory ranges globally used for crash kernel to boot into

[Dave Young]

> > > system ram and crash_reserved together before building EFL header.
> > 
> > There's already below code which ignore the ram ranges..
> >         for (i = 0; i < ranges; i++, range++) {
> > 		if (range->type != RANGE_RAM)
> > 			continue;
> 
> Hmm, this RANGE_RAM is converted from Crash Kernel ranges. so for what I suggested it need change
> to != RANGE_CRASH_KERNEL

They are the 1st kernel RANGE_RAM ranges, please ignore above reply, I confused myself :(

Thanks
Dave

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

Re: [PATCH v4 4/4] x86: Pass memory range via E820 for kdump

[Vivek Goyal]

On Fri, Mar 28, 2014 at 12:52:54PM +0800, WANG Chao wrote:

[..]
> > > +static void add_setup_data(struct kexec_info *info,
> > > +			   struct x86_linux_param_header *real_mode,
> > > +			   struct setup_data *sd)
> > > +{
> > 
> > What is setup_data? A little comment above function will make it easy
> > to read. Is it that list of elements which contains extra memory map
> > entries?
> 
> Not exactly. All extra memory maps (for SETUP_E820_EXT type) are
> sealed into a single setup_data structure. Different types of setup_data
> are linked in a list.
> 
> setup_data can be used to pass extra data for boot, for example EFI
> data (SETUP_EFI), extended E820 map (SETUP_E820_EXT), SETUP_PCI and
> SETUP_DTB. These types are defined when defining struct setup_data.
> 
> It's offically documented in Documentation/x86/boot.txt.
> 
> Field name:	setup_data
> Type:		write (special)
> Offset/size:	0x250/8
> Protocol:	2.09+
> 
>   The 64-bit physical pointer to NULL terminated single linked list of
>   struct setup_data. This is used to define a more extensible boot
>   parameters passing mechanism. The definition of struct setup_data
>   is as follow:
> 
>   struct setup_data {
> 	  u64 next;
> 	  u32 type;
> 	  u32 len;
> 	  u8  data[0];
>   };
> 
>   Where, the next is a 64-bit physical pointer to the next node of
>   linked list, the next field of the last node is 0; the type is used
>   to identify the contents of data; the len is the length of data
>   field; the data holds the real payload.
> 
>   This list may be modified at a number of points during the bootup
>   process. Therefore, when modifying this list one should always make
>   sure to consider the case where the linked list already contains
>   entries.
> 
> I think I would comment add_setup_data as follows:
> 
> /*
>  * Added another instance to single linked list of struct setup_data.
>  * Please refer to kernel Documentation/x86/boot.txt for more details
>  * about setup_data structure.
>  */

Thanks for the explanation. Above comment will help the reader.

Thanks
Vivek

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

Re: [PATCH v4 2/4] x86: Store memory ranges globally used for crash kernel to boot into

[Vivek Goyal]

On Fri, Mar 28, 2014 at 01:23:49PM +0800, WANG Chao wrote:

[..]
> > And even if we have to, then why do we need to return pointer to
> > crash_memory_range array?
> 
> For historical reason, get_crash_memory_ranges() is not only set
> crash_memory_range but also return the pointer of another copy of
> crash_memory_range:
> 
> static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
> 				   int kexec_flags, unsigned long lowmem_limit)
> {
> 	[..]
> 	crash_memory_range = xxx;
> 	[..]
> 	*range = crash_memory_range;
> 	*ranges = memory_range;
> }
> 

Current implementation if fine. I saves all entries in a static array and
then returns pointer to that static array so that others could use it. So
point seems to be that access this array using returned pointer and one
can't access it directly. There is no extra array copy.

Once we start storing crash array entries in kexec_info, we should not
have to do any cleanup here and be able to access crash entries array
everywhere.

Thanks
Vivek

> I was just trying to keep the change as minimal as possible, so that the
> reviewers can be more clear of what the patch does instead of something
> looks messed up. But if you have no problem review it, I can do some
> clean up within this patch. However I think it's better to be addressed
> the cleanup in the future, or at least as a separated patch in this
> series.
> 
> Thanks
> WANG Chao
> 
> > 
> > Thanks
> > Vivek
> > 
> > > 
> > > Signed-off-by: WANG Chao <chaowang@redhat.com>
> > > Tested-by: Linn Crosetto <linn@hp.com>
> > > ---
> > >  kexec/arch/i386/crashdump-x86.c | 134 ++++++++++++++++++++++------------------
> > >  kexec/arch/i386/crashdump-x86.h |   5 +-
> > >  2 files changed, 77 insertions(+), 62 deletions(-)
> > > 
> > > diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> > > index 979c2bd..c55a6b1 100644
> > > --- a/kexec/arch/i386/crashdump-x86.c
> > > +++ b/kexec/arch/i386/crashdump-x86.c
> > > @@ -179,7 +179,8 @@ static int exclude_region(int *nr_ranges, uint64_t start, uint64_t end);
> > >  
> > >  /* Stores a sorted list of RAM memory ranges for which to create elf headers.
> > >   * A separate program header is created for backup region */
> > > -static struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
> > > +struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
> > > +int crash_memory_ranges;
> > >  
> > >  /* Memory region reserved for storing panic kernel and other data. */
> > >  #define CRASH_RESERVED_MEM_NR	8
> > > @@ -201,7 +202,7 @@ static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
> > >  				   int kexec_flags, unsigned long lowmem_limit)
> > >  {
> > >  	const char *iomem = proc_iomem();
> > > -	int memory_ranges = 0, gart = 0, i;
> > > +	int gart = 0, i;
> > >  	char line[MAX_LINE];
> > >  	FILE *fp;
> > >  	unsigned long long start, end;
> > > @@ -218,7 +219,7 @@ static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
> > >  		char *str;
> > >  		int type, consumed, count;
> > >  
> > > -		if (memory_ranges >= CRASH_MAX_MEMORY_RANGES)
> > > +		if (crash_memory_ranges >= CRASH_MAX_MEMORY_RANGES)
> > >  			break;
> > >  		count = sscanf(line, "%Lx-%Lx : %n",
> > >  			&start, &end, &consumed);
> > > @@ -250,17 +251,17 @@ static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
> > >  			continue;
> > >  		}
> > >  
> > > -		crash_memory_range[memory_ranges].start = start;
> > > -		crash_memory_range[memory_ranges].end = end;
> > > -		crash_memory_range[memory_ranges].type = type;
> > > +		crash_memory_range[crash_memory_ranges].start = start;
> > > +		crash_memory_range[crash_memory_ranges].end = end;
> > > +		crash_memory_range[crash_memory_ranges].type = type;
> > >  
> > > -		segregate_lowmem_region(&memory_ranges, lowmem_limit);
> > > +		segregate_lowmem_region(&crash_memory_ranges, lowmem_limit);
> > >  
> > > -		memory_ranges++;
> > > +		crash_memory_ranges++;
> > >  	}
> > >  	fclose(fp);
> > >  	if (kexec_flags & KEXEC_PRESERVE_CONTEXT) {
> > > -		for (i = 0; i < memory_ranges; i++) {
> > > +		for (i = 0; i < crash_memory_ranges; i++) {
> > >  			if (crash_memory_range[i].end > 0x0009ffff) {
> > >  				crash_reserved_mem[0].start = \
> > >  					crash_memory_range[i].start;
> > > @@ -278,17 +279,17 @@ static int get_crash_memory_ranges(struct memory_range **range, int *ranges,
> > >  	}
> > >  
> > >  	for (i = 0; i < crash_reserved_mem_nr; i++)
> > > -		if (exclude_region(&memory_ranges, crash_reserved_mem[i].start,
> > > +		if (exclude_region(&crash_memory_ranges, crash_reserved_mem[i].start,
> > >  				crash_reserved_mem[i].end) < 0)
> > >  			return -1;
> > >  
> > >  	if (gart) {
> > >  		/* exclude GART region if the system has one */
> > > -		if (exclude_region(&memory_ranges, gart_start, gart_end) < 0)
> > > +		if (exclude_region(&crash_memory_ranges, gart_start, gart_end) < 0)
> > >  			return -1;
> > >  	}
> > >  	*range = crash_memory_range;
> > > -	*ranges = memory_ranges;
> > > +	*ranges = crash_memory_ranges;
> > >  
> > >  	return 0;
> > >  }
> > > @@ -324,7 +325,7 @@ static int get_crash_memory_ranges_xen(struct memory_range **range,
> > >  	}
> > >  
> > >  	*range = crash_memory_range;
> > > -	*ranges = j;
> > > +	*ranges = crash_memory_ranges = j;
> > >  
> > >  	qsort(*range, *ranges, sizeof(struct memory_range), compare_ranges);
> > >  
> > > @@ -417,8 +418,8 @@ static int exclude_region(int *nr_ranges, uint64_t start, uint64_t end)
> > >  
> > >  /* Adds a segment from list of memory regions which new kernel can use to
> > >   * boot. Segment start and end should be aligned to 1K boundary. */
> > > -static int add_memmap(struct memory_range *memmap_p, unsigned long long addr,
> > > -								size_t size)
> > > +static int add_memmap(struct memory_range *memmap_p, int *nr_range,
> > > +					unsigned long long addr, size_t size)
> > >  {
> > >  	int i, j, nr_entries = 0, tidx = 0, align = 1024;
> > >  	unsigned long long mstart, mend;
> > > @@ -450,29 +451,23 @@ static int add_memmap(struct memory_range *memmap_p, unsigned long long addr,
> > >  		else if (addr > mend)
> > >  			tidx = i+1;
> > >  	}
> > > -		/* Insert the memory region. */
> > > -		for (j = nr_entries-1; j >= tidx; j--)
> > > -			memmap_p[j+1] = memmap_p[j];
> > > -		memmap_p[tidx].start = addr;
> > > -		memmap_p[tidx].end = addr + size - 1;
> > > +	/* Insert the memory region. */
> > > +	for (j = nr_entries-1; j >= tidx; j--)
> > > +		memmap_p[j+1] = memmap_p[j];
> > > +	memmap_p[tidx].start = addr;
> > > +	memmap_p[tidx].end = addr + size - 1;
> > > +	memmap_p[tidx].type = RANGE_RAM;
> > > +	*nr_range = nr_entries + 1;
> > >  
> > > -	dbgprintf("Memmap after adding segment\n");
> > > -	for (i = 0; i < CRASH_MAX_MEMMAP_NR;  i++) {
> > > -		mstart = memmap_p[i].start;
> > > -		mend = memmap_p[i].end;
> > > -		if (mstart == 0 && mend == 0)
> > > -			break;
> > > -		dbgprintf("%016llx - %016llx\n",
> > > -			mstart, mend);
> > > -	}
> > > +	dbgprint_mem_range("Memmap after adding segment", memmap_p, *nr_range);
> > >  
> > >  	return 0;
> > >  }
> > >  
> > >  /* Removes a segment from list of memory regions which new kernel can use to
> > >   * boot. Segment start and end should be aligned to 1K boundary. */
> > > -static int delete_memmap(struct memory_range *memmap_p, unsigned long long addr,
> > > -								size_t size)
> > > +static int delete_memmap(struct memory_range *memmap_p, int *nr_range,
> > > +					unsigned long long addr, size_t size)
> > >  {
> > >  	int i, j, nr_entries = 0, tidx = -1, operation = 0, align = 1024;
> > >  	unsigned long long mstart, mend;
> > > @@ -534,24 +529,17 @@ static int delete_memmap(struct memory_range *memmap_p, unsigned long long addr,
> > >  		for (j = nr_entries-1; j > tidx; j--)
> > >  			memmap_p[j+1] = memmap_p[j];
> > >  		memmap_p[tidx+1] = temp_region;
> > > +		*nr_range = nr_entries + 1;
> > >  	}
> > >  	if ((operation == -1) && tidx >=0) {
> > >  		/* Delete the exact match memory region. */
> > >  		for (j = i+1; j < CRASH_MAX_MEMMAP_NR; j++)
> > >  			memmap_p[j-1] = memmap_p[j];
> > >  		memmap_p[j-1].start = memmap_p[j-1].end = 0;
> > > +		*nr_range = nr_entries - 1;
> > >  	}
> > >  
> > > -	dbgprintf("Memmap after deleting segment\n");
> > > -	for (i = 0; i < CRASH_MAX_MEMMAP_NR;  i++) {
> > > -		mstart = memmap_p[i].start;
> > > -		mend = memmap_p[i].end;
> > > -		if (mstart == 0 && mend == 0) {
> > > -			break;
> > > -		}
> > > -		dbgprintf("%016llx - %016llx\n",
> > > -			mstart, mend);
> > > -	}
> > > +	dbgprint_mem_range("Memmap after deleting segment", memmap_p, *nr_range);
> > >  
> > >  	return 0;
> > >  }
> > > @@ -626,6 +614,9 @@ static int cmdline_add_memmap(char *cmdline, struct memory_range *memmap_p)
> > >  			/* All regions traversed. */
> > >  			break;
> > >  
> > > +		if (memmap_p[i].type != RANGE_RAM)
> > > +			continue;
> > > +
> > >  		/* A region is not worth adding if region size < 100K. It eats
> > >  		 * up precious command line length. */
> > >  		if ((endk - startk) < min_sizek)
> > > @@ -797,6 +788,25 @@ static void get_backup_area(struct kexec_info *info,
> > >  	info->backup_src_size = BACKUP_SRC_END - BACKUP_SRC_START + 1;
> > >  }
> > >  
> > > +static void exclude_ram(struct memory_range *mr, int *nr_mr)
> > > +{
> > > +	int ranges, i, j, m;
> > > +
> > > +	ranges = *nr_mr;
> > > +	for (i = 0, j = 0; i < ranges; i++) {
> > > +		if (mr[j].type == RANGE_RAM) {
> > > +			dbgprintf("Remove RAM %016llx-%016llxx: (%d)\n", mr[j].start, mr[j].end, mr[j].type);
> > > +			for (m = j; m < *nr_mr; m++)
> > > +				mr[m] = mr[m+1];
> > > +			(*nr_mr)--;
> > > +		} else {
> > > +			j++;
> > > +		}
> > > +	}
> > > +
> > > +	dbgprint_mem_range("After remove RAM", mr, *nr_mr);
> > > +}
> > > +
> > >  /* Loads additional segments in case of a panic kernel is being loaded.
> > >   * One segment for backup region, another segment for storing elf headers
> > >   * for crash memory image.
> > > @@ -807,7 +817,7 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > >  	void *tmp;
> > >  	unsigned long sz, bufsz, memsz, elfcorehdr;
> > >  	int nr_ranges = 0, align = 1024, i;
> > > -	struct memory_range *mem_range, *memmap_p;
> > > +	struct memory_range *mem_range;
> > >  	struct crash_elf_info elf_info;
> > >  	unsigned kexec_arch;
> > >  
> > > @@ -850,10 +860,7 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > >  
> > >  	get_backup_area(info, mem_range, nr_ranges);
> > >  
> > > -	dbgprintf("CRASH MEMORY RANGES\n");
> > > -
> > > -	for(i = 0; i < nr_ranges; ++i)
> > > -		dbgprintf("%016Lx-%016Lx\n", mem_range[i].start, mem_range[i].end);
> > > +	dbgprint_mem_range("CRASH MEMORY RANGES", mem_range, nr_ranges);
> > >  
> > >  	/*
> > >  	 * if the core type has not been set on command line, set it here
> > > @@ -878,17 +885,6 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > >  	if (get_kernel_vaddr_and_size(info, &elf_info))
> > >  		return -1;
> > >  
> > > -	/* Memory regions which panic kernel can safely use to boot into */
> > > -	sz = (sizeof(struct memory_range) * CRASH_MAX_MEMMAP_NR);
> > > -	memmap_p = xmalloc(sz);
> > > -	memset(memmap_p, 0, sz);
> > > -	add_memmap(memmap_p, info->backup_src_start, info->backup_src_size);
> > > -	for (i = 0; i < crash_reserved_mem_nr; i++) {
> > > -		sz = crash_reserved_mem[i].end - crash_reserved_mem[i].start +1;
> > > -		if (add_memmap(memmap_p, crash_reserved_mem[i].start, sz) < 0)
> > > -			return ENOCRASHKERNEL;
> > > -	}
> > > -
> > >  	/* Create a backup region segment to store backup data*/
> > >  	if (!(info->kexec_flags & KEXEC_PRESERVE_CONTEXT)) {
> > >  		sz = _ALIGN(info->backup_src_size, align);
> > > @@ -898,8 +894,6 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > >  						0, max_addr, -1);
> > >  		dbgprintf("Created backup segment at 0x%lx\n",
> > >  			  info->backup_start);
> > > -		if (delete_memmap(memmap_p, info->backup_start, sz) < 0)
> > > -			return EFAILED;
> > >  	}
> > >  
> > >  	/* Create elf header segment and store crash image data. */
> > > @@ -915,6 +909,23 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > >  						ELF_CORE_HEADER_ALIGN) < 0)
> > >  			return EFAILED;
> > >  	}
> > > +
> > > +	/* Memory regions which panic kernel can safely use to boot into */
> > > +	exclude_ram(crash_memory_range, &crash_memory_ranges);
> > > +
> > > +	add_memmap(crash_memory_range, &crash_memory_ranges, info->backup_src_start, info->backup_src_size);
> > > +	for (i = 0; i < crash_reserved_mem_nr; i++) {
> > > +		sz = crash_reserved_mem[i].end - crash_reserved_mem[i].start +1;
> > > +		if (add_memmap(crash_memory_range, &crash_memory_ranges, crash_reserved_mem[i].start, sz) < 0)
> > > +			return ENOCRASHKERNEL;
> > > +	}
> > > +
> > > +	/* exclude backup region from crash dump memory range */
> > > +	sz = _ALIGN(info->backup_src_size, align);
> > > +	if (delete_memmap(crash_memory_range, &crash_memory_ranges, info->backup_start, sz) < 0) {
> > > +		return EFAILED;
> > > +	}
> > > +
> > >  	/* the size of the elf headers allocated is returned in 'bufsz' */
> > >  
> > >  	/* Hack: With some ld versions (GNU ld version 2.14.90.0.4 20030523),
> > > @@ -934,9 +945,9 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > >  	elfcorehdr = add_buffer(info, tmp, bufsz, memsz, align, min_base,
> > >  							max_addr, -1);
> > >  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
> > > -	if (delete_memmap(memmap_p, elfcorehdr, memsz) < 0)
> > > +	if (delete_memmap(crash_memory_range, &crash_memory_ranges, elfcorehdr, memsz) < 0)
> > >  		return -1;
> > > -	cmdline_add_memmap(mod_cmdline, memmap_p);
> > > +	cmdline_add_memmap(mod_cmdline, crash_memory_range);
> > >  	if (!bzImage_support_efi_boot)
> > >  		cmdline_add_efi(mod_cmdline);
> > >  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
> > > @@ -951,6 +962,7 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > >  		end = mem_range[i].end;
> > >  		cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > >  	}
> > > +
> > >  	return 0;
> > >  }
> > >  
> > > diff --git a/kexec/arch/i386/crashdump-x86.h b/kexec/arch/i386/crashdump-x86.h
> > > index b61cf0a..633ee0e 100644
> > > --- a/kexec/arch/i386/crashdump-x86.h
> > > +++ b/kexec/arch/i386/crashdump-x86.h
> > > @@ -20,7 +20,7 @@ int load_crashdump_segments(struct kexec_info *info, char *mod_cmdline,
> > >  /* Kernel text size */
> > >  #define X86_64_KERNEL_TEXT_SIZE  (512UL*1024*1024)
> > >  
> > > -#define CRASH_MAX_MEMMAP_NR	(KEXEC_MAX_SEGMENTS + 1)
> > > +#define CRASH_MAX_MEMMAP_NR	CRASH_MAX_MEMORY_RANGES
> > >  #define CRASH_MAX_MEMORY_RANGES	(MAX_MEMORY_RANGES + 2)
> > >  
> > >  /* Backup Region, First 640K of System RAM. */
> > > @@ -28,4 +28,7 @@ int load_crashdump_segments(struct kexec_info *info, char *mod_cmdline,
> > >  #define BACKUP_SRC_END		0x0009ffff
> > >  #define BACKUP_SRC_SIZE	(BACKUP_SRC_END - BACKUP_SRC_START + 1)
> > >  
> > > +extern struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
> > > +extern int crash_memory_ranges;
> > > +
> > >  #endif /* CRASHDUMP_X86_H */
> > > -- 
> > > 1.8.5.3
> > > 
> > > 
> > > _______________________________________________
> > > kexec mailing list
> > > kexec@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/kexec

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

Re: [PATCH v4 2/4] x86: Store memory ranges globally used for crash kernel to boot into

[Thomas Renninger]

On Friday, March 28, 2014 01:23:49 PM WANG Chao wrote:
> On 03/27/14 at 06:32pm, Vivek Goyal wrote:
...

> I was just trying to keep the change as minimal as possible, so that the
> reviewers can be more clear of what the patch does instead of something
> looks messed up.
Sounds very sane. I tried it the other way around:
clean up and then do the functional change
and I ended up in a mess changing back and forth and I had to move on
to other stuff in the end.

> But if you have no problem review it, I can do some
> clean up within this patch. However I think it's better to be addressed
> the cleanup in the future, or at least as a separated patch in this
> series.
Seeing some cleanups, especially getting rid of the duplicate code to get 
memory ranges in kdump and kexec case (which I expect still exists?) on top 
later would be great.

     Thomas


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

Re: [PATCH v4 2/4] x86: Store memory ranges globally used for crash kernel to boot into

[Vivek Goyal]

On Fri, Mar 28, 2014 at 04:44:33PM +0100, Thomas Renninger wrote:

[..]
> > But if you have no problem review it, I can do some
> > clean up within this patch. However I think it's better to be addressed
> > the cleanup in the future, or at least as a separated patch in this
> > series.
> Seeing some cleanups, especially getting rid of the duplicate code to get 
> memory ranges in kdump and kexec case (which I expect still exists?) on top 
> later would be great.

I think let us first get this patch series in. It is important for kaslr
as well as making sure calgary IOMMU works in new kernels. 

And then there can be a patch series on top for cleanup stuff.

Thanks
Vivek

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

Re: [PATCH v4 2/4] x86: Store memory ranges globally used for crash kernel to boot into

[WANG Chao]

On 03/28/14 at 02:43pm, Dave Young wrote:
> On 03/28/14 at 02:13pm, WANG Chao wrote:
> > On 03/28/14 at 11:24am, Dave Young wrote:
> > > >  
> > > > +static void exclude_ram(struct memory_range *mr, int *nr_mr)
> > > > +{
> > > > +	int ranges, i, j, m;
> > > > +
> > > > +	ranges = *nr_mr;
> > > > +	for (i = 0, j = 0; i < ranges; i++) {
> > > > +		if (mr[j].type == RANGE_RAM) {
> > > > +			dbgprintf("Remove RAM %016llx-%016llxx: (%d)\n", mr[j].start, mr[j].end, mr[j].type);
> > > > +			for (m = j; m < *nr_mr; m++)
> > > > +				mr[m] = mr[m+1];
> > > > +			(*nr_mr)--;
> > > > +		} else {
> > > > +			j++;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	dbgprint_mem_range("After remove RAM", mr, *nr_mr);
> > > > +}
> > > 
> > > This is probably not necessary, what I understand you are doing is below:
> > > 
> > > get_crash_memory_ranges()
> > >  -> collect all SYSTEM_RAM, ACPI, ACPI_NVS ranges, exclude crash reserved ranges.
> > >  -> the system ram ranges are used to create elf header
> > >  -> the ACPI, ACPI_NVS ranges are used by cmdline_add_memmap_acpi etc.
> > 
> > Yes.
> > 
> > > 
> > > memmap_p
> > >  -> contains all the crash reserved ranges
> > >  -> to be used by cmdline_add_memmap
> > 
> > There's no memmap_p. I'll reuse crash_memory_ranges structure to store
> > crash reserved ranges, ACPI and ACPI_NVS ranges. So after building ELF
> > headers for 1st kernel memory ranges, all I have to do is exclude the
> > SYSTEM_RAM and add crash_reserved to crash_memory_ranges. And then
> > crash_memory_ranges can be used as 2nd kernel memory ranges.
> 
> How about do nothing and directly use the mem_ranges:
>  * skip RANGE_RAM, only add the range which is not RANGE_RAM
>  * if the range is RANGE_CRASH_KERNEL (introduce a new type?) then use it as SYSTEM_RAM for 2nd kernel.

I prefer not do this change in this patchset. Since current
implementation is fine, it looks more like a cleanup to me and we can do
that later.

Thanks
WANG Chao

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

Re: [PATCH v4 2/4] x86: Store memory ranges globally used for crash kernel to boot into

[Dave Young]

On 04/01/14 at 03:04pm, WANG Chao wrote:
> On 03/28/14 at 02:43pm, Dave Young wrote:
> > On 03/28/14 at 02:13pm, WANG Chao wrote:
> > > On 03/28/14 at 11:24am, Dave Young wrote:
> > > > >  
> > > > > +static void exclude_ram(struct memory_range *mr, int *nr_mr)
> > > > > +{
> > > > > +	int ranges, i, j, m;
> > > > > +
> > > > > +	ranges = *nr_mr;
> > > > > +	for (i = 0, j = 0; i < ranges; i++) {
> > > > > +		if (mr[j].type == RANGE_RAM) {
> > > > > +			dbgprintf("Remove RAM %016llx-%016llxx: (%d)\n", mr[j].start, mr[j].end, mr[j].type);
> > > > > +			for (m = j; m < *nr_mr; m++)
> > > > > +				mr[m] = mr[m+1];
> > > > > +			(*nr_mr)--;
> > > > > +		} else {
> > > > > +			j++;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	dbgprint_mem_range("After remove RAM", mr, *nr_mr);
> > > > > +}
> > > > 
> > > > This is probably not necessary, what I understand you are doing is below:
> > > > 
> > > > get_crash_memory_ranges()
> > > >  -> collect all SYSTEM_RAM, ACPI, ACPI_NVS ranges, exclude crash reserved ranges.
> > > >  -> the system ram ranges are used to create elf header
> > > >  -> the ACPI, ACPI_NVS ranges are used by cmdline_add_memmap_acpi etc.
> > > 
> > > Yes.
> > > 
> > > > 
> > > > memmap_p
> > > >  -> contains all the crash reserved ranges
> > > >  -> to be used by cmdline_add_memmap
> > > 
> > > There's no memmap_p. I'll reuse crash_memory_ranges structure to store
> > > crash reserved ranges, ACPI and ACPI_NVS ranges. So after building ELF
> > > headers for 1st kernel memory ranges, all I have to do is exclude the
> > > SYSTEM_RAM and add crash_reserved to crash_memory_ranges. And then
> > > crash_memory_ranges can be used as 2nd kernel memory ranges.
> > 
> > How about do nothing and directly use the mem_ranges:
> >  * skip RANGE_RAM, only add the range which is not RANGE_RAM
> >  * if the range is RANGE_CRASH_KERNEL (introduce a new type?) then use it as SYSTEM_RAM for 2nd kernel.
> 
> I prefer not do this change in this patchset. Since current
> implementation is fine, it looks more like a cleanup to me and we can do
> that later.

Ok, that's fine, but I'm still not keen about exclude_ram. Could you manage
to drop this function? 

Thanks
Dave

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

Re: [PATCH v4 2/4] x86: Store memory ranges globally used for crash kernel to boot into

[WANG Chao]

On 04/01/14 at 04:41pm, Dave Young wrote:
> On 04/01/14 at 03:04pm, WANG Chao wrote:
> > On 03/28/14 at 02:43pm, Dave Young wrote:
> > > On 03/28/14 at 02:13pm, WANG Chao wrote:
> > > > On 03/28/14 at 11:24am, Dave Young wrote:
> > > > > >  
> > > > > > +static void exclude_ram(struct memory_range *mr, int *nr_mr)
> > > > > > +{
> > > > > > +	int ranges, i, j, m;
> > > > > > +
> > > > > > +	ranges = *nr_mr;
> > > > > > +	for (i = 0, j = 0; i < ranges; i++) {
> > > > > > +		if (mr[j].type == RANGE_RAM) {
> > > > > > +			dbgprintf("Remove RAM %016llx-%016llxx: (%d)\n", mr[j].start, mr[j].end, mr[j].type);
> > > > > > +			for (m = j; m < *nr_mr; m++)
> > > > > > +				mr[m] = mr[m+1];
> > > > > > +			(*nr_mr)--;
> > > > > > +		} else {
> > > > > > +			j++;
> > > > > > +		}
> > > > > > +	}
> > > > > > +
> > > > > > +	dbgprint_mem_range("After remove RAM", mr, *nr_mr);
> > > > > > +}
> > > > > 
> > > > > This is probably not necessary, what I understand you are doing is below:
> > > > > 
> > > > > get_crash_memory_ranges()
> > > > >  -> collect all SYSTEM_RAM, ACPI, ACPI_NVS ranges, exclude crash reserved ranges.
> > > > >  -> the system ram ranges are used to create elf header
> > > > >  -> the ACPI, ACPI_NVS ranges are used by cmdline_add_memmap_acpi etc.
> > > > 
> > > > Yes.
> > > > 
> > > > > 
> > > > > memmap_p
> > > > >  -> contains all the crash reserved ranges
> > > > >  -> to be used by cmdline_add_memmap
> > > > 
> > > > There's no memmap_p. I'll reuse crash_memory_ranges structure to store
> > > > crash reserved ranges, ACPI and ACPI_NVS ranges. So after building ELF
> > > > headers for 1st kernel memory ranges, all I have to do is exclude the
> > > > SYSTEM_RAM and add crash_reserved to crash_memory_ranges. And then
> > > > crash_memory_ranges can be used as 2nd kernel memory ranges.
> > > 
> > > How about do nothing and directly use the mem_ranges:
> > >  * skip RANGE_RAM, only add the range which is not RANGE_RAM
> > >  * if the range is RANGE_CRASH_KERNEL (introduce a new type?) then use it as SYSTEM_RAM for 2nd kernel.
> > 
> > I prefer not do this change in this patchset. Since current
> > implementation is fine, it looks more like a cleanup to me and we can do
> > that later.
> 
> Ok, that's fine, but I'm still not keen about exclude_ram. Could you manage
> to drop this function? 

exclude_ram() is just a static function that my code would use and I
certainly need a function like that this time. So why are you still keen
about dropping it :(

Thanks
WANG Chao

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

Re: [PATCH v4 2/4] x86: Store memory ranges globally used for crash kernel to boot into

[Dave Young]

On 04/01/14 at 04:54pm, WANG Chao wrote:
> On 04/01/14 at 04:41pm, Dave Young wrote:
> > On 04/01/14 at 03:04pm, WANG Chao wrote:
> > > On 03/28/14 at 02:43pm, Dave Young wrote:
> > > > On 03/28/14 at 02:13pm, WANG Chao wrote:
> > > > > On 03/28/14 at 11:24am, Dave Young wrote:
> > > > > > >  
> > > > > > > +static void exclude_ram(struct memory_range *mr, int *nr_mr)
> > > > > > > +{
> > > > > > > +	int ranges, i, j, m;
> > > > > > > +
> > > > > > > +	ranges = *nr_mr;
> > > > > > > +	for (i = 0, j = 0; i < ranges; i++) {
> > > > > > > +		if (mr[j].type == RANGE_RAM) {
> > > > > > > +			dbgprintf("Remove RAM %016llx-%016llxx: (%d)\n", mr[j].start, mr[j].end, mr[j].type);
> > > > > > > +			for (m = j; m < *nr_mr; m++)
> > > > > > > +				mr[m] = mr[m+1];
> > > > > > > +			(*nr_mr)--;
> > > > > > > +		} else {
> > > > > > > +			j++;
> > > > > > > +		}
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	dbgprint_mem_range("After remove RAM", mr, *nr_mr);
> > > > > > > +}
> > > > > > 
> > > > > > This is probably not necessary, what I understand you are doing is below:
> > > > > > 
> > > > > > get_crash_memory_ranges()
> > > > > >  -> collect all SYSTEM_RAM, ACPI, ACPI_NVS ranges, exclude crash reserved ranges.
> > > > > >  -> the system ram ranges are used to create elf header
> > > > > >  -> the ACPI, ACPI_NVS ranges are used by cmdline_add_memmap_acpi etc.
> > > > > 
> > > > > Yes.
> > > > > 
> > > > > > 
> > > > > > memmap_p
> > > > > >  -> contains all the crash reserved ranges
> > > > > >  -> to be used by cmdline_add_memmap
> > > > > 
> > > > > There's no memmap_p. I'll reuse crash_memory_ranges structure to store
> > > > > crash reserved ranges, ACPI and ACPI_NVS ranges. So after building ELF
> > > > > headers for 1st kernel memory ranges, all I have to do is exclude the
> > > > > SYSTEM_RAM and add crash_reserved to crash_memory_ranges. And then
> > > > > crash_memory_ranges can be used as 2nd kernel memory ranges.
> > > > 
> > > > How about do nothing and directly use the mem_ranges:
> > > >  * skip RANGE_RAM, only add the range which is not RANGE_RAM
> > > >  * if the range is RANGE_CRASH_KERNEL (introduce a new type?) then use it as SYSTEM_RAM for 2nd kernel.
> > > 
> > > I prefer not do this change in this patchset. Since current
> > > implementation is fine, it looks more like a cleanup to me and we can do
> > > that later.
> > 
> > Ok, that's fine, but I'm still not keen about exclude_ram. Could you manage
> > to drop this function? 
> 
> exclude_ram() is just a static function that my code would use and I
> certainly need a function like that this time. So why are you still keen
> about dropping it :(

Just suppose you agree with the future cleanup, you introduce it now and remove it
later it looks not good so I wonder if it's possible to drop it now and check the
range type instead.

If it's a *must* then I will not object

Thanks
Dave.

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

Re: [PATCH v4 2/4] x86: Store memory ranges globally used for crash kernel to boot into

[WANG Chao]

On 04/01/14 at 05:36pm, Dave Young wrote:
> On 04/01/14 at 04:54pm, WANG Chao wrote:
> > On 04/01/14 at 04:41pm, Dave Young wrote:
> > > On 04/01/14 at 03:04pm, WANG Chao wrote:
> > > > On 03/28/14 at 02:43pm, Dave Young wrote:
> > > > > On 03/28/14 at 02:13pm, WANG Chao wrote:
> > > > > > On 03/28/14 at 11:24am, Dave Young wrote:
> > > > > > > >  
> > > > > > > > +static void exclude_ram(struct memory_range *mr, int *nr_mr)
> > > > > > > > +{
> > > > > > > > +	int ranges, i, j, m;
> > > > > > > > +
> > > > > > > > +	ranges = *nr_mr;
> > > > > > > > +	for (i = 0, j = 0; i < ranges; i++) {
> > > > > > > > +		if (mr[j].type == RANGE_RAM) {
> > > > > > > > +			dbgprintf("Remove RAM %016llx-%016llxx: (%d)\n", mr[j].start, mr[j].end, mr[j].type);
> > > > > > > > +			for (m = j; m < *nr_mr; m++)
> > > > > > > > +				mr[m] = mr[m+1];
> > > > > > > > +			(*nr_mr)--;
> > > > > > > > +		} else {
> > > > > > > > +			j++;
> > > > > > > > +		}
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	dbgprint_mem_range("After remove RAM", mr, *nr_mr);
> > > > > > > > +}
> > > > > > > 
> > > > > > > This is probably not necessary, what I understand you are doing is below:
> > > > > > > 
> > > > > > > get_crash_memory_ranges()
> > > > > > >  -> collect all SYSTEM_RAM, ACPI, ACPI_NVS ranges, exclude crash reserved ranges.
> > > > > > >  -> the system ram ranges are used to create elf header
> > > > > > >  -> the ACPI, ACPI_NVS ranges are used by cmdline_add_memmap_acpi etc.
> > > > > > 
> > > > > > Yes.
> > > > > > 
> > > > > > > 
> > > > > > > memmap_p
> > > > > > >  -> contains all the crash reserved ranges
> > > > > > >  -> to be used by cmdline_add_memmap
> > > > > > 
> > > > > > There's no memmap_p. I'll reuse crash_memory_ranges structure to store
> > > > > > crash reserved ranges, ACPI and ACPI_NVS ranges. So after building ELF
> > > > > > headers for 1st kernel memory ranges, all I have to do is exclude the
> > > > > > SYSTEM_RAM and add crash_reserved to crash_memory_ranges. And then
> > > > > > crash_memory_ranges can be used as 2nd kernel memory ranges.
> > > > > 
> > > > > How about do nothing and directly use the mem_ranges:
> > > > >  * skip RANGE_RAM, only add the range which is not RANGE_RAM
> > > > >  * if the range is RANGE_CRASH_KERNEL (introduce a new type?) then use it as SYSTEM_RAM for 2nd kernel.
> > > > 
> > > > I prefer not do this change in this patchset. Since current
> > > > implementation is fine, it looks more like a cleanup to me and we can do
> > > > that later.
> > > 
> > > Ok, that's fine, but I'm still not keen about exclude_ram. Could you manage
> > > to drop this function? 
> > 
> > exclude_ram() is just a static function that my code would use and I
> > certainly need a function like that this time. So why are you still keen
> > about dropping it :(
> 
> Just suppose you agree with the future cleanup, you introduce it now and remove it
> later it looks not good so I wonder if it's possible to drop it now and check the
> range type instead.

The cleanup is another topic. What to clean up and how to clean up is
yet to be determined.

I personally agree with cleanup in the future, but it still depends on
many things and I can't make any promise that exclude_ram can be removed
in the future.

> 
> If it's a *must* then I will not object

OK. I'll use it, unless I figure out something else ...

Thanks for your suggestions!
WANG Chao

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