Re: [PATCH] restore sysenter MSRs at resume

Re: [PATCH] restore sysenter MSRs at resume

[mikpe]

On Wed, 7 May 2003 10:39:35 -0700 (PDT), Linus Torvalds wrote:
>On Wed, 7 May 2003 mikpe@csd.uu.se wrote:
>> I could probably get away with simply having apm.c invoke the C code
>> in suspend.c, which does restore the SYSENTER MSRs. suspend.c itself
>> doesn't seem to depend on the SOFTWARE_SUSPEND machinery, but
>> suspend_asm.S does.
>> 
>> Does that sound reasonable?
>
>Sounds reasonable to me. In fact, it looks like it really already exists
>as the current "restore_processor_state()" thing.
>
>In fact, that one already _does_ call "enable_sep_cpu()", so what's up?

This patch should be better. It changes apm.c to invoke
suspend.c's save and restore processor state procedures
around suspends, which fixes the SYSENTER MSR problem.

The patch also decouples sysenter.c from SOFTWARE_SUSPEND:
the variables used (only!) in suspend_asm.S are moved there,
and the include file now declares the procedures called from
apm.c (previously they were only called from suspend_asm.S).

/Mikael

--- linux-2.5.69/arch/i386/kernel/Makefile.~1~	2003-05-05 22:56:28.000000000 +0200
+++ linux-2.5.69/arch/i386/kernel/Makefile	2003-05-10 16:24:56.710471008 +0200
@@ -17,7 +17,7 @@
 obj-$(CONFIG_X86_MSR)		+= msr.o
 obj-$(CONFIG_X86_CPUID)		+= cpuid.o
 obj-$(CONFIG_MICROCODE)		+= microcode.o
-obj-$(CONFIG_APM)		+= apm.o
+obj-$(CONFIG_APM)		+= apm.o suspend.o
 obj-$(CONFIG_X86_SMP)		+= smp.o smpboot.o
 obj-$(CONFIG_X86_TRAMPOLINE)	+= trampoline.o
 obj-$(CONFIG_X86_MPPARSE)	+= mpparse.o
--- linux-2.5.69/arch/i386/kernel/apm.c.~1~	2003-05-05 22:56:28.000000000 +0200
+++ linux-2.5.69/arch/i386/kernel/apm.c	2003-05-10 16:27:40.556562616 +0200
@@ -226,6 +226,7 @@
 #include <asm/system.h>
 #include <asm/uaccess.h>
 #include <asm/desc.h>
+#include <asm/suspend.h>
 
 #include "io_ports.h"
 
@@ -1212,7 +1213,9 @@
 	spin_unlock(&i8253_lock);
 	write_sequnlock_irq(&xtime_lock);
 
+	save_processor_state();
 	err = set_system_power_state(APM_STATE_SUSPEND);
+	restore_processor_state();
 
 	write_seqlock_irq(&xtime_lock);
 	spin_lock(&i8253_lock);
--- linux-2.5.69/arch/i386/kernel/suspend.c.~1~	2003-04-20 13:08:15.000000000 +0200
+++ linux-2.5.69/arch/i386/kernel/suspend.c	2003-05-10 16:24:02.261748472 +0200
@@ -27,9 +27,7 @@
 #include <asm/tlbflush.h>
 
 static struct saved_context saved_context;
-unsigned long saved_context_eax, saved_context_ebx, saved_context_ecx, saved_context_edx;
-unsigned long saved_context_esp, saved_context_ebp, saved_context_esi, saved_context_edi;
-unsigned long saved_context_eflags;
+static void fix_processor_context(void);
 
 extern void enable_sep_cpu(void *);
 
@@ -107,7 +105,7 @@
 	do_fpu_end();
 }
 
