x86, boot, 64bit: Add support for loading ramdisk and bzImage above 4G

During debugging loading kernel above 4G, found one page if is not used in BRK with early page allocation.

pgt_buf_top is address that can not be used, so should check if that new end is above that top, otherwise last page will not be used.

Fix that checking and also add print out for every allocation from BRK.

Quoted 7 lines · Yinghai Lu

During debugging loading kernel above 4G, found one page if is not used in BRK with early page allocation.

pgt_buf_top is address that can not be used, so should check if that new end is above that top, otherwise last page will not be used.

Fix that checking and also add print out for every allocation from BRK.

This commit message still bothers the hell out of me. Please, fix it up to something more readable like the below, for example:

"pgt_buf_top is an address which cannot be used so we should check whether the new 'end' is above it. Otherwise, the last BRK page remains unused.

Fix that check and add a debug printout of every BRK allocation."

Quoted 16 lines · Borislav Petkov

Quoted 7 lines · Yinghai Lu

During debugging loading kernel above 4G, found one page if is not used in BRK with early page allocation.

pgt_buf_top is address that can not be used, so should check if that new end is above that top, otherwise last page will not be used.

Fix that checking and also add print out for every allocation from BRK.

This commit message still bothers the hell out of me. Please, fix it up to something more readable like the below, for example:

"pgt_buf_top is an address which cannot be used so we should check whether the new 'end' is above it. Otherwise, the last BRK page remains unused.

Quoted 1 line

Fix that check and add a debug printout of every BRK allocation."

but your changelog is wrong.

it is NOT last BRK page.

it is NOT every BRK allocation.

During debugging loading kernel above 4G, found one page if is not used in BRK with early page allocation.

pgt_buf_top is address that can not be used, so should check if that new end is above that top, otherwise last page will not be used.

Fix that checking and also add print out for every allocation from BRK.

Signed-off-by Yinghai Lu <[email protected]>

 arch/x86/mm/init.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

arch/x86/mm/init.c +3 -1

index 6f85de8..c4293cf 100644
@@ -47,7 +47,7 @@ __ref void *alloc_low_pages(unsigned int num)
 						__GFP_ZERO, order);
 	}
 
