# Commit 10f969150025498fe27d985f9021a68f8c7acc31
# Date 2013-06-04 17:23:11 +0200
# Author Jan Beulich <jbeulich@suse.com>
# Committer Jan Beulich <jbeulich@suse.com>
x86: preserve FPU selectors for 32-bit guest code

Doing {,F}X{SAVE,RSTOR} unconditionally with 64-bit operand size leads
to the selector values associated with the last instruction/data
pointers getting lost. This, besides being inconsistent and not
compatible with native hardware behavior especially for 32-bit guests,
leads to bug checks in 32-bit Windows when running with "Driver
verifier" (see e.g. http://support.microsoft.com/kb/244617).

In a first try I made the code figure out the current guest mode, but
that has the disadvantage of only taking care of the issue when the
guest executes in the mode for which the state currently is (i.e.
namely not in the case of a 64-bit guest running a 32-bit application,
but being in kernle [64-bit] mode).

The solution presented here is to conditionally execute an auxiliary
FNSTENV and use the selectors from there.

In either case the determined word size gets stored in the last byte
of the FPU/SSE save area, which is available for software use (and I
verified is being cleared to zero by all versions of Xen, i.e. will not
present a problem when migrating guests from older to newer hosts), and
evaluated for determining the operand size {,F}XRSTOR is to be issued
with.

Note that I did check whether using a second FXSAVE or a partial second
XSAVE would be faster than FNSTENV - neither on my Westmere (FXSAVE)
nor on my Romley (XSAVE) they are.

I was really tempted to use branches into the middle of instructions
(i.e. past the REX64 prefixes) here, as that would have allowed to
collapse the otherwise identical fault recovery blocks. I stayed away
from doing so just because I expect others to dislike such slightly
subtle/tricky code.

Reported-by: Ben Guthro <Benjamin.Guthro@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Keir Fraser <keir@xen.org>
Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1533,7 +1533,7 @@ long arch_do_domctl(
             }
             offset += sizeof(v->arch.xcr0_accum);
             if ( copy_to_guest_offset(domctl->u.vcpuextstate.buffer,
-                                      offset, v->arch.xsave_area,
+                                      offset, (void *)v->arch.xsave_area,
                                       xsave_cntxt_size) )
             {
                 ret = -EFAULT;
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -18,28 +18,6 @@
 
 static bool_t __read_mostly cpu_has_xsaveopt;
 
-static void xsave(struct vcpu *v)
-{
-    struct xsave_struct *ptr = v->arch.xsave_area;
-
-    asm volatile (
-        ".byte " REX_PREFIX "0x0f,0xae,0x27"
-        :
-        : "a" (-1), "d" (-1), "D"(ptr)
-        : "memory" );
-}
-
-static void xsaveopt(struct vcpu *v)
-{
-    struct xsave_struct *ptr = v->arch.xsave_area;
-
-    asm volatile (
-        ".byte " REX_PREFIX "0x0f,0xae,0x37"
-        :
-        : "a" (-1), "d" (-1), "D"(ptr)
-        : "memory" );
-}
-
 static void xrstor(struct vcpu *v)
 {
     struct xsave_struct *ptr = v->arch.xsave_area;
@@ -64,20 +42,44 @@ static void xrstor(struct vcpu *v)
      * possibility, which may occur if the block was passed to us by control
      * tools or through VCPUOP_initialise, by silently clearing the block.
      */
-    asm volatile ( "1: .byte " REX_PREFIX "0x0f,0xae,0x2f\n"
-                   ".section .fixup,\"ax\"\n"
-                   "2: mov %4,%%ecx       \n"
-                   "   xor %1,%1          \n"
-                   "   rep stosb          \n"
-                   "   lea %3,%0          \n"
-                   "   dec %1             \n"
-                   "   jmp 1b             \n"
-                   ".previous             \n"
-                   _ASM_EXTABLE(1b, 2b)
-                   : "+&D" (ptr)
-                   : "a" (-1), "d" (-1), "m" (*ptr),
-                     "m" (xsave_cntxt_size)
-                   : "ecx" );
+    switch ( __builtin_expect(ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET],
+                              sizeof(long)) )
+    {
+    default:
+#ifdef CONFIG_X86_64
+        asm volatile ( "1: .byte 0x48,0x0f,0xae,0x2f\n"
+                       ".section .fixup,\"ax\"      \n"
+                       "2: mov %4,%%ecx             \n"
+                       "   xor %1,%1                \n"
+                       "   rep stosb                \n"
+                       "   lea %3,%0                \n"
+                       "   dec %1                   \n"
+                       "   jmp 1b                   \n"
+                       ".previous                   \n"
+                       _ASM_EXTABLE(1b, 2b)
+                       : "+&D" (ptr)
+                       : "a" (-1), "d" (-1), "m" (*ptr),
+                         "m" (xsave_cntxt_size)
+                       : "ecx" );
+        break;
+    case 4: case 2:
+#endif
+        asm volatile ( "1: .byte 0x0f,0xae,0x2f\n"
+                       ".section .fixup,\"ax\" \n"
+                       "2: mov %4,%%ecx        \n"
+                       "   xor %1,%1           \n"
+                       "   rep stosb           \n"
+                       "   lea %3,%0           \n"
+                       "   dec %1              \n"
+                       "   jmp 1b              \n"
+                       ".previous              \n"
+                       _ASM_EXTABLE(1b, 2b)
+                       : "+&D" (ptr)
+                       : "a" (-1), "d" (-1), "m" (*ptr),
+                         "m" (xsave_cntxt_size)
+                       : "ecx" );
+        break;
+    }
 }
 
 static void load_mxcsr(unsigned long val)