-void fix_processor_context(void)
+static void fix_processor_context(void)
 {
 	int cpu = smp_processor_id();
 	struct tss_struct * t = init_tss + cpu;
--- linux-2.5.69/arch/i386/kernel/suspend_asm.S.~1~	2003-01-02 14:27:54.000000000 +0100
+++ linux-2.5.69/arch/i386/kernel/suspend_asm.S	2003-05-10 16:19:58.000000000 +0200
@@ -6,6 +6,28 @@
 #include <asm/segment.h>
 #include <asm/page.h>
 
+	.data
+saved_context_eax:
+	.long	0
+saved_context_ebx:
+	.long	0
+saved_context_ecx:
+	.long	0
+saved_context_edx:
+	.long	0
+saved_context_esp:
+	.long	0
+saved_context_ebp:
+	.long	0
+saved_context_esi:
+	.long	0
+saved_context_edi:
+	.long	0
+saved_context_eflags:
+	.long	0
+
+	.text
+
 ENTRY(do_magic)
 	pushl %ebx
 	cmpl $0,8(%esp)
--- linux-2.5.69/include/asm-i386/suspend.h.~1~	2002-11-23 17:59:41.000000000 +0100
+++ linux-2.5.69/include/asm-i386/suspend.h	2003-05-10 16:23:26.257221992 +0200
@@ -30,18 +30,14 @@
 	unsigned long return_address;
 } __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 */ \
                        :"r" ((thread)->debugreg[register]))
 
-extern void fix_processor_context(void);
+extern void save_processor_state(void);
+extern void restore_processor_state(void);
+
 extern void do_magic(int resume);
 
 #ifdef CONFIG_ACPI_SLEEP

Re: [PATCH] restore sysenter MSRs at resume

[Linus Torvalds]

On Sat, 10 May 2003 mikpe@csd.uu.se wrote:
> 
> This patch should be better. It changes apm.c to invoke
> suspend.c's save and restore processor state procedures
> around suspends, which fixes the SYSENTER MSR problem.

Applied.

However, the fact that the SYSENTER MSR needs to be restored makes me
suspect that the other MSR/MTRR also will need restoring. I don't see 
where we'd be doing that, but it sounds to me like it should be done here 
too..

		Linus

Re: [PATCH] restore sysenter MSRs at resume

[Pavel Machek]

Hi!

Nigel, perhaps this is the right time for retransmitting the mtrr
patch?

					Pavel
> 
> On Sat, 10 May 2003 mikpe@csd.uu.se wrote:
> > 
> > This patch should be better. It changes apm.c to invoke
> > suspend.c's save and restore processor state procedures
> > around suspends, which fixes the SYSENTER MSR problem.
> 
> Applied.
> 
> However, the fact that the SYSENTER MSR needs to be restored makes me
> suspect that the other MSR/MTRR also will need restoring. I don't see 
> where we'd be doing that, but it sounds to me like it should be done here 
> too..
> 
> 		Linus

-- 
Horseback riding is like software...
...vgf orggre jura vgf serr.

Re: [PATCH] restore sysenter MSRs at resume

[Nigel Cunningham]

Ok. I haven't updated it for 2.5.69 version, but it doesn't look like
any changes are required. Here is the relevant part of the full swsusp
patch.

Regards,

Nigel

On Mon, 2003-05-12 at 07:08, Pavel Machek wrote:
> Hi!
> 
> Nigel, perhaps this is the right time for retransmitting the mtrr
> patch?
> 
> 					Pavel
> > 
> > On Sat, 10 May 2003 mikpe@csd.uu.se wrote:
> > > 
> > > This patch should be better. It changes apm.c to invoke
> > > suspend.c's save and restore processor state procedures
> > > around suspends, which fixes the SYSENTER MSR problem.
> > 
> > Applied.
> > 
> > However, the fact that the SYSENTER MSR needs to be restored makes me
> > suspect that the other MSR/MTRR also will need restoring. I don't see 
> > where we'd be doing that, but it sounds to me like it should be done here 
> > too..
> > 
> > 		Linus

diff -ruN linux-2.5.68/arch/i386/kernel/cpu/mtrr/main.c linux-2.5.68-swsusp1925/arch/i386/kernel/cpu/mtrr/main.c
--- linux-2.5.68/arch/i386/kernel/cpu/mtrr/main.c	2003-01-15 17:00:38.000000000 +1300
+++ linux-2.5.68-swsusp1925/arch/i386/kernel/cpu/mtrr/main.c	2003-04-25 14:13:05.000000000 +1200
@@ -35,6 +35,7 @@
 #include <linux/init.h>
 #include <linux/pci.h>
 #include <linux/smp.h>
