merge 32/64 bits realmode wakeup code

RFC In review matthieu castet 3 participants 9 messages Jan 27, 2011 – Jan 30, 2011

Subsystems (3)

x86 ACPI Suspend to RAM

lore

Share & tools

lore

matthieu castet

author

2011-01-27 21:56 UTC #0 lore raw

RFC 1/1 merge 32/64 bits realmode wakeup code

Hi,

this patch merge 32 and 64 realmode wakeup code :
- this mean less ifdef in code
- we could remove now unused field in wakeup_header (pmode_*)
- this allow to set the first 1 MB NX.

Do you think it is safe on all X86 32 bit machine ?

Matthieu

Signed-off-by Matthieu CASTET <[email protected]>

arch/x86/Kconfig +1 -1

index 3ed5ad9..3baa11c 100644
@@ -219,7 +219,7 @@ config X86_HT
 
 config X86_TRAMPOLINE
 	def_bool y
-	depends on SMP || (64BIT && ACPI_SLEEP)
+	depends on SMP || ACPI_SLEEP
 
 config X86_32_LAZY_GS
 	def_bool y

arch/x86/kernel/acpi/realmode/wakeup.S +0 ~~-26~~

index 28595d6..2c2d5b8 100644
@@ -90,36 +90,10 @@ _start:
 
 	/* Do any other stuff... */
 
-#ifndef CONFIG_64BIT
-	/* This could also be done in C code... */
-	movl	pmode_cr3, %eax
-	movl	%eax, %cr3
-
-	movl	pmode_cr4, %ecx
-	jecxz	1f
-	movl	%ecx, %cr4
-1:
-	movl	pmode_efer, %eax
-	movl	pmode_efer + 4, %edx
-	movl	%eax, %ecx
-	orl	%edx, %ecx
-	jz	1f
-	movl	$MSR_EFER, %ecx
-	wrmsr
-1:
-
-	lgdtl	pmode_gdt
-
-	/* This really couldn't... */
-	movl	pmode_cr0, %eax
-	movl	%eax, %cr0
-	jmp	pmode_return
-#else
 	pushw	$0
 	pushw	trampoline_segment
 	pushw	$0
 	lret
-#endif
 
 bogus_real_magic:
 1:

arch/x86/kernel/acpi/sleep.c +8 ~~-15~~

index 69fd72a..d32bef9 100644
@@ -26,7 +26,7 @@ unsigned long acpi_realmode_flags;
 /* address in low memory of the wakeup routine. */
 static unsigned long acpi_realmode;
 
-#if defined(CONFIG_SMP) && defined(CONFIG_64BIT)
+#if defined(CONFIG_SMP)
 static char temp_stack[4096];
 #endif

@@ -80,31 +80,24 @@ int acpi_save_state_mem(void)
 	header->wakeup_gdt[2] =
 		GDT_ENTRY(0x8093, acpi_wakeup_address, 0xfffff);
 
-#ifndef CONFIG_64BIT
-	store_gdt((struct desc_ptr *)&header->pmode_gdt);
-
-	if (rdmsr_safe(MSR_EFER, &header->pmode_efer_low,
-		       &header->pmode_efer_high))
-		header->pmode_efer_low = header->pmode_efer_high = 0;
-#endif /* !CONFIG_64BIT */
-
 	header->pmode_cr0 = read_cr0();
 	header->pmode_cr4 = read_cr4_safe();
 	header->realmode_flags = acpi_realmode_flags;
 	header->real_magic = 0x12345678;
-
-#ifndef CONFIG_64BIT
-	header->pmode_entry = (u32)&wakeup_pmode_return;
-	header->pmode_cr3 = (u32)__pa(&initial_page_table);
-	saved_magic = 0x12345678;
-#else /* CONFIG_64BIT */
 	header->trampoline_segment = setup_trampoline() >> 4;
 #ifdef CONFIG_SMP
 	stack_start.sp = temp_stack + sizeof(temp_stack);
 	early_gdt_descr.address =
 			(unsigned long)get_cpu_gdt_table(smp_processor_id());
