[PATCHv3 0/3] x86: workaround inability to fully restore FPU state

[PATCHv3 0/3] x86: workaround inability to fully restore FPU state

[David Vrabel]

This series extends the workaround for the inability for some x86 CPUs
to fully restore the FPU exception state (64-bit FIP/FDP and FCS/FDS).

Toolstack (or the guest) may override the default behaviour to always
do a 32-bit save/restore.

Running Microsoft's Driver Verifier continues to work in a 32-bit
Windows guest and (if HVM_PARAM_X87_FIP_WIDTH is set to 4) now works
in a 64-bit Windows guest.

Changes in v3:

- Use a random value to poison FIP when checking if the hardware
  writes it.
- Skip checking for FIP updates if 8 is always requested.
- Only update word size if FP state was requested.
- Improve comments a bit.

Changes in v2:

- Improve xsave()'s detection of whether the hardware updated FIP/FDP.
- Leave 64-bit PV guests in auto mode.
- Don't automatically set FIP width for Windows guests -- it's safer
  to leave auto-mode on and leave it up to the admin to enable the
  mode when running Driver Verifier.
- Use a HVM param to change the default.

David



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

[PATCHv3 1/3] x86/fpu: improve check for XSAVE* not writing FIP/FDP fields

[David Vrabel]

The hardware may not write the FIP/FDP fields with a XSAVE*
instruction.  e.g., with XSAVEOPT/XSAVES if the state hasn't changed
or on AMD CPUs when a floating point exception is not pending.  We
need to identify this case so we can correctly apply the check for
whether to save/restore FCS/FDS.

By poisoning FIP in the saved state we can check if the hardware
writes to this field.  The poison value is both: a) non-canonical; and
b) random with a vanishingly small probability of matching a value
written by the hardware (1 / (2^63) = 10^-19).

The poison value is fixed and thus knowable by a guest (or guest
userspace).  This could allow the guest to cause Xen to incorrectly
detect that the field has not been written.  But: a) this requires the
FIP register to be a full 64 bits internally which is not the case for
all current AMD and Intel CPUs; and b) this only allows the guest (or
a guest userspace process) to corrupt its own state (i.e., it cannot
affect the state of another guest or another user space process).

This results in smaller code with fewer branches and is more
understandable.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
v3:
- Use a random (and still non-canonical) value to poison FIP.
---
 xen/arch/x86/xstate.c | 47 +++++++++++++++++------------------------------
 1 file changed, 17 insertions(+), 30 deletions(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 4f2fb8e..6a917f8 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -263,41 +263,28 @@ void xsave(struct vcpu *v, uint64_t mask)
 
     if ( word_size <= 0 || !is_pv_32bit_vcpu(v) )
     {
-        typeof(ptr->fpu_sse.fip.sel) fcs = ptr->fpu_sse.fip.sel;
-        typeof(ptr->fpu_sse.fdp.sel) fds = ptr->fpu_sse.fdp.sel;
+        uint64_t orig_fip;
+        static const uint64_t bad_fip = 0x6a3f5c4b13a533f6;
 
-        if ( cpu_has_xsaveopt || cpu_has_xsaves )
-        {
-            /*
-             * XSAVEOPT/XSAVES may not write the FPU portion even when the
-             * respective mask bit is set. For the check further down to work
-             * we hence need to put the save image back into the state that
-             * it was in right after the previous XSAVEOPT.
-             */
-            if ( word_size > 0 &&
-                 (ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] == 4 ||
-                  ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] == 2) )
-            {
-                ptr->fpu_sse.fip.sel = 0;
-                ptr->fpu_sse.fdp.sel = 0;
-            }
-        }
+        /*
+         * FIP/FDP may not be written in some cases (e.g., if
+         * XSAVEOPT/XSAVES is used, or on AMD CPUs if an exception
+         * isn't pending).
+         *
+         * To tell if the hardware writes these fields, poison the FIP
+         * field.  The poison is both a) non-canonical; and b) random
+         * with a vanishingly small probability to match a value the
+         * hardware may write (1e-19).
+         */
+        orig_fip = ptr->fpu_sse.fip.addr;
+        ptr->fpu_sse.fip.addr = bad_fip;
 
         XSAVE("0x48,");
 
