[RFC][PATCH -mm 0/4] Hibernation: Arbitrary boot kernel support on x86_64

[RFC][PATCH -mm 0/4] Hibernation: Arbitrary boot kernel support on x86_64

[Rafael J. Wysocki]

Hi,

After the kexec hibernation discussion I started to think what it would take to
change the restore code so that a hibernation image could be loaded and
restored with the help of a kernel different from the image one.  As a result,
I have created the following series of patches.

Except for that being an interesting problem, my motivation to do it is
two-fold.  First, it will allow us to eliminate the (annoying) requirement to
use the same kernel for hibernation and restore.  Second, it will allow us to
carry out the restore in a more or less ACPI-compliant way, by using an
ACPI-less kernel as the boot one.

As you can see in the patches, it doesn't take a lot of code to do that,
although conceptually it is a bit compilcated.  The first patch is the
essential one, as it contains all of the necessary bits.  The second one
makes the ACPI restore code work if ACPI has not been enabled during boot
before loading the image.  The last two patches are auxiliary, although they
improve the flexibility of the restore code.

Comments welcome.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

[RFC][PATCH -mm 1/4] Hibernation: Arbitrary boot kernel support on x86_64

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Make it possible to restore a hibernation image on x86_64 with the help of a
kernel different from the one in the image.

The idea is to split the core restoration code into two separate parts and to
place each of them in a different page.  The first part belongs to the boot
kernel and is executed as the last step of the image kernel's memory restoration
procedure.  It restores all of the image kernel's memory that has not been
restored yet except for the one page containing the very code that is being
executed at that time.  The final operation performed by it is a jump to the
second part of the core restoration code that belongs to the image kernel and
has just been restored.  This code restores the last remaining page of the image
kernel's memory containing the first, already executed, part of the core
restoration code (temporary page tables created by the boot kernel are used at
this stage).  It also makes the CPU switch to the image kernel's page tables and
restores the state of general purpose registers (including the stack pointer)
from before the hibernation.

The main issue with this idea is that in order to jump to the second part of the
restoration code the boot kernel needs to know its address.  However, this
address may be passed to it in the image header.  Namely, the part of the image
header previously used for checking if the version of the image kernel is
correct can be replaced with an architecture specific data that will allow the
boot kernel to jump to the right address within the image kernel.  This data
should also be used for checking if the image kernel is compatible with the boot
kernel (as far as the memory restroration procedure is concerned).  It can be
done, for example, with the help of a "magic" value that has to be equal in both
kernels, so that they can be regarded as compatible.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 arch/x86_64/kernel/suspend.c     |   43 +++++++++++++++++++++++++++
 arch/x86_64/kernel/suspend_asm.S |   61 +++++++++++++++++++++++++++++++++------
 include/asm-x86_64/suspend.h     |    6 +++
 kernel/power/power.h             |    6 ++-
 kernel/power/snapshot.c          |   60 ++++++++++++++++++++++++++++----------
 5 files changed, 150 insertions(+), 26 deletions(-)

Index: linux-2.6.23-rc3/arch/x86_64/kernel/suspend_asm.S
===================================================================
--- linux-2.6.23-rc3.orig/arch/x86_64/kernel/suspend_asm.S	2007-08-19 14:43:19.000000000 +0200
+++ linux-2.6.23-rc3/arch/x86_64/kernel/suspend_asm.S	2007-08-19 19:12:56.000000000 +0200
@@ -2,8 +2,8 @@
  *
  * Distribute under GPLv2.
  *
- * swsusp_arch_resume may not use any stack, nor any variable that is
- * not "NoSave" during copying pages:
+ * swsusp_arch_resume must not use any stack or any nonlocal variables while
+ * copying pages:
  *
  * Its rewriting one kernel image with another. What is stack in "old"
  * image could very well be data page in "new" image, and overwriting
@@ -36,10 +36,20 @@ ENTRY(swsusp_arch_suspend)
 	pushfq
 	popq	pt_regs_eflags(%rax)
 
+	/* save the address of restore_registers */
+	movq	$restore_registers, %rax
+	movq	%rax, restore_jump_address(%rip)
+
 	call swsusp_save
 	ret
 
 ENTRY(restore_image)
+	/* compute the address of the page we are at and store it in R9 */
+	movq	$(restore_image - __START_KERNEL_map), %rax
+	movq	$__PAGE_OFFSET, %r9
+	addq	%rax, %r9
+	andq	$PAGE_MASK, %r9
+
 	/* switch to temporary page tables */
 	movq	$__PAGE_OFFSET, %rdx
 	movq	temp_level4_pgt(%rip), %rax
@@ -54,6 +64,11 @@ ENTRY(restore_image)
 	movq	%rcx, %cr3;
 	movq	%rax, %cr4;  # turn PGE back on
 
+	/* prepare to jump to the image kernel */
+	movq	restore_jump_address(%rip), %rax
+
+	/* copy image data to their original locations */
+	xorq	%r10, %r10
 	movq	restore_pblist(%rip), %rdx
 loop:
 	testq	%rdx, %rdx
@@ -62,16 +77,44 @@ loop:
 	/* get addresses from the pbe and copy the page */
 	movq	pbe_address(%rdx), %rsi
 	movq	pbe_orig_address(%rdx), %rdi
-	movq	$512, %rcx
+	/* skip the page we are at (address stored in R9) */
+	cmpq	%rdi, %r9
+	jne	1f
+	movq	%rsi, %r10
+	jmp	2f
+1:	movq	$(PAGE_SIZE >> 3), %rcx
 	rep
 	movsq
 
 	/* progress to the next pbe */
-	movq	pbe_next(%rdx), %rdx
+2:	movq	pbe_next(%rdx), %rdx
 	jmp	loop
 done:
