Patchwork [1/1] x86-32, resume: do a global tlb flush in S4 resume

login
register
mail settings
Submitter Andy Whitcroft
Date March 30, 2010, 9:05 p.m.
Message ID <1269983121-4495-2-git-send-email-apw@canonical.com>
Download mbox | patch
Permalink /patch/49055/
State Accepted
Delegated to: Andy Whitcroft
Headers show

Comments

Andy Whitcroft - March 30, 2010, 9:05 p.m.
From: Shaohua Li <shaohua.li@intel.com>

Colin King reported a strange oops in S4 resume code path (see below). The test
system has i5/i7 CPU. The kernel doesn't open PAE, so 4M page table is used.
The oops always happen a virtual address 0xc03ff000, which is mapped to the
last 4k of first 4M memory. Doing a global tlb flush fixes the issue.

EIP: 0060:[<c0493a01>] EFLAGS: 00010086 CPU: 0
EIP is at copy_loop+0xe/0x15
EAX: 36aeb000 EBX: 00000000 ECX: 00000400 EDX: f55ad46c
ESI: 0f800000 EDI: c03ff000 EBP: f67fbec4 ESP: f67fbea8
DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
...
...
CR2: 00000000c03ff000

Tested-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: Shaohua Li <shaohua.li@intel.com>
LKML-Reference: <20100305005932.GA22675@sli10-desk.sh.intel.com>
Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
Cc: <stable@kernel.org>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 arch/x86/power/hibernate_asm_32.S |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)
Andy Whitcroft - March 31, 2010, 7:27 a.m.
On Tue, Mar 30, 2010 at 10:05:21PM +0100, Andy Whitcroft wrote:
> From: Shaohua Li <shaohua.li@intel.com>
> 
> Colin King reported a strange oops in S4 resume code path (see below). The test
> system has i5/i7 CPU. The kernel doesn't open PAE, so 4M page table is used.
> The oops always happen a virtual address 0xc03ff000, which is mapped to the
> last 4k of first 4M memory. Doing a global tlb flush fixes the issue.
> 
> EIP: 0060:[<c0493a01>] EFLAGS: 00010086 CPU: 0
> EIP is at copy_loop+0xe/0x15
> EAX: 36aeb000 EBX: 00000000 ECX: 00000400 EDX: f55ad46c
> ESI: 0f800000 EDI: c03ff000 EBP: f67fbec4 ESP: f67fbea8
> DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
> ...
> ...
> CR2: 00000000c03ff000
> 
> Tested-by: Colin Ian King <colin.king@canonical.com>
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> LKML-Reference: <20100305005932.GA22675@sli10-desk.sh.intel.com>
> Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
> Signed-off-by: H. Peter Anvin <hpa@zytor.com>
> Cc: <stable@kernel.org>
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
> ---
>  arch/x86/power/hibernate_asm_32.S |   15 +++++++--------
>  1 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/power/hibernate_asm_32.S b/arch/x86/power/hibernate_asm_32.S
> index b641388..ad47dae 100644
> --- a/arch/x86/power/hibernate_asm_32.S
> +++ b/arch/x86/power/hibernate_asm_32.S
> @@ -27,10 +27,17 @@ ENTRY(swsusp_arch_suspend)
>  	ret
>  
>  ENTRY(restore_image)
> +	movl	mmu_cr4_features, %ecx
>  	movl	resume_pg_dir, %eax
>  	subl	$__PAGE_OFFSET, %eax
>  	movl	%eax, %cr3
>  
> +	jecxz	1f	# cr4 Pentium and higher, skip if zero
> +	andl	$~(X86_CR4_PGE), %ecx
> +	movl	%ecx, %cr4;  # turn off PGE
> +	movl	%cr3, %eax;  # flush TLB
> +	movl	%eax, %cr3
> +1:
>  	movl	restore_pblist, %edx
>  	.p2align 4,,7
>  
> @@ -54,16 +61,8 @@ done:
>  	movl	$swapper_pg_dir, %eax
>  	subl	$__PAGE_OFFSET, %eax
>  	movl	%eax, %cr3
> -	/* Flush TLB, including "global" things (vmalloc) */
>  	movl	mmu_cr4_features, %ecx
>  	jecxz	1f	# cr4 Pentium and higher, skip if zero
> -	movl	%ecx, %edx
> -	andl	$~(X86_CR4_PGE), %edx
> -	movl	%edx, %cr4;  # turn off PGE
> -1:
> -	movl	%cr3, %eax;  # flush TLB
> -	movl	%eax, %cr3
> -	jecxz	1f	# cr4 Pentium and higher, skip if zero
>  	movl	%ecx, %cr4;  # turn PGE back on
>  1:

