[PATCH 1/3] x86, acpi: Eliminate saved_eip

[PATCH 1/3] x86, acpi: Eliminate saved_eip

[Denys Vlasenko]

It always contains the address of "ret_point" label

Run-tested.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: Pavel Machek <pavel@ucw.cz>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Ingo Molnar <mingo@kernel.org>
CC: Borislav Petkov <bp@alien8.de>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Frederic Weisbecker <fweisbec@gmail.com>
CC: Alexei Starovoitov <ast@plumgrid.com>
CC: Will Drewry <wad@chromium.org>
CC: Kees Cook <keescook@chromium.org>
CC: x86@kernel.org
CC: linux-kernel@vger.kernel.org
---
 arch/x86/kernel/acpi/wakeup_32.S | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/arch/x86/kernel/acpi/wakeup_32.S b/arch/x86/kernel/acpi/wakeup_32.S
index 665c6b7..eef2bd3 100644
--- a/arch/x86/kernel/acpi/wakeup_32.S
+++ b/arch/x86/kernel/acpi/wakeup_32.S
@@ -31,17 +31,11 @@ wakeup_pmode_return:
 
 	movl	%cs:saved_magic, %eax
 	cmpl	$0x12345678, %eax
-	jne	bogus_magic
-
-	# jump to place where we left off
-	movl	saved_eip, %eax
-	jmp	*%eax
-
+	je	ret_point
 bogus_magic:
 	jmp	bogus_magic
 
 
-
 save_registers:
 	sidt	saved_idt
 	sldt	saved_ldt
@@ -56,7 +50,6 @@ save_registers:
 	pushfl
 	popl	saved_context_eflags
 
-	movl	$ret_point, saved_eip
 	ret
 
 
@@ -88,7 +81,6 @@ ret_point:
 .data
 ALIGN
 ENTRY(saved_magic)	.long	0
-ENTRY(saved_eip)	.long	0
 
 # saved registers
 saved_idt:	.long	0,0
-- 
1.8.1.4

[PATCH 2/3] x86, acpi: On S3 failure, just fall through

[Denys Vlasenko]

Th "jmp" deleted by this patch in fact compiles to "jmp to next insn",
since ".p2align 4,,7" happens to not do any alignment -
aligning to 2^4 would require more than 7 bytes of padding:

000000b0 <do_suspend_lowlevel>:
  b0:   e8 fc ff ff ff          call   <save_processor_state>
  b5:   e8 8d ff ff ff          call   47 <save_registers>
  ba:   6a 03                   push   $0x3
  bc:   e8 fc ff ff ff          call   <x86_acpi_enter_sleep_state>
  c1:   83 c4 04                add    $0x4,%esp
  c4:   eb 00                   jmp    c6 <ret_point>   <============ THIS
000000c6 <ret_point>:
  c6:   e8 c4 ff ff ff          call   8f <restore_registers>

Run-tested.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: Pavel Machek <pavel@ucw.cz>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Ingo Molnar <mingo@kernel.org>
CC: Borislav Petkov <bp@alien8.de>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Frederic Weisbecker <fweisbec@gmail.com>
CC: Alexei Starovoitov <ast@plumgrid.com>
CC: Will Drewry <wad@chromium.org>
CC: Kees Cook <keescook@chromium.org>
CC: x86@kernel.org
CC: linux-kernel@vger.kernel.org
---
 arch/x86/kernel/acpi/wakeup_32.S | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/x86/kernel/acpi/wakeup_32.S b/arch/x86/kernel/acpi/wakeup_32.S
index eef2bd3..5fec9b0 100644
--- a/arch/x86/kernel/acpi/wakeup_32.S
+++ b/arch/x86/kernel/acpi/wakeup_32.S
@@ -68,11 +68,8 @@ ENTRY(do_suspend_lowlevel)
 	pushl	$3
 	call	x86_acpi_enter_sleep_state
 	addl	$4, %esp
+# If S3 fails, we simply fall through to S3 wakeup code:
 
-#	In case of S3 failure, we'll emerge here.  Jump
-# 	to ret_point to recover
-	jmp	ret_point
-	.p2align 4,,7
 ret_point:
 	call	restore_registers
 	call	restore_processor_state
-- 
1.8.1.4

[PATCH 3/3] x86, acpi: Fold {save,restore}_registers into their single callers

[Denys Vlasenko]

This change simply block-moves bodies of "save_registers"
and "restore_registers" routines into their callers.

Well, almost. In save_registers, we were saving %esp+4
to saved_context_esp, because we were in a subroutine,
there was one additional word (return address) on stack.
Now it is gone, and we are saving %esp to saved_context_esp.

(The patch looks more confusing than the change really is.)

Run-tested.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: Pavel Machek <pavel@ucw.cz>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Ingo Molnar <mingo@kernel.org>
CC: Borislav Petkov <bp@alien8.de>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Frederic Weisbecker <fweisbec@gmail.com>
CC: Alexei Starovoitov <ast@plumgrid.com>
CC: Will Drewry <wad@chromium.org>
CC: Kees Cook <keescook@chromium.org>
CC: x86@kernel.org
CC: linux-kernel@vger.kernel.org
---
 arch/x86/kernel/acpi/wakeup_32.S | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kernel/acpi/wakeup_32.S b/arch/x86/kernel/acpi/wakeup_32.S
index 5fec9b0..b8ab4d3 100644
--- a/arch/x86/kernel/acpi/wakeup_32.S
+++ b/arch/x86/kernel/acpi/wakeup_32.S
@@ -36,13 +36,13 @@ bogus_magic:
 	jmp	bogus_magic
 
 
-save_registers:
+ENTRY(do_suspend_lowlevel)
+	call	save_processor_state
+
 	sidt	saved_idt
 	sldt	saved_ldt
 	str	saved_tss
-
-	leal	4(%esp), %eax
-	movl	%eax, saved_context_esp
+	movl	%esp, saved_context_esp
 	movl	%ebx, saved_context_ebx
 	movl	%ebp, saved_context_ebp
 	movl	%esi, saved_context_esi
@@ -50,28 +50,19 @@ save_registers:
 	pushfl
 	popl	saved_context_eflags
 
-	ret
-
+	pushl	$3
+	call	x86_acpi_enter_sleep_state
+	addl	$4, %esp
+# If S3 fails, we simply fall through to S3 wakeup code:
 
-restore_registers:
+ret_point:
 	movl	saved_context_ebp, %ebp
 	movl	saved_context_ebx, %ebx
 	movl	saved_context_esi, %esi
 	movl	saved_context_edi, %edi
 	pushl	saved_context_eflags
 	popfl
-	ret
-
-ENTRY(do_suspend_lowlevel)
-	call	save_processor_state
-	call	save_registers
-	pushl	$3
-	call	x86_acpi_enter_sleep_state
-	addl	$4, %esp
-# If S3 fails, we simply fall through to S3 wakeup code:
 
-ret_point:
-	call	restore_registers
 	call	restore_processor_state
 	ret
 
-- 
1.8.1.4

Re: [PATCH 1/3] x86, acpi: Eliminate saved_eip

[Borislav Petkov]

On Sun, Jun 14, 2015 at 03:23:08PM +0200, Denys Vlasenko wrote:
> It always contains the address of "ret_point" label

This commit message is very laconic - I had to go look at the code just
to see what you mean. I guess it should be something like:

"Get rid of the global variable saved_eip as it is only written to and
jump directly to ret_point instead. No functionality change."

Other than that, patch looks ok.

