[pm] Strange cleanups in -test8 kernel/acpi/wakeup.S

[pm] Strange cleanups in -test8 kernel/acpi/wakeup.S

[Pavel Machek]

Hi!

Some more changes landed in -test8. I have not seen them
before. Patrick, please, if you change something, can you post patch
somewhere for review before merging with Linus?

bkcvs info is:
BitKeeper to RCS/CVS export
----------------------------
revision 1.5
date: 2003/10/08 22:55:45;  author: mochel;  state: Exp;  lines: +37
-89
[power] Clean up ACPI STR assembly.

diff -Nru a/arch/i386/kernel/acpi/wakeup.S
b/arch/i386/kernel/acpi/wakeup.S
--- a/arch/i386/kernel/acpi/wakeup.S    Fri Oct 17 14:43:50 2003
+++ b/arch/i386/kernel/acpi/wakeup.S    Fri Oct 17 14:43:50 2003
@@ -172,14 +172,13 @@
 .org   0x1000

 wakeup_pmode_return:
-       movl    $__KERNEL_DS, %eax
-       movl    %eax, %ds
-       movw    $0x0e00 + 'u', %ds:(0xb8016)
-
-       # restore other segment registers
-       xorl    %eax, %eax
+       movw    $__KERNEL_DS, %ax
+       movw    %ax, %ss
+       movw    %ax, %ds
+       movw    %ax, %es
        movw    %ax, %fs
        movw    %ax, %gs
+       movw    $0x0e00 + 'u', 0xb8016

        # reload the gdt, as we need the full 32 bit address
        lgdt    saved_gdt
	~~~~~~~~~~~~~~~~~

Notice lgdt here. You have moved setup of segment registers before
loading gdt. This is actually okay, if you can be sure that all such
registers are in gdt (and not in ldt, for example).

Now, if you are sure this is okay (there is another load of gdt in
real mode), you can kill this lgdt, too, and  kill the comment below.

@@ -192,46 +191,30 @@
        wbinvd

        # and restore the stack ... but you need gdt for this to work
-       movl    $__KERNEL_DS, %eax
-       movw    %ax, %ss
-       movw    %ax, %ds
-       movw    %ax, %es
-       movw    %ax, %fs
-       movw    %ax, %gs
-       movl    saved_esp, %esp
+       movl    saved_context_esp, %esp

...

-       movw    $0x0e00 + 'W', %ds:(0xb8018)
-       movl    $(1024*1024*3), %ecx
-       movl    $0, %esi
-       rep     lodsb
-       movw    $0x0e00 + 'O', %ds:(0xb8018)
+       movw    $0x0e00 + 'W', 0xb8018
+       outl    %eax, $0x80
+       outl    %eax, $0x80
+       movw    $0x0e00 + 'O', 0xb8018

What are these outl-s? I do not see %ax being initialized... Some kind
of debugging hack? If you are trying to emulate delays, those are not
needed, what you killed were debugging hacks.

You no longer save %eax, %ecx, %edx, %esp, %ebp, %edi, %esi. This
means you probably need to add asmlinkage somewhere...
							Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: [pm] Strange cleanups in -test8 kernel/acpi/wakeup.S

[Patrick Mochel]

> Some more changes landed in -test8. I have not seen them
> before. Patrick, please, if you change something, can you post patch
> somewhere for review before merging with Linus?

Pavel, I wrote the code in the first place, before you littered your
'debug hacks' throughout it. I have merely been trying to simplify it for 
debugging on other processors that are known not to work. While I 
understand your generic plea for review, I fail to see how it would help 
with this assembly.. 

> bkcvs info is:
> BitKeeper to RCS/CVS export
> ----------------------------
> revision 1.5
> date: 2003/10/08 22:55:45;  author: mochel;  state: Exp;  lines: +37
> -89
> [power] Clean up ACPI STR assembly.

It might help if you read the full changeset comments. 

> diff -Nru a/arch/i386/kernel/acpi/wakeup.S
> b/arch/i386/kernel/acpi/wakeup.S
> --- a/arch/i386/kernel/acpi/wakeup.S    Fri Oct 17 14:43:50 2003
> +++ b/arch/i386/kernel/acpi/wakeup.S    Fri Oct 17 14:43:50 2003
> @@ -172,14 +172,13 @@
>  .org   0x1000
> 
>  wakeup_pmode_return:
> -       movl    $__KERNEL_DS, %eax
> -       movl    %eax, %ds
> -       movw    $0x0e00 + 'u', %ds:(0xb8016)
> -
> -       # restore other segment registers
> -       xorl    %eax, %eax
> +       movw    $__KERNEL_DS, %ax
> +       movw    %ax, %ss
> +       movw    %ax, %ds
> +       movw    %ax, %es
>         movw    %ax, %fs
>         movw    %ax, %gs
> +       movw    $0x0e00 + 'u', 0xb8016
> 
>         # reload the gdt, as we need the full 32 bit address
>         lgdt    saved_gdt
> 	~~~~~~~~~~~~~~~~~
> 
> Notice lgdt here. You have moved setup of segment registers before
> loading gdt. This is actually okay, if you can be sure that all such
> registers are in gdt (and not in ldt, for example).

All segments are in the GDT, as we use the same GDT in real mode as we do 
in protected mode. However, you must reload the GDT in protected mode 
because the GDTR is only 24 bits in real mode, but 32 in protected mode. 

>         # and restore the stack ... but you need gdt for this to work
> -       movl    $__KERNEL_DS, %eax
> -       movw    %ax, %ss
> -       movw    %ax, %ds
> -       movw    %ax, %es
> -       movw    %ax, %fs
> -       movw    %ax, %gs
> -       movl    saved_esp, %esp
> +       movl    saved_context_esp, %esp
> 
> ...
> 
> -       movw    $0x0e00 + 'W', %ds:(0xb8018)
> -       movl    $(1024*1024*3), %ecx
> -       movl    $0, %esi
> -       rep     lodsb
> -       movw    $0x0e00 + 'O', %ds:(0xb8018)
> +       movw    $0x0e00 + 'W', 0xb8018
> +       outl    %eax, $0x80
> +       outl    %eax, $0x80
> +       movw    $0x0e00 + 'O', 0xb8018
> 
> What are these outl-s? I do not see %ax being initialized... Some kind
> of debugging hack? If you are trying to emulate delays, those are not
> needed, what you killed were debugging hacks.

Debugging hacks that did what? AFAICT, it only implemented a small delay 
by loading %eax with 0 a few million times. 

That's better done by writing to port 0x80. It's a debug port that the
BIOS uses to report progress. It can also be used for delay. There are
systems and PCI cards that have LEDs that report what is written to port
0x80.

> You no longer save %eax, %ecx, %edx, %esp, %ebp, %edi, %esi. This
> means you probably need to add asmlinkage somewhere...

Uh, first read save_registers() near the end of the file. Next, please
understand, as I've told you before, that %eax, %ecx, and %edx are
caller-savable registers. They're not expected to be saved. 


	Pat

Re: [pm] Strange cleanups in -test8 kernel/acpi/wakeup.S

[Pavel Machek]

Hi!

> > bkcvs info is:
> > BitKeeper to RCS/CVS export
> > ----------------------------
> > revision 1.5
> > date: 2003/10/08 22:55:45;  author: mochel;  state: Exp;  lines: +37
> > -89
> > [power] Clean up ACPI STR assembly.
> 
> It might help if you read the full changeset comments. 

Sorry, I did not see them anywhere :-(.

> > diff -Nru a/arch/i386/kernel/acpi/wakeup.S
> > b/arch/i386/kernel/acpi/wakeup.S
> > --- a/arch/i386/kernel/acpi/wakeup.S    Fri Oct 17 14:43:50 2003
> > +++ b/arch/i386/kernel/acpi/wakeup.S    Fri Oct 17 14:43:50 2003
> > @@ -172,14 +172,13 @@
> >  .org   0x1000
> > 
> >  wakeup_pmode_return:
> > -       movl    $__KERNEL_DS, %eax
> > -       movl    %eax, %ds
> > -       movw    $0x0e00 + 'u', %ds:(0xb8016)
> > -
> > -       # restore other segment registers
> > -       xorl    %eax, %eax
> > +       movw    $__KERNEL_DS, %ax
> > +       movw    %ax, %ss
> > +       movw    %ax, %ds
> > +       movw    %ax, %es
> >         movw    %ax, %fs
> >         movw    %ax, %gs
> > +       movw    $0x0e00 + 'u', 0xb8016
> > 
> >         # reload the gdt, as we need the full 32 bit address
> >         lgdt    saved_gdt
> > 	~~~~~~~~~~~~~~~~~
> > 
> > Notice lgdt here. You have moved setup of segment registers before
> > loading gdt. This is actually okay, if you can be sure that all such
> > registers are in gdt (and not in ldt, for example).
> 
> All segments are in the GDT, as we use the same GDT in real mode as we do 
> in protected mode. However, you must reload the GDT in protected mode 
> because the GDTR is only 24 bits in real mode, but 32 in protected
> mode. 

Ok, sorry.

> >         # and restore the stack ... but you need gdt for this to work
> > -       movl    $__KERNEL_DS, %eax
> > -       movw    %ax, %ss
> > -       movw    %ax, %ds
> > -       movw    %ax, %es
> > -       movw    %ax, %fs
> > -       movw    %ax, %gs
> > -       movl    saved_esp, %esp
> > +       movl    saved_context_esp, %esp
> > 
> > ...
> > 
> > -       movw    $0x0e00 + 'W', %ds:(0xb8018)
> > -       movl    $(1024*1024*3), %ecx
> > -       movl    $0, %esi
> > -       rep     lodsb
> > -       movw    $0x0e00 + 'O', %ds:(0xb8018)
> > +       movw    $0x0e00 + 'W', 0xb8018
> > +       outl    %eax, $0x80
> > +       outl    %eax, $0x80
> > +       movw    $0x0e00 + 'O', 0xb8018
> > 
> > What are these outl-s? I do not see %ax being initialized... Some kind
> > of debugging hack? If you are trying to emulate delays, those are not
> > needed, what you killed were debugging hacks.
> 
> Debugging hacks that did what? AFAICT, it only implemented a small delay 
> by loading %eax with 0 a few million times. 

One was test "is stack working or do we get fault", and second was "do
our page tables look reasonable". I was not trying to pause.

Anyway it was undocumented and it was hack. Best thing is probably to
kill it. Here's a patch.

								Pavel

--- tmp/linux/arch/i386/kernel/acpi/wakeup.S	2003-10-18 20:26:27.000000000 +0200
+++ linux/arch/i386/kernel/acpi/wakeup.S	2003-10-20 21:16:06.000000000 +0200
@@ -193,11 +193,6 @@
 	# and restore the stack ... but you need gdt for this to work
 	movl	saved_context_esp, %esp
 
-	movw	$0x0e00 + 'W', 0xb8018
-	outl	%eax, $0x80
-	outl	%eax, $0x80
-	movw	$0x0e00 + 'O', 0xb8018
-
 	movl	%cs:saved_magic, %eax
 	cmpl	$0x12345678, %eax
 	jne	bogus_magic
@@ -205,9 +200,6 @@
 	# jump to place where we left off
 	movl	saved_eip,%eax
 	movw	$0x0e00 + 'x', 0xb8018
-	outl	%eax, $0x80
-	outl	%eax, $0x80
-	movw	$0x0e00 + '!', 0xb801a
 	jmp	*%eax
 
 bogus_magic:


-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: [pm] Strange cleanups in -test8 kernel/acpi/wakeup.S

[Patrick Mochel]

> One was test "is stack working or do we get fault", and second was "do
> our page tables look reasonable". I was not trying to pause.
> 
> Anyway it was undocumented and it was hack. Best thing is probably to
> kill it. Here's a patch.

Ah, I see. Will apply patch.

Thanks,


	Pat

Re: [pm] Strange cleanups in -test8 kernel/acpi/wakeup.S

[Mika Penttilä]

Patrick Mochel wrote:

>>Some more changes landed in -test8. I have not seen them
>>before. Patrick, please, if you change something, can you post patch
>>somewhere for review before merging with Linus?
>>    
>>
>
>Pavel, I wrote the code in the first place, before you littered your
>'debug hacks' throughout it. I have merely been trying to simplify it for 
>debugging on other processors that are known not to work. While I 
>understand your generic plea for review, I fail to see how it would help 
>with this assembly.. 
>
>  
>
>>bkcvs info is:
>>BitKeeper to RCS/CVS export
>>----------------------------
>>revision 1.5
>>date: 2003/10/08 22:55:45;  author: mochel;  state: Exp;  lines: +37
>>-89
>>[power] Clean up ACPI STR assembly.
>>    
>>
>
>It might help if you read the full changeset comments. 
>
>  
>
>>diff -Nru a/arch/i386/kernel/acpi/wakeup.S
>>b/arch/i386/kernel/acpi/wakeup.S
>>--- a/arch/i386/kernel/acpi/wakeup.S    Fri Oct 17 14:43:50 2003
>>+++ b/arch/i386/kernel/acpi/wakeup.S    Fri Oct 17 14:43:50 2003
>>@@ -172,14 +172,13 @@
>> .org   0x1000
>>
>> wakeup_pmode_return:
>>-       movl    $__KERNEL_DS, %eax
>>-       movl    %eax, %ds
>>-       movw    $0x0e00 + 'u', %ds:(0xb8016)
>>-
>>-       # restore other segment registers
>>-       xorl    %eax, %eax
>>+       movw    $__KERNEL_DS, %ax
>>+       movw    %ax, %ss
>>+       movw    %ax, %ds
>>+       movw    %ax, %es
>>        movw    %ax, %fs
>>        movw    %ax, %gs
>>+       movw    $0x0e00 + 'u', 0xb8016
>>
>>        # reload the gdt, as we need the full 32 bit address
>>        lgdt    saved_gdt
>>	~~~~~~~~~~~~~~~~~
>>
>>Notice lgdt here. You have moved setup of segment registers before
>>loading gdt. This is actually okay, if you can be sure that all such
>>registers are in gdt (and not in ldt, for example).
>>    
>>
>
>All segments are in the GDT, as we use the same GDT in real mode as we do 
>in protected mode. However, you must reload the GDT in protected mode 
>because the GDTR is only 24 bits in real mode, but 32 in protected mode. 
>
>  
>
To be exact, GDTR is always 48 bits in x86. Reloading it twice seems 
pretty pointless.

--Mika