swsusp_restore crap

swsusp_restore crap

[Benjamin Herrenschmidt]

Hi Pavel !

Please kill that swsusp_restore() call that itself calls
flush_tlb_global(), it's junk. First, the flush_tlb_global() thing is
arch specific, and that's all swsusp_restore() does. Then, the asm just
calls this before returning to C code, so it makes no sense to have a
hook there. The x86 asm can have it's own call to some arch stuff if it
wants or just do the tlb flush in asm...

Ben.

Re: swsusp_restore crap

[Benjamin Herrenschmidt]

On Tue, 2005-03-15 at 14:24 +1100, Benjamin Herrenschmidt wrote:
> Hi Pavel !
> 
> Please kill that swsusp_restore() call that itself calls
> flush_tlb_global(), it's junk. First, the flush_tlb_global() thing is
> arch specific, and that's all swsusp_restore() does. Then, the asm just
> calls this before returning to C code, so it makes no sense to have a
> hook there. The x86 asm can have it's own call to some arch stuff if it
> wants or just do the tlb flush in asm...

Better, here is a patch... (note: flush_tlb_global() is an x86'ism,
doesn't exist on ppc, thus breaks compile, and that has nothing to do in
the generic code imho, it should be clearly defined as the
responsibility of the asm code).

--

This patch removes the quite x86-specific swsusp_restore() hook from the
generic swsusp code and moves it to arch/i386. This also fixes build on
ppc with swsusp enabled.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Index: linux-work/arch/i386/power/swsusp.S
===================================================================
--- linux-work.orig/arch/i386/power/swsusp.S	2005-03-15 11:56:17.000000000 +1100
+++ linux-work/arch/i386/power/swsusp.S	2005-03-15 14:29:09.000000000 +1100
@@ -58,5 +58,5 @@
 	movl saved_context_edi, %edi
 
 	pushl saved_context_eflags ; popfl
-	call swsusp_restore
+	call __swsusp_flush_tlb
 	ret
Index: linux-work/arch/i386/power/cpu.c
===================================================================
--- linux-work.orig/arch/i386/power/cpu.c	2005-03-15 11:56:17.000000000 +1100
+++ linux-work/arch/i386/power/cpu.c	2005-03-15 14:28:26.000000000 +1100
@@ -147,6 +147,15 @@
 	__restore_processor_state(&saved_context);
 }
 
+asmlinkage int __swsusp_flush_tlb(void)
+{
+	BUG_ON (nr_copy_pages_check != nr_copy_pages);
+	
+	/* Even mappings of "global" things (vmalloc) need to be fixed */
+	__flush_tlb_global();
+	return 0;
+}
+
 /* Needed by apm.c */
 EXPORT_SYMBOL(save_processor_state);
 EXPORT_SYMBOL(restore_processor_state);
Index: linux-work/kernel/power/swsusp.c
===================================================================
--- linux-work.orig/kernel/power/swsusp.c	2005-03-15 12:00:13.000000000 +1100
+++ linux-work/kernel/power/swsusp.c	2005-03-15 14:29:19.000000000 +1100
@@ -907,15 +907,6 @@
 }
 
 