-        if ( !(mask & ptr->xsave_hdr.xstate_bv & XSTATE_FP) ||
-             /*
-              * AMD CPUs don't save/restore FDP/FIP/FOP unless an exception
-              * is pending.
-              */
-             (!(ptr->fpu_sse.fsw & 0x0080) &&
-              boot_cpu_data.x86_vendor == X86_VENDOR_AMD) )
+        /* FIP/FDP not updated? Restore the old FIP value. */
+        if ( ptr->fpu_sse.fip.addr == bad_fip )
         {
-            if ( (cpu_has_xsaveopt || cpu_has_xsaves) && word_size > 0 )
-            {
-                ptr->fpu_sse.fip.sel = fcs;
-                ptr->fpu_sse.fdp.sel = fds;
-            }
+            ptr->fpu_sse.fip.addr = orig_fip;
             return;
         }
 
-- 
2.1.4


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

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

[David Vrabel]

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>
---
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(-)

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 6a917f8..fa45ad9 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 )
+    {
+        XSAVE("0x48,");
+    }
+    else if ( fip_width == 4 )
+    {
+        XSAVE("");
+    }
+    else
     {
         uint64_t orig_fip;
         static const uint64_t bad_fip = 0x6a3f5c4b13a533f6;
@@ -288,25 +296,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;
-- 
2.1.4


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

[PATCHv3 3/3] x86/hvm: add HVM_PARAM_X87_FIP_WIDTH

[David Vrabel]

The HVM parameter HVM_PARAM_X87_FIP_WIDTH to allow tools and the guest
to adjust the width of the FIP/FDP registers to be saved/restored by
the hypervisor.  This is in case the hypervisor hueristics do not do
the right thing.

Add this parameter to the set saved during domain save/migrate.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxc/xc_sr_save_x86_hvm.c |  1 +
 xen/arch/x86/hvm/hvm.c           | 13 +++++++++++++
 xen/include/public/hvm/params.h  | 24 +++++++++++++++++++++++-
 3 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/xc_sr_save_x86_hvm.c b/tools/libxc/xc_sr_save_x86_hvm.c
index e347b3b..ba50a43 100644
--- a/tools/libxc/xc_sr_save_x86_hvm.c
+++ b/tools/libxc/xc_sr_save_x86_hvm.c
@@ -76,6 +76,7 @@ static int write_hvm_params(struct xc_sr_context *ctx)
         HVM_PARAM_VM_GENERATION_ID_ADDR,
         HVM_PARAM_IOREQ_SERVER_PFN,
         HVM_PARAM_NR_IOREQ_SERVER_PAGES,
+        HVM_PARAM_X87_FIP_WIDTH,
     };
 
     xc_interface *xch = ctx->xch;
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index fe382ce..3583c9e 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -6027,6 +6027,7 @@ static int hvm_allow_set_param(struct domain *d,
     case HVM_PARAM_VM_GENERATION_ID_ADDR:
     case HVM_PARAM_STORE_EVTCHN:
     case HVM_PARAM_CONSOLE_EVTCHN:
+    case HVM_PARAM_X87_FIP_WIDTH:
         break;
     /*
      * The following parameters must not be set by the guest
@@ -6222,6 +6223,14 @@ static int hvmop_set_param(
 
         break;
     }
+    case HVM_PARAM_X87_FIP_WIDTH:
+        if ( a.value != 0 && a.value != 4 && a.value != 8 )
+        {
+            rc = -EINVAL;
+            break;
+        }
+        d->arch.x87_fip_width = a.value;
+        break;
     }
 
     if ( rc != 0 )
@@ -6258,6 +6267,7 @@ static int hvm_allow_get_param(struct domain *d,
     case HVM_PARAM_CONSOLE_PFN:
     case HVM_PARAM_CONSOLE_EVTCHN:
     case HVM_PARAM_ALTP2M:
+    case HVM_PARAM_X87_FIP_WIDTH:
         break;
     /*
      * The following parameters must not be read by the guest
@@ -6307,6 +6317,9 @@ static int hvmop_get_param(
     case HVM_PARAM_ACPI_S_STATE:
         a.value = d->arch.hvm_domain.is_s3_suspended ? 3 : 0;
         break;
+    case HVM_PARAM_X87_FIP_WIDTH:
+        a.value = d->arch.x87_fip_width;
+        break;
     case HVM_PARAM_IOREQ_PFN:
     case HVM_PARAM_BUFIOREQ_PFN:
     case HVM_PARAM_BUFIOREQ_EVTCHN:
diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
index 81f9451..73d4718 100644
--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -210,6 +210,28 @@
 /* Boolean: Enable altp2m */
 #define HVM_PARAM_ALTP2M       35
 
-#define HVM_NR_PARAMS          36
+/*
+ * Size of the x87 FPU FIP/FDP registers that the hypervisor needs to
+ * save/restore.  This is a workaround for a hardware limitation that
+ * does not allow the full FIP/FDP and FCS/FDS to be restored.
+ *
+ * Valid values are:
+ *
+ * 8: save/restore 64-bit FIP/FDP and clear FCS/FDS (default if CPU
+ *    has FPCSDS feature).
+ *
+ * 4: save/restore 32-bit FIP/FDP, FCS/FDS, and clear upper 32-bits of
+ *    FIP/FDP.
+ *
+ * 0: allow hypervisor to choose based on the value of FIP/FDP
+ *    (default if CPU does not have FPCSDS).
+ *
+ * If FPCSDS (bit 13 in CPUID leaf 0x7, subleaf 0x0) is set, the CPU
+ * never saves FCS/FDS and this parameter should be left at the
+ * default of 8.
+ */
+#define HVM_PARAM_X87_FIP_WIDTH 36
+
+#define HVM_NR_PARAMS 37
 
 #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */
-- 
2.1.4


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

Re: [PATCHv3 2/3] x86/fpu: Add a per-domain field to set the width of FIP/FDP

[Jan Beulich]

>>> On 25.02.16 at 11:58, <david.vrabel@citrix.com> wrote:
> @@ -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 )
> +    {
> +        XSAVE("0x48,");
> +    }
> +    else if ( fip_width == 4 )
> +    {
> +        XSAVE("");
> +    }
> +    else