+#ifdef CONFIG_64BIT
 	initial_gs = per_cpu_offset(smp_processor_id());
 #endif
+#endif
+
+#ifndef CONFIG_64BIT
+	initial_code = (unsigned long)&wakeup_pmode_return;
+	saved_magic = 0x12345678;
+#else
 	initial_code = (unsigned long)wakeup_long64;
        saved_magic = 0x123456789abcdef0L;
 #endif /* CONFIG_64BIT */

arch/x86/kernel/trampoline_32.S +5 -1

index 8508237..11f0387 100644
@@ -32,8 +32,12 @@
 #include <asm/segment.h>
 #include <asm/page_types.h>
 
-/* We can free up trampoline after bootup if cpu hotplug is not supported. */
+#ifdef CONFIG_ACPI_SLEEP
+.section .rodata, "a", @progbits
+#else
+/* We can free up the trampoline after bootup if cpu hotplug is not supported. */
 __CPUINITRODATA
+#endif
 .code16
 
 ENTRY(trampoline_data)

Rafael J. Wysocki

2011-01-27 22:54 UTC #1 lore raw

Quoted 7 lines · matthieu castet

Hi,
this patch merge 32 and 64 realmode wakeup code :
- this mean less ifdef in code
- we could remove now unused field in wakeup_header (pmode_*)
- this allow to set the first 1 MB NX.

Quoted 6 lines

Do you think it is safe on all X86 32 bit machine ?
Matthieu
Signed-off-by Matthieu CASTET <[email protected]>

It _looks_ sane, but it will require some serious testing. I'm not sure, though, if we can get enough testing coverage without actually committing that change.

How much testing has it received so far?

[Patch appended below for the convenience of future reviewers. Really,
please don't attach patches if you want people to comment them. That's a major inconvenience. Please put them into the message body instead.]

Rafael

---

arch/x86/Kconfig +1 -1

index 3ed5ad9..3baa11c 100644
@@ -219,7 +219,7 @@ config X86_HT
 
 config X86_TRAMPOLINE
 	def_bool y
-	depends on SMP || (64BIT && ACPI_SLEEP)
+	depends on SMP || ACPI_SLEEP
 
 config X86_32_LAZY_GS
 	def_bool y

arch/x86/kernel/acpi/realmode/wakeup.S +0 ~~-26~~

index 28595d6..2c2d5b8 100644
@@ -90,36 +90,10 @@ _start:
 
 	/* Do any other stuff... */
 
-#ifndef CONFIG_64BIT
-	/* This could also be done in C code... */
-	movl	pmode_cr3, %eax
-	movl	%eax, %cr3
-
-	movl	pmode_cr4, %ecx
-	jecxz	1f
-	movl	%ecx, %cr4
-1:
-	movl	pmode_efer, %eax
-	movl	pmode_efer + 4, %edx
-	movl	%eax, %ecx
-	orl	%edx, %ecx
-	jz	1f
-	movl	$MSR_EFER, %ecx
-	wrmsr
-1:
-
-	lgdtl	pmode_gdt
-
-	/* This really couldn't... */
-	movl	pmode_cr0, %eax
-	movl	%eax, %cr0
-	jmp	pmode_return
-#else
 	pushw	$0
 	pushw	trampoline_segment
 	pushw	$0
 	lret
-#endif
 
 bogus_real_magic:
 1:

arch/x86/kernel/acpi/sleep.c +8 ~~-15~~

index 69fd72a..d32bef9 100644
@@ -26,7 +26,7 @@ unsigned long acpi_realmode_flags;
 /* address in low memory of the wakeup routine. */
 static unsigned long acpi_realmode;
 
-#if defined(CONFIG_SMP) && defined(CONFIG_64BIT)
+#if defined(CONFIG_SMP)
 static char temp_stack[4096];
 #endif

@@ -80,31 +80,24 @@ int acpi_save_state_mem(void)
 	header->wakeup_gdt[2] =
 		GDT_ENTRY(0x8093, acpi_wakeup_address, 0xfffff);
 
-#ifndef CONFIG_64BIT
-	store_gdt((struct desc_ptr *)&header->pmode_gdt);
-
-	if (rdmsr_safe(MSR_EFER, &header->pmode_efer_low,
-		       &header->pmode_efer_high))
-		header->pmode_efer_low = header->pmode_efer_high = 0;
-#endif /* !CONFIG_64BIT */
-
 	header->pmode_cr0 = read_cr0();
 	header->pmode_cr4 = read_cr4_safe();
 	header->realmode_flags = acpi_realmode_flags;
 	header->real_magic = 0x12345678;
-
-#ifndef CONFIG_64BIT
-	header->pmode_entry = (u32)&wakeup_pmode_return;
-	header->pmode_cr3 = (u32)__pa(&initial_page_table);
-	saved_magic = 0x12345678;
-#else /* CONFIG_64BIT */
 	header->trampoline_segment = setup_trampoline() >> 4;
 #ifdef CONFIG_SMP
 	stack_start.sp = temp_stack + sizeof(temp_stack);
 	early_gdt_descr.address =
 			(unsigned long)get_cpu_gdt_table(smp_processor_id());
+#ifdef CONFIG_64BIT
 	initial_gs = per_cpu_offset(smp_processor_id());
 #endif
+#endif
+
+#ifndef CONFIG_64BIT
+	initial_code = (unsigned long)&wakeup_pmode_return;
+	saved_magic = 0x12345678;
+#else
 	initial_code = (unsigned long)wakeup_long64;
        saved_magic = 0x123456789abcdef0L;
 #endif /* CONFIG_64BIT */

arch/x86/kernel/trampoline_32.S +5 -1

index 8508237..11f0387 100644
@@ -32,8 +32,12 @@
 #include <asm/segment.h>
 #include <asm/page_types.h>
 
-/* We can free up trampoline after bootup if cpu hotplug is not supported. */
+#ifdef CONFIG_ACPI_SLEEP
+.section .rodata, "a", @progbits
+#else
+/* We can free up the trampoline after bootup if cpu hotplug is not supported. */
 __CPUINITRODATA
+#endif
 .code16
 
 ENTRY(trampoline_data)

matthieu castet

author

2011-01-29 20:43 UTC #2 lore raw

Quoted 13 lines · Rafael J. Wysocki

Quoted 12 lines · matthieu castet
Hi,
this patch merge 32 and 64 realmode wakeup code :
- this mean less ifdef in code
- we could remove now unused field in wakeup_header (pmode_*)
- this allow to set the first 1 MB NX.
Do you think it is safe on all X86 32 bit machine ?
Matthieu

Quoted 8 lines

Quoted 1 line
Signed-off-by Matthieu CASTET <[email protected]>
It _looks_ sane, but it will require some serious testing. I'm not sure, though, if we can get enough testing coverage without actually committing that change.
How much testing has it received so far?

Not too much only qemu with 1 and 2 core and a laptop with 1 cpu.

Please note that with this patch we can also remove the gdt, idt, ldt stuff from arch/x86/kernel/acpi/wakeup_32.S

Matthieu
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Rafael J. Wysocki

2011-01-29 21:16 UTC #3 lore raw

Quoted 20 lines · matthieu castet

Quoted 19 lines · Rafael J. Wysocki
Quoted 7 lines · matthieu castet
Hi,
this patch merge 32 and 64 realmode wakeup code :
- this mean less ifdef in code
- we could remove now unused field in wakeup_header (pmode_*)
- this allow to set the first 1 MB NX.
Quoted 6 lines
Do you think it is safe on all X86 32 bit machine ?
Matthieu
Signed-off-by Matthieu CASTET <[email protected]>
It _looks_ sane, but it will require some serious testing. I'm not sure, though, if we can get enough testing coverage without actually committing that change.

Quoted 6 lines

Quoted 2 lines
How much testing has it received so far?
Not too much only qemu with 1 and 2 core and a laptop with 1 cpu.
Please note that with this patch we can also remove the gdt, idt, ldt stuff from arch/x86/kernel/acpi/wakeup_32.S

That's correct, but those changes can wait. The most urgent thing is to fix the resume regression at hand. I guess we should ask the reporter's in
https://bugzilla.kernel.org/show_bug.cgi?id=27472 to test that patch, right?

Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

H. Peter Anvin

reviewer

2011-01-30 00:19 UTC #4 lore raw

Quoted 5 lines · Rafael J. Wysocki

That's correct, but those changes can wait. The most urgent thing is to fix the resume regression at hand. I guess we should ask the reporter's in
https://bugzilla.kernel.org/show_bug.cgi?id=27472 to test that patch, right?

By the way, guys: I'm on vacation at the moment, but I do monitor this thread. I think at this point we should revert NX for the low 1 MiB and worry about doing it properly for .39. That being said, this kind of unification is a good thing in general.

-hpa

Rafael J. Wysocki

2011-01-30 13:00 UTC #5 lore raw

Quoted 10 lines · H. Peter Anvin

Quoted 5 lines · Rafael J. Wysocki
That's correct, but those changes can wait. The most urgent thing is to fix the resume regression at hand. I guess we should ask the reporter's in
https://bugzilla.kernel.org/show_bug.cgi?id=27472 to test that patch, right?
By the way, guys: I'm on vacation at the moment, but I do monitor this thread. I think at this point we should revert NX for the low 1 MiB and worry about doing it properly for .39.

Fine by me, this is your call anyway.

Quoted 1 line

That being said, this kind of unification is a good thing in general.

Sure.

Thanks,
Rafael

matthieu castet

author

2011-01-30 19:57 UTC #6 lore raw

In reply to H. Peter Anvin

Quoted 11 lines · H. Peter Anvin

Quoted 6 lines · Rafael J. Wysocki
That's correct, but those changes can wait. The most urgent thing is to fix the resume regression at hand. I guess we should ask the reporter's in https://bugzilla.kernel.org/show_bug.cgi?id=27472 to test that patch, right?
By the way, guys: I'm on vacation at the moment, but I do monitor this thread. I think at this point we should revert NX for the low 1 MiB and worry about doing it properly for .39.

So #9 patch in https://bugzilla.kernel.org/show_bug.cgi?id=27472 is okay, do you want some cleanup ?

Matthieu
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

matthieu castet

author

2011-01-30 20:04 UTC #7 lore raw

In reply to Rafael J. Wysocki

Quoted 7 lines · Rafael J. Wysocki

Quoted 6 lines · matthieu castet
Quoted 5 lines · Rafael J. Wysocki
It _looks_ sane, but it will require some serious testing. I'm not sure, though, if we can get enough testing coverage without actually committing that change.

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
How much testing has it received so far?
Not too much only qemu with 1 and 2 core and a laptop with 1 cpu.
Please note that with this patch we can also remove the gdt, idt, ldt stuff from arch/x86/kernel/acpi/wakeup_32.S
That's correct, but those changes can wait.

Any idea how to test this patch with more configurations ?

Submitting it to tip ?
But as resume regression showed, there aren't many user with 32 bits system using it.

Matthieu
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Rafael J. Wysocki

2011-01-30 22:42 UTC #8 lore raw

Quoted 17 lines · matthieu castet

Quoted 14 lines · Rafael J. Wysocki
Quoted 12 lines · matthieu castet
Quoted 7 lines · Rafael J. Wysocki
It _looks_ sane, but it will require some serious testing. I'm not sure, though, if we can get enough testing coverage without actually committing that change.
How much testing has it received so far?
Not too much only qemu with 1 and 2 core and a laptop with 1 cpu.
Please note that with this patch we can also remove the gdt, idt, ldt stuff from arch/x86/kernel/acpi/wakeup_32.S

Quoted 6 lines

Quoted 1 line
That's correct, but those changes can wait.
Any idea how to test this patch with more configurations ?
Submitting it to tip ?
But as resume regression showed, there aren't many user with 32 bits system using it.

No, I don't think there are too many users testing suspend with -tip on x86-32. I'm afraid the changes in question won't get any substantial testing until included into the mainline, but it will be easier to diagnose problems with them if they go in during the .39 merge window.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html