arch/x86/power/cpu_32.c __save_processor_state calls read_cr4()
only a i486 CPU doesn't have the CR4 register. Trying to read it
produces an invalid opcode oops during suspend to disk.
Added the check (boot_cpu_data.x86 >= 5) before reading the
register. If the value to be written is zero the write is
skipped.
arch/x86/power/hibernate_asm_32.S
done: swapped the use of %eax and %ecx to use jecxz for
the zero test and jump over store to %cr4.
restore_image: s/%ecx/%eax/ to be consistent with done:
In addition to __save_processor_state, acpi_save_state_mem,
efi_call_phys_prelog, and efi_call_phys_epilog had checks added
(acpi restore was in assembly and already had a check for
non-zero). There were other reads and writes of CR4, but MCE and
virtualization shouldn't be executed on a i486 anyway.
Signed-off-by: David Fries <[email protected]>
---
arch/x86/kernel/acpi/sleep.c | 4 +++-
arch/x86/kernel/efi_32.c | 6 ++++--
arch/x86/power/cpu_32.c | 8 ++++++--
arch/x86/power/hibernate_asm_32.S | 26 +++++++++++++++-----------
4 files changed, 28 insertions(+), 16 deletions(-)
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 81e5ab6..bd0f2a3 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -86,7 +86,9 @@ int acpi_save_state_mem(void)
#endif /* !CONFIG_64BIT */
header->pmode_cr0 = read_cr0();
- header->pmode_cr4 = read_cr4();
+ /* cr4 was introduced in the Pentium CPU */
+ if (boot_cpu_data.x86 >= 5)
+ header->pmode_cr4 = read_cr4();
header->realmode_flags = acpi_realmode_flags;
header->real_magic = 0x12345678;
diff --git a/arch/x86/kernel/efi_32.c b/arch/x86/kernel/efi_32.c
index 4b63c8e..ad62d31 100644
--- a/arch/x86/kernel/efi_32.c
+++ b/arch/x86/kernel/efi_32.c
@@ -53,7 +53,8 @@ void efi_call_phys_prelog(void)
* directory. If I have PAE, I just need to duplicate one entry in
* page directory.
*/
- cr4 = read_cr4();
+ /* cr4 was introduced in the Pentium CPU */
+ cr4 = boot_cpu_data.x86 >= 5 ? read_cr4() : 0;
if (cr4 & X86_CR4_PAE) {
efi_bak_pg_dir_pointer[0].pgd =
@@ -91,7 +92,8 @@ void efi_call_phys_epilog(void)
gdt_descr.size = GDT_SIZE - 1;
load_gdt(&gdt_descr);
- cr4 = read_cr4();
+ /* cr4 was introduced in the Pentium CPU */
+ cr4 = boot_cpu_data.x86 >= 5 ? read_cr4() : 0;
if (cr4 & X86_CR4_PAE) {
swapper_pg_dir[pgd_index(0)].pgd =
diff --git a/arch/x86/power/cpu_32.c b/arch/x86/power/cpu_32.c
index 7dc5d5c..c5647ea 100644
--- a/arch/x86/power/cpu_32.c
+++ b/arch/x86/power/cpu_32.c
@@ -45,7 +45,9 @@ static void __save_processor_state(struct saved_context *ctxt)
ctxt->cr0 = read_cr0();
ctxt->cr2 = read_cr2();
ctxt->cr3 = read_cr3();
- ctxt->cr4 = read_cr4();
+ /* cr4 was introduced in the Pentium CPU */
+ if (boot_cpu_data.x86 >= 5)
+ ctxt->cr4 = read_cr4();
}
/* Needed by apm.c */
@@ -98,7 +100,9 @@ static void __restore_processor_state(struct saved_context *ctxt)
/*
* control registers
*/
- write_cr4(ctxt->cr4);
+ /* cr4 was introduced in the Pentium CPU */
+ if (ctxt->cr4)
+ write_cr4(ctxt->cr4);
write_cr3(ctxt->cr3);
write_cr2(ctxt->cr2);
write_cr0(ctxt->cr0);
diff --git a/arch/x86/power/hibernate_asm_32.S b/arch/x86/power/hibernate_asm_32.S
index b95aa6c..4fc7e87 100644
--- a/arch/x86/power/hibernate_asm_32.S
+++ b/arch/x86/power/hibernate_asm_32.S
@@ -28,9 +28,9 @@ ENTRY(swsusp_arch_suspend)
ret
ENTRY(restore_image)
- movl resume_pg_dir, %ecx
- subl $__PAGE_OFFSET, %ecx
- movl %ecx, %cr3
+ movl resume_pg_dir, %eax
+ subl $__PAGE_OFFSET, %eax
+ movl %eax, %cr3
movl restore_pblist, %edx
.p2align 4,,7
@@ -52,17 +52,21 @@ copy_loop:
done:
/* go back to the original page tables */
- movl $swapper_pg_dir, %ecx
- subl $__PAGE_OFFSET, %ecx
- movl %ecx, %cr3
+ movl $swapper_pg_dir, %eax
+ subl $__PAGE_OFFSET, %eax
+ movl %eax, %cr3
/* Flush TLB, including "global" things (vmalloc) */
- movl mmu_cr4_features, %eax
- movl %eax, %edx
+ movl mmu_cr4_features, %ecx
+ jecxz 1f # cr4 Pentium and higher, skip if zero
+ movl %ecx, %edx
andl $~(1<<7), %edx; # PGE
movl %edx, %cr4; # turn off PGE
- movl %cr3, %ecx; # flush TLB
- movl %ecx, %cr3
- movl %eax, %cr4; # turn PGE back on
+1:
+ movl %cr3, %eax; # flush TLB
+ movl %eax, %cr3
+ jecxz 1f # cr4 Pentium and higher, skip if zero
+ movl %ecx, %cr4; # turn PGE back on
+1:
movl saved_context_esp, %esp
movl saved_context_ebp, %ebp
--
1.4.4.4
On Sun, 17 Aug 2008, David Fries wrote:
> arch/x86/power/cpu_32.c __save_processor_state calls read_cr4()
> only a i486 CPU doesn't have the CR4 register. Trying to read it
> produces an invalid opcode oops during suspend to disk.
[...]
> diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
> index 81e5ab6..bd0f2a3 100644
> --- a/arch/x86/kernel/acpi/sleep.c
> +++ b/arch/x86/kernel/acpi/sleep.c
> @@ -86,7 +86,9 @@ int acpi_save_state_mem(void)
> #endif /* !CONFIG_64BIT */
>
> header->pmode_cr0 = read_cr0();
> - header->pmode_cr4 = read_cr4();
> + /* cr4 was introduced in the Pentium CPU */
> + if (boot_cpu_data.x86 >= 5)
> + header->pmode_cr4 = read_cr4();
> header->realmode_flags = acpi_realmode_flags;
> header->real_magic = 0x12345678;
>
NACK. Later i486 chips do have CR4 -- for PSE, VME, etc. (the set of
features varies across the line). Use a fixup as elsewhere or something.
Maciej
Maciej W. Rozycki wrote:
> On Sun, 17 Aug 2008, David Fries wrote:
>
>> arch/x86/power/cpu_32.c __save_processor_state calls read_cr4()
>> only a i486 CPU doesn't have the CR4 register. Trying to read it
>> produces an invalid opcode oops during suspend to disk.
> [...]
>> diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
>> index 81e5ab6..bd0f2a3 100644
>> --- a/arch/x86/kernel/acpi/sleep.c
>> +++ b/arch/x86/kernel/acpi/sleep.c
>> @@ -86,7 +86,9 @@ int acpi_save_state_mem(void)
>> #endif /* !CONFIG_64BIT */
>>
>> header->pmode_cr0 = read_cr0();
>> - header->pmode_cr4 = read_cr4();
>> + /* cr4 was introduced in the Pentium CPU */
>> + if (boot_cpu_data.x86 >= 5)
>> + header->pmode_cr4 = read_cr4();
>> header->realmode_flags = acpi_realmode_flags;
>> header->real_magic = 0x12345678;
>>
>
> NACK. Later i486 chips do have CR4 -- for PSE, VME, etc. (the set of
> features varies across the line). Use a fixup as elsewhere or something.
>
The other alternative is to probe for the CPUID instruction (via
EFLAGS.ID) -- CR4 is present if and only if CPUID exists.
-hpa
"H. Peter Anvin" <[email protected]> writes:
> Maciej W. Rozycki wrote:
>> On Sun, 17 Aug 2008, David Fries wrote:
>>
>>> arch/x86/power/cpu_32.c __save_processor_state calls read_cr4()
>>> only a i486 CPU doesn't have the CR4 register. Trying to read it
>>> produces an invalid opcode oops during suspend to disk.
>> [...]
>>> diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
>>> index 81e5ab6..bd0f2a3 100644
>>> --- a/arch/x86/kernel/acpi/sleep.c
>>> +++ b/arch/x86/kernel/acpi/sleep.c
>>> @@ -86,7 +86,9 @@ int acpi_save_state_mem(void)
>>> #endif /* !CONFIG_64BIT */
>>> header->pmode_cr0 = read_cr0();
>>> - header->pmode_cr4 = read_cr4();
>>> + /* cr4 was introduced in the Pentium CPU */
>>> + if (boot_cpu_data.x86 >= 5)
>>> + header->pmode_cr4 = read_cr4();
>>> header->realmode_flags = acpi_realmode_flags;
>>> header->real_magic = 0x12345678;
>>>
>> NACK. Later i486 chips do have CR4 -- for PSE, VME, etc. (the set
>> of
>> features varies across the line). Use a fixup as elsewhere or something.
>>
>
> The other alternative is to probe for the CPUID instruction (via
> EFLAGS.ID) -- CR4 is present if and only if CPUID exists.
Can be already checked for with boot_cpu_data.extended_cpuid_level.
-Andi
Andi Kleen wrote:
>>>
>> The other alternative is to probe for the CPUID instruction (via
>> EFLAGS.ID) -- CR4 is present if and only if CPUID exists.
>
> Can be already checked for with boot_cpu_data.extended_cpuid_level.
>
I believe you mean just plain cpuid_level (which is set to -1 if CPUID
doesn't exist.) extended_cpuid_level is only defined on x86-64, but all
x86-64 CPUs have CR4.
-hpa
On Sun, Aug 17, 2008 at 11:34:50PM -0700, H. Peter Anvin wrote:
> Andi Kleen wrote:
> >>>
> >>The other alternative is to probe for the CPUID instruction (via
> >>EFLAGS.ID) -- CR4 is present if and only if CPUID exists.
> >
> >Can be already checked for with boot_cpu_data.extended_cpuid_level.
> >
>
> I believe you mean just plain cpuid_level (which is set to -1 if CPUID
> doesn't exist.) extended_cpuid_level is only defined on x86-64, but all
> x86-64 CPUs have CR4.
Yes I meant plain cpuid_level. Just wanted to mention that before someone
opencodes another cpuid test.
-Andi
* David Fries <[email protected]> wrote:
> arch/x86/power/cpu_32.c __save_processor_state calls read_cr4() only a
> i486 CPU doesn't have the CR4 register. Trying to read it produces an
> invalid opcode oops during suspend to disk.
>
> Added the check (boot_cpu_data.x86 >= 5) before reading the register.
> If the value to be written is zero the write is skipped.
>
> arch/x86/power/hibernate_asm_32.S
> done: swapped the use of %eax and %ecx to use jecxz for
> the zero test and jump over store to %cr4.
> restore_image: s/%ecx/%eax/ to be consistent with done:
>
> In addition to __save_processor_state, acpi_save_state_mem,
> efi_call_phys_prelog, and efi_call_phys_epilog had checks added (acpi
> restore was in assembly and already had a check for non-zero). There
> were other reads and writes of CR4, but MCE and virtualization
> shouldn't be executed on a i486 anyway.
>
> Signed-off-by: David Fries <[email protected]>
applied to tip/x86/urgent, thanks David. I've changed the conditions to
read_cr4_safe() instead - that's cleaner. Could you please check whether
the patch below works fine too on your box?
Rafael, Pavel - does the commit below look good to you too?
Ingo
---------------------->
>From e437fa5586f2e3b2aaeba649fae52be1f9a6eabb Mon Sep 17 00:00:00 2001
From: David Fries <[email protected]>
Date: Sun, 17 Aug 2008 23:03:40 -0500
Subject: [PATCH] x86: fix i486 suspend to disk CR4 oops
arch/x86/power/cpu_32.c __save_processor_state calls read_cr4()
only a i486 CPU doesn't have the CR4 register. Trying to read it
produces an invalid opcode oops during suspend to disk.
Use the safe rc4 reading op instead. If the value to be written is
zero the write is skipped.
arch/x86/power/hibernate_asm_32.S
done: swapped the use of %eax and %ecx to use jecxz for
the zero test and jump over store to %cr4.
restore_image: s/%ecx/%eax/ to be consistent with done:
In addition to __save_processor_state, acpi_save_state_mem,
efi_call_phys_prelog, and efi_call_phys_epilog had checks added
(acpi restore was in assembly and already had a check for
non-zero). There were other reads and writes of CR4, but MCE and
virtualization shouldn't be executed on a i486 anyway.
Signed-off-by: David Fries <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/kernel/acpi/sleep.c | 2 +-
arch/x86/kernel/efi_32.c | 4 ++--
arch/x86/power/cpu_32.c | 6 ++++--
arch/x86/power/hibernate_asm_32.S | 26 +++++++++++++++-----------
4 files changed, 22 insertions(+), 16 deletions(-)
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 81e5ab6..426e5d9 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -86,7 +86,7 @@ int acpi_save_state_mem(void)
#endif /* !CONFIG_64BIT */
header->pmode_cr0 = read_cr0();
- header->pmode_cr4 = read_cr4();
+ header->pmode_cr4 = read_cr4_safe();
header->realmode_flags = acpi_realmode_flags;
header->real_magic = 0x12345678;
diff --git a/arch/x86/kernel/efi_32.c b/arch/x86/kernel/efi_32.c
index 4b63c8e..5cab48e 100644
--- a/arch/x86/kernel/efi_32.c
+++ b/arch/x86/kernel/efi_32.c
@@ -53,7 +53,7 @@ void efi_call_phys_prelog(void)
* directory. If I have PAE, I just need to duplicate one entry in
* page directory.
*/
- cr4 = read_cr4();
+ cr4 = read_cr4_safe();
if (cr4 & X86_CR4_PAE) {
efi_bak_pg_dir_pointer[0].pgd =
@@ -91,7 +91,7 @@ void efi_call_phys_epilog(void)
gdt_descr.size = GDT_SIZE - 1;
load_gdt(&gdt_descr);
- cr4 = read_cr4();
+ cr4 = read_cr4_safe();
if (cr4 & X86_CR4_PAE) {
swapper_pg_dir[pgd_index(0)].pgd =
diff --git a/arch/x86/power/cpu_32.c b/arch/x86/power/cpu_32.c
index 7dc5d5c..d3e083d 100644
--- a/arch/x86/power/cpu_32.c
+++ b/arch/x86/power/cpu_32.c
@@ -45,7 +45,7 @@ static void __save_processor_state(struct saved_context *ctxt)
ctxt->cr0 = read_cr0();
ctxt->cr2 = read_cr2();
ctxt->cr3 = read_cr3();
- ctxt->cr4 = read_cr4();
+ ctxt->cr4 = read_cr4_safe();
}
/* Needed by apm.c */
@@ -98,7 +98,9 @@ static void __restore_processor_state(struct saved_context *ctxt)
/*
* control registers
*/
- write_cr4(ctxt->cr4);
+ /* cr4 was introduced in the Pentium CPU */
+ if (ctxt->cr4)
+ write_cr4(ctxt->cr4);
write_cr3(ctxt->cr3);
write_cr2(ctxt->cr2);
write_cr0(ctxt->cr0);
diff --git a/arch/x86/power/hibernate_asm_32.S b/arch/x86/power/hibernate_asm_32.S
index b95aa6c..4fc7e87 100644
--- a/arch/x86/power/hibernate_asm_32.S
+++ b/arch/x86/power/hibernate_asm_32.S
@@ -28,9 +28,9 @@ ENTRY(swsusp_arch_suspend)
ret
ENTRY(restore_image)
- movl resume_pg_dir, %ecx
- subl $__PAGE_OFFSET, %ecx
- movl %ecx, %cr3
+ movl resume_pg_dir, %eax
+ subl $__PAGE_OFFSET, %eax
+ movl %eax, %cr3
movl restore_pblist, %edx
.p2align 4,,7
@@ -52,17 +52,21 @@ copy_loop:
done:
/* go back to the original page tables */
- movl $swapper_pg_dir, %ecx
- subl $__PAGE_OFFSET, %ecx
- movl %ecx, %cr3
+ movl $swapper_pg_dir, %eax
+ subl $__PAGE_OFFSET, %eax
+ movl %eax, %cr3
/* Flush TLB, including "global" things (vmalloc) */
- movl mmu_cr4_features, %eax
- movl %eax, %edx
+ movl mmu_cr4_features, %ecx
+ jecxz 1f # cr4 Pentium and higher, skip if zero
+ movl %ecx, %edx
andl $~(1<<7), %edx; # PGE
movl %edx, %cr4; # turn off PGE
- movl %cr3, %ecx; # flush TLB
- movl %ecx, %cr3
- movl %eax, %cr4; # turn PGE back on
+1:
+ movl %cr3, %eax; # flush TLB
+ movl %eax, %cr3
+ jecxz 1f # cr4 Pentium and higher, skip if zero
+ movl %ecx, %cr4; # turn PGE back on
+1:
movl saved_context_esp, %esp
movl saved_context_ebp, %ebp
Ingo Molnar wrote:
>
> applied to tip/x86/urgent, thanks David. I've changed the conditions to
> read_cr4_safe() instead - that's cleaner. Could you please check whether
> the patch below works fine too on your box?
>
> Rafael, Pavel - does the commit below look good to you too?
>
Looks good to me.
-hpa
>
> * David Fries <[email protected]> wrote:
>
> > arch/x86/power/cpu_32.c __save_processor_state calls read_cr4() only a
> > i486 CPU doesn't have the CR4 register. Trying to read it produces an
> > invalid opcode oops during suspend to disk.
> >
> > Added the check (boot_cpu_data.x86 >= 5) before reading the register.
> > If the value to be written is zero the write is skipped.
> >
> > arch/x86/power/hibernate_asm_32.S
> > done: swapped the use of %eax and %ecx to use jecxz for
> > the zero test and jump over store to %cr4.
> > restore_image: s/%ecx/%eax/ to be consistent with done:
> >
> > In addition to __save_processor_state, acpi_save_state_mem,
> > efi_call_phys_prelog, and efi_call_phys_epilog had checks added (acpi
> > restore was in assembly and already had a check for non-zero). There
> > were other reads and writes of CR4, but MCE and virtualization
> > shouldn't be executed on a i486 anyway.
> >
> > Signed-off-by: David Fries <[email protected]>
>
> applied to tip/x86/urgent, thanks David. I've changed the conditions to
> read_cr4_safe() instead - that's cleaner. Could you please check whether
> the patch below works fine too on your box?
>
> Rafael, Pavel - does the commit below look good to you too?
>
> Ingo
>
> ---------------------->
> >From e437fa5586f2e3b2aaeba649fae52be1f9a6eabb Mon Sep 17 00:00:00 2001
> From: David Fries <[email protected]>
> Date: Sun, 17 Aug 2008 23:03:40 -0500
> Subject: [PATCH] x86: fix i486 suspend to disk CR4 oops
>
> arch/x86/power/cpu_32.c __save_processor_state calls read_cr4()
> only a i486 CPU doesn't have the CR4 register. Trying to read it
> produces an invalid opcode oops during suspend to disk.
>
> Use the safe rc4 reading op instead. If the value to be written is
> zero the write is skipped.
>
> arch/x86/power/hibernate_asm_32.S
> done: swapped the use of %eax and %ecx to use jecxz for
> the zero test and jump over store to %cr4.
> restore_image: s/%ecx/%eax/ to be consistent with done:
>
> In addition to __save_processor_state, acpi_save_state_mem,
> efi_call_phys_prelog, and efi_call_phys_epilog had checks added
> (acpi restore was in assembly and already had a check for
> non-zero). There were other reads and writes of CR4, but MCE and
> virtualization shouldn't be executed on a i486 anyway.
>
> Signed-off-by: David Fries <[email protected]>
> Signed-off-by: Ingo Molnar <[email protected]>
Acked-by: Pavel Machek <[email protected]>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Monday, 18 of August 2008, Ingo Molnar wrote:
>
> * David Fries <[email protected]> wrote:
>
> > arch/x86/power/cpu_32.c __save_processor_state calls read_cr4() only a
> > i486 CPU doesn't have the CR4 register. Trying to read it produces an
> > invalid opcode oops during suspend to disk.
> >
> > Added the check (boot_cpu_data.x86 >= 5) before reading the register.
> > If the value to be written is zero the write is skipped.
> >
> > arch/x86/power/hibernate_asm_32.S
> > done: swapped the use of %eax and %ecx to use jecxz for
> > the zero test and jump over store to %cr4.
> > restore_image: s/%ecx/%eax/ to be consistent with done:
> >
> > In addition to __save_processor_state, acpi_save_state_mem,
> > efi_call_phys_prelog, and efi_call_phys_epilog had checks added (acpi
> > restore was in assembly and already had a check for non-zero). There
> > were other reads and writes of CR4, but MCE and virtualization
> > shouldn't be executed on a i486 anyway.
> >
> > Signed-off-by: David Fries <[email protected]>
>
> applied to tip/x86/urgent, thanks David. I've changed the conditions to
> read_cr4_safe() instead - that's cleaner. Could you please check whether
> the patch below works fine too on your box?
>
> Rafael, Pavel - does the commit below look good to you too?
>
> Ingo
>
> ---------------------->
> From e437fa5586f2e3b2aaeba649fae52be1f9a6eabb Mon Sep 17 00:00:00 2001
> From: David Fries <[email protected]>
> Date: Sun, 17 Aug 2008 23:03:40 -0500
> Subject: [PATCH] x86: fix i486 suspend to disk CR4 oops
>
> arch/x86/power/cpu_32.c __save_processor_state calls read_cr4()
> only a i486 CPU doesn't have the CR4 register. Trying to read it
> produces an invalid opcode oops during suspend to disk.
>
> Use the safe rc4 reading op instead. If the value to be written is
> zero the write is skipped.
>
> arch/x86/power/hibernate_asm_32.S
> done: swapped the use of %eax and %ecx to use jecxz for
> the zero test and jump over store to %cr4.
> restore_image: s/%ecx/%eax/ to be consistent with done:
>
> In addition to __save_processor_state, acpi_save_state_mem,
> efi_call_phys_prelog, and efi_call_phys_epilog had checks added
> (acpi restore was in assembly and already had a check for
> non-zero). There were other reads and writes of CR4, but MCE and
> virtualization shouldn't be executed on a i486 anyway.
>
> Signed-off-by: David Fries <[email protected]>
> Signed-off-by: Ingo Molnar <[email protected]>
Acked-by: Rafael J. Wysocki <[email protected]>
> ---
> arch/x86/kernel/acpi/sleep.c | 2 +-
> arch/x86/kernel/efi_32.c | 4 ++--
> arch/x86/power/cpu_32.c | 6 ++++--
> arch/x86/power/hibernate_asm_32.S | 26 +++++++++++++++-----------
> 4 files changed, 22 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
> index 81e5ab6..426e5d9 100644
> --- a/arch/x86/kernel/acpi/sleep.c
> +++ b/arch/x86/kernel/acpi/sleep.c
> @@ -86,7 +86,7 @@ int acpi_save_state_mem(void)
> #endif /* !CONFIG_64BIT */
>
> header->pmode_cr0 = read_cr0();
> - header->pmode_cr4 = read_cr4();
> + header->pmode_cr4 = read_cr4_safe();
> header->realmode_flags = acpi_realmode_flags;
> header->real_magic = 0x12345678;
>
> diff --git a/arch/x86/kernel/efi_32.c b/arch/x86/kernel/efi_32.c
> index 4b63c8e..5cab48e 100644
> --- a/arch/x86/kernel/efi_32.c
> +++ b/arch/x86/kernel/efi_32.c
> @@ -53,7 +53,7 @@ void efi_call_phys_prelog(void)
> * directory. If I have PAE, I just need to duplicate one entry in
> * page directory.
> */
> - cr4 = read_cr4();
> + cr4 = read_cr4_safe();
>
> if (cr4 & X86_CR4_PAE) {
> efi_bak_pg_dir_pointer[0].pgd =
> @@ -91,7 +91,7 @@ void efi_call_phys_epilog(void)
> gdt_descr.size = GDT_SIZE - 1;
> load_gdt(&gdt_descr);
>
> - cr4 = read_cr4();
> + cr4 = read_cr4_safe();
>
> if (cr4 & X86_CR4_PAE) {
> swapper_pg_dir[pgd_index(0)].pgd =
> diff --git a/arch/x86/power/cpu_32.c b/arch/x86/power/cpu_32.c
> index 7dc5d5c..d3e083d 100644
> --- a/arch/x86/power/cpu_32.c
> +++ b/arch/x86/power/cpu_32.c
> @@ -45,7 +45,7 @@ static void __save_processor_state(struct saved_context *ctxt)
> ctxt->cr0 = read_cr0();
> ctxt->cr2 = read_cr2();
> ctxt->cr3 = read_cr3();
> - ctxt->cr4 = read_cr4();
> + ctxt->cr4 = read_cr4_safe();
> }
>
> /* Needed by apm.c */
> @@ -98,7 +98,9 @@ static void __restore_processor_state(struct saved_context *ctxt)
> /*
> * control registers
> */
> - write_cr4(ctxt->cr4);
> + /* cr4 was introduced in the Pentium CPU */
> + if (ctxt->cr4)
> + write_cr4(ctxt->cr4);
> write_cr3(ctxt->cr3);
> write_cr2(ctxt->cr2);
> write_cr0(ctxt->cr0);
> diff --git a/arch/x86/power/hibernate_asm_32.S b/arch/x86/power/hibernate_asm_32.S
> index b95aa6c..4fc7e87 100644
> --- a/arch/x86/power/hibernate_asm_32.S
> +++ b/arch/x86/power/hibernate_asm_32.S
> @@ -28,9 +28,9 @@ ENTRY(swsusp_arch_suspend)
> ret
>
> ENTRY(restore_image)
> - movl resume_pg_dir, %ecx
> - subl $__PAGE_OFFSET, %ecx
> - movl %ecx, %cr3
> + movl resume_pg_dir, %eax
> + subl $__PAGE_OFFSET, %eax
> + movl %eax, %cr3
>
> movl restore_pblist, %edx
> .p2align 4,,7
> @@ -52,17 +52,21 @@ copy_loop:
>
> done:
> /* go back to the original page tables */
> - movl $swapper_pg_dir, %ecx
> - subl $__PAGE_OFFSET, %ecx
> - movl %ecx, %cr3
> + movl $swapper_pg_dir, %eax
> + subl $__PAGE_OFFSET, %eax
> + movl %eax, %cr3
> /* Flush TLB, including "global" things (vmalloc) */
> - movl mmu_cr4_features, %eax
> - movl %eax, %edx
> + movl mmu_cr4_features, %ecx
> + jecxz 1f # cr4 Pentium and higher, skip if zero
> + movl %ecx, %edx
> andl $~(1<<7), %edx; # PGE
> movl %edx, %cr4; # turn off PGE
> - movl %cr3, %ecx; # flush TLB
> - movl %ecx, %cr3
> - movl %eax, %cr4; # turn PGE back on
> +1:
> + movl %cr3, %eax; # flush TLB
> + movl %eax, %cr3
> + jecxz 1f # cr4 Pentium and higher, skip if zero
> + movl %ecx, %cr4; # turn PGE back on
> +1:
>
> movl saved_context_esp, %esp
> movl saved_context_ebp, %ebp
>
>
On Mon, Aug 18, 2008 at 05:14:50AM +0100, Maciej W. Rozycki wrote:
> On Sun, 17 Aug 2008, David Fries wrote:
> > + /* cr4 was introduced in the Pentium CPU */
>
> NACK. Later i486 chips do have CR4 -- for PSE, VME, etc. (the set of
> features varies across the line). Use a fixup as elsewhere or something.
>
> Maciej
That's what I get for reading the Intel instruction set reference,
"The CR4 register was added to the Intel Architecture beginning with
the Pentium processor."
Ingo Molnar, thanks, I'll try the read_cr4_safe() version tonight (the
computer is in the trunk of my car and I'm about ready to head to
work).
In light of the above, how about updating the comments
- /* cr4 was introduced in the Pentium CPU */
- jecxz 1f # cr4 Pentium and higher, skip if zero
+ /* cr4 not in i386 only some i486, skip if zero */
+ jecxz 1f # cr4 not in i386 only some i486, skip if zero
I'm not being bit by arch/x86/kernel/relocate_kernel_32.S, but it is
using cr4. Should that be fixed up as well?
--
David Fries <[email protected]>
http://fries.net/~david/ (PGP encryption key available)
On Mon, Aug 18, 2008 at 05:14:50AM +0100, Maciej W. Rozycki wrote:
> On Sun, 17 Aug 2008, David Fries wrote:
> > + /* cr4 was introduced in the Pentium CPU */
>
> NACK. Later i486 chips do have CR4 -- for PSE, VME, etc. (the set of
> features varies across the line). Use a fixup as elsewhere or
> something.
the version i committed (reproduced below) should work fine. It uses cr4
opportunistically (we get a fault if it does not exist), and we write it
back if the cr4 value is non-zero. (which it must always be on a
CR4-enabled processor)
agreed?
Ingo
----------->
>From e532c06f2a835b5cc4f4166f467437d9b09c1d0e Mon Sep 17 00:00:00 2001
From: David Fries <[email protected]>
Date: Sun, 17 Aug 2008 23:03:40 -0500
Subject: [PATCH] x86: fix i486 suspend to disk CR4 oops
arch/x86/power/cpu_32.c __save_processor_state calls read_cr4()
only a i486 CPU doesn't have the CR4 register. Trying to read it
produces an invalid opcode oops during suspend to disk.
Use the safe rc4 reading op instead. If the value to be written is
zero the write is skipped.
arch/x86/power/hibernate_asm_32.S
done: swapped the use of %eax and %ecx to use jecxz for
the zero test and jump over store to %cr4.
restore_image: s/%ecx/%eax/ to be consistent with done:
In addition to __save_processor_state, acpi_save_state_mem,
efi_call_phys_prelog, and efi_call_phys_epilog had checks added
(acpi restore was in assembly and already had a check for
non-zero). There were other reads and writes of CR4, but MCE and
virtualization shouldn't be executed on a i486 anyway.
Signed-off-by: David Fries <[email protected]>
Acked-by: H. Peter Anvin <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/kernel/acpi/sleep.c | 2 +-
arch/x86/kernel/efi_32.c | 4 ++--
arch/x86/power/cpu_32.c | 6 ++++--
arch/x86/power/hibernate_asm_32.S | 26 +++++++++++++++-----------
4 files changed, 22 insertions(+), 16 deletions(-)
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 81e5ab6..426e5d9 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -86,7 +86,7 @@ int acpi_save_state_mem(void)
#endif /* !CONFIG_64BIT */
header->pmode_cr0 = read_cr0();
- header->pmode_cr4 = read_cr4();
+ header->pmode_cr4 = read_cr4_safe();
header->realmode_flags = acpi_realmode_flags;
header->real_magic = 0x12345678;
diff --git a/arch/x86/kernel/efi_32.c b/arch/x86/kernel/efi_32.c
index 4b63c8e..5cab48e 100644
--- a/arch/x86/kernel/efi_32.c
+++ b/arch/x86/kernel/efi_32.c
@@ -53,7 +53,7 @@ void efi_call_phys_prelog(void)
* directory. If I have PAE, I just need to duplicate one entry in
* page directory.
*/
- cr4 = read_cr4();
+ cr4 = read_cr4_safe();
if (cr4 & X86_CR4_PAE) {
efi_bak_pg_dir_pointer[0].pgd =
@@ -91,7 +91,7 @@ void efi_call_phys_epilog(void)
gdt_descr.size = GDT_SIZE - 1;
load_gdt(&gdt_descr);
- cr4 = read_cr4();
+ cr4 = read_cr4_safe();
if (cr4 & X86_CR4_PAE) {
swapper_pg_dir[pgd_index(0)].pgd =
diff --git a/arch/x86/power/cpu_32.c b/arch/x86/power/cpu_32.c
index 7dc5d5c..d3e083d 100644
--- a/arch/x86/power/cpu_32.c
+++ b/arch/x86/power/cpu_32.c
@@ -45,7 +45,7 @@ static void __save_processor_state(struct saved_context *ctxt)
ctxt->cr0 = read_cr0();
ctxt->cr2 = read_cr2();
ctxt->cr3 = read_cr3();
- ctxt->cr4 = read_cr4();
+ ctxt->cr4 = read_cr4_safe();
}
/* Needed by apm.c */
@@ -98,7 +98,9 @@ static void __restore_processor_state(struct saved_context *ctxt)
/*
* control registers
*/
- write_cr4(ctxt->cr4);
+ /* cr4 was introduced in the Pentium CPU */
+ if (ctxt->cr4)
+ write_cr4(ctxt->cr4);
write_cr3(ctxt->cr3);
write_cr2(ctxt->cr2);
write_cr0(ctxt->cr0);
diff --git a/arch/x86/power/hibernate_asm_32.S b/arch/x86/power/hibernate_asm_32.S
index b95aa6c..4fc7e87 100644
--- a/arch/x86/power/hibernate_asm_32.S
+++ b/arch/x86/power/hibernate_asm_32.S
@@ -28,9 +28,9 @@ ENTRY(swsusp_arch_suspend)
ret
ENTRY(restore_image)
- movl resume_pg_dir, %ecx
- subl $__PAGE_OFFSET, %ecx
- movl %ecx, %cr3
+ movl resume_pg_dir, %eax
+ subl $__PAGE_OFFSET, %eax
+ movl %eax, %cr3
movl restore_pblist, %edx
.p2align 4,,7
@@ -52,17 +52,21 @@ copy_loop:
done:
/* go back to the original page tables */
- movl $swapper_pg_dir, %ecx
- subl $__PAGE_OFFSET, %ecx
- movl %ecx, %cr3
+ movl $swapper_pg_dir, %eax
+ subl $__PAGE_OFFSET, %eax
+ movl %eax, %cr3
/* Flush TLB, including "global" things (vmalloc) */
- movl mmu_cr4_features, %eax
- movl %eax, %edx
+ movl mmu_cr4_features, %ecx
+ jecxz 1f # cr4 Pentium and higher, skip if zero
+ movl %ecx, %edx
andl $~(1<<7), %edx; # PGE
movl %edx, %cr4; # turn off PGE
- movl %cr3, %ecx; # flush TLB
- movl %ecx, %cr3
- movl %eax, %cr4; # turn PGE back on
+1:
+ movl %cr3, %eax; # flush TLB
+ movl %eax, %cr3
+ jecxz 1f # cr4 Pentium and higher, skip if zero
+ movl %ecx, %cr4; # turn PGE back on
+1:
movl saved_context_esp, %esp
movl saved_context_ebp, %ebp
On Mon, 18 Aug 2008, David Fries wrote:
> That's what I get for reading the Intel instruction set reference,
> "The CR4 register was added to the Intel Architecture beginning with
> the Pentium processor."
Indeed. It started with the Pentium and some of the i486
models/steppings that were released after the Pentium got the CR4 register
implemented too. These include at the least the write-back enhanced
versions of the iDX2 and iDX4 processors as well later steppings of the
i486SX, which I encountered myself. There could be others -- iSX2?
Unfortunately as usually with Intel you have to read all the documents
they release to hope to get a full image and even then you'll probably
come out confused and with bits of data missing -- if in doubt, check the
silicon: it has the most definite information. However these additions
were actually documented quite well in the respective i486-class processor
datasheets -- IIRC Intel have released a combined version at one point.
I am not sure whether it has ever been available on-line.
Maciej
On Mon, 18 Aug 2008, Ingo Molnar wrote:
> the version i committed (reproduced below) should work fine. It uses cr4
> opportunistically (we get a fault if it does not exist), and we write it
> back if the cr4 value is non-zero. (which it must always be on a
> CR4-enabled processor)
>
> agreed?
Acked-by: Maciej W. Rozycki <[email protected]>
Maciej
On Mon, Aug 18, 2008 at 08:41:20AM +0200, Ingo Molnar wrote:
> diff --git a/arch/x86/kernel/efi_32.c b/arch/x86/kernel/efi_32.c
> index 4b63c8e..5cab48e 100644
> --- a/arch/x86/kernel/efi_32.c
> +++ b/arch/x86/kernel/efi_32.c
> @@ -53,7 +53,7 @@ void efi_call_phys_prelog(void)
> * directory. If I have PAE, I just need to duplicate one entry in
> * page directory.
> */
> - cr4 = read_cr4();
> + cr4 = read_cr4_safe();
>
> if (cr4 & X86_CR4_PAE) {
> efi_bak_pg_dir_pointer[0].pgd =
> @@ -91,7 +91,7 @@ void efi_call_phys_epilog(void)
> gdt_descr.size = GDT_SIZE - 1;
> load_gdt(&gdt_descr);
>
> - cr4 = read_cr4();
> + cr4 = read_cr4_safe();
>
> if (cr4 & X86_CR4_PAE) {
> swapper_pg_dir[pgd_index(0)].pgd =
Is this part really necessary ?
Why would a 486 be in EFI code?
Dave
--
http://www.codemonkey.org.uk
On Mon, Aug 18, 2008 at 11:24:40AM -0400, Dave Jones wrote:
> Is this part really necessary ?
>
> Why would a 486 be in EFI code?
Someone has a hobby writing replacement firmware for their 486?
--
Len Sorensen
On Mon, Aug 18, 2008 at 12:04:26PM -0400, Lennart Sorensen wrote:
> On Mon, Aug 18, 2008 at 11:24:40AM -0400, Dave Jones wrote:
> > Is this part really necessary ?
> >
> > Why would a 486 be in EFI code?
>
> Someone has a hobby writing replacement firmware for their 486?
Anyone who writes EFI code for a hobby really needs to see
a mental health specialist.
Dave
--
http://www.codemonkey.org.uk
Dave Jones wrote:
>
> Is this part really necessary ?
>
> Why would a 486 be in EFI code?
>
Necessary, almost certainly not, but (a) it doesn't hurt anything, (b)
it's good to be consistent and (c) it might still be useful to virtualizers.
EFI is an abomination no matter how you look at it, of course.
-hpa
On Mon 2008-08-18 10:32:24, H. Peter Anvin wrote:
> Dave Jones wrote:
>>
>> Is this part really necessary ?
>>
>> Why would a 486 be in EFI code?
>>
>
> Necessary, almost certainly not, but (a) it doesn't hurt anything, (b) it's
> good to be consistent and (c) it might still be useful to virtualizers.
...it may be useful for something obscure... like system-on-chip...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Mon 2008-08-18 07:58:03, David Fries wrote:
> On Mon, Aug 18, 2008 at 05:14:50AM +0100, Maciej W. Rozycki wrote:
> > On Sun, 17 Aug 2008, David Fries wrote:
> > > + /* cr4 was introduced in the Pentium CPU */
> >
> > NACK. Later i486 chips do have CR4 -- for PSE, VME, etc. (the set of
> > features varies across the line). Use a fixup as elsewhere or something.
>
> That's what I get for reading the Intel instruction set reference,
> "The CR4 register was added to the Intel Architecture beginning with
> the Pentium processor."
>
> Ingo Molnar, thanks, I'll try the read_cr4_safe() version tonight (the
> computer is in the trunk of my car and I'm about ready to head to
> work).
>
> In light of the above, how about updating the comments
> - /* cr4 was introduced in the Pentium CPU */
> - jecxz 1f # cr4 Pentium and higher, skip if zero
> + /* cr4 not in i386 only some i486, skip if zero */
> + jecxz 1f # cr4 not in i386 only some i486, skip if zero
Okay, can it happen that that cr4 is zero legitimately? If newer 486SX
chips support cr4 but not coprocessor...?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Pavel Machek wrote:
>
> Okay, can it happen that that cr4 is zero legitimately? If newer 486SX
> chips support cr4 but not coprocessor...?
> Pavel
Theoretically it can, but that means no features are enabled, so there
is no need to enable the features.
The real question is if the following can happen: can it be such that we
want CR4 to be zero in a situation where CR4 is nonzero to start out with?
The main bit in CR4 that could be set that we wouldn't want set would be
CR4.PAE, so this could happen if there is a CPU with CR4.PAE but none of
the other CR4 bits that we would normally set unconditionally.
I'm pretty sure this can't happen on any physical CPUs, since all
physical CPUs supporting PAE would also support DE, MCE, and PGE. It
could possibly happen on a virtual CPU, although it is of course
extremely unlikely we'd get there with CR4 not zero to start out with.
Still, it is at least theoretically wrong.
-hpa
On Mon, Aug 18, 2008 at 08:41:20AM +0200, Ingo Molnar wrote:
>
> applied to tip/x86/urgent, thanks David. I've changed the conditions to
> read_cr4_safe() instead - that's cleaner. Could you please check whether
> the patch below works fine too on your box?
Yes the 486 suspends and resumes with this patch.
Is there any known problem with no_console_suspend and serial
consoles? It worked to print the oops for me to track this down, and
Documentation/kernel-parameters.txt says it is known to work with
serial consoles, but on resume only kernel messages can output to the
serial console. The getty on the serial port can't raed or write and
processes trying to write to the port just hang without getting any
data across. The serial port works fine across suspends without
the no_console_suspend argument. Does anyone else see this?
--
David Fries <[email protected]>
http://fries.net/~david/ (PGP encryption key available)
* David Fries <[email protected]> wrote:
> On Mon, Aug 18, 2008 at 08:41:20AM +0200, Ingo Molnar wrote:
> >
> > applied to tip/x86/urgent, thanks David. I've changed the conditions
> > to read_cr4_safe() instead - that's cleaner. Could you please check
> > whether the patch below works fine too on your box?
>
> Yes the 486 suspends and resumes with this patch.
good - it's now upstream and should show up in 2.6.27-rc4 as well.
> Is there any known problem with no_console_suspend and serial
> consoles? It worked to print the oops for me to track this down, and
> Documentation/kernel-parameters.txt says it is known to work with
> serial consoles, but on resume only kernel messages can output to the
> serial console. The getty on the serial port can't raed or write and
> processes trying to write to the port just hang without getting any
> data across. The serial port works fine across suspends without the
> no_console_suspend argument. Does anyone else see this?
i've had trouble no end with getting even kernel messages out to the
serial console during critical phases of suspend/resume. (especially in
combination with earlyprintk=ttyS0 - not surprisingly)
Especially during resume the UART is initialized back to something
really slow - 300 bauds or 9600 bauds. (depends on the chipset i guess)
So even though it works, it will only worked reliably when i
standardized all my baud settings to that very low setting.
Ingo
Ingo Molnar wrote:
>
> i've had trouble no end with getting even kernel messages out to the
> serial console during critical phases of suspend/resume. (especially in
> combination with earlyprintk=ttyS0 - not surprisingly)
>
> Especially during resume the UART is initialized back to something
> really slow - 300 bauds or 9600 bauds. (depends on the chipset i guess)
> So even though it works, it will only worked reliably when i
> standardized all my baud settings to that very low setting.
>
Probably depends on the BIOS, actually; I suspect you end up with the
standard BIOS setting, which is *usually* 9600 bps.
-hpa
On Tue, Aug 19, 2008 at 09:07:26AM -0700, H. Peter Anvin wrote:
> Ingo Molnar wrote:
> >
> >i've had trouble no end with getting even kernel messages out to the
> >serial console during critical phases of suspend/resume. (especially in
> >combination with earlyprintk=ttyS0 - not surprisingly)
> >
> >Especially during resume the UART is initialized back to something
> >really slow - 300 bauds or 9600 bauds. (depends on the chipset i guess)
> >So even though it works, it will only worked reliably when i
> >standardized all my baud settings to that very low setting.
> >
>
> Probably depends on the BIOS, actually; I suspect you end up with the
> standard BIOS setting, which is *usually* 9600 bps.
I have grub, the kernel, and a getty all set to 115200, for suspend,
resume, or reboot it's always 115200. With grub there's no BIOS
guessing, it sets it to a configured state.
--
David Fries <[email protected]>
http://fries.net/~david/ (PGP encryption key available)
David Fries wrote:
> On Tue, Aug 19, 2008 at 09:07:26AM -0700, H. Peter Anvin wrote:
>> Ingo Molnar wrote:
>>> i've had trouble no end with getting even kernel messages out to the
>>> serial console during critical phases of suspend/resume. (especially in
>>> combination with earlyprintk=ttyS0 - not surprisingly)
>>>
>>> Especially during resume the UART is initialized back to something
>>> really slow - 300 bauds or 9600 bauds. (depends on the chipset i guess)
>>> So even though it works, it will only worked reliably when i
>>> standardized all my baud settings to that very low setting.
>>>
>> Probably depends on the BIOS, actually; I suspect you end up with the
>> standard BIOS setting, which is *usually* 9600 bps.
>
> I have grub, the kernel, and a getty all set to 115200, for suspend,
> resume, or reboot it's always 115200. With grub there's no BIOS
> guessing, it sets it to a configured state.
>
Most likely it never gets reinitialized during resume. This is a bug,
obviously.
-hpa