Both conditions would now better also check mask & XSTATE_FP,
since going these routes (and namely bypassing the FIP check, as
was done before) is fine when FP state is not being saved.

With that adjustment
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCHv3 1/3] x86/fpu: improve check for XSAVE* not writing FIP/FDP fields

[Jan Beulich]

>>> On 25.02.16 at 11:58, <david.vrabel@citrix.com> wrote:
> The hardware may not write the FIP/FDP fields with a XSAVE*
> instruction.  e.g., with XSAVEOPT/XSAVES if the state hasn't changed
> or on AMD CPUs when a floating point exception is not pending.  We
> need to identify this case so we can correctly apply the check for
> whether to save/restore FCS/FDS.
> 
> By poisoning FIP in the saved state we can check if the hardware
> writes to this field.  The poison value is both: a) non-canonical; and
> b) random with a vanishingly small probability of matching a value
> written by the hardware (1 / (2^63) = 10^-19).

The hardware by itself will always write a canonical value with
the 64-bit save variants. The case to consider really is, as said
before, that of software storing an arbitrary value there, and
for that case I don't think a how ever small probability would
make my concerns go away (or else I would have suggested
this variation of your previous approach during v2 review).

> The poison value is fixed and thus knowable by a guest (or guest
> userspace).  This could allow the guest to cause Xen to incorrectly
> detect that the field has not been written.  But: a) this requires the
> FIP register to be a full 64 bits internally which is not the case for
> all current AMD and Intel CPUs; and b) this only allows the guest (or
> a guest userspace process) to corrupt its own state (i.e., it cannot
> affect the state of another guest or another user space process).
> 
> This results in smaller code with fewer branches and is more
> understandable.
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

Pending confirmation on FIP register width by at least Intel,
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCHv3 2/3] x86/fpu: Add a per-domain field to set the width of FIP/FDP

[David Vrabel]