+#include <linux/suspend.h>
 
 #include <asm/mtrr.h>
 
@@ -644,6 +645,65 @@
     "write-protect",            /* 5 */
     "write-back",               /* 6 */
 };
-
+ 
+#ifdef SOFTWARE_SUSPEND_MTRR
+struct mtrr_suspend_state
+{
+     mtrr_type ltype;
+     unsigned long lbase;
+     unsigned int lsize;
+};
+/* We return a pointer ptr on an area of *ptr bytes
+   beginning at ptr+sizeof(int)
+   This buffer has to be saved in some way during suspension */
+int *mtrr_suspend(void)
+{
+     int i, len;
+     int *ptr = NULL;
+     static struct mtrr_suspend_state *mtrr_suspend_buffer=NULL;
+     
+     if(!mtrr_suspend_buffer)
+     {
+	  len = num_var_ranges * sizeof (struct mtrr_suspend_state) + sizeof(int);
+	  ptr = kmalloc (len, GFP_KERNEL);
+	  if (ptr == NULL)
+	       return(NULL);
+	  *ptr = len;
+	  ptr++;
+	  mtrr_suspend_buffer = (struct mtrr_suspend_state *)ptr;
+	  ptr--;
+     }
+     for (i = 0; i < num_var_ranges; ++i,mtrr_suspend_buffer++)
+	  mtrr_if->get (i,
+		       &(mtrr_suspend_buffer->lbase),
+		       &(mtrr_suspend_buffer->lsize),
+		       &(mtrr_suspend_buffer->ltype));
+     return(ptr);
+}
+
+/* We restore mtrrs from buffer ptr */
+void mtrr_resume(int *ptr)
+{
+     int i, len;
+     struct mtrr_suspend_state *mtrr_suspend_buffer;
+     
+     len = num_var_ranges * sizeof (struct mtrr_suspend_state) + sizeof(int);
+     if(*ptr != len)
+     {
+	  printk ("mtrr: Resuming failed due to different number of MTRRs\n");
+	  return;
+     }
+     ptr++;
+     mtrr_suspend_buffer=(struct mtrr_suspend_state *)ptr;
+     for (i = 0; i < num_var_ranges; ++i,mtrr_suspend_buffer++)     
+	  if (mtrr_suspend_buffer->lsize)	  
+	       set_mtrr(i,
+			mtrr_suspend_buffer->lbase,
+			mtrr_suspend_buffer->lsize,
+			mtrr_suspend_buffer->ltype);
+}
+EXPORT_SYMBOL(mtrr_suspend);
+EXPORT_SYMBOL(mtrr_resume);
+#endif
 core_initcall(mtrr_init);
 

Re: [PATCH] restore sysenter MSRs at resume

[Alan Cox]

On Sul, 2003-05-11 at 20:01, Linus Torvalds wrote:
> However, the fact that the SYSENTER MSR needs to be restored makes me
> suspect that the other MSR/MTRR also will need restoring. I don't see 
> where we'd be doing that, but it sounds to me like it should be done here 
> too..

Some laptops certainly require the MTRR restore to happen. MSRs are
mostly less of a problem because the profiling stuff is broken on PIII
unless you disable/re-enable it across power save anyway.

Some laptops also lose all the AGP settings in the chipset.

Re: [PATCH] restore sysenter MSRs at resume

[Linus Torvalds]

On 11 May 2003, Alan Cox wrote:
> 
> Some laptops also lose all the AGP settings in the chipset.

Well, that's definitely a driver issue, and should be handled that way. I
suspect even the MTRR's should be handled as a driver, since unlike things
like the SYSENTER things, it really _is_ a driver already and is
conditional on kernel configuration etc.

		Linus

Re: [PATCH] restore sysenter MSRs at resume

[Alan Cox]