-asmlinkage int swsusp_restore(void)
-{
-	BUG_ON (nr_copy_pages_check != nr_copy_pages);
-	
-	/* Even mappings of "global" things (vmalloc) need to be fixed */
-	__flush_tlb_global();
-	return 0;
-}
-
 int swsusp_resume(void)
 {
 	int error;

Re: swsusp_restore crap

[Pavel Machek]

On Út 15-03-05 14:31:56, Benjamin Herrenschmidt wrote:
> On Tue, 2005-03-15 at 14:24 +1100, Benjamin Herrenschmidt wrote:
> > Hi Pavel !
> > 
> > Please kill that swsusp_restore() call that itself calls
> > flush_tlb_global(), it's junk. First, the flush_tlb_global() thing is
> > arch specific, and that's all swsusp_restore() does. Then, the asm just
> > calls this before returning to C code, so it makes no sense to have a
> > hook there. The x86 asm can have it's own call to some arch stuff if it
> > wants or just do the tlb flush in asm...
> 
> Better, here is a patch... (note: flush_tlb_global() is an x86'ism,
> doesn't exist on ppc, thus breaks compile, and that has nothing to do in
> the generic code imho, it should be clearly defined as the
> responsibility of the asm code).

x86-64 needs this, too.... Otherwise it looks okay.



> --
> 
> This patch removes the quite x86-specific swsusp_restore() hook from the
> generic swsusp code and moves it to arch/i386. This also fixes build on
> ppc with swsusp enabled.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> Index: linux-work/arch/i386/power/swsusp.S
> ===================================================================
> --- linux-work.orig/arch/i386/power/swsusp.S	2005-03-15 11:56:17.000000000 +1100
> +++ linux-work/arch/i386/power/swsusp.S	2005-03-15 14:29:09.000000000 +1100
> @@ -58,5 +58,5 @@
>  	movl saved_context_edi, %edi
>  
>  	pushl saved_context_eflags ; popfl
> -	call swsusp_restore
> +	call __swsusp_flush_tlb
>  	ret
> Index: linux-work/arch/i386/power/cpu.c
> ===================================================================
> --- linux-work.orig/arch/i386/power/cpu.c	2005-03-15 11:56:17.000000000 +1100
> +++ linux-work/arch/i386/power/cpu.c	2005-03-15 14:28:26.000000000 +1100
> @@ -147,6 +147,15 @@
>  	__restore_processor_state(&saved_context);
>  }
>  
> +asmlinkage int __swsusp_flush_tlb(void)
> +{
> +	BUG_ON (nr_copy_pages_check != nr_copy_pages);
> +	
> +	/* Even mappings of "global" things (vmalloc) need to be fixed */
> +	__flush_tlb_global();
> +	return 0;
> +}
> +
>  /* Needed by apm.c */
>  EXPORT_SYMBOL(save_processor_state);
>  EXPORT_SYMBOL(restore_processor_state);
> Index: linux-work/kernel/power/swsusp.c
> ===================================================================
> --- linux-work.orig/kernel/power/swsusp.c	2005-03-15 12:00:13.000000000 +1100
> +++ linux-work/kernel/power/swsusp.c	2005-03-15 14:29:19.000000000 +1100
> @@ -907,15 +907,6 @@
>  }
>  
>  
> -asmlinkage int swsusp_restore(void)
> -{
> -	BUG_ON (nr_copy_pages_check != nr_copy_pages);
> -	
> -	/* Even mappings of "global" things (vmalloc) need to be fixed */
> -	__flush_tlb_global();
> -	return 0;
> -}
> -
>  int swsusp_resume(void)
>  {
>  	int error;
> 

-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: swsusp_restore crap

[Rafael J. Wysocki]

Hi,

On Tuesday, 15 of March 2005 12:03, Pavel Machek wrote:
> On Út 15-03-05 14:31:56, Benjamin Herrenschmidt wrote:
> > On Tue, 2005-03-15 at 14:24 +1100, Benjamin Herrenschmidt wrote:
> > > Hi Pavel !
> > > 
> > > Please kill that swsusp_restore() call that itself calls
> > > flush_tlb_global(), it's junk. First, the flush_tlb_global() thing is
> > > arch specific, and that's all swsusp_restore() does. Then, the asm just
> > > calls this before returning to C code, so it makes no sense to have a
> > > hook there. The x86 asm can have it's own call to some arch stuff if it
> > > wants or just do the tlb flush in asm...
> > 
> > Better, here is a patch... (note: flush_tlb_global() is an x86'ism,
> > doesn't exist on ppc, thus breaks compile, and that has nothing to do in
> > the generic code imho, it should be clearly defined as the
> > responsibility of the asm code).
> 
> x86-64 needs this, too.... Otherwise it looks okay.

It breaks compilation on i386 either, because nr_copy_pages_check
is static in swsusp.c.  May I propose the following patch instead (tested on
x86-64 and i386)?

Greets,
Rafael

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

diff -Nrup linux-2.6.11-bk10-a/arch/i386/power/cpu.c linux-2.6.11-bk10-b/arch/i386/power/cpu.c
--- linux-2.6.11-bk10-a/arch/i386/power/cpu.c	2005-03-15 09:20:53.000000000 +0100
+++ linux-2.6.11-bk10-b/arch/i386/power/cpu.c	2005-03-15 12:16:57.000000000 +0100
@@ -147,6 +147,15 @@ void restore_processor_state(void)
 	__restore_processor_state(&saved_context);
 }
 
+asmlinkage int __swsusp_flush_tlb(void)
+{
+	swsusp_restore_check();
+
+	/* Even mappings of "global" things (vmalloc) need to be fixed */
+	__flush_tlb_global();
+	return 0;
+}
+
 /* Needed by apm.c */
 EXPORT_SYMBOL(save_processor_state);
 EXPORT_SYMBOL(restore_processor_state);
