[Xen-devel] [PATCH 0/6] x86/suspend: State cleanup

[Xen-devel] [PATCH 0/6] x86/suspend: State cleanup

[Andrew Cooper]

Andrew Cooper (6):
  x86/suspend: Clarify and improve the behaviour of do_suspend_lowlevel()
  x86/suspend: Don't bother saving %cr3, %ss or flags
  x86/suspend: Don't save unnecessary GPRs
  x86/suspend: Restore cr4 later during resume
  x86/suspend: Expand macros in wakeup_prot.S
  x86/suspend: Drop save_rest_processor_state() completely

 xen/arch/x86/acpi/suspend.c     |  55 ++--------------
 xen/arch/x86/acpi/wakeup_prot.S | 136 ++++++++++++----------------------------
 xen/arch/x86/boot/wakeup.S      |   2 +-
 3 files changed, 46 insertions(+), 147 deletions(-)

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 1/6] x86/suspend: Clarify and improve the behaviour of do_suspend_lowlevel()

[Andrew Cooper]

do_suspend_lowlevel() behaves as a function call, even when the trampoline
jumps back into the middle of it.  Discuss this property, while renaming the
far-too-generic __ret_point to s3_resume.

Optimise the calling logic for acpi_enter_sleep_state().  $3 doesn't require a
64bit write, and the function isn't variadic so doesn't need to specify zero
FPU registers in use.

In the case of an acpi_enter_sleep_state() error, we didn't actually lose
state so don't need to restore it.  Jump straight to the end.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/acpi/wakeup_prot.S | 23 ++++++++++++++++-------
 xen/arch/x86/boot/wakeup.S      |  2 +-
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/acpi/wakeup_prot.S b/xen/arch/x86/acpi/wakeup_prot.S
index 74261cb4f1..8c525a802b 100644
--- a/xen/arch/x86/acpi/wakeup_prot.S
+++ b/xen/arch/x86/acpi/wakeup_prot.S
@@ -42,15 +42,23 @@ ENTRY(do_suspend_lowlevel)
 
         call    save_rest_processor_state
 
-        mov     $3, %rdi
-        xor     %eax, %eax
-
         /* enter sleep state physically */
+        mov     $3, %edi
         call    acpi_enter_sleep_state
-        jmp     __ret_point
-
 
-ENTRY(__ret_point)
+        /* It seems we didn't suspend.  Get out of here. */
+        jmp     .Lsuspend_err
+
+        /*
+         * do_suspend_lowlevel() is arranged to behave as a regular function
+         * call, even if hardware actually goes to sleep in the middle.
+         *
+         * The trampoline re-intercepts here.  State is:
+         *  - 64bit mode
+         *
+         * Everything else, including the stack, needs restoring.
+         */
+ENTRY(s3_resume)
         lgdt    boot_gdtr(%rip)
 
         /* mmu_cr4_features contains latest cr4 setting */
@@ -92,7 +100,8 @@ ENTRY(__ret_point)
         LOAD_GREG(13)
         LOAD_GREG(14)
         LOAD_GREG(15)
-        ret 
+.Lsuspend_err:
+        ret
 
 .data
         .align 16
diff --git a/xen/arch/x86/boot/wakeup.S b/xen/arch/x86/boot/wakeup.S
index fc47721f43..c17d613b61 100644
--- a/xen/arch/x86/boot/wakeup.S
+++ b/xen/arch/x86/boot/wakeup.S
@@ -151,7 +151,7 @@ wakeup_32:
         .code64
 wakeup_64:
         /* Jump to high mappings and the higher-level wakeup code. */
-        movabs  $__ret_point, %rbx
+        movabs  $s3_resume, %rbx
         jmp     *%rbx
 
 bogus_saved_magic:
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 2/6] x86/suspend: Don't bother saving %cr3, %ss or flags

[Andrew Cooper]

The trampoline has already set up the idle pagetables (which are the correct
ones to use), and sanitised the flags state.

For %ss, __HYPERVISOR_DS64 is the correct descriptor to restore.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/acpi/wakeup_prot.S | 20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/acpi/wakeup_prot.S b/xen/arch/x86/acpi/wakeup_prot.S
index 8c525a802b..35fd7a5e9f 100644
--- a/xen/arch/x86/acpi/wakeup_prot.S
+++ b/xen/arch/x86/acpi/wakeup_prot.S
@@ -29,17 +29,10 @@ ENTRY(do_suspend_lowlevel)
         SAVE_GREG(13)
         SAVE_GREG(14)
         SAVE_GREG(15)
-        pushfq;
-        popq    SAVED_GREG(flags)
-
-        mov     %ss, REF(saved_ss)
 
         mov     %cr0, GREG(ax)
         mov     GREG(ax), REF(saved_cr0)
 
-        mov     %cr3, GREG(ax)
-        mov     GREG(ax), REF(saved_cr3)
-
         call    save_rest_processor_state
 
         /* enter sleep state physically */
@@ -55,6 +48,7 @@ ENTRY(do_suspend_lowlevel)
          *
          * The trampoline re-intercepts here.  State is:
          *  - 64bit mode
+         *  - %cr3 => idle_pg_table[]
          *
          * Everything else, including the stack, needs restoring.
          */
@@ -65,13 +59,11 @@ ENTRY(s3_resume)
         mov     REF(mmu_cr4_features), GREG(ax)
         mov     GREG(ax), %cr4
 