> Run-tested.
> 
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> CC: Pavel Machek <pavel@ucw.cz>
> CC: Linus Torvalds <torvalds@linux-foundation.org>
> CC: Steven Rostedt <rostedt@goodmis.org>
> CC: Ingo Molnar <mingo@kernel.org>
> CC: Borislav Petkov <bp@alien8.de>
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: Andy Lutomirski <luto@amacapital.net>
> CC: Oleg Nesterov <oleg@redhat.com>
> CC: Frederic Weisbecker <fweisbec@gmail.com>
> CC: Alexei Starovoitov <ast@plumgrid.com>
> CC: Will Drewry <wad@chromium.org>
> CC: Kees Cook <keescook@chromium.org>
> CC: x86@kernel.org
> CC: linux-kernel@vger.kernel.org
> ---
>  arch/x86/kernel/acpi/wakeup_32.S | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)

Reviewed-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH 2/3] x86, acpi: On S3 failure, just fall through

[Borislav Petkov]

On Sun, Jun 14, 2015 at 03:23:09PM +0200, Denys Vlasenko wrote:
> Th "jmp" deleted by this patch in fact compiles to "jmp to next insn",
> since ".p2align 4,,7" happens to not do any alignment -
> aligning to 2^4 would require more than 7 bytes of padding:
> 
> 000000b0 <do_suspend_lowlevel>:
>   b0:   e8 fc ff ff ff          call   <save_processor_state>
>   b5:   e8 8d ff ff ff          call   47 <save_registers>
>   ba:   6a 03                   push   $0x3
>   bc:   e8 fc ff ff ff          call   <x86_acpi_enter_sleep_state>
>   c1:   83 c4 04                add    $0x4,%esp
>   c4:   eb 00                   jmp    c6 <ret_point>   <============ THIS
> 000000c6 <ret_point>:
>   c6:   e8 c4 ff ff ff          call   8f <restore_registers>
> 
> Run-tested.
> 
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> CC: Pavel Machek <pavel@ucw.cz>
> CC: Linus Torvalds <torvalds@linux-foundation.org>
> CC: Steven Rostedt <rostedt@goodmis.org>
> CC: Ingo Molnar <mingo@kernel.org>
> CC: Borislav Petkov <bp@alien8.de>
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: Andy Lutomirski <luto@amacapital.net>
> CC: Oleg Nesterov <oleg@redhat.com>
> CC: Frederic Weisbecker <fweisbec@gmail.com>
> CC: Alexei Starovoitov <ast@plumgrid.com>
> CC: Will Drewry <wad@chromium.org>
> CC: Kees Cook <keescook@chromium.org>
> CC: x86@kernel.org
> CC: linux-kernel@vger.kernel.org
> ---
>  arch/x86/kernel/acpi/wakeup_32.S | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/acpi/wakeup_32.S b/arch/x86/kernel/acpi/wakeup_32.S
> index eef2bd3..5fec9b0 100644
> --- a/arch/x86/kernel/acpi/wakeup_32.S
> +++ b/arch/x86/kernel/acpi/wakeup_32.S
> @@ -68,11 +68,8 @@ ENTRY(do_suspend_lowlevel)
>  	pushl	$3
>  	call	x86_acpi_enter_sleep_state
>  	addl	$4, %esp
> +# If S3 fails, we simply fall through to S3 wakeup code:
>  
> -#	In case of S3 failure, we'll emerge here.  Jump
> -# 	to ret_point to recover
> -	jmp	ret_point
> -	.p2align 4,,7
>  ret_point:
>  	call	restore_registers
>  	call	restore_processor_state

Reviewed-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH 1/3] x86, acpi: Eliminate saved_eip

[Pavel Machek]

> diff --git a/arch/x86/kernel/acpi/wakeup_32.S b/arch/x86/kernel/acpi/wakeup_32.S
> index 665c6b7..eef2bd3 100644
> --- a/arch/x86/kernel/acpi/wakeup_32.S
> +++ b/arch/x86/kernel/acpi/wakeup_32.S
> @@ -31,17 +31,11 @@ wakeup_pmode_return:
>  
>  	movl	%cs:saved_magic, %eax
>  	cmpl	$0x12345678, %eax
> -	jne	bogus_magic
> -
> -	# jump to place where we left off
> -	movl	saved_eip, %eax
> -	jmp	*%eax
> -
> +	je	ret_point
>  bogus_magic:

