Skip to contentSkip to conversation

swsusp: Yet another attempt to fix Bug #4959

RFC PATCH In review Rafael J. Wysocki 3 participants 11 messages Oct 1, 2005 – Oct 8, 2005

Subsystems (2)
Suspend to RAM Hibernation
lore
Share & tools
lore

Rafael J. Wysocki

author
#0 lore raw

RFC PATCH 1/1 swsusp: Yet another attempt to fix Bug #4959

Hi,

The following is yet another attempt to fix Bug #4959.

This one uses the code in mm/init.c directly. For this purpose it introduces a modified version of init_memory_mapping() which is compiled if CONFIG_SOFTWARE_SUSPEND is set.

This function allocates twice as much memory as needed for the direct mapping page tables and assigns the second half of it to the resume page tables. This area is later marked with PG_nosave by swsusp, so that it is not overwritten during resume.

Your comments, criticisms and (preferably) suggestions will be appreciated.

Greetings,
Rafael

arch/x86_64/kernel/suspend.c +12 -0
Index: linux-2.6.14-rc3/arch/x86_64/kernel/suspend.c
===================================================================
@@ -9,6 +9,7 @@
 
 #include <linux/config.h>
 #include <linux/smp.h>
+#include <linux/mm.h>
 #include <linux/suspend.h>
 #include <asm/proto.h>
 

@@ -140,4 +141,15 @@
 
 }
 
+#ifdef CONFIG_SOFTWARE_SUSPEND
+extern unsigned long resume_table_start, resume_table_end;
 
+int arch_prepare_suspend(void)
+{
+	unsigned long pfn;
+
+	for (pfn = resume_table_start; pfn < resume_table_end; pfn++)
+		SetPageNosave(pfn_to_page(pfn));
+	return 0;
+}
+#endif /* CONFIG_SOFTWARE_SUSPEND */
arch/x86_64/kernel/suspend_asm.S +19 -5
Index: linux-2.6.14-rc3/arch/x86_64/kernel/suspend_asm.S
===================================================================
@@ -40,11 +40,11 @@
 	ret
 
 ENTRY(swsusp_arch_resume)
-	/* set up cr3 */	
-	leaq	init_level4_pgt(%rip),%rax
-	subq	$__START_KERNEL_map,%rax
-	movq	%rax,%cr3
-
+	/* switch to the resume page tables */
+	leaq	resume_level4_pgt(%rip), %rax
+	subq	$__START_KERNEL_map, %rax
+	movq	%rax, %cr3
+	/* Flush TLB */
 	movq	mmu_cr4_features(%rip), %rax
 	movq	%rax, %rdx
 	andq	$~(1<<7), %rdx	# PGE

@@ -69,6 +69,10 @@
 	movq	pbe_next(%rdx), %rdx
 	jmp	loop
 done:
+	/* go back to the original page tables */
+	leaq	init_level4_pgt(%rip), %rax
+	subq	$__START_KERNEL_map, %rax
+	movq	%rax, %cr3
 	/* Flush TLB, including "global" things (vmalloc) */
 	movq	mmu_cr4_features(%rip), %rax
 	movq	%rax, %rdx

@@ -102,3 +106,13 @@
 	xorq	%rax, %rax
 
 	ret
+
+	.section	".data.nosave"
+	.align PAGE_SIZE
+ENTRY(resume_level4_pgt)
+	.quad	0x0000000000002007 + __PHYSICAL_START	/* -> level3_ident_pgt */
+	.fill	255,8,0
+	.quad	0x000000000000a007 + __PHYSICAL_START
+	.fill	254,8,0
+	/* (2^48-(2*1024*1024*1024))/(2^39) = 511 */
+	.quad	0x0000000000003007 + __PHYSICAL_START	/* -> level3_kernel_pgt */
include/asm-x86_64/suspend.h +4 -0
Index: linux-2.6.14-rc3/include/asm-x86_64/suspend.h
===================================================================
@@ -6,11 +6,15 @@
 #include <asm/desc.h>
 #include <asm/i387.h>
 
+#ifdef CONFIG_SOFTWARE_SUSPEND
+extern int arch_prepare_suspend(void);
+#else
 static inline int
 arch_prepare_suspend(void)
 {
 	return 0;
 }