-	if ((pgt_buf_end + num) >= pgt_buf_top) {
+	if ((pgt_buf_end + num) > pgt_buf_top) {
 		unsigned long ret;
 		if (min_pfn_mapped >= max_pfn_mapped)
 			panic("alloc_low_page: ran out of memory");

@@ -61,6 +61,8 @@ __ref void *alloc_low_pages(unsigned int num)
 	} else {
 		pfn = pgt_buf_end;
 		pgt_buf_end += num;
+		printk(KERN_DEBUG "BRK [%#010lx, %#010lx] PGTABLE\n",
+			pfn << PAGE_SHIFT, (pgt_buf_end << PAGE_SHIFT) - 1);
 	}
 
 	for (i = 0; i < num; i++) {

Signature · 7 lines

1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Trampoline code is executed by APs with kernel low mapping. We need to set trampoline code to EXEC early before we do smp AP bootings.

Found the problem after switching to #PF handler set page table, and we do not set initial kernel low mapping with EXEC anymore in

arch/x86/kernel/head_64.S.

Change to use early_initcall instead that will make sure tramopline

will have EXEC set.

Signed-off-by Yinghai Lu <[email protected]>

---
 arch/x86/realmode/init.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

arch/x86/realmode/init.c +0 -0

index 8045026..b96fe6f 100644
@@ -111,5 +111,9 @@ static int __init set_real_mode_permissions(void)
 

/*
- * set_real_mode_permissions() gets called very early, to guarantee the
- * availability of low memory. This is before the proper kernel page
- * tables are set up, so we cannot set page permissions in that
- * function. Thus, we use an arch_initcall instead.
+ * This function gets called very early to guarantee the availability
+ * of low memory. This is even before the proper kernel page tables are
+ * set up, so we cannot set page permissions in that function. However,
+ * trampoline code will be executed by APs so we need it to be marked
+ * executable at pre-SMP time, thus run it as a early_initcall(). */

more than that, that set_real_mode_permissions reference is wrong, actually it is set_real_mode.

Quoted 2 lines · Yinghai Lu

more than that, that set_real_mode_permissions reference is wrong, actually it is set_real_mode.

Huh, set_real_mode_permissions is the name of the function above which the comment is located. There's no set_real_mode. What do you mean?

Quoted 4 lines · Borislav Petkov

Quoted 2 lines · Yinghai Lu

more than that, that set_real_mode_permissions reference is wrong, actually it is set_real_mode.

Quoted 2 lines

Huh, set_real_mode_permissions is the name of the function above which the comment is located. There's no set_real_mode. What do you mean?

old comments is wrong.

setup_read_mode reserve from low ram under 1M and copy etc.

set_real_mode_permissions will change to +x etc....

Or do you mean that the function naming is wrong? WTF?

arch/x86/mm/init_64.c line 146

Quoted 9 lines · Yinghai Lu

arch/x86/mm/init_64.c +9 -0

+int kernel_mapping_init(pgd_t *pgd_page, unsigned long addr, unsigned long end)
+{
+     struct x86_mapping_info info = {
+             .alloc_pgt_page = alloc_pgt_page,
+             .pmd_flag       = __PAGE_KERNEL_LARGE,
+             .kernel_mapping = true,
+     };
+
+     return kernel_ident_mapping_init(&info, pgd_page, addr, end);

This patch looks good so far except this:
kernel_ident_mapping_init says it initializes ident mapping but this is wrong and the type of mapping is actually controlled by info.kernel_mapping.

So this function which gets &info, etc should be called kernel_mapping_init, AFAICT. And wrt the one wrapping kernel_ident_mapping_init, I can't seem to find where it is called. What's up?

Quoted 15 lines · Borislav Petkov

arch/x86/mm/init_64.c line 146

Quoted 9 lines · Yinghai Lu

arch/x86/mm/init_64.c +9 -0

+int kernel_mapping_init(pgd_t *pgd_page, unsigned long addr, unsigned long end)
+{
+     struct x86_mapping_info info = {
+             .alloc_pgt_page = alloc_pgt_page,
+             .pmd_flag       = __PAGE_KERNEL_LARGE,
+             .kernel_mapping = true,
+     };
+
+     return kernel_ident_mapping_init(&info, pgd_page, addr, end);

This patch looks good so far except this:
kernel_ident_mapping_init says it initializes ident mapping but this is wrong and the type of mapping is actually controlled by info.kernel_mapping.

it is not wrong, and it could do two things.
kernel_mapping
ident_mapping

Quoted 4 lines

So this function which gets &info, etc should be called kernel_mapping_init, AFAICT. And wrt the one wrapping kernel_ident_mapping_init, I can't seem to find where it is called. What's up?

this kernel_mapping_init is for -v8 ..., should be dropped if -v7 is used at last.

real_mode_data aka zero-page could be above 4g.

We will have #PF handler to set page table for not accessible ram early, but could limit it before x86_64_start_reservations to limit the change to native path.

Also we will need to ramdisk info in zero-page to access microcode

blob in ramdisk in x86_64_start_kernel, so copy zero-page early make

it accessing ramdisk info simple.

Signed-off-by Yinghai Lu <[email protected]>

Cc Alexander Duyck <[email protected]>

Cc Fenghua Yu <[email protected]>

with #PF handler way to set early page table, level3_ident will go away with 64bit native path.

So just use entries in init_level4_pgt to set them in tramopline_pgt

Signed-off-by Yinghai Lu <[email protected]>

Cc Jarkko Sakkinen <[email protected]>

---
 arch/x86/realmode/init.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/x86/realmode/init.c +0 -0

index b96fe6f..384b3f4 100644

Quoted 7 lines · Yinghai Lu

with #PF handler way to set early page table, level3_ident will go away with 64bit native path.

So just use entries in init_level4_pgt to set them in tramopline_pgt

Signed-off-by Yinghai Lu <[email protected]>

Cc Jarkko Sakkinen <[email protected]>

Acked-by Jarkko Sakkinen <[email protected]>

Quoted 6 lines · Jarkko

Quoted 5 lines · Yinghai Lu

with #PF handler way to set early page table, level3_ident will go away with 64bit native path.

So just use entries in init_level4_pgt to set them in tramopline_pgt

Quoted 4 lines

Quoted 2 lines

Signed-off-by Yinghai Lu <[email protected]>

Cc Jarkko Sakkinen <[email protected]>

Acked-by Jarkko Sakkinen <[email protected]>

Thanks.

updated the patch, and would save some time for HPA.

with #PF handler way to set early page table, level3_ident will go away with 64bit native path.

So just use entries in init_level4_pgt to set them in tramopline_pgt

Signed-off-by Yinghai Lu <[email protected]>

Cc Jarkko Sakkinen <[email protected]>

---
 arch/x86/realmode/init.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/x86/realmode/init.c +2 -2

index b96fe6f..384b3f4 100644
@@ -78,8 +78,8 @@ void __init setup_real_mode(void)
 	*trampoline_cr4_features = read_cr4();
 
 	trampoline_pgd = (u64 *) __va(real_mode_header->trampoline_pgd);
-	trampoline_pgd[0] = __pa_symbol(level3_ident_pgt) + _KERNPG_TABLE;
-	trampoline_pgd[511] = __pa_symbol(level3_kernel_pgt) + _KERNPG_TABLE;
+	trampoline_pgd[0] = init_level4_pgt[pgd_index(__PAGE_OFFSET)].pgd;
+	trampoline_pgd[511] = init_level4_pgt[511].pgd;
 #endif
 }
 

Signature · 1 line

1.7.10.4

After we switch to use #PF handler help to set page table, init_level4_pgt will only have entries set after init_mem_mapping.
We need to move copying init_level4_pgt to trampoline_pgd after that.

So split reserve and setup, and move the setup after init_mem_mapping()

Signed-off-by Yinghai Lu <[email protected]>

Cc Jarkko Sakkinen <[email protected]>

---
 arch/x86/include/asm/realmode.h |    3 ++-
 arch/x86/kernel/setup.c         |    4 +++-
 arch/x86/realmode/init.c        |   30 +++++++++++++++++++-----------
 3 files changed, 24 insertions(+), 13 deletions(-)

arch/x86/include/asm/realmode.h +2 -1

index fe1ec5b..9c6b890 100644
@@ -58,6 +58,7 @@ extern unsigned char boot_gdt[];
 extern unsigned char secondary_startup_64[];
 #endif
 
-extern void __init setup_real_mode(void);
+void reserve_real_mode(void);
+void setup_real_mode(void);
 
 #endif /* _ARCH_X86_REALMODE_H */

arch/x86/kernel/setup.c +3 -1

index 81ea5a5..01b22d0 100644
@@ -913,10 +913,12 @@ void __init setup_arch(char **cmdline_p)
 	printk(KERN_DEBUG "initial memory mapped: [mem 0x00000000-%#010lx]\n",
 			(max_pfn_mapped<<PAGE_SHIFT) - 1);
 
-	setup_real_mode();
+	reserve_real_mode();
 
 	init_mem_mapping();
 
+	setup_real_mode();
+
 	memblock.current_limit = get_max_mapped();
 	dma_contiguous_reserve(0);
 

arch/x86/realmode/init.c +18 -5

index 384b3f4..3baae96 100644
@@ -8,9 +8,26 @@
 struct real_mode_header *real_mode_header;
 u32 *trampoline_cr4_features;
 
-void __init setup_real_mode(void)
+void __init reserve_real_mode(void)
 {
 	phys_addr_t mem;
+	unsigned char *base;
+	size_t size = PAGE_ALIGN(real_mode_blob_end - real_mode_blob);
+
+	/* Has to be in very low memory so we can execute real-mode AP code. */
+	mem = memblock_find_in_range(0, 1<<20, size, PAGE_SIZE);
+	if (!mem)
+		panic("Cannot allocate trampoline\n");
+
+	base = __va(mem);
+	memblock_reserve(mem, size);
+	real_mode_header = (struct real_mode_header *) base;
+	printk(KERN_DEBUG "Base memory trampoline at [%p] %llx size %zu\n",
+	       base, (unsigned long long)mem, size);
+}
+
+void __init setup_real_mode(void)
+{
 	u16 real_mode_seg;
 	u32 *rel;
 	u32 count;

@@ -25,16 +42,7 @@ void __init setup_real_mode(void)
 	u64 efer;
 #endif
 
-	/* Has to be in very low memory so we can execute real-mode AP code. */
-	mem = memblock_find_in_range(0, 1<<20, size, PAGE_SIZE);
-	if (!mem)
-		panic("Cannot allocate trampoline\n");

After we switch to use #PF handler help to set page table, init_level4_pgt will only have entries set after init_mem_mapping.
We need to move copying init_level4_pgt to trampoline_pgd after that.

So split reserve and setup, and move the setup after init_mem_mapping()

Signed-off-by Yinghai Lu <[email protected]>

Cc Jarkko Sakkinen <[email protected]>

---
 arch/x86/include/asm/realmode.h |    3 ++-
 arch/x86/kernel/setup.c         |    4 +++-
 arch/x86/realmode/init.c        |   30 +++++++++++++++++++-----------
 3 files changed, 24 insertions(+), 13 deletions(-)

arch/x86/include/asm/realmode.h +2 -1

index fe1ec5b..9c6b890 100644
@@ -58,6 +58,7 @@ extern unsigned char boot_gdt[];
 extern unsigned char secondary_startup_64[];
 #endif
 
-extern void __init setup_real_mode(void);
+void reserve_real_mode(void);
+void setup_real_mode(void);
 
 #endif /* _ARCH_X86_REALMODE_H */

arch/x86/kernel/setup.c +3 -1

index 81ea5a5..01b22d0 100644
@@ -913,10 +913,12 @@ void __init setup_arch(char **cmdline_p)
 	printk(KERN_DEBUG "initial memory mapped: [mem 0x00000000-%#010lx]\n",
 			(max_pfn_mapped<<PAGE_SHIFT) - 1);
 
-	setup_real_mode();
+	reserve_real_mode();
 
 	init_mem_mapping();
 
+	setup_real_mode();
+
 	memblock.current_limit = get_max_mapped();
 	dma_contiguous_reserve(0);
 

arch/x86/realmode/init.c +0 -0

index 384b3f4..3baae96 100644
@@ -8,9 +8,26 @@
 struct real_mode_header *real_mode_header;

From: "H. Peter Anvin" <[email protected]>

two use cases:
1. We will support load and run kernel above 4G, and zero_page, ramdisk will be above 4G, too
2. need to access ramdisk early to get microcode to update that as early possible.

We could use early_iomap to access them, but it will make code to

messy and hard to unified with 32bit.

So here comes #PF handler to set page page.

When #PF happen, handler will use pages in __initdata to set page page

to cover accessed page.

those code and page in __INIT sections, so will not increase ram usages.

The good point is: with help of #PF handler, we can set kernel mapping from blank, and switch to init_level4_pgt later.

switchover in head_64.S is only using three page to handle kernel crossing 1G, 512G with shareing page, most insteresting part.

early_make_pgtable is using kernel high mapping address to access pages to set page table.

-v4: Add phys_base offset to make kexec happy, and add
init_mapping_kernel() - Yinghai

(quoted diff) +0 -1

-v5: fix compiling with xen, and add back ident level3 and level2 for xen
     also move back init_level4_pgt from BSS to DATA again.
     because we have to clear it anyway.  - Yinghai

Signed-off-by Yinghai Lu <[email protected]>

Those -vXX version lines need to go under the "---" line. Alternatively, you might want to add some of them to the commit message with a proper explanation since they are not that trivial at a first glance, for example the -v5, -v6, -v8, -v9 with a better explanation.

Quoted 2 lines

This needs hpa's S-O-B.

Quoted 4 lines · Borislav Petkov

Those -vXX version lines need to go under the "---" line. Alternatively, you might want to add some of them to the commit message with a proper explanation since they are not that trivial at a first glance, for example the -v5, -v6, -v8, -v9 with a better explanation.

mostly they are for tracking version.

Quoted 1 line

This needs hpa's S-O-B.

he will add later when he put the in the tip.

This is not how SOB chaining works:

SOB: Author
SOB: Handler - this is you, who has added it to the patchset
SOB: Committer - maintainer

You need to read Documentation/SubmittingPatches if there's still things unclear.

Quoted 8 lines · Borislav Petkov

This is not how SOB chaining works:

SOB: Author
SOB: Handler - this is you, who has added it to the patchset
SOB: Committer - maintainer

You need to read Documentation/SubmittingPatches if there's still things unclear.

Really don't know what you are doing here.

We did that before for a long time.

During reviewing some patches, Linus or HPA or Eric has better idea and drafted some patch,
without their Signed-offs.

then first version submitter will continue the debugging and testing and make the patch working.

At last the submit the patch with authorship from Linus or HPA or Eric.

So at that time how can the Signed-off from them?

And there are commits in the upstream does not have Signed-off from the Author.

Quoted 4 lines

So at that time how can the Signed-off from them?

And there are commits in the upstream does not have Signed-off from the Author.

I certainly hope those are a very very small number, if any.

Now #PF hanlder could map 1G per #PF, That causes same problem that is fixed by
x86, mm: Only direct map addresses that are marked as E820_RAM

only add one 2M mapping instead of 1G accessing one time for dynamically per #PF.

We are not having max_pfn_mapped set correctly until init_memory_mapping.

so don't print it initial value for 64bit

Also need to use KERNEL_IMAGE_SIZE directly for highmap cleanup.

Signed-off-by Yinghai Lu <[email protected]>

Quoted 3 lines · Yinghai Lu

We are not having max_pfn_mapped set correctly until init_memory_mapping.

so don't print it initial value for 64bit

Just a minor nitpick:

"So don't print its initial value for 64bit."

And you don't need the newlines between each sentence.

Acked-by Borislav Petkov <[email protected]>

Just want to make it more readable.

Also, congratulation for the new job.

We are not having max_pfn_mapped set correctly until init_memory_mapping.

so don't print it initial value for 64bit

Also need to use KERNEL_IMAGE_SIZE directly for highmap cleanup.

Signed-off-by Yinghai Lu <[email protected]>

---
 arch/x86/kernel/head64.c |    3 ---
 arch/x86/kernel/setup.c  |    2 ++
 arch/x86/mm/init_64.c    |    6 +++++-
 3 files changed, 7 insertions(+), 4 deletions(-)

arch/x86/kernel/head64.c +0 -3

index a3fc233..7061d8b 100644
@@ -146,9 +146,6 @@ void __init x86_64_start_kernel(char * real_mode_data)
 	/* clear bss before set_intr_gate with early_idt_handler */
 	clear_bss();
 
-	/* XXX - this is wrong... we need to build page tables from scratch */
-	max_pfn_mapped = KERNEL_IMAGE_SIZE >> PAGE_SHIFT;
-
 	for (i = 0; i < NUM_EXCEPTION_VECTORS; i++) {
 #ifdef CONFIG_EARLY_PRINTK
 		set_intr_gate(i, &early_idt_handlers[i]);

arch/x86/kernel/setup.c +2 -0

index 63160c6..04797e78 100644
@@ -910,8 +910,10 @@ void __init setup_arch(char **cmdline_p)
 	setup_bios_corruption_check();
 #endif
 
+#ifdef CONFIG_X86_32
 	printk(KERN_DEBUG "initial memory mapped: [mem 0x00000000-%#010lx]\n",
 			(max_pfn_mapped<<PAGE_SHIFT) - 1);
+#endif
 
 	reserve_real_mode();
 

arch/x86/mm/init_64.c +0 -0

index 9c5f2b1..98385a2 100644
@@ -394,10 +394,14 @@ void __init init_extra_mapping_uc(unsigned long phys, unsigned long size)
 void __init cleanup_highmap(void)

arch/x86/mm/init_64.c +0 -0

 	for (; vaddr + PMD_SIZE - 1 < vaddr_end; pmd++, vaddr += PMD_SIZE) {
 		if (pmd_none(*pmd))
 			continue;

Signature · 7 lines

1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 55 lines · Yinghai Lu

We are not having max_pfn_mapped set correctly until init_memory_mapping.

so don't print it initial value for 64bit

Also need to use KERNEL_IMAGE_SIZE directly for highmap cleanup.

Signed-off-by Yinghai Lu <[email protected]>

---
 arch/x86/kernel/head64.c |    3 ---
 arch/x86/kernel/setup.c  |    2 ++
 arch/x86/mm/init_64.c    |    6 +++++-
 3 files changed, 7 insertions(+), 4 deletions(-)

arch/x86/kernel/head64.c +0 -3

index a3fc233..7061d8b 100644
@@ -146,9 +146,6 @@ void __init x86_64_start_kernel(char * real_mode_data)
 	/* clear bss before set_intr_gate with early_idt_handler */
 	clear_bss();
 
-	/* XXX - this is wrong... we need to build page tables from scratch */
-	max_pfn_mapped = KERNEL_IMAGE_SIZE >> PAGE_SHIFT;
-
 	for (i = 0; i < NUM_EXCEPTION_VECTORS; i++) {
 #ifdef CONFIG_EARLY_PRINTK
 		set_intr_gate(i, &early_idt_handlers[i]);

arch/x86/kernel/setup.c +2 -0

index 63160c6..04797e78 100644
@@ -910,8 +910,10 @@ void __init setup_arch(char **cmdline_p)
 	setup_bios_corruption_check();
 #endif
 
+#ifdef CONFIG_X86_32
 	printk(KERN_DEBUG "initial memory mapped: [mem 0x00000000-%#010lx]\n",
 			(max_pfn_mapped<<PAGE_SHIFT) - 1);
+#endif
 
 	reserve_real_mode();
 

arch/x86/mm/init_64.c +1 -1

index 9c5f2b1..98385a2 100644
@@ -394,10 +394,14 @@ void __init init_extra_mapping_uc(unsigned long phys, unsigned long size)
 void __init cleanup_highmap(void)
 {
 	unsigned long vaddr = __START_KERNEL_map;
-	unsigned long vaddr_end = __START_KERNEL_map + (max_pfn_mapped << PAGE_SHIFT);
+	unsigned long vaddr_end = __START_KERNEL_map + KERNEL_IMAGE_SIZE;
 	unsigned long end = roundup((unsigned long)_brk_end, PMD_SIZE) - 1;
 	pmd_t *pmd = level2_kernel_pgt;
 

arch/x86/mm/init_64.c void __init init_extra_mapping_uc(unsigned long phys, unsigned long size) · line 403

Quoted 3 lines

arch/x86/mm/init_64.c +3 -0

+	/* Xen has its own end somehow with abused max_pfn_mapped */
+	if (max_pfn_mapped)
+		vaddr_end = __START_KERNEL_map + (max_pfn_mapped << PAGE_SHIFT);

If you are going to put a comment like that in the code, could you please at least add some useful details, rather than a generic "somehow"? It doesn't seem very helpful to me or to any other hackers looking at the code.

The issue is even described as a comment in the code at the beginning of arch/x86/xen/mmu.c:xen_setup_kernel_pagetable:

/* max_pfn_mapped is the last pfn mapped in the initial memory
* mappings. Considering that on Xen after the kernel mappings we
* have the mappings of some pages that don't exist in pfn space, we
* set max_pfn_mapped to the last real pfn mapped. */

Now if max_pfn_mapped is supposed to represent the last pfn mapped in the initial memory mapping, then I think that the way Xen uses max_pfn_mapped is actually correct.

The question is: has max_pfn_mapped actually changed meaning? Because if it hasn't I don't see why you need this change.

Quoted 12 lines

arch/x86/mm/init_64.c +0 -0

 	for (; vaddr + PMD_SIZE - 1 < vaddr_end; pmd++, vaddr += PMD_SIZE) {
 		if (pmd_none(*pmd))
 			continue;

Signature · 7 lines

1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 56 lines · Stefano Stabellini

Quoted 55 lines · Yinghai Lu

We are not having max_pfn_mapped set correctly until init_memory_mapping.

so don't print it initial value for 64bit

Also need to use KERNEL_IMAGE_SIZE directly for highmap cleanup.

Signed-off-by Yinghai Lu <[email protected]>

---
 arch/x86/kernel/head64.c |    3 ---
 arch/x86/kernel/setup.c  |    2 ++
 arch/x86/mm/init_64.c    |    6 +++++-
 3 files changed, 7 insertions(+), 4 deletions(-)

arch/x86/kernel/head64.c +0 -3

index a3fc233..7061d8b 100644
@@ -146,9 +146,6 @@ void __init x86_64_start_kernel(char * real_mode_data)
 	/* clear bss before set_intr_gate with early_idt_handler */
 	clear_bss();
 
-	/* XXX - this is wrong... we need to build page tables from scratch */
-	max_pfn_mapped = KERNEL_IMAGE_SIZE >> PAGE_SHIFT;
-
 	for (i = 0; i < NUM_EXCEPTION_VECTORS; i++) {
 #ifdef CONFIG_EARLY_PRINTK
 		set_intr_gate(i, &early_idt_handlers[i]);

arch/x86/kernel/setup.c +2 -0

index 63160c6..04797e78 100644
@@ -910,8 +910,10 @@ void __init setup_arch(char **cmdline_p)
 	setup_bios_corruption_check();
 #endif
 
+#ifdef CONFIG_X86_32
 	printk(KERN_DEBUG "initial memory mapped: [mem 0x00000000-%#010lx]\n",
 			(max_pfn_mapped<<PAGE_SHIFT) - 1);
+#endif
 
 	reserve_real_mode();
 

arch/x86/mm/init_64.c +1 -1

index 9c5f2b1..98385a2 100644
@@ -394,10 +394,14 @@ void __init init_extra_mapping_uc(unsigned long phys, unsigned long size)
 void __init cleanup_highmap(void)
 {
 	unsigned long vaddr = __START_KERNEL_map;
-	unsigned long vaddr_end = __START_KERNEL_map + (max_pfn_mapped << PAGE_SHIFT);
+	unsigned long vaddr_end = __START_KERNEL_map + KERNEL_IMAGE_SIZE;
 	unsigned long end = roundup((unsigned long)_brk_end, PMD_SIZE) - 1;
 	pmd_t *pmd = level2_kernel_pgt;
 

Quoted 8 lines

arch/x86/mm/init_64.c void __init init_extra_mapping_uc(unsigned long phys, unsigned long size) · line 403

Quoted 3 lines

arch/x86/mm/init_64.c +3 -0

+	/* Xen has its own end somehow with abused max_pfn_mapped */
+	if (max_pfn_mapped)
+		vaddr_end = __START_KERNEL_map + (max_pfn_mapped << PAGE_SHIFT);

If you are going to put a comment like that in the code, could you please at least add some useful details, rather than a generic "somehow"? It doesn't seem very helpful to me or to any other hackers looking at the code.

Hm I think I actually pointed out to him in the previous reviews how we alter it and that he should ingest some of those comments in this patch.

Quoted 30 lines

The issue is even described as a comment in the code at the beginning of arch/x86/xen/mmu.c:xen_setup_kernel_pagetable:

/* max_pfn_mapped is the last pfn mapped in the initial memory
* mappings. Considering that on Xen after the kernel mappings we
* have the mappings of some pages that don't exist in pfn space, we
* set max_pfn_mapped to the last real pfn mapped. */

Now if max_pfn_mapped is supposed to represent the last pfn mapped in the initial memory mapping, then I think that the way Xen uses max_pfn_mapped is actually correct.

The question is: has max_pfn_mapped actually changed meaning? Because if it hasn't I don't see why you need this change.

Quoted 12 lines

arch/x86/mm/init_64.c +0 -0

 	for (; vaddr + PMD_SIZE - 1 < vaddr_end; pmd++, vaddr += PMD_SIZE) {
 		if (pmd_none(*pmd))
 			continue;

Signature · 7 lines

1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

If you are going to put a comment like that in the code, could you please at least add some useful details, rather than a generic "somehow"? It doesn't seem very helpful to me or to any other hackers looking at the code.

The issue is even described as a comment in the code at the beginning of arch/x86/xen/mmu.c:xen_setup_kernel_pagetable:

/* max_pfn_mapped is the last pfn mapped in the initial memory
* mappings. Considering that on Xen after the kernel mappings we
* have the mappings of some pages that don't exist in pfn space, we
* set max_pfn_mapped to the last real pfn mapped. */

Now if max_pfn_mapped is supposed to represent the last pfn mapped in the initial memory mapping, then I think that the way Xen uses max_pfn_mapped is actually correct.

There are several places to find ramdisk information early for reserving and relocating.

Use functions to make code more readable and consistent.

Later will add ext_ramdisk_image/size in those functions to support loading ramdisk above 4g.

Signed-off-by Yinghai Lu <[email protected]>

---
 arch/x86/kernel/setup.c |   29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

arch/x86/kernel/setup.c +0 -0

index 1b8a8cc..644a123 100644

arch/x86/kernel/setup.c +12 -8

+
+	return ramdisk_size;
+}
+
 #define MAX_MAP_CHUNK	(NR_FIX_BTMAPS << PAGE_SHIFT)
 static void __init relocate_initrd(void)
 {
 	/* Assume only end is not page aligned */
-	u64 ramdisk_image = boot_params.hdr.ramdisk_image;
-	u64 ramdisk_size  = boot_params.hdr.ramdisk_size;
+	u64 ramdisk_image = get_ramdisk_image();
+	u64 ramdisk_size  = get_ramdisk_size();
 	u64 area_size     = PAGE_ALIGN(ramdisk_size);
 	u64 ramdisk_here;
 	unsigned long slop, clen, mapaddr;

@@ -338,8 +351,8 @@ static void __init relocate_initrd(void)
 		ramdisk_size  -= clen;
 	}
 
-	ramdisk_image = boot_params.hdr.ramdisk_image;
-	ramdisk_size  = boot_params.hdr.ramdisk_size;
+	ramdisk_image = get_ramdisk_image();
+	ramdisk_size  = get_ramdisk_size();
 	printk(KERN_INFO "Move RAMDISK from [mem %#010llx-%#010llx] to"
 		" [mem %#010llx-%#010llx]\n",
 		ramdisk_image, ramdisk_image + ramdisk_size - 1,

@@ -363,8 +376,8 @@ static u64 __init get_mem_size(unsigned long limit_pfn)
 static void __init early_reserve_initrd(void)
 {
 	/* Assume only end is not page aligned */
-	u64 ramdisk_image = boot_params.hdr.ramdisk_image;
-	u64 ramdisk_size  = boot_params.hdr.ramdisk_size;
+	u64 ramdisk_image = get_ramdisk_image();
+	u64 ramdisk_size  = get_ramdisk_size();
 	u64 ramdisk_end   = PAGE_ALIGN(ramdisk_image + ramdisk_size);
 
 	if (!boot_params.hdr.type_of_loader ||

@@ -376,8 +389,8 @@ static void __init early_reserve_initrd(void)
 static void __init reserve_initrd(void)
 {
 	/* Assume only end is not page aligned */
-	u64 ramdisk_image = boot_params.hdr.ramdisk_image;
-	u64 ramdisk_size  = boot_params.hdr.ramdisk_size;
+	u64 ramdisk_image = get_ramdisk_image();
+	u64 ramdisk_size  = get_ramdisk_size();
 	u64 ramdisk_end   = PAGE_ALIGN(ramdisk_image + ramdisk_size);
 	u64 mapped_size;
 

Signature · 1 line

1.7.10.4

Quoted 21 lines · Yinghai Lu

There are several places to find ramdisk information early for reserving and relocating.

Use functions to make code more readable and consistent.

Later will add ext_ramdisk_image/size in those functions to support loading ramdisk above 4g.

Signed-off-by Yinghai Lu <[email protected]>

---
 arch/x86/kernel/setup.c |   29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

arch/x86/kernel/setup.c +0 -0

index 1b8a8cc..644a123 100644
@@ -294,12 +294,25 @@ static void __init reserve_brk(void)
 
 #ifdef CONFIG_BLK_DEV_INITRD
 

arch/x86/kernel/setup.c static void __init reserve_brk(void) · line 301

Quoted 5 lines

arch/x86/kernel/setup.c +5 -0

+static u64 __init get_ramdisk_image(void)
+{
+     u64 ramdisk_image = boot_params.hdr.ramdisk_image;
+
+     return ramdisk_image;

just do

Quoted 7 lines

arch/x86/kernel/setup.c static void __init reserve_brk(void) · line 301

Quoted 5 lines

arch/x86/kernel/setup.c +5 -0

+static u64 __init get_ramdisk_image(void)
+{
+     u64 ramdisk_image = boot_params.hdr.ramdisk_image;
+
+     return ramdisk_image;

just do

No, I will insert line between them.

later will check ext_cmd_line_ptr at the same time.

Signed-off-by Yinghai Lu <[email protected]>

Cc Gokul Caushik <[email protected]>

Cc Josh Triplett <[email protected]>

Cc Joe Millenbach <[email protected]>

Cc Alexander Duyck <[email protected]>

---
 arch/x86/boot/compressed/cmdline.c |   10 ++++++++--
 arch/x86/kernel/head64.c           |   13 +++++++++++--
 2 files changed, 19 insertions(+), 4 deletions(-)

arch/x86/boot/compressed/cmdline.c +0 -0

index 10f6b11..b4c913c 100644

arch/x86/boot/compressed/cmdline.c +3 -2

+}
 int cmdline_find_option(const char *option, char *buffer, int bufsize)
 {
-	return __cmdline_find_option(real_mode->hdr.cmd_line_ptr, option, buffer, bufsize);
+	return __cmdline_find_option(get_cmd_line_ptr(), option, buffer, bufsize);
 }
 int cmdline_find_option_bool(const char *option)
 {
-	return __cmdline_find_option_bool(real_mode->hdr.cmd_line_ptr, option);
+	return __cmdline_find_option_bool(get_cmd_line_ptr(), option);
 }
 
 #endif

arch/x86/kernel/head64.c +0 -0

index c463725..316e7b2 100644

arch/x86/kernel/head64.c +6 -2

+}
+
 static void __init copy_bootdata(char *real_mode_data)
 {
 	char * command_line;
+	unsigned long cmd_line_ptr;
 
 	memcpy(&boot_params, real_mode_data, sizeof boot_params);
-	if (boot_params.hdr.cmd_line_ptr) {
-		command_line = __va(boot_params.hdr.cmd_line_ptr);
+	cmd_line_ptr = get_cmd_line_ptr();
+	if (cmd_line_ptr) {
+		command_line = __va(cmd_line_ptr);
 		memcpy(boot_command_line, command_line, COMMAND_LINE_SIZE);
 	}
 }

Signature · 1 line

1.7.10.4

cmdline.c::__cmdline_find_option... are shared between 16-bit setup code and 32/64 bit decompressor code.

for 32/64 only path via kexec, we should not check if ptr is less 1M. as those cmdline could be put above 1M, or even 4G.

Move out accessible checking out of __cmdline_find_option() So decompressor in misc.c can parse cmdline correctly.

Signed-off-by Yinghai Lu <[email protected]>

---
 arch/x86/boot/boot.h    |   14 ++++++++++++--
 arch/x86/boot/cmdline.c |    8 ++++----
 2 files changed, 16 insertions(+), 6 deletions(-)

arch/x86/boot/boot.h +0 -0

index 18997e5..7fadf80 100644

arch/x86/kernel/machine_kexec_64.c +5 -2

 	unsigned long mstart, mend;
 	pgd_t *level4p;
 	int result;
 	int i;
 
 	level4p = (pgd_t *)__va(start_pgtable);
-	result = init_level4_page(image, level4p, 0, max_pfn << PAGE_SHIFT);
+	clear_page(level4p);
+	result = kernel_ident_mapping_init(&info, level4p,
+						0, max_pfn << PAGE_SHIFT);
 	if (result)
 		return result;

@@ -225,7 +115,8 @@ static int init_pgtable(struct kimage *image, unsigned long start_pgtable)
 		mstart = image->segment[i].mem;
 		mend   = mstart + image->segment[i].memsz;
 
-		result = ident_mapping_init(image, level4p, mstart, mend);
+		result = kernel_ident_mapping_init(&info,
+						 level4p, mstart, mend);
 
 		if (result)
 			return result;

Signature · 1 line

1.7.10.4

arch/x86/kernel/machine_kexec_64.c err: · line 95

Quoted 7 lines · Yinghai Lu

static int init_pgtable(struct kimage *image, unsigned long start_pgtable) {

arch/x86/kernel/machine_kexec_64.c +5 -0

+     struct x86_mapping_info info = {
+             .alloc_pgt_page = alloc_pgt_page,
+             .context        = image,
+             .pmd_flag       = __PAGE_KERNEL_LARGE_EXEC,
+     };

This is leaving ->kernel_mapping uninitialized to contain a random, previous stack value. I don't think we want that.

Quoted 9 lines · Borislav Petkov

Quoted 7 lines · Yinghai Lu

static int init_pgtable(struct kimage *image, unsigned long start_pgtable) {

arch/x86/kernel/machine_kexec_64.c +5 -0

+     struct x86_mapping_info info = {
+             .alloc_pgt_page = alloc_pgt_page,
+             .context        = image,
+             .pmd_flag       = __PAGE_KERNEL_LARGE_EXEC,
+     };

Quoted 2 lines

This is leaving ->kernel_mapping uninitialized to contain a random, previous stack value. I don't think we want that.

that should be initialized to false by default.

Quoted 10 lines · Yinghai Lu

Quoted 9 lines · Borislav Petkov

Quoted 7 lines · Yinghai Lu

static int init_pgtable(struct kimage *image, unsigned long start_pgtable) {

arch/x86/kernel/machine_kexec_64.c +5 -0

+     struct x86_mapping_info info = {
+             .alloc_pgt_page = alloc_pgt_page,
+             .context        = image,
+             .pmd_flag       = __PAGE_KERNEL_LARGE_EXEC,
+     };

Quoted 4 lines

Quoted 2 lines

This is leaving ->kernel_mapping uninitialized to contain a random, previous stack value. I don't think we want that.

that should be initialized to false by default.

So make it explicit. You can't possibly rely on what the stack contains when you allocate that struct there.

I should say:

that *is* initialized to false by default.

please check

http://stackoverflow.com/questions/10828294/c-and-c-partial-initialization-of-automatic-structure

We should not set mapping for all under max_pfn.

That causes same problem that is fixed by

x86, mm: Only direct map addresses that are marked as E820_RAM

This patch expose pfn_mapped array, and only set ident mapping for ranges

in that array.

This patch rely on new ident_mapping_init that could handle existing

pgd/pud between different calling.

Signed-off-by Yinghai Lu <[email protected]>

Cc Alexander Duyck <[email protected]>

Quoted 1 line · Yinghai Lu

We should not set mapping for all under max_pfn.

"We should not establish mappings for all memory under max_pfn."

Quoted 2 lines

That causes same problem that is fixed by

"Otherwise, it causes the same ..."

Quoted 2 lines

x86, mm: Only direct map addresses that are marked as E820_RAM

You could add this patch's commit id since it is in tip:x86/mm2 and it shouldn't change: 66520ebc2df3.

Ditto for patch 09/31, "x86, 64bit: #PF handler set page to cover 2M only".

Quoted 2 lines

This patch expose pfn_mapped array, and only set ident mapping for ranges

exposes the... sets

Quoted 2 lines

in that array.

Quoted 1 line

This patch rely on new ident_mapping_init that could handle existing

relies on the new

Quoted 1 line

pgd/pud between different calling.

calls.

Quoted 4 lines · Borislav Petkov

Quoted 1 line · Yinghai Lu

We should not set mapping for all under max_pfn.

"We should not establish mappings for all memory under max_pfn."

that is not accurate.

We should not set mapping for all range under max_pfn.

or

We should set mappings only for memory ranges under max_pfn.

Quoted 5 lines

Quoted 1 line

That causes same problem that is fixed by

"Otherwise, it causes the same ..."

Quoted 1 line

Quoted 4 lines

Quoted 1 line

x86, mm: Only direct map addresses that are marked as E820_RAM

You could add this patch's commit id since it is in tip:x86/mm2 and it shouldn't change: 66520ebc2df3.

why ? they are not in linus tree yet, so it could change if that tip branch is rebased.

Quoted 6 lines · Yinghai Lu

Quoted 4 lines · Borislav Petkov

Quoted 1 line · Yinghai Lu

We should not set mapping for all under max_pfn.

"We should not establish mappings for all memory under max_pfn."

Quoted 7 lines

that is not accurate.

We should not set mapping for all range under max_pfn.

or

We should set mappings only for memory ranges under max_pfn.

Ok, that last thing is getting close. So do I understand it correctly now:

"We should establish mappings only for memory (memory which is not marked reserved or whatever by E820 or some other mechanism) under max_pfn."

?

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

That causes same problem that is fixed by

"Otherwise, it causes the same ..."

Quoted 2 lines

x86, mm: Only direct map addresses that are marked as E820_RAM

Quoted 5 lines

Quoted 2 lines

You could add this patch's commit id since it is in tip:x86/mm2 and it shouldn't change: 66520ebc2df3.

why ? they are not in linus tree yet, so it could change if that tip branch is rebased.

Oh, you didn't know: tip branches don't get rebased. At least almost never.

ext_ramdisk_image/size will record high 32bits for ramdisk info.

xloadflags bit0 will be set if relocatable with 64bit.

Let get_ramdisk_image/size to use ext_ramdisk_image/size to get right positon for ramdisk.

bootloader will fill value to ext_ramdisk_image/size when it load ramdisk above 4G.

Also bootloader will check if xloadflags bit0 is set to decicde if

it could load ramdisk high above 4G.

sentinel is used to make sure kernel have ext_* valid values set

Update header version to 2.12.

(quoted diff) +0 -5

-v2: add ext_cmd_line_ptr for above 4G support.
-v3: update to xloadflags from HPA.
-v4: use fields from bootparam instead setup_header according to HPA.
-v5: add checking for USE_EXT_BOOT_PARAMS
-v6: use sentinel to check if ext_* are valid suggested by HPA.
     HPA said:

1. add a field in the uninitialized portion, call it "sentinel";
2. make sure the byte position corresponding to the "sentinel" field is nonzero in the bzImage file;
3. if the kernel boots up and sentinel is nonzero, erase those fields that you identified as uninitialized;

-v7: change to 0x1ef instead of 0x1f0, HPA said:

	it is quite plausible that someone may (fairly sanely) start the
	copy range at 0x1f0 instead of 0x1f1

Signed-off-by Yinghai Lu <[email protected]>

Cc Rob Landley <[email protected]>

Cc Matt Fleming <[email protected]>

Cc Gokul Caushik <[email protected]>

Cc Josh Triplett <[email protected]>

Cc Joe Millenbach <[email protected]>

---
 Documentation/x86/boot.txt            |   15 ++++++++++++++-
 Documentation/x86/zero-page.txt       |    4 ++++
 arch/x86/boot/compressed/cmdline.c    |    2 ++
 arch/x86/boot/compressed/misc.c       |   12 ++++++++++++
 arch/x86/boot/header.S                |   12 ++++++++++--
 arch/x86/boot/setup.ld                |    7 +++++++
 arch/x86/include/uapi/asm/bootparam.h |   13 ++++++++++---
 arch/x86/kernel/head64.c              |    2 ++
 arch/x86/kernel/setup.c               |    4 ++++
 9 files changed, 65 insertions(+), 6 deletions(-)

Documentation/x86/boot.txt +0 -0

index 406d82d..18ca9fb 100644

Documentation/x86/boot.txt +2 -1

+
 **** MEMORY LAYOUT
 
 The traditional memory map for the kernel loader, used for Image or

@@ -182,7 +185,7 @@ Offset	Proto	Name		Meaning
 0230/4	2.05+	kernel_alignment Physical addr alignment required for kernel
 0234/1	2.05+	relocatable_kernel Whether kernel is relocatable or not
 0235/1	2.10+	min_alignment	Minimum alignment, as a power of two
-0236/2	N/A	pad3		Unused
+0236/2	2.12+	xloadflags	Boot protocol option flags
 0238/4	2.06+	cmdline_size	Maximum size of the kernel command line
 023C/4	2.07+	hardware_subarch Hardware subarchitecture
 0240/8	2.07+	hardware_subarch_data Subarchitecture-specific data

@@ -582,6 +585,16 @@ Protocol:	2.10+
   misaligned kernel.  Therefore, a loader should typically try each
   power-of-two alignment from kernel_alignment down to this alignment.