diff -Nrup linux-2.6.11-bk10-a/arch/i386/power/swsusp.S linux-2.6.11-bk10-b/arch/i386/power/swsusp.S
--- linux-2.6.11-bk10-a/arch/i386/power/swsusp.S	2005-03-15 09:20:53.000000000 +0100
+++ linux-2.6.11-bk10-b/arch/i386/power/swsusp.S	2005-03-15 12:16:28.000000000 +0100
@@ -58,5 +58,6 @@ done:
 	movl saved_context_edi, %edi
 
 	pushl saved_context_eflags ; popfl
-	call swsusp_restore
+
+	call	__swsusp_flush_tlb
 	ret
diff -Nrup linux-2.6.11-bk10-a/arch/x86_64/kernel/suspend_asm.S linux-2.6.11-bk10-b/arch/x86_64/kernel/suspend_asm.S
--- linux-2.6.11-bk10-a/arch/x86_64/kernel/suspend_asm.S	2005-03-15 09:20:53.000000000 +0100
+++ linux-2.6.11-bk10-b/arch/x86_64/kernel/suspend_asm.S	2005-03-15 12:14:47.000000000 +0100
@@ -89,5 +89,6 @@ done:
 	movq saved_context_r14(%rip), %r14
 	movq saved_context_r15(%rip), %r15
 	pushq saved_context_eflags(%rip) ; popfq
-	call	swsusp_restore
+
+	call	__swsusp_flush_tlb
 	ret
diff -Nrup linux-2.6.11-bk10-a/arch/x86_64/kernel/suspend.c linux-2.6.11-bk10-b/arch/x86_64/kernel/suspend.c
--- linux-2.6.11-bk10-a/arch/x86_64/kernel/suspend.c	2005-03-02 08:38:09.000000000 +0100
+++ linux-2.6.11-bk10-b/arch/x86_64/kernel/suspend.c	2005-03-15 12:15:25.000000000 +0100
@@ -154,4 +154,11 @@ void fix_processor_context(void)
 
 }
 
+int __swsusp_flush_tlb(void)
+{
+	swsusp_restore_check();
 
+	/* Even mappings of "global" things (vmalloc) need to be fixed */
+	__flush_tlb_global();
+	return 0;
+}
diff -Nrup linux-2.6.11-bk10-a/include/linux/suspend.h linux-2.6.11-bk10-b/include/linux/suspend.h
--- linux-2.6.11-bk10-a/include/linux/suspend.h	2005-03-15 09:21:23.000000000 +0100
+++ linux-2.6.11-bk10-b/include/linux/suspend.h	2005-03-15 12:20:06.000000000 +0100
@@ -68,6 +68,8 @@ static inline void disable_nonboot_cpus(
 static inline void enable_nonboot_cpus(void) {}
 #endif
 
+void swsusp_restore_check(void);
+
 void save_processor_state(void);
 void restore_processor_state(void);
 struct saved_context;
diff -Nrup linux-2.6.11-bk10-a/kernel/power/swsusp.c linux-2.6.11-bk10-b/kernel/power/swsusp.c
--- linux-2.6.11-bk10-a/kernel/power/swsusp.c	2005-03-15 09:21:23.000000000 +0100
+++ linux-2.6.11-bk10-b/kernel/power/swsusp.c	2005-03-15 12:18:36.000000000 +0100
@@ -906,14 +906,9 @@ int swsusp_suspend(void)
 	return error;
 }
 
-
-asmlinkage int swsusp_restore(void)
+void swsusp_restore_check(void)
 {
 	BUG_ON (nr_copy_pages_check != nr_copy_pages);
-	
-	/* Even mappings of "global" things (vmalloc) need to be fixed */
-	__flush_tlb_global();
-	return 0;
 }
 
 int swsusp_resume(void)

-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: swsusp_restore crap

[Pavel Machek]

Hi!

