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

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

[WANG Chao]

Hi, All

This patchset enables passing memory map via E820 map on x86 platform instead
of memmap=exactmap. It's a better design and will solve the following problem
so far:

- kernel cmdline is limited resource and large machines tend to have many
    memory ranges that may excceed kernel cmdline limit size.
- kASLR doesn't work with memmap=exactmap, because kASLR happens early than
    user defined memmap=exactmap takes effect.

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.

This patchset contains massive updates from the previous one. I take some
suggestions from reviewers. I try to split the changes into smaller pieces and
keep the whole change as minimal as I can so it wouldn't be too harsh to review
the patch.

Any comment is appreciate!

v6->v5:
dyoung:
 - use nr_memmap instead of nr_memmap_p
 - .end inclusive
Simon:
 - more description on some patches

v5->v4:
Dave:
 - separate add_setup_data() to another patch.
Vivek:
 - adding comments for setup_data.
 - store crash memory range info golobally in kexec_info.
me:
 -remove dbgprint_mem_range defination, Simon has merged the patch.

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 (9):
  x86, cleanup: add extra arguments to add_memmap() and delete_memmap()
  x86, cleanup: add_memmap() only do alignment check on RANGE_RAM
  x86, cleanup: add other types of memory range for 2nd kernel boot to
    memmap_p
  x86, cleanup: use dbgprint_mem_range for memory range debugging
  x86, cleanup: increase CRASH_MAX_MEMMAP_NR up to
    CRASH_MAX_MEMORY_RANGES
  x86, cleanup: Store crash memory ranges kexec_info
  x86, cleanup: kexec memory range .end to be inclusive
  x86: add --pass-memmap-cmdline option
  x86: Pass memory range via E820 for kdump

 kexec/arch/i386/crashdump-x86.c        |  69 +++++++--------
 kexec/arch/i386/crashdump-x86.h        |   2 +-
 kexec/arch/i386/include/arch/options.h |   2 +
 kexec/arch/i386/kexec-x86-common.c     |   3 +-
 kexec/arch/i386/kexec-x86.c            |   4 +
 kexec/arch/i386/kexec-x86.h            |   1 +
 kexec/arch/i386/x86-linux-setup.c      | 149 ++++++++++++++++++++++-----------
 kexec/arch/i386/x86-linux-setup.h      |   1 +
 kexec/arch/x86_64/kexec-x86_64.c       |   5 ++
 kexec/firmware_memmap.c                |   1 -
 kexec/kexec.h                          |   2 +
 11 files changed, 147 insertions(+), 92 deletions(-)

-- 
1.9.0


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

[PATCH v6 1/9] x86, cleanup: add extra arguments to add_memmap() and delete_memmap()

[WANG Chao]

This change will be used later:

add_memmap(.., int *nr_memmap, .., int type);
delete_memmap(.., int *nr_memmap, ..);

memmap_p[] is statically allocated for a certain amount. It will be used
later when mapping these memory maps to e820 map.

It's convenient to keep track of the count of memmap_p (nr_memmap) in
add_memmap and delete_memmap, because the counting has already been
taken care of in these two functions.

The original add_memmap() can only add memory range of RANGE_RAM type.
For adding other types of memory range, add another argument for
indicating the type.

Signed-off-by: WANG Chao <chaowang@redhat.com>
---
 kexec/arch/i386/crashdump-x86.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
index 72bce0b..dfcce17 100644
--- a/kexec/arch/i386/crashdump-x86.c
+++ b/kexec/arch/i386/crashdump-x86.c
@@ -476,8 +476,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_memmap,
+			unsigned long long addr, size_t size, int type)
 {
 	int i, j, nr_entries = 0, tidx = 0, align = 1024;
 	unsigned long long mstart, mend;
@@ -514,6 +514,8 @@ static int add_memmap(struct memory_range *memmap_p, unsigned long long addr,
 		memmap_p[j+1] = memmap_p[j];
 	memmap_p[tidx].start = addr;
 	memmap_p[tidx].end = addr + size - 1;
+	memmap_p[tidx].type = type;
+	*nr_memmap = nr_entries + 1;
 
 	dbgprintf("Memmap after adding segment\n");
 	for (i = 0; i < CRASH_MAX_MEMMAP_NR;  i++) {
@@ -530,8 +532,8 @@ static int add_memmap(struct memory_range *memmap_p, unsigned long long addr,
 
 /* 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_memmap,
+				unsigned long long addr, size_t size)
 {
 	int i, j, nr_entries = 0, tidx = -1, operation = 0, align = 1024;
 	unsigned long long mstart, mend;
@@ -593,12 +595,14 @@ 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_memmap = 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_memmap = nr_entries - 1;
 	}
 
 	dbgprintf("Memmap after deleting segment\n");
@@ -865,7 +869,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;
+	int nr_ranges = 0, nr_memmap = 0, align = 1024, i;
 	struct memory_range *mem_range, *memmap_p;
 	struct crash_elf_info elf_info;
 	unsigned kexec_arch;
@@ -941,10 +945,10 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
 	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);
+	add_memmap(memmap_p, &nr_memmap, info->backup_src_start, info->backup_src_size, RANGE_RAM);
 	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)
+		if (add_memmap(memmap_p, &nr_memmap, crash_reserved_mem[i].start, sz, RANGE_RAM) < 0)
 			return ENOCRASHKERNEL;
 	}
 
@@ -957,7 +961,7 @@ 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)
+		if (delete_memmap(memmap_p, &nr_memmap, info->backup_start, sz) < 0)
 			return EFAILED;
 	}
 
@@ -993,7 +997,7 @@ 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(memmap_p, &nr_memmap, elfcorehdr, memsz) < 0)
 		return -1;
 	cmdline_add_memmap(mod_cmdline, memmap_p);
 	if (!bzImage_support_efi_boot)
-- 
1.9.0


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

[PATCH v6 2/9] x86, cleanup: add_memmap() only do alignment check on RANGE_RAM

[WANG Chao]

add_memmap() will also add memory range with type RANGE_ACPI and
RANGE_ACPI_NVS (RANGE_RESERVED in the future) besides RANGE_RAM to
memmap_p.

Among these types of memory range, only RANGE_RAM needs to
be aligned with certain alignment. RANGE_ACPI, RANGE_ACPI_NVS and
RANGE_RESERVED doesn't have to be aligned.

Signed-off-by: WANG Chao <chaowang@redhat.com>
---
 kexec/arch/i386/crashdump-x86.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
index dfcce17..6dd2e65 100644
--- a/kexec/arch/i386/crashdump-x86.c
+++ b/kexec/arch/i386/crashdump-x86.c
@@ -482,8 +482,8 @@ static int add_memmap(struct memory_range *memmap_p, int *nr_memmap,
 	int i, j, nr_entries = 0, tidx = 0, align = 1024;
 	unsigned long long mstart, mend;
 
-	/* Do alignment check. */
-	if ((addr%align) || (size%align))
+	/* Do alignment check if it's RANGE_RAM */
+	if ((type == RANGE_RAM) && ((addr%align) || (size%align)))
 		return -1;
 
 	/* Make sure at least one entry in list is free. */
-- 
1.9.0


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

[PATCH v6 3/9] x86, cleanup: add other types of memory range for 2nd kernel boot to memmap_p

[WANG Chao]

In load_crashdump_segments(), memmap_p[] is used to contain RANGE_RAM
memory range for booting 2nd kernel. Now adding types of RANGE_ACPI and
RANGE_ACPI_NVS to memmap_p, so later we can pass all the types of memory
range to 2nd kernel. These all types of memory ranges are all stored in
memmap_p for later reference.

Signed-off-by: WANG Chao <chaowang@redhat.com>
---
 kexec/arch/i386/crashdump-x86.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
index 6dd2e65..f97d79a 100644
--- a/kexec/arch/i386/crashdump-x86.c
+++ b/kexec/arch/i386/crashdump-x86.c
@@ -1006,12 +1006,15 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
 
 	/* Inform second kernel about the presence of ACPI tables. */
 	for (i = 0; i < CRASH_MAX_MEMORY_RANGES; i++) {
-		unsigned long start, end;
+		unsigned long start, end, size, type;
 		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;
+		type = mem_range[i].type;
+		size = end - start + 1;
+		add_memmap(memmap_p, &nr_memmap, start, size, type);
 		cmdline_add_memmap_acpi(mod_cmdline, start, end);
 	}
 	return 0;
-- 
1.9.0


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

[PATCH v6 4/9] x86, cleanup: use dbgprint_mem_range for memory range debugging

[WANG Chao]

Signed-off-by: WANG Chao <chaowang@redhat.com>
---
 kexec/arch/i386/crashdump-x86.c | 26 +++-----------------------
 1 file changed, 3 insertions(+), 23 deletions(-)

diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
index f97d79a..39c35ea 100644
--- a/kexec/arch/i386/crashdump-x86.c
+++ b/kexec/arch/i386/crashdump-x86.c
@@ -517,15 +517,7 @@ static int add_memmap(struct memory_range *memmap_p, int *nr_memmap,
 	memmap_p[tidx].type = type;
 	*nr_memmap = 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_memmap);
 
 	return 0;
 }
@@ -605,16 +597,7 @@ static int delete_memmap(struct memory_range *memmap_p, int *nr_memmap,
 		*nr_memmap = 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_memmap);
 
 	return 0;
 }
@@ -913,10 +896,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
-- 
1.9.0


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

[PATCH v6 5/9] x86, cleanup: increase CRASH_MAX_MEMMAP_NR up to CRASH_MAX_MEMORY_RANGES

[WANG Chao]

CRASH_MAX_MEMMAP_NR now is defined (KEXEC_MAX_SEGMENTS + 2) which is way
lower than the memmap we can pass to 2nd kernel.

Signed-off-by: WANG Chao <chaowang@redhat.com>
---
 kexec/arch/i386/crashdump-x86.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kexec/arch/i386/crashdump-x86.h b/kexec/arch/i386/crashdump-x86.h
index b61cf0a..0edeb27 100644
--- a/kexec/arch/i386/crashdump-x86.h
+++ b/kexec/arch/i386/crashdump-x86.h
@@ -20,8 +20,8 @@ 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_MEMORY_RANGES	(MAX_MEMORY_RANGES + 2)
+#define CRASH_MAX_MEMMAP_NR	CRASH_MAX_MEMORY_RANGES
 
 /* Backup Region, First 640K of System RAM. */
 #define BACKUP_SRC_START	0x00000000
-- 
1.9.0


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

[PATCH v6 6/9] x86, cleanup: Store crash memory ranges kexec_info

[WANG Chao]

Add two new members to kexec_info structure:

struct memory_range *crash_range
int nr_crash_ranges;

crash_range contains the memory ranges used to boot 2nd kernel.
nr_crash_ranges contains the count of the crash memory ranges.

Signed-off-by: WANG Chao <chaowang@redhat.com>
---
 kexec/arch/i386/crashdump-x86.c | 6 ++++++
 kexec/kexec.h                   | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
index 39c35ea..7b618a6 100644
--- a/kexec/arch/i386/crashdump-x86.c
+++ b/kexec/arch/i386/crashdump-x86.c
@@ -997,6 +997,12 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
 		add_memmap(memmap_p, &nr_memmap, start, size, type);
 		cmdline_add_memmap_acpi(mod_cmdline, start, end);
 	}
+
+	/* Store 2nd kernel boot memory ranges for later reference in
+	 * x86-setup-linux.c: setup_linux_system_parameters() */
+	info->crash_range = memmap_p;
+	info->nr_crash_ranges = nr_memmap;
+
 	return 0;
 }
 
