It always contains the address of "ret_point" label
Run-tested.
Signed-off-by: Denys Vlasenko <[email protected]>
CC: Pavel Machek <[email protected]>
CC: Linus Torvalds <[email protected]>
CC: Steven Rostedt <[email protected]>
CC: Ingo Molnar <[email protected]>
CC: Borislav Petkov <[email protected]>
CC: "H. Peter Anvin" <[email protected]>
CC: Andy Lutomirski <[email protected]>
CC: Oleg Nesterov <[email protected]>
CC: Frederic Weisbecker <[email protected]>
CC: Alexei Starovoitov <[email protected]>
CC: Will Drewry <[email protected]>
CC: Kees Cook <[email protected]>
CC: [email protected]
CC: [email protected]
---
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
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 <[email protected]>
CC: Pavel Machek <[email protected]>
CC: Linus Torvalds <[email protected]>
CC: Steven Rostedt <[email protected]>
CC: Ingo Molnar <[email protected]>
CC: Borislav Petkov <[email protected]>
CC: "H. Peter Anvin" <[email protected]>
CC: Andy Lutomirski <[email protected]>
CC: Oleg Nesterov <[email protected]>
CC: Frederic Weisbecker <[email protected]>
CC: Alexei Starovoitov <[email protected]>
CC: Will Drewry <[email protected]>
CC: Kees Cook <[email protected]>
CC: [email protected]
CC: [email protected]
---
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
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 <[email protected]>
CC: Pavel Machek <[email protected]>
CC: Linus Torvalds <[email protected]>
CC: Steven Rostedt <[email protected]>
CC: Ingo Molnar <[email protected]>
CC: Borislav Petkov <[email protected]>
CC: "H. Peter Anvin" <[email protected]>
CC: Andy Lutomirski <[email protected]>
CC: Oleg Nesterov <[email protected]>
CC: Frederic Weisbecker <[email protected]>
CC: Alexei Starovoitov <[email protected]>
CC: Will Drewry <[email protected]>
CC: Kees Cook <[email protected]>
CC: [email protected]
CC: [email protected]
---
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
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 <[email protected]>
> CC: Pavel Machek <[email protected]>
> CC: Linus Torvalds <[email protected]>
> CC: Steven Rostedt <[email protected]>
> CC: Ingo Molnar <[email protected]>
> CC: Borislav Petkov <[email protected]>
> CC: "H. Peter Anvin" <[email protected]>
> CC: Andy Lutomirski <[email protected]>
> CC: Oleg Nesterov <[email protected]>
> CC: Frederic Weisbecker <[email protected]>
> CC: Alexei Starovoitov <[email protected]>
> CC: Will Drewry <[email protected]>
> CC: Kees Cook <[email protected]>
> CC: [email protected]
> CC: [email protected]
> ---
> arch/x86/kernel/acpi/wakeup_32.S | 10 +---------
> 1 file changed, 1 insertion(+), 9 deletions(-)
Reviewed-by: Borislav Petkov <[email protected]>
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
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 <[email protected]>
> CC: Pavel Machek <[email protected]>
> CC: Linus Torvalds <[email protected]>
> CC: Steven Rostedt <[email protected]>
> CC: Ingo Molnar <[email protected]>
> CC: Borislav Petkov <[email protected]>
> CC: "H. Peter Anvin" <[email protected]>
> CC: Andy Lutomirski <[email protected]>
> CC: Oleg Nesterov <[email protected]>
> CC: Frederic Weisbecker <[email protected]>
> CC: Alexei Starovoitov <[email protected]>
> CC: Will Drewry <[email protected]>
> CC: Kees Cook <[email protected]>
> CC: [email protected]
> CC: [email protected]
> ---
> 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 <[email protected]>
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
> 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
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.
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 <[email protected]>
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html