__restore_processor_state() fixes

__restore_processor_state() was spaghetti code, made no sense, and had bugs. And I broke resume on 32-bit systems. This series cleans it up and fixes it (hopefully!).

Andy Lutomirski (3):
x86/power/64: Use struct desc_ptr for the IDT in struct saved_context x86/power/32: Move SYSENTER MSR restoration to fix_processor_context() x86/power: Make restore_processor_context() sane

 arch/x86/include/asm/suspend_32.h |  8 +++-
 arch/x86/include/asm/suspend_64.h | 19 ++++++--
 arch/x86/power/cpu.c              | 99 ++++++++++++++++++---------------------
 3 files changed, 67 insertions(+), 59 deletions(-)

2.13.6

x86_64's saved_context nonsensically used separate idt_limit and idt_base fields and then cast &idt_limit to struct desc_ptr *. This was correct (with -fno-strict-aliasing), but it's confusing, served no purpose, and required ifdeffery. Just use struct desc_ptr directly.

---
 arch/x86/include/asm/suspend_64.h |  3 +--
 arch/x86/power/cpu.c              | 11 +----------
 2 files changed, 2 insertions(+), 12 deletions(-)

index 7306e911faee..600e9e0aea51 100644
@@ -30,8 +30,7 @@ struct saved_context {
 	u16 gdt_pad; /* Unused */
 	struct desc_ptr gdt_desc;
 	u16 idt_pad;
-	u16 idt_limit;
-	unsigned long idt_base;
+	struct desc_ptr idt;
 	u16 ldt;
 	u16 tss;
 	unsigned long tr;

index 5191de14f4df..472bc8c8212b 100644
@@ -82,12 +82,8 @@ static void __save_processor_state(struct saved_context *ctxt)
 	/*
 	 * descriptor tables
 	 */
-#ifdef CONFIG_X86_32
 	store_idt(&ctxt->idt);
-#else
-/* CONFIG_X86_64 */
-	store_idt((struct desc_ptr *)&ctxt->idt_limit);
-#endif
+
 	/*
 	 * We save it here, but restore it only in the hibernate case.
 	 * For ACPI S3 resume, this is loaded via 'early_gdt_desc' in 64-bit

@@ -219,12 +215,7 @@ static void notrace __restore_processor_state(struct saved_context *ctxt)
 	 * now restore the descriptor tables to their proper values
 	 * ltr is done i fix_processor_context().
 	 */
-#ifdef CONFIG_X86_32
 	load_idt(&ctxt->idt);
-#else
-/* CONFIG_X86_64 */
-	load_idt((const struct desc_ptr *)&ctxt->idt_limit);
-#endif
 
 #ifdef CONFIG_X86_64
 	/*

2.13.6

x86_64 restores system call MSRs in fix_processor_context(), and x86_32 restored them along with segment registers. The 64-bit variant makes more sense, so move the 32-bit code to match the 64-bit code.

---
 arch/x86/power/cpu.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

index 472bc8c8212b..033c61e6891b 100644
@@ -174,6 +174,9 @@ static void fix_processor_context(void)
 	write_gdt_entry(desc, GDT_ENTRY_TSS, &tss, DESC_TSS);
 
 	syscall_init();				/* This sets MSR_*STAR and related */
+#else
+	if (boot_cpu_has(X86_FEATURE_SEP))
+		enable_sep_cpu();
 #endif
 	load_TR_desc();				/* This does ltr */
 	load_mm_ldt(current->active_mm);	/* This does lldt */

@@ -237,12 +240,6 @@ static void notrace __restore_processor_state(struct saved_context *ctxt)
 	loadsegment(fs, ctxt->fs);
 	loadsegment(gs, ctxt->gs);
 	loadsegment(ss, ctxt->ss);
-
-	/*
-	 * sysenter MSRs
-	 */
-	if (boot_cpu_has(X86_FEATURE_SEP))
-		enable_sep_cpu();
 #else
 /* CONFIG_X86_64 */
 	asm volatile ("movw %0, %%ds" :: "r" (ctxt->ds));

2.13.6

My previous attempt to fix a couple of bugs in __restore_processor_context() introduced yet another bug. Rather than trying to come up with a minimal fix, let's try to clean it up for real. This patch fixes quite a few things:

- The old code saved a nonsensical subset of segment registers. The only registers that need to be saved are those that contain userspace state or those that can't be trivially restored without percpu access working. (On x86_32, we can restore percpu access by writing __KERNEL_PERCPU to %fs. On x86_64, it's easier to save and restore the kernel's GSBASE.) With this patch, we restore hardcoded values to the kernel state where applicable and explicitly restore the user state after fixing all the descriptor tables.

- We used to use an unholy mix of inline asm and C helpers for segment register access. Let's get rid of the inline asm.

---
 arch/x86/include/asm/suspend_32.h |  8 +++-
 arch/x86/include/asm/suspend_64.h | 16 +++++++-
 arch/x86/power/cpu.c              | 79 ++++++++++++++++++++-------------------
 3 files changed, 62 insertions(+), 41 deletions(-)

index 982c325dad33..8be6afb58471 100644
@@ -12,7 +12,13 @@
 
 /* image of the saved processor state */
 struct saved_context {
-	u16 es, fs, gs, ss;
+	/*
+	 * On x86_32, all segment registers, with the possible exception of
+	 * gs, are saved at kernel entry in pt_regs.
+	 */
+#ifdef CONFIG_X86_32_LAZY_GS
+	u16 gs;
+#endif
 	unsigned long cr0, cr2, cr3, cr4;
 	u64 misc_enable;
 	bool misc_enable_saved;

index 600e9e0aea51..a7af9f53c0cb 100644
@@ -20,8 +20,20 @@
  */
 struct saved_context {
 	struct pt_regs regs;
-	u16 ds, es, fs, gs, ss;
-	unsigned long gs_base, gs_kernel_base, fs_base;
+
+	/*
+	 * User CS and SS are saved in current_pt_regs().  The rest of the
+	 * segment selectors need to be saved and restored here.
+	 */
+	u16 ds, es, fs, gs;
+
+	/*
+	 * Usermode FSBASE and GSBASE may not match the fs and gs selectors,
+	 * so we save them separately.  We save the kernelmode GSBASE to
+	 * restore percpu access after resume.
+	 */
+	unsigned long kernelmode_gs_base, usermode_gs_base, fs_base;
+
 	unsigned long cr0, cr2, cr3, cr4, cr8;
 	u64 misc_enable;
 	bool misc_enable_saved;

2.13.6

Hi!

Thanks!

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

For all three:

Thanks!

And I'm assuming I'll get these from the x86 -tip tree.

If not, somebody please holler, and I'll pick them up directly.

Linus

Works still after split up so my tested by remains. Please note Pavel gave his reported/tested-by in 3/3.

Jarkko

I have applied them to tip:x86/urgent with small changes to the changelogs, and will send them to you in a couple of hours.

These fixes are really nice, thanks guys for all the effort!

Thanks,

Ingo

Thanks, I've updated all the changelogs - and I've also added your Reported-by tag to patch #3.

Thanks,

Ingo