diff --git a/kexec/kexec.h b/kexec/kexec.h
index d69bba2..22d4a42 100644
--- a/kexec/kexec.h
+++ b/kexec/kexec.h
@@ -148,6 +148,8 @@ struct kexec_info {
 	int nr_segments;
 	struct memory_range *memory_range;
 	int memory_ranges;
+	struct memory_range *crash_range;
+	int nr_crash_ranges;
 	void *entry;
 	struct mem_ehdr rhdr;
 	unsigned long backup_start;
-- 
1.9.0


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

[PATCH v6 7/9] x86, cleanup: kexec memory range .end to be inclusive

[WANG Chao]

Later kexec and kdump memory range will be mapped to E820entry. But
currently kexec memory range .end field is exclusive while crash memory
range is inclusive.

Given the fact that the exported proc iomem and sysfs memmap are both
inclusive, change kexec memory range .end to be inclusive. Later the
unified memory range of both kexec and kdump can use the same E820
filling code.

Signed-off-by: WANG Chao <chaowang@redhat.com>
---
 kexec/arch/i386/kexec-x86-common.c | 3 +--
 kexec/arch/i386/x86-linux-setup.c  | 2 +-
 kexec/firmware_memmap.c            | 1 -
 3 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/kexec/arch/i386/kexec-x86-common.c b/kexec/arch/i386/kexec-x86-common.c
index e416177..bc622e9 100644
--- a/kexec/arch/i386/kexec-x86-common.c
+++ b/kexec/arch/i386/kexec-x86-common.c
@@ -79,7 +79,6 @@ static int get_memory_ranges_proc_iomem(struct memory_range **range, int *ranges
 		if (count != 2)
 			continue;
 		str = line + consumed;
-		end = end + 1;
 
 		dbgprintf("%016Lx-%016Lx : %s", start, end, str);
 
@@ -188,7 +187,7 @@ static int get_memory_ranges_xen(struct memory_range **range, int *ranges)
 
 	for (i = 0; i < rc; ++i) {
 		memory_range[i].start = e820entries[i].addr;
-		memory_range[i].end = e820entries[i].addr + e820entries[i].size;
+		memory_range[i].end = e820entries[i].addr + e820entries[i].size - 1;
 		memory_range[i].type = xen_e820_to_kexec_type(e820entries[i].type);
 	}
 
diff --git a/kexec/arch/i386/x86-linux-setup.c b/kexec/arch/i386/x86-linux-setup.c
index 8ed36cc..9f8355f 100644
--- a/kexec/arch/i386/x86-linux-setup.c
+++ b/kexec/arch/i386/x86-linux-setup.c
@@ -784,7 +784,7 @@ void setup_linux_system_parameters(struct kexec_info *info,
 	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;
+		real_mode->e820_map[i].size = range[i].end - range[i].start + 1;
 		switch (range[i].type) {
 		case RANGE_RAM:
 			real_mode->e820_map[i].type = E820_RAM; 
diff --git a/kexec/firmware_memmap.c b/kexec/firmware_memmap.c
index 9598933..6be3c7c 100644
--- a/kexec/firmware_memmap.c
+++ b/kexec/firmware_memmap.c
@@ -145,7 +145,6 @@ static int parse_memmap_entry(const char *entry, struct memory_range *range)
 	range->end = parse_numeric_sysfs(filename);
 	if (range->end == ULLONG_MAX)
 		return -1;
-	range->end++; /* inclusive vs. exclusive ranges */
 
 	/*
 	 * entry/type
-- 
1.9.0


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

[PATCH v6 8/9] 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/x86_64/kexec-x86_64.c       | 5 +++++
 4 files changed, 12 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/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.9.0


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

[PATCH v6 9/9] 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. And also memmap=exactmap and
kASLR doesn't work together.

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   |   6 +-
 kexec/arch/i386/x86-linux-setup.c | 149 +++++++++++++++++++++++++-------------
 kexec/arch/i386/x86-linux-setup.h |   1 +
 3 files changed, 103 insertions(+), 53 deletions(-)

diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
index 7b618a6..4a1491b 100644
--- a/kexec/arch/i386/crashdump-x86.c
+++ b/kexec/arch/i386/crashdump-x86.c
@@ -979,7 +979,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
 	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
 	if (delete_memmap(memmap_p, &nr_memmap, elfcorehdr, memsz) < 0)
 		return -1;
-	cmdline_add_memmap(mod_cmdline, memmap_p);
+	if (arch_options.pass_memmap_cmdline)
+		cmdline_add_memmap(mod_cmdline, memmap_p);
 	if (!bzImage_support_efi_boot)
 		cmdline_add_efi(mod_cmdline);
 	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
@@ -995,7 +996,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
 		type = mem_range[i].type;
 		size = end - start + 1;
 		add_memmap(memmap_p, &nr_memmap, start, size, type);
-		cmdline_add_memmap_acpi(mod_cmdline, start, end);
+		if (arch_options.pass_memmap_cmdline)
+			cmdline_add_memmap_acpi(mod_cmdline, start, end);
 	}
 
 	/* Store 2nd kernel boot memory ranges for later reference in
diff --git a/kexec/arch/i386/x86-linux-setup.c b/kexec/arch/i386/x86-linux-setup.c
index 9f8355f..4437ed7 100644
--- a/kexec/arch/i386/x86-linux-setup.c
+++ b/kexec/arch/i386/x86-linux-setup.c
@@ -36,8 +36,6 @@
 #include "x86-linux-setup.h"
 #include "../../kexec/kexec-syscall.h"
 
-#define SETUP_EFI	4
-
 void init_linux_parameters(struct x86_linux_param_header *real_mode)
 {
 	/* Fill in the values that are usually provided by the kernel. */
@@ -502,6 +500,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));
@@ -684,6 +687,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 + 1;
+		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 && !arch_options.pass_memmap_cmdline) {
+		range = info->crash_range;
+		nr_range = info->nr_crash_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)
 {
@@ -725,10 +820,6 @@ out:
 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)
@@ -769,51 +860,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 + 1;
-		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);
diff --git a/kexec/arch/i386/x86-linux-setup.h b/kexec/arch/i386/x86-linux-setup.h
index 6fb84b4..f5d23d3 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 struct arch_options_t arch_options;
 
 #endif /* X86_LINUX_SETUP_H */
-- 
1.9.0


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

Re: [PATCH v6 9/9] x86: Pass memory range via E820 for kdump

[Dave Young]

On 04/14/14 at 10:55pm, WANG Chao wrote:
> command line size is restricted by kernel, sometimes memmap=exactmap has
> too many memory ranges to pass to cmdline. And also memmap=exactmap and
> kASLR doesn't work together.
> 
> 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   |   6 +-
>  kexec/arch/i386/x86-linux-setup.c | 149 +++++++++++++++++++++++++-------------
>  kexec/arch/i386/x86-linux-setup.h |   1 +
>  3 files changed, 103 insertions(+), 53 deletions(-)
> 
> diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> index 7b618a6..4a1491b 100644
> --- a/kexec/arch/i386/crashdump-x86.c
> +++ b/kexec/arch/i386/crashdump-x86.c
> @@ -979,7 +979,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
>  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
>  	if (delete_memmap(memmap_p, &nr_memmap, elfcorehdr, memsz) < 0)
>  		return -1;
> -	cmdline_add_memmap(mod_cmdline, memmap_p);
> +	if (arch_options.pass_memmap_cmdline)
> +		cmdline_add_memmap(mod_cmdline, memmap_p);
>  	if (!bzImage_support_efi_boot)
>  		cmdline_add_efi(mod_cmdline);
>  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
> @@ -995,7 +996,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
>  		type = mem_range[i].type;
>  		size = end - start + 1;
>  		add_memmap(memmap_p, &nr_memmap, start, size, type);
> -		cmdline_add_memmap_acpi(mod_cmdline, start, end);
> +		if (arch_options.pass_memmap_cmdline)
> +			cmdline_add_memmap_acpi(mod_cmdline, start, end);

Seems memmap_p contains the acpi ranges as well, so cmdline_add_memmap_acpi is
not necessary anymore, just improve cmdline_add_memmap to add both RAM and ACPI
ranges is enough.

Thanks
Dave

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

Re: [PATCH v6 9/9] x86: Pass memory range via E820 for kdump

[Dave Young]

On 04/17/14 at 01:29pm, Dave Young wrote:
> On 04/14/14 at 10:55pm, WANG Chao wrote:
> > command line size is restricted by kernel, sometimes memmap=exactmap has
> > too many memory ranges to pass to cmdline. And also memmap=exactmap and
> > kASLR doesn't work together.
> > 
> > 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   |   6 +-
> >  kexec/arch/i386/x86-linux-setup.c | 149 +++++++++++++++++++++++++-------------
> >  kexec/arch/i386/x86-linux-setup.h |   1 +
> >  3 files changed, 103 insertions(+), 53 deletions(-)
> > 
> > diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> > index 7b618a6..4a1491b 100644
> > --- a/kexec/arch/i386/crashdump-x86.c
> > +++ b/kexec/arch/i386/crashdump-x86.c
> > @@ -979,7 +979,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> >  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
> >  	if (delete_memmap(memmap_p, &nr_memmap, elfcorehdr, memsz) < 0)
> >  		return -1;
> > -	cmdline_add_memmap(mod_cmdline, memmap_p);
> > +	if (arch_options.pass_memmap_cmdline)
> > +		cmdline_add_memmap(mod_cmdline, memmap_p);
> >  	if (!bzImage_support_efi_boot)
> >  		cmdline_add_efi(mod_cmdline);
> >  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
> > @@ -995,7 +996,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> >  		type = mem_range[i].type;
> >  		size = end - start + 1;
> >  		add_memmap(memmap_p, &nr_memmap, start, size, type);
> > -		cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > +		if (arch_options.pass_memmap_cmdline)
> > +			cmdline_add_memmap_acpi(mod_cmdline, start, end);
> 
> Seems memmap_p contains the acpi ranges as well, so cmdline_add_memmap_acpi is
> not necessary anymore, just improve cmdline_add_memmap to add both RAM and ACPI
> ranges is enough.

CRASH_MAX_MEMMAP_NR is used in cmdline_add_memmap, it does not include the acpi
ranges, it looks strange, but anyway there's checking about cmdline overflow, tos
I think maybe this CRASH_MAX_MEMMAP_NR can be dropped.

> 
> Thanks
> Dave
> 
> _______________________________________________
> 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 v6 0/9] kexec-tools, x86: E820 memmap pass for kdump

[Dave Young]

On 04/14/14 at 10:55pm, WANG Chao wrote:
> Hi, All
> 
> This patchset enables passing memory map via E820 map on x86 platform instead
> of memmap=exactmap. It's a better design and will solve the following problem
> so far:
> 
> - kernel cmdline is limited resource and large machines tend to have many
>     memory ranges that may excceed kernel cmdline limit size.
> - kASLR doesn't work with memmap=exactmap, because kASLR happens early than
>     user defined memmap=exactmap takes effect.
> 
> 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.
> 
> This patchset contains massive updates from the previous one. I take some
> suggestions from reviewers. I try to split the changes into smaller pieces and
> keep the whole change as minimal as I can so it wouldn't be too harsh to review
> the patch.
> 
> Any comment is appreciate!
> 
> v6->v5:
> dyoung:
>  - use nr_memmap instead of nr_memmap_p
>  - .end inclusive
> Simon:
>  - more description on some patches
> 
> v5->v4:
> Dave:
>  - separate add_setup_data() to another patch.
> Vivek:
>  - adding comments for setup_data.
>  - store crash memory range info golobally in kexec_info.
> me:
>  -remove dbgprint_mem_range defination, Simon has merged the patch.
> 
> 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 (9):
>   x86, cleanup: add extra arguments to add_memmap() and delete_memmap()
>   x86, cleanup: add_memmap() only do alignment check on RANGE_RAM
>   x86, cleanup: add other types of memory range for 2nd kernel boot to
>     memmap_p
>   x86, cleanup: use dbgprint_mem_range for memory range debugging
>   x86, cleanup: increase CRASH_MAX_MEMMAP_NR up to
>     CRASH_MAX_MEMORY_RANGES
>   x86, cleanup: Store crash memory ranges kexec_info
>   x86, cleanup: kexec memory range .end to be inclusive
>   x86: add --pass-memmap-cmdline option
>   x86: Pass memory range via E820 for kdump
> 
>  kexec/arch/i386/crashdump-x86.c        |  69 +++++++--------
>  kexec/arch/i386/crashdump-x86.h        |   2 +-
>  kexec/arch/i386/include/arch/options.h |   2 +
>  kexec/arch/i386/kexec-x86-common.c     |   3 +-
>  kexec/arch/i386/kexec-x86.c            |   4 +
>  kexec/arch/i386/kexec-x86.h            |   1 +
>  kexec/arch/i386/x86-linux-setup.c      | 149 ++++++++++++++++++++++-----------
>  kexec/arch/i386/x86-linux-setup.h      |   1 +
>  kexec/arch/x86_64/kexec-x86_64.c       |   5 ++
>  kexec/firmware_memmap.c                |   1 -
>  kexec/kexec.h                          |   2 +
>  11 files changed, 147 insertions(+), 92 deletions(-)
> 