@@ -132,7 +134,8 @@ static void init_fpu(void)
 void save_init_fpu(struct vcpu *v)
 {
     unsigned long cr0;
-    char *fpu_ctxt;
+    typeof(v->arch.xsave_area->fpu_sse) *fpu_ctxt;
+    int word_size = cpu_has_fpu_sel ? sizeof(long) : 0;
 
     if ( !v->fpu_dirtied )
         return;
@@ -140,7 +143,7 @@ void save_init_fpu(struct vcpu *v)
     ASSERT(!is_idle_vcpu(v));
 
     cr0 = read_cr0();
-    fpu_ctxt = v->arch.guest_context.fpu_ctxt.x;
+    fpu_ctxt = (void *)&v->arch.guest_context.fpu_ctxt;
 
     /* This can happen, if a paravirtualised guest OS has set its CR0.TS. */
     if ( cr0 & X86_CR0_TS )
@@ -148,35 +151,106 @@ void save_init_fpu(struct vcpu *v)
 
     if ( xsave_enabled(v) )
     {
+        struct xsave_struct *ptr = v->arch.xsave_area;
+
         /* XCR0 normally represents what guest OS set. In case of Xen itself,
          * we set all accumulated feature mask before doing save/restore.
          */
         set_xcr0(v->arch.xcr0_accum);
-        if ( cpu_has_xsaveopt )
-            xsaveopt(v);
+
+#ifdef CONFIG_X86_64
+        if ( word_size <= 0 || !is_pv_32bit_vcpu(v) )
+        {
+            if ( cpu_has_xsaveopt )
+                asm volatile ( ".byte 0x48,0x0f,0xae,0x37"
+                               : "=m" (*ptr)
+                               : "a" (-1), "d" (-1), "D" (ptr) );
+            else
+                asm volatile ( ".byte 0x48,0x0f,0xae,0x27"
+                               : "=m" (*ptr)
+                               : "a" (-1), "d" (-1), "D" (ptr) );
+
+            /*
+             * AMD CPUs don't save/restore FDP/FIP/FOP unless an exception
+             * is pending.
+             */
+            if ( !(ptr->fpu_sse.fsw & 0x0080) &&
+                 boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
+                word_size = 0;
+
+            if ( word_size > 0 &&
+                 !((ptr->fpu_sse.fip.addr | ptr->fpu_sse.fdp.addr) >> 32) )
+            {
+                struct ix87_env fpu_env;
+
+                asm volatile ( "fnstenv %0" : "=m" (fpu_env) );
+                ptr->fpu_sse.fip.sel = fpu_env.fcs;
+                ptr->fpu_sse.fdp.sel = fpu_env.fds;
+                word_size = 4;
+            }
+        }
         else
-            xsave(v);
+#endif
+        {
+            if ( cpu_has_xsaveopt )
+                asm volatile ( ".byte 0x0f,0xae,0x37"
+                               : "=m" (*ptr)
+                               : "a" (-1), "d" (-1), "D" (ptr) );
+            else
+                asm volatile ( ".byte 0x0f,0xae,0x27"
+                               : "=m" (*ptr)
+                               : "a" (-1), "d" (-1), "D" (ptr) );
+            word_size = 4;
+        }
+        if ( word_size >= 0 )
+            ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = word_size;
+
         set_xcr0(v->arch.xcr0);
     }
     else if ( cpu_has_fxsr )
     {
-#ifdef __i386__
-        asm volatile (
-            "fxsave %0"
-            : "=m" (*fpu_ctxt) );
-#else /* __x86_64__ */
-        /*
-         * The only way to force fxsaveq on a wide range of gas versions. On 
-         * older versions the rex64 prefix works only if we force an
-         * addressing mode that doesn't require extended registers.
-         */
-        asm volatile (
-            REX64_PREFIX "fxsave (%1)"
-            : "=m" (*fpu_ctxt) : "cdaSDb" (fpu_ctxt) );
+#ifdef __x86_64__
+        if ( !is_pv_32bit_vcpu(v) )
+        {
+            /*
+             * The only way to force fxsaveq on a wide range of gas versions.
+             * On older versions the rex64 prefix works only if we force an
+             * addressing mode that doesn't require extended registers.
+             */
+            asm volatile ( REX64_PREFIX "fxsave (%1)"
+                           : "=m" (*fpu_ctxt) : "cdaSDb" (fpu_ctxt) );
+
+            /*
+             * AMD CPUs don't save/restore FDP/FIP/FOP unless an exception
+             * is pending.
+             */
+            if ( !(fpu_ctxt->fsw & 0x0080) &&
+                 boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
+                word_size = -1;
+
+            if ( word_size > 0 &&
+                 !((fpu_ctxt->fip.addr | fpu_ctxt->fdp.addr) >> 32) )
+            {
+                struct ix87_env fpu_env;
+
+                asm volatile ( "fnstenv %0" : "=m" (fpu_env) );
+                fpu_ctxt->fip.sel = fpu_env.fcs;
+                fpu_ctxt->fdp.sel = fpu_env.fds;
+                word_size = 4;
+            }
+        }
+        else
 #endif
+        {
+            asm volatile ( "fxsave %0" : "=m" (*fpu_ctxt) );
+            word_size = 4;
+        }
+
+        if ( word_size >= 0 )
+            fpu_ctxt->x[FPU_WORD_SIZE_OFFSET] = word_size;
 
         /* Clear exception flags if FSW.ES is set. */
-        if ( unlikely(fpu_ctxt[2] & 0x80) )
+        if ( unlikely(fpu_ctxt->fsw & 0x0080) )
             asm volatile ("fnclex");
 
         /*
@@ -215,34 +289,55 @@ static void restore_fpu(struct vcpu *v)
      */
     if ( cpu_has_fxsr )
     {
-        asm volatile (
-#ifdef __i386__
-            "1: fxrstor %0            \n"
-#else /* __x86_64__ */
-            /* See above for why the operands/constraints are this way. */
-            "1: " REX64_PREFIX "fxrstor (%2)\n"
-#endif
-            ".section .fixup,\"ax\"   \n"
-            "2: push %%"__OP"ax       \n"
-            "   push %%"__OP"cx       \n"
-            "   push %%"__OP"di       \n"
-            "   lea  %0,%%"__OP"di    \n"
-            "   mov  %1,%%ecx         \n"
-            "   xor  %%eax,%%eax      \n"
-            "   rep ; stosl           \n"
-            "   pop  %%"__OP"di       \n"
-            "   pop  %%"__OP"cx       \n"
-            "   pop  %%"__OP"ax       \n"
-            "   jmp  1b               \n"
-            ".previous                \n"
-            _ASM_EXTABLE(1b, 2b)
-            : 
-            : "m" (*fpu_ctxt),
-              "i" (sizeof(v->arch.guest_context.fpu_ctxt)/4)
+        switch ( __builtin_expect(fpu_ctxt[FPU_WORD_SIZE_OFFSET], sizeof(long)) )
+        {
+        default:
 #ifdef __x86_64__
-             ,"cdaSDb" (fpu_ctxt)
+            asm volatile (
+                /* See above for why the operands/constraints are this way. */
+                "1: " REX64_PREFIX "fxrstor (%2)\n"
+                ".section .fixup,\"ax\"   \n"
+                "2: push %%"__OP"ax       \n"
+                "   push %%"__OP"cx       \n"
+                "   push %%"__OP"di       \n"
+                "   mov  %2,%%"__OP"di    \n"
+                "   mov  %1,%%ecx         \n"
+                "   xor  %%eax,%%eax      \n"
+                "   rep ; stosl           \n"
+                "   pop  %%"__OP"di       \n"
+                "   pop  %%"__OP"cx       \n"
+                "   pop  %%"__OP"ax       \n"
+                "   jmp  1b               \n"
+                ".previous                \n"
+                _ASM_EXTABLE(1b, 2b)
+                :
+                : "m" (*fpu_ctxt),
+                  "i" (sizeof(v->arch.guest_context.fpu_ctxt) / 4),
+                  "cdaSDb" (fpu_ctxt) );
+            break;
+        case 4: case 2:
 #endif
-            );
+            asm volatile (
+                "1: fxrstor %0         \n"
+                ".section .fixup,\"ax\"\n"
+                "2: push %%"__OP"ax    \n"
+                "   push %%"__OP"cx    \n"
+                "   push %%"__OP"di    \n"
+                "   lea  %0,%%"__OP"di \n"
+                "   mov  %1,%%ecx      \n"
+                "   xor  %%eax,%%eax   \n"
+                "   rep ; stosl        \n"
+                "   pop  %%"__OP"di    \n"
+                "   pop  %%"__OP"cx    \n"
+                "   pop  %%"__OP"ax    \n"
+                "   jmp  1b            \n"
+                ".previous             \n"
+                _ASM_EXTABLE(1b, 2b)
+                :
+                : "m" (*fpu_ctxt),
+                  "i" (sizeof(v->arch.guest_context.fpu_ctxt) / 4) );
+            break;
+        }
     }
     else
     {
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -153,6 +153,7 @@
 #define X86_FEATURE_BMI2	(7*32+ 8) /* 2nd bit manipulation extensions */
 #define X86_FEATURE_ERMS	(7*32+ 9) /* Enhanced REP MOVSB/STOSB */
 #define X86_FEATURE_INVPCID	(7*32+10) /* Invalidate Process Context ID */
+#define X86_FEATURE_NO_FPU_SEL 	(7*32+13) /* FPU CS/DS stored as zero */
 
 #define cpu_has(c, bit)		test_bit(bit, (c)->x86_capability)
 #define boot_cpu_has(bit)	test_bit(bit, boot_cpu_data.x86_capability)
@@ -217,6 +218,7 @@
 #endif
 
 #define cpu_has_smep            boot_cpu_has(X86_FEATURE_SMEP)
+#define cpu_has_fpu_sel         (!boot_cpu_has(X86_FEATURE_NO_FPU_SEL))
 
 #define cpu_has_ffxsr           ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD) \
                                  && boot_cpu_has(X86_FEATURE_FFXSR))
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -419,7 +419,7 @@ struct arch_vcpu
      * dirtied FPU/SSE) is scheduled out we XSAVE the states here; 2) in
      * #NM handler, we XRSTOR the states we XSAVE-ed;
      */
-    void *xsave_area;
+    struct xsave_struct *xsave_area;
     uint64_t xcr0;
     /* Accumulated eXtended features mask for using XSAVE/XRESTORE by Xen
      * itself, as we can never know whether guest OS depends on content
--- a/xen/include/asm-x86/i387.h
+++ b/xen/include/asm-x86/i387.h
@@ -38,6 +38,27 @@ bool_t xsave_enabled(const struct vcpu *
 #define FCW_DEFAULT               0x037f
 #define MXCSR_DEFAULT             0x1f80
 
+/* Byte offset of the stored word size within the FXSAVE area/portion. */
+#define FPU_WORD_SIZE_OFFSET 511
+
+struct ix87_state {
+    struct ix87_env {
+        uint16_t fcw, _res0;
+        uint16_t fsw, _res1;
+        uint16_t ftw, _res2;
+        uint32_t fip;
+        uint16_t fcs;
+        uint16_t fop;
+        uint32_t fdp;
+        uint16_t fds, _res6;
+    } env;
+    struct ix87_reg {
+        uint64_t mantissa;
+        uint16_t exponent:15;
+        uint16_t sign:1;
+    } __attribute__((__packed__)) r[8];
+};
+
 struct xsave_struct
 {
     union {                                  /* FPU/MMX, SSE */
@@ -75,12 +96,6 @@ struct xsave_struct
 
 #define XCR_XFEATURE_ENABLED_MASK   0
 
-#ifdef CONFIG_X86_64
-#define REX_PREFIX "0x48, "
-#else
-#define REX_PREFIX
-#endif
-
 DECLARE_PER_CPU(uint64_t, xcr0);
 
 static inline void xsetbv(u32 index, u64 xfeatures)
