[PATCH] xen: fix s3 resume on AMD CPUs

[PATCH] xen: fix s3 resume on AMD CPUs

[Christoph Egger]

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


Hi,

attached patch fixes S3 resume on AMD CPUs.

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

[-- Attachment #2: xen_s3.diff --]
[-- Type: text/x-diff, Size: 3447 bytes --]

diff -r 2e522b843a21 xen/arch/x86/boot/wakeup.S
--- a/xen/arch/x86/boot/wakeup.S	Wed Jun 03 18:27:05 2009 +0100
+++ b/xen/arch/x86/boot/wakeup.S	Fri Jun 05 08:32:21 2009 +0200
@@ -4,6 +4,7 @@
 
         .align 16
 ENTRY(wakeup_start)
+        nop
         cli
         cld
 
@@ -105,11 +106,14 @@ video_mode:     .long 0
 video_flags:    .long 0
 
         .code32
+        .align 16
 
         # Now in protect mode, with paging disabled
         # Add offset for any reference to xen specific symbols
 
 wakeup_32:
+        nop
+        /* Set up segment registers and initial stack for protected mode */
         mov     $BOOT_DS, %eax
         mov     %eax, %ds
         mov     %eax, %ss
@@ -128,35 +132,55 @@ wakeup_32:
         mov     $X86_CR4_PAE, %ecx
         mov     %ecx, %cr4
 
-        /* Load pagetable base register */
-        mov     $sym_phys(idle_pg_table),%eax
-        add     bootsym_phys(trampoline_xen_phys_start),%eax
-        mov     %eax,%cr3
-
-        /* Will cpuid feature change after resume? */
-        /* Set up EFER (Extended Feature Enable Register). */
+        /*
+         * First switch to Long Mode. Do not restore the original
+         * MSR EFER value directly as enabling the NX bit without
+         * paging will result in a #GPF on AMD CPUs.
+         */
         mov     bootsym_phys(cpuid_ext_features),%edi
         test    $0x20100800,%edi /* SYSCALL/SYSRET, No Execute, Long Mode? */
-        jz      .Lskip_eferw
+        jz      .Lskip_efer1
         movl    $MSR_EFER,%ecx
         rdmsr
 #if CONFIG_PAGING_LEVELS == 4
         btsl    $_EFER_LME,%eax /* Long Mode      */
-        btsl    $_EFER_SCE,%eax /* SYSCALL/SYSRET */
+        wrmsr
 #endif
-        btl     $20,%edi        /* No Execute?    */
+
+.Lskip_efer1:
+        /* Load pagetable base register */
+        movl    $sym_phys(idle_pg_table),%eax
+        addl    bootsym_phys(trampoline_xen_phys_start),%eax
+        movl    %eax,%cr3
+
+        /* Enable paging */
+        movl    %cr0,%eax
+        orl $(X86_CR0_PG|X86_CR0_AM|X86_CR0_WP|X86_CR0_NE|X86_CR0_ET|X86_CR0_TS|X86_CR0_MP|X86_CR0_PE),%eax
+        movl    %eax,%cr0
+        /* Flush prefetch queue */
+        jmp     1f
+1:      jmp     1f
+1:
+
+        /*
+         * Load the normal system of MSR EFER.  This includes
+         * enabling the SYSCALL extension and NXE (if supported). 
+         */
+        testl   $0x20100800,%edi /* SYSCALL/SYSRET, No Execute, Long Mode? */
+        jz      .Lskip_efer2
+        movl    $MSR_EFER, %ecx
+        rdmsr
+#if CONFIG_PAGING_LEVELS == 4
+        btsl    $_EFER_SCE,%eax	/* SYSCALL/SYSRET */
+#endif
+        btl     $20,%edi        /* No Execute? */
         jnc     1f
-        btsl    $_EFER_NX,%eax  /* No Execute     */
+        btsl    $_EFER_NX,%eax  /* No Execute */
 1:      wrmsr
-.Lskip_eferw:
+.Lskip_efer2:
 
         wbinvd
 
-        mov     $0x80050033,%eax /* hi-to-lo: PG,AM,WP,NE,ET,MP,PE */
-        mov     %eax,%cr0
-        jmp     1f
-1:
-
 #if defined(__x86_64__)
 
         /* Now in compatibility mode. Long-jump to 64-bit mode */
@@ -174,8 +198,9 @@ wakeup_64:
         mov     $(__HYPERVISOR_DS64), %eax
         mov     %eax, %ds
 
-        # long jump to return point, with cs reload
-        rex64 ljmp    *ret_point(%rip)
+        /* long jump to return point, with cs reload */
+        movq    ret_point(%rip), %rbx
+        jmp     *%rbx
 
         .align 8
 ret_point:

[-- 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] xen: fix s3 resume on AMD CPUs

[Keir Fraser]

On 05/06/2009 09:00, "Christoph Egger" <Christoph.Egger@amd.com> wrote:

> attached patch fixes S3 resume on AMD CPUs.
> 
> Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>

Please make a minimal patch and explain each individual change. This one has
bizarre additional alignment, nops, double short jumps, longjmp->shortjmp
but keeps a comment which is then incorrect. Why are they needed? Is the
need documented? I don't see anything similar in Linux 2.6.27 wakeup
routines (2.6.27 is what I happen to have to hand).

I won't take your random permutations on this file of all files especially,
since it is a pain in the arse to debug when it gets broken.

 -- Keir

Re: [PATCH] xen: fix s3 resume on AMD CPUs

[Christoph Egger]

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

On Friday 05 June 2009 10:21:26 Keir Fraser wrote:
> On 05/06/2009 09:00, "Christoph Egger" <Christoph.Egger@amd.com> wrote:
> > attached patch fixes S3 resume on AMD CPUs.
> >
> > Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
>
> Please make a minimal patch and explain each individual change. This one
> has bizarre additional alignment, nops, double short jumps,
> longjmp->shortjmp but keeps a comment which is then incorrect. Why are they
> needed? Is the need documented? I don't see anything similar in Linux
> 2.6.27 wakeup routines (2.6.27 is what I happen to have to hand).
>
> I won't take your random permutations on this file of all files especially,
> since it is a pain in the arse to debug when it gets broken.

Attached patch adds a few comments, turns a long jump into a short jump
to avoid a #GP.
Reload %cs via lretq right after stack pointer has been restored.

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>

[-- Attachment #2: xen_s3.diff --]
[-- Type: text/x-diff, Size: 1550 bytes --]

diff -r 112680f620bf xen/arch/x86/acpi/wakeup_prot.S
--- a/xen/arch/x86/acpi/wakeup_prot.S	Mon Jun 08 18:23:57 2009 +0100
+++ b/xen/arch/x86/acpi/wakeup_prot.S	Wed Jun 10 15:15:16 2009 +0200
@@ -142,6 +142,12 @@ __ret_point:
         LOAD_GREG(sp)
 
 #if defined(__x86_64__)
+	/* Reload code selector */
+	pushq	$(__HYPERVISOR_CS64)
+	leaq	1f(%rip),%rax
+	pushq	%rax
+	lretq
+1:
 
         mov     REF(saved_cr8), %rax
         mov     %rax, %cr8
diff -r 112680f620bf xen/arch/x86/boot/wakeup.S
--- a/xen/arch/x86/boot/wakeup.S	Mon Jun 08 18:23:57 2009 +0100
+++ b/xen/arch/x86/boot/wakeup.S	Wed Jun 10 15:15:16 2009 +0200
@@ -110,6 +110,7 @@ video_flags:    .long 0
         # Add offset for any reference to xen specific symbols
 
 wakeup_32:
+        /* Set up segment registers and initial stack for protected mode */
         mov     $BOOT_DS, %eax
         mov     %eax, %ds
         mov     %eax, %ss
@@ -152,8 +153,10 @@ 1:      wrmsr
 
         wbinvd
 
+        /* Enable paging */
         mov     $0x80050033,%eax /* hi-to-lo: PG,AM,WP,NE,ET,MP,PE */
         mov     %eax,%cr0
+        /* Flush prefetch queue */
         jmp     1f
 1:
 
@@ -174,8 +177,11 @@ wakeup_64:
         mov     $(__HYPERVISOR_DS64), %eax
         mov     %eax, %ds
 
-        # long jump to return point, with cs reload
-        rex64 ljmp    *ret_point(%rip)
+        /* Continue with wakeup in the high-level wakeup code.
+         * Reload cs there.
+         */
+        movq    ret_point(%rip), %rbx
+        jmp     *%rbx
 
         .align 8
 ret_point:

[-- 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] xen: fix s3 resume on AMD CPUs

[Jan Beulich]

>>> Christoph Egger <Christoph.Egger@amd.com> 15.06.09 17:33 >>>
>@@ -174,8 +177,11 @@ wakeup_64:
>         mov     $(__HYPERVISOR_DS64), %eax
>         mov     %eax, %ds
> 
>-        # long jump to return point, with cs reload
>-        rex64 ljmp    *ret_point(%rip)
>+        /* Continue with wakeup in the high-level wakeup code.
>+         * Reload cs there.
>+         */
>+        movq    ret_point(%rip), %rbx
>+        jmp     *%rbx
> 
>         .align 8
> ret_point:

Why do you add a comment ("Reload cs here") here that is not in sync with
the changed code?

Also, if the sole reference ro ret_point is now a near jump, why don't you
remove the selector part of ret_point itself? Further more, why does this
need to be an indirect jump now that it's not a far jump anymore?

Jan

Re: [PATCH] xen: fix s3 resume on AMD CPUs

[Christoph Egger]

On Tuesday 16 June 2009 10:30:57 Jan Beulich wrote:
> >>> Christoph Egger <Christoph.Egger@amd.com> 15.06.09 17:33 >>>
> >
> >@@ -174,8 +177,11 @@ wakeup_64:
> >         mov     $(__HYPERVISOR_DS64), %eax
> >         mov     %eax, %ds
> >
> >-        # long jump to return point, with cs reload
> >-        rex64 ljmp    *ret_point(%rip)
> >+        /* Continue with wakeup in the high-level wakeup code.
> >+         * Reload cs there.
> >+         */
> >+        movq    ret_point(%rip), %rbx
> >+        jmp     *%rbx
> >
> >         .align 8
> > ret_point:
>
> Why do you add a comment ("Reload cs here") here that is not in sync with
> the changed code?

Re-read: You missed the "t"
:)

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH] xen: fix s3 resume on AMD CPUs

[Keir Fraser]

On 16/06/2009 09:30, "Jan Beulich" <JBeulich@novell.com> wrote:

> Also, if the sole reference ro ret_point is now a near jump, why don't you
> remove the selector part of ret_point itself? Further more, why does this
> need to be an indirect jump now that it's not a far jump anymore?

I'll be cleaning this patch up a little bit before applying it.

 - Keir