Hi,

Thanks for the update, I have one comment about the last patch which also
related to the CRASH_MAX_MEMMAP_NR patch, otherwise the patches looks good to me.

Dave

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

Re: [PATCH v6 9/9] x86: Pass memory range via E820 for kdump

[WANG Chao]

On 04/17/14 at 01:29pm, Dave Young wrote:
> On 04/14/14 at 10:55pm, WANG Chao wrote:
> > command line size is restricted by kernel, sometimes memmap=exactmap has
> > too many memory ranges to pass to cmdline. And also memmap=exactmap and
> > kASLR doesn't work together.
> > 
> > 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   |   6 +-
> >  kexec/arch/i386/x86-linux-setup.c | 149 +++++++++++++++++++++++++-------------
> >  kexec/arch/i386/x86-linux-setup.h |   1 +
> >  3 files changed, 103 insertions(+), 53 deletions(-)
> > 
> > diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> > index 7b618a6..4a1491b 100644
> > --- a/kexec/arch/i386/crashdump-x86.c
> > +++ b/kexec/arch/i386/crashdump-x86.c
> > @@ -979,7 +979,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> >  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
> >  	if (delete_memmap(memmap_p, &nr_memmap, elfcorehdr, memsz) < 0)
> >  		return -1;
> > -	cmdline_add_memmap(mod_cmdline, memmap_p);
> > +	if (arch_options.pass_memmap_cmdline)
> > +		cmdline_add_memmap(mod_cmdline, memmap_p);
> >  	if (!bzImage_support_efi_boot)
> >  		cmdline_add_efi(mod_cmdline);
> >  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
> > @@ -995,7 +996,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> >  		type = mem_range[i].type;
> >  		size = end - start + 1;
> >  		add_memmap(memmap_p, &nr_memmap, start, size, type);
> > -		cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > +		if (arch_options.pass_memmap_cmdline)
> > +			cmdline_add_memmap_acpi(mod_cmdline, start, end);
> 
> Seems memmap_p contains the acpi ranges as well, so cmdline_add_memmap_acpi is
> not necessary anymore, just improve cmdline_add_memmap to add both RAM and ACPI
> ranges is enough.

I can do that. But is it what the patchset is really about ...

I'm not keen about doing too much cleanup in this series now since it's
already v6. I really want to get this in as early as possible to
cope with calgary iommu change in the kernel.

I prefer to first get this patch in if there's no problem in it and then
look back and think about how we can clean up the code block which have
been there for historical reason.

Thanks
WANG Chao

> 
> Thanks
> Dave

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

Re: [PATCH v6 9/9] x86: Pass memory range via E820 for kdump

[Dave Young]

On 04/17/14 at 01:48pm, WANG Chao wrote:
> On 04/17/14 at 01:29pm, Dave Young wrote:
> > On 04/14/14 at 10:55pm, WANG Chao wrote:
> > > command line size is restricted by kernel, sometimes memmap=exactmap has
> > > too many memory ranges to pass to cmdline. And also memmap=exactmap and
> > > kASLR doesn't work together.
> > > 
> > > 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   |   6 +-
> > >  kexec/arch/i386/x86-linux-setup.c | 149 +++++++++++++++++++++++++-------------
> > >  kexec/arch/i386/x86-linux-setup.h |   1 +
> > >  3 files changed, 103 insertions(+), 53 deletions(-)
> > > 
> > > diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> > > index 7b618a6..4a1491b 100644
> > > --- a/kexec/arch/i386/crashdump-x86.c
> > > +++ b/kexec/arch/i386/crashdump-x86.c
> > > @@ -979,7 +979,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > >  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
> > >  	if (delete_memmap(memmap_p, &nr_memmap, elfcorehdr, memsz) < 0)
> > >  		return -1;
> > > -	cmdline_add_memmap(mod_cmdline, memmap_p);
> > > +	if (arch_options.pass_memmap_cmdline)
> > > +		cmdline_add_memmap(mod_cmdline, memmap_p);
> > >  	if (!bzImage_support_efi_boot)
> > >  		cmdline_add_efi(mod_cmdline);
> > >  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
> > > @@ -995,7 +996,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > >  		type = mem_range[i].type;
> > >  		size = end - start + 1;
> > >  		add_memmap(memmap_p, &nr_memmap, start, size, type);
> > > -		cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > > +		if (arch_options.pass_memmap_cmdline)
> > > +			cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > 
> > Seems memmap_p contains the acpi ranges as well, so cmdline_add_memmap_acpi is
> > not necessary anymore, just improve cmdline_add_memmap to add both RAM and ACPI
> > ranges is enough.
> 
> I can do that. But is it what the patchset is really about ...
> 
> I'm not keen about doing too much cleanup in this series now since it's
> already v6. I really want to get this in as early as possible to
> cope with calgary iommu change in the kernel.
> 
> I prefer to first get this patch in if there's no problem in it and then
> look back and think about how we can clean up the code block which have
> been there for historical reason.

I think the cleanup is worth, but if you want to do it later I'm fine.
So should better leave the patch 5/9 to later cleanup as well.

Thus except for 5/9, for other patches:
Acked-by: Dave Young <dyoung@redhat.com>

Thanks
Dave

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

Re: [PATCH v6 9/9] x86: Pass memory range via E820 for kdump

[WANG Chao]

On 04/17/14 at 01:35pm, Dave Young wrote:
> On 04/17/14 at 01:29pm, Dave Young wrote:
> > On 04/14/14 at 10:55pm, WANG Chao wrote:
> > > command line size is restricted by kernel, sometimes memmap=exactmap has
> > > too many memory ranges to pass to cmdline. And also memmap=exactmap and
> > > kASLR doesn't work together.
> > > 
> > > 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   |   6 +-
> > >  kexec/arch/i386/x86-linux-setup.c | 149 +++++++++++++++++++++++++-------------
> > >  kexec/arch/i386/x86-linux-setup.h |   1 +
> > >  3 files changed, 103 insertions(+), 53 deletions(-)
> > > 
> > > diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> > > index 7b618a6..4a1491b 100644
> > > --- a/kexec/arch/i386/crashdump-x86.c
> > > +++ b/kexec/arch/i386/crashdump-x86.c
> > > @@ -979,7 +979,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > >  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
> > >  	if (delete_memmap(memmap_p, &nr_memmap, elfcorehdr, memsz) < 0)
> > >  		return -1;
> > > -	cmdline_add_memmap(mod_cmdline, memmap_p);
> > > +	if (arch_options.pass_memmap_cmdline)
> > > +		cmdline_add_memmap(mod_cmdline, memmap_p);
> > >  	if (!bzImage_support_efi_boot)
> > >  		cmdline_add_efi(mod_cmdline);
> > >  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
> > > @@ -995,7 +996,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > >  		type = mem_range[i].type;
> > >  		size = end - start + 1;
> > >  		add_memmap(memmap_p, &nr_memmap, start, size, type);
> > > -		cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > > +		if (arch_options.pass_memmap_cmdline)
> > > +			cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > 
> > Seems memmap_p contains the acpi ranges as well, so cmdline_add_memmap_acpi is
> > not necessary anymore, just improve cmdline_add_memmap to add both RAM and ACPI
> > ranges is enough.
> 
> CRASH_MAX_MEMMAP_NR is used in cmdline_add_memmap, it does not include the acpi
> ranges, it looks strange, but anyway there's checking about cmdline overflow, tos
> I think maybe this CRASH_MAX_MEMMAP_NR can be dropped.

First, naming cmdline_add_memmap is not accurate regarding what the
function does (adding RAM only). But it's historical naming issue,
nothing to do with this patch :(

I'm sure that could be improved later.

Second, about dropping CRASH_MAX_MEMMAP_NR, I'm not sure if it's a good
idea.

CRASH_MAX_MEMMAP_NR is used to allocate memmap_p memory:

load_crash_segments(){
	[..]
	sz = (sizeof(struct memory_range) * CRASH_MAX_MEMMAP_NR);
	memmap_p = xmalloc(sz);
	memset(memmap_p, 0, sz);
	[..]
}

And so that every time when we walk through memmap_p,
CRASH_MAX_MEMMAP_NR can be used as a upper boundary.

I think maintain CRASH_MAX_MEMMAP_NR is reasonable and necessary based
on the current code base.

Thanks
WANG Chao

> 
> > 
> > Thanks
> > Dave
> > 
> > _______________________________________________
> > 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 v6 0/9] kexec-tools, x86: E820 memmap pass for kdump

[WANG Chao]

On 04/14/14 at 10:55pm, WANG Chao wrote:
> Hi, All
> 
> This patchset enables passing memory map via E820 map on x86 platform instead
> of memmap=exactmap. It's a better design and will solve the following problem
> so far:
> 
> - kernel cmdline is limited resource and large machines tend to have many
>     memory ranges that may excceed kernel cmdline limit size.
> - kASLR doesn't work with memmap=exactmap, because kASLR happens early than
>     user defined memmap=exactmap takes effect.
> 
> 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.
> 
> This patchset contains massive updates from the previous one. I take some
> suggestions from reviewers. I try to split the changes into smaller pieces and
> keep the whole change as minimal as I can so it wouldn't be too harsh to review
> the patch.
> 
> Any comment is appreciate!

Hi, Linn

Thanks for testing the patch in the past. Do you have chance to test
this update? 

This updated patchset changed too much and I want things to work as it's
used to be on your prototype machine with large number of memory ranges.

Thanks in advance!
WANG Chao

> 
> v6->v5:
> dyoung:
>  - use nr_memmap instead of nr_memmap_p
>  - .end inclusive
> Simon:
>  - more description on some patches
> 
> v5->v4:
> Dave:
>  - separate add_setup_data() to another patch.
> Vivek:
>  - adding comments for setup_data.
>  - store crash memory range info golobally in kexec_info.
> me:
>  -remove dbgprint_mem_range defination, Simon has merged the patch.
> 
> 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 (9):
>   x86, cleanup: add extra arguments to add_memmap() and delete_memmap()
>   x86, cleanup: add_memmap() only do alignment check on RANGE_RAM
>   x86, cleanup: add other types of memory range for 2nd kernel boot to
>     memmap_p
>   x86, cleanup: use dbgprint_mem_range for memory range debugging
>   x86, cleanup: increase CRASH_MAX_MEMMAP_NR up to
>     CRASH_MAX_MEMORY_RANGES
>   x86, cleanup: Store crash memory ranges kexec_info
>   x86, cleanup: kexec memory range .end to be inclusive
>   x86: add --pass-memmap-cmdline option
>   x86: Pass memory range via E820 for kdump
> 
>  kexec/arch/i386/crashdump-x86.c        |  69 +++++++--------
>  kexec/arch/i386/crashdump-x86.h        |   2 +-
>  kexec/arch/i386/include/arch/options.h |   2 +
>  kexec/arch/i386/kexec-x86-common.c     |   3 +-
>  kexec/arch/i386/kexec-x86.c            |   4 +
>  kexec/arch/i386/kexec-x86.h            |   1 +
>  kexec/arch/i386/x86-linux-setup.c      | 149 ++++++++++++++++++++++-----------
>  kexec/arch/i386/x86-linux-setup.h      |   1 +
>  kexec/arch/x86_64/kexec-x86_64.c       |   5 ++
>  kexec/firmware_memmap.c                |   1 -
>  kexec/kexec.h                          |   2 +
>  11 files changed, 147 insertions(+), 92 deletions(-)
> 
> -- 
> 1.9.0
> 

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