The description of this is terrible, actually it appears to move the
flush earlier in the resume process.  On the assumption its been tested
on non-i5/i7 kit and not shown any regression, and that its now going
through stable:

Acked-by: Andy Whitcroft <apw@canonical.com>

-apw
Stefan Bader - March 31, 2010, 9:59 a.m.
Looks to me too like just moving the TLB flush a bit earlier. And given the fact
that Colin has spent quite some time testing and I saw this coming in on the
urgent queue with stable target, I think this is good.

Andy Whitcroft wrote:
> From: Shaohua Li <shaohua.li@intel.com>
> 
> Colin King reported a strange oops in S4 resume code path (see below). The test
> system has i5/i7 CPU. The kernel doesn't open PAE, so 4M page table is used.
> The oops always happen a virtual address 0xc03ff000, which is mapped to the
> last 4k of first 4M memory. Doing a global tlb flush fixes the issue.
> 
> EIP: 0060:[<c0493a01>] EFLAGS: 00010086 CPU: 0
> EIP is at copy_loop+0xe/0x15
> EAX: 36aeb000 EBX: 00000000 ECX: 00000400 EDX: f55ad46c
> ESI: 0f800000 EDI: c03ff000 EBP: f67fbec4 ESP: f67fbea8
> DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
> ...
> ...
> CR2: 00000000c03ff000
> 
> Tested-by: Colin Ian King <colin.king@canonical.com>
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> LKML-Reference: <20100305005932.GA22675@sli10-desk.sh.intel.com>
> Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
> Signed-off-by: H. Peter Anvin <hpa@zytor.com>
> Cc: <stable@kernel.org>
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  arch/x86/power/hibernate_asm_32.S |   15 +++++++--------
>  1 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/power/hibernate_asm_32.S b/arch/x86/power/hibernate_asm_32.S
> index b641388..ad47dae 100644
> --- a/arch/x86/power/hibernate_asm_32.S
> +++ b/arch/x86/power/hibernate_asm_32.S
> @@ -27,10 +27,17 @@ ENTRY(swsusp_arch_suspend)
>  	ret
>  
>  ENTRY(restore_image)
> +	movl	mmu_cr4_features, %ecx
>  	movl	resume_pg_dir, %eax
>  	subl	$__PAGE_OFFSET, %eax
>  	movl	%eax, %cr3
>  
> +	jecxz	1f	# cr4 Pentium and higher, skip if zero
> +	andl	$~(X86_CR4_PGE), %ecx
> +	movl	%ecx, %cr4;  # turn off PGE
> +	movl	%cr3, %eax;  # flush TLB
> +	movl	%eax, %cr3
> +1:
>  	movl	restore_pblist, %edx
>  	.p2align 4,,7
>  
> @@ -54,16 +61,8 @@ done:
>  	movl	$swapper_pg_dir, %eax
>  	subl	$__PAGE_OFFSET, %eax
>  	movl	%eax, %cr3
> -	/* Flush TLB, including "global" things (vmalloc) */
>  	movl	mmu_cr4_features, %ecx
>  	jecxz	1f	# cr4 Pentium and higher, skip if zero
> -	movl	%ecx, %edx
> -	andl	$~(X86_CR4_PGE), %edx
> -	movl	%edx, %cr4;  # turn off PGE
> -1:
> -	movl	%cr3, %eax;  # flush TLB
> -	movl	%eax, %cr3
> -	jecxz	1f	# cr4 Pentium and higher, skip if zero
>  	movl	%ecx, %cr4;  # turn PGE back on
>  1:
>
Colin King - April 6, 2010, 1:15 p.m.
I also think this should go into Karmic as a SRU. Shall I open up a bug
against karmic and formally push it through the SRU process?

Colin

On Tue, 2010-03-30 at 22:05 +0100, Andy Whitcroft wrote:
> From: Shaohua Li <shaohua.li@intel.com>
> 
> Colin King reported a strange oops in S4 resume code path (see below). The test
> system has i5/i7 CPU. The kernel doesn't open PAE, so 4M page table is used.
> The oops always happen a virtual address 0xc03ff000, which is mapped to the
> last 4k of first 4M memory. Doing a global tlb flush fixes the issue.
> 
> EIP: 0060:[<c0493a01>] EFLAGS: 00010086 CPU: 0
> EIP is at copy_loop+0xe/0x15
> EAX: 36aeb000 EBX: 00000000 ECX: 00000400 EDX: f55ad46c
> ESI: 0f800000 EDI: c03ff000 EBP: f67fbec4 ESP: f67fbea8
> DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
> ...
> ...
> CR2: 00000000c03ff000
> 
> Tested-by: Colin Ian King <colin.king@canonical.com>
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> LKML-Reference: <20100305005932.GA22675@sli10-desk.sh.intel.com>
> Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
> Signed-off-by: H. Peter Anvin <hpa@zytor.com>
> Cc: <stable@kernel.org>
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
> ---
>  arch/x86/power/hibernate_asm_32.S |   15 +++++++--------
>  1 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/power/hibernate_asm_32.S b/arch/x86/power/hibernate_asm_32.S
> index b641388..ad47dae 100644
> --- a/arch/x86/power/hibernate_asm_32.S
> +++ b/arch/x86/power/hibernate_asm_32.S
> @@ -27,10 +27,17 @@ ENTRY(swsusp_arch_suspend)
>  	ret
>  
>  ENTRY(restore_image)
> +	movl	mmu_cr4_features, %ecx
>  	movl	resume_pg_dir, %eax
>  	subl	$__PAGE_OFFSET, %eax
>  	movl	%eax, %cr3
>  
> +	jecxz	1f	# cr4 Pentium and higher, skip if zero
> +	andl	$~(X86_CR4_PGE), %ecx
> +	movl	%ecx, %cr4;  # turn off PGE
> +	movl	%cr3, %eax;  # flush TLB
> +	movl	%eax, %cr3
> +1:
>  	movl	restore_pblist, %edx
>  	.p2align 4,,7
>  
> @@ -54,16 +61,8 @@ done:
>  	movl	$swapper_pg_dir, %eax
>  	subl	$__PAGE_OFFSET, %eax
>  	movl	%eax, %cr3
> -	/* Flush TLB, including "global" things (vmalloc) */
>  	movl	mmu_cr4_features, %ecx
>  	jecxz	1f	# cr4 Pentium and higher, skip if zero
> -	movl	%ecx, %edx
> -	andl	$~(X86_CR4_PGE), %edx
> -	movl	%edx, %cr4;  # turn off PGE
> -1:
> -	movl	%cr3, %eax;  # flush TLB
> -	movl	%eax, %cr3
> -	jecxz	1f	# cr4 Pentium and higher, skip if zero
>  	movl	%ecx, %cr4;  # turn PGE back on
>  1:
>  
> -- 
> 1.7.0
> 
>
Stefan Bader - April 6, 2010, 2:28 p.m.
Colin Ian King wrote:
> I also think this should go into Karmic as a SRU. Shall I open up a bug
> against karmic and formally push it through the SRU process?
> 
Same bug is ok, just nominate for karmic. But sending the patch a second time
around for karmic helps to get it on patchworks for me.

Stefan

