diff mbox

[PATCHv4,1/3] x86/fpu: Add a per-domain field to set the width of FIP/FDP

Message ID 1456413031-4507-2-git-send-email-david.vrabel@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

David Vrabel Feb. 25, 2016, 3:10 p.m. UTC 
The x86 architecture allows either: a) the 64-bit FIP/FDP registers to
be restored (clearing FCS and FDS); or b) the 32-bit FIP/FDP and
FCS/FDS registers to be restored (clearing the upper 32-bits).

Add a per-domain field to indicate which of these options a guest
needs.  The options are: 8, 4 or 0.  Where 0 indicates that the
hypervisor should automatically guess the FIP width by checking the
value of FIP/FDP when saving the state (this is the existing
behaviour).

The FIP width is initially automatic but is set explicitly in the
following cases:

- 32-bit PV guest: 4
- Newer CPUs that do not save FCS/FDS: 8

The x87_fip_width field is placed into an existing 1 byte hole in
struct arch_domain.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
v4:
- Skip FIP check if FP state isn't being saved.

v3:
- Skip checking for FIP updates if 8 is always requested.
- Only update word size if FP state was requested.
- Improve comments a bit.

v2:
- Retain logic to detect if XSAVE* writes FIP/FDP.
- Keep 64-bit PV guests in auto-mode.
---
 xen/arch/x86/domain.c        | 10 ++++++++++
 xen/arch/x86/i387.c          | 19 ++++++++++++-------
 xen/arch/x86/xstate.c        | 32 ++++++++++++++++++++------------
 xen/include/asm-x86/domain.h | 15 +++++++++++++++
 4 files changed, 57 insertions(+), 19 deletions(-)

Comments

Jan Beulich Feb. 26, 2016, 11:43 a.m. UTC | #1 
>>> On 25.02.16 at 16:10, <david.vrabel@citrix.com> wrote:
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -249,7 +249,7 @@ void xsave(struct vcpu *v, uint64_t mask)
>      struct xsave_struct *ptr = v->arch.xsave_area;
>      uint32_t hmask = mask >> 32;
>      uint32_t lmask = mask;
> -    int word_size = mask & XSTATE_FP ? (cpu_has_fpu_sel ? 8 : 0) : -1;
> +    unsigned int fip_width = v->domain->arch.x87_fip_width;
>  #define XSAVE(pfx) \
>          alternative_io_3(".byte " pfx "0x0f,0xae,0x27\n", /* xsave */ \
>                           ".byte " pfx "0x0f,0xae,0x37\n", /* xsaveopt */ \
> @@ -261,7 +261,15 @@ void xsave(struct vcpu *v, uint64_t mask)
>                           "=m" (*ptr), \
>                           "a" (lmask), "d" (hmask), "D" (ptr))
>  
> -    if ( word_size <= 0 || !is_pv_32bit_vcpu(v) )
> +    if ( fip_width == 8 || !(mask & XSTATE_FP) )
> +    {
> +        XSAVE("0x48,");
> +    }
> +    else if ( fip_width == 4 )
> +    {
> +        XSAVE("");
> +    }
> +    else
>      {
>          typeof(ptr->fpu_sse.fip.sel) fcs = ptr->fpu_sse.fip.sel;
>          typeof(ptr->fpu_sse.fdp.sel) fds = ptr->fpu_sse.fdp.sel;
> @@ -301,25 +309,25 @@ void xsave(struct vcpu *v, uint64_t mask)
>              return;
>          }
>  
> -        if ( word_size > 0 &&
> -             !((ptr->fpu_sse.fip.addr | ptr->fpu_sse.fdp.addr) >> 32) )
> +        /*
> +         * If the FIP/FDP[63:32] are both zero, it is safe to use the
> +         * 32-bit restore to also restore the selectors.
> +         */
> +        if ( !((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;
> +            fip_width = 4;
>          }
> -    }
> -    else
> -    {
> -        XSAVE("");
> -        word_size = 4;
> +        else
> +            fip_width = 8;
>      }
>  #undef XSAVE
> -    if ( word_size >= 0 )
> -        ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = word_size;
> +    if ( mask & XSTATE_FP )
> +        ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = fip_width;
>  }

This needed further fixing up, as two references to word_size
had been left in. Which means that patch 3 now will need to be
re-based. And please double check the adjustments.

Jan
diff mbox

Patch

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 9d43f7b..a6d721b 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -343,6 +343,8 @@  int switch_native(struct domain *d)
             hvm_set_mode(v, 8);
     }
 
+    d->arch.x87_fip_width = cpu_has_fpu_sel ? 0 : 8;
+
     return 0;
 }
 
@@ -377,6 +379,8 @@  int switch_compat(struct domain *d)
 
     domain_set_alloc_bitsize(d);
 
+    d->arch.x87_fip_width = 4;
+
     return 0;
 
  undo_and_fail:
@@ -653,6 +657,12 @@  int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     /* PV/PVH guests get an emulated PIT too for video BIOSes to use. */
     pit_init(d, cpu_khz);
 
+    /*
+     * If the FPU does not save FCS/FDS then we can always
+     * save/restore the 64-bit FIP/FDP and ignore the selectors.
+     */
+    d->arch.x87_fip_width = cpu_has_fpu_sel ? 0 : 8;
+
     return 0;
 
  fail:
diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c
index 67016c9..c29d0fa 100644
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -144,9 +144,9 @@  static inline void fpu_xsave(struct vcpu *v)
 static inline void fpu_fxsave(struct vcpu *v)
 {
     typeof(v->arch.xsave_area->fpu_sse) *fpu_ctxt = v->arch.fpu_ctxt;
-    int word_size = cpu_has_fpu_sel ? 8 : 0;
+    unsigned int fip_width = v->domain->arch.x87_fip_width;
 
-    if ( !is_pv_32bit_vcpu(v) )
+    if ( fip_width != 4 )
     {
         /*
          * The only way to force fxsaveq on a wide range of gas versions.
@@ -164,7 +164,11 @@  static inline void fpu_fxsave(struct vcpu *v)
              boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
             return;
 
-        if ( word_size > 0 &&
+        /*
+         * If the FIP/FDP[63:32] are both zero, it is safe to use the
+         * 32-bit restore to also restore the selectors.
+         */
+        if ( !fip_width &&
              !((fpu_ctxt->fip.addr | fpu_ctxt->fdp.addr) >> 32) )
         {
             struct ix87_env fpu_env;
@@ -172,17 +176,18 @@  static inline void fpu_fxsave(struct vcpu *v)
             asm volatile ( "fnstenv %0" : "=m" (fpu_env) );
             fpu_ctxt->fip.sel = fpu_env.fcs;
             fpu_ctxt->fdp.sel = fpu_env.fds;
-            word_size = 4;
+            fip_width = 4;
         }
+        else
+            fip_width = 8;
     }
     else
     {
         asm volatile ( "fxsave %0" : "=m" (*fpu_ctxt) );
-        word_size = 4;
+        fip_width = 4;
     }
 
-    if ( word_size >= 0 )
-        fpu_ctxt->x[FPU_WORD_SIZE_OFFSET] = word_size;
+    fpu_ctxt->x[FPU_WORD_SIZE_OFFSET] = fip_width;
 }
 
 /*******************************/
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 4f2fb8e..65a4f64 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -249,7 +249,7 @@  void xsave(struct vcpu *v, uint64_t mask)
     struct xsave_struct *ptr = v->arch.xsave_area;
     uint32_t hmask = mask >> 32;
     uint32_t lmask = mask;
-    int word_size = mask & XSTATE_FP ? (cpu_has_fpu_sel ? 8 : 0) : -1;
+    unsigned int fip_width = v->domain->arch.x87_fip_width;
 #define XSAVE(pfx) \
         alternative_io_3(".byte " pfx "0x0f,0xae,0x27\n", /* xsave */ \
                          ".byte " pfx "0x0f,0xae,0x37\n", /* xsaveopt */ \
@@ -261,7 +261,15 @@  void xsave(struct vcpu *v, uint64_t mask)
                          "=m" (*ptr), \
                          "a" (lmask), "d" (hmask), "D" (ptr))
 
-    if ( word_size <= 0 || !is_pv_32bit_vcpu(v) )
+    if ( fip_width == 8 || !(mask & XSTATE_FP) )
+    {
+        XSAVE("0x48,");
+    }
+    else if ( fip_width == 4 )
+    {
+        XSAVE("");
+    }
+    else
     {
         typeof(ptr->fpu_sse.fip.sel) fcs = ptr->fpu_sse.fip.sel;
         typeof(ptr->fpu_sse.fdp.sel) fds = ptr->fpu_sse.fdp.sel;
@@ -301,25 +309,25 @@  void xsave(struct vcpu *v, uint64_t mask)
             return;
         }
 
-        if ( word_size > 0 &&
-             !((ptr->fpu_sse.fip.addr | ptr->fpu_sse.fdp.addr) >> 32) )
+        /*
+         * If the FIP/FDP[63:32] are both zero, it is safe to use the
+         * 32-bit restore to also restore the selectors.
+         */
+        if ( !((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;
+            fip_width = 4;
         }
-    }
-    else
-    {
-        XSAVE("");
-        word_size = 4;
+        else
+            fip_width = 8;
     }
 #undef XSAVE
-    if ( word_size >= 0 )
-        ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = word_size;
+    if ( mask & XSTATE_FP )
+        ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = fip_width;
 }
 
 void xrstor(struct vcpu *v, uint64_t mask)
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 4fad638..7135709 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -339,6 +339,21 @@  struct arch_domain
     u8 x86_vendor;           /* CPU vendor */
     u8 x86_model;            /* CPU model */
 
+    /*
+     * The width of the FIP/FDP register in the FPU that needs to be
+     * saved/restored during a context switch.  This is needed because
+     * the FPU can either: a) restore the 64-bit FIP/FDP and clear FCS
+     * and FDS; or b) restore the 32-bit FIP/FDP (clearing the upper
+     * 32-bits of FIP/FDP) and restore FCS/FDS.
+     *
+     * Which one is needed depends on the guest.
+     *
+     * This can be either: 8, 4 or 0.  0 means auto-detect the size
+     * based on the width of FIP/FDP values that are written by the
+     * guest.
+     */
+    uint8_t x87_fip_width;
+
     cpuid_input_t *cpuids;
 
     struct PITState vpit;