+#endif
 
 /* Image of the saved processor state. If you touch this, fix acpi_wakeup.S. */
 struct saved_context {
arch/x86_64/mm/init.c +67 -0

85 lines not on this page · raw message

kernel/power/swsusp.c +0 -1
Index: linux-2.6.14-rc3/kernel/power/swsusp.c
===================================================================
@@ -672,7 +672,6 @@
 		return 0;
 
 	page = pfn_to_page(pfn);
-	BUG_ON(PageReserved(page) && PageNosave(page));
 	if (PageNosave(page))
 		return 0;
 	if (PageReserved(page) && pfn_is_nosave(pfn)) {
#1 lore raw
Quoted 6 lines · Rafael J. Wysocki

This function allocates twice as much memory as needed for the direct mapping page tables and assigns the second half of it to the resume page tables. This area is later marked with PG_nosave by swsusp, so that it is not overwritten during resume.

I prefered it when the additional page tables were allocated only on demand.

-Andi

author
#2 lore raw
Quoted 9 lines · Andi Kleen
Quoted 6 lines · Rafael J. Wysocki

This function allocates twice as much memory as needed for the direct mapping page tables and assigns the second half of it to the resume page tables. This area is later marked with PG_nosave by swsusp, so that it is not overwritten during resume.

I prefered it when the additional page tables were allocated only on demand.

Me too.  Let's get back to that patch, then. :-)

Comments etc. will be appreciated.

Greetings,
Rafael

Signed-off-by Rafael J. Wysocki <[email protected]>
arch/x86_64/kernel/suspend.c +130 -0
Index: linux-2.6.14-rc3-git1/arch/x86_64/kernel/suspend.c
===================================================================
@@ -11,6 +11,8 @@
 #include <linux/smp.h>
 #include <linux/suspend.h>
 #include <asm/proto.h>
+#include <asm/page.h>
+#include <asm/pgtable.h>
 
 struct saved_context saved_context;
 

@@ -140,4 +142,132 @@
 
 }
 
+#ifdef CONFIG_SOFTWARE_SUSPEND
+/* Defined in kernel/power/swsusp.c */
+extern unsigned long get_usable_page(unsigned gfp_mask);
+extern void free_eaten_memory(void);
+/* Defined in arch/x86_64/kernel/suspend_asm.S */
+extern int restore_image(void);
 
+pgd_t *temp_level4_pgt;
+
+static void **pages;
+
+static inline void *__add_page(void)
+{
+	void **c;
+
+	c = (void **)get_usable_page(GFP_ATOMIC);
+	if (c) {
+		*c = pages;
+		pages = c;
+	}
+	return c;
+}
+
+static inline void *__next_page(void)
+{
+	void **c;
+
+	c = pages;
+	if (c) {
+		pages = *c;
+		*c = NULL;
+	}
+	return c;
+}
+
+/*
+ * Try to allocate as many usable pages as needed and daisy chain them.
+ * If one allocation fails, free the pages allocated so far
+ */
+static int alloc_usable_pages(unsigned long n)
+{
+	void *p;
+
+	pages = NULL;
+	do
+		if (!__add_page())
+			break;
+	while (--n);
+	if (n) {
+		p = __next_page();
+		while (p) {
+			free_page((unsigned long)p);
+			p = __next_page();
+		}
+		return -ENOMEM;
+	}
+	return 0;
+}
+
+static void phys_pud_init(pud_t *pud, unsigned long address, unsigned long end)
+{
+	long i, j;
+
+	i = pud_index(address);
+	pud = pud + i;
+	for (; i < PTRS_PER_PUD; pud++, i++) {
+		unsigned long paddr;
+		pmd_t *pmd;
+
+		paddr = address + i*PUD_SIZE;
+		if (paddr >= end)
+			break;
+
+		pmd = (pmd_t *)__next_page();
+		set_pud(pud, __pud(__pa(pmd) | _KERNPG_TABLE));
+		for (j = 0; j < PTRS_PER_PMD; pmd++, j++, paddr += PMD_SIZE) {
+			unsigned long pe;
+
+			if (paddr >= end)
+				break;
+			pe = _PAGE_NX|_PAGE_PSE | _KERNPG_TABLE | _PAGE_GLOBAL | paddr;
+			pe &= __supported_pte_mask;
+			set_pmd(pmd, __pmd(pe));
+		}
+	}
+}
+
+static void set_up_temporary_mappings(void)
+{
+	unsigned long start, end, next;
+
+	temp_level4_pgt = (pgd_t *)__next_page();
+
+	/* It is safe to reuse the original kernel mapping */
+	set_pgd(temp_level4_pgt + pgd_index(__START_KERNEL_map),
+		init_level4_pgt[pgd_index(__START_KERNEL_map)]);
+
+	/* Set up the direct mapping from scratch */
+	start = (unsigned long)pfn_to_kaddr(0);
+	end = (unsigned long)pfn_to_kaddr(end_pfn);
+
+	for (; start < end; start = next) {
+		pud_t *pud = (pud_t *)__next_page();
+		next = start + PGDIR_SIZE;
+		if (next > end)
+			next = end;
+		phys_pud_init(pud, __pa(start), __pa(next));
+		set_pgd(temp_level4_pgt + pgd_index(start),
+			mk_kernel_pgd(__pa(pud)));
+	}
+}
+
+int swsusp_arch_resume(void)
+{
+	unsigned long n;
+
+	n = ((end_pfn << PAGE_SHIFT) + PUD_SIZE - 1) >> PUD_SHIFT;
+	n += (n + PTRS_PER_PUD - 1) / PTRS_PER_PUD + 1;
+	pr_debug("swsusp_arch_resume(): pages needed = %lu\n", n);
+	if (alloc_usable_pages(n)) {
+		free_eaten_memory();
+		return -ENOMEM;
+	}
+	/* We have got enough memory and from now on we cannot recover */
+	set_up_temporary_mappings();
+	restore_image();
+	return 0;
+}
+#endif /* CONFIG_SOFTWARE_SUSPEND */
kernel/power/swsusp.c +4 -3
Index: linux-2.6.14-rc3-git1/kernel/power/swsusp.c
===================================================================
@@ -1095,7 +1095,7 @@
 	*eaten_memory = c;
 }
 
-static unsigned long get_usable_page(unsigned gfp_mask)
+unsigned long get_usable_page(unsigned gfp_mask)
 {
 	unsigned long m;
 

@@ -1109,7 +1109,7 @@
 	return m;
 }
 
-static void free_eaten_memory(void)
+void free_eaten_memory(void)
 {
 	unsigned long m;
 	void **c;

@@ -1481,11 +1481,12 @@
 	/* Allocate memory for the image and read the data from swap */
 
 	error = check_pagedir(pagedir_nosave);
-	free_eaten_memory();
+
 	if (!error)
 		error = data_read(pagedir_nosave);
 
 	if (error) { /* We fail cleanly */
+		free_eaten_memory();
 		for_each_pbe (p, pagedir_nosave)
 			if (p->address) {
 				free_page(p->address);
arch/x86_64/kernel/suspend_asm.S +11 -6
Index: linux-2.6.14-rc3-git1/arch/x86_64/kernel/suspend_asm.S
===================================================================
@@ -39,12 +39,13 @@
 	call swsusp_save
 	ret
 
-ENTRY(swsusp_arch_resume)
-	/* set up cr3 */	
-	leaq	init_level4_pgt(%rip),%rax
-	subq	$__START_KERNEL_map,%rax
-	movq	%rax,%cr3
-
+ENTRY(restore_image)
+	/* switch to temporary page tables */
+	movq	$__PAGE_OFFSET, %rdx
+	movq	temp_level4_pgt(%rip), %rax
+	subq	%rdx, %rax
+	movq	%rax, %cr3
+	/* Flush TLB */
 	movq	mmu_cr4_features(%rip), %rax
 	movq	%rax, %rdx
 	andq	$~(1<<7), %rdx	# PGE

@@ -69,6 +70,10 @@
 	movq	pbe_next(%rdx), %rdx
 	jmp	loop
 done:
+	/* go back to the original page tables */
+	leaq	init_level4_pgt(%rip), %rax
+	subq	$__START_KERNEL_map, %rax
+	movq	%rax, %cr3
 	/* Flush TLB, including "global" things (vmalloc) */
 	movq	mmu_cr4_features(%rip), %rax
 	movq	%rax, %rdx
author
#3 lore raw
In reply to Andi Kleen
Quoted 9 lines · Rafael J. Wysocki
Quoted 8 lines · Andi Kleen
Quoted 6 lines · Rafael J. Wysocki

This function allocates twice as much memory as needed for the direct mapping page tables and assigns the second half of it to the resume page tables. This area is later marked with PG_nosave by swsusp, so that it is not overwritten during resume.

Quoted 5 lines
Quoted 1 line

I prefered it when the additional page tables were allocated only on demand.

Me too. Let's get back to that patch, then. :-)

Comments etc. will be appreciated.

I haven't got any comments since I posted it on Saturday, so I gather there are no objections. Or are there any?

Greetings,
Rafael

#4 lore raw
In reply to Rafael J. Wysocki
Quoted 1 line · Rafael J. Wysocki

Your comments, criticisms and (preferably) suggestions will be appreciated.

First always write a full description of the problem and the rationale of the change and a overview what it changes. Also please add Signed-off-by lines.

arch/x86_64/kernel/suspend.c line 145

Quoted 2 lines
arch/x86_64/kernel/suspend.c +2 -0
+#ifdef CONFIG_SOFTWARE_SUSPEND
+extern unsigned long resume_table_start, resume_table_end;

These should be all in some include. Adding externs in C files is near always wrong because it avoids cross file type checking.

Also the convention is to add _pfn to variables that are in PFNs, otherwise it's full addresses.

arch/x86_64/mm/init.c line 265

Quoted 9 lines

10:40:03.000000000 +0200 +++
linux-2.6.14-rc3/arch/x86_64/mm/init.c 2005-10-01 14:31:34.000000000 +0200

arch/x86_64/mm/init.c +3 -0
@@ -260,6 +260,9 @@
 	pmds = (end + PMD_SIZE - 1) >> PMD_SHIFT;
 	tables = round_up(puds * sizeof(pud_t), PAGE_SIZE) +
 		 round_up(pmds * sizeof(pmd_t), PAGE_SIZE);
+#ifdef CONFIG_SOFTWARE_SUSPEND
+	tables += tables;
+#endif

This needs a comment. Also I would still prefer if it was allocated only when suspend is actually attempted.

Quoted 11 lines
arch/x86_64/mm/init.c +0 -0
 	table_start = find_e820_area(0x8000, __pa_symbol(&_text), tables);
 	if (table_start == -1UL)

@@ -272,6 +275,7 @@
 /* Setup the direct mapping of the physical memory at PAGE_OFFSET.
    This runs before bootmem is initialized and gets pages directly from

the physical memory. To access them they are temporarily mapped. */

arch/x86_64/mm/init.c +1 -0
+#ifndef CONFIG_SOFTWARE_SUSPEND
 void __init init_memory_mapping(unsigned long start, unsigned long end)
 {
 	unsigned long next;

arch/x86_64/mm/init.c line 316

Quoted 6 lines
arch/x86_64/mm/init.c +3 -0
@@ -307,6 +311,69 @@
 	       table_start<<PAGE_SHIFT,
 	       table_end<<PAGE_SHIFT);
 }
+#else
+
+extern pgd_t resume_level4_pgt[];

These should be in some include again.

I don't like it that you duplicated the function fully. Is that really needed?

-Andi

author
#5 lore raw

Hi,

Thanks a lot for the comments.

Quoted 8 lines · Andi Kleen
Quoted 1 line · Rafael J. Wysocki

Your comments, criticisms and (preferably) suggestions will be appreciated.

First always write a full description of the problem and the rationale of the change and a overview what it changes. Also please add Signed-off-by lines.

I forgot to add the Signed-off-by line, sorry.

I will add the full problem description etc. to the next iteration of the patch.

Quoted 6 lines

arch/x86_64/kernel/suspend.c line 145

Quoted 3 lines
arch/x86_64/kernel/suspend.c +2 -0
+#ifdef CONFIG_SOFTWARE_SUSPEND
+extern unsigned long resume_table_start, resume_table_end;

These should be all in some include. Adding externs in C files is near always wrong because it avoids cross file type checking.

I will do that.

Quoted 2 lines

Also the convention is to add _pfn to variables that are in PFNs, otherwise it's full addresses.

I will do that too.

Quoted 4 lines
Quoted 4 lines

10:40:03.000000000 +0200 +++
linux-2.6.14-rc3/arch/x86_64/mm/init.c 2005-10-01 14:31:34.000000000 +0200

arch/x86_64/mm/init.c +0 -0
@@ -260,6 +260,9 @@
 	pmds = (end + PMD_SIZE - 1) >> PMD_SHIFT;
Quoted 8 lines

arch/x86_64/mm/init.c line 265

Quoted 5 lines
arch/x86_64/mm/init.c +3 -0
 	tables = round_up(puds * sizeof(pud_t), PAGE_SIZE) +
 		 round_up(pmds * sizeof(pmd_t), PAGE_SIZE);
+#ifdef CONFIG_SOFTWARE_SUSPEND
+	tables += tables;
+#endif

This needs a comment. Also I would still prefer if it was allocated only when suspend is actually attempted.

I will add a comment here.

The problem with allocating the resume page tables is that I have to make sure they won't be overwritten by swsusp during resume. There are three possible solutions to it:
(1) to use static arrays of predefined size marked as __nosavedata,
(2) to allocate the page tables as early as in the init code so that they always end up at the same physical addresses and (later on) mark them as nosave for swsusp,
(3) to place the page tables in the areas of memory that won't be modified by swsusp.

The downsides of (1) are that by using it I'd artificially limit the allowed size of the suspend image and I'd waste some page frames on systems with relatively small RAM. Also I'd have to modify phys_pud_init() to use it for populating these page tables.

The downside of (3) is that I do not know which pages will be overwritten by swsusp until the image is loaded into memory, because the addresses that will be affected by swsusp are stored within the image. Thus in that case I can only allocate the resume page tables right before copying the image pages to their original locations. This requires the use of GFP_ATOMIC allocations that may fail, and the suitable pages have to be cherry picked from the pages returned by the kernel (some of them will be overwritten by swsusp). The failure of a memory allocation at that time, although not very probable, would lead to a resume failure and the loss of the system's state from before suspend, so this is not an ideal solution. Moreover, in that case I'd have to modify phys_pud_init() to use it for populating the resume page tables.

The only downside of (2) is that it keeps some 4KB page frames permanently unavailable to the system. However, for a typical systems the number of them is limited (it's (1 page per every started 1GB of RAM) + 2, or 3 pages for a system with no more than 1GB of RAM). On the other hand, in that case I can use the original phys_pud_init() to populate the resume page tables and the required code changes are quite limited in scope. Therefore I've chosen this one.

Quoted 11 lines
Quoted 11 lines
arch/x86_64/mm/init.c +0 -0
 	table_start = find_e820_area(0x8000, __pa_symbol(&_text), tables);
 	if (table_start == -1UL)

@@ -272,6 +275,7 @@
 /* Setup the direct mapping of the physical memory at PAGE_OFFSET.
    This runs before bootmem is initialized and gets pages directly from

the physical memory. To access them they are temporarily mapped. */

arch/x86_64/mm/init.c +1 -0
+#ifndef CONFIG_SOFTWARE_SUSPEND
 void __init init_memory_mapping(unsigned long start, unsigned long end)
 {
 	unsigned long next;

@@ -307,6 +311,69 @@
Quoted 8 lines

arch/x86_64/mm/init.c line 316

Quoted 6 lines
arch/x86_64/mm/init.c +3 -0
 	       table_start<<PAGE_SHIFT,
 	       table_end<<PAGE_SHIFT);
 }
+#else
+
+extern pgd_t resume_level4_pgt[];

These should be in some include again.

OK

Quoted 2 lines

I don't like it that you duplicated the function fully. Is that really needed?

Not really, but otherwise I'll have to use a bunch of #ifdefs in the function body. I'll do that in the next iteration of the patch.

Thanks again,
Rafael

author
#6 lore raw

PATCH 1/1 swsusp: avoid possible page tables corruption during resume on x86-64

In reply to Andi Kleen

Summary =========
The following patch makes swsusp avoid the possible temporary corruption of page translation tables during resume on x86-64. This is achieved by creating a copy of the relevant page tables that will not be modified by swsusp and can be safely used by it on resume.

Problem description ==========
The problem is that during resume on x86-64 swsusp may temporarily corrupt the page tables used for the direct mapping of RAM. If that happens, a page fault occurs and cannot be handled properly, which leads to the solid hang of the affected system. This leads to the loss of the system's state from before suspend and may result in the loss of data or the corruption of filesystems, so it is a serious issue. Also, it appears to happen quite often (for me, as often as 50% of the time).

The problem is related to the fact that (at least) one of the PMD entries used in the direct memory mapping (starting at PAGE_OFFSET) points to a page table the physical address of which is much greater than the physical address of the PMD entry itself. Moreover, unfortunately, the physical address of the page table before suspend (i.e. the one stored in the suspend image) happens to be different to the physical address of the corresponding page table used during resume (i.e. the one that is valid right before swsusp_arch_resume() in arch/x86_64/kernel/suspend_asm.S is executed). Thus while the image is restored, the "offending" PMD entry gets overwritten, so it does not point to the right physical address any more (i.e. there's no page table at the address pointed to by it, because it points to the address the page table has been at during suspend). Consequently, if the PMD entry is used later on, and it _is_ used in the process of copying the image pages, a page fault occurs, but it cannot be handled in the normal way and the system hangs.

Proposed solution =========
To avoid the corruption of the page tables during resume the patch creates a copy of them that will not be overwritten by swsusp and can be used by it safely during resume. This copy is created during the initialization of the system along with the original page tables, because it has to be located in the same page frames on every boot (otherwise swsusp could overwrite it).

Alternatively, we could create such a copy of the page translation tables on demand, before swsusp starts to restore the original state of the system from the suspend image, but this would require that atomic memory allocations be used while there's almost no free RAM. Then, theoretically the memory allocations could fail leading to the failure of the entire resume process. To avoid that risk I have decided to use the preallocated resume page tables, although this makes some 4KB page frames be permanently reserved (eg. 3 page frames for a system with no more than 1GB of RAM).

The additional advantage of the proposed approach is that the code used for populatingvthe original page tables can also be used for populating the resume pagevtables (any alternative solution would require the use of some additional code for this purpose).

Please consider the patch for applying.

Greetings,
Rafael

Signed-off-by Rafael J. Wysocki <[email protected]>
arch/x86_64/kernel/suspend.c +12 -0
Index: linux-2.6.14-rc3-git5/arch/x86_64/kernel/suspend.c
===================================================================
@@ -9,6 +9,7 @@
 
 #include <linux/config.h>
 #include <linux/smp.h>
+#include <linux/mm.h>
 #include <linux/suspend.h>
 #include <asm/proto.h>
 

@@ -140,4 +141,15 @@
 
 }
 
+#ifdef CONFIG_SOFTWARE_SUSPEND
+unsigned long resume_table_start_pfn, resume_table_end_pfn;
 
+int arch_prepare_suspend(void)
+{
+	unsigned long pfn;
+
+	for (pfn = resume_table_start_pfn; pfn < resume_table_end_pfn; pfn++)
+		SetPageNosave(pfn_to_page(pfn));
+	return 0;
+}
+#endif /* CONFIG_SOFTWARE_SUSPEND */
arch/x86_64/kernel/suspend_asm.S +19 -5
Index: linux-2.6.14-rc3-git5/arch/x86_64/kernel/suspend_asm.S
===================================================================
@@ -40,11 +40,11 @@
 	ret
 
 ENTRY(swsusp_arch_resume)
-	/* set up cr3 */	
-	leaq	init_level4_pgt(%rip),%rax
-	subq	$__START_KERNEL_map,%rax
-	movq	%rax,%cr3
-
+	/* switch to the resume page tables */
+	leaq	resume_level4_pgt(%rip), %rax
+	subq	$__START_KERNEL_map, %rax
+	movq	%rax, %cr3
+	/* Flush TLB */
 	movq	mmu_cr4_features(%rip), %rax
 	movq	%rax, %rdx
 	andq	$~(1<<7), %rdx	# PGE

@@ -69,6 +69,10 @@
 	movq	pbe_next(%rdx), %rdx
 	jmp	loop
 done:
+	/* go back to the original page tables */
+	leaq	init_level4_pgt(%rip), %rax
+	subq	$__START_KERNEL_map, %rax
+	movq	%rax, %cr3
 	/* Flush TLB, including "global" things (vmalloc) */
 	movq	mmu_cr4_features(%rip), %rax
 	movq	%rax, %rdx

@@ -102,3 +106,13 @@
 	xorq	%rax, %rax
 
 	ret
+
+	.section	".data.nosave"
+	.align PAGE_SIZE
+ENTRY(resume_level4_pgt)
+	.quad	0x0000000000002007 + __PHYSICAL_START	/* -> level3_ident_pgt */
+	.fill	255,8,0
+	.quad	0x000000000000a007 + __PHYSICAL_START
+	.fill	254,8,0
+	/* (2^48-(2*1024*1024*1024))/(2^39) = 511 */
+	.quad	0x0000000000003007 + __PHYSICAL_START	/* -> level3_kernel_pgt */
include/asm-x86_64/suspend.h +9 -0
Index: linux-2.6.14-rc3-git5/include/asm-x86_64/suspend.h
===================================================================
@@ -6,11 +6,20 @@
 #include <asm/desc.h>
 #include <asm/i387.h>
 
+#ifdef CONFIG_SOFTWARE_SUSPEND
+extern unsigned long resume_table_start_pfn, resume_table_end_pfn;
+extern pgd_t resume_level4_pgt[];
+
+#define pgd_offset_resume(address) (resume_level4_pgt + pgd_index(address))
+
+extern int arch_prepare_suspend(void);
+#else
 static inline int
 arch_prepare_suspend(void)
 {
 	return 0;
 }
+#endif
 
 /* Image of the saved processor state. If you touch this, fix acpi_wakeup.S. */
 struct saved_context {
arch/x86_64/mm/init.c +32 -0
Index: linux-2.6.14-rc3-git5/arch/x86_64/mm/init.c
===================================================================
@@ -36,6 +36,7 @@
 #include <asm/mmu_context.h>
 #include <asm/proto.h>
 #include <asm/smp.h>
+#include <asm/suspend.h>
 
 #ifndef Dprintk
 #define Dprintk(x...)

@@ -260,6 +261,14 @@
 	pmds = (end + PMD_SIZE - 1) >> PMD_SHIFT;
 	tables = round_up(puds * sizeof(pud_t), PAGE_SIZE) +
 		 round_up(pmds * sizeof(pmd_t), PAGE_SIZE);
+#ifdef CONFIG_SOFTWARE_SUSPEND
+	/*
+	 * We need to create a copy of the direct mapping page tables
+	 * that will be used during resume from disk, so we allocate
+	 * twice as much room as needed for the direct mapping alone
+	 */
+	tables += tables;
+#endif
 
 	table_start = find_e820_area(0x8000, __pa_symbol(&_text), tables);
 	if (table_start == -1UL)

@@ -275,6 +284,9 @@
 void __init init_memory_mapping(unsigned long start, unsigned long end)
 { 
 	unsigned long next; 
+#ifdef CONFIG_SOFTWARE_SUSPEND
+	unsigned long start_phys = start;
+#endif
 
 	Dprintk("init_memory_mapping\n");
 

@@ -306,6 +318,26 @@
 	early_printk("kernel direct mapping tables upto %lx @ %lx-%lx\n", end, 
 	       table_start<<PAGE_SHIFT, 
 	       table_end<<PAGE_SHIFT);
+#ifdef CONFIG_SOFTWARE_SUSPEND
+
+	resume_table_start_pfn = table_end;
+
+	start = (unsigned long)__va(start_phys);
+
+	for (; start < end; start = next) {
+		int map;
+		unsigned long pud_phys;
+		pud_t *pud = alloc_low_page(&map, &pud_phys);
+		next = start + PGDIR_SIZE;
+		if (next > end)
+			next = end;
+		phys_pud_init(pud, __pa(start), __pa(next));
+		set_pgd(pgd_offset_resume(start), mk_kernel_pgd(pud_phys));
+		unmap_low_page(map);
+	}
+
+	resume_table_end_pfn = table_end;
+#endif
 }
 
 extern struct x8664_pda cpu_pda[NR_CPUS];
kernel/power/swsusp.c +0 -1
Index: linux-2.6.14-rc3-git5/kernel/power/swsusp.c
===================================================================
@@ -672,7 +672,6 @@
 		return 0;
 
 	page = pfn_to_page(pfn);
-	BUG_ON(PageReserved(page) && PageNosave(page));
 	if (PageNosave(page))
 		return 0;
 	if (PageReserved(page) && pfn_is_nosave(pfn)) {
#7 lore raw

Re: [PATCH][Fix] swsusp: avoid possible page tables corruption during resume on x86-64

Hi!

Quoted 5 lines

Summary =========
The following patch makes swsusp avoid the possible temporary corruption of page translation tables during resume on x86-64. This is achieved by creating a copy of the relevant page tables that will not be modified by swsusp and can be safely used by it on resume.

Andi, this means swsusp fails 50% of time on x86-64. I believe we even have one report in suse bugzilla by now... Could we get this somehow merged?

kernel/power/swsusp.c line 677

Quoted 12 lines

Index: linux-2.6.14-rc3-git5/kernel/power/swsusp.c
===================================================================

kernel/power/swsusp.c +0 -1
@@ -672,7 +672,6 @@
 		return 0;
 
 	page = pfn_to_page(pfn);
-	BUG_ON(PageReserved(page) && PageNosave(page));
 	if (PageNosave(page))
 		return 0;
 	if (PageReserved(page) && pfn_is_nosave(pfn)) {

Rafael, are you sure? This will clash with snapshot.c split and probably belongs to some other patch.
Pavel

Signature · 1 line
if you have sharp zaurus hardware you don't need... you know my address
author
#8 lore raw

Re: [discuss] Re: [PATCH][Fix] swsusp: avoid possible page tables corruption during resume on x86-64

Hi,

Quoted 20 lines · Pavel Machek

Hi!

Quoted 5 lines

Summary =========
The following patch makes swsusp avoid the possible temporary corruption of page translation tables during resume on x86-64. This is achieved by creating a copy of the relevant page tables that will not be modified by swsusp and can be safely used by it on resume.

Andi, this means swsusp fails 50% of time on x86-64. I believe we even have one report in suse bugzilla by now... Could we get this somehow merged?

Quoted 7 lines

Index: linux-2.6.14-rc3-git5/kernel/power/swsusp.c
===================================================================

kernel/power/swsusp.c +0 -0
@@ -672,7 +672,6 @@
 		return 0;
Quoted 7 lines

kernel/power/swsusp.c line 677

Quoted 5 lines
kernel/power/swsusp.c +0 -1
 	page = pfn_to_page(pfn);
-	BUG_ON(PageReserved(page) && PageNosave(page));
 	if (PageNosave(page))
 		return 0;
 	if (PageReserved(page) && pfn_is_nosave(pfn)) {

Rafael, are you sure?

Yes, I am. The pages allocated in init_memory_mapping() are marked with PG_reserved by the init code.

Quoted 1 line

This will clash with snapshot.c split and probably belongs to some other patch.

I am aware of that. This will conflict with the Nigel's patch, so we probably can arrange to apply that patch before this one, if you prefer.

As far as the split is concerned, if you recall my doubts wrt it, the "bugfixes pending" is the first point on the list. :-)

Greetings,
Rafael

#9 lore raw

Re: [discuss] Re: [PATCH][Fix] swsusp: avoid possible page tables corruption during resume on x86-64

I reworked the patch a bit to do on demand page table setup. Does it still work for you?

-Andi

ftp://ftp.firstfloor.org/pub/ak/x86_64/quilt-current/patches/suspend-pgtables

author
#10 lore raw

Re: [discuss] Re: [PATCH][Fix] swsusp: avoid possible page tables corruption during resume on x86-64

Hi Andi,

Thanks a lot for looking into this!

Quoted 3 lines · Andi Kleen

I reworked the patch a bit to do on demand page table setup. Does it still work for you?

No, it doesn't, because arch_prepare_suspend() is only called during suspend (ie. by the kernel that creates the image) and the temporary page tables have to exist during resume (ie. be available to the kernel that reads the image and restores the saved state of the system).

In principle we can call create_resume_mapping() from swsusp_arch_resume() (ie. from suspend_asm.S), but then the memory allocations in create_resume_mapping(), resume_pud_mapping(), and resume_pmd_mapping() must be made carefully so that we use _only_ NosaveFree pages in them (the other pages are overwritten by the loop in swsusp_arch_resume()). Additionally, we are in atomic context at that time, so we cannot use GFP_KERNEL. Moreover, if one of the allocations fails, we should free all of the allocated pages, so we need to trace them somehow.

All of this is done in the appended patch, except that the functions populating the page tables are located in arch/x86_64/kernel/suspend.c rather than in init.c. It may be done in a more elegan way in the future, with the help of some swsusp patches that are in the works now.

Greetings,
Rafael

Signed-off-by Rafael J. Wysocki <[email protected]>
arch/x86_64/kernel/suspend.c +130 -0
Index: linux-2.6.14-rc3-git1/arch/x86_64/kernel/suspend.c
===================================================================
@@ -11,6 +11,8 @@
 #include <linux/smp.h>
 #include <linux/suspend.h>
 #include <asm/proto.h>
+#include <asm/page.h>
+#include <asm/pgtable.h>
 
 struct saved_context saved_context;
 

@@ -140,4 +142,132 @@
 
 }
 
+#ifdef CONFIG_SOFTWARE_SUSPEND
+/* Defined in kernel/power/swsusp.c */
+extern unsigned long get_usable_page(unsigned gfp_mask);
+extern void free_eaten_memory(void);
+/* Defined in arch/x86_64/kernel/suspend_asm.S */
+extern int restore_image(void);
 
+pgd_t *temp_level4_pgt;
+
+static void **pages;
+
+static inline void *__add_page(void)
+{
+	void **c;
+
+	c = (void **)get_usable_page(GFP_ATOMIC);
+	if (c) {
+		*c = pages;
+		pages = c;
+	}
+	return c;
+}
+
+static inline void *__next_page(void)
+{
+	void **c;
+
+	c = pages;
+	if (c) {
+		pages = *c;
+		*c = NULL;
+	}
+	return c;
+}
+
+/*
+ * Try to allocate as many usable pages as needed and daisy chain them.
+ * If one allocation fails, free the pages allocated so far
+ */
+static int alloc_usable_pages(unsigned long n)
+{
+	void *p;
+
+	pages = NULL;
+	do
+		if (!__add_page())
+			break;
+	while (--n);
+	if (n) {
+		p = __next_page();
+		while (p) {
+			free_page((unsigned long)p);
+			p = __next_page();
+		}
+		return -ENOMEM;
+	}
+	return 0;
+}
+
+static void phys_pud_init(pud_t *pud, unsigned long address, unsigned long end)
+{
+	long i, j;
+
+	i = pud_index(address);
+	pud = pud + i;
+	for (; i < PTRS_PER_PUD; pud++, i++) {
+		unsigned long paddr;
+		pmd_t *pmd;
+
+		paddr = address + i*PUD_SIZE;
+		if (paddr >= end)
+			break;
+
+		pmd = (pmd_t *)__next_page();
+		set_pud(pud, __pud(__pa(pmd) | _KERNPG_TABLE));
+		for (j = 0; j < PTRS_PER_PMD; pmd++, j++, paddr += PMD_SIZE) {
+			unsigned long pe;
+
+			if (paddr >= end)
+				break;
+			pe = _PAGE_NX | _PAGE_PSE | _KERNPG_TABLE | paddr;
+			pe &= __supported_pte_mask;
+			set_pmd(pmd, __pmd(pe));
+		}
+	}
+}
+
+static void set_up_temporary_mappings(void)
+{
+	unsigned long start, end, next;
+
+	temp_level4_pgt = (pgd_t *)__next_page();
+
+	/* It is safe to reuse the original kernel mapping */
+	set_pgd(temp_level4_pgt + pgd_index(__START_KERNEL_map),
+		init_level4_pgt[pgd_index(__START_KERNEL_map)]);
+
+	/* Set up the direct mapping from scratch */
+	start = (unsigned long)pfn_to_kaddr(0);
+	end = (unsigned long)pfn_to_kaddr(end_pfn);
+
+	for (; start < end; start = next) {
+		pud_t *pud = (pud_t *)__next_page();
+		next = start + PGDIR_SIZE;
+		if (next > end)
+			next = end;
+		phys_pud_init(pud, __pa(start), __pa(next));
+		set_pgd(temp_level4_pgt + pgd_index(start),
+			mk_kernel_pgd(__pa(pud)));
+	}
+}
+
+int swsusp_arch_resume(void)
+{
+	unsigned long n;
+
+	n = ((end_pfn << PAGE_SHIFT) + PUD_SIZE - 1) >> PUD_SHIFT;
+	n += (n + PTRS_PER_PUD - 1) / PTRS_PER_PUD + 1;
+	pr_debug("swsusp_arch_resume(): pages needed = %lu\n", n);
+	if (alloc_usable_pages(n)) {
+		free_eaten_memory();
+		return -ENOMEM;
+	}
+	/* We have got enough memory and from now on we cannot recover */
+	set_up_temporary_mappings();
+	restore_image();
+	return 0;
+}
+#endif /* CONFIG_SOFTWARE_SUSPEND */
kernel/power/swsusp.c +4 -3
Index: linux-2.6.14-rc3-git1/kernel/power/swsusp.c
===================================================================
@@ -1095,7 +1095,7 @@
 	*eaten_memory = c;
 }
 
-static unsigned long get_usable_page(unsigned gfp_mask)
+unsigned long get_usable_page(unsigned gfp_mask)
 {
 	unsigned long m;
 

@@ -1109,7 +1109,7 @@
 	return m;
 }
 
-static void free_eaten_memory(void)
+void free_eaten_memory(void)
 {
 	unsigned long m;
 	void **c;

@@ -1481,11 +1481,12 @@
 	/* Allocate memory for the image and read the data from swap */
 
 	error = check_pagedir(pagedir_nosave);
-	free_eaten_memory();
+
 	if (!error)
 		error = data_read(pagedir_nosave);
 
 	if (error) { /* We fail cleanly */
+		free_eaten_memory();
 		for_each_pbe (p, pagedir_nosave)
 			if (p->address) {
 				free_page(p->address);
arch/x86_64/kernel/suspend_asm.S +11 -6
Index: linux-2.6.14-rc3-git1/arch/x86_64/kernel/suspend_asm.S
===================================================================
@@ -39,12 +39,13 @@
 	call swsusp_save
 	ret
 
-ENTRY(swsusp_arch_resume)
-	/* set up cr3 */	
-	leaq	init_level4_pgt(%rip),%rax
-	subq	$__START_KERNEL_map,%rax
-	movq	%rax,%cr3
-
+ENTRY(restore_image)
+	/* switch to temporary page tables */
+	movq	$__PAGE_OFFSET, %rdx
+	movq	temp_level4_pgt(%rip), %rax
+	subq	%rdx, %rax
+	movq	%rax, %cr3
+	/* Flush TLB */
 	movq	mmu_cr4_features(%rip), %rax
 	movq	%rax, %rdx
 	andq	$~(1<<7), %rdx	# PGE

@@ -69,6 +70,10 @@
 	movq	pbe_next(%rdx), %rdx
 	jmp	loop
 done:
+	/* go back to the original page tables */
+	leaq	init_level4_pgt(%rip), %rax
+	subq	$__START_KERNEL_map, %rax
+	movq	%rax, %cr3
 	/* Flush TLB, including "global" things (vmalloc) */
 	movq	mmu_cr4_features(%rip), %rax
 	movq	%rax, %rdx