+	/* jump to the restore_registers address from the image header */
+	jmpq	*%rax
+	/*
+	 * NOTE: This assumes that the boot kernel's text mapping covers the
+	 * image kernel's page containing restore_registers and the address of
+	 * this page is the same as in the image kernel's text mapping (it
+	 * should always be true, because the text mapping is linear, starting
+	 * from 0, and is supposed to cover the entire kernel text for every
+	 * kernel).
+	 */
+
+.balign PAGE_SIZE
+ENTRY(restore_registers)
+	/* we are in the image kernel's text now */
+	testq	%r10, %r10
+	jz	1f
+	/* copy the skipped page */
+	movq	%r10, %rsi
+	movq	%r9, %rdi
+	movq	$(PAGE_SIZE >> 3), %rcx
+	rep
+	movsq
+
 	/* go back to the original page tables */
-	movq    $(init_level4_pgt - __START_KERNEL_map), %rax
+1:	movq    $(init_level4_pgt - __START_KERNEL_map), %rax
 	addq    phys_base(%rip), %rax
 	movq    %rax, %cr3
 
@@ -84,10 +127,7 @@ done:
 	movq	%rcx, %cr3
 	movq	%rax, %cr4;  # turn PGE back on
 
-	movl	$24, %eax
-	movl	%eax, %ds
-
-	/* We don't restore %rax, it must be 0 anyway */
+	/* restore GPRs (we don't restore %rax, it must be 0 anyway) */
 	movq	$saved_context, %rax
 	movq	pt_regs_rsp(%rax), %rsp
 	movq	pt_regs_rbp(%rax), %rbp
@@ -109,4 +149,7 @@ done:
 
 	xorq	%rax, %rax
 
+	/* tell the hibernation core that we've just restored the memory */
+	movq	%rax, in_suspend(%rip)
+
 	ret
Index: linux-2.6.23-rc3/arch/x86_64/kernel/suspend.c
===================================================================
--- linux-2.6.23-rc3.orig/arch/x86_64/kernel/suspend.c	2007-08-19 14:43:19.000000000 +0200
+++ linux-2.6.23-rc3/arch/x86_64/kernel/suspend.c	2007-08-19 19:12:38.000000000 +0200
@@ -144,6 +144,12 @@ void fix_processor_context(void)
 /* Defined in arch/x86_64/kernel/suspend_asm.S */
 extern int restore_image(void);
 
+/*
+ * Address to jump to in the last phase of restore in order to get to the image
+ * kernel's text (this value is passed in the image header).
+ */
+unsigned long restore_jump_address;
+
 pgd_t *temp_level4_pgt;
 
 static int res_phys_pud_init(pud_t *pud, unsigned long address, unsigned long end)
@@ -230,4 +236,41 @@ int pfn_is_nosave(unsigned long pfn)
 	unsigned long nosave_end_pfn = PAGE_ALIGN(__pa_symbol(&__nosave_end)) >> PAGE_SHIFT;
 	return (pfn >= nosave_begin_pfn) && (pfn < nosave_end_pfn);
 }
+
+struct restore_data_record {
+	unsigned long jump_address;
+	unsigned long control;
+};
+
+#define RESTORE_MAGIC	0x0123456789ABCDEFUL
+
+/**
+ *	arch_hibernation_header_save - populate the architecture specific part
+ *		of a hibernation image header
+ *	@addr: address to save the data at
+ */
+int arch_hibernation_header_save(void *addr, unsigned int max_size)
+{
+	struct restore_data_record *rdr = addr;
+
+	if (max_size < sizeof(struct restore_data_record))
+		return -EOVERFLOW;
+	rdr->jump_address = restore_jump_address;
+	rdr->control = (restore_jump_address ^ RESTORE_MAGIC);
+	return 0;
+}
+
+/**
+ *	arch_hibernation_header_restore - read the architecture specific data
+ *		from the hibernation image header
+ *	@addr: address to read the data from
+ */
+int arch_hibernation_header_restore(void *addr)
+{
+	struct restore_data_record *rdr = addr;
+
+	restore_jump_address = rdr->jump_address;
+	return (rdr->control == (restore_jump_address ^ RESTORE_MAGIC)) ?
+			0 : -EINVAL;
+}
 #endif /* CONFIG_HIBERNATION */
Index: linux-2.6.23-rc3/include/asm-x86_64/suspend.h
===================================================================
--- linux-2.6.23-rc3.orig/include/asm-x86_64/suspend.h	2007-08-19 14:43:19.000000000 +0200
+++ linux-2.6.23-rc3/include/asm-x86_64/suspend.h	2007-08-19 15:09:50.000000000 +0200
@@ -43,4 +43,10 @@ extern void fix_processor_context(void);
 /* routines for saving/restoring kernel state */
 extern int acpi_save_state_mem(void);
 
+#define ARCH_HAS_HIBERNATION_HEADER
+
+/* arch/x86_64/kernel/suspend.c */
+extern int arch_hibernation_header_save(void *addr, unsigned int max_size);
+extern int arch_hibernation_header_restore(void *addr);
+
 #endif /* __ASM_X86_64_SUSPEND_H */
Index: linux-2.6.23-rc3/kernel/power/power.h
===================================================================
--- linux-2.6.23-rc3.orig/kernel/power/power.h	2007-08-19 14:42:41.000000000 +0200
+++ linux-2.6.23-rc3/kernel/power/power.h	2007-08-19 14:44:12.000000000 +0200
@@ -11,14 +11,16 @@ struct swsusp_info {
 	unsigned long		size;
 } __attribute__((aligned(PAGE_SIZE)));
 
-
-
 #ifdef CONFIG_HIBERNATION