On Llu, 2003-05-12 at 01:07, Linus Torvalds wrote:
> On 11 May 2003, Alan Cox wrote:
> > 
> > Some laptops also lose all the AGP settings in the chipset.
> 
> Well, that's definitely a driver issue, and should be handled that way. I
> suspect even the MTRR's should be handled as a driver, since unlike things
> like the SYSENTER things, it really _is_ a driver already and is
> conditional on kernel configuration etc.

True, although mtrr is kind of special because bad things happen in some
cases if they dont match across CPU's or with I/O device mappings.

Re: [PATCH] restore sysenter MSRs at resume

[Pavel Machek]

Hi!

> Ok. I haven't updated it for 2.5.69 version, but it doesn't look like
> any changes are required. Here is the relevant part of the full swsusp
> patch.

I guess it still needs to be updated for the driver model....


> Regards,
> 
> Nigel
> 
> On Mon, 2003-05-12 at 07:08, Pavel Machek wrote:
> > Hi!
> > 
> > Nigel, perhaps this is the right time for retransmitting the mtrr
> > patch?
> > 
> > 					Pavel
> > > 
> > > On Sat, 10 May 2003 mikpe@csd.uu.se wrote:
> > > > 
> > > > This patch should be better. It changes apm.c to invoke
> > > > suspend.c's save and restore processor state procedures
> > > > around suspends, which fixes the SYSENTER MSR problem.
> > > 
> > > Applied.
> > > 
> > > However, the fact that the SYSENTER MSR needs to be restored makes me
> > > suspect that the other MSR/MTRR also will need restoring. I don't see 
> > > where we'd be doing that, but it sounds to me like it should be done here 
> > > too..
> > > 
> > > 		Linus
> 
> diff -ruN linux-2.5.68/arch/i386/kernel/cpu/mtrr/main.c linux-2.5.68-swsusp1925/arch/i386/kernel/cpu/mtrr/main.c
> --- linux-2.5.68/arch/i386/kernel/cpu/mtrr/main.c	2003-01-15 17:00:38.000000000 +1300
> +++ linux-2.5.68-swsusp1925/arch/i386/kernel/cpu/mtrr/main.c	2003-04-25 14:13:05.000000000 +1200
> @@ -35,6 +35,7 @@
>  #include <linux/init.h>
>  #include <linux/pci.h>
>  #include <linux/smp.h>
> +#include <linux/suspend.h>
>  
>  #include <asm/mtrr.h>
>  
> @@ -644,6 +645,65 @@
>      "write-protect",            /* 5 */
>      "write-back",               /* 6 */
>  };
> -
> + 
> +#ifdef SOFTWARE_SUSPEND_MTRR
> +struct mtrr_suspend_state
> +{
> +     mtrr_type ltype;
> +     unsigned long lbase;
> +     unsigned int lsize;
> +};
> +/* We return a pointer ptr on an area of *ptr bytes
> +   beginning at ptr+sizeof(int)
> +   This buffer has to be saved in some way during suspension */
> +int *mtrr_suspend(void)
> +{
> +     int i, len;
> +     int *ptr = NULL;
> +     static struct mtrr_suspend_state *mtrr_suspend_buffer=NULL;
> +     
> +     if(!mtrr_suspend_buffer)
> +     {
> +	  len = num_var_ranges * sizeof (struct mtrr_suspend_state) + sizeof(int);
> +	  ptr = kmalloc (len, GFP_KERNEL);
> +	  if (ptr == NULL)
> +	       return(NULL);
> +	  *ptr = len;
> +	  ptr++;
> +	  mtrr_suspend_buffer = (struct mtrr_suspend_state *)ptr;
> +	  ptr--;
> +     }
> +     for (i = 0; i < num_var_ranges; ++i,mtrr_suspend_buffer++)
> +	  mtrr_if->get (i,
> +		       &(mtrr_suspend_buffer->lbase),
> +		       &(mtrr_suspend_buffer->lsize),
> +		       &(mtrr_suspend_buffer->ltype));
> +     return(ptr);
> +}
> +
> +/* We restore mtrrs from buffer ptr */
> +void mtrr_resume(int *ptr)
> +{
> +     int i, len;
> +     struct mtrr_suspend_state *mtrr_suspend_buffer;
> +     
> +     len = num_var_ranges * sizeof (struct mtrr_suspend_state) + sizeof(int);
> +     if(*ptr != len)
> +     {
> +	  printk ("mtrr: Resuming failed due to different number of MTRRs\n");
> +	  return;
> +     }
> +     ptr++;
> +     mtrr_suspend_buffer=(struct mtrr_suspend_state *)ptr;
> +     for (i = 0; i < num_var_ranges; ++i,mtrr_suspend_buffer++)     
> +	  if (mtrr_suspend_buffer->lsize)	  
> +	       set_mtrr(i,
> +			mtrr_suspend_buffer->lbase,
> +			mtrr_suspend_buffer->lsize,
> +			mtrr_suspend_buffer->ltype);
> +}
> +EXPORT_SYMBOL(mtrr_suspend);
> +EXPORT_SYMBOL(mtrr_resume);
> +#endif
>  core_initcall(mtrr_init);
>  
> 
> 

-- 
Horseback riding is like software...
...vgf orggre jura vgf serr.

Re: [PATCH] restore sysenter MSRs at resume

[Nigel Cunningham]

On Mon, 2003-05-12 at 23:30, Pavel Machek wrote:
> Hi!
> 
> > Ok. I haven't updated it for 2.5.69 version, but it doesn't look like
> > any changes are required. Here is the relevant part of the full swsusp
> > patch.
> 
> I guess it still needs to be updated for the driver model....

Yes,

No time here :> I'm busy with my real work and with work on 2.4 swsusp.

Regards,

Nigel

-- 
Nigel Cunningham
495 St Georges Road South, Hastings 4201, New Zealand

Be diligent to present yourself approved to God as a workman who does
not need to be ashamed, handling accurately the word of truth.
	-- 2 Timothy 2:14, NASB.

Re: [PATCH] restore sysenter MSRs at resume

[Pavel Machek]

Hi!

> > > Ok. I haven't updated it for 2.5.69 version, but it doesn't look like
> > > any changes are required. Here is the relevant part of the full swsusp
> > > patch.
> > 
> > I guess it still needs to be updated for the driver model....
> 
> Yes,
> 
> No time here :> I'm busy with my real work and with work on 2.4 swsusp.

Okay, I'll do it.
								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: [PATCH] restore sysenter MSRs at resume

[Pavel Machek]

Hi!

> > Some laptops also lose all the AGP settings in the chipset.
> 
> Well, that's definitely a driver issue, and should be handled that way. I
> suspect even the MTRR's should be handled as a driver, since unlike things
> like the SYSENTER things, it really _is_ a driver already and is
> conditional on kernel configuration etc.

Here's a patch, it simply adds mtrrs into as "sys" device...

Please apply,
								Pavel

Index: linux/arch/i386/kernel/cpu/mtrr/main.c
===================================================================
--- linux.orig/arch/i386/kernel/cpu/mtrr/main.c	2003-01-05 22:58:19.000000000 +0100
+++ linux/arch/i386/kernel/cpu/mtrr/main.c	2003-05-12 22:06:05.000000000 +0200
@@ -2,6 +2,7 @@
 
     Copyright (C) 1997-2000  Richard Gooch
     Copyright (c) 2002	     Patrick Mochel
+    Copyright (c) 2003	     Nigel Cunningham & Pavel Machek 
 
     This library is free software; you can redistribute it and/or
     modify it under the terms of the GNU Library General Public
@@ -35,6 +36,7 @@
 #include <linux/init.h>
 #include <linux/pci.h>
 #include <linux/smp.h>
+#include <linux/suspend.h>
 
 #include <asm/mtrr.h>
 
@@ -644,6 +646,104 @@
     "write-protect",            /* 5 */
     "write-back",               /* 6 */
 };
+ 
+#ifdef CONFIG_PM
+struct mtrr_suspend_state
+{
+     mtrr_type ltype;
+     unsigned long lbase;
+     unsigned int lsize;
+};
+
+/* We return a pointer ptr on an area of *ptr bytes
+   beginning at ptr+sizeof(int)
+   This buffer has to be saved in some way during suspension */
+static int *mtrr_save_state(void)
+{
+     int i, len;
+     int *ptr = NULL;
+     static struct mtrr_suspend_state *mtrr_suspend_buffer=NULL;
+     
+     if(!mtrr_suspend_buffer)
+     {
+	  len = num_var_ranges * sizeof (struct mtrr_suspend_state) + sizeof(int);
+	  ptr = kmalloc (len, GFP_KERNEL);
+	  if (ptr == NULL)
+	       return(NULL);
+	  *ptr = len;
+	  ptr++;
+	  mtrr_suspend_buffer = (struct mtrr_suspend_state *)ptr;
+	  ptr--;
+     }
+     for (i = 0; i < num_var_ranges; ++i,mtrr_suspend_buffer++)
+	  mtrr_if->get (i,
+		       &(mtrr_suspend_buffer->lbase),
+		       &(mtrr_suspend_buffer->lsize),
+		       &(mtrr_suspend_buffer->ltype));
+     return(ptr);
+}
+
+/* We restore mtrrs from buffer ptr */
+static void mtrr_restore_state(int *ptr)
+{
+     int i, len;
+     struct mtrr_suspend_state *mtrr_suspend_buffer;
+     
+     len = num_var_ranges * sizeof (struct mtrr_suspend_state) + sizeof(int);
+     if(*ptr != len)
+     {
+	  printk ("mtrr: Resuming failed due to different number of MTRRs\n");
+	  return;
+     }
+     ptr++;
+     mtrr_suspend_buffer=(struct mtrr_suspend_state *)ptr;
+     for (i = 0; i < num_var_ranges; ++i,mtrr_suspend_buffer++)     
+	  if (mtrr_suspend_buffer->lsize)	  
+	       set_mtrr(i,
+			mtrr_suspend_buffer->lbase,
+			mtrr_suspend_buffer->lsize,
+			mtrr_suspend_buffer->ltype);
+}
+
+static void *mtrr_state;
+
+static int mtrr_suspend(struct device *dev, u32 state, u32 level)
+{
+	if (level == SUSPEND_SAVE_STATE)
+		mtrr_state = mtrr_save_state();
+	return 0;
+}
+
+static int mtrr_resume(struct device *dev, u32 level)
+{
+	if (level == RESUME_POWER_ON)
+		mtrr_restore_state(mtrr_state);
+	return 0;
+}
+
+static struct device_driver mtrr_driver = {
+	.name		= "mtrr",
+	.bus		= &system_bus_type,
+	.suspend	= mtrr_suspend,
+	.resume		= mtrr_resume,
+};
+
+static struct sys_device device_mtrr = {
+	.name		= "mtrr",
+	.id		= 0,
+	.dev		= {
+		.name	= "mtrr",
+		.driver	= &mtrr_driver,
+	},
+};
 
+static int __init init_mtrr_devicefs(void)
+{
+	driver_register(&mtrr_driver);
+	return sys_device_register(&device_mtrr);
+}
+
+device_initcall(init_mtrr_devicefs);
+#endif
 core_initcall(mtrr_init);
 
								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: [PATCH] restore sysenter MSRs at resume

[Pavel Machek]

Hi!

> > We don't do apm suspend/resume on SMP, so this is no different from the
> > current situation. I don't know if acpi does it or not.
> 
> Well, the thing is, if we ever do want to support it (and I suspect we 
> do), we should have the infrastructure ready. It shouldn't be too hard to 
> support SMP suspend in a 2.7.x timeframe, since it from a technology angle 
> looks like simply hot-plug CPU's. Some of the infrastructure for that 
> already exists.

Actually, then MSRs should restored during hotadd operation, so resume
still does *not* care about non-boot cpus...
								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: [PATCH] restore sysenter MSRs at resume

[Linus Torvalds]

On Wed, 7 May 2003 mikpe@csd.uu.se wrote:
> 
> We don't do apm suspend/resume on SMP, so this is no different from the
> current situation. I don't know if acpi does it or not.

Well, the thing is, if we ever do want to support it (and I suspect we 
do), we should have the infrastructure ready. It shouldn't be too hard to 
support SMP suspend in a 2.7.x timeframe, since it from a technology angle 
looks like simply hot-plug CPU's. Some of the infrastructure for that 
already exists.

But I seriously doubt we want to do CPU hot-plug as a device driver. 
Having a hook in place for it in the arch directory will make it easyish 
to add once we integrate all the other hotplug code (which is very 
unlikely in the 2.6.x timeframe).

> I could probably get away with simply having apm.c invoke the C code
> in suspend.c, which does restore the SYSENTER MSRs. suspend.c itself
> doesn't seem to depend on the SOFTWARE_SUSPEND machinery, but
> suspend_asm.S does.
> 
> Does that sound reasonable?

Sounds reasonable to me. In fact, it looks like it really already exists
as the current "restore_processor_state()" thing.

In fact, that one already _does_ call "enable_sep_cpu()", so what's up?

		Linus

Re: [PATCH] restore sysenter MSRs at resume

[mikpe]

Linus Torvalds writes:
 > 
 > On Wed, 7 May 2003 mikpe@csd.uu.se wrote:
 > > 
 > > The patch below hooks sysenter into the driver model and implements
 > > a resume() method which restores the sysenter MSRs.
 > 
 > This is wrong.
 > 
 > For one thing, you screw up SMP seriously, by not enabling sysenter on all
 > CPU's, only the boot one.

We don't do apm suspend/resume on SMP, so this is no different from the
current situation. I don't know if acpi does it or not.

 > For another, we shouldn't have "device drivers" for the CPU. I certainly
 > agree about restoring the sysenter MSR's, but they should be restored by
 > the CPU-specific code long _before_ we start initializing devices.
 > 
 > So I think we should just make it part of the CPU initialization (which
 > should be in two parts: the low-level asm part for the "core" CPU
 > registers, and then the high-level C part for things like the MSR's, 
 > user-space segment stuff etc).
 > 
 > So why not just add an explicit call to "cpu_resume()" in one of the 
 > "do_magic_resume()" things, instead of playing games with device trees..

Where would cpu_resume() [and cpu_suspend()] live?
arch/i386/kernel/suspend* belong to SOFTWARE_SUSPEND, but I don't
think that approach is desirable when apm mostly works for UP.

I could probably get away with simply having apm.c invoke the C code
in suspend.c, which does restore the SYSENTER MSRs. suspend.c itself
doesn't seem to depend on the SOFTWARE_SUSPEND machinery, but
suspend_asm.S does.

Does that sound reasonable?

 > > The patch has a debug printk() for problematic systems that require
 > > the fix. If it says your machine didn't preserve the MSRs, please
 > > post a note about this to LKML with your machine model, so we can
 > > estimate the scope of the problem.
 > 
 > I really think that it should be done unconditionally - there's no point 
 > in even _expecting_ the BIOS to restore various random MSR's. I can't 
 > imagine that many do.

It does the restore unconditionally, the check is just informational.

/Mikael

Re: [PATCH] restore sysenter MSRs at resume

[Linus Torvalds]

On Wed, 7 May 2003 mikpe@csd.uu.se wrote:
> 
> The patch below hooks sysenter into the driver model and implements
> a resume() method which restores the sysenter MSRs.

This is wrong.

For one thing, you screw up SMP seriously, by not enabling sysenter on all
CPU's, only the boot one.

For another, we shouldn't have "device drivers" for the CPU. I certainly
agree about restoring the sysenter MSR's, but they should be restored by
the CPU-specific code long _before_ we start initializing devices.

So I think we should just make it part of the CPU initialization (which
should be in two parts: the low-level asm part for the "core" CPU
registers, and then the high-level C part for things like the MSR's, 
user-space segment stuff etc).

So why not just add an explicit call to "cpu_resume()" in one of the 
"do_magic_resume()" things, instead of playing games with device trees..

> The patch has a debug printk() for problematic systems that require
> the fix. If it says your machine didn't preserve the MSRs, please
> post a note about this to LKML with your machine model, so we can
> estimate the scope of the problem.

I really think that it should be done unconditionally - there's no point 
in even _expecting_ the BIOS to restore various random MSR's. I can't 
imagine that many do.

		Linus

[PATCH] restore sysenter MSRs at resume

[mikpe]

Dave Jones writes:
 > On Tue, May 06, 2003 at 09:52:24PM +0200, mikpe@csd.uu.se wrote:
 >  > suspending the box (apm). At resume, I am immediately greeted with an
 >  > oops looking like:
 >  > 
 >  > general protection fault: 0000 [#?]
 >  > CPU:	0
 >  > EIP:	0060:[<c0109079>]	Not tainted
 >  > EFLAGS:	00010246
 >  > EIP is at systenter_past_esp+0x6e/0x71
 > 
 > I wonder if your BIOS is trashing the sysenter MSRs on suspend.
 > Maybe they need restoring ?

I've confirmed that that's exatly what's happening. EIP points to
the sysexit instruction in entry.S, and the sysenter MSRs are all zero.

The patch below hooks sysenter into the driver model and implements
a resume() method which restores the sysenter MSRs. On my '98 vintage
Latitude, this is necessary since those MSRs are cleared at resume.
Failure to restore them leads to oopses and eventual kernel hang.
(Of course, your user-space must also use sysenter. RH9 does.)

The patch has a debug printk() for problematic systems that require
the fix. If it says your machine didn't preserve the MSRs, please
post a note about this to LKML with your machine model, so we can
estimate the scope of the problem.

/Mikael

diff -ruN linux-2.5.69/arch/i386/kernel/sysenter.c linux-2.5.69.sysenter-pm/arch/i386/kernel/sysenter.c
--- linux-2.5.69/arch/i386/kernel/sysenter.c	2003-05-05 22:56:28.000000000 +0200
+++ linux-2.5.69.sysenter-pm/arch/i386/kernel/sysenter.c	2003-05-07 10:50:39.690468848 +0200
@@ -51,6 +51,53 @@
 	put_cpu();	
 }
 
+#ifdef CONFIG_PM
+#include <linux/device.h>
+
+static int sysenter_resume(struct device *dev, u32 state, u32 level)
+{
+	if (level != RESUME_POWER_ON)
+		return 0;
+	/* for collecting statistics, will go away */
+	{
+		unsigned int h, l0, l1, l2;
+		rdmsr(MSR_IA32_SYSENTER_CS, l0, h);
+		rdmsr(MSR_IA32_SYSENTER_ESP, l1, h);
+		rdmsr(MSR_IA32_SYSENTER_EIP, l2, h);
+		if (!l0 || !l1 || !l2)
+			printk("sysenter_resume: your BIOS didn't preserve the SYSENTER MSRs\n");
+		else
+			printk("sysenter_resume: congratulations, your BIOS seems Ok\n");
+	}
+	enable_sep_cpu(NULL);
+	return 0;
+}
+
+static struct device_driver sysenter_driver = {
+	.name		= "sysenter",
+	.bus		= &system_bus_type,
+	.resume		= sysenter_resume,
+};
+
+static struct sys_device device_sysenter = {
+	.name		= "sysenter",
+	.id		= 0,
+	.dev		= {
+		.name	= "sysenter",
+		.driver	= &sysenter_driver,
+	},
+};
+
+static int __init init_sysenter_devicefs(void)
+{
+	driver_register(&sysenter_driver);
+	return sys_device_register(&device_sysenter);
+}
+
+#else	/* CONFIG_PM */
+static inline int init_sysenter_devicefs(void) { return 0; }
+#endif	/* CONFIG_PM */
+
 /*
  * These symbols are defined by vsyscall.o to mark the bounds
  * of the ELF DSO images included therein.
@@ -76,7 +123,7 @@
 	       &vsyscall_sysenter_end - &vsyscall_sysenter_start);
 
 	on_each_cpu(enable_sep_cpu, NULL, 1, 1);
-	return 0;
+	return init_sysenter_devicefs();
 }
 
 __initcall(sysenter_setup);