Longjump is supposed to flush the prefetch, but hopefully nothing
depends on that detail... ...

no.

https://en.wikibooks.org/wiki/X86_Assembly/Protected_Mode#Entering_Protected_Mode

longjump is supposed to be there. Sorry about that. I guess indirect
jump is not neccessary, and we should add a comment there.

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

Re: [PATCH 1/3] x86, acpi: Eliminate saved_eip

[Denys Vlasenko]

On 06/16/2015 12:05 PM, Pavel Machek wrote:
> 
>> diff --git a/arch/x86/kernel/acpi/wakeup_32.S b/arch/x86/kernel/acpi/wakeup_32.S
>> index 665c6b7..eef2bd3 100644
>> --- a/arch/x86/kernel/acpi/wakeup_32.S
>> +++ b/arch/x86/kernel/acpi/wakeup_32.S
>> @@ -31,17 +31,11 @@ wakeup_pmode_return:
>>  
>>  	movl	%cs:saved_magic, %eax
>>  	cmpl	$0x12345678, %eax
>> -	jne	bogus_magic
>> -
>> -	# jump to place where we left off
>> -	movl	saved_eip, %eax
>> -	jmp	*%eax
>> -
>> +	je	ret_point
>>  bogus_magic:
> 
> Longjump is supposed to flush the prefetch, but hopefully nothing
> depends on that detail... ...
> 
> no.
> 
> https://en.wikibooks.org/wiki/X86_Assembly/Protected_Mode#Entering_Protected_Mode
> 
> longjump is supposed to be there. Sorry about that. I guess indirect
> jump is not neccessary, and we should add a comment there.

What "long jump"? It's not a far (segment:offset) jump, it's a near
jump (one which only changes the offset), indirect one through a register.

And we aren't switching to or from protected mode here.

Re: [PATCH 1/3] x86, acpi: Eliminate saved_eip

[Pavel Machek]

On Tue 2015-06-16 14:46:20, Denys Vlasenko wrote:
> On 06/16/2015 12:05 PM, Pavel Machek wrote:
> > 
> >> diff --git a/arch/x86/kernel/acpi/wakeup_32.S b/arch/x86/kernel/acpi/wakeup_32.S
> >> index 665c6b7..eef2bd3 100644
> >> --- a/arch/x86/kernel/acpi/wakeup_32.S
> >> +++ b/arch/x86/kernel/acpi/wakeup_32.S
> >> @@ -31,17 +31,11 @@ wakeup_pmode_return:
> >>  
> >>  	movl	%cs:saved_magic, %eax
> >>  	cmpl	$0x12345678, %eax
> >> -	jne	bogus_magic
> >> -
> >> -	# jump to place where we left off
> >> -	movl	saved_eip, %eax
> >> -	jmp	*%eax
> >> -
> >> +	je	ret_point
> >>  bogus_magic:
> > 
> > Longjump is supposed to flush the prefetch, but hopefully nothing
> > depends on that detail... ...
> > 
> > no.
> > 
> > https://en.wikibooks.org/wiki/X86_Assembly/Protected_Mode#Entering_Protected_Mode
> > 
> > longjump is supposed to be there. Sorry about that. I guess indirect
> > jump is not neccessary, and we should add a comment there.
> 
> What "long jump"? It's not a far (segment:offset) jump, it's a near
> jump (one which only changes the offset), indirect one through a register.

Yup, you are right, the ljmp is few instructions above that.

> And we aren't switching to or from protected mode here.

wakeup_pmode_return... that's just after switching to protected mode
AFAICT.

Anyway,

Acked-by: Pavel Machek <pavel@ucw.cz>

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