> > > > Please kill that swsusp_restore() call that itself calls
> > > > flush_tlb_global(), it's junk. First, the flush_tlb_global() thing is
> > > > arch specific, and that's all swsusp_restore() does. Then, the asm just
> > > > calls this before returning to C code, so it makes no sense to have a
> > > > hook there. The x86 asm can have it's own call to some arch stuff if it
> > > > wants or just do the tlb flush in asm...
> > > 
> > > Better, here is a patch... (note: flush_tlb_global() is an x86'ism,
> > > doesn't exist on ppc, thus breaks compile, and that has nothing to do in
> > > the generic code imho, it should be clearly defined as the
> > > responsibility of the asm code).
> > 
> > x86-64 needs this, too.... Otherwise it looks okay.
> 
> It breaks compilation on i386 either, because nr_copy_pages_check
> is static in swsusp.c.  May I propose the following patch instead (tested on
> x86-64 and i386)?


> +asmlinkage int __swsusp_flush_tlb(void)
> +{
> +	swsusp_restore_check();

Someone will certainly forget this one, and it is probably
nicer/easier to just move BUG_ON into swsusp_suspend(), just after
restore_processor_state() or something like that...
								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: swsusp_restore crap

[Benjamin Herrenschmidt]

> 
> > +asmlinkage int __swsusp_flush_tlb(void)
> > +{
> > +	swsusp_restore_check();
> 
> Someone will certainly forget this one, and it is probably
> nicer/easier to just move BUG_ON into swsusp_suspend(), just after
> restore_processor_state() or something like that...

Agreed.

Ben.

Re: swsusp_restore crap

[Rafael J. Wysocki]

Hi,

On Tuesday, 15 of March 2005 13:02, Pavel Machek wrote:
> Hi!
> 
> > > > > Please kill that swsusp_restore() call that itself calls
> > > > > flush_tlb_global(), it's junk. First, the flush_tlb_global() thing is
> > > > > arch specific, and that's all swsusp_restore() does. Then, the asm just
> > > > > calls this before returning to C code, so it makes no sense to have a
> > > > > hook there. The x86 asm can have it's own call to some arch stuff if it
> > > > > wants or just do the tlb flush in asm...
> > > > 
> > > > Better, here is a patch... (note: flush_tlb_global() is an x86'ism,
> > > > doesn't exist on ppc, thus breaks compile, and that has nothing to do in
> > > > the generic code imho, it should be clearly defined as the
> > > > responsibility of the asm code).
> > > 
> > > x86-64 needs this, too.... Otherwise it looks okay.
> > 
> > It breaks compilation on i386 either, because nr_copy_pages_check
> > is static in swsusp.c.  May I propose the following patch instead (tested on
> > x86-64 and i386)?
> 
> 
> > +asmlinkage int __swsusp_flush_tlb(void)
> > +{
> > +	swsusp_restore_check();
> 
> Someone will certainly forget this one, and it is probably
> nicer/easier to just move BUG_ON into swsusp_suspend(), just after
> restore_processor_state() or something like that...

... in which case __swsusp_flush_tlb() would only contain a "call" to
__flush_tlb_global(), but this is a macro on both x86-64 and i386, so we can
drop the __swsusp_flush_tlb() altogether and do it in assembly (before the
GPRs are restored, perhaps).  Patch follows.

Greets,
Rafael


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

diff -Nrup linux-2.6.11-bk10-a/arch/i386/power/swsusp.S linux-2.6.11-bk10-b/arch/i386/power/swsusp.S
--- linux-2.6.11-bk10-a/arch/i386/power/swsusp.S	2005-03-15 09:20:53.000000000 +0100
+++ linux-2.6.11-bk10-b/arch/i386/power/swsusp.S	2005-03-15 15:37:25.000000000 +0100
@@ -51,6 +51,15 @@ copy_loop:
 	.p2align 4,,7
 
 done:
+	/* Flush TLB, including "global" things (vmalloc) */
+	movl	mmu_cr4_features, %eax
+	movl	%eax, %edx
+	andl	$~(1<<7), %edx;  # PGE
+	movl	%edx, %cr4;  # turn off PGE
+	movl	%cr3, %ecx;  # flush TLB
+	movl	%ecx, %cr3
+	movl	%eax, %cr4;  # turn PGE back on
+
 	movl saved_context_esp, %esp
 	movl saved_context_ebp, %ebp
 	movl saved_context_ebx, %ebx
@@ -58,5 +67,5 @@ done:
 	movl saved_context_edi, %edi
 
 	pushl saved_context_eflags ; popfl
-	call swsusp_restore
+
 	ret
diff -Nrup linux-2.6.11-bk10-a/arch/x86_64/kernel/suspend_asm.S linux-2.6.11-bk10-b/arch/x86_64/kernel/suspend_asm.S
--- linux-2.6.11-bk10-a/arch/x86_64/kernel/suspend_asm.S	2005-03-15 09:20:53.000000000 +0100
+++ linux-2.6.11-bk10-b/arch/x86_64/kernel/suspend_asm.S	2005-03-15 15:36:29.000000000 +0100
@@ -69,6 +69,14 @@ loop:
 	movq	pbe_next(%rdx), %rdx
 	jmp	loop
 done:
+	/* Flush TLB, including "global" things (vmalloc) */
+	movq	%rax, %rdx;  # mmu_cr4_features(%rip)
+	andq	$~(1<<7), %rdx;  # PGE
+	movq	%rdx, %cr4;  # turn off PGE
+	movq	%cr3, %rcx;  # flush TLB
+	movq	%rcx, %cr3
+	movq	%rax, %cr4;  # turn PGE back on
+
 	movl	$24, %eax
 	movl	%eax, %ds
 
@@ -89,5 +97,5 @@ done:
 	movq saved_context_r14(%rip), %r14
 	movq saved_context_r15(%rip), %r15
 	pushq saved_context_eflags(%rip) ; popfq
-	call	swsusp_restore
+
 	ret
diff -Nrup linux-2.6.11-bk10-a/kernel/power/swsusp.c linux-2.6.11-bk10-b/kernel/power/swsusp.c
--- linux-2.6.11-bk10-a/kernel/power/swsusp.c	2005-03-15 09:21:23.000000000 +0100
+++ linux-2.6.11-bk10-b/kernel/power/swsusp.c	2005-03-15 15:35:44.000000000 +0100
@@ -900,22 +900,13 @@ int swsusp_suspend(void)
 	error = swsusp_arch_suspend();
 	/* Restore control flow magically appears here */
 	restore_processor_state();
+	BUG_ON (nr_copy_pages_check != nr_copy_pages);
 	restore_highmem();
 	device_power_up();
 	local_irq_enable();
 	return error;
 }
 
-
-asmlinkage int swsusp_restore(void)
-{
-	BUG_ON (nr_copy_pages_check != nr_copy_pages);
-	
-	/* Even mappings of "global" things (vmalloc) need to be fixed */
-	__flush_tlb_global();
-	return 0;
-}
-
 int swsusp_resume(void)
 {
 	int error;

-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: swsusp_restore crap

[Pavel Machek]

Hi!

> > > > x86-64 needs this, too.... Otherwise it looks okay.
> > > 
> > > It breaks compilation on i386 either, because nr_copy_pages_check
> > > is static in swsusp.c.  May I propose the following patch instead (tested on
> > > x86-64 and i386)?
> > 
> > 
> > > +asmlinkage int __swsusp_flush_tlb(void)
> > > +{
> > > +	swsusp_restore_check();
> > 
> > Someone will certainly forget this one, and it is probably
> > nicer/easier to just move BUG_ON into swsusp_suspend(), just after
> > restore_processor_state() or something like that...
> 
> ... in which case __swsusp_flush_tlb() would only contain a "call" to
> __flush_tlb_global(), but this is a macro on both x86-64 and i386, so we can
> drop the __swsusp_flush_tlb() altogether and do it in assembly (before the
> GPRs are restored, perhaps).  Patch follows.
> 
> Greets,
> Rafael
> 
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

> diff -Nrup linux-2.6.11-bk10-a/arch/x86_64/kernel/suspend_asm.S linux-2.6.11-bk10-b/arch/x86_64/kernel/suspend_asm.S
> --- linux-2.6.11-bk10-a/arch/x86_64/kernel/suspend_asm.S	2005-03-15 09:20:53.000000000 +0100
> +++ linux-2.6.11-bk10-b/arch/x86_64/kernel/suspend_asm.S	2005-03-15 15:36:29.000000000 +0100
> @@ -69,6 +69,14 @@ loop:
>  	movq	pbe_next(%rdx), %rdx
>  	jmp	loop
>  done:
> +	/* Flush TLB, including "global" things (vmalloc) */
> +	movq	%rax, %rdx;  # mmu_cr4_features(%rip)

I somehow don't think %rax contains mmu_cr4_features at this
point. Otherwise it seems to look ok.
								Pavel

-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: swsusp_restore crap

[Rafael J. Wysocki]

On Tuesday, 15 of March 2005 21:46, Pavel Machek wrote:
> Hi!
> 
> > > > > x86-64 needs this, too.... Otherwise it looks okay.
> > > > 
> > > > It breaks compilation on i386 either, because nr_copy_pages_check
> > > > is static in swsusp.c.  May I propose the following patch instead (tested on
> > > > x86-64 and i386)?
> > > 
> > > 
> > > > +asmlinkage int __swsusp_flush_tlb(void)
> > > > +{
> > > > +	swsusp_restore_check();
> > > 
> > > Someone will certainly forget this one, and it is probably
> > > nicer/easier to just move BUG_ON into swsusp_suspend(), just after
> > > restore_processor_state() or something like that...
> > 
> > ... in which case __swsusp_flush_tlb() would only contain a "call" to
> > __flush_tlb_global(), but this is a macro on both x86-64 and i386, so we can
> > drop the __swsusp_flush_tlb() altogether and do it in assembly (before the
> > GPRs are restored, perhaps).  Patch follows.
> > 
> > Greets,
> > Rafael
> > 
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
> > diff -Nrup linux-2.6.11-bk10-a/arch/x86_64/kernel/suspend_asm.S linux-2.6.11-bk10-b/arch/x86_64/kernel/suspend_asm.S
> > --- linux-2.6.11-bk10-a/arch/x86_64/kernel/suspend_asm.S	2005-03-15 09:20:53.000000000 +0100
> > +++ linux-2.6.11-bk10-b/arch/x86_64/kernel/suspend_asm.S	2005-03-15 15:36:29.000000000 +0100
> > @@ -69,6 +69,14 @@ loop:
> >  	movq	pbe_next(%rdx), %rdx
> >  	jmp	loop
> >  done:
> > +	/* Flush TLB, including "global" things (vmalloc) */
> > +	movq	%rax, %rdx;  # mmu_cr4_features(%rip)
> 
> I somehow don't think %rax contains mmu_cr4_features at this
> point. Otherwise it seems to look ok.

Yes, it does, because on x86-64 the TLBs are flushed before the loop,
right after %cr3 is loaded with init_level4_pgt.  %rax is not touched
afterwards, so it contains the right value.  Here's the relevant code
from suspend_asm.S (with the patch applied):

ENTRY(swsusp_arch_resume)
	/* set up cr3 */	
	leaq	init_level4_pgt(%rip),%rax
	subq	$__START_KERNEL_map,%rax
	movq	%rax,%cr3

	movq	mmu_cr4_features(%rip), %rax
	movq	%rax, %rdx
	andq	$~(1<<7), %rdx	# PGE
	movq	%rdx, %cr4;  # turn off PGE
	movq	%cr3, %rcx;  # flush TLB
	movq	%rcx, %cr3;
	movq	%rax, %cr4;  # turn PGE back on

	movq	pagedir_nosave(%rip), %rdx
loop:
	testq	%rdx, %rdx
	jz	done

	/* get addresses from the pbe and copy the page */
	movq	pbe_address(%rdx), %rsi
	movq	pbe_orig_address(%rdx), %rdi
	movq	$512, %rcx
	rep
	movsq

	/* progress to the next pbe */
	movq	pbe_next(%rdx), %rdx
	jmp	loop
done:
	/* Flush TLB, including "global" things (vmalloc) */
	movq	%rax, %rdx;  # mmu_cr4_features(%rip)
	andq	$~(1<<7), %rdx;  # PGE
	movq	%rdx, %cr4;  # turn off PGE
	movq	%cr3, %rcx;  # flush TLB
	movq	%rcx, %cr3
	movq	%rax, %cr4;  # turn PGE back on


Greets,
Rafael


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: swsusp_restore crap

[Pavel Machek]

Hi!

> > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > > diff -Nrup linux-2.6.11-bk10-a/arch/x86_64/kernel/suspend_asm.S linux-2.6.11-bk10-b/arch/x86_64/kernel/suspend_asm.S
> > > --- linux-2.6.11-bk10-a/arch/x86_64/kernel/suspend_asm.S	2005-03-15 09:20:53.000000000 +0100
> > > +++ linux-2.6.11-bk10-b/arch/x86_64/kernel/suspend_asm.S	2005-03-15 15:36:29.000000000 +0100
> > > @@ -69,6 +69,14 @@ loop:
> > >  	movq	pbe_next(%rdx), %rdx
> > >  	jmp	loop
> > >  done:
> > > +	/* Flush TLB, including "global" things (vmalloc) */
> > > +	movq	%rax, %rdx;  # mmu_cr4_features(%rip)
> > 
> > I somehow don't think %rax contains mmu_cr4_features at this
> > point. Otherwise it seems to look ok.
> 
> Yes, it does, because on x86-64 the TLBs are flushed before the loop,
> right after %cr3 is loaded with init_level4_pgt.  %rax is not touched
> afterwards, so it contains the right value.  Here's the relevant code
> from suspend_asm.S (with the patch applied):

Well, it is mmu_cr4_features from "old" kernel, while you are flushing
tlb in "new" kernel. It is probably same anyway, but.... %rax is
commonly-used scratch register, and memory load is not that
expensive. Can you just load it from memory?
								Pavel

> ENTRY(swsusp_arch_resume)
> 	/* set up cr3 */	
> 	leaq	init_level4_pgt(%rip),%rax
> 	subq	$__START_KERNEL_map,%rax
> 	movq	%rax,%cr3
> 
> 	movq	mmu_cr4_features(%rip), %rax
> 	movq	%rax, %rdx
> 	andq	$~(1<<7), %rdx	# PGE
> 	movq	%rdx, %cr4;  # turn off PGE
> 	movq	%cr3, %rcx;  # flush TLB
> 	movq	%rcx, %cr3;
> 	movq	%rax, %cr4;  # turn PGE back on
> 
> 	movq	pagedir_nosave(%rip), %rdx
> loop:
> 	testq	%rdx, %rdx
> 	jz	done
> 
> 	/* get addresses from the pbe and copy the page */
> 	movq	pbe_address(%rdx), %rsi
> 	movq	pbe_orig_address(%rdx), %rdi
> 	movq	$512, %rcx
> 	rep
> 	movsq
> 
> 	/* progress to the next pbe */
> 	movq	pbe_next(%rdx), %rdx
> 	jmp	loop
> done:
> 	/* Flush TLB, including "global" things (vmalloc) */
> 	movq	%rax, %rdx;  # mmu_cr4_features(%rip)
> 	andq	$~(1<<7), %rdx;  # PGE
> 	movq	%rdx, %cr4;  # turn off PGE
> 	movq	%cr3, %rcx;  # flush TLB
> 	movq	%rcx, %cr3
> 	movq	%rax, %cr4;  # turn PGE back on
> 
> 
> Greets,
> Rafael
> 
> 

-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: swsusp_restore crap

[Rafael J. Wysocki]

Hi,

On Wednesday, 16 of March 2005 00:39, Pavel Machek wrote:
> Hi!
> 
> > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > 
> > > > diff -Nrup linux-2.6.11-bk10-a/arch/x86_64/kernel/suspend_asm.S linux-2.6.11-bk10-b/arch/x86_64/kernel/suspend_asm.S
> > > > --- linux-2.6.11-bk10-a/arch/x86_64/kernel/suspend_asm.S	2005-03-15 09:20:53.000000000 +0100
> > > > +++ linux-2.6.11-bk10-b/arch/x86_64/kernel/suspend_asm.S	2005-03-15 15:36:29.000000000 +0100
> > > > @@ -69,6 +69,14 @@ loop:
> > > >  	movq	pbe_next(%rdx), %rdx
> > > >  	jmp	loop
> > > >  done:
> > > > +	/* Flush TLB, including "global" things (vmalloc) */
> > > > +	movq	%rax, %rdx;  # mmu_cr4_features(%rip)
> > > 
> > > I somehow don't think %rax contains mmu_cr4_features at this
> > > point. Otherwise it seems to look ok.
> > 
> > Yes, it does, because on x86-64 the TLBs are flushed before the loop,
> > right after %cr3 is loaded with init_level4_pgt.  %rax is not touched
> > afterwards, so it contains the right value.  Here's the relevant code
> > from suspend_asm.S (with the patch applied):
> 
> Well, it is mmu_cr4_features from "old" kernel, while you are flushing
> tlb in "new" kernel. It is probably same anyway, but.... %rax is
> commonly-used scratch register, and memory load is not that
> expensive. Can you just load it from memory?

Sure, revised patch follows.

Greets,
Rafael


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

diff -Nrup linux-2.6.11-bk10-a/arch/i386/power/swsusp.S linux-2.6.11-bk10-b/arch/i386/power/swsusp.S
--- linux-2.6.11-bk10-a/arch/i386/power/swsusp.S	2005-03-15 09:20:53.000000000 +0100
+++ linux-2.6.11-bk10-b/arch/i386/power/swsusp.S	2005-03-15 15:37:25.000000000 +0100
@@ -51,6 +51,15 @@ copy_loop:
 	.p2align 4,,7
 
 done:
+	/* Flush TLB, including "global" things (vmalloc) */
+	movl	mmu_cr4_features, %eax
+	movl	%eax, %edx
+	andl	$~(1<<7), %edx;  # PGE
+	movl	%edx, %cr4;  # turn off PGE
+	movl	%cr3, %ecx;  # flush TLB
+	movl	%ecx, %cr3
+	movl	%eax, %cr4;  # turn PGE back on
+
 	movl saved_context_esp, %esp
 	movl saved_context_ebp, %ebp
 	movl saved_context_ebx, %ebx
@@ -58,5 +67,5 @@ done:
 	movl saved_context_edi, %edi
 
 	pushl saved_context_eflags ; popfl
-	call swsusp_restore
+
 	ret
diff -Nrup linux-2.6.11-bk10-a/arch/x86_64/kernel/suspend_asm.S linux-2.6.11-bk10-b/arch/x86_64/kernel/suspend_asm.S
--- linux-2.6.11-bk10-a/arch/x86_64/kernel/suspend_asm.S	2005-03-15 09:20:53.000000000 +0100
+++ linux-2.6.11-bk10-b/arch/x86_64/kernel/suspend_asm.S	2005-03-16 00:56:53.000000000 +0100
@@ -69,6 +69,15 @@ loop:
 	movq	pbe_next(%rdx), %rdx
 	jmp	loop
 done:
+	/* Flush TLB, including "global" things (vmalloc) */
+	movq	mmu_cr4_features(%rip), %rax
+	movq	%rax, %rdx
+	andq	$~(1<<7), %rdx;  # PGE
+	movq	%rdx, %cr4;  # turn off PGE
+	movq	%cr3, %rcx;  # flush TLB
+	movq	%rcx, %cr3
+	movq	%rax, %cr4;  # turn PGE back on
+
 	movl	$24, %eax
 	movl	%eax, %ds
 
@@ -89,5 +98,5 @@ done:
 	movq saved_context_r14(%rip), %r14
 	movq saved_context_r15(%rip), %r15
 	pushq saved_context_eflags(%rip) ; popfq
-	call	swsusp_restore
+
 	ret
diff -Nrup linux-2.6.11-bk10-a/kernel/power/swsusp.c linux-2.6.11-bk10-b/kernel/power/swsusp.c
--- linux-2.6.11-bk10-a/kernel/power/swsusp.c	2005-03-15 09:21:23.000000000 +0100
+++ linux-2.6.11-bk10-b/kernel/power/swsusp.c	2005-03-15 15:35:44.000000000 +0100
@@ -900,22 +900,13 @@ int swsusp_suspend(void)
 	error = swsusp_arch_suspend();
 	/* Restore control flow magically appears here */
 	restore_processor_state();
+	BUG_ON (nr_copy_pages_check != nr_copy_pages);
 	restore_highmem();
 	device_power_up();
 	local_irq_enable();
 	return error;
 }
 
-
-asmlinkage int swsusp_restore(void)
-{
-	BUG_ON (nr_copy_pages_check != nr_copy_pages);
-	
-	/* Even mappings of "global" things (vmalloc) need to be fixed */
-	__flush_tlb_global();
-	return 0;
-}
-
 int swsusp_resume(void)
 {
 	int error;

-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: swsusp_restore crap

[Benjamin Herrenschmidt]

> It breaks compilation on i386 either, because nr_copy_pages_check
> is static in swsusp.c.  May I propose the following patch instead (tested on
> x86-64 and i386)?
> 
> Greets,
> Rafael
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
> diff -Nrup linux-2.6.11-bk10-a/arch/i386/power/cpu.c linux-2.6.11-bk10-b/arch/i386/power/cpu.c
> --- linux-2.6.11-bk10-a/arch/i386/power/cpu.c	2005-03-15 09:20:53.000000000 +0100
> +++ linux-2.6.11-bk10-b/arch/i386/power/cpu.c	2005-03-15 12:16:57.000000000 +0100
> @@ -147,6 +147,15 @@ void restore_processor_state(void)
>  	__restore_processor_state(&saved_context);
>  }
>  
> +asmlinkage int __swsusp_flush_tlb(void)
> +{
> +	swsusp_restore_check();
> +

Do we really need that check there ? Can't it be moved elsewhere ?

Ben.