[PATCH] small adjustment to asm constraints for c/s 19400

[PATCH] small adjustment to asm constraints for c/s 19400

[Jan Beulich]

Properly indicate the variable that the asm writes to, and allow
compiler more freedom in selecting a suitable input operand for the ltr
instruction.

Signed-off-by: Jan Beulich <jbeulich@novell.com>

--- 2009-03-27.orig/xen/arch/x86/traps.c	2009-03-24 09:04:02.000000000 +0100
+++ 2009-03-27/xen/arch/x86/traps.c	2009-03-27 10:10:57.000000000 +0100
@@ -3048,8 +3048,8 @@ void load_TR(void)
 
     /* Switch to non-compat GDT (which has B bit clear) to execute LTR. */
     asm volatile (
-        "sgdt %1; lgdt %2; ltr %%ax; lgdt %1"
-        : : "a" (TSS_ENTRY << 3), "m" (old_gdt), "m" (tss_gdt) : "memory" );
+        "sgdt %0; lgdt %2; ltr %w1; lgdt %0"
+        : "=m" (old_gdt) : "rm" (TSS_ENTRY << 3), "m" (tss_gdt) : "memory" );
 }
 
 void __devinit percpu_traps_init(void)

RE: [PATCH] small adjustment to asm constraints for c/s 19400

[Lu, Guanqun]

Hi Jan,

On Saturday, March 28, 2009 12:36 AM Jan Beulich wrote:

> Properly indicate the variable that the asm writes to, and allow
> compiler more freedom in selecting a suitable input operand for the
> ltr instruction.
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> 
> --- 2009-03-27.orig/xen/arch/x86/traps.c	2009-03-24
> 09:04:02.000000000 +0100 +++
> 2009-03-27/xen/arch/x86/traps.c	2009-03-27 10:10:57.000000000 +0100
> @@ -3048,8 +3048,8 @@ void load_TR(void) 
> 
>      /* Switch to non-compat GDT (which has B bit clear) to execute
>      LTR. */ asm volatile (
> -        "sgdt %1; lgdt %2; ltr %%ax; lgdt %1"
> -        : : "a" (TSS_ENTRY << 3), "m" (old_gdt), "m" (tss_gdt) :
> "memory" ); +        "sgdt %0; lgdt %2; ltr %w1; lgdt %0"
> +        : "=m" (old_gdt) : "rm" (TSS_ENTRY << 3), "m" (tss_gdt) :
>  "memory" ); }
> 
>  void __devinit percpu_traps_init(void)
> 

With this adjustment or previous patch 19400, S3 still fails on 64 xen / 32 dom0.
Do you have any idea what will cause this problem?

Thanks.

> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel



-- 
Guanqun

RE: [PATCH] small adjustment to asm constraints for c/s19400

[Jan Beulich]

>>> "Lu, Guanqun" <guanqun.lu@intel.com> 30.03.09 06:45 >>>
>With this adjustment or previous patch 19400, S3 still fails on 64 xen / 32 dom0.
>Do you have any idea what will cause this problem?

And it's still failing at the ltr? Then no, I don't have any clue without additional
data. I'd say try dumping out relevant variables and descriptors.

Jan

RE: [PATCH] small adjustment to asm constraints for c/s19400

[Lu, Guanqun]

On Monday, March 30, 2009 2:43 PM Jan Beulich wrote:

>>>> "Lu, Guanqun" <guanqun.lu@intel.com> 30.03.09 06:45 >>>
>> With this adjustment or previous patch 19400, S3 still fails on 64
>> xen / 32 dom0. Do you have any idea what will cause this problem?
> 
> And it's still failing at the ltr? Then no, I don't have any clue
> without additional data. I'd say try dumping out relevant variables
> and descriptors. 
> 
> Jan

it doesn't fail at ltr. please see my previous mail, thanks.

-- 
Guanqun

Re: [PATCH] small adjustment to asm constraints for c/s 19400

[Keir Fraser]

On 30/03/2009 05:45, "Lu, Guanqun" <guanqun.lu@intel.com> wrote:

> With this adjustment or previous patch 19400, S3 still fails on 64 xen / 32
> dom0.
> Do you have any idea what will cause this problem?

Does it still fail at the LTR? Does the alternative fix of settign the type
to 9 rather than 11 work?

 -- Keir

RE: [PATCH] small adjustment to asm constraints for c/s 19400

[Lu, Guanqun]

On Monday, March 30, 2009 4:45 PM Keir Fraser wrote:

> On 30/03/2009 05:45, "Lu, Guanqun" <guanqun.lu@intel.com> wrote:
> 
>> With this adjustment or previous patch 19400, S3 still fails on 64
>> xen / 32 dom0. Do you have any idea what will cause this problem?
> 
> Does it still fail at the LTR? Does the alternative fix of settign
> the type to 9 rather than 11 work?
> 
>  -- Keir

It doesn't fail at the LTR.
Xen resumes back, but dom0 hangs there(technically speaking, it doen't hang, eip changes, but can't come back to shell console),
I used xen serial line to dump the dom0 registers,
the eip it runs to from time to time is 'general_protection' in dom0.

Any idea?

Fixing type to 9 doesn't really solves the problem, it has the same phenomenon.
I'll have a double check, and try to dump more registers and variable according to Jan's advice and see if 
something un-expected happens.

Thanks.
-- 
Guanqun

RE: [PATCH] small adjustment to asm constraints for c/s19400

[Jan Beulich]

>>> "Lu, Guanqun" <guanqun.lu@intel.com> 30.03.09 10:56 >>>
>On Monday, March 30, 2009 4:45 PM Keir Fraser wrote:
>
>> On 30/03/2009 05:45, "Lu, Guanqun" <guanqun.lu@intel.com> wrote:
>> 
>>> With this adjustment or previous patch 19400, S3 still fails on 64
>>> xen / 32 dom0. Do you have any idea what will cause this problem?
>> 
>> Does it still fail at the LTR? Does the alternative fix of settign
>> the type to 9 rather than 11 work?
>> 
>>  -- Keir
>
>It doesn't fail at the LTR.
>Xen resumes back, but dom0 hangs there(technically speaking, it doen't hang, eip changes, but can't come back to shell console),
>I used xen serial line to dump the dom0 registers,
>the eip it runs to from time to time is 'general_protection' in dom0.

Since under normal circumstances GP faults are rare, why don't you just
print out the faulting vCPU's state from Xen at the point where the fault
gets forwarded to the guest? That'll tell you exactly what the guest is
dying on.

Jan

RE: [PATCH] small adjustment to asm constraints for c/s19400

[Lu, Guanqun]

On Monday, March 30, 2009 5:13 PM Jan Beulich wrote:

>>>> "Lu, Guanqun" <guanqun.lu@intel.com> 30.03.09 10:56 >>>
>> On Monday, March 30, 2009 4:45 PM Keir Fraser wrote:
>> 
>>> On 30/03/2009 05:45, "Lu, Guanqun" <guanqun.lu@intel.com> wrote:
>>> 
>>>> With this adjustment or previous patch 19400, S3 still fails on 64
>>>> xen / 32 dom0. Do you have any idea what will cause this problem?
>>> 
>>> Does it still fail at the LTR? Does the alternative fix of settign
>>> the type to 9 rather than 11 work?
>>> 
>>>  -- Keir
>> 
>> It doesn't fail at the LTR.
>> Xen resumes back, but dom0 hangs there(technically speaking, it
>> doen't hang, eip changes, but can't come back to shell console), I
>> used xen serial line to dump the dom0 registers, 
>> the eip it runs to from time to time is 'general_protection' in dom0.
> 
> Since under normal circumstances GP faults are rare, why don't you
> just print out the faulting vCPU's state from Xen at the point where
> the fault gets forwarded to the guest? That'll tell you exactly what
> the guest is dying on.
> 
> Jan

Hi Jan,

The point when S3 resumes back, it's running in compat mode.
But the code doesn't differentiate it, as in xen/arch/x86/acpi/wakeup_prot.S.
Maybe we lose some context during the switches which may causes the problem.

When it resumes back, hitting '0' on xen serial console gives us:

(XEN) '0' pressed -> dumping Dom0's registers
(XEN) *** Dumping Dom0 vcpu#0 state: ***
(XEN) ----[ Xen-3.4-unstable  x86_64  debug=y  Not tainted ]----
(XEN) CPU:    0
(XEN) RIP:    0061:[<00000000c02a9208>]
(XEN) RFLAGS: 0000000000200246   EM: 0   CONTEXT: pv guest
(XEN) rax: 00000000000000af   rbx: 0000000000000000   rcx: 00000000bfe7d8e4
(XEN) rdx: 00000000bfe7d864   rsi: 0000000000000008   rdi: 0000000000d3cff4
(XEN) rbp: 00000000bfe7d790   rsp: 00000000c6aa3fe0   r8:  0000000000000000
(XEN) r9:  0000000000000000   r10: 0000000000000000   r11: 0000000000000000
(XEN) r12: 0000000000000000   r13: 0000000000000000   r14: 0000000000000000
(XEN) r15: 0000000000000000   cr0: 000000008005003b   cr4: 00000000000026f0
(XEN) cr3: 00000000bd6e4000   cr2: 00000000b7fbe000
(XEN) ds: 007b   es: 007b   fs: 0000   gs: 0033   ss: 0069   cs: 0061
(XEN) Guest stack trace from esp=c6aa3fe0:
(XEN)   00000000 b7f9d405 00000073 00210246 bfe7d790 0000007b 00000000 00000000
(XEN)   c6aa1080 c02fd6a0 00000000 00000000 00000000 00000000 ffffffff 00000000
(XEN)   c0126a47 00000000 00000000 00000000 00000000 00000000 00000000 00000000
(XEN)   ffffffff ffffffbf ffffffff ffffffff 00000000 00000000 ffffffff ffffffff
(XEN)   ffffffff ffffffff 00000000 00000000 00000000 00000000 00000000 00000000
(XEN)   ffffffff ffffffff ffffffff ffffffff 00000000 00000000 ffffffff ffffffff
(XEN)   ffbfffff ffffffff 00000000 00000000 00000000 00000000 00000000 00000000
(XEN)   fffffeff ffffffff ffffffff ffffffff 00000000 00000000 ffffffff ffffffff
(XEN)   ffffffff ffffffff 00000000 00000000 00000000 00000000 00000000 00000000
(XEN)   ffffffff ffffffff ffffffff ffffffff 00000000 00000000 ffffffff ffffffff
(XEN)   ffffffff ffffffff 00000000 00000000 00000000 00000000 00000000 00000000
(XEN)   ffffffff ffffffff ffffffff ffffffff 00000000 00000000 ffffffff ffffffff
(XEN)   bfffffff ffffffff 00000000 00000000 00000000 00000000 00000000 00000000
(XEN)   fffbffff ffffffff ffefffff ffffffff 00000000 00000000 ffffffff ffffffff
(XEN)   ff7fffff ffffffff 00000000 00000000 00000000 00000000 00000000 00000000
(XEN)   ffffffff ffffffff ffffffff ffffffff 00000000 00000000 ffffffff ffffffff
(XEN)   ffffffff ffffffff 00000000 00000000 00000000 00000000 00000000 00000000
(XEN)   fff7dfff ffffffff ffffffff ffffffff 00000000 00000000 ffffffff ffffffff
(XEN)   ffffffff fffffff7 00000000 00000000 00000000 00000000 00000000 00000000
(XEN)   bfffffff ffffffff ffffffff ffffffff 00000000 00000000 ffffffff ffffffff