> Colin
> 
> On Tue, 2010-03-30 at 22:05 +0100, Andy Whitcroft wrote:
>> From: Shaohua Li <shaohua.li@intel.com>
>>
>> Colin King reported a strange oops in S4 resume code path (see below). The test
>> system has i5/i7 CPU. The kernel doesn't open PAE, so 4M page table is used.
>> The oops always happen a virtual address 0xc03ff000, which is mapped to the
>> last 4k of first 4M memory. Doing a global tlb flush fixes the issue.
>>
>> EIP: 0060:[<c0493a01>] EFLAGS: 00010086 CPU: 0
>> EIP is at copy_loop+0xe/0x15
>> EAX: 36aeb000 EBX: 00000000 ECX: 00000400 EDX: f55ad46c
>> ESI: 0f800000 EDI: c03ff000 EBP: f67fbec4 ESP: f67fbea8
>> DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
>> ...
>> ...
>> CR2: 00000000c03ff000
>>
>> Tested-by: Colin Ian King <colin.king@canonical.com>
>> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
>> LKML-Reference: <20100305005932.GA22675@sli10-desk.sh.intel.com>
>> Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
>> Signed-off-by: H. Peter Anvin <hpa@zytor.com>
>> Cc: <stable@kernel.org>
>> Signed-off-by: Andy Whitcroft <apw@canonical.com>
>> ---
>>  arch/x86/power/hibernate_asm_32.S |   15 +++++++--------
>>  1 files changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/x86/power/hibernate_asm_32.S b/arch/x86/power/hibernate_asm_32.S
>> index b641388..ad47dae 100644
>> --- a/arch/x86/power/hibernate_asm_32.S
>> +++ b/arch/x86/power/hibernate_asm_32.S
>> @@ -27,10 +27,17 @@ ENTRY(swsusp_arch_suspend)
>>  	ret
>>  
>>  ENTRY(restore_image)
>> +	movl	mmu_cr4_features, %ecx
>>  	movl	resume_pg_dir, %eax
>>  	subl	$__PAGE_OFFSET, %eax
>>  	movl	%eax, %cr3
>>  
>> +	jecxz	1f	# cr4 Pentium and higher, skip if zero
>> +	andl	$~(X86_CR4_PGE), %ecx
>> +	movl	%ecx, %cr4;  # turn off PGE
>> +	movl	%cr3, %eax;  # flush TLB
>> +	movl	%eax, %cr3
>> +1:
>>  	movl	restore_pblist, %edx
>>  	.p2align 4,,7
>>  
>> @@ -54,16 +61,8 @@ done:
>>  	movl	$swapper_pg_dir, %eax
>>  	subl	$__PAGE_OFFSET, %eax
>>  	movl	%eax, %cr3
>> -	/* Flush TLB, including "global" things (vmalloc) */
>>  	movl	mmu_cr4_features, %ecx
>>  	jecxz	1f	# cr4 Pentium and higher, skip if zero
>> -	movl	%ecx, %edx
>> -	andl	$~(X86_CR4_PGE), %edx
>> -	movl	%edx, %cr4;  # turn off PGE
>> -1:
>> -	movl	%cr3, %eax;  # flush TLB
>> -	movl	%eax, %cr3
>> -	jecxz	1f	# cr4 Pentium and higher, skip if zero
>>  	movl	%ecx, %cr4;  # turn PGE back on
>>  1:
>>  
>> -- 
>> 1.7.0
>>
>>
> 
> 
>

Patch

diff --git a/arch/x86/power/hibernate_asm_32.S b/arch/x86/power/hibernate_asm_32.S
index b641388..ad47dae 100644
--- a/arch/x86/power/hibernate_asm_32.S
+++ b/arch/x86/power/hibernate_asm_32.S
@@ -27,10 +27,17 @@  ENTRY(swsusp_arch_suspend)
 	ret
 
 ENTRY(restore_image)
+	movl	mmu_cr4_features, %ecx
 	movl	resume_pg_dir, %eax
 	subl	$__PAGE_OFFSET, %eax
 	movl	%eax, %cr3
 
+	jecxz	1f	# cr4 Pentium and higher, skip if zero
+	andl	$~(X86_CR4_PGE), %ecx
+	movl	%ecx, %cr4;  # turn off PGE
+	movl	%cr3, %eax;  # flush TLB
+	movl	%eax, %cr3
+1:
 	movl	restore_pblist, %edx
 	.p2align 4,,7
 
@@ -54,16 +61,8 @@  done:
 	movl	$swapper_pg_dir, %eax
 	subl	$__PAGE_OFFSET, %eax
 	movl	%eax, %cr3
-	/* Flush TLB, including "global" things (vmalloc) */
 	movl	mmu_cr4_features, %ecx
 	jecxz	1f	# cr4 Pentium and higher, skip if zero
-	movl	%ecx, %edx
-	andl	$~(X86_CR4_PGE), %edx
-	movl	%edx, %cr4;  # turn off PGE
-1:
-	movl	%cr3, %eax;  # flush TLB
-	movl	%eax, %cr3
-	jecxz	1f	# cr4 Pentium and higher, skip if zero
 	movl	%ecx, %cr4;  # turn PGE back on
 1: