Strange cleanups in -test8 kernel/acpi/wakeup.S
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.Sb/arch/i386/kernel/acpi/wakeup.S
arch/i386/kernel/acpi/wakeup.S Fri Oct 17 14:43:50 2003 +5 -6
@@ -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
Signature · 2 lines
When do you have a heart between your knees? [Johanka's followup: and *two* hearts?]
Quoted 4 lines
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..
Quoted 7 lines
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.
Quoted 26 lines
diff -Nru
a/arch/i386/kernel/acpi/wakeup.Sb/arch/i386/kernel/acpi/wakeup.Sarch/i386/kernel/acpi/wakeup.S Fri Oct 17 14:43:50 2003 +5
-6@@ -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~~~~~~~~~~~~~~~~~
Quoted 3 lines
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.
Quoted 22 lines
# and restore the stack ... but you need gdt for this to workarch/i386/kernel/acpi/wakeup.S Fri Oct 17 14:43:50 2003 +5
-12- 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
Quoted 3 lines
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.
Quoted 2 lines
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
Hi!
Quoted 9 lines
Quoted 7 lines
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 :-(.
Quoted 26 lines
Quoted 26 lines
diff -Nru
a/arch/i386/kernel/acpi/wakeup.Sb/arch/i386/kernel/acpi/wakeup.Sarch/i386/kernel/acpi/wakeup.S Fri Oct 17 14:43:50 2003 +5
-6@@ -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~~~~~~~~~~~~~~~~~
Quoted 8 lines
Quoted 3 lines
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.
Quoted 22 lines
Quoted 22 lines
# and restore the stack ... but you need gdt for this to workarch/i386/kernel/acpi/wakeup.S Fri Oct 17 14:43:50 2003 +5
-12- 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
Quoted 6 lines
Quoted 3 lines
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
arch/i386/kernel/acpi/wakeup.S renamed from linux/arch/i386/kernel/acpi/wakeup.S +0 -8
@@ -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:
Signature · 2 lines
When do you have a heart between your knees? [Johanka's followup: and *two* hearts?]
Quoted 6 lines
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
Quoted 51 lines · Patrick Mochel
Quoted 5 lines
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..Quoted 9 lines
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.
Quoted 26 lines
diff -Nru
a/arch/i386/kernel/acpi/wakeup.Sb/arch/i386/kernel/acpi/wakeup.Sarch/i386/kernel/acpi/wakeup.S Fri Oct 17 14:43:50 2003 +0
-0@@ -172,14 +172,13 @@.org
0x1000
wakeup_pmode_return:arch/i386/kernel/acpi/wakeup.S Fri Oct 17 14:43:50 2003 +5
-6- 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~~~~~~~~~~~~~~~~~
Quoted 10 lines
Quoted 5 lines
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