Comparing the eip with the dom0 symbol table,
0xc02a9208 -> general_protection
0xc02fd6a0 -> default_exec_domain
0xc0126a47 -> do_no_restart_syscall
>From the stack, such invocation chain is observed.

Any idea what's happening there?

And I also dump the MSR just before and after S3, there're no difference.
patch:

diff -r d5ddc782bc49 xen/arch/x86/acpi/power.c
--- a/xen/arch/x86/acpi/power.c Mon Mar 30 16:48:26 2009 +0100
+++ b/xen/arch/x86/acpi/power.c Tue Mar 31 12:26:12 2009 +0800
@@ -42,6 +42,28 @@ struct acpi_sleep_info acpi_sinfo;

 void do_suspend_lowlevel(void);

+static void dump_msr_registers(void)
+{
+    unsigned long msr;
+
+    rdmsrl(MSR_EFER, msr);
+    printk("<0> EFER: %lx", msr);
+    rdmsrl(MSR_STAR, msr);
+    printk("<0> STAR: %lx", msr);
+    rdmsrl(MSR_LSTAR, msr);
+    printk("<0> LSTAR: %lx", msr);
+    rdmsrl(MSR_CSTAR, msr);
+    printk("<0> CSTAR: %lx", msr);
+    rdmsrl(MSR_SYSCALL_MASK, msr);
+    printk("<0> SYSCALL_MASK: %lx", msr);
+    rdmsrl(MSR_FS_BASE, msr);
+    printk("<0> FS_BASE: %lx", msr);
+    rdmsrl(MSR_GS_BASE, msr);
+    printk("<0> GS_BASE: %lx", msr);
+    rdmsrl(MSR_SHADOW_GS_BASE, msr);
+    printk("<0> SH_GS_BASE: %lx", msr);
+}
+
 static int device_power_down(void)
 {
     console_suspend();
@@ -173,6 +195,7 @@ static int enter_state(u32 state)

     console_start_sync();
     printk("Entering ACPI S%d state.\n", state);
+    dump_msr_registers();

     local_irq_save(flags);
     spin_debug_disable();
@@ -208,6 +231,7 @@ static int enter_state(u32 state)
     device_power_up();

     printk(XENLOG_INFO "Finishing wakeup from ACPI S%d state.\n", state);
+    dump_msr_registers();

     if ( (state == ACPI_STATE_S3) && error )
         panic("Memory integrity was lost on resume (%d)\n", error);

the result is:
[before] (XEN)  EFER: d01<0> STAR: e023e00800000000<0> LSTAR: ffff828c8029b000<0> CSTAR: ffff828c8029b080<0> SYSCALL_MASK: 34700<0> FS_BASE: 0<0> GS_BASE: b7f4a6c0<0> SH_GS_BASE: 0
[after] (XEN)  EFER: d01<0> STAR: e023e00800000000<0> LSTAR: ffff828c8029b000<0> CSTAR: ffff828c8029b080<0> SYSCALL_MASK: 34700<0> FS_BASE: 0<0> GS_BASE: b7f4a6c0<0> SH_GS_BASE: 0
 
They're the same.

And aslo I dump GDT, IDT, LDT and TR register and tss struct, there are no difference seen.


So can you give some advice some more registers should be dumped?
Or maybe our S3 code doesn't take the 32dom0 into consideration from the beginning, is there anyway to fix this?



Thanks!

-- 
Guanqun

RE: [PATCH] small adjustment to asm constraints for c/s19400

[Tian, Kevin]

[-- Attachment #1: Type: text/plain, Size: 6531 bytes --]

>From: Lu, Guanqun
>Sent: 2009年3月31日 16:55
>Hi Jan,
>
>The point when S3 resumes back, it's running in compat mode.
>But the code doesn't differentiate it, as in 
>xen/arch/x86/acpi/wakeup_prot.S.
>Maybe we lose some context during the switches which may 
>causes the problem.

One correction here. pCPU is of course in long mode when resuming
from S3 since it happens in Xen. It's current vcpu as a 32bit vcpu with
compatibility context (32bit dom0). There may be some registers only 
used in compatibility mode, but kept even when Xen is running in long
mode. However current low level context save/restore across S3
suspend/resume, as you can see, have nothing specific for whether
current context is 32bit vcpu. So we're not sure whether some registers
content specific for compatibility mode get lost (cpu is reset in S3) and
not be recovered by current resume code.

Jan, since you're the No.1 expert in this area, we'd appreciate your
possible suggestions here. :-)

Thanks
Kevin

>
>When it resumes back, hitting '0' on xen serial console gives us:
>
>(XEN) '0' pressed -> dumping Dom0's registers
>(XEN) *** Dumping Dom0 vcpu#0 state: ***
>(XEN) ----[ Xen-3.4-unstable  x86_64  debug=y  Not tainted ]----
>(XEN) CPU:    0
>(XEN) RIP:    0061:[<00000000c02a9208>]
>(XEN) RFLAGS: 0000000000200246   EM: 0   CONTEXT: pv guest
>(XEN) rax: 00000000000000af   rbx: 0000000000000000   rcx: 
>00000000bfe7d8e4
>(XEN) rdx: 00000000bfe7d864   rsi: 0000000000000008   rdi: 
>0000000000d3cff4
>(XEN) rbp: 00000000bfe7d790   rsp: 00000000c6aa3fe0   r8:  
>0000000000000000
>(XEN) r9:  0000000000000000   r10: 0000000000000000   r11: 
>0000000000000000
>(XEN) r12: 0000000000000000   r13: 0000000000000000   r14: 
>0000000000000000
>(XEN) r15: 0000000000000000   cr0: 000000008005003b   cr4: 
>00000000000026f0
>(XEN) cr3: 00000000bd6e4000   cr2: 00000000b7fbe000
>(XEN) ds: 007b   es: 007b   fs: 0000   gs: 0033   ss: 0069   cs: 0061
>(XEN) Guest stack trace from esp=c6aa3fe0:
>(XEN)   00000000 b7f9d405 00000073 00210246 bfe7d790 0000007b 
>00000000 00000000
>(XEN)   c6aa1080 c02fd6a0 00000000 00000000 00000000 00000000 
>ffffffff 00000000
>(XEN)   c0126a47 00000000 00000000 00000000 00000000 00000000 
>00000000 00000000
>(XEN)   ffffffff ffffffbf ffffffff ffffffff 00000000 00000000 
>ffffffff ffffffff
>(XEN)   ffffffff ffffffff 00000000 00000000 00000000 00000000 
>00000000 00000000
>(XEN)   ffffffff ffffffff ffffffff ffffffff 00000000 00000000 
>ffffffff ffffffff
>(XEN)   ffbfffff ffffffff 00000000 00000000 00000000 00000000 
>00000000 00000000
>(XEN)   fffffeff ffffffff ffffffff ffffffff 00000000 00000000 
>ffffffff ffffffff
>(XEN)   ffffffff ffffffff 00000000 00000000 00000000 00000000 
>00000000 00000000
>(XEN)   ffffffff ffffffff ffffffff ffffffff 00000000 00000000 
>ffffffff ffffffff
>(XEN)   ffffffff ffffffff 00000000 00000000 00000000 00000000 
>00000000 00000000
>(XEN)   ffffffff ffffffff ffffffff ffffffff 00000000 00000000 
>ffffffff ffffffff
>(XEN)   bfffffff ffffffff 00000000 00000000 00000000 00000000 
>00000000 00000000
>(XEN)   fffbffff ffffffff ffefffff ffffffff 00000000 00000000 
>ffffffff ffffffff
>(XEN)   ff7fffff ffffffff 00000000 00000000 00000000 00000000 
>00000000 00000000
>(XEN)   ffffffff ffffffff ffffffff ffffffff 00000000 00000000 
>ffffffff ffffffff
>(XEN)   ffffffff ffffffff 00000000 00000000 00000000 00000000 
>00000000 00000000
>(XEN)   fff7dfff ffffffff ffffffff ffffffff 00000000 00000000 
>ffffffff ffffffff
>(XEN)   ffffffff fffffff7 00000000 00000000 00000000 00000000 
>00000000 00000000
>(XEN)   bfffffff ffffffff ffffffff ffffffff 00000000 00000000 
>ffffffff ffffffff
>
>Comparing the eip with the dom0 symbol table,
>0xc02a9208 -> general_protection
>0xc02fd6a0 -> default_exec_domain
>0xc0126a47 -> do_no_restart_syscall
>>From the stack, such invocation chain is observed.
>
>Any idea what's happening there?
>
>And I also dump the MSR just before and after S3, there're no 
>difference.
>patch:
>
>diff -r d5ddc782bc49 xen/arch/x86/acpi/power.c
>--- a/xen/arch/x86/acpi/power.c Mon Mar 30 16:48:26 2009 +0100
>+++ b/xen/arch/x86/acpi/power.c Tue Mar 31 12:26:12 2009 +0800
>@@ -42,6 +42,28 @@ struct acpi_sleep_info acpi_sinfo;
>
> void do_suspend_lowlevel(void);
>
>+static void dump_msr_registers(void)
>+{
>+    unsigned long msr;
>+
>+    rdmsrl(MSR_EFER, msr);
>+    printk("<0> EFER: %lx", msr);
>+    rdmsrl(MSR_STAR, msr);
>+    printk("<0> STAR: %lx", msr);
>+    rdmsrl(MSR_LSTAR, msr);
>+    printk("<0> LSTAR: %lx", msr);
>+    rdmsrl(MSR_CSTAR, msr);
>+    printk("<0> CSTAR: %lx", msr);
>+    rdmsrl(MSR_SYSCALL_MASK, msr);
>+    printk("<0> SYSCALL_MASK: %lx", msr);
>+    rdmsrl(MSR_FS_BASE, msr);
>+    printk("<0> FS_BASE: %lx", msr);
>+    rdmsrl(MSR_GS_BASE, msr);
>+    printk("<0> GS_BASE: %lx", msr);
>+    rdmsrl(MSR_SHADOW_GS_BASE, msr);
>+    printk("<0> SH_GS_BASE: %lx", msr);
>+}
>+
> static int device_power_down(void)
> {
>     console_suspend();
>@@ -173,6 +195,7 @@ static int enter_state(u32 state)
>
>     console_start_sync();
>     printk("Entering ACPI S%d state.\n", state);
>+    dump_msr_registers();
>
>     local_irq_save(flags);
>     spin_debug_disable();
>@@ -208,6 +231,7 @@ static int enter_state(u32 state)
>     device_power_up();
>
>     printk(XENLOG_INFO "Finishing wakeup from ACPI S%d 
>state.\n", state);
>+    dump_msr_registers();
>
>     if ( (state == ACPI_STATE_S3) && error )
>         panic("Memory integrity was lost on resume (%d)\n", error);
>
>the result is:
>[before] (XEN)  EFER: d01<0> STAR: e023e00800000000<0> LSTAR: 
>ffff828c8029b000<0> CSTAR: ffff828c8029b080<0> SYSCALL_MASK: 
>34700<0> FS_BASE: 0<0> GS_BASE: b7f4a6c0<0> SH_GS_BASE: 0
>[after] (XEN)  EFER: d01<0> STAR: e023e00800000000<0> LSTAR: 
>ffff828c8029b000<0> CSTAR: ffff828c8029b080<0> SYSCALL_MASK: 
>34700<0> FS_BASE: 0<0> GS_BASE: b7f4a6c0<0> SH_GS_BASE: 0
> 
>They're the same.
>
>And aslo I dump GDT, IDT, LDT and TR register and tss struct, 
>there are no difference seen.
>
>
>So can you give some advice some more registers should be dumped?
>Or maybe our S3 code doesn't take the 32dom0 into 
>consideration from the beginning, is there anyway to fix this?
>
>
>
>Thanks!
>
>-- 
>Guanqun
>_______________________________________________
>Xen-devel mailing list
>Xen-devel@lists.xensource.com
>http://lists.xensource.com/xen-devel
>

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

RE: [PATCH] small adjustment to asm constraints forc/s19400

[Jan Beulich]

>>> "Lu, Guanqun" <guanqun.lu@intel.com> 31.03.09 10:54 >>>
>[before] (XEN)  EFER: d01<0> STAR: e023e00800000000<0> LSTAR: ffff828c8029b000<0> CSTAR: ffff828c8029b080<0>
>SYSCALL_MASK: 34700<0> FS_BASE: 0<0> GS_BASE: b7f4a6c0<0> SH_GS_BASE: 0
>[after] (XEN)  EFER: d01<0> STAR: e023e00800000000<0> LSTAR: ffff828c8029b000<0> CSTAR: ffff828c8029b080<0>
>SYSCALL_MASK: 34700<0> FS_BASE: 0<0> GS_BASE: b7f4a6c0<0> SH_GS_BASE: 0
> 
>They're the same.
>
>And aslo I dump GDT, IDT, LDT and TR register and tss struct, there are no difference seen.
>
>
>So can you give some advice some more registers should be dumped?

Assuming this is an Intel box, the guest is certainly using sysenter (which the user mode EIP saved on the stack would also suggest), so you're really after the three SYSENTER MSRs. And indeed - restore_rest_processor_state() seems to only care for the STAR ones.

Jan

RE: [PATCH] small adjustment to asm constraints forc/s19400

[Tian, Kevin]

[-- Attachment #1: Type: text/plain, Size: 1129 bytes --]

>From: Jan Beulich
>Sent: 2009年3月31日 17:24
>
>>>> "Lu, Guanqun" <guanqun.lu@intel.com> 31.03.09 10:54 >>>
>>[before] (XEN)  EFER: d01<0> STAR: e023e00800000000<0> LSTAR: 
>ffff828c8029b000<0> CSTAR: ffff828c8029b080<0>
>>SYSCALL_MASK: 34700<0> FS_BASE: 0<0> GS_BASE: b7f4a6c0<0> 
>SH_GS_BASE: 0
>>[after] (XEN)  EFER: d01<0> STAR: e023e00800000000<0> LSTAR: 
>ffff828c8029b000<0> CSTAR: ffff828c8029b080<0>
>>SYSCALL_MASK: 34700<0> FS_BASE: 0<0> GS_BASE: b7f4a6c0<0> 
>SH_GS_BASE: 0
>> 
>>They're the same.
>>
>>And aslo I dump GDT, IDT, LDT and TR register and tss struct, 
>there are no difference seen.
>>
>>
>>So can you give some advice some more registers should be dumped?
>
>Assuming this is an Intel box, the guest is certainly using 
>sysenter (which the user mode EIP saved on the stack would 
>also suggest), so you're really after the three SYSENTER MSRs. 
>And indeed - restore_rest_processor_state() seems to only care 
>for the STAR ones.
>

There are also some set of registers save/restored in assembly code
in wakeup_prot.S, such as segment selectors, CRs, etc.

Thanks
Kevin

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

RE: [PATCH] small adjustment to asm constraints forc/s19400

[Jan Beulich]

>>> "Tian, Kevin" <kevin.tian@intel.com> 31.03.09 11:26 >>>
>>From: Jan Beulich
>>Assuming this is an Intel box, the guest is certainly using 
>>sysenter (which the user mode EIP saved on the stack would 
>>also suggest), so you're really after the three SYSENTER MSRs. 
>>And indeed - restore_rest_processor_state() seems to only care 
>>for the STAR ones.
>>
>
>There are also some set of registers save/restored in assembly code
>in wakeup_prot.S, such as segment selectors, CRs, etc.

Sure, but just grep-ing for MSR_IA32_SYSENTER across all of xen/ shows
that the suspend/resume paths don't seem to consider them at all.

Jan

RE: [PATCH] small adjustment to asm constraints forc/s19400

[Tian, Kevin]

[-- Attachment #1: Type: text/plain, Size: 1307 bytes --]

>From: Jan Beulich
>Sent: 2009年3月31日 17:24
>
>>>> "Lu, Guanqun" <guanqun.lu@intel.com> 31.03.09 10:54 >>>
>>[before] (XEN)  EFER: d01<0> STAR: e023e00800000000<0> LSTAR: 
>ffff828c8029b000<0> CSTAR: ffff828c8029b080<0>
>>SYSCALL_MASK: 34700<0> FS_BASE: 0<0> GS_BASE: b7f4a6c0<0> 
>SH_GS_BASE: 0
>>[after] (XEN)  EFER: d01<0> STAR: e023e00800000000<0> LSTAR: 
>ffff828c8029b000<0> CSTAR: ffff828c8029b080<0>
>>SYSCALL_MASK: 34700<0> FS_BASE: 0<0> GS_BASE: b7f4a6c0<0> 
>SH_GS_BASE: 0
>> 
>>They're the same.
>>
>>And aslo I dump GDT, IDT, LDT and TR register and tss struct, 
>there are no difference seen.
>>
>>
>>So can you give some advice some more registers should be dumped?
>
>Assuming this is an Intel box, the guest is certainly using 
>sysenter (which the user mode EIP saved on the stack would 
>also suggest), so you're really after the three SYSENTER MSRs. 
>And indeed - restore_rest_processor_state() seems to only care 
>for the STAR ones.
>

Well, got your point. Those three SYSENTER MSRs are lost after resume.
We'll verify whether this is the only missing parts.  BTW, in our box 32-on
-32 S3 works fine, does that mean SYSENTER/SYSEXIT not used in pure
32bit pv environment, since in Xen 32bit path also misses that important
recovery?

Thanks
Kevin

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

RE: [PATCH] small adjustment to asm constraints forc/s19400

[Jan Beulich]

>>> "Tian, Kevin" <kevin.tian@intel.com> 31.03.09 11:47 >>>
>Well, got your point. Those three SYSENTER MSRs are lost after resume.
>We'll verify whether this is the only missing parts.  BTW, in our box 32-on
>-32 S3 works fine, does that mean SYSENTER/SYSEXIT not used in pure
>32bit pv environment, since in Xen 32bit path also misses that important
>recovery?

A 32-bit hypervisor does not expose sysenter to the guest (the feature bit
gets cleared) - iirc this is to prevent the guest from thinking that using
sysenter would provide a performance benefit. Hence restoring these MSRs
in that case would only be needed if Xen itself made use of sysenter, which
however it doesn't (except for the now only theoretical case of the
supervisor-mode-kernel feature).

Jan

RE: [PATCH] small adjustment to asm constraints forc/s19400

[Tian, Kevin]

[-- Attachment #1: Type: text/plain, Size: 1302 bytes --]

>From: Jan Beulich [mailto:jbeulich@novell.com] 
>Sent: 2009年3月31日 18:15
>
>>>> "Tian, Kevin" <kevin.tian@intel.com> 31.03.09 11:47 >>>
>>Well, got your point. Those three SYSENTER MSRs are lost after resume.
>>We'll verify whether this is the only missing parts.  BTW, in 
>our box 32-on
>>-32 S3 works fine, does that mean SYSENTER/SYSEXIT not used in pure
>>32bit pv environment, since in Xen 32bit path also misses 
>that important
>>recovery?
>
>A 32-bit hypervisor does not expose sysenter to the guest (the 
>feature bit
>gets cleared) - iirc this is to prevent the guest from 
>thinking that using
>sysenter would provide a performance benefit. Hence restoring 
>these MSRs
>in that case would only be needed if Xen itself made use of 
>sysenter, which
>however it doesn't (except for the now only theoretical case of the
>supervisor-mode-kernel feature).
>

Clear enough. BTW, another question comes when I'm clearing the code.
in load_segments, fs/gs base MSRs are only updated for non 32-on-64
domain. I guess that it's just for performance reason as 32bit guest 
doesn't require to switch gs base. I want to get your confirmation 
since in current low level context S3 resume code, we always restore 
those base MSRs even for 32-on-64 case.

Thanks,
Kevin

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

RE: [PATCH] small adjustment to asm constraintsforc/s19400

[Jan Beulich]

>>> "Tian, Kevin" <kevin.tian@intel.com> 31.03.09 12:21 >>>
>BTW, another question comes when I'm clearing the code.
>in load_segments, fs/gs base MSRs are only updated for non 32-on-64
>domain. I guess that it's just for performance reason as 32bit guest 
>doesn't require to switch gs base. I want to get your confirmation 
>since in current low level context S3 resume code, we always restore 
>those base MSRs even for 32-on-64 case.

Correct - your code isn't performance sensitive, so keeping it simple is
probably preferable. load_segments() sits in the context switch code, and
thus it tries to avoid things not needed for a particular guest.

Jan

RE: [PATCH] small adjustment to asm constraintsforc/s19400

[Tian, Kevin]

[-- Attachment #1: Type: text/plain, Size: 2764 bytes --]

>From: Jan Beulich [mailto:jbeulich@novell.com] 
>Sent: 2009年3月31日 18:55
>
>>>> "Tian, Kevin" <kevin.tian@intel.com> 31.03.09 12:21 >>>
>>BTW, another question comes when I'm clearing the code.
>>in load_segments, fs/gs base MSRs are only updated for non 32-on-64
>>domain. I guess that it's just for performance reason as 32bit guest 
>>doesn't require to switch gs base. I want to get your confirmation 
>>since in current low level context S3 resume code, we always restore 
>>those base MSRs even for 32-on-64 case.
>
>Correct - your code isn't performance sensitive, so keeping it 
>simple is
>probably preferable. load_segments() sits in the context 
>switch code, and
>thus it tries to avoid things not needed for a particular guest.
>

Thanks. After recovering those 3 SYSENTER MSRs, now 32-on-64
S3 could succeeds several times which is a big step. However there
seems to be some other instability issues to cause machine 
occasionally reboot after more S3 actions. Both screen and serial
port is dark and thus no more info is available. We'll continue to dig out 
the exact reason. But this shouldn't prevent sending out this 
reasonable fix anyway.

Thanks,
Kevin

-----

Enable S3 for 32bit dom0 on 64bit Xen

Three SYSENTER MSRs should be taken care of at save/restore BSP context,
or else 32bit dom0 rejects working after S3 resume. Thanks for Jan's help to
find this missing part.

Signed-off-by Guanqun Lu <guanqun.lu@intel.com>
Signed-off-by Kevin Tian <kevin.tian@intel.com>

diff -r e152892a0b1e xen/arch/x86/acpi/suspend.c
--- a/xen/arch/x86/acpi/suspend.c	Tue Mar 31 10:21:55 2009 +0800
+++ b/xen/arch/x86/acpi/suspend.c	Tue Mar 31 19:06:08 2009 +0800
@@ -16,6 +16,7 @@
 
 #if defined(CONFIG_X86_64)
 static unsigned long saved_lstar, saved_cstar;
+static unsigned long saved_sysenter_esp, saved_sysenter_eip;
 #endif
 
 void save_rest_processor_state(void)
@@ -26,6 +27,8 @@
 #if defined(CONFIG_X86_64)
     rdmsrl(MSR_CSTAR, saved_cstar);
     rdmsrl(MSR_LSTAR, saved_lstar);
+    rdmsrl(MSR_IA32_SYSENTER_ESP, saved_sysenter_esp);
+    rdmsrl(MSR_IA32_SYSENTER_EIP, saved_sysenter_eip);
 #endif
 }
 
@@ -41,6 +44,11 @@
     wrmsrl(MSR_CSTAR, saved_cstar);
     wrmsr(MSR_STAR, 0, (FLAT_RING3_CS32<<16) | __HYPERVISOR_CS);
     wrmsr(MSR_SYSCALL_MASK, EF_VM|EF_RF|EF_NT|EF_DF|EF_IE|EF_TF, 0U);    
+
+    /* Recover sysenter MSRs */
+    wrmsrl(MSR_IA32_SYSENTER_ESP, saved_sysenter_esp);
+    wrmsrl(MSR_IA32_SYSENTER_EIP, saved_sysenter_eip);
+    wrmsr(MSR_IA32_SYSENTER_CS, __HYPERVISOR_CS, 0);
 #else /* !defined(CONFIG_X86_64) */
     if ( supervisor_mode_kernel && cpu_has_sep )
         wrmsr(MSR_IA32_SYSENTER_ESP, &init_tss[smp_processor_id()].esp1, 0);


[-- Attachment #2: 32-on-64-s3.patch --]
[-- Type: application/octet-stream, Size: 1193 bytes --]

diff -r e152892a0b1e xen/arch/x86/acpi/suspend.c
--- a/xen/arch/x86/acpi/suspend.c	Tue Mar 31 10:21:55 2009 +0800
+++ b/xen/arch/x86/acpi/suspend.c	Tue Mar 31 19:06:08 2009 +0800
@@ -16,6 +16,7 @@
 
 #if defined(CONFIG_X86_64)
 static unsigned long saved_lstar, saved_cstar;
+static unsigned long saved_sysenter_esp, saved_sysenter_eip;
 #endif
 
 void save_rest_processor_state(void)
@@ -26,6 +27,8 @@
 #if defined(CONFIG_X86_64)
     rdmsrl(MSR_CSTAR, saved_cstar);
     rdmsrl(MSR_LSTAR, saved_lstar);
+    rdmsrl(MSR_IA32_SYSENTER_ESP, saved_sysenter_esp);
+    rdmsrl(MSR_IA32_SYSENTER_EIP, saved_sysenter_eip);
 #endif
 }
 
@@ -41,6 +44,11 @@
     wrmsrl(MSR_CSTAR, saved_cstar);
     wrmsr(MSR_STAR, 0, (FLAT_RING3_CS32<<16) | __HYPERVISOR_CS);
     wrmsr(MSR_SYSCALL_MASK, EF_VM|EF_RF|EF_NT|EF_DF|EF_IE|EF_TF, 0U);    
+
+    /* Recover sysenter MSRs */
+    wrmsrl(MSR_IA32_SYSENTER_ESP, saved_sysenter_esp);
+    wrmsrl(MSR_IA32_SYSENTER_EIP, saved_sysenter_eip);
+    wrmsr(MSR_IA32_SYSENTER_CS, __HYPERVISOR_CS, 0);
 #else /* !defined(CONFIG_X86_64) */
     if ( supervisor_mode_kernel && cpu_has_sep )
         wrmsr(MSR_IA32_SYSENTER_ESP, &init_tss[smp_processor_id()].esp1, 0);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

RE: [PATCH] small adjustment to asm constraintsforc/s19400

[Jan Beulich]

>>> "Tian, Kevin" <kevin.tian@intel.com> 31.03.09 13:09 >>>
>Thanks. After recovering those 3 SYSENTER MSRs, now 32-on-64
>S3 could succeeds several times which is a big step. However there
>seems to be some other instability issues to cause machine 
>occasionally reboot after more S3 actions. Both screen and serial
>port is dark and thus no more info is available. We'll continue to dig out 
>the exact reason. But this shouldn't prevent sending out this 
>reasonable fix anyway.

I think you need to be a little more careful here - subarch_percpu_traps_init()
writes them only on Intel CPUs, and I think you ought to follow this for
both saving and restoring them.

Jan

RE: [PATCH] small adjustment to asm constraintsforc/s19400

[Tian, Kevin]

[-- Attachment #1: Type: text/plain, Size: 2591 bytes --]

 

>From: Jan Beulich [mailto:jbeulich@novell.com] 
>Sent: 2009年3月31日 19:30
>
>>>> "Tian, Kevin" <kevin.tian@intel.com> 31.03.09 13:09 >>>
>>Thanks. After recovering those 3 SYSENTER MSRs, now 32-on-64
>>S3 could succeeds several times which is a big step. However there
>>seems to be some other instability issues to cause machine 
>>occasionally reboot after more S3 actions. Both screen and serial
>>port is dark and thus no more info is available. We'll 
>continue to dig out 
>>the exact reason. But this shouldn't prevent sending out this 
>>reasonable fix anyway.
>
>I think you need to be a little more careful here - 
>subarch_percpu_traps_init()
>writes them only on Intel CPUs, and I think you ought to 
>follow this for
>both saving and restoring them.
>

Ah, yes, I need add same condition check here. Below is updated
one:

-----

Enable S3 for 32bit dom0 on 64bit Xen

Three SYSENTER MSRs should be taken care of at save/restore BSP context,
or else 32bit dom0 rejects working after S3 resume. Thanks for Jan's help to
find this missing part.

Signed-off-by Guanqun Lu <guanqun.lu@intel.com>
Signed-off-by Kevin Tian <kevin.tian@intel.com>

diff -r e152892a0b1e xen/arch/x86/acpi/suspend.c
--- a/xen/arch/x86/acpi/suspend.c	Tue Mar 31 10:21:55 2009 +0800
+++ b/xen/arch/x86/acpi/suspend.c	Tue Mar 31 19:32:41 2009 +0800
@@ -16,6 +16,7 @@
 
 #if defined(CONFIG_X86_64)
 static unsigned long saved_lstar, saved_cstar;
+static unsigned long saved_sysenter_esp, saved_sysenter_eip;
 #endif
 
 void save_rest_processor_state(void)
@@ -26,6 +27,11 @@
 #if defined(CONFIG_X86_64)
     rdmsrl(MSR_CSTAR, saved_cstar);
     rdmsrl(MSR_LSTAR, saved_lstar);
+    if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
+    {
+        rdmsrl(MSR_IA32_SYSENTER_ESP, saved_sysenter_esp);
+        rdmsrl(MSR_IA32_SYSENTER_EIP, saved_sysenter_eip);
+    }
 #endif
 }
 
@@ -41,6 +47,14 @@
     wrmsrl(MSR_CSTAR, saved_cstar);
     wrmsr(MSR_STAR, 0, (FLAT_RING3_CS32<<16) | __HYPERVISOR_CS);
     wrmsr(MSR_SYSCALL_MASK, EF_VM|EF_RF|EF_NT|EF_DF|EF_IE|EF_TF, 0U);    
+
+    if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
+    {
+        /* Recover sysenter MSRs */
+        wrmsrl(MSR_IA32_SYSENTER_ESP, saved_sysenter_esp);
+        wrmsrl(MSR_IA32_SYSENTER_EIP, saved_sysenter_eip);
+        wrmsr(MSR_IA32_SYSENTER_CS, __HYPERVISOR_CS, 0);
+    }
 #else /* !defined(CONFIG_X86_64) */
     if ( supervisor_mode_kernel && cpu_has_sep )
         wrmsr(MSR_IA32_SYSENTER_ESP, &init_tss[smp_processor_id()].esp1, 0);

[-- Attachment #2: 32-on-64-s3.patch --]
[-- Type: application/octet-stream, Size: 1360 bytes --]

diff -r e152892a0b1e xen/arch/x86/acpi/suspend.c
--- a/xen/arch/x86/acpi/suspend.c	Tue Mar 31 10:21:55 2009 +0800
+++ b/xen/arch/x86/acpi/suspend.c	Tue Mar 31 19:32:41 2009 +0800
@@ -16,6 +16,7 @@
 
 #if defined(CONFIG_X86_64)
 static unsigned long saved_lstar, saved_cstar;
+static unsigned long saved_sysenter_esp, saved_sysenter_eip;
 #endif
 
 void save_rest_processor_state(void)
@@ -26,6 +27,11 @@
 #if defined(CONFIG_X86_64)
     rdmsrl(MSR_CSTAR, saved_cstar);
     rdmsrl(MSR_LSTAR, saved_lstar);
+    if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
+    {
+        rdmsrl(MSR_IA32_SYSENTER_ESP, saved_sysenter_esp);
+        rdmsrl(MSR_IA32_SYSENTER_EIP, saved_sysenter_eip);
+    }
 #endif
 }
 
@@ -41,6 +47,14 @@
     wrmsrl(MSR_CSTAR, saved_cstar);
     wrmsr(MSR_STAR, 0, (FLAT_RING3_CS32<<16) | __HYPERVISOR_CS);
     wrmsr(MSR_SYSCALL_MASK, EF_VM|EF_RF|EF_NT|EF_DF|EF_IE|EF_TF, 0U);    
+
+    if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
+    {
+        /* Recover sysenter MSRs */
+        wrmsrl(MSR_IA32_SYSENTER_ESP, saved_sysenter_esp);
+        wrmsrl(MSR_IA32_SYSENTER_EIP, saved_sysenter_eip);
+        wrmsr(MSR_IA32_SYSENTER_CS, __HYPERVISOR_CS, 0);
+    }
 #else /* !defined(CONFIG_X86_64) */
     if ( supervisor_mode_kernel && cpu_has_sep )
         wrmsr(MSR_IA32_SYSENTER_ESP, &init_tss[smp_processor_id()].esp1, 0);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel