* [PATCH 2/3] x86, acpi: On S3 failure, just fall through
2015-06-14 13:23 [PATCH 1/3] x86, acpi: Eliminate saved_eip Denys Vlasenko
@ 2015-06-14 13:23 ` Denys Vlasenko
2015-06-15 10:35 ` Borislav Petkov
2015-06-14 13:23 ` [PATCH 3/3] x86, acpi: Fold {save,restore}_registers into their single callers Denys Vlasenko
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Denys Vlasenko @ 2015-06-14 13:23 UTC (permalink / raw)
To: Ingo Molnar
Cc: Denys Vlasenko, Pavel Machek, Linus Torvalds, Steven Rostedt,
Borislav Petkov, H. Peter Anvin, Andy Lutomirski, Oleg Nesterov,
Frederic Weisbecker, Alexei Starovoitov, Will Drewry, Kees Cook,
x86, linux-kernel
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 2/3] x86, acpi: On S3 failure, just fall through
2015-06-14 13:23 ` [PATCH 2/3] x86, acpi: On S3 failure, just fall through Denys Vlasenko
@ 2015-06-15 10:35 ` Borislav Petkov
0 siblings, 0 replies; 8+ messages in thread
From: Borislav Petkov @ 2015-06-15 10:35 UTC (permalink / raw)
To: Denys Vlasenko
Cc: Ingo Molnar, Pavel Machek, Linus Torvalds, Steven Rostedt,
H. Peter Anvin, Andy Lutomirski, Oleg Nesterov,
Frederic Weisbecker, Alexei Starovoitov, Will Drewry, Kees Cook,
x86, linux-kernel
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.
--
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] x86, acpi: Fold {save,restore}_registers into their single callers
2015-06-14 13:23 [PATCH 1/3] x86, acpi: Eliminate saved_eip Denys Vlasenko
2015-06-14 13:23 ` [PATCH 2/3] x86, acpi: On S3 failure, just fall through Denys Vlasenko
@ 2015-06-14 13:23 ` Denys Vlasenko
2015-06-15 8:58 ` [PATCH 1/3] x86, acpi: Eliminate saved_eip Borislav Petkov
2015-06-16 10:05 ` Pavel Machek
3 siblings, 0 replies; 8+ messages in thread
From: Denys Vlasenko @ 2015-06-14 13:23 UTC (permalink / raw)
To: Ingo Molnar
Cc: Denys Vlasenko, Pavel Machek, Linus Torvalds, Steven Rostedt,
Borislav Petkov, H. Peter Anvin, Andy Lutomirski, Oleg Nesterov,
Frederic Weisbecker, Alexei Starovoitov, Will Drewry, Kees Cook,
x86, linux-kernel
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 1/3] x86, acpi: Eliminate saved_eip
2015-06-14 13:23 [PATCH 1/3] x86, acpi: Eliminate saved_eip Denys Vlasenko
2015-06-14 13:23 ` [PATCH 2/3] x86, acpi: On S3 failure, just fall through Denys Vlasenko
2015-06-14 13:23 ` [PATCH 3/3] x86, acpi: Fold {save,restore}_registers into their single callers Denys Vlasenko
@ 2015-06-15 8:58 ` Borislav Petkov
2015-06-16 10:05 ` Pavel Machek
3 siblings, 0 replies; 8+ messages in thread
From: Borislav Petkov @ 2015-06-15 8:58 UTC (permalink / raw)
To: Denys Vlasenko
Cc: Ingo Molnar, Pavel Machek, Linus Torvalds, Steven Rostedt,
H. Peter Anvin, Andy Lutomirski, Oleg Nesterov,
Frederic Weisbecker, Alexei Starovoitov, Will Drewry, Kees Cook,
x86, linux-kernel
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.
--
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/3] x86, acpi: Eliminate saved_eip
2015-06-14 13:23 [PATCH 1/3] x86, acpi: Eliminate saved_eip Denys Vlasenko
` (2 preceding siblings ...)
2015-06-15 8:58 ` [PATCH 1/3] x86, acpi: Eliminate saved_eip Borislav Petkov
@ 2015-06-16 10:05 ` Pavel Machek
2015-06-16 12:46 ` Denys Vlasenko
3 siblings, 1 reply; 8+ messages in thread
From: Pavel Machek @ 2015-06-16 10:05 UTC (permalink / raw)
To: Denys Vlasenko
Cc: Ingo Molnar, Linus Torvalds, Steven Rostedt, Borislav Petkov,
H. Peter Anvin, Andy Lutomirski, Oleg Nesterov,
Frederic Weisbecker, Alexei Starovoitov, Will Drewry, Kees Cook,
x86, linux-kernel
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/3] x86, acpi: Eliminate saved_eip
2015-06-16 10:05 ` Pavel Machek
@ 2015-06-16 12:46 ` Denys Vlasenko
2015-06-17 9:28 ` Pavel Machek
0 siblings, 1 reply; 8+ messages in thread
From: Denys Vlasenko @ 2015-06-16 12:46 UTC (permalink / raw)
To: Pavel Machek
Cc: Ingo Molnar, Linus Torvalds, Steven Rostedt, Borislav Petkov,
H. Peter Anvin, Andy Lutomirski, Oleg Nesterov,
Frederic Weisbecker, Alexei Starovoitov, Will Drewry, Kees Cook,
x86, linux-kernel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] x86, acpi: Eliminate saved_eip
2015-06-16 12:46 ` Denys Vlasenko
@ 2015-06-17 9:28 ` Pavel Machek
0 siblings, 0 replies; 8+ messages in thread
From: Pavel Machek @ 2015-06-17 9:28 UTC (permalink / raw)
To: Denys Vlasenko
Cc: Ingo Molnar, Linus Torvalds, Steven Rostedt, Borislav Petkov,
H. Peter Anvin, Andy Lutomirski, Oleg Nesterov,
Frederic Weisbecker, Alexei Starovoitov, Will Drewry, Kees Cook,
x86, linux-kernel
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
^ permalink raw reply [flat|nested] 8+ messages in thread