On 25/02/16 11:24, Jan Beulich wrote:
>>>> On 25.02.16 at 11:58, <david.vrabel@citrix.com> wrote:
>> @@ -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 )
>> +    {
>> +        XSAVE("0x48,");
>> +    }
>> +    else if ( fip_width == 4 )
>> +    {
>> +        XSAVE("");
>> +    }
>> +    else
> 
> Both conditions would now better also check mask & XSTATE_FP,
> since going these routes (and namely bypassing the FIP check, as
> was done before) is fine when FP state is not being saved.

Is this what you mean?

--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -261,7 +261,7 @@ void xsave(struct vcpu *v, uint64_t mask)
                          "=m" (*ptr), \
                          "a" (lmask), "d" (hmask), "D" (ptr))

-    if ( fip_width == 8 )
+    if ( fip_width == 8 || !(mask & XSTATE_FP) )
     {
         XSAVE("0x48,");
     }

David

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

Re: [PATCHv3 2/3] x86/fpu: Add a per-domain field to set the width of FIP/FDP

[Jan Beulich]

>>> On 25.02.16 at 12:38, <david.vrabel@citrix.com> wrote:
> On 25/02/16 11:24, Jan Beulich wrote:
>>>>> On 25.02.16 at 11:58, <david.vrabel@citrix.com> wrote:
>>> @@ -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 )
>>> +    {
>>> +        XSAVE("0x48,");
>>> +    }
>>> +    else if ( fip_width == 4 )
>>> +    {
>>> +        XSAVE("");
>>> +    }
>>> +    else
>> 
>> Both conditions would now better also check mask & XSTATE_FP,
>> since going these routes (and namely bypassing the FIP check, as
>> was done before) is fine when FP state is not being saved.
> 
> Is this what you mean?
> 
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -261,7 +261,7 @@ void xsave(struct vcpu *v, uint64_t mask)
>                           "=m" (*ptr), \
>                           "a" (lmask), "d" (hmask), "D" (ptr))
> 
> -    if ( fip_width == 8 )
> +    if ( fip_width == 8 || !(mask & XSTATE_FP) )
>      {
>          XSAVE("0x48,");
>      }

Oh, you're right, it's just one of the two that needs it.

Jan


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

Re: [PATCHv3 1/3] x86/fpu: improve check for XSAVE* not writing FIP/FDP fields

[David Vrabel]

On 25/02/16 11:32, Jan Beulich wrote:
>>>> On 25.02.16 at 11:58, <david.vrabel@citrix.com> wrote:
>> The hardware may not write the FIP/FDP fields with a XSAVE*
>> instruction.  e.g., with XSAVEOPT/XSAVES if the state hasn't changed
>> or on AMD CPUs when a floating point exception is not pending.  We
>> need to identify this case so we can correctly apply the check for
>> whether to save/restore FCS/FDS.
>>
>> By poisoning FIP in the saved state we can check if the hardware
>> writes to this field.  The poison value is both: a) non-canonical; and
>> b) random with a vanishingly small probability of matching a value
>> written by the hardware (1 / (2^63) = 10^-19).
> 
> The hardware by itself will always write a canonical value with
> the 64-bit save variants. The case to consider really is, as said
> before, that of software storing an arbitrary value there, and
> for that case I don't think a how ever small probability would
> make my concerns go away (or else I would have suggested
> this variation of your previous approach during v2 review).

Do you not appreciate how unlikely 10^-19 is?

Assuming a context switch every 1 ms the probability of a error in a
year is 3e-9.

The probability of a dinosaur killing asteroid strike in a year is about
2e-8.

I know which one I'd be worried about...

>> The poison value is fixed and thus knowable by a guest (or guest
>> userspace).  This could allow the guest to cause Xen to incorrectly
>> detect that the field has not been written.  But: a) this requires the
>> FIP register to be a full 64 bits internally which is not the case for
>> all current AMD and Intel CPUs; and b) this only allows the guest (or
>> a guest userspace process) to corrupt its own state (i.e., it cannot
>> affect the state of another guest or another user space process).
>>
>> This results in smaller code with fewer branches and is more
>> understandable.
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> 
> Pending confirmation on FIP register width by at least Intel,
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

For Intel CPUs, FIP is 48-bits internally and newer CPUs have FPCSDS and
thus we will always use the 64-bit save.

For AMD, which only writes FIP and FDP if an exception is pending, if a
guest wanted to use FIP to store an arbitrary 64-bit value (in some
future CPU) it would have to manually set an exception as pending.  Its
seems implausible that any software would actually do this.

David


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

Re: [PATCHv3 1/3] x86/fpu: improve check for XSAVE* not writing FIP/FDP fields

[Jan Beulich]

>>> On 25.02.16 at 13:18, <david.vrabel@citrix.com> wrote:
> On 25/02/16 11:32, Jan Beulich wrote:
>>>>> On 25.02.16 at 11:58, <david.vrabel@citrix.com> wrote:
>>> The hardware may not write the FIP/FDP fields with a XSAVE*
>>> instruction.  e.g., with XSAVEOPT/XSAVES if the state hasn't changed
>>> or on AMD CPUs when a floating point exception is not pending.  We
>>> need to identify this case so we can correctly apply the check for
>>> whether to save/restore FCS/FDS.
>>>
>>> By poisoning FIP in the saved state we can check if the hardware
>>> writes to this field.  The poison value is both: a) non-canonical; and
>>> b) random with a vanishingly small probability of matching a value
>>> written by the hardware (1 / (2^63) = 10^-19).
>> 
>> The hardware by itself will always write a canonical value with
>> the 64-bit save variants. The case to consider really is, as said
>> before, that of software storing an arbitrary value there, and
>> for that case I don't think a how ever small probability would
>> make my concerns go away (or else I would have suggested
>> this variation of your previous approach during v2 review).
> 
> Do you not appreciate how unlikely 10^-19 is?
> 
> Assuming a context switch every 1 ms the probability of a error in a
> year is 3e-9.
> 
> The probability of a dinosaur killing asteroid strike in a year is about
> 2e-8.
> 
> I know which one I'd be worried about...

:-)

>>> The poison value is fixed and thus knowable by a guest (or guest
>>> userspace).  This could allow the guest to cause Xen to incorrectly
>>> detect that the field has not been written.  But: a) this requires the
>>> FIP register to be a full 64 bits internally which is not the case for
>>> all current AMD and Intel CPUs; and b) this only allows the guest (or
>>> a guest userspace process) to corrupt its own state (i.e., it cannot
>>> affect the state of another guest or another user space process).
>>>
>>> This results in smaller code with fewer branches and is more
>>> understandable.
>>>
>>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>> 
>> Pending confirmation on FIP register width by at least Intel,
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> For Intel CPUs, FIP is 48-bits internally and newer CPUs have FPCSDS and
> thus we will always use the 64-bit save.

Has Intel told you (but not us), or is this just based on experiments
you did, or re-stating what I've found from experimenting?

> For AMD, which only writes FIP and FDP if an exception is pending, if a
> guest wanted to use FIP to store an arbitrary 64-bit value (in some
> future CPU) it would have to manually set an exception as pending.  Its
> seems implausible that any software would actually do this.

All of these uses of FIP/FDP are implausible, yet we're aiming at
correctly mimicking hardware behavior, allowing folks to even do
implausible things that work on bare hardware.

Jan


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

Re: [PATCHv3 1/3] x86/fpu: improve check for XSAVE* not writing FIP/FDP fields

[David Vrabel]

On 25/02/16 12:27, Jan Beulich wrote:
>>>> On 25.02.16 at 13:18, <david.vrabel@citrix.com> wrote:
>> On 25/02/16 11:32, Jan Beulich wrote:
>>>>>> On 25.02.16 at 11:58, <david.vrabel@citrix.com> wrote:
>>>> The poison value is fixed and thus knowable by a guest (or guest
>>>> userspace).  This could allow the guest to cause Xen to incorrectly
>>>> detect that the field has not been written.  But: a) this requires the
>>>> FIP register to be a full 64 bits internally which is not the case for
>>>> all current AMD and Intel CPUs; and b) this only allows the guest (or
>>>> a guest userspace process) to corrupt its own state (i.e., it cannot
>>>> affect the state of another guest or another user space process).
>>>>
>>>> This results in smaller code with fewer branches and is more
>>>> understandable.
>>>>
>>>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>>>
>>> Pending confirmation on FIP register width by at least Intel,
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>> For Intel CPUs, FIP is 48-bits internally and newer CPUs have FPCSDS and
>> thus we will always use the 64-bit save.
> 
> Has Intel told you (but not us), or is this just based on experiments
> you did, or re-stating what I've found from experimenting?

I'm just restating things already mentioned in the various threads.

>> For AMD, which only writes FIP and FDP if an exception is pending, if a
>> guest wanted to use FIP to store an arbitrary 64-bit value (in some
>> future CPU) it would have to manually set an exception as pending.  Its
>> seems implausible that any software would actually do this.
> 
> All of these uses of FIP/FDP are implausible, yet we're aiming at
> correctly mimicking hardware behavior, allowing folks to even do
> implausible things that work on bare hardware.

I think:

a) On hardware with FPCSDS, we always do a 64-bit save/restore and thus
always match the hardware behaviour.

b) On hardware without FPCSDS we /cannot/ match the hardware behaviour.
 We must have some sort of heuristic to cover the common use cases.  The
existing heuristic is /already/ inadequate since Driver Verifier
confuses it. So IMO, making the heuristic a teeny, tiny bit less precise
doesn't matter.

c) For the uncommon use cases, there is always HVM_PARAM_X87_FIP_WIDTH
to force a particular behaviour.

David

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

Re: [PATCHv3 1/3] x86/fpu: improve check for XSAVE* not writing FIP/FDP fields

[Andrew Cooper]

On 25/02/16 12:49, David Vrabel wrote:
> On 25/02/16 12:27, Jan Beulich wrote:
>>>>> On 25.02.16 at 13:18, <david.vrabel@citrix.com> wrote:
>>> On 25/02/16 11:32, Jan Beulich wrote:
>>>>>>> On 25.02.16 at 11:58, <david.vrabel@citrix.com> wrote:
>>>>> The poison value is fixed and thus knowable by a guest (or guest
>>>>> userspace).  This could allow the guest to cause Xen to incorrectly
>>>>> detect that the field has not been written.  But: a) this requires the
>>>>> FIP register to be a full 64 bits internally which is not the case for
>>>>> all current AMD and Intel CPUs; and b) this only allows the guest (or
>>>>> a guest userspace process) to corrupt its own state (i.e., it cannot
>>>>> affect the state of another guest or another user space process).
>>>>>
>>>>> This results in smaller code with fewer branches and is more
>>>>> understandable.
>>>>>
>>>>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>>>> Pending confirmation on FIP register width by at least Intel,
>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> For Intel CPUs, FIP is 48-bits internally and newer CPUs have FPCSDS and
>>> thus we will always use the 64-bit save.
>> Has Intel told you (but not us), or is this just based on experiments
>> you did, or re-stating what I've found from experimenting?
> I'm just restating things already mentioned in the various threads.
>
>>> For AMD, which only writes FIP and FDP if an exception is pending, if a
>>> guest wanted to use FIP to store an arbitrary 64-bit value (in some
>>> future CPU) it would have to manually set an exception as pending.  Its
>>> seems implausible that any software would actually do this.
>> All of these uses of FIP/FDP are implausible, yet we're aiming at
>> correctly mimicking hardware behavior, allowing folks to even do
>> implausible things that work on bare hardware.
> I think:
>
> a) On hardware with FPCSDS, we always do a 64-bit save/restore and thus
> always match the hardware behaviour.
>
> b) On hardware without FPCSDS we /cannot/ match the hardware behaviour.
>  We must have some sort of heuristic to cover the common use cases.  The
> existing heuristic is /already/ inadequate since Driver Verifier
> confuses it. So IMO, making the heuristic a teeny, tiny bit less precise
> doesn't matter.
>
> c) For the uncommon use cases, there is always HVM_PARAM_X87_FIP_WIDTH
> to force a particular behaviour.

No OS is plausibly going to hide non-IP information in FIP.

If some theoretical OS does do something like that, there is always the
override available.

~Andrew

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

Re: [PATCHv3 1/3] x86/fpu: improve check for XSAVE* not writing FIP/FDP fields

[Jan Beulich]

>>> On 25.02.16 at 14:16, <andrew.cooper3@citrix.com> wrote:
> No OS is plausibly going to hide non-IP information in FIP.
> 
> If some theoretical OS does do something like that, there is always the
> override available.

But who says it needs to be an OS to do so? User mode code could
as much (and would imo even be more likely to do so, compared to
an OS).

Jan


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

Re: [PATCHv3 1/3] x86/fpu: improve check for XSAVE* not writing FIP/FDP fields

[Andrew Cooper]

On 25/02/16 14:27, Jan Beulich wrote:
>>>> On 25.02.16 at 14:16, <andrew.cooper3@citrix.com> wrote:
>> No OS is plausibly going to hide non-IP information in FIP.
>>
>> If some theoretical OS does do something like that, there is always the
>> override available.
> But who says it needs to be an OS to do so? User mode code could
> as much (and would imo even be more likely to do so, compared to
> an OS).

Why does this matter?  We *cannot* make a perfect heuristic in this case.

The new heuristic is wrong substantially less often than the old
heuristic, so is an improvement.

~Andrew

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

Re: [PATCHv3 1/3] x86/fpu: improve check for XSAVE* not writing FIP/FDP fields

[David Vrabel]

On 25/02/16 15:07, Andrew Cooper wrote:
> On 25/02/16 14:27, Jan Beulich wrote:
>>>>> On 25.02.16 at 14:16, <andrew.cooper3@citrix.com> wrote:
>>> No OS is plausibly going to hide non-IP information in FIP.
>>>
>>> If some theoretical OS does do something like that, there is always the
>>> override available.
>> But who says it needs to be an OS to do so? User mode code could
>> as much (and would imo even be more likely to do so, compared to
>> an OS).
> 
> Why does this matter?  We *cannot* make a perfect heuristic in this case.
> 
> The new heuristic is wrong substantially less often than the old
> heuristic, so is an improvement.

Well, no.  The current one is wrong 0.01% of the time[1], and is now
wrong 0.0100000000000000001% of the time.  I would suggest that this
isn't a significant different.

Anyway, since it was trivial to reorder the series, I'm posting a v4
with this controversial patch at the end.

David

[1] Based on the number of XenServer test cases it breaks.


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

Re: [PATCHv3 1/3] x86/fpu: improve check for XSAVE* not writing FIP/FDP fields

[Tian, Kevin]

> From: Jan Beulich
> Sent: Thursday, February 25, 2016 8:28 PM
> 
> >>
> >> Pending confirmation on FIP register width by at least Intel,
> >> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> >
> > For Intel CPUs, FIP is 48-bits internally and newer CPUs have FPCSDS and
> > thus we will always use the 64-bit save.
> 
> Has Intel told you (but not us), or is this just based on experiments
> you did, or re-stating what I've found from experimenting?
> 

Still wait for an internal clarification... but could I think this
question is more for older CPUs which don't claim FPCSDS
since otherwise no such trick is required with 64bit save/restore?

Thanks
Kevin
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCHv3 1/3] x86/fpu: improve check for XSAVE* not writing FIP/FDP fields

[Jan Beulich]

>>> On 01.03.16 at 07:27, <kevin.tian@intel.com> wrote:
>>  From: Jan Beulich
>> Sent: Thursday, February 25, 2016 8:28 PM
>> 
>> >>
>> >> Pending confirmation on FIP register width by at least Intel,
>> >> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> >
>> > For Intel CPUs, FIP is 48-bits internally and newer CPUs have FPCSDS and
>> > thus we will always use the 64-bit save.
>> 
>> Has Intel told you (but not us), or is this just based on experiments
>> you did, or re-stating what I've found from experimenting?
>> 
> 
> Still wait for an internal clarification... but could I think this
> question is more for older CPUs which don't claim FPCSDS
> since otherwise no such trick is required with 64bit save/restore?

Yes, the question regards just CPUs storing non-zero selector
values. Yet with that functionality being dropped having an
impact on migration of Windows guests, I'm not actually sure you
can retain the decision to no longer store these selectors without
the OS (or hypervisor in our case) explicitly stating its consent
(via some control register bit).

Jan


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