+/* Maximum size of architecture specific data in a hibernation header */
+#define MAX_ARCH_HEADER_SIZE	(sizeof(struct new_utsname) + 4)
+
 /*
  * Keep some memory free so that I/O operations can succeed without paging
  * [Might this be more than 4 MB?]
  */
 #define PAGES_FOR_IO	((4096 * 1024) >> PAGE_SHIFT)
+
 /*
  * Keep 1 MB of memory free so that device drivers can allocate some pages in
  * their .suspend() routines without breaking the suspend to disk.
Index: linux-2.6.23-rc3/kernel/power/snapshot.c
===================================================================
--- linux-2.6.23-rc3.orig/kernel/power/snapshot.c	2007-08-19 14:42:41.000000000 +0200
+++ linux-2.6.23-rc3/kernel/power/snapshot.c	2007-08-19 14:44:12.000000000 +0200
@@ -1239,17 +1239,29 @@ asmlinkage int swsusp_save(void)
 	return 0;
 }
 
-static void init_header(struct swsusp_info *info)
+#ifdef ARCH_HAS_HIBERNATION_HEADER
+static int init_header_complete(struct swsusp_info *info)
 {
-	memset(info, 0, sizeof(struct swsusp_info));
+	return arch_hibernation_header_save(info, MAX_ARCH_HEADER_SIZE);
+}
+#else /* !ARCH_HAS_HIBERNATION_HEADER */
+static int init_header_complete(struct swsusp_info *info)
+{
+	memcpy(&info->uts, init_utsname(), sizeof(struct new_utsname));
 	info->version_code = LINUX_VERSION_CODE;
+	return 0;
+}
+#endif /* !ARCH_HAS_HIBERNATION_HEADER */
+
+static int init_header(struct swsusp_info *info)
+{
+	memset(info, 0, sizeof(struct swsusp_info));
 	info->num_physpages = num_physpages;
-	memcpy(&info->uts, init_utsname(), sizeof(struct new_utsname));
-	info->cpus = num_online_cpus();
 	info->image_pages = nr_copy_pages;
 	info->pages = nr_copy_pages + nr_meta_pages + 1;
 	info->size = info->pages;
 	info->size <<= PAGE_SHIFT;
+	return init_header_complete(info);
 }
 
 /**
@@ -1303,7 +1315,11 @@ int snapshot_read_next(struct snapshot_h
 			return -ENOMEM;
 	}
 	if (!handle->offset) {
-		init_header((struct swsusp_info *)buffer);
+		int error;
+
+		error = init_header((struct swsusp_info *)buffer);
+		if (error)
+			return error;
 		handle->buffer = buffer;
 		memory_bm_position_reset(&orig_bm);
 		memory_bm_position_reset(&copy_bm);
@@ -1394,22 +1410,36 @@ duplicate_memory_bitmap(struct memory_bi
 	}
 }
 
-static inline int check_header(struct swsusp_info *info)
+#ifdef ARCH_HAS_HIBERNATION_HEADER
+static char *check_image_kernel(struct swsusp_info *info)
+{
+	return arch_hibernation_header_restore(info) ?
+			"architecture specific data" : NULL;
+}
+#else /* !ARCH_HAS_HIBERNATION_HEADER */
+static char *check_image_kernel(struct swsusp_info *info)
 {
-	char *reason = NULL;
-
 	if (info->version_code != LINUX_VERSION_CODE)
-		reason = "kernel version";
-	if (info->num_physpages != num_physpages)
-		reason = "memory size";
+		return "kernel version";
 	if (strcmp(info->uts.sysname,init_utsname()->sysname))
-		reason = "system type";
+		return "system type";
 	if (strcmp(info->uts.release,init_utsname()->release))
-		reason = "kernel release";
+		return "kernel release";
 	if (strcmp(info->uts.version,init_utsname()->version))
-		reason = "version";
+		return "version";
 	if (strcmp(info->uts.machine,init_utsname()->machine))
-		reason = "machine";
+		return "machine";
+	return NULL;
+}
+#endif /* !ARCH_HAS_HIBERNATION_HEADER */
+
+static int check_header(struct swsusp_info *info)
+{
+	char *reason;
+
+	reason = check_image_kernel(info);
+	if (!reason && info->num_physpages != num_physpages)
+		reason = "memory size";
 	if (reason) {
 		printk(KERN_ERR "swsusp: Resume mismatch: %s\n", reason);
 		return -EPERM;

Re: [RFC][PATCH -mm 1/4] Hibernation: Arbitrary boot kernel support on x86_64

[Pavel Machek]

Hi!

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Make it possible to restore a hibernation image on x86_64 with the help of a
> kernel different from the one in the image.

Looks mostly ok to me.

Should this be split in half (generic support, x86-64 support) and be
done last in the series, so that suspend still works when bisecting?

>  done:
> +	/* jump to the restore_registers address from the image header */
> +	jmpq	*%rax

So this is where the change from kernel text 1 to kernel text 2
happens, right? I see you are using %r10 for something here, perhaps
that should be commented?

> +.balign PAGE_SIZE
> +ENTRY(restore_registers)
> +	/* we are in the image kernel's text now */
> +	testq	%r10, %r10
> +	jz	1f
> +	/* copy the skipped page */
> +	movq	%r10, %rsi
> +	movq	%r9, %rdi
> +	movq	$(PAGE_SIZE >> 3), %rcx
> +	rep
> +	movsq

> @@ -84,10 +127,7 @@ done:
>  	movq	%rcx, %cr3
>  	movq	%rax, %cr4;  # turn PGE back on
>  
> -	movl	$24, %eax
> -	movl	%eax, %ds
> -
> -	/* We don't restore %rax, it must be 0 anyway */
> +	/* restore GPRs (we don't restore %rax, it must be 0 anyway) */
>  	movq	$saved_context, %rax
>  	movq	pt_regs_rsp(%rax), %rsp
>  	movq	pt_regs_rbp(%rax), %rbp

Hmm, in the old code, we knew we don't have to restore %ds, because it
is constant for one kernel. Now, we rely on %ds being constant accross
kernels. Not nice, and should be at least documented.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [discuss] [RFC][PATCH -mm 1/4] Hibernation: Arbitrary boot kernel support on x86_64

[Rafael J. Wysocki]

On Tuesday, 21 August 2007 09:57, Pavel Machek wrote:
> Hi!
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Make it possible to restore a hibernation image on x86_64 with the help of a
> > kernel different from the one in the image.
> 
> Looks mostly ok to me.
> 
> Should this be split in half (generic support, x86-64 support) and be
> done last in the series, so that suspend still works when bisecting?

Yes, I can do that, but the x86-64 support must go at least before the 4/4
patch which depends on it.

> >  done:
> > +	/* jump to the restore_registers address from the image header */
> > +	jmpq	*%rax
> 
> So this is where the change from kernel text 1 to kernel text 2
> happens, right?

Yes.

> I see you are using %r10 for something here, perhaps that should be
> commented? 

%r10 is used to store the address of the data to copy into the skipped page.
I'll try to add a comment for that.

> > +.balign PAGE_SIZE
> > +ENTRY(restore_registers)
> > +	/* we are in the image kernel's text now */
> > +	testq	%r10, %r10
> > +	jz	1f
> > +	/* copy the skipped page */
> > +	movq	%r10, %rsi
> > +	movq	%r9, %rdi
> > +	movq	$(PAGE_SIZE >> 3), %rcx
> > +	rep
> > +	movsq
> 
> > @@ -84,10 +127,7 @@ done:
> >  	movq	%rcx, %cr3
> >  	movq	%rax, %cr4;  # turn PGE back on
> >  
> > -	movl	$24, %eax
> > -	movl	%eax, %ds
> > -
> > -	/* We don't restore %rax, it must be 0 anyway */
> > +	/* restore GPRs (we don't restore %rax, it must be 0 anyway) */
> >  	movq	$saved_context, %rax
> >  	movq	pt_regs_rsp(%rax), %rsp
> >  	movq	pt_regs_rbp(%rax), %rbp
> 
> Hmm, in the old code, we knew we don't have to restore %ds, because it
> is constant for one kernel. Now, we rely on %ds being constant accross
> kernels. Not nice, and should be at least documented.

Well, in fact we rely on it all the time (eg. the
"movq mmu_cr4_features(%rip), %rax" above wouldn't work if that's not true),
but I can keep the old code here just fine. ;-)

I'll send the reworked patch series in a new thread.

Greetings,
Rafael

Re: [discuss] [RFC][PATCH -mm 1/4] Hibernation: Arbitrary boot kernel support on x86_64

[Rafael J. Wysocki]

On Tuesday, 21 August 2007 16:37, Rafael J. Wysocki wrote:
> On Tuesday, 21 August 2007 09:57, Pavel Machek wrote:
> > Hi!
> > 
> > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > 
> > > Make it possible to restore a hibernation image on x86_64 with the help of a
> > > kernel different from the one in the image.
> > 
> > Looks mostly ok to me.
> > 
> > Should this be split in half (generic support, x86-64 support) and be
> > done last in the series, so that suspend still works when bisecting?
> 
> Yes, I can do that, but the x86-64 support must go at least before the 4/4
> patch which depends on it.
> 
> > >  done:
> > > +	/* jump to the restore_registers address from the image header */
> > > +	jmpq	*%rax
> > 
> > So this is where the change from kernel text 1 to kernel text 2
> > happens, right?
> 
> Yes.
> 
> > I see you are using %r10 for something here, perhaps that should be
> > commented? 
> 
> %r10 is used to store the address of the data to copy into the skipped page.
> I'll try to add a comment for that.
> 
> > > +.balign PAGE_SIZE
> > > +ENTRY(restore_registers)
> > > +	/* we are in the image kernel's text now */
> > > +	testq	%r10, %r10
> > > +	jz	1f
> > > +	/* copy the skipped page */
> > > +	movq	%r10, %rsi
> > > +	movq	%r9, %rdi
> > > +	movq	$(PAGE_SIZE >> 3), %rcx
> > > +	rep
> > > +	movsq
> > 
> > > @@ -84,10 +127,7 @@ done:
> > >  	movq	%rcx, %cr3
> > >  	movq	%rax, %cr4;  # turn PGE back on
> > >  
> > > -	movl	$24, %eax
> > > -	movl	%eax, %ds
> > > -
> > > -	/* We don't restore %rax, it must be 0 anyway */
> > > +	/* restore GPRs (we don't restore %rax, it must be 0 anyway) */
> > >  	movq	$saved_context, %rax
> > >  	movq	pt_regs_rsp(%rax), %rsp
> > >  	movq	pt_regs_rbp(%rax), %rbp
> > 
> > Hmm, in the old code, we knew we don't have to restore %ds, because it
> > is constant for one kernel. Now, we rely on %ds being constant accross
> > kernels. Not nice, and should be at least documented.
> 
> Well, in fact we rely on it all the time (eg. the
> "movq mmu_cr4_features(%rip), %rax" above wouldn't work if that's not true),
> but I can keep the old code here just fine. ;-)

Well, I was wrong.  The new code doesn't work without this change and I'm not
exactly sure why.

Greetings,
Rafael

Re: [discuss] [RFC][PATCH -mm 1/4] Hibernation: Arbitrary boot kernel support on x86_64

[Rafael J. Wysocki]

On Tuesday, 21 August 2007 21:30, Rafael J. Wysocki wrote:
> On Tuesday, 21 August 2007 16:37, Rafael J. Wysocki wrote:
> > On Tuesday, 21 August 2007 09:57, Pavel Machek wrote:
[--snip--]
> > > >  
> > > > -	movl	$24, %eax
> > > > -	movl	%eax, %ds
> > > > -
> > > > -	/* We don't restore %rax, it must be 0 anyway */
> > > > +	/* restore GPRs (we don't restore %rax, it must be 0 anyway) */
> > > >  	movq	$saved_context, %rax
> > > >  	movq	pt_regs_rsp(%rax), %rsp
> > > >  	movq	pt_regs_rbp(%rax), %rbp
> > > 
> > > Hmm, in the old code, we knew we don't have to restore %ds, because it
> > > is constant for one kernel. Now, we rely on %ds being constant accross
> > > kernels. Not nice, and should be at least documented.
> > 
> > Well, in fact we rely on it all the time (eg. the
> > "movq mmu_cr4_features(%rip), %rax" above wouldn't work if that's not true),
> > but I can keep the old code here just fine. ;-)
> 
> Well, I was wrong.  The new code doesn't work without this change and I'm not
> exactly sure why.