Re: [PATCH v6 9/9] x86: Pass memory range via E820 for kdump

[Dave Young]

On 04/17/14 at 02:17pm, WANG Chao wrote:
> On 04/17/14 at 01:35pm, Dave Young wrote:
> > On 04/17/14 at 01:29pm, Dave Young wrote:
> > > On 04/14/14 at 10:55pm, WANG Chao wrote:
> > > > command line size is restricted by kernel, sometimes memmap=exactmap has
> > > > too many memory ranges to pass to cmdline. And also memmap=exactmap and
> > > > kASLR doesn't work together.
> > > > 
> > > > 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   |   6 +-
> > > >  kexec/arch/i386/x86-linux-setup.c | 149 +++++++++++++++++++++++++-------------
> > > >  kexec/arch/i386/x86-linux-setup.h |   1 +
> > > >  3 files changed, 103 insertions(+), 53 deletions(-)
> > > > 
> > > > diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> > > > index 7b618a6..4a1491b 100644
> > > > --- a/kexec/arch/i386/crashdump-x86.c
> > > > +++ b/kexec/arch/i386/crashdump-x86.c
> > > > @@ -979,7 +979,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > > >  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
> > > >  	if (delete_memmap(memmap_p, &nr_memmap, elfcorehdr, memsz) < 0)
> > > >  		return -1;
> > > > -	cmdline_add_memmap(mod_cmdline, memmap_p);
> > > > +	if (arch_options.pass_memmap_cmdline)
> > > > +		cmdline_add_memmap(mod_cmdline, memmap_p);
> > > >  	if (!bzImage_support_efi_boot)
> > > >  		cmdline_add_efi(mod_cmdline);
> > > >  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
> > > > @@ -995,7 +996,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > > >  		type = mem_range[i].type;
> > > >  		size = end - start + 1;
> > > >  		add_memmap(memmap_p, &nr_memmap, start, size, type);
> > > > -		cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > > > +		if (arch_options.pass_memmap_cmdline)
> > > > +			cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > > 
> > > Seems memmap_p contains the acpi ranges as well, so cmdline_add_memmap_acpi is
> > > not necessary anymore, just improve cmdline_add_memmap to add both RAM and ACPI
> > > ranges is enough.
> > 
> > CRASH_MAX_MEMMAP_NR is used in cmdline_add_memmap, it does not include the acpi
> > ranges, it looks strange, but anyway there's checking about cmdline overflow, tos
> > I think maybe this CRASH_MAX_MEMMAP_NR can be dropped.
> 
> First, naming cmdline_add_memmap is not accurate regarding what the
> function does (adding RAM only). But it's historical naming issue,
> nothing to do with this patch :(
> 
> I'm sure that could be improved later.
> 
> Second, about dropping CRASH_MAX_MEMMAP_NR, I'm not sure if it's a good
> idea.
> 
> CRASH_MAX_MEMMAP_NR is used to allocate memmap_p memory:
> 
> load_crash_segments(){
> 	[..]
> 	sz = (sizeof(struct memory_range) * CRASH_MAX_MEMMAP_NR);
> 	memmap_p = xmalloc(sz);
> 	memset(memmap_p, 0, sz);
> 	[..]
> }
> 
> And so that every time when we walk through memmap_p,
> CRASH_MAX_MEMMAP_NR can be used as a upper boundary.
> 
> I think maintain CRASH_MAX_MEMMAP_NR is reasonable and necessary based
> on the current code base.

Hmm, if it's used for allocate mem range array then how about directly use
CRASH_MAX_MEMORY_RANGES which is same in your 5/9.

The memmap_p changed in this patchset, it contains not only the ram ranges
thus it's reasonable to use same upper limit as memory_range.

In the long run, I think moving the array to a list struct will be better
so we can dynamiclly allocate/insert/delete the ranges.

Thanks
Dave

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

Re: [PATCH v6 9/9] x86: Pass memory range via E820 for kdump

[Dave Young]

On 04/17/14 at 02:32pm, Dave Young wrote:
> On 04/17/14 at 02:17pm, WANG Chao wrote:
> > On 04/17/14 at 01:35pm, Dave Young wrote:
> > > On 04/17/14 at 01:29pm, Dave Young wrote:
> > > > On 04/14/14 at 10:55pm, WANG Chao wrote:
> > > > > command line size is restricted by kernel, sometimes memmap=exactmap has
> > > > > too many memory ranges to pass to cmdline. And also memmap=exactmap and
> > > > > kASLR doesn't work together.
> > > > > 
> > > > > 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   |   6 +-
> > > > >  kexec/arch/i386/x86-linux-setup.c | 149 +++++++++++++++++++++++++-------------
> > > > >  kexec/arch/i386/x86-linux-setup.h |   1 +
> > > > >  3 files changed, 103 insertions(+), 53 deletions(-)
> > > > > 
> > > > > diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> > > > > index 7b618a6..4a1491b 100644
> > > > > --- a/kexec/arch/i386/crashdump-x86.c
> > > > > +++ b/kexec/arch/i386/crashdump-x86.c
> > > > > @@ -979,7 +979,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > > > >  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
> > > > >  	if (delete_memmap(memmap_p, &nr_memmap, elfcorehdr, memsz) < 0)
> > > > >  		return -1;
> > > > > -	cmdline_add_memmap(mod_cmdline, memmap_p);
> > > > > +	if (arch_options.pass_memmap_cmdline)
> > > > > +		cmdline_add_memmap(mod_cmdline, memmap_p);
> > > > >  	if (!bzImage_support_efi_boot)
> > > > >  		cmdline_add_efi(mod_cmdline);
> > > > >  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
> > > > > @@ -995,7 +996,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > > > >  		type = mem_range[i].type;
> > > > >  		size = end - start + 1;
> > > > >  		add_memmap(memmap_p, &nr_memmap, start, size, type);
> > > > > -		cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > > > > +		if (arch_options.pass_memmap_cmdline)
> > > > > +			cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > > > 
> > > > Seems memmap_p contains the acpi ranges as well, so cmdline_add_memmap_acpi is
> > > > not necessary anymore, just improve cmdline_add_memmap to add both RAM and ACPI
> > > > ranges is enough.
> > > 
> > > CRASH_MAX_MEMMAP_NR is used in cmdline_add_memmap, it does not include the acpi
> > > ranges, it looks strange, but anyway there's checking about cmdline overflow, tos
> > > I think maybe this CRASH_MAX_MEMMAP_NR can be dropped.
> > 
> > First, naming cmdline_add_memmap is not accurate regarding what the
> > function does (adding RAM only). But it's historical naming issue,
> > nothing to do with this patch :(
> > 
> > I'm sure that could be improved later.
> > 
> > Second, about dropping CRASH_MAX_MEMMAP_NR, I'm not sure if it's a good
> > idea.
> > 
> > CRASH_MAX_MEMMAP_NR is used to allocate memmap_p memory:
> > 
> > load_crash_segments(){
> > 	[..]
> > 	sz = (sizeof(struct memory_range) * CRASH_MAX_MEMMAP_NR);
> > 	memmap_p = xmalloc(sz);
> > 	memset(memmap_p, 0, sz);
> > 	[..]
> > }
> > 
> > And so that every time when we walk through memmap_p,
> > CRASH_MAX_MEMMAP_NR can be used as a upper boundary.
> > 
> > I think maintain CRASH_MAX_MEMMAP_NR is reasonable and necessary based
> > on the current code base.
> 
> Hmm, if it's used for allocate mem range array then how about directly use
> CRASH_MAX_MEMORY_RANGES which is same in your 5/9.

I really would like to see a cleaner code, but since this issue is trival
I will not object it now.

For the calgary issue I'm curious if there are people who report a bug.
At least if that happens we will know that there's actually someone who
are using the calgary code.

Thanks
Dave

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

Re: [PATCH v6 9/9] x86: Pass memory range via E820 for kdump

[Dave Young]

On 04/17/14 at 02:44pm, Dave Young wrote:
> On 04/17/14 at 02:32pm, Dave Young wrote:
> > On 04/17/14 at 02:17pm, WANG Chao wrote:
> > > On 04/17/14 at 01:35pm, Dave Young wrote:
> > > > On 04/17/14 at 01:29pm, Dave Young wrote:
> > > > > On 04/14/14 at 10:55pm, WANG Chao wrote:
> > > > > > command line size is restricted by kernel, sometimes memmap=exactmap has
> > > > > > too many memory ranges to pass to cmdline. And also memmap=exactmap and
> > > > > > kASLR doesn't work together.
> > > > > > 
> > > > > > 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   |   6 +-
> > > > > >  kexec/arch/i386/x86-linux-setup.c | 149 +++++++++++++++++++++++++-------------
> > > > > >  kexec/arch/i386/x86-linux-setup.h |   1 +
> > > > > >  3 files changed, 103 insertions(+), 53 deletions(-)
> > > > > > 
> > > > > > diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> > > > > > index 7b618a6..4a1491b 100644
> > > > > > --- a/kexec/arch/i386/crashdump-x86.c
> > > > > > +++ b/kexec/arch/i386/crashdump-x86.c
> > > > > > @@ -979,7 +979,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > > > > >  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
> > > > > >  	if (delete_memmap(memmap_p, &nr_memmap, elfcorehdr, memsz) < 0)
> > > > > >  		return -1;
> > > > > > -	cmdline_add_memmap(mod_cmdline, memmap_p);
> > > > > > +	if (arch_options.pass_memmap_cmdline)
> > > > > > +		cmdline_add_memmap(mod_cmdline, memmap_p);
> > > > > >  	if (!bzImage_support_efi_boot)
> > > > > >  		cmdline_add_efi(mod_cmdline);
> > > > > >  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
> > > > > > @@ -995,7 +996,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > > > > >  		type = mem_range[i].type;
> > > > > >  		size = end - start + 1;
> > > > > >  		add_memmap(memmap_p, &nr_memmap, start, size, type);
> > > > > > -		cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > > > > > +		if (arch_options.pass_memmap_cmdline)
> > > > > > +			cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > > > > 
> > > > > Seems memmap_p contains the acpi ranges as well, so cmdline_add_memmap_acpi is
> > > > > not necessary anymore, just improve cmdline_add_memmap to add both RAM and ACPI
> > > > > ranges is enough.
> > > > 
> > > > CRASH_MAX_MEMMAP_NR is used in cmdline_add_memmap, it does not include the acpi
> > > > ranges, it looks strange, but anyway there's checking about cmdline overflow, tos
> > > > I think maybe this CRASH_MAX_MEMMAP_NR can be dropped.
> > > 
> > > First, naming cmdline_add_memmap is not accurate regarding what the
> > > function does (adding RAM only). But it's historical naming issue,
> > > nothing to do with this patch :(
> > > 
> > > I'm sure that could be improved later.
> > > 
> > > Second, about dropping CRASH_MAX_MEMMAP_NR, I'm not sure if it's a good
> > > idea.
> > > 
> > > CRASH_MAX_MEMMAP_NR is used to allocate memmap_p memory:
> > > 
> > > load_crash_segments(){
> > > 	[..]
> > > 	sz = (sizeof(struct memory_range) * CRASH_MAX_MEMMAP_NR);
> > > 	memmap_p = xmalloc(sz);
> > > 	memset(memmap_p, 0, sz);
> > > 	[..]
> > > }
> > > 
> > > And so that every time when we walk through memmap_p,
> > > CRASH_MAX_MEMMAP_NR can be used as a upper boundary.
> > > 
> > > I think maintain CRASH_MAX_MEMMAP_NR is reasonable and necessary based
> > > on the current code base.
> > 
> > Hmm, if it's used for allocate mem range array then how about directly use
> > CRASH_MAX_MEMORY_RANGES which is same in your 5/9.
> 
> I really would like to see a cleaner code, but since this issue is trival
> I will not object it now.
> 
> For the calgary issue I'm curious if there are people who report a bug.
> At least if that happens we will know that there's actually someone who
> are using the calgary code.
> 

For the cleanup, usually if it's not hard I would suggest let's do it when
we see the problem because there's many real examples that we have no chance to revise
it for long time though we agreed to do it. Either because we are occupied
by some other stuff or we just forgot it.

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

Re: [PATCH v6 9/9] x86: Pass memory range via E820 for kdump

[WANG Chao]

On 04/17/14 at 02:32pm, Dave Young wrote:
> On 04/17/14 at 02:17pm, WANG Chao wrote:
> > On 04/17/14 at 01:35pm, Dave Young wrote:
> > > On 04/17/14 at 01:29pm, Dave Young wrote:
> > > > On 04/14/14 at 10:55pm, WANG Chao wrote:
> > > > > command line size is restricted by kernel, sometimes memmap=exactmap has
> > > > > too many memory ranges to pass to cmdline. And also memmap=exactmap and
> > > > > kASLR doesn't work together.
> > > > > 
> > > > > 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   |   6 +-
> > > > >  kexec/arch/i386/x86-linux-setup.c | 149 +++++++++++++++++++++++++-------------
> > > > >  kexec/arch/i386/x86-linux-setup.h |   1 +
> > > > >  3 files changed, 103 insertions(+), 53 deletions(-)
> > > > > 
> > > > > diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> > > > > index 7b618a6..4a1491b 100644
> > > > > --- a/kexec/arch/i386/crashdump-x86.c
> > > > > +++ b/kexec/arch/i386/crashdump-x86.c
> > > > > @@ -979,7 +979,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > > > >  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
> > > > >  	if (delete_memmap(memmap_p, &nr_memmap, elfcorehdr, memsz) < 0)
> > > > >  		return -1;
> > > > > -	cmdline_add_memmap(mod_cmdline, memmap_p);
> > > > > +	if (arch_options.pass_memmap_cmdline)
> > > > > +		cmdline_add_memmap(mod_cmdline, memmap_p);
> > > > >  	if (!bzImage_support_efi_boot)
> > > > >  		cmdline_add_efi(mod_cmdline);
> > > > >  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
> > > > > @@ -995,7 +996,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > > > >  		type = mem_range[i].type;
> > > > >  		size = end - start + 1;
> > > > >  		add_memmap(memmap_p, &nr_memmap, start, size, type);
> > > > > -		cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > > > > +		if (arch_options.pass_memmap_cmdline)
> > > > > +			cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > > > 
> > > > Seems memmap_p contains the acpi ranges as well, so cmdline_add_memmap_acpi is
> > > > not necessary anymore, just improve cmdline_add_memmap to add both RAM and ACPI
> > > > ranges is enough.
> > > 
> > > CRASH_MAX_MEMMAP_NR is used in cmdline_add_memmap, it does not include the acpi
> > > ranges, it looks strange, but anyway there's checking about cmdline overflow, tos
> > > I think maybe this CRASH_MAX_MEMMAP_NR can be dropped.
> > 
> > First, naming cmdline_add_memmap is not accurate regarding what the
> > function does (adding RAM only). But it's historical naming issue,
> > nothing to do with this patch :(
> > 
> > I'm sure that could be improved later.
> > 
> > Second, about dropping CRASH_MAX_MEMMAP_NR, I'm not sure if it's a good
> > idea.
> > 
> > CRASH_MAX_MEMMAP_NR is used to allocate memmap_p memory:
> > 
> > load_crash_segments(){
> > 	[..]
> > 	sz = (sizeof(struct memory_range) * CRASH_MAX_MEMMAP_NR);
> > 	memmap_p = xmalloc(sz);
> > 	memset(memmap_p, 0, sz);
> > 	[..]
> > }
> > 
> > And so that every time when we walk through memmap_p,
> > CRASH_MAX_MEMMAP_NR can be used as a upper boundary.
> > 
> > I think maintain CRASH_MAX_MEMMAP_NR is reasonable and necessary based
> > on the current code base.
> 
> Hmm, if it's used for allocate mem range array then how about directly use
> CRASH_MAX_MEMORY_RANGES which is same in your 5/9.

I did that on purpose for two reasons:

- I tend to make the changes as minimal as possible to make reviewer's
  life easier. So that I choose the same value for CRASH_MAX_MEMMAP_NR
  as CRASH_MAX_MEMORY_RANGES.

- CRASH_MAX_MEMORY_RANGES and CRASH_MAX_MEMMAP_NR are used for
  allocating different memory_range structures, crash_memory_range vs.
  memmap_p. Those two different variables are used in different places
  and serve for different purpose (1st kernel's memmap vs. 2nd kernel's
  memmap). In long term, I think keeping both macros separately is a
  better choice because one of them could change for whatever reason.

> 
> The memmap_p changed in this patchset, it contains not only the ram ranges
> thus it's reasonable to use same upper limit as memory_range.

crash_memory_range[] contains memory ranges from 1st kernel.
memmap_p[] contains memory ranges we need for 2nd kernel.

They can be different. So I'd like to keep two different variables for
upper limit. Even though these two upper limits is set to the same value
currently.

> 
> In the long run, I think moving the array to a list struct will be better
> so we can dynamiclly allocate/insert/delete the ranges.

Agree. The only problem left is whether the memory ranges need a upper
boundary as it is 1024 now.

Thanks
WANG Chao

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

Re: [PATCH v6 9/9] x86: Pass memory range via E820 for kdump

[WANG Chao]

On 04/17/14 at 02:44pm, Dave Young wrote:
> On 04/17/14 at 02:32pm, Dave Young wrote:
> > On 04/17/14 at 02:17pm, WANG Chao wrote:
> > > On 04/17/14 at 01:35pm, Dave Young wrote:
> > > > On 04/17/14 at 01:29pm, Dave Young wrote:
> > > > > On 04/14/14 at 10:55pm, WANG Chao wrote:
> > > > > > command line size is restricted by kernel, sometimes memmap=exactmap has
> > > > > > too many memory ranges to pass to cmdline. And also memmap=exactmap and
> > > > > > kASLR doesn't work together.
> > > > > > 
> > > > > > 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   |   6 +-
> > > > > >  kexec/arch/i386/x86-linux-setup.c | 149 +++++++++++++++++++++++++-------------
> > > > > >  kexec/arch/i386/x86-linux-setup.h |   1 +
> > > > > >  3 files changed, 103 insertions(+), 53 deletions(-)
> > > > > > 
> > > > > > diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> > > > > > index 7b618a6..4a1491b 100644
> > > > > > --- a/kexec/arch/i386/crashdump-x86.c
> > > > > > +++ b/kexec/arch/i386/crashdump-x86.c
> > > > > > @@ -979,7 +979,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > > > > >  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
> > > > > >  	if (delete_memmap(memmap_p, &nr_memmap, elfcorehdr, memsz) < 0)
> > > > > >  		return -1;
> > > > > > -	cmdline_add_memmap(mod_cmdline, memmap_p);
> > > > > > +	if (arch_options.pass_memmap_cmdline)
> > > > > > +		cmdline_add_memmap(mod_cmdline, memmap_p);
> > > > > >  	if (!bzImage_support_efi_boot)
> > > > > >  		cmdline_add_efi(mod_cmdline);
> > > > > >  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
> > > > > > @@ -995,7 +996,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > > > > >  		type = mem_range[i].type;
> > > > > >  		size = end - start + 1;
> > > > > >  		add_memmap(memmap_p, &nr_memmap, start, size, type);
> > > > > > -		cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > > > > > +		if (arch_options.pass_memmap_cmdline)
> > > > > > +			cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > > > > 
> > > > > Seems memmap_p contains the acpi ranges as well, so cmdline_add_memmap_acpi is
> > > > > not necessary anymore, just improve cmdline_add_memmap to add both RAM and ACPI
> > > > > ranges is enough.
> > > > 
> > > > CRASH_MAX_MEMMAP_NR is used in cmdline_add_memmap, it does not include the acpi
> > > > ranges, it looks strange, but anyway there's checking about cmdline overflow, tos
> > > > I think maybe this CRASH_MAX_MEMMAP_NR can be dropped.
> > > 
> > > First, naming cmdline_add_memmap is not accurate regarding what the
> > > function does (adding RAM only). But it's historical naming issue,
> > > nothing to do with this patch :(
> > > 
> > > I'm sure that could be improved later.
> > > 
> > > Second, about dropping CRASH_MAX_MEMMAP_NR, I'm not sure if it's a good
> > > idea.
> > > 
> > > CRASH_MAX_MEMMAP_NR is used to allocate memmap_p memory:
> > > 
> > > load_crash_segments(){
> > > 	[..]
> > > 	sz = (sizeof(struct memory_range) * CRASH_MAX_MEMMAP_NR);
> > > 	memmap_p = xmalloc(sz);
> > > 	memset(memmap_p, 0, sz);
> > > 	[..]
> > > }
> > > 
> > > And so that every time when we walk through memmap_p,
> > > CRASH_MAX_MEMMAP_NR can be used as a upper boundary.
> > > 
> > > I think maintain CRASH_MAX_MEMMAP_NR is reasonable and necessary based
> > > on the current code base.
> > 
> > Hmm, if it's used for allocate mem range array then how about directly use
> > CRASH_MAX_MEMORY_RANGES which is same in your 5/9.
> 
> I really would like to see a cleaner code, but since this issue is trival
> I will not object it now.
> 
> For the calgary issue I'm curious if there are people who report a bug.
> At least if that happens we will know that there's actually someone who
> are using the calgary code.

No, there's not any, as far as I know. But I wouldn't take the chance.

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

Re: [PATCH v6 9/9] x86: Pass memory range via E820 for kdump

[Dave Young]

On 04/17/14 at 02:57pm, WANG Chao wrote:
> On 04/17/14 at 02:32pm, Dave Young wrote:
> > On 04/17/14 at 02:17pm, WANG Chao wrote:
> > > On 04/17/14 at 01:35pm, Dave Young wrote:
> > > > On 04/17/14 at 01:29pm, Dave Young wrote:
> > > > > On 04/14/14 at 10:55pm, WANG Chao wrote:
> > > > > > command line size is restricted by kernel, sometimes memmap=exactmap has
> > > > > > too many memory ranges to pass to cmdline. And also memmap=exactmap and
> > > > > > kASLR doesn't work together.
> > > > > > 
> > > > > > 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   |   6 +-
> > > > > >  kexec/arch/i386/x86-linux-setup.c | 149 +++++++++++++++++++++++++-------------
> > > > > >  kexec/arch/i386/x86-linux-setup.h |   1 +
> > > > > >  3 files changed, 103 insertions(+), 53 deletions(-)
> > > > > > 
> > > > > > diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> > > > > > index 7b618a6..4a1491b 100644
> > > > > > --- a/kexec/arch/i386/crashdump-x86.c
> > > > > > +++ b/kexec/arch/i386/crashdump-x86.c
> > > > > > @@ -979,7 +979,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > > > > >  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
> > > > > >  	if (delete_memmap(memmap_p, &nr_memmap, elfcorehdr, memsz) < 0)
> > > > > >  		return -1;
> > > > > > -	cmdline_add_memmap(mod_cmdline, memmap_p);
> > > > > > +	if (arch_options.pass_memmap_cmdline)
> > > > > > +		cmdline_add_memmap(mod_cmdline, memmap_p);
> > > > > >  	if (!bzImage_support_efi_boot)
> > > > > >  		cmdline_add_efi(mod_cmdline);
> > > > > >  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
> > > > > > @@ -995,7 +996,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > > > > >  		type = mem_range[i].type;
> > > > > >  		size = end - start + 1;
> > > > > >  		add_memmap(memmap_p, &nr_memmap, start, size, type);
> > > > > > -		cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > > > > > +		if (arch_options.pass_memmap_cmdline)
> > > > > > +			cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > > > > 
> > > > > Seems memmap_p contains the acpi ranges as well, so cmdline_add_memmap_acpi is
> > > > > not necessary anymore, just improve cmdline_add_memmap to add both RAM and ACPI
> > > > > ranges is enough.
> > > > 
> > > > CRASH_MAX_MEMMAP_NR is used in cmdline_add_memmap, it does not include the acpi
> > > > ranges, it looks strange, but anyway there's checking about cmdline overflow, tos
> > > > I think maybe this CRASH_MAX_MEMMAP_NR can be dropped.
> > > 
> > > First, naming cmdline_add_memmap is not accurate regarding what the
> > > function does (adding RAM only). But it's historical naming issue,
> > > nothing to do with this patch :(
> > > 
> > > I'm sure that could be improved later.
> > > 
> > > Second, about dropping CRASH_MAX_MEMMAP_NR, I'm not sure if it's a good
> > > idea.
> > > 
> > > CRASH_MAX_MEMMAP_NR is used to allocate memmap_p memory:
> > > 
> > > load_crash_segments(){
> > > 	[..]
> > > 	sz = (sizeof(struct memory_range) * CRASH_MAX_MEMMAP_NR);
> > > 	memmap_p = xmalloc(sz);
> > > 	memset(memmap_p, 0, sz);
> > > 	[..]
> > > }
> > > 
> > > And so that every time when we walk through memmap_p,
> > > CRASH_MAX_MEMMAP_NR can be used as a upper boundary.
> > > 
> > > I think maintain CRASH_MAX_MEMMAP_NR is reasonable and necessary based
> > > on the current code base.
> > 
> > Hmm, if it's used for allocate mem range array then how about directly use
> > CRASH_MAX_MEMORY_RANGES which is same in your 5/9.
> 
> I did that on purpose for two reasons:
> 
> - I tend to make the changes as minimal as possible to make reviewer's
>   life easier. So that I choose the same value for CRASH_MAX_MEMMAP_NR
>   as CRASH_MAX_MEMORY_RANGES.

As long as the patches are logically clear, it does not matter to me :)

> 
> - CRASH_MAX_MEMORY_RANGES and CRASH_MAX_MEMMAP_NR are used for
>   allocating different memory_range structures, crash_memory_range vs.
>   memmap_p. Those two different variables are used in different places
>   and serve for different purpose (1st kernel's memmap vs. 2nd kernel's
>   memmap). In long term, I think keeping both macros separately is a
>   better choice because one of them could change for whatever reason.

It's unlikely at least now IMO, I think we should even drop the limit
because we have e820 + setup_data.

Thanks
Dave

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

Re: [PATCH v6 9/9] x86: Pass memory range via E820 for kdump

[WANG Chao]

On 04/17/14 at 03:07pm, Dave Young wrote:
> On 04/17/14 at 02:57pm, WANG Chao wrote:
> > On 04/17/14 at 02:32pm, Dave Young wrote:
> > > On 04/17/14 at 02:17pm, WANG Chao wrote:
> > > > On 04/17/14 at 01:35pm, Dave Young wrote:
> > > > > On 04/17/14 at 01:29pm, Dave Young wrote:
> > > > > > On 04/14/14 at 10:55pm, WANG Chao wrote:
> > > > > > > command line size is restricted by kernel, sometimes memmap=exactmap has
> > > > > > > too many memory ranges to pass to cmdline. And also memmap=exactmap and
> > > > > > > kASLR doesn't work together.
> > > > > > > 
> > > > > > > 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   |   6 +-
> > > > > > >  kexec/arch/i386/x86-linux-setup.c | 149 +++++++++++++++++++++++++-------------
> > > > > > >  kexec/arch/i386/x86-linux-setup.h |   1 +
> > > > > > >  3 files changed, 103 insertions(+), 53 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> > > > > > > index 7b618a6..4a1491b 100644
> > > > > > > --- a/kexec/arch/i386/crashdump-x86.c
> > > > > > > +++ b/kexec/arch/i386/crashdump-x86.c
> > > > > > > @@ -979,7 +979,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > > > > > >  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
> > > > > > >  	if (delete_memmap(memmap_p, &nr_memmap, elfcorehdr, memsz) < 0)
> > > > > > >  		return -1;
> > > > > > > -	cmdline_add_memmap(mod_cmdline, memmap_p);
> > > > > > > +	if (arch_options.pass_memmap_cmdline)
> > > > > > > +		cmdline_add_memmap(mod_cmdline, memmap_p);
> > > > > > >  	if (!bzImage_support_efi_boot)
> > > > > > >  		cmdline_add_efi(mod_cmdline);
> > > > > > >  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
> > > > > > > @@ -995,7 +996,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > > > > > >  		type = mem_range[i].type;
> > > > > > >  		size = end - start + 1;
> > > > > > >  		add_memmap(memmap_p, &nr_memmap, start, size, type);
> > > > > > > -		cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > > > > > > +		if (arch_options.pass_memmap_cmdline)
> > > > > > > +			cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > > > > > 
> > > > > > Seems memmap_p contains the acpi ranges as well, so cmdline_add_memmap_acpi is
> > > > > > not necessary anymore, just improve cmdline_add_memmap to add both RAM and ACPI
> > > > > > ranges is enough.
> > > > > 
> > > > > CRASH_MAX_MEMMAP_NR is used in cmdline_add_memmap, it does not include the acpi
> > > > > ranges, it looks strange, but anyway there's checking about cmdline overflow, tos
> > > > > I think maybe this CRASH_MAX_MEMMAP_NR can be dropped.
> > > > 
> > > > First, naming cmdline_add_memmap is not accurate regarding what the
> > > > function does (adding RAM only). But it's historical naming issue,
> > > > nothing to do with this patch :(
> > > > 
> > > > I'm sure that could be improved later.
> > > > 
> > > > Second, about dropping CRASH_MAX_MEMMAP_NR, I'm not sure if it's a good
> > > > idea.
> > > > 
> > > > CRASH_MAX_MEMMAP_NR is used to allocate memmap_p memory:
> > > > 
> > > > load_crash_segments(){
> > > > 	[..]
> > > > 	sz = (sizeof(struct memory_range) * CRASH_MAX_MEMMAP_NR);
> > > > 	memmap_p = xmalloc(sz);
> > > > 	memset(memmap_p, 0, sz);
> > > > 	[..]
> > > > }
> > > > 
> > > > And so that every time when we walk through memmap_p,
> > > > CRASH_MAX_MEMMAP_NR can be used as a upper boundary.
> > > > 
> > > > I think maintain CRASH_MAX_MEMMAP_NR is reasonable and necessary based
> > > > on the current code base.
> > > 
> > > Hmm, if it's used for allocate mem range array then how about directly use
> > > CRASH_MAX_MEMORY_RANGES which is same in your 5/9.
> > 
> > I did that on purpose for two reasons:
> > 
> > - I tend to make the changes as minimal as possible to make reviewer's
> >   life easier. So that I choose the same value for CRASH_MAX_MEMMAP_NR
> >   as CRASH_MAX_MEMORY_RANGES.
> 
> As long as the patches are logically clear, it does not matter to me :)

Maybe I should have set CRASH_MAX_MEMMAP_NR to 1024 directly.

> 
> > 
> > - CRASH_MAX_MEMORY_RANGES and CRASH_MAX_MEMMAP_NR are used for
> >   allocating different memory_range structures, crash_memory_range vs.
> >   memmap_p. Those two different variables are used in different places
> >   and serve for different purpose (1st kernel's memmap vs. 2nd kernel's
> >   memmap). In long term, I think keeping both macros separately is a
> >   better choice because one of them could change for whatever reason.
> 
> It's unlikely at least now IMO, I think we should even drop the limit
> because we have e820 + setup_data.

Do you know if kernel has limitation for setup_data?

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

Re: [PATCH v6 9/9] x86: Pass memory range via E820 for kdump

[Dave Young]

On 04/17/14 at 03:17pm, WANG Chao wrote:
> On 04/17/14 at 03:07pm, Dave Young wrote:
> > On 04/17/14 at 02:57pm, WANG Chao wrote:
> > > On 04/17/14 at 02:32pm, Dave Young wrote:
> > > > On 04/17/14 at 02:17pm, WANG Chao wrote:
> > > > > On 04/17/14 at 01:35pm, Dave Young wrote:
> > > > > > On 04/17/14 at 01:29pm, Dave Young wrote:
> > > > > > > On 04/14/14 at 10:55pm, WANG Chao wrote:
> > > > > > > > command line size is restricted by kernel, sometimes memmap=exactmap has
> > > > > > > > too many memory ranges to pass to cmdline. And also memmap=exactmap and
> > > > > > > > kASLR doesn't work together.
> > > > > > > > 
> > > > > > > > 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   |   6 +-
> > > > > > > >  kexec/arch/i386/x86-linux-setup.c | 149 +++++++++++++++++++++++++-------------
> > > > > > > >  kexec/arch/i386/x86-linux-setup.h |   1 +
> > > > > > > >  3 files changed, 103 insertions(+), 53 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> > > > > > > > index 7b618a6..4a1491b 100644
> > > > > > > > --- a/kexec/arch/i386/crashdump-x86.c
> > > > > > > > +++ b/kexec/arch/i386/crashdump-x86.c
> > > > > > > > @@ -979,7 +979,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > > > > > > >  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
> > > > > > > >  	if (delete_memmap(memmap_p, &nr_memmap, elfcorehdr, memsz) < 0)
> > > > > > > >  		return -1;
> > > > > > > > -	cmdline_add_memmap(mod_cmdline, memmap_p);
> > > > > > > > +	if (arch_options.pass_memmap_cmdline)
> > > > > > > > +		cmdline_add_memmap(mod_cmdline, memmap_p);
> > > > > > > >  	if (!bzImage_support_efi_boot)
> > > > > > > >  		cmdline_add_efi(mod_cmdline);
> > > > > > > >  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
> > > > > > > > @@ -995,7 +996,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > > > > > > >  		type = mem_range[i].type;
> > > > > > > >  		size = end - start + 1;
> > > > > > > >  		add_memmap(memmap_p, &nr_memmap, start, size, type);
> > > > > > > > -		cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > > > > > > > +		if (arch_options.pass_memmap_cmdline)
> > > > > > > > +			cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > > > > > > 
> > > > > > > Seems memmap_p contains the acpi ranges as well, so cmdline_add_memmap_acpi is
> > > > > > > not necessary anymore, just improve cmdline_add_memmap to add both RAM and ACPI
> > > > > > > ranges is enough.
> > > > > > 
> > > > > > CRASH_MAX_MEMMAP_NR is used in cmdline_add_memmap, it does not include the acpi
> > > > > > ranges, it looks strange, but anyway there's checking about cmdline overflow, tos
> > > > > > I think maybe this CRASH_MAX_MEMMAP_NR can be dropped.
> > > > > 
> > > > > First, naming cmdline_add_memmap is not accurate regarding what the
> > > > > function does (adding RAM only). But it's historical naming issue,
> > > > > nothing to do with this patch :(
> > > > > 
> > > > > I'm sure that could be improved later.
> > > > > 
> > > > > Second, about dropping CRASH_MAX_MEMMAP_NR, I'm not sure if it's a good
> > > > > idea.
> > > > > 
> > > > > CRASH_MAX_MEMMAP_NR is used to allocate memmap_p memory:
> > > > > 
> > > > > load_crash_segments(){
> > > > > 	[..]
> > > > > 	sz = (sizeof(struct memory_range) * CRASH_MAX_MEMMAP_NR);
> > > > > 	memmap_p = xmalloc(sz);
> > > > > 	memset(memmap_p, 0, sz);
> > > > > 	[..]
> > > > > }
> > > > > 
> > > > > And so that every time when we walk through memmap_p,
> > > > > CRASH_MAX_MEMMAP_NR can be used as a upper boundary.
> > > > > 
> > > > > I think maintain CRASH_MAX_MEMMAP_NR is reasonable and necessary based
> > > > > on the current code base.
> > > > 
> > > > Hmm, if it's used for allocate mem range array then how about directly use
> > > > CRASH_MAX_MEMORY_RANGES which is same in your 5/9.
> > > 
> > > I did that on purpose for two reasons:
> > > 
> > > - I tend to make the changes as minimal as possible to make reviewer's
> > >   life easier. So that I choose the same value for CRASH_MAX_MEMMAP_NR
> > >   as CRASH_MAX_MEMORY_RANGES.
> > 
> > As long as the patches are logically clear, it does not matter to me :)
> 
> Maybe I should have set CRASH_MAX_MEMMAP_NR to 1024 directly.

Rethinking about it the hard limit is not good I remember there's report from
SGI there's a lot of memory ranges so in the patch they increased the max ranges
to 2048, but the patch was reverted.

Since there's several possible improvements I'm fine with current way in
your patch, let keep it as is for now.

I will ack the whole series from my point of view. 

> 
> > 
> > > 
> > > - CRASH_MAX_MEMORY_RANGES and CRASH_MAX_MEMMAP_NR are used for
> > >   allocating different memory_range structures, crash_memory_range vs.
> > >   memmap_p. Those two different variables are used in different places
> > >   and serve for different purpose (1st kernel's memmap vs. 2nd kernel's
> > >   memmap). In long term, I think keeping both macros separately is a
> > >   better choice because one of them could change for whatever reason.
> > 
> > It's unlikely at least now IMO, I think we should even drop the limit
> > because we have e820 + setup_data.
> 
> Do you know if kernel has limitation for setup_data?

I do not think there's limitation for setup_data as it's a link list..

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

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

[Dave Young]

On 04/14/14 at 10:55pm, WANG Chao wrote:
> Hi, All
> 
> This patchset enables passing memory map via E820 map on x86 platform instead
> of memmap=exactmap. It's a better design and will solve the following problem
> so far:
> 
> - kernel cmdline is limited resource and large machines tend to have many
>     memory ranges that may excceed kernel cmdline limit size.
> - kASLR doesn't work with memmap=exactmap, because kASLR happens early than
>     user defined memmap=exactmap takes effect.
> 
> 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.
> 
> This patchset contains massive updates from the previous one. I take some
> suggestions from reviewers. I try to split the changes into smaller pieces and
> keep the whole change as minimal as I can so it wouldn't be too harsh to review
> the patch.
> 
> Any comment is appreciate!
> 
> v6->v5:
> dyoung:
>  - use nr_memmap instead of nr_memmap_p
>  - .end inclusive
> Simon:
>  - more description on some patches
> 
> v5->v4:
> Dave:
>  - separate add_setup_data() to another patch.
> Vivek:
>  - adding comments for setup_data.
>  - store crash memory range info golobally in kexec_info.
> me:
>  -remove dbgprint_mem_range defination, Simon has merged the patch.
> 
> 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 (9):
>   x86, cleanup: add extra arguments to add_memmap() and delete_memmap()
>   x86, cleanup: add_memmap() only do alignment check on RANGE_RAM
>   x86, cleanup: add other types of memory range for 2nd kernel boot to
>     memmap_p
>   x86, cleanup: use dbgprint_mem_range for memory range debugging
>   x86, cleanup: increase CRASH_MAX_MEMMAP_NR up to
>     CRASH_MAX_MEMORY_RANGES
>   x86, cleanup: Store crash memory ranges kexec_info
>   x86, cleanup: kexec memory range .end to be inclusive
>   x86: add --pass-memmap-cmdline option
>   x86: Pass memory range via E820 for kdump
> 
>  kexec/arch/i386/crashdump-x86.c        |  69 +++++++--------
>  kexec/arch/i386/crashdump-x86.h        |   2 +-
>  kexec/arch/i386/include/arch/options.h |   2 +
>  kexec/arch/i386/kexec-x86-common.c     |   3 +-
>  kexec/arch/i386/kexec-x86.c            |   4 +
>  kexec/arch/i386/kexec-x86.h            |   1 +
>  kexec/arch/i386/x86-linux-setup.c      | 149 ++++++++++++++++++++++-----------
>  kexec/arch/i386/x86-linux-setup.h      |   1 +
>  kexec/arch/x86_64/kexec-x86_64.c       |   5 ++
>  kexec/firmware_memmap.c                |   1 -
>  kexec/kexec.h                          |   2 +
>  11 files changed, 147 insertions(+), 92 deletions(-)
> 

After discussion with Chao, I'm fine with current patch series

Acked-by: Dave Young <dyoung@redhat.com>

Thanks
Dave

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

Re: [PATCH v6 9/9] x86: Pass memory range via E820 for kdump

[WANG Chao]

On 04/17/14 at 03:30pm, Dave Young wrote:
> On 04/17/14 at 03:17pm, WANG Chao wrote:
> > On 04/17/14 at 03:07pm, Dave Young wrote:
> > > On 04/17/14 at 02:57pm, WANG Chao wrote:
> > > > On 04/17/14 at 02:32pm, Dave Young wrote:
> > > > > On 04/17/14 at 02:17pm, WANG Chao wrote:
> > > > > > On 04/17/14 at 01:35pm, Dave Young wrote:
> > > > > > > On 04/17/14 at 01:29pm, Dave Young wrote:
> > > > > > > > On 04/14/14 at 10:55pm, WANG Chao wrote:
> > > > > > > > > command line size is restricted by kernel, sometimes memmap=exactmap has
> > > > > > > > > too many memory ranges to pass to cmdline. And also memmap=exactmap and
> > > > > > > > > kASLR doesn't work together.
> > > > > > > > > 
> > > > > > > > > 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   |   6 +-
> > > > > > > > >  kexec/arch/i386/x86-linux-setup.c | 149 +++++++++++++++++++++++++-------------
> > > > > > > > >  kexec/arch/i386/x86-linux-setup.h |   1 +
> > > > > > > > >  3 files changed, 103 insertions(+), 53 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> > > > > > > > > index 7b618a6..4a1491b 100644
> > > > > > > > > --- a/kexec/arch/i386/crashdump-x86.c
> > > > > > > > > +++ b/kexec/arch/i386/crashdump-x86.c
> > > > > > > > > @@ -979,7 +979,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > > > > > > > >  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
> > > > > > > > >  	if (delete_memmap(memmap_p, &nr_memmap, elfcorehdr, memsz) < 0)
> > > > > > > > >  		return -1;
> > > > > > > > > -	cmdline_add_memmap(mod_cmdline, memmap_p);
> > > > > > > > > +	if (arch_options.pass_memmap_cmdline)
> > > > > > > > > +		cmdline_add_memmap(mod_cmdline, memmap_p);
> > > > > > > > >  	if (!bzImage_support_efi_boot)
> > > > > > > > >  		cmdline_add_efi(mod_cmdline);
> > > > > > > > >  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
> > > > > > > > > @@ -995,7 +996,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > > > > > > > >  		type = mem_range[i].type;
> > > > > > > > >  		size = end - start + 1;
> > > > > > > > >  		add_memmap(memmap_p, &nr_memmap, start, size, type);
> > > > > > > > > -		cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > > > > > > > > +		if (arch_options.pass_memmap_cmdline)
> > > > > > > > > +			cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > > > > > > > 
> > > > > > > > Seems memmap_p contains the acpi ranges as well, so cmdline_add_memmap_acpi is
> > > > > > > > not necessary anymore, just improve cmdline_add_memmap to add both RAM and ACPI
> > > > > > > > ranges is enough.
> > > > > > > 
> > > > > > > CRASH_MAX_MEMMAP_NR is used in cmdline_add_memmap, it does not include the acpi
> > > > > > > ranges, it looks strange, but anyway there's checking about cmdline overflow, tos
> > > > > > > I think maybe this CRASH_MAX_MEMMAP_NR can be dropped.
> > > > > > 
> > > > > > First, naming cmdline_add_memmap is not accurate regarding what the
> > > > > > function does (adding RAM only). But it's historical naming issue,
> > > > > > nothing to do with this patch :(
> > > > > > 
> > > > > > I'm sure that could be improved later.
> > > > > > 
> > > > > > Second, about dropping CRASH_MAX_MEMMAP_NR, I'm not sure if it's a good
> > > > > > idea.
> > > > > > 
> > > > > > CRASH_MAX_MEMMAP_NR is used to allocate memmap_p memory:
> > > > > > 
> > > > > > load_crash_segments(){
> > > > > > 	[..]
> > > > > > 	sz = (sizeof(struct memory_range) * CRASH_MAX_MEMMAP_NR);
> > > > > > 	memmap_p = xmalloc(sz);
> > > > > > 	memset(memmap_p, 0, sz);
> > > > > > 	[..]
> > > > > > }
> > > > > > 
> > > > > > And so that every time when we walk through memmap_p,
> > > > > > CRASH_MAX_MEMMAP_NR can be used as a upper boundary.
> > > > > > 
> > > > > > I think maintain CRASH_MAX_MEMMAP_NR is reasonable and necessary based
> > > > > > on the current code base.
> > > > > 
> > > > > Hmm, if it's used for allocate mem range array then how about directly use
> > > > > CRASH_MAX_MEMORY_RANGES which is same in your 5/9.
> > > > 
> > > > I did that on purpose for two reasons:
> > > > 
> > > > - I tend to make the changes as minimal as possible to make reviewer's
> > > >   life easier. So that I choose the same value for CRASH_MAX_MEMMAP_NR
> > > >   as CRASH_MAX_MEMORY_RANGES.
> > > 
> > > As long as the patches are logically clear, it does not matter to me :)
> > 
> > Maybe I should have set CRASH_MAX_MEMMAP_NR to 1024 directly.
> 
> Rethinking about it the hard limit is not good I remember there's report from
> SGI there's a lot of memory ranges so in the patch they increased the max ranges
> to 2048, but the patch was reverted.

Probally you mean this:

commit 8274916
Author: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
Date:   Wed Mar 27 20:42:28 2013 +0800

    Revert: "kexec: lengthen the kernel command line image"

Cliff increased kernel cmdline size restriction in kexec-tools from 2048
to 64K and this patch got reverted since kernel has its restriction.

They wanted to increase kernel cmdline size in order to pass more memmap
to 2nd kernel. Now I have increased the memmap limitation to 1024. It
would be definitely enough.

> 
> Since there's several possible improvements I'm fine with current way in
> your patch, let keep it as is for now.

It comes to me that in the future there are two places that can be
improved or cleaned up. I summarize them as the followings:

- clean up cmdline_add_memmap and cmdline_add_memmap_acpi
- Dropping hard limitation memory maps, like CRASH_MAX_MEMMAP_NR

Do you have additional issues needed to be addressed?

> 
> I will ack the whole series from my point of view. 

And I really appreciate it. Thanks for review.

> 
> > 
> > > 
> > > > 
> > > > - CRASH_MAX_MEMORY_RANGES and CRASH_MAX_MEMMAP_NR are used for
> > > >   allocating different memory_range structures, crash_memory_range vs.
> > > >   memmap_p. Those two different variables are used in different places
> > > >   and serve for different purpose (1st kernel's memmap vs. 2nd kernel's
> > > >   memmap). In long term, I think keeping both macros separately is a
> > > >   better choice because one of them could change for whatever reason.
> > > 
> > > It's unlikely at least now IMO, I think we should even drop the limit
> > > because we have e820 + setup_data.
> > 
> > Do you know if kernel has limitation for setup_data?
> 
> I do not think there's limitation for setup_data as it's a link list..

It's not limited how many memory maps can be passed via setup_data. But
there does have a limitation of how many *sanitized maps* can be stored in kernel.

All e820 maps in e820 and setup_data will be appended to a global
(struct e820map e820).  And that struct e820map has 128 slots.

Thanks
WANG Chao

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

Re: [PATCH v6 9/9] x86: Pass memory range via E820 for kdump

[Dave Young]

> 
> It comes to me that in the future there are two places that can be
> improved or cleaned up. I summarize them as the followings:
> 
> - clean up cmdline_add_memmap and cmdline_add_memmap_acpi
> - Dropping hard limitation memory maps, like CRASH_MAX_MEMMAP_NR

Hmm, kernel E820MAX is 128 in case non EFI like below, likely 1024
is enough, so keep current limitation is good:

#ifdef CONFIG_EFI
#include <linux/numa.h>
#define E820_X_MAX (E820MAX + 3 * MAX_NUMNODES)
#else   /* ! CONFIG_EFI */
#define E820_X_MAX E820MAX
#endif

> 
> Do you have additional issues needed to be addressed?

Another in mind is using one array for both 1st kernel and 2nd kernel
if possible.

Thanks
dave

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

Re: [PATCH v6 9/9] x86: Pass memory range via E820 for kdump

[WANG Chao]

On 04/17/14 at 05:45pm, Dave Young wrote:
> > 
> > It comes to me that in the future there are two places that can be
> > improved or cleaned up. I summarize them as the followings:
> > 
> > - clean up cmdline_add_memmap and cmdline_add_memmap_acpi
> > - Dropping hard limitation memory maps, like CRASH_MAX_MEMMAP_NR
> 
> Hmm, kernel E820MAX is 128 in case non EFI like below, likely 1024
> is enough, so keep current limitation is good:
> 
> #ifdef CONFIG_EFI
> #include <linux/numa.h>
> #define E820_X_MAX (E820MAX + 3 * MAX_NUMNODES)
> #else   /* ! CONFIG_EFI */
> #define E820_X_MAX E820MAX
> #endif

That's ok. I'm also fine with hard code 1024 for now.

> 
> > 
> > Do you have additional issues needed to be addressed?
> 
> Another in mind is using one array for both 1st kernel and 2nd kernel
> if possible.

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

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

[Linn Crosetto]

> Hi, Linn
> 
> Thanks for testing the patch in the past. Do you have chance to test
> this update? 
> 
> This updated patchset changed too much and I want things to work as it's
> used to be on your prototype machine with large number of memory ranges.
> 
> Thanks in advance!
> WANG Chao

I will try to send you some results by early next week.


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

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

[Linn Crosetto]

On Thu, Apr 17, 2014 at 02:24:20PM +0800, WANG Chao wrote:
> On 04/14/14 at 10:55pm, WANG Chao wrote:
> > Hi, All
> > 
> > This patchset enables passing memory map via E820 map on x86 platform instead
> > of memmap=exactmap. It's a better design and will solve the following problem
> > so far:
> > 
> > - kernel cmdline is limited resource and large machines tend to have many
> >     memory ranges that may excceed kernel cmdline limit size.
> > - kASLR doesn't work with memmap=exactmap, because kASLR happens early than
> >     user defined memmap=exactmap takes effect.
> > 
> > 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.
> > 
> > This patchset contains massive updates from the previous one. I take some
> > suggestions from reviewers. I try to split the changes into smaller pieces and
> > keep the whole change as minimal as I can so it wouldn't be too harsh to review
> > the patch.
> > 
> > Any comment is appreciate!
> 
> Hi, Linn
> 
> Thanks for testing the patch in the past. Do you have chance to test
> this update? 
> 
> This updated patchset changed too much and I want things to work as it's
> used to be on your prototype machine with large number of memory ranges.
> 
> Thanks in advance!
> WANG Chao

Tested the series on a system with 200+ entries in the map; both kexec and kdump
(with and without --pass-memmap-cmdline). Thanks for the patches.

Tested-by: Linn Crosetto <linn@hp.com>


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

Re: [PATCH v6 9/9] x86: Pass memory range via E820 for kdump

[Simon Horman]

On Thu, Apr 17, 2014 at 01:58:17PM +0800, Dave Young wrote:
> On 04/17/14 at 01:48pm, WANG Chao wrote:
> > On 04/17/14 at 01:29pm, Dave Young wrote:
> > > On 04/14/14 at 10:55pm, WANG Chao wrote:
> > > > command line size is restricted by kernel, sometimes memmap=exactmap has
> > > > too many memory ranges to pass to cmdline. And also memmap=exactmap and
> > > > kASLR doesn't work together.
> > > > 
> > > > 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   |   6 +-
> > > >  kexec/arch/i386/x86-linux-setup.c | 149 +++++++++++++++++++++++++-------------
> > > >  kexec/arch/i386/x86-linux-setup.h |   1 +
> > > >  3 files changed, 103 insertions(+), 53 deletions(-)
> > > > 
> > > > diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> > > > index 7b618a6..4a1491b 100644
> > > > --- a/kexec/arch/i386/crashdump-x86.c
> > > > +++ b/kexec/arch/i386/crashdump-x86.c
> > > > @@ -979,7 +979,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > > >  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
> > > >  	if (delete_memmap(memmap_p, &nr_memmap, elfcorehdr, memsz) < 0)
> > > >  		return -1;
> > > > -	cmdline_add_memmap(mod_cmdline, memmap_p);
> > > > +	if (arch_options.pass_memmap_cmdline)
> > > > +		cmdline_add_memmap(mod_cmdline, memmap_p);
> > > >  	if (!bzImage_support_efi_boot)
> > > >  		cmdline_add_efi(mod_cmdline);
> > > >  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
> > > > @@ -995,7 +996,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > > >  		type = mem_range[i].type;
> > > >  		size = end - start + 1;
> > > >  		add_memmap(memmap_p, &nr_memmap, start, size, type);
> > > > -		cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > > > +		if (arch_options.pass_memmap_cmdline)
> > > > +			cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > > 
> > > Seems memmap_p contains the acpi ranges as well, so cmdline_add_memmap_acpi is
> > > not necessary anymore, just improve cmdline_add_memmap to add both RAM and ACPI
> > > ranges is enough.
> > 
> > I can do that. But is it what the patchset is really about ...
> > 
> > I'm not keen about doing too much cleanup in this series now since it's
> > already v6. I really want to get this in as early as possible to
> > cope with calgary iommu change in the kernel.
> > 
> > I prefer to first get this patch in if there's no problem in it and then
> > look back and think about how we can clean up the code block which have
> > been there for historical reason.
> 
> I think the cleanup is worth, but if you want to do it later I'm fine.
> So should better leave the patch 5/9 to later cleanup as well.
> 
> Thus except for 5/9, for other patches:
> Acked-by: Dave Young <dyoung@redhat.com>

This sounds fine to me.

Chao, would you care to re-post the series without 5/9 ?

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

Re: [PATCH v6 9/9] x86: Pass memory range via E820 for kdump

[WANG Chao]

On 04/21/14 at 09:01am, Simon Horman wrote:
> On Thu, Apr 17, 2014 at 01:58:17PM +0800, Dave Young wrote:
> > On 04/17/14 at 01:48pm, WANG Chao wrote:
> > > On 04/17/14 at 01:29pm, Dave Young wrote:
> > > > On 04/14/14 at 10:55pm, WANG Chao wrote:
> > > > > command line size is restricted by kernel, sometimes memmap=exactmap has
> > > > > too many memory ranges to pass to cmdline. And also memmap=exactmap and
> > > > > kASLR doesn't work together.
> > > > > 
> > > > > 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   |   6 +-
> > > > >  kexec/arch/i386/x86-linux-setup.c | 149 +++++++++++++++++++++++++-------------
> > > > >  kexec/arch/i386/x86-linux-setup.h |   1 +
> > > > >  3 files changed, 103 insertions(+), 53 deletions(-)
> > > > > 
> > > > > diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> > > > > index 7b618a6..4a1491b 100644
> > > > > --- a/kexec/arch/i386/crashdump-x86.c
> > > > > +++ b/kexec/arch/i386/crashdump-x86.c
> > > > > @@ -979,7 +979,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > > > >  	dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
> > > > >  	if (delete_memmap(memmap_p, &nr_memmap, elfcorehdr, memsz) < 0)
> > > > >  		return -1;
> > > > > -	cmdline_add_memmap(mod_cmdline, memmap_p);
> > > > > +	if (arch_options.pass_memmap_cmdline)
> > > > > +		cmdline_add_memmap(mod_cmdline, memmap_p);
> > > > >  	if (!bzImage_support_efi_boot)
> > > > >  		cmdline_add_efi(mod_cmdline);
> > > > >  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
> > > > > @@ -995,7 +996,8 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> > > > >  		type = mem_range[i].type;
> > > > >  		size = end - start + 1;
> > > > >  		add_memmap(memmap_p, &nr_memmap, start, size, type);
> > > > > -		cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > > > > +		if (arch_options.pass_memmap_cmdline)
> > > > > +			cmdline_add_memmap_acpi(mod_cmdline, start, end);
> > > > 
> > > > Seems memmap_p contains the acpi ranges as well, so cmdline_add_memmap_acpi is
> > > > not necessary anymore, just improve cmdline_add_memmap to add both RAM and ACPI
> > > > ranges is enough.
> > > 
> > > I can do that. But is it what the patchset is really about ...
> > > 
> > > I'm not keen about doing too much cleanup in this series now since it's
> > > already v6. I really want to get this in as early as possible to
> > > cope with calgary iommu change in the kernel.
> > > 
> > > I prefer to first get this patch in if there's no problem in it and then
> > > look back and think about how we can clean up the code block which have
> > > been there for historical reason.
> > 
> > I think the cleanup is worth, but if you want to do it later I'm fine.
> > So should better leave the patch 5/9 to later cleanup as well.
> > 
> > Thus except for 5/9, for other patches:
> > Acked-by: Dave Young <dyoung@redhat.com>

Hi, Simon

Later, after a discussion with Dave Young, he is ok with 5/9. Here's his
ACK for the whole patch series:

http://lists.infradead.org/pipermail/kexec/2014-April/011615.html

I'll explain why 5/9 is necessary below.

> 
> This sounds fine to me.

Patch 5/9 increased memmap_p from a small array to a large one. That
patch is necessary.

Originally memmap_p was used to store the RANGE_RAM only for 2nd kernel
and that was fine for memmap_p to be small, since we only have several
memory range of RANGE_RAM to be passed to 2nd kernel.

However now memmap_p will be used to store all types of memory ranges to
pass to 2nd kernel, which means not only RANGE_RAM but also RANGE_ACPI
and RANGE_ACPI_NVS (probably RANGE_RESERVED in the future). That small
memmap_p is not likely to be enough to contain all the memory ranges. So
that's why we should increase CRASH_MAX_MEMMAP_NR to about 1024 to adapt
the change of memmap_p will contains all type of memory ranges.

Thanks
WANG Chao

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

Re: [PATCH v6 9/9] x86: Pass memory range via E820 for kdump

[Dave Young]

[snip]

> > > > I prefer to first get this patch in if there's no problem in it and then
> > > > look back and think about how we can clean up the code block which have
> > > > been there for historical reason.
> > > 
> > > I think the cleanup is worth, but if you want to do it later I'm fine.
> > > So should better leave the patch 5/9 to later cleanup as well.
> > > 
> > > Thus except for 5/9, for other patches:
> > > Acked-by: Dave Young <dyoung@redhat.com>
> 
> Hi, Simon
> 
> Later, after a discussion with Dave Young, he is ok with 5/9. Here's his
> ACK for the whole patch series:
> 
> http://lists.infradead.org/pipermail/kexec/2014-April/011615.html
> 
> I'll explain why 5/9 is necessary below.

Yes, it's fine to me for the whole series after more discussion as Chao
mentioned below. In the future if kexec and kdump cases can use just one
array then this macro can be removed that time. 


> Patch 5/9 increased memmap_p from a small array to a large one. That
> patch is necessary.
> 
> Originally memmap_p was used to store the RANGE_RAM only for 2nd kernel
> and that was fine for memmap_p to be small, since we only have several
> memory range of RANGE_RAM to be passed to 2nd kernel.
> 
> However now memmap_p will be used to store all types of memory ranges to
> pass to 2nd kernel, which means not only RANGE_RAM but also RANGE_ACPI
> and RANGE_ACPI_NVS (probably RANGE_RESERVED in the future). That small
> memmap_p is not likely to be enough to contain all the memory ranges. So
> that's why we should increase CRASH_MAX_MEMMAP_NR to about 1024 to adapt
> the change of memmap_p will contains all type of memory ranges.

Thanks
Dave

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