Skip to contentSkip to conversation

swsusp critical code rewritten to assembly

PATCH Pavel Machek 2 participants 3 messages Nov 10, 2002

lore
Share & tools
lore
#0 lore raw

Hi!

do_magic was really too fragile to be written in C. This patch rewrites it into assembly, to make sure C compiler does not generate stack access and corrupt memory that way. Plus it cleans up suspend.c a bit, makes it really free memory it needs, an no longer calls drivers from atomic context.

Please apply,
Pavel

arch/i386/kernel/Makefile +1 -1
@@ -25,7 +25,7 @@
 obj-$(CONFIG_X86_MPPARSE)	+= mpparse.o
 obj-$(CONFIG_X86_LOCAL_APIC)	+= apic.o nmi.o
 obj-$(CONFIG_X86_IO_APIC)	+= io_apic.o
-obj-$(CONFIG_SOFTWARE_SUSPEND)	+= suspend.o
+obj-$(CONFIG_SOFTWARE_SUSPEND)	+= suspend.o suspend_asm.o
 obj-$(CONFIG_X86_NUMAQ)		+= numaq.o
 obj-$(CONFIG_PROFILING)		+= profile.o
 obj-$(CONFIG_EDD)             	+= edd.o
arch/i386/kernel/suspend.c +123 -106

287 lines not on this page · raw message

arch/i386/kernel/suspend_asm.S +87 -0

87 lines not on this page · raw message

include/asm-i386/suspend.h +6 -3
@@ -15,8 +15,6 @@
 
 /* image of the saved processor state */
 struct saved_context {
-	unsigned long eax, ebx, ecx, edx;
-	unsigned long esp, ebp, esi, edi;
   	u16 es, fs, gs, ss;
 	unsigned long cr0, cr2, cr3, cr4;
 	u16 gdt_pad;
@@ -30,9 +28,14 @@
 	unsigned long tr;
 	unsigned long safety;
 	unsigned long return_address;
-	unsigned long eflags;
 } __attribute__((packed));
 
+/* We'll access these from assembly, so we'd better have them outside struct */
+extern unsigned long saved_context_eax, saved_context_ebx, saved_context_ecx, saved_context_edx;
+extern unsigned long saved_context_esp, saved_context_ebp, saved_context_esi, saved_context_edi;
+extern unsigned long saved_context_eflags;
+
+
 #define loaddebug(thread,register) \
                __asm__("movl %0,%%db" #register  \
                        : /* no output */ \
kernel/suspend.c +18 -11
@@ -235,6 +235,7 @@
 	} while(todo);
 	
 	printk( "|\n" );
+	BUG_ON(in_atomic());
 	return 0;
 }
 
@@ -489,7 +490,6 @@
 			if (PageNosave(page))
 				continue;
 
-
 			if ((chunk_size=is_head_of_free_region(page))!=0) {
 				pfn += chunk_size - 1;
 				continue;
@@ -500,9 +500,8 @@
 			/*
 			 * Just copy whole code segment. Hopefully it is not that big.
 			 */
-			if (ADDRESS(pfn) >= (unsigned long)
-				&__nosave_begin && ADDRESS(pfn) < 
-				(unsigned long)&__nosave_end) {
+			if ((ADDRESS(pfn) >= (unsigned long) &__nosave_begin) && 
+			    (ADDRESS(pfn) <  (unsigned long) &__nosave_end)) {
 				PRINTK("[nosave %x]", ADDRESS(pfn));
 				continue;
 			}
@@ -623,7 +623,7 @@
 static void free_some_memory(void)
 {
 	printk("Freeing memory: ");
-	while (try_to_free_pages(&contig_page_data.node_zones[ZONE_HIGHMEM], GFP_KSWAPD, 0))
+	while (shrink_all_memory(10000))
 		printk(".");
 	printk("|\n");
 }
@@ -676,7 +676,7 @@
 	}
 }
 
-static int suspend_save_image(void)
+static int suspend_prepare_image(void)
 {
 	struct sysinfo i;
 	unsigned int nr_needed_pages = 0;
@@ -725,9 +725,15 @@
 	 *
 	 * Following line enforces not writing to disk until we choose.
 	 */
-	drivers_unsuspend();
-	spin_unlock_irq(&suspend_pagedir_lock);
+
 	printk( "critical section/: done (%d pages copied)\n", nr_copy_pages );
+	spin_unlock_irq(&suspend_pagedir_lock);
+	return 0;
+}
+
+void suspend_save_image(void)
+{
+	drivers_unsuspend();
 
 	lock_swapdevices();
 	write_suspend_image();
@@ -738,7 +744,6 @@
 	 * filesystem clean: it is not. (And it does not matter, if we resume
 	 * correctly, we'll mark system clean, anyway.)
 	 */
-	return 0;
 }
 
 void suspend_power_down(void)
@@ -788,8 +793,8 @@
 
 	PRINTK( "Freeing prev allocated pagedir\n" );
 	free_suspend_pagedir((unsigned long) pagedir_save);
-	drivers_resume(RESUME_ALL_PHASES);
 	spin_unlock_irq(&suspend_pagedir_lock);
+	drivers_resume(RESUME_ALL_PHASES);
 
 	PRINTK( "Fixing swap signatures... " );
 	mark_swapfiles(((swp_entry_t) {0}), MARK_SWAP_RESUME);
@@ -804,14 +809,17 @@
 {
 	mb();
 	barrier();
+	BUG_ON(in_atomic());
 	spin_lock_irq(&suspend_pagedir_lock);
 }
 
 void do_magic_suspend_2(void)
 {
 	read_swapfiles();
-	if (!suspend_save_image())
+	if (!suspend_prepare_image()) {	/* suspend_save_image realeses suspend_pagedir_lock */
+		suspend_save_image();
 		suspend_power_down();	/* FIXME: if suspend_power_down is commented out, console is lost after few suspends ?! */
+	}
 
 	printk(KERN_EMERG "%sSuspend failed, trying to recover...\n", name_suspend);
 	MDELAY(1000); /* So user can wait and report us messages if armageddon comes :-) */
Signature · 2 lines
Worst form of spam? Adding advertisment signatures ala sourceforge.net.
What goes next? Inserting advertisment *into* email?
#1 lore raw
Quoted 6 lines · Pavel Machek

do_magic was really too fragile to be written in C. This patch rewrites it into assembly, to make sure C compiler does not generate stack access and corrupt memory that way. Plus it cleans up suspend.c a bit, makes it really free memory it needs, an no longer calls drivers from atomic context.

But this still has stuff in C:

arch/i386/kernel/suspend.c line 213

Quoted 11 lines
arch/i386/kernel/suspend.c +9 -1
+	asm volatile ("movl %0, %%esp" :: "m" (saved_context_esp));
+	asm volatile ("movl %0, %%ebp" :: "m" (saved_context_ebp));
+	asm volatile ("movl %0, %%eax" :: "m" (saved_context_eax));
+	asm volatile ("movl %0, %%ebx" :: "m" (saved_context_ebx));
+	asm volatile ("movl %0, %%ecx" :: "m" (saved_context_ecx));
+	asm volatile ("movl %0, %%edx" :: "m" (saved_context_edx));
+	asm volatile ("movl %0, %%esi" :: "m" (saved_context_esi));
+	asm volatile ("movl %0, %%edi" :: "m" (saved_context_edi));
 
-	fix_processor_context();
+	restore_processor_state();

What's up with that? There's no way you can safely restore regular registers and _especially_ %%esp from C code, since the compiler may be using them for other things.

Linus

#2 lore raw

Hi!

Quoted 17 lines
Quoted 5 lines

do_magic was really too fragile to be written in C. This patch rewrites it into assembly, to make sure C compiler does not generate stack access and corrupt memory that way. Plus it cleans up suspend.c a bit, makes it really free memory it needs, an no longer calls drivers from atomic context.

But this still has stuff in C:

Quoted 9 lines
arch/i386/kernel/suspend.c +8 -0
+	asm volatile ("movl %0, %%esp" :: "m" (saved_context_esp));
+	asm volatile ("movl %0, %%ebp" :: "m" (saved_context_ebp));
+	asm volatile ("movl %0, %%eax" :: "m" (saved_context_eax));
+	asm volatile ("movl %0, %%ebx" :: "m" (saved_context_ebx));
+	asm volatile ("movl %0, %%ecx" :: "m" (saved_context_ecx));
+	asm volatile ("movl %0, %%edx" :: "m" (saved_context_edx));
+	asm volatile ("movl %0, %%esi" :: "m" (saved_context_esi));
+	asm volatile ("movl %0, %%edi" :: "m" (saved_context_edi));
Quoted 6 lines

arch/i386/kernel/suspend.c line 213

Quoted 2 lines
arch/i386/kernel/suspend.c +1 -1
-	fix_processor_context();
+	restore_processor_state();

What's up with that? There's no way you can safely restore regular registers and _especially_ %%esp from C code, since the compiler may be using them for other things.

After applying that patch, this stuff is no longer used for suspend-to-disk. Its still there for suspend-to-ram; I'll fix that.

Pavel

Signature · 2 lines
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.