Actually, it works if the boot kernel is the same as the image kernel and it
doesn't work if the boot kernel is different.

I think what happens is that by loading %ds with a new selector we cause
the descriptor to be loaded from the descriptor table pointed to by the boot
kernel's GDT and this need not be in the same area as the image kernel's
global descriptor table.  If that is the case, an exception will trigger, so we
should not load %ds until the GDTR value is restored.

Moreover, according to the AMD tech docs, in 64-bit mode the value of %ds is
ignored, so we don't need to bother loading it anyway.

Greetings,
Rafael

[RFC][PATCH -mm 2/4] Hibernation: Turn on ACPI during restore if necessary

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

If the boot kernel doesn't support ACPI and ACPI is not enabled by the platform
boot code, it may be possible to enable ACPI after restoring the system memory
from a hibernation image.  Implement that.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/sleep/main.c |    2 ++
 1 file changed, 2 insertions(+)

Index: linux-2.6.23-rc3/drivers/acpi/sleep/main.c
===================================================================
--- linux-2.6.23-rc3.orig/drivers/acpi/sleep/main.c	2007-08-14 00:41:10.000000000 +0200
+++ linux-2.6.23-rc3/drivers/acpi/sleep/main.c	2007-08-19 20:01:05.000000000 +0200
@@ -240,6 +240,8 @@ static int acpi_hibernation_enter(void)
 
 static void acpi_hibernation_finish(void)
 {
+	/* If the boot kernel doesn't support ACPI, we may need to enable it */
+	acpi_enable();
 	acpi_leave_sleep_state(ACPI_STATE_S4);
 	acpi_disable_wakeup_device(ACPI_STATE_S4);
 

Re: [RFC][PATCH -mm 2/4] Hibernation: Turn on ACPI during restore if necessary

[Pavel Machek]

Hi!

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> If the boot kernel doesn't support ACPI and ACPI is not enabled by the platform
> boot code, it may be possible to enable ACPI after restoring the system memory
> from a hibernation image.  Implement that.

ACK... and I guess this should go in early. It is possible to test w/o
rest of patches, (just pass acpi=off to resume kernel, no?), and it
should get lot of testing.
								Pavel


> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/acpi/sleep/main.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> Index: linux-2.6.23-rc3/drivers/acpi/sleep/main.c
> ===================================================================
> --- linux-2.6.23-rc3.orig/drivers/acpi/sleep/main.c	2007-08-14 00:41:10.000000000 +0200
> +++ linux-2.6.23-rc3/drivers/acpi/sleep/main.c	2007-08-19 20:01:05.000000000 +0200
> @@ -240,6 +240,8 @@ static int acpi_hibernation_enter(void)
>  
>  static void acpi_hibernation_finish(void)
>  {
> +	/* If the boot kernel doesn't support ACPI, we may need to enable it */
> +	acpi_enable();
>  	acpi_leave_sleep_state(ACPI_STATE_S4);
>  	acpi_disable_wakeup_device(ACPI_STATE_S4);
>  

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC][PATCH -mm 2/4] Hibernation: Turn on ACPI during restore if necessary

[Rafael J. Wysocki]

On Tuesday, 21 August 2007 09:57, Pavel Machek wrote:
> Hi!
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > If the boot kernel doesn't support ACPI and ACPI is not enabled by the platform
> > boot code, it may be possible to enable ACPI after restoring the system memory
> > from a hibernation image.  Implement that.
> 
> ACK... and I guess this should go in early. It is possible to test w/o
> rest of patches, (just pass acpi=off to resume kernel, no?),

Yup, that might work.

> and it  should get lot of testing.

Why do you think so?

Rafael


> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  drivers/acpi/sleep/main.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > Index: linux-2.6.23-rc3/drivers/acpi/sleep/main.c
> > ===================================================================
> > --- linux-2.6.23-rc3.orig/drivers/acpi/sleep/main.c	2007-08-14 00:41:10.000000000 +0200
> > +++ linux-2.6.23-rc3/drivers/acpi/sleep/main.c	2007-08-19 20:01:05.000000000 +0200
> > @@ -240,6 +240,8 @@ static int acpi_hibernation_enter(void)
> >  
> >  static void acpi_hibernation_finish(void)
> >  {
> > +	/* If the boot kernel doesn't support ACPI, we may need to enable it */
> > +	acpi_enable();
> >  	acpi_leave_sleep_state(ACPI_STATE_S4);
> >  	acpi_disable_wakeup_device(ACPI_STATE_S4);
> >  
> 

-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [RFC][PATCH -mm 2/4] Hibernation: Turn on ACPI during restore if necessary

[Pavel Machek]

On Tue 2007-08-21 16:39:01, Rafael J. Wysocki wrote:
> On Tuesday, 21 August 2007 09:57, Pavel Machek wrote:
> > Hi!
> > 
> > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > 
> > > If the boot kernel doesn't support ACPI and ACPI is not enabled by the platform
> > > boot code, it may be possible to enable ACPI after restoring the system memory
> > > from a hibernation image.  Implement that.
> > 
> > ACK... and I guess this should go in early. It is possible to test w/o
> > rest of patches, (just pass acpi=off to resume kernel, no?),
> 
> Yup, that might work.
> 
> > and it  should get lot of testing.
> 
> Why do you think so?

Anything that touches acpi is almost guaranteed to produce nice
fireworks :-). Okay, maybe I'm too paranoid.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [discuss] [RFC][PATCH -mm 2/4] Hibernation: Turn on ACPI during restore if necessary

[Rafael J. Wysocki]

On Wednesday, 22 August 2007 01:36, Pavel Machek wrote:
> On Tue 2007-08-21 16:39:01, Rafael J. Wysocki wrote:
> > On Tuesday, 21 August 2007 09:57, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > 
> > > > If the boot kernel doesn't support ACPI and ACPI is not enabled by the platform
> > > > boot code, it may be possible to enable ACPI after restoring the system memory
> > > > from a hibernation image.  Implement that.
> > > 
> > > ACK... and I guess this should go in early. It is possible to test w/o
> > > rest of patches, (just pass acpi=off to resume kernel, no?),
> > 
> > Yup, that might work.
> > 
> > > and it  should get lot of testing.
> > 
> > Why do you think so?
> 
> Anything that touches acpi is almost guaranteed to produce nice
> fireworks :-). Okay, maybe I'm too paranoid.

Well, there's a catch, but it requires quite a bit more patching.

Namely, some ACPI code may be called from within CPU hotplug, so in
principle we should enable ACPI before that.  Fortunately, on contemporary SMP
machines ACPI is usually enabled by the BIOS, AFAICS, so that should not be
an issue.

Still, I have some patches that handle it, among other things.  I think I'll
just post them (tomorrow).

Greetings,
Rafael

[RFC][PATCH -mm 3/4] Hibernation: Use temporary kernel text mapping during restore on x86_64

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Use temporary page tables for the kernel text mapping during hibernation restore
on x86_64.

Without the patch, the original boot kernel's page tables that represent the
kernel text mapping are used while the core of the image kernel is being
restored.  However, in principle, if the boot kernel is not identical to the
image kernel, the location of these page tables in the image kernel need not be
the same, so we should create a safe copy of the kernel text mapping prior to
restoring the core of the image kernel.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 arch/x86_64/kernel/suspend.c |   41 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

Index: linux-2.6.23-rc3/arch/x86_64/kernel/suspend.c
===================================================================
--- linux-2.6.23-rc3.orig/arch/x86_64/kernel/suspend.c	2007-08-14 01:06:28.000000000 +0200
+++ linux-2.6.23-rc3/arch/x86_64/kernel/suspend.c	2007-08-14 01:06:34.000000000 +0200
@@ -175,7 +175,7 @@ static int res_phys_pud_init(pud_t *pud,
 
 			if (paddr >= end)
 				break;
-			pe = _PAGE_NX | _PAGE_PSE | _KERNPG_TABLE | paddr;
+			pe = __PAGE_KERNEL_LARGE | paddr;
 			pe &= __supported_pte_mask;
 			set_pmd(pmd, __pmd(pe));
 		}
@@ -183,25 +183,42 @@ static int res_phys_pud_init(pud_t *pud,
 	return 0;
 }
 
+static int res_kernel_text_pud_init(pud_t *pud, unsigned long start)
+{
+	pmd_t *pmd;
+	unsigned long paddr;
+
+	pmd = (pmd_t *)get_safe_page(GFP_ATOMIC);
+	if (!pmd)
+		return -ENOMEM;
+	set_pud(pud + pud_index(start), __pud(__pa(pmd) | _KERNPG_TABLE));
+	for (paddr = 0; paddr < KERNEL_TEXT_SIZE; pmd++, paddr += PMD_SIZE) {
+		unsigned long pe;
+
+		pe = __PAGE_KERNEL_LARGE_EXEC | _PAGE_GLOBAL | paddr;
+		pe &= __supported_pte_mask;
+		set_pmd(pmd, __pmd(pe));
+	}
+
+	return 0;
+}
+
 static int set_up_temporary_mappings(void)
 {
 	unsigned long start, end, next;
+	pud_t *pud;
 	int error;
 
 	temp_level4_pgt = (pgd_t *)get_safe_page(GFP_ATOMIC);
 	if (!temp_level4_pgt)
 		return -ENOMEM;
 
-	/* 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 *)get_safe_page(GFP_ATOMIC);
+		pud = (pud_t *)get_safe_page(GFP_ATOMIC);
 		if (!pud)
 			return -ENOMEM;
 		next = start + PGDIR_SIZE;
@@ -212,7 +229,17 @@ static int set_up_temporary_mappings(voi
 		set_pgd(temp_level4_pgt + pgd_index(start),
 			mk_kernel_pgd(__pa(pud)));
 	}
-	return 0;
+
+	/* Set up the kernel text mapping from scratch */
+	pud = (pud_t *)get_safe_page(GFP_ATOMIC);
+	if (!pud)
+		return -ENOMEM;
+	error = res_kernel_text_pud_init(pud, __START_KERNEL_map);
+	if (!error)
+		set_pgd(temp_level4_pgt + pgd_index(__START_KERNEL_map),
+			__pgd(__pa(pud) | _PAGE_TABLE));
+
+	return error;
 }
 
 int swsusp_arch_resume(void)

Re: [RFC][PATCH -mm 3/4] Hibernation: Use temporary kernel text mapping during restore on x86_64

[Pavel Machek]

Hi!

> Use temporary page tables for the kernel text mapping during hibernation restore
> on x86_64.
> 
> Without the patch, the original boot kernel's page tables that represent the
> kernel text mapping are used while the core of the image kernel is being
> restored.  However, in principle, if the boot kernel is not identical to the
> image kernel, the location of these page tables in the image kernel need not be
> the same, so we should create a safe copy of the kernel text mapping prior to
> restoring the core of the image kernel.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  arch/x86_64/kernel/suspend.c |   41 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 34 insertions(+), 7 deletions(-)
> 
> Index: linux-2.6.23-rc3/arch/x86_64/kernel/suspend.c
> ===================================================================
> --- linux-2.6.23-rc3.orig/arch/x86_64/kernel/suspend.c	2007-08-14 01:06:28.000000000 +0200
> +++ linux-2.6.23-rc3/arch/x86_64/kernel/suspend.c	2007-08-14 01:06:34.000000000 +0200
> @@ -175,7 +175,7 @@ static int res_phys_pud_init(pud_t *pud,
>  
>  			if (paddr >= end)
>  				break;
> -			pe = _PAGE_NX | _PAGE_PSE | _KERNPG_TABLE | paddr;
> +			pe = __PAGE_KERNEL_LARGE | paddr;
>  			pe &= __supported_pte_mask;
>  			set_pmd(pmd, __pmd(pe));
>  		}
> @@ -183,25 +183,42 @@ static int res_phys_pud_init(pud_t *pud,
>  	return 0;
>  }
>  
> +static int res_kernel_text_pud_init(pud_t *pud, unsigned long start)
> +{
> +	pmd_t *pmd;
> +	unsigned long paddr;
> +
> +	pmd = (pmd_t *)get_safe_page(GFP_ATOMIC);
> +	if (!pmd)
> +		return -ENOMEM;
> +	set_pud(pud + pud_index(start), __pud(__pa(pmd) | _KERNPG_TABLE));
> +	for (paddr = 0; paddr < KERNEL_TEXT_SIZE; pmd++, paddr += PMD_SIZE) {

Should we have something like "< max possible kernel size here"? IOW
this needs to cover both suspend _and_ resume kernels, no?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC][PATCH -mm 3/4] Hibernation: Use temporary kernel text mapping during restore on x86_64

[Rafael J. Wysocki]

On Tuesday, 21 August 2007 09:59, Pavel Machek wrote:
> Hi!
> 
> > Use temporary page tables for the kernel text mapping during hibernation restore
> > on x86_64.
> > 
> > Without the patch, the original boot kernel's page tables that represent the
> > kernel text mapping are used while the core of the image kernel is being
> > restored.  However, in principle, if the boot kernel is not identical to the
> > image kernel, the location of these page tables in the image kernel need not be
> > the same, so we should create a safe copy of the kernel text mapping prior to
> > restoring the core of the image kernel.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  arch/x86_64/kernel/suspend.c |   41 ++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 34 insertions(+), 7 deletions(-)
> > 
> > Index: linux-2.6.23-rc3/arch/x86_64/kernel/suspend.c
> > ===================================================================
> > --- linux-2.6.23-rc3.orig/arch/x86_64/kernel/suspend.c	2007-08-14 01:06:28.000000000 +0200
> > +++ linux-2.6.23-rc3/arch/x86_64/kernel/suspend.c	2007-08-14 01:06:34.000000000 +0200
> > @@ -175,7 +175,7 @@ static int res_phys_pud_init(pud_t *pud,
> >  
> >  			if (paddr >= end)
> >  				break;
> > -			pe = _PAGE_NX | _PAGE_PSE | _KERNPG_TABLE | paddr;
> > +			pe = __PAGE_KERNEL_LARGE | paddr;
> >  			pe &= __supported_pte_mask;
> >  			set_pmd(pmd, __pmd(pe));
> >  		}
> > @@ -183,25 +183,42 @@ static int res_phys_pud_init(pud_t *pud,
> >  	return 0;
> >  }
> >  
> > +static int res_kernel_text_pud_init(pud_t *pud, unsigned long start)
> > +{
> > +	pmd_t *pmd;
> > +	unsigned long paddr;
> > +
> > +	pmd = (pmd_t *)get_safe_page(GFP_ATOMIC);
> > +	if (!pmd)
> > +		return -ENOMEM;
> > +	set_pud(pud + pud_index(start), __pud(__pa(pmd) | _KERNPG_TABLE));
> > +	for (paddr = 0; paddr < KERNEL_TEXT_SIZE; pmd++, paddr += PMD_SIZE) {
> 
> Should we have something like "< max possible kernel size here"?

AFAICS, KERNEL_TEXT_SIZE == "max possible kernel text size".  This even is
documented. :-)

Greetings,
Rafael

[RFC][PATCH -mm 4/4] Hibernation: Pass CR3 value in hibernation header on x86_64

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Since we already pass the address of restore_registers() in the image header,
we can also pass the value of the CR3 register from before the hibernation in
the same way.  This will allow us to avoid using init_level4_pgt page tables
during the restore.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 arch/x86_64/kernel/suspend.c     |   14 ++++++++++++--
 arch/x86_64/kernel/suspend_asm.S |    8 +++++---
 2 files changed, 17 insertions(+), 5 deletions(-)

Index: linux-2.6.23-rc3/arch/x86_64/kernel/suspend.c
===================================================================
--- linux-2.6.23-rc3.orig/arch/x86_64/kernel/suspend.c	2007-08-19 15:12:33.000000000 +0200
+++ linux-2.6.23-rc3/arch/x86_64/kernel/suspend.c	2007-08-19 15:28:24.000000000 +0200
@@ -150,6 +150,12 @@ extern int restore_image(void);
  */
 unsigned long restore_jump_address;
 
+/*
+ * Value of the cr3 register from before the hibernation (this value is passed
+ * in the image header).
+ */
+unsigned long restore_cr3;
+
 pgd_t *temp_level4_pgt;
 
 static int res_phys_pud_init(pud_t *pud, unsigned long address, unsigned long end)
@@ -266,6 +272,7 @@ int pfn_is_nosave(unsigned long pfn)
 
 struct restore_data_record {
 	unsigned long jump_address;
+	unsigned long cr3;
 	unsigned long control;
 };
 
@@ -283,7 +290,8 @@ int arch_hibernation_header_save(void *a
 	if (max_size < sizeof(struct restore_data_record))
 		return -EOVERFLOW;
 	rdr->jump_address = restore_jump_address;
-	rdr->control = (restore_jump_address ^ RESTORE_MAGIC);
+	rdr->cr3 = restore_cr3;
+	rdr->control = (restore_jump_address ^ (restore_cr3 ^ RESTORE_MAGIC));
 	return 0;
 }
 
@@ -297,7 +305,9 @@ int arch_hibernation_header_restore(void
 	struct restore_data_record *rdr = addr;
 
 	restore_jump_address = rdr->jump_address;
-	return (rdr->control == (restore_jump_address ^ RESTORE_MAGIC)) ?
+	restore_cr3 = rdr->cr3;
+	return (rdr->control ==
+		(restore_jump_address ^ (restore_cr3 ^ RESTORE_MAGIC))) ?
 			0 : -EINVAL;
 }
 #endif /* CONFIG_HIBERNATION */
Index: linux-2.6.23-rc3/arch/x86_64/kernel/suspend_asm.S
===================================================================
--- linux-2.6.23-rc3.orig/arch/x86_64/kernel/suspend_asm.S	2007-08-19 15:09:50.000000000 +0200
+++ linux-2.6.23-rc3/arch/x86_64/kernel/suspend_asm.S	2007-08-19 15:31:20.000000000 +0200
@@ -39,6 +39,9 @@ ENTRY(swsusp_arch_suspend)
 	/* save the address of restore_registers */
 	movq	$restore_registers, %rax
 	movq	%rax, restore_jump_address(%rip)
+	/* save cr3 */
+	movq	%cr3, %rax
+	movq	%rax, restore_cr3(%rip)
 
 	call swsusp_save
 	ret
@@ -66,6 +69,7 @@ ENTRY(restore_image)
 
 	/* prepare to jump to the image kernel */
 	movq	restore_jump_address(%rip), %rax
+	movq	restore_cr3(%rip), %rbx
 
 	/* copy image data to their original locations */
 	xorq	%r10, %r10
@@ -114,9 +118,7 @@ ENTRY(restore_registers)
 	movsq
 
 	/* go back to the original page tables */
-1:	movq    $(init_level4_pgt - __START_KERNEL_map), %rax
-	addq    phys_base(%rip), %rax
-	movq    %rax, %cr3
+1:	movq    %rbx, %cr3
 
 	/* Flush TLB, including "global" things (vmalloc) */
 	movq	mmu_cr4_features(%rip), %rax

Re: [RFC][PATCH -mm 4/4] Hibernation: Pass CR3 value in hibernation header on x86_64

[Pavel Machek]

Hi!

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Since we already pass the address of restore_registers() in the image header,
> we can also pass the value of the CR3 register from before the hibernation in
> the same way.  This will allow us to avoid using init_level4_pgt page tables
> during the restore.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

> @@ -283,7 +290,8 @@ int arch_hibernation_header_save(void *a
>  	if (max_size < sizeof(struct restore_data_record))
>  		return -EOVERFLOW;
>  	rdr->jump_address = restore_jump_address;
> -	rdr->control = (restore_jump_address ^ RESTORE_MAGIC);
> +	rdr->cr3 = restore_cr3;
> +	rdr->control = (restore_jump_address ^ (restore_cr3 ^ RESTORE_MAGIC));
>  	return 0;

Does control want to be renamed as "crc"? Or perphaps we want to just
store MAGIC there, w/o any obfuscation, since obfuscation does not
really buy us anything?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC][PATCH -mm 4/4] Hibernation: Pass CR3 value in hibernation header on x86_64

[Rafael J. Wysocki]

On Tuesday, 21 August 2007 10:01, Pavel Machek wrote:
> Hi!
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Since we already pass the address of restore_registers() in the image header,
> > we can also pass the value of the CR3 register from before the hibernation in
> > the same way.  This will allow us to avoid using init_level4_pgt page tables
> > during the restore.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
> > @@ -283,7 +290,8 @@ int arch_hibernation_header_save(void *a
> >  	if (max_size < sizeof(struct restore_data_record))
> >  		return -EOVERFLOW;
> >  	rdr->jump_address = restore_jump_address;
> > -	rdr->control = (restore_jump_address ^ RESTORE_MAGIC);
> > +	rdr->cr3 = restore_cr3;
> > +	rdr->control = (restore_jump_address ^ (restore_cr3 ^ RESTORE_MAGIC));
> >  	return 0;
> 
> Does control want to be renamed as "crc"?

I think we should use CRC here, but I need to figure out how to get the
dependencies right.

> Or perphaps we want to just store MAGIC there, w/o any obfuscation, since
> obfuscation does not really buy us anything?

Well, the idea is to avoid using corrupted cr3 value or the jump address ...

Greetings,
Rafael