-        mov     REF(saved_cr3), GREG(ax)
-        mov     GREG(ax), %cr3
-
         mov     REF(saved_cr0), GREG(ax)
         mov     GREG(ax), %cr0
 
-        mov     REF(saved_ss), %ss
+        mov     $__HYPERVISOR_DS64, %eax
+        mov     %eax, %ss
         LOAD_GREG(sp)
 
         /* Reload code selector */
@@ -80,8 +72,6 @@ ENTRY(s3_resume)
         pushq   %rax
         lretq
 1:
-        pushq   SAVED_GREG(flags)
-        popfq
 
         call restore_rest_processor_state
 
@@ -109,8 +99,6 @@ ENTRY(s3_resume)
 GLOBAL(saved_magic)
         .long   0x9abcdef0
 
-saved_ss:        .word   0
-
         .align 8
 DECLARE_GREG(sp)
 DECLARE_GREG(bp)
@@ -120,7 +108,6 @@ DECLARE_GREG(cx)
 DECLARE_GREG(dx)
 DECLARE_GREG(si)
 DECLARE_GREG(di)
-DECLARE_GREG(flags)
 
 DECLARE_GREG(8)
 DECLARE_GREG(9)
@@ -132,4 +119,3 @@ DECLARE_GREG(14)
 DECLARE_GREG(15)
 
 saved_cr0:      .quad   0
-saved_cr3:      .quad   0
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 3/6] x86/suspend: Don't save unnecessary GPRs

[Andrew Cooper]

Only the callee-preserved registers need saving/restoring.  Spill them to the
stack like regular functions do.  %rsp is now the only GPR which gets stashed
in .data

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/acpi/wakeup_prot.S | 59 +++++++++--------------------------------
 1 file changed, 12 insertions(+), 47 deletions(-)

diff --git a/xen/arch/x86/acpi/wakeup_prot.S b/xen/arch/x86/acpi/wakeup_prot.S
index 35fd7a5e9f..2f6c8e18ef 100644
--- a/xen/arch/x86/acpi/wakeup_prot.S
+++ b/xen/arch/x86/acpi/wakeup_prot.S
@@ -11,24 +11,14 @@
 #define REF(x)          x(%rip)
 
 ENTRY(do_suspend_lowlevel)
+        push    %rbp
+        push    %rbx
+        push    %r12
+        push    %r13
+        push    %r14
+        push    %r15
 
         SAVE_GREG(sp)
-        SAVE_GREG(ax)
-        SAVE_GREG(bx)
-        SAVE_GREG(cx)
-        SAVE_GREG(dx)
-        SAVE_GREG(bp)
-        SAVE_GREG(si)
-        SAVE_GREG(di)
-
-        SAVE_GREG(8)     # save r8...r15
-        SAVE_GREG(9)
-        SAVE_GREG(10)
-        SAVE_GREG(11)
-        SAVE_GREG(12)
-        SAVE_GREG(13)
-        SAVE_GREG(14)
-        SAVE_GREG(15)
 
         mov     %cr0, GREG(ax)
         mov     GREG(ax), REF(saved_cr0)
@@ -75,22 +65,13 @@ ENTRY(s3_resume)
 
         call restore_rest_processor_state
 
-        LOAD_GREG(bp)
-        LOAD_GREG(ax)
-        LOAD_GREG(bx)
-        LOAD_GREG(cx)
-        LOAD_GREG(dx)
-        LOAD_GREG(si)
-        LOAD_GREG(di)
-        LOAD_GREG(8)     # save r8...r15
-        LOAD_GREG(9)
-        LOAD_GREG(10)
-        LOAD_GREG(11)
-        LOAD_GREG(12)
-        LOAD_GREG(13)
-        LOAD_GREG(14)
-        LOAD_GREG(15)
 .Lsuspend_err:
+        pop     %r15
+        pop     %r14
+        pop     %r13
+        pop     %r12
+        pop     %rbx
+        pop     %rbp
         ret
 
 .data
@@ -101,21 +82,5 @@ GLOBAL(saved_magic)
 
         .align 8
 DECLARE_GREG(sp)
-DECLARE_GREG(bp)
-DECLARE_GREG(ax)
-DECLARE_GREG(bx)
-DECLARE_GREG(cx)
-DECLARE_GREG(dx)
-DECLARE_GREG(si)
-DECLARE_GREG(di)
-
-DECLARE_GREG(8)
-DECLARE_GREG(9)
-DECLARE_GREG(10)
-DECLARE_GREG(11)
-DECLARE_GREG(12)
-DECLARE_GREG(13)
-DECLARE_GREG(14)
-DECLARE_GREG(15)
 
 saved_cr0:      .quad   0
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 4/6] x86/suspend: Restore cr4 later during resume

[Andrew Cooper]

Just like the BSP/AP paths, %cr4 is loaded with only PAE.  Defer restoring all
of %cr4 (MCE in particular) until all the system structures (IDT/TSS in
particular) have been loaded.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/acpi/suspend.c     | 3 +++
 xen/arch/x86/acpi/wakeup_prot.S | 4 ----
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/acpi/suspend.c b/xen/arch/x86/acpi/suspend.c
index c9dea67bf3..32d0f71ffd 100644
--- a/xen/arch/x86/acpi/suspend.c
+++ b/xen/arch/x86/acpi/suspend.c
@@ -43,6 +43,9 @@ void restore_rest_processor_state(void)
 {
     load_system_tables();
 
+    /* Restore full CR4 (inc MCE) now that the IDT is in place. */
+    write_cr4(mmu_cr4_features);
+
     /* Recover syscall MSRs */
     wrmsrl(MSR_LSTAR, saved_lstar);
     wrmsrl(MSR_CSTAR, saved_cstar);
diff --git a/xen/arch/x86/acpi/wakeup_prot.S b/xen/arch/x86/acpi/wakeup_prot.S
index 2f6c8e18ef..a81849fd2b 100644
--- a/xen/arch/x86/acpi/wakeup_prot.S
+++ b/xen/arch/x86/acpi/wakeup_prot.S
@@ -45,10 +45,6 @@ ENTRY(do_suspend_lowlevel)
 ENTRY(s3_resume)
         lgdt    boot_gdtr(%rip)
 
-        /* mmu_cr4_features contains latest cr4 setting */
-        mov     REF(mmu_cr4_features), GREG(ax)
-        mov     GREG(ax), %cr4
-
         mov     REF(saved_cr0), GREG(ax)
         mov     GREG(ax), %cr0
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 5/6] x86/suspend: Expand macros in wakeup_prot.S

[Andrew Cooper]

Most users have been dropped, and they do nothing but obfuscate the assembly.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/acpi/wakeup_prot.S | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)

diff --git a/xen/arch/x86/acpi/wakeup_prot.S b/xen/arch/x86/acpi/wakeup_prot.S
index a81849fd2b..0ce96e26a9 100644
--- a/xen/arch/x86/acpi/wakeup_prot.S
+++ b/xen/arch/x86/acpi/wakeup_prot.S
@@ -2,14 +2,6 @@
         .text
         .code64
 
-#define GREG(x)         %r##x
-#define SAVED_GREG(x)   saved_r##x(%rip)
-#define DECLARE_GREG(x) saved_r##x:     .quad   0
-#define SAVE_GREG(x)    movq GREG(x), SAVED_GREG(x)
-#define LOAD_GREG(x)    movq SAVED_GREG(x), GREG(x)
-
-#define REF(x)          x(%rip)
-
 ENTRY(do_suspend_lowlevel)
         push    %rbp
         push    %rbx
@@ -18,10 +10,10 @@ ENTRY(do_suspend_lowlevel)
         push    %r14
         push    %r15
 
-        SAVE_GREG(sp)
+        mov     %rsp, saved_rsp(%rip)
 
-        mov     %cr0, GREG(ax)
-        mov     GREG(ax), REF(saved_cr0)
+        mov     %cr0, %rax
+        mov     %rax, saved_cr0(%rip)
 
         call    save_rest_processor_state
 
@@ -45,12 +37,12 @@ ENTRY(do_suspend_lowlevel)
 ENTRY(s3_resume)
         lgdt    boot_gdtr(%rip)
 
-        mov     REF(saved_cr0), GREG(ax)
-        mov     GREG(ax), %cr0
+        mov     saved_cr0(%rip), %rax
+        mov     %rax, %cr0
 
         mov     $__HYPERVISOR_DS64, %eax
         mov     %eax, %ss
-        LOAD_GREG(sp)
+        mov     saved_rsp(%rip), %rsp
 
         /* Reload code selector */
         pushq   $__HYPERVISOR_CS
@@ -73,10 +65,8 @@ ENTRY(s3_resume)
 .data
         .align 16
 
+saved_rsp:      .quad   0
+saved_cr0:      .quad   0
+
 GLOBAL(saved_magic)
         .long   0x9abcdef0
-
-        .align 8
-DECLARE_GREG(sp)
-
-saved_cr0:      .quad   0
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 6/6] x86/suspend: Drop save_rest_processor_state() completely

[Andrew Cooper]

Construct the system linkage MSRs using percpu_traps_init(), brining the S3
path in line with the BSP/AP path.  Restore xcr0 from the per-cpu shadow copy.

The FS/GS base values are unused in Xen context, and will be loaded
appropriately by the next vcpu context switch.

Trim the include list substantially, as most are unused.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/acpi/suspend.c     | 54 +++--------------------------------------
 xen/arch/x86/acpi/wakeup_prot.S |  2 --
 2 files changed, 3 insertions(+), 53 deletions(-)

diff --git a/xen/arch/x86/acpi/suspend.c b/xen/arch/x86/acpi/suspend.c
index 32d0f71ffd..629d117965 100644
--- a/xen/arch/x86/acpi/suspend.c
+++ b/xen/arch/x86/acpi/suspend.c
@@ -4,40 +4,8 @@
  *  Copyright (c) 2001 Patrick Mochel <mochel@osdl.org>
  */
 
-#include <xen/acpi.h>
-#include <xen/smp.h>
-#include <asm/processor.h>
-#include <asm/msr.h>
-#include <asm/debugreg.h>
-#include <asm/hvm/hvm.h>
-#include <asm/hvm/support.h>
-#include <asm/i387.h>
+#include <asm/system.h>
 #include <asm/xstate.h>
-#include <xen/hypercall.h>
-
-static unsigned long saved_lstar, saved_cstar;
-static unsigned long saved_sysenter_esp, saved_sysenter_eip;
-static unsigned long saved_fs_base, saved_gs_base, saved_kernel_gs_base;
-static uint64_t saved_xcr0;
-
-void save_rest_processor_state(void)
-{
-    saved_fs_base = rdfsbase();
-    saved_gs_base = rdgsbase();
-    rdmsrl(MSR_SHADOW_GS_BASE, saved_kernel_gs_base);
-    rdmsrl(MSR_CSTAR, saved_cstar);
-    rdmsrl(MSR_LSTAR, saved_lstar);
-
-    if ( cpu_has_sep )
-    {
-        rdmsrl(MSR_IA32_SYSENTER_ESP, saved_sysenter_esp);
-        rdmsrl(MSR_IA32_SYSENTER_EIP, saved_sysenter_eip);
-    }
-
-    if ( cpu_has_xsave )
-        saved_xcr0 = get_xcr0();
-}
-
 
 void restore_rest_processor_state(void)
 {
@@ -46,25 +14,9 @@ void restore_rest_processor_state(void)
     /* Restore full CR4 (inc MCE) now that the IDT is in place. */
     write_cr4(mmu_cr4_features);
 
-    /* Recover syscall MSRs */
-    wrmsrl(MSR_LSTAR, saved_lstar);
-    wrmsrl(MSR_CSTAR, saved_cstar);
-    wrmsrl(MSR_STAR, XEN_MSR_STAR);
-    wrmsrl(MSR_SYSCALL_MASK, XEN_SYSCALL_MASK);
-
-    wrfsbase(saved_fs_base);
-    wrgsbase(saved_gs_base);
-    wrmsrl(MSR_SHADOW_GS_BASE, saved_kernel_gs_base);
-
-    if ( cpu_has_sep )
-    {
-        /* 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);
-    }
+    percpu_traps_init();
 
-    if ( cpu_has_xsave && !set_xcr0(saved_xcr0) )
+    if ( cpu_has_xsave && !set_xcr0(get_xcr0()) )
         BUG();
 
     wrmsrl(MSR_IA32_CR_PAT, XEN_MSR_PAT);
diff --git a/xen/arch/x86/acpi/wakeup_prot.S b/xen/arch/x86/acpi/wakeup_prot.S
index 0ce96e26a9..fed114c0b2 100644
--- a/xen/arch/x86/acpi/wakeup_prot.S
+++ b/xen/arch/x86/acpi/wakeup_prot.S
@@ -15,8 +15,6 @@ ENTRY(do_suspend_lowlevel)
         mov     %cr0, %rax
         mov     %rax, saved_cr0(%rip)
 
-        call    save_rest_processor_state
-
         /* enter sleep state physically */
         mov     $3, %edi
         call    acpi_enter_sleep_state
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/6] x86/suspend: Clarify and improve the behaviour of do_suspend_lowlevel()

[Roger Pau Monné]

On Fri, Dec 13, 2019 at 07:04:31PM +0000, Andrew Cooper wrote:
> do_suspend_lowlevel() behaves as a function call, even when the trampoline
> jumps back into the middle of it.  Discuss this property, while renaming the
> far-too-generic __ret_point to s3_resume.
> 
> Optimise the calling logic for acpi_enter_sleep_state().  $3 doesn't require a
> 64bit write, and the function isn't variadic so doesn't need to specify zero
> FPU registers in use.
> 
> In the case of an acpi_enter_sleep_state() error, we didn't actually lose
> state so don't need to restore it.  Jump straight to the end.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/6] x86/suspend: Don't bother saving %cr3, %ss or flags

[Roger Pau Monné]

On Fri, Dec 13, 2019 at 07:04:32PM +0000, Andrew Cooper wrote:
> The trampoline has already set up the idle pagetables (which are the correct
> ones to use), and sanitised the flags state.

I wonder why do we have wakeup.S and wakeup_prot.S, it would be easier
to follow if it all was in the same file IMO.

> 
> For %ss, __HYPERVISOR_DS64 is the correct descriptor to restore.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/6] x86/suspend: Don't save unnecessary GPRs

[Roger Pau Monné]

On Fri, Dec 13, 2019 at 07:04:33PM +0000, Andrew Cooper wrote:
> Only the callee-preserved registers need saving/restoring.  Spill them to the
> stack like regular functions do.  %rsp is now the only GPR which gets stashed
> in .data
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/acpi/wakeup_prot.S | 59 +++++++++--------------------------------
>  1 file changed, 12 insertions(+), 47 deletions(-)
> 
> diff --git a/xen/arch/x86/acpi/wakeup_prot.S b/xen/arch/x86/acpi/wakeup_prot.S
> index 35fd7a5e9f..2f6c8e18ef 100644
> --- a/xen/arch/x86/acpi/wakeup_prot.S
> +++ b/xen/arch/x86/acpi/wakeup_prot.S
> @@ -11,24 +11,14 @@
>  #define REF(x)          x(%rip)
>  
>  ENTRY(do_suspend_lowlevel)
> +        push    %rbp
> +        push    %rbx
> +        push    %r12
> +        push    %r13
> +        push    %r14
> +        push    %r15

I was expecting Xen had a macro for this (and the restore
counterpart), but I haven't found any (neither any other places where
it would be useful).

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/6] x86/suspend: Don't bother saving %cr3, %ss or flags

[Andrew Cooper]

On 17/12/2019 11:52, Roger Pau Monné wrote:
> On Fri, Dec 13, 2019 at 07:04:32PM +0000, Andrew Cooper wrote:
>> The trampoline has already set up the idle pagetables (which are the correct
>> ones to use), and sanitised the flags state.
> I wonder why do we have wakeup.S and wakeup_prot.S, it would be easier
> to follow if it all was in the same file IMO.

wakeup.S is the 16bit entry point, and lives in the trampoline below 1M.

wakeup_prot.S is a bit of logic which lives in the main hypervisor.

The naming could probably do with some improvement, but they can't
feasibly be part of the same file.

>
>> For %ss, __HYPERVISOR_DS64 is the correct descriptor to restore.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks,

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/6] x86/suspend: Don't save unnecessary GPRs

[Andrew Cooper]

On 17/12/2019 12:04, Roger Pau Monné wrote:
>> diff --git a/xen/arch/x86/acpi/wakeup_prot.S b/xen/arch/x86/acpi/wakeup_prot.S
>> index 35fd7a5e9f..2f6c8e18ef 100644
>> --- a/xen/arch/x86/acpi/wakeup_prot.S
>> +++ b/xen/arch/x86/acpi/wakeup_prot.S
>> @@ -11,24 +11,14 @@
>>  #define REF(x)          x(%rip)
>>  
>>  ENTRY(do_suspend_lowlevel)
>> +        push    %rbp
>> +        push    %rbx
>> +        push    %r12
>> +        push    %r13
>> +        push    %r14
>> +        push    %r15
> I was expecting Xen had a macro for this (and the restore
> counterpart), but I haven't found any (neither any other places where
> it would be useful).

We have macros for saving and restoring all GPRs as part of an
exception, but this is just regular function prologue/epilogue logic
(which happens to be hand written asm because this function isn't quite
a regular function).

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/6] x86/suspend: Restore cr4 later during resume

[Roger Pau Monné]

On Fri, Dec 13, 2019 at 07:04:34PM +0000, Andrew Cooper wrote:
> Just like the BSP/AP paths, %cr4 is loaded with only PAE.  Defer restoring all
> of %cr4 (MCE in particular) until all the system structures (IDT/TSS in
> particular) have been loaded.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/6] x86/suspend: Don't save unnecessary GPRs

[Roger Pau Monné]

On Tue, Dec 17, 2019 at 12:10:55PM +0000, Andrew Cooper wrote:
> On 17/12/2019 12:04, Roger Pau Monné wrote:
> >> diff --git a/xen/arch/x86/acpi/wakeup_prot.S b/xen/arch/x86/acpi/wakeup_prot.S
> >> index 35fd7a5e9f..2f6c8e18ef 100644
> >> --- a/xen/arch/x86/acpi/wakeup_prot.S
> >> +++ b/xen/arch/x86/acpi/wakeup_prot.S
> >> @@ -11,24 +11,14 @@
> >>  #define REF(x)          x(%rip)
> >>  
> >>  ENTRY(do_suspend_lowlevel)
> >> +        push    %rbp
> >> +        push    %rbx
> >> +        push    %r12
> >> +        push    %r13
> >> +        push    %r14
> >> +        push    %r15
> > I was expecting Xen had a macro for this (and the restore
> > counterpart), but I haven't found any (neither any other places where
> > it would be useful).
> 
> We have macros for saving and restoring all GPRs as part of an
> exception, but this is just regular function prologue/epilogue logic
> (which happens to be hand written asm because this function isn't quite
> a regular function).

Yes, I've found SAVE_ALL, but as you say that's overkill (and it's
partly what you are trying to avoid here).

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/6] x86/suspend: Don't bother saving %cr3, %ss or flags

[Roger Pau Monné]

On Tue, Dec 17, 2019 at 12:06:01PM +0000, Andrew Cooper wrote:
> On 17/12/2019 11:52, Roger Pau Monné wrote:
> > On Fri, Dec 13, 2019 at 07:04:32PM +0000, Andrew Cooper wrote:
> >> The trampoline has already set up the idle pagetables (which are the correct
> >> ones to use), and sanitised the flags state.
> > I wonder why do we have wakeup.S and wakeup_prot.S, it would be easier
> > to follow if it all was in the same file IMO.
> 
> wakeup.S is the 16bit entry point, and lives in the trampoline below 1M.
> 
> wakeup_prot.S is a bit of logic which lives in the main hypervisor.
> 
> The naming could probably do with some improvement, but they can't
> feasibly be part of the same file.

Hm, I'm not sure I follow. Isn't this trampoline copied by Xen in a
suitable position below the 1M boundary, and hence could use symbols
in order to figure out which part to copy?

Ie: both the low and the high part could live in the same file as long
as Xen knows how to differentiate those and which chunk needs
positioning below 1M?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/6] x86/suspend: Don't bother saving %cr3, %ss or flags

[Andrew Cooper]

On 17/12/2019 12:18, Roger Pau Monné wrote:
> On Tue, Dec 17, 2019 at 12:06:01PM +0000, Andrew Cooper wrote:
>> On 17/12/2019 11:52, Roger Pau Monné wrote:
>>> On Fri, Dec 13, 2019 at 07:04:32PM +0000, Andrew Cooper wrote:
>>>> The trampoline has already set up the idle pagetables (which are the correct
>>>> ones to use), and sanitised the flags state.
>>> I wonder why do we have wakeup.S and wakeup_prot.S, it would be easier
>>> to follow if it all was in the same file IMO.
>> wakeup.S is the 16bit entry point, and lives in the trampoline below 1M.
>>
>> wakeup_prot.S is a bit of logic which lives in the main hypervisor.
>>
>> The naming could probably do with some improvement, but they can't
>> feasibly be part of the same file.
> Hm, I'm not sure I follow. Isn't this trampoline copied by Xen in a
> suitable position below the 1M boundary, and hence could use symbols
> in order to figure out which part to copy?
>
> Ie: both the low and the high part could live in the same file as long
> as Xen knows how to differentiate those and which chunk needs
> positioning below 1M?

There is one trampoline.S (and trampoline.o) which gathers together
various files (including wakeup.S) to construct the trampoline.

It is not something which can be constructed simply by putting code/data
in the requisite sections.  There are two main entrypoints, one with a
4k alignment requirement, one with 16 byte alignment, and we split the
trampoline into two parts - one which is BSP-only and is several pages
in size, and one which is post-boot which is only a single page.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 5/6] x86/suspend: Expand macros in wakeup_prot.S

[Roger Pau Monné]

On Fri, Dec 13, 2019 at 07:04:35PM +0000, Andrew Cooper wrote:
> Most users have been dropped, and they do nothing but obfuscate the assembly.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 6/6] x86/suspend: Drop save_rest_processor_state() completely

[Roger Pau Monné]

On Fri, Dec 13, 2019 at 07:04:36PM +0000, Andrew Cooper wrote:
> Construct the system linkage MSRs using percpu_traps_init(), brining the S3
> path in line with the BSP/AP path.  Restore xcr0 from the per-cpu shadow copy.
> 
> The FS/GS base values are unused in Xen context, and will be loaded
> appropriately by the next vcpu context switch.
> 
> Trim the include list substantially, as most are unused.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/acpi/suspend.c     | 54 +++--------------------------------------
>  xen/arch/x86/acpi/wakeup_prot.S |  2 --
>  2 files changed, 3 insertions(+), 53 deletions(-)
> 
> diff --git a/xen/arch/x86/acpi/suspend.c b/xen/arch/x86/acpi/suspend.c
> index 32d0f71ffd..629d117965 100644
> --- a/xen/arch/x86/acpi/suspend.c
> +++ b/xen/arch/x86/acpi/suspend.c
> @@ -4,40 +4,8 @@
>   *  Copyright (c) 2001 Patrick Mochel <mochel@osdl.org>
>   */
>  
> -#include <xen/acpi.h>
> -#include <xen/smp.h>
> -#include <asm/processor.h>
> -#include <asm/msr.h>
> -#include <asm/debugreg.h>
> -#include <asm/hvm/hvm.h>
> -#include <asm/hvm/support.h>
> -#include <asm/i387.h>
> +#include <asm/system.h>
>  #include <asm/xstate.h>
> -#include <xen/hypercall.h>
> -
> -static unsigned long saved_lstar, saved_cstar;
> -static unsigned long saved_sysenter_esp, saved_sysenter_eip;
> -static unsigned long saved_fs_base, saved_gs_base, saved_kernel_gs_base;
> -static uint64_t saved_xcr0;
> -
> -void save_rest_processor_state(void)
> -{
> -    saved_fs_base = rdfsbase();
> -    saved_gs_base = rdgsbase();
> -    rdmsrl(MSR_SHADOW_GS_BASE, saved_kernel_gs_base);
> -    rdmsrl(MSR_CSTAR, saved_cstar);
> -    rdmsrl(MSR_LSTAR, saved_lstar);
> -
> -    if ( cpu_has_sep )
> -    {
> -        rdmsrl(MSR_IA32_SYSENTER_ESP, saved_sysenter_esp);
> -        rdmsrl(MSR_IA32_SYSENTER_EIP, saved_sysenter_eip);
> -    }
> -
> -    if ( cpu_has_xsave )
> -        saved_xcr0 = get_xcr0();
> -}
> -
>  
>  void restore_rest_processor_state(void)
>  {
> @@ -46,25 +14,9 @@ void restore_rest_processor_state(void)
>      /* Restore full CR4 (inc MCE) now that the IDT is in place. */
>      write_cr4(mmu_cr4_features);
>  
> -    /* Recover syscall MSRs */
> -    wrmsrl(MSR_LSTAR, saved_lstar);
> -    wrmsrl(MSR_CSTAR, saved_cstar);
> -    wrmsrl(MSR_STAR, XEN_MSR_STAR);
> -    wrmsrl(MSR_SYSCALL_MASK, XEN_SYSCALL_MASK);
> -
> -    wrfsbase(saved_fs_base);
> -    wrgsbase(saved_gs_base);
> -    wrmsrl(MSR_SHADOW_GS_BASE, saved_kernel_gs_base);
> -
> -    if ( cpu_has_sep )
> -    {
> -        /* 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);
> -    }
> +    percpu_traps_init();
>  
> -    if ( cpu_has_xsave && !set_xcr0(saved_xcr0) )
> +    if ( cpu_has_xsave && !set_xcr0(get_xcr0()) )
>          BUG();
>  
>      wrmsrl(MSR_IA32_CR_PAT, XEN_MSR_PAT);

Given what this functions does after this change, would it be feasible
to place such calls directly in enter_state?

AFAICT there's already some restoring done there anyway.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/6] x86/suspend: Don't bother saving %cr3, %ss or flags

[Roger Pau Monné]

On Tue, Dec 17, 2019 at 12:26:24PM +0000, Andrew Cooper wrote:
> On 17/12/2019 12:18, Roger Pau Monné wrote:
> > On Tue, Dec 17, 2019 at 12:06:01PM +0000, Andrew Cooper wrote:
> >> On 17/12/2019 11:52, Roger Pau Monné wrote:
> >>> On Fri, Dec 13, 2019 at 07:04:32PM +0000, Andrew Cooper wrote:
> >>>> The trampoline has already set up the idle pagetables (which are the correct
> >>>> ones to use), and sanitised the flags state.
> >>> I wonder why do we have wakeup.S and wakeup_prot.S, it would be easier
> >>> to follow if it all was in the same file IMO.
> >> wakeup.S is the 16bit entry point, and lives in the trampoline below 1M.
> >>
> >> wakeup_prot.S is a bit of logic which lives in the main hypervisor.
> >>
> >> The naming could probably do with some improvement, but they can't
> >> feasibly be part of the same file.
> > Hm, I'm not sure I follow. Isn't this trampoline copied by Xen in a
> > suitable position below the 1M boundary, and hence could use symbols
> > in order to figure out which part to copy?
> >
> > Ie: both the low and the high part could live in the same file as long
> > as Xen knows how to differentiate those and which chunk needs
> > positioning below 1M?
> 
> There is one trampoline.S (and trampoline.o) which gathers together
> various files (including wakeup.S) to construct the trampoline.

Oh, I see it's all included to make a single unit, and the symbols
used to mark the start and end of the trampoline chunk are defined
outside of the included file.

> It is not something which can be constructed simply by putting code/data
> in the requisite sections.  There are two main entrypoints, one with a
> 4k alignment requirement, one with 16 byte alignment, and we split the
> trampoline into two parts - one which is BSP-only and is several pages
> in size, and one which is post-boot which is only a single page.

Given the size of s3_resume I would guess there's space in that single
page to fit it, but since it doesn't need to live below the 1M
boundary it could be seen as a waste.

Anyway, leaving it as-is is fine since placing it in wakeup.S would be
a waste of space or require some restructuring of how the trampoline
code is assembled.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/6] x86/suspend: State cleanup

[Jan Beulich]

On 13.12.2019 20:04, Andrew Cooper wrote:
> Andrew Cooper (6):
>   x86/suspend: Clarify and improve the behaviour of do_suspend_lowlevel()
>   x86/suspend: Don't bother saving %cr3, %ss or flags
>   x86/suspend: Don't save unnecessary GPRs
>   x86/suspend: Restore cr4 later during resume
>   x86/suspend: Expand macros in wakeup_prot.S
>   x86/suspend: Drop save_rest_processor_state() completely
> 
>  xen/arch/x86/acpi/suspend.c     |  55 ++--------------
>  xen/arch/x86/acpi/wakeup_prot.S | 136 ++++++++++++----------------------------
>  xen/arch/x86/boot/wakeup.S      |   2 +-
>  3 files changed, 46 insertions(+), 147 deletions(-)

Based on Roger's review
Acked-by: Jan Beulich <jbeulich@suse.com>

One remark on the combination of patches 2 and 5: The loading of
the stack related registers would now seem to be a fair candidate
for using LSS (generally to be preferred over MOV-to-SS).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/6] x86/suspend: State cleanup

[Andrew Cooper]

On 17/12/2019 16:17, Jan Beulich wrote:
> On 13.12.2019 20:04, Andrew Cooper wrote:
>> Andrew Cooper (6):
>>   x86/suspend: Clarify and improve the behaviour of do_suspend_lowlevel()
>>   x86/suspend: Don't bother saving %cr3, %ss or flags
>>   x86/suspend: Don't save unnecessary GPRs
>>   x86/suspend: Restore cr4 later during resume
>>   x86/suspend: Expand macros in wakeup_prot.S
>>   x86/suspend: Drop save_rest_processor_state() completely
>>
>>  xen/arch/x86/acpi/suspend.c     |  55 ++--------------
>>  xen/arch/x86/acpi/wakeup_prot.S | 136 ++++++++++++----------------------------
>>  xen/arch/x86/boot/wakeup.S      |   2 +-
>>  3 files changed, 46 insertions(+), 147 deletions(-)
> Based on Roger's review
> Acked-by: Jan Beulich <jbeulich@suse.com>
>
> One remark on the combination of patches 2 and 5: The loading of
> the stack related registers would now seem to be a fair candidate
> for using LSS (generally to be preferred over MOV-to-SS).

Well... You've just fixed c/s ffa21ea5303 in the emulator, and it
demonstrates why LSS can't be used.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/6] x86/suspend: State cleanup

[Jan Beulich]

On 17.12.2019 17:33, Andrew Cooper wrote:
> On 17/12/2019 16:17, Jan Beulich wrote:
>> On 13.12.2019 20:04, Andrew Cooper wrote:
>>> Andrew Cooper (6):
>>>   x86/suspend: Clarify and improve the behaviour of do_suspend_lowlevel()
>>>   x86/suspend: Don't bother saving %cr3, %ss or flags
>>>   x86/suspend: Don't save unnecessary GPRs
>>>   x86/suspend: Restore cr4 later during resume
>>>   x86/suspend: Expand macros in wakeup_prot.S
>>>   x86/suspend: Drop save_rest_processor_state() completely
>>>
>>>  xen/arch/x86/acpi/suspend.c     |  55 ++--------------
>>>  xen/arch/x86/acpi/wakeup_prot.S | 136 ++++++++++++----------------------------
>>>  xen/arch/x86/boot/wakeup.S      |   2 +-
>>>  3 files changed, 46 insertions(+), 147 deletions(-)
>> Based on Roger's review
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>
>> One remark on the combination of patches 2 and 5: The loading of
>> the stack related registers would now seem to be a fair candidate
>> for using LSS (generally to be preferred over MOV-to-SS).
> 
> Well... You've just fixed c/s ffa21ea5303 in the emulator, and it
> demonstrates why LSS can't be used.

Hmm, indeed, how did I forget? (It's really very counter-intuitive
for this insn to not be universally usable.)

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 6/6] x86/suspend: Drop save_rest_processor_state() completely

[Andrew Cooper]

On 17/12/2019 12:38, Roger Pau Monné wrote:
>> @@ -46,25 +14,9 @@ void restore_rest_processor_state(void)
>>      /* Restore full CR4 (inc MCE) now that the IDT is in place. */
>>      write_cr4(mmu_cr4_features);
>>  
>> -    /* Recover syscall MSRs */
>> -    wrmsrl(MSR_LSTAR, saved_lstar);
>> -    wrmsrl(MSR_CSTAR, saved_cstar);
>> -    wrmsrl(MSR_STAR, XEN_MSR_STAR);
>> -    wrmsrl(MSR_SYSCALL_MASK, XEN_SYSCALL_MASK);
>> -
>> -    wrfsbase(saved_fs_base);
>> -    wrgsbase(saved_gs_base);
>> -    wrmsrl(MSR_SHADOW_GS_BASE, saved_kernel_gs_base);
>> -
>> -    if ( cpu_has_sep )
>> -    {
>> -        /* 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);
>> -    }
>> +    percpu_traps_init();
>>  
>> -    if ( cpu_has_xsave && !set_xcr0(saved_xcr0) )
>> +    if ( cpu_has_xsave && !set_xcr0(get_xcr0()) )
>>          BUG();
>>  
>>      wrmsrl(MSR_IA32_CR_PAT, XEN_MSR_PAT);
> Given what this functions does after this change, would it be feasible
> to place such calls directly in enter_state?
>
> AFAICT there's already some restoring done there anyway.

Hmm - we already appear to double up CR4/EFER restoration, so there is
clearly more cleanup to do.  I'll see if I can make
restore_rest_processor_state() disappear completely.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/6] x86/suspend: State cleanup

[Andrew Cooper]

On 17/12/2019 16:39, Jan Beulich wrote:
> On 17.12.2019 17:33, Andrew Cooper wrote:
>> On 17/12/2019 16:17, Jan Beulich wrote:
>>> On 13.12.2019 20:04, Andrew Cooper wrote:
>>>> Andrew Cooper (6):
>>>>   x86/suspend: Clarify and improve the behaviour of do_suspend_lowlevel()
>>>>   x86/suspend: Don't bother saving %cr3, %ss or flags
>>>>   x86/suspend: Don't save unnecessary GPRs
>>>>   x86/suspend: Restore cr4 later during resume
>>>>   x86/suspend: Expand macros in wakeup_prot.S
>>>>   x86/suspend: Drop save_rest_processor_state() completely
>>>>
>>>>  xen/arch/x86/acpi/suspend.c     |  55 ++--------------
>>>>  xen/arch/x86/acpi/wakeup_prot.S | 136 ++++++++++++----------------------------
>>>>  xen/arch/x86/boot/wakeup.S      |   2 +-
>>>>  3 files changed, 46 insertions(+), 147 deletions(-)
>>> Based on Roger's review
>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> One remark on the combination of patches 2 and 5: The loading of
>>> the stack related registers would now seem to be a fair candidate
>>> for using LSS (generally to be preferred over MOV-to-SS).
>> Well... You've just fixed c/s ffa21ea5303 in the emulator, and it
>> demonstrates why LSS can't be used.
> Hmm, indeed, how did I forget? (It's really very counter-intuitive
> for this insn to not be universally usable.)

The differing behaviour between Intel and AMD makes L*S and call gates
totally unusable, even in situations where they might be useful.

In practice, call gates where killed by SYS{CALL,ENTER}/{RET,EXIT} being
4x faster than anything referencing the IDT/GDT, and L*S have had a
complicated history of availability even in the 32bit days.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/6] x86/suspend: State cleanup

[Jan Beulich]

On 18.12.2019 12:39, Andrew Cooper wrote:
> In practice, call gates where killed by SYS{CALL,ENTER}/{RET,EXIT} being
> 4x faster than anything referencing the IDT/GDT, and L*S have had a
> complicated history of availability even in the 32bit days.

I'm curious - what complicated history? They'd been added with the
386, and I don't recall any quirks or issues with them.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel