[RFC PATCH 0/5] x86: workaround inability to fully restore FPU state

[RFC PATCH 0/5] 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).

32-bit save/restore is always done for Windows guests and the
toolstack may override the default behaviour.

The first 2 patches add a domctl to get/set numeric parameters.  I was
surprised there wasn't an existing mechanism for this.

This series is RFC since I haven't tested it very much.

David

[PATCHv1 1/5] domctl: Add op to get/set generic numeric parameters

[David Vrabel]

Add XEN_DOMCTL_param to get/set generic numeric parameters for a domain.
This should reduce the number of specialized domctls that need to be
added.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 xen/arch/x86/domctl.c       | 14 ++++++++++++++
 xen/common/domctl.c         | 11 +++++++++++
 xen/include/public/domctl.h | 21 +++++++++++++++++++++
 xen/include/xen/domain.h    |  3 +++
 4 files changed, 49 insertions(+)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 55aecdc..3a3ebbf 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1408,6 +1408,20 @@ void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
 #undef c
 }
 
+int arch_domctl_param(struct domain *d, uint32_t param, bool_t set,
+                      uint64_t *value)
+{
+    uint64_t new_value = *value;
+
+    switch ( param )
+    {
+    default:
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 22fa5d5..54c51ba 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -1186,6 +1186,17 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
             copyback = 1;
         break;
 
+    case XEN_DOMCTL_param:
+    {
+        uint32_t param = op->u.param.param & ~XEN_DOMCTL_PARAM_SET;
+        bool_t set = !!(op->u.param.param & XEN_DOMCTL_PARAM_SET);
+
+        ret = arch_domctl_param(d, param, set, &op->u.param.value);
+        if ( ret == 0 )
+            copyback = 1;
+        break;
+    }
+
     default:
         ret = arch_do_domctl(op, d, u_domctl);
         break;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index a934318..330b3e7 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1092,6 +1092,25 @@ struct xen_domctl_psr_cat_op {
 typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t;
 DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t);
 
+/*
+ * Get/set a per-domain numeric parameter.
+ *
+ * If bit 31 of @param is set, the original value is returned and the
+ * new value is written.  If bit 31 is clear, the value is returned.
+ *
+ * Not all parameters are valid for all architectures or domain types.
+ */
+#define XEN_DOMCTL_PARAM_SET (1u << 31)
+
+struct xen_domctl_param {
+    /* IN */
+    uint32_t param;
+    /* IN/OUT */
+    uint64_t value;
+};
+typedef struct xen_domctl_param xen_domctl_param;
+DEFINE_XEN_GUEST_HANDLE(xen_domctl_param);
+
 struct xen_domctl {
     uint32_t cmd;
 #define XEN_DOMCTL_createdomain                   1
@@ -1169,6 +1188,7 @@ struct xen_domctl {
 #define XEN_DOMCTL_monitor_op                    77
 #define XEN_DOMCTL_psr_cat_op                    78
 #define XEN_DOMCTL_soft_reset                    79
+#define XEN_DOMCTL_param                         80
 #define XEN_DOMCTL_gdbsx_guestmemio            1000
 #define XEN_DOMCTL_gdbsx_pausevcpu             1001
 #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
@@ -1231,6 +1251,7 @@ struct xen_domctl {
         struct xen_domctl_psr_cmt_op        psr_cmt_op;
         struct xen_domctl_monitor_op        monitor_op;
         struct xen_domctl_psr_cat_op        psr_cat_op;
+        struct xen_domctl_param             param;
         uint8_t                             pad[128];
     } u;
 };
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index a1a6f25..a8a6d7d 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -77,6 +77,9 @@ void arch_dump_domain_info(struct domain *d);
 
 int arch_vcpu_reset(struct vcpu *);
 
+int arch_domctl_param(struct domain *d, uint32_t param, bool_t set,
+                      uint64_t *value);
+
 extern spinlock_t vcpu_alloc_lock;
 bool_t domctl_lock_acquire(void);
 void domctl_lock_release(void);
-- 
2.1.4

[PATCHv1 2/5] tools/libxc: add xc_domain_get_param() and xc_domain_set_param()

[David Vrabel]

These are a thin wrapper around the XEN_DOMCTL_param hypercall.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 tools/libxc/include/xenctrl.h |  5 +++++
 tools/libxc/xc_domain.c       | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 42eafa4..862c748 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -812,6 +812,11 @@ int xc_domain_hvm_setcontext(xc_interface *xch,
 const char *xc_domain_get_native_protocol(xc_interface *xch,
                                           uint32_t domid);
 
+int xc_domain_get_param(xc_interface *xch, unsigned int domid,
+                        uint32_t param, uint64_t *value);
+int xc_domain_set_param(xc_interface *xch, unsigned int domid,
+                        uint32_t param, uint64_t value, uint64_t *old_value);
+
 /**
  * This function returns information about the execution context of a
  * particular vcpu of a domain.
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 4fa993a..4caa250 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -2574,6 +2574,41 @@ int xc_domain_soft_reset(xc_interface *xch,
     domctl.domain = (domid_t)domid;
     return do_domctl(xch, &domctl);
 }
+
+static int xc_domain_param(xc_interface *xch, unsigned int domid,
+                           uint32_t param, int set, uint64_t *value)
+{
+    int ret;
+    DECLARE_DOMCTL;
+
+    domctl.cmd = XEN_DOMCTL_param;
+    domctl.domain = domid;
+    domctl.u.param.param = param | (set ? XEN_DOMCTL_PARAM_SET : 0);
+    domctl.u.param.value = *value;
+
+    ret = do_domctl(xch, &domctl);
+    if ( ret == 0 )
+        *value = domctl.u.param.value;
+    return ret;
+}
+
+int xc_domain_get_param(xc_interface *xch, unsigned int domid,
+                        uint32_t param, uint64_t *value)
+{
+    return xc_domain_param(xch, domid, param, 0, value);
+}
+
+int xc_domain_set_param(xc_interface *xch, unsigned int domid,
+                        uint32_t param, uint64_t value, uint64_t *old_value)
+{
+    int ret;
+
+    ret = xc_domain_param(xch, domid, param, 1, &value);
+    if ( ret == 0 && old_value )
+        *old_value = value;
+    return ret;
+}
+
 /*
  * Local variables:
  * mode: C
-- 
2.1.4

[PATCHv1 3/5] 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
- 64-bit PV guest: 8
- Newer CPUs that do not save FCS/FDS: 8

xsave() has been simplified to not require clearing the FCS/FDS fields --
either these are updated by the XSAVE* instruction or they are not
written.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 xen/arch/x86/domain.c        | 10 ++++++++++
 xen/arch/x86/i387.c          | 15 ++++++++-------
 xen/arch/x86/xstate.c        | 43 +++++++++++--------------------------------
 xen/include/asm-x86/domain.h | 15 +++++++++++++++
 4 files changed, 44 insertions(+), 39 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 9d43f7b..a15cdf7 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 = 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 not to 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..ef08a52 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,7 @@ static inline void fpu_fxsave(struct vcpu *v)
              boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
             return;
 
-        if ( word_size > 0 &&
+        if ( !fip_width &&
              !((fpu_ctxt->fip.addr | fpu_ctxt->fdp.addr) >> 32) )
         {
             struct ix87_env fpu_env;
@@ -172,17 +172,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..c4cc071 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,28 +261,8 @@ 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 != 4 )
     {
-        typeof(ptr->fpu_sse.fip.sel) fcs = ptr->fpu_sse.fip.sel;
-        typeof(ptr->fpu_sse.fdp.sel) fds = ptr->fpu_sse.fdp.sel;
-
-        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;
-            }
-        }
-
         XSAVE("0x48,");
 
         if ( !(mask & ptr->xsave_hdr.xstate_bv & XSTATE_FP) ||
@@ -293,15 +273,14 @@ void xsave(struct vcpu *v, uint64_t mask)
              (!(ptr->fpu_sse.fsw & 0x0080) &&
               boot_cpu_data.x86_vendor == X86_VENDOR_AMD) )
         {
-            if ( (cpu_has_xsaveopt || cpu_has_xsaves) && word_size > 0 )
-            {
-                ptr->fpu_sse.fip.sel = fcs;
-                ptr->fpu_sse.fdp.sel = fds;
-            }
             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 &&
              !((ptr->fpu_sse.fip.addr | ptr->fpu_sse.fdp.addr) >> 32) )
         {
             struct ix87_env fpu_env;
@@ -309,17 +288,17 @@ void xsave(struct vcpu *v, uint64_t mask)
             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
+            fip_width = 8;
     }
     else
     {
         XSAVE("");
-        word_size = 4;
     }
 #undef XSAVE
-    if ( word_size >= 0 )
-        ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = word_size;
+    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..362fd8f 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -394,6 +394,21 @@ struct arch_domain
 
     /* Emulated devices enabled bitmap. */
     uint32_t emulation_flags;
+
+    /*
+     * 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) retstore the 32-bit FIP/FDP (clearing the upper
+     * 32-bits) 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 by the
+     * guest.
+     */
+    uint8_t x87_fip_width;
 } __cacheline_aligned;
 
 #define has_vlapic(d)      (!!((d)->arch.emulation_flags & XEN_X86_EMU_LAPIC))
-- 
2.1.4

[PATCHv1 4/5] x86/viridian: set x87 FIP width to 4 for Windows guests

[David Vrabel]

Microsoft Windows always uses a 32-bit FPU state save/restore and expects
the FCS/FDS to be saved/restored.  Ensure that for these guests, the
hypervisor does 32-bit save/restore to preserve FCS/FDS.

These guests are identified by the write to the Guest OS ID MSR.

This fixes an 0x3D BugCheck when running the Driver Verifier in 64-bit
Windows.  This BugCheck occurs because a context switch would clear
FCS/FDS and Driver Verifier would assert because the FPU state changed.

We only set FIP width if it is still in auto-mode, to allow the toolstack
to override if necessary.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 xen/arch/x86/hvm/viridian.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
index 6bd844b..fb9f044 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -148,6 +148,30 @@ static void dump_guest_os_id(const struct domain *d)
            goi->fields.service_pack, goi->fields.build_number);
 }
 
+static void set_guest_os_id(struct domain *d, uint64_t val)
+{
+    const union viridian_guest_os_id *goi;
+
+    d->arch.hvm_domain.viridian.guest_os_id.raw = val;
+    goi = &d->arch.hvm_domain.viridian.guest_os_id;
+
+    /*
+     * Microsoft Windows only saves the lower 32-bits of FIP/FDP and
+     * can get upset if the selectors are not saved/restored by the
+     * hypervisor.
+     *
+     * Only do this if the FIP width is not in auto-mode, so this
+     * heuristic can be overriden by the toolstack.
+     */
+    if ( !d->arch.x87_fip_width )
+    {
+        if ( goi->fields.vendor == 1 && goi->fields.os == 4 )
+            d->arch.x87_fip_width = 4;
+    }
+
+    dump_guest_os_id(d);
+}
+
 static void dump_hypercall(const struct domain *d)
 {
     const union viridian_hypercall_gpa *hg;
@@ -334,8 +358,7 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
     {
     case VIRIDIAN_MSR_GUEST_OS_ID:
         perfc_incr(mshv_wrmsr_osid);
-        d->arch.hvm_domain.viridian.guest_os_id.raw = val;
-        dump_guest_os_id(d);
+        set_guest_os_id(d, val);
         break;
 
     case VIRIDIAN_MSR_HYPERCALL:
-- 
2.1.4

[PATCHv1 5/5] x86/domctl: Add XEN_DOMCTL_PARAM_ARCH_X86_FIP_WIDTH parameter

[David Vrabel]

Add a parameter to allow the toolstack to set the x87 FIP width in case
the hypervisor's heuristics do the wrong thing.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 xen/arch/x86/domctl.c       | 10 ++++++++++
 xen/include/public/domctl.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 3a3ebbf..f75cd69 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1415,6 +1415,16 @@ int arch_domctl_param(struct domain *d, uint32_t param, bool_t set,
 
     switch ( param )
     {
+    case XEN_DOMCTL_PARAM_ARCH_X86_FIP_WIDTH:
+        *value = d->arch.x87_fip_width;
+        if ( set )
+        {
+            if ( new_value != 0 && new_value != 4 && new_value != 8 )
+                return -EINVAL;
+            d->arch.x87_fip_width = new_value;
+        }
+        break;
+
     default:
         return -EINVAL;
     }
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 330b3e7..26d4096 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1100,6 +1100,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t);
  *
  * Not all parameters are valid for all architectures or domain types.
  */
+#define XEN_DOMCTL_PARAM_ARCH_X86_FIP_WIDTH 0
 #define XEN_DOMCTL_PARAM_SET (1u << 31)
 
 struct xen_domctl_param {
-- 
2.1.4

Re: [PATCHv1 1/5] domctl: Add op to get/set generic numeric parameters

[Andrew Cooper]

On 18/02/16 18:52, David Vrabel wrote:
> Add XEN_DOMCTL_param to get/set generic numeric parameters for a domain.
> This should reduce the number of specialized domctls that need to be
> added.
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

As far as the code here goes, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

However, you will need to patch flask_domctl() as well, possibly
introducing a new flask bit.

~Andrew

Re: [PATCHv1 2/5] tools/libxc: add xc_domain_get_param() and xc_domain_set_param()

[Andrew Cooper]

On 18/02/16 18:52, David Vrabel wrote:
> These are a thin wrapper around the XEN_DOMCTL_param hypercall.
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[Andrew Cooper]

On 18/02/16 18:52, David Vrabel wrote:
> 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).

^ of FIP/FDP

> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index 4fad638..362fd8f 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -394,6 +394,21 @@ struct arch_domain
>  
>      /* Emulated devices enabled bitmap. */
>      uint32_t emulation_flags;
> +
> +    /*
> +     * 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) retstore the 32-bit FIP/FDP (clearing the upper
> +     * 32-bits) 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 by the
> +     * guest.
> +     */
> +    uint8_t x87_fip_width;

Can we get away with always using fpu_sse.x[FPU_WORD_SIZE_OFFSET],
instead if duplicating the information in arch_domain and risking them
getting out of sync?

VMs which have migrated in will already have some policy latched, and we
should preserve their old behaviour if possible.

~Andrew

Re: [PATCHv1 4/5] x86/viridian: set x87 FIP width to 4 for Windows guests

[Andrew Cooper]

On 18/02/16 18:52, David Vrabel wrote:
> diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
> index 6bd844b..fb9f044 100644
> --- a/xen/arch/x86/hvm/viridian.c
> +++ b/xen/arch/x86/hvm/viridian.c
> @@ -148,6 +148,30 @@ static void dump_guest_os_id(const struct domain *d)
>             goi->fields.service_pack, goi->fields.build_number);
>  }
>  
> +static void set_guest_os_id(struct domain *d, uint64_t val)
> +{
> +    const union viridian_guest_os_id *goi;
> +
> +    d->arch.hvm_domain.viridian.guest_os_id.raw = val;
> +    goi = &d->arch.hvm_domain.viridian.guest_os_id;
> +
> +    /*
> +     * Microsoft Windows only saves the lower 32-bits of FIP/FDP and
> +     * can get upset if the selectors are not saved/restored by the
> +     * hypervisor.
> +     *
> +     * Only do this if the FIP width is not in auto-mode, so this
> +     * heuristic can be overriden by the toolstack.
> +     */
> +    if ( !d->arch.x87_fip_width )
> +    {
> +        if ( goi->fields.vendor == 1 && goi->fields.os == 4 )

Are there any named parameters we could use here?

In principle, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(allowing for the potential change of x86_fip_width per the discussion
on patch 3).

> +            d->arch.x87_fip_width = 4;
> +    }
> +
> +    dump_guest_os_id(d);
> +}
> +
>  static void dump_hypercall(const struct domain *d)
>  {
>      const union viridian_hypercall_gpa *hg;

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

[David Vrabel]

On 18/02/16 19:13, Andrew Cooper wrote:
> On 18/02/16 18:52, David Vrabel wrote:
>> --- a/xen/include/asm-x86/domain.h
>> +++ b/xen/include/asm-x86/domain.h
>> @@ -394,6 +394,21 @@ struct arch_domain
>>  
>>      /* Emulated devices enabled bitmap. */
>>      uint32_t emulation_flags;
>> +
>> +    /*
>> +     * 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) retstore the 32-bit FIP/FDP (clearing the upper
>> +     * 32-bits) 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 by the
>> +     * guest.
>> +     */
>> +    uint8_t x87_fip_width;
> 
> Can we get away with always using fpu_sse.x[FPU_WORD_SIZE_OFFSET],
> instead if duplicating the information in arch_domain and risking them
> getting out of sync?

No.  Because if x87_fip_width == 0 (auto-mode) we check every save for
the correct width to use, storing the result at FPU_WORD_SIZE_OFFSET.

> VMs which have migrated in will already have some policy latched, and we
> should preserve their old behaviour if possible.

This does.  x87_fip_width is only used on save. The restore behaviour is
unchanged and is conditional on the word size written to the save state.

David

Re: [PATCHv1 5/5] x86/domctl: Add XEN_DOMCTL_PARAM_ARCH_X86_FIP_WIDTH parameter

[David Vrabel]

On 18/02/16 18:52, David Vrabel wrote:
> Add a parameter to allow the toolstack to set the x87 FIP width in case
> the hypervisor's heuristics do the wrong thing.

I think this would be better as a HVM param since: a) it only needs to
be changed for HVM guests; b) it would allow the guest agent or OS
(which may be able to make a better decision) to set it; and c) it can
be saved/restored across a migrate.

David

> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>  xen/arch/x86/domctl.c       | 10 ++++++++++
>  xen/include/public/domctl.h |  1 +
>  2 files changed, 11 insertions(+)
> 
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 3a3ebbf..f75cd69 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1415,6 +1415,16 @@ int arch_domctl_param(struct domain *d, uint32_t param, bool_t set,
>  
>      switch ( param )
>      {
> +    case XEN_DOMCTL_PARAM_ARCH_X86_FIP_WIDTH:
> +        *value = d->arch.x87_fip_width;
> +        if ( set )
> +        {
> +            if ( new_value != 0 && new_value != 4 && new_value != 8 )
> +                return -EINVAL;
> +            d->arch.x87_fip_width = new_value;
> +        }
> +        break;
> +
>      default:
>          return -EINVAL;
>      }
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 330b3e7..26d4096 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1100,6 +1100,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t);
>   *
>   * Not all parameters are valid for all architectures or domain types.
>   */
> +#define XEN_DOMCTL_PARAM_ARCH_X86_FIP_WIDTH 0
>  #define XEN_DOMCTL_PARAM_SET (1u << 31)
>  
>  struct xen_domctl_param {
> 

Re: [PATCHv1 1/5] domctl: Add op to get/set generic numeric parameters

[Jan Beulich]

>>> On 18.02.16 at 19:52, <david.vrabel@citrix.com> wrote:
> ---
>  xen/arch/x86/domctl.c       | 14 ++++++++++++++

What about ARM?

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1092,6 +1092,25 @@ struct xen_domctl_psr_cat_op {
>  typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t);
>  
> +/*
> + * Get/set a per-domain numeric parameter.
> + *
> + * If bit 31 of @param is set, the original value is returned and the
> + * new value is written.  If bit 31 is clear, the value is returned.
> + *
> + * Not all parameters are valid for all architectures or domain types.
> + */
> +#define XEN_DOMCTL_PARAM_SET (1u << 31)
> +
> +struct xen_domctl_param {
> +    /* IN */
> +    uint32_t param;
> +    /* IN/OUT */
> +    uint64_t value;

This needs to be uint64_aligned_t, and I wonder whether using
the padding field wouldn't be the more natural way for conveying
get/set intentions.

Jan

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

[Jan Beulich]

>>> On 18.02.16 at 19:52, <david.vrabel@citrix.com> wrote:
> 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
> - 64-bit PV guest: 8

The latter is wrong: 64-bit OSes may, for the benefit of compat
mode processes, use 32-bit save/restore operations.

> @@ -261,28 +261,8 @@ 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 != 4 )
>      {
> -        typeof(ptr->fpu_sse.fip.sel) fcs = ptr->fpu_sse.fip.sel;
> -        typeof(ptr->fpu_sse.fdp.sel) fds = ptr->fpu_sse.fdp.sel;
> -
> -        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;
> -            }
> -        }
> -
>          XSAVE("0x48,");
>  
>          if ( !(mask & ptr->xsave_hdr.xstate_bv & XSTATE_FP) ||
> @@ -293,15 +273,14 @@ void xsave(struct vcpu *v, uint64_t mask)
>               (!(ptr->fpu_sse.fsw & 0x0080) &&
>                boot_cpu_data.x86_vendor == X86_VENDOR_AMD) )
>          {
> -            if ( (cpu_has_xsaveopt || cpu_has_xsaves) && word_size > 0 )
> -            {
> -                ptr->fpu_sse.fip.sel = fcs;
> -                ptr->fpu_sse.fdp.sel = fds;
> -            }
>              return;

I don't see how you can validly delete all of the above code without
any replacement. Can you explain the rationale behind this?

Jan

Re: [PATCHv1 4/5] x86/viridian: set x87 FIP width to 4 for Windows guests

[Jan Beulich]

>>> On 18.02.16 at 19:52, <david.vrabel@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/viridian.c
> +++ b/xen/arch/x86/hvm/viridian.c
> @@ -148,6 +148,30 @@ static void dump_guest_os_id(const struct domain *d)
>             goi->fields.service_pack, goi->fields.build_number);
>  }
>  
> +static void set_guest_os_id(struct domain *d, uint64_t val)
> +{
> +    const union viridian_guest_os_id *goi;
> +
> +    d->arch.hvm_domain.viridian.guest_os_id.raw = val;
> +    goi = &d->arch.hvm_domain.viridian.guest_os_id;

Please flip the two lines and use goi also for the assignment
of val.

Jan

Re: [PATCHv1 5/5] x86/domctl: Add XEN_DOMCTL_PARAM_ARCH_X86_FIP_WIDTH parameter

[Jan Beulich]

>>> On 19.02.16 at 11:05, <david.vrabel@citrix.com> wrote:
> On 18/02/16 18:52, David Vrabel wrote:
>> Add a parameter to allow the toolstack to set the x87 FIP width in case
>> the hypervisor's heuristics do the wrong thing.
> 
> I think this would be better as a HVM param since: a) it only needs to
> be changed for HVM guests; b) it would allow the guest agent or OS
> (which may be able to make a better decision) to set it; and c) it can
> be saved/restored across a migrate.

I had wondered namely about a) when seeing patch 1, and I agree
with all of the points made (thus eliminating patches 1 and 2).

Jan

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

[David Vrabel]

On 19/02/16 14:08, Jan Beulich wrote:
>>>> On 18.02.16 at 19:52, <david.vrabel@citrix.com> wrote:
>> 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
>> - 64-bit PV guest: 8
> 
> The latter is wrong: 64-bit OSes may, for the benefit of compat
> mode processes, use 32-bit save/restore operations.

Ok. I'll leave PV guests as auto (unless FPCSDS feature is set).

>> @@ -261,28 +261,8 @@ 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 != 4 )
>>      {
>> -        typeof(ptr->fpu_sse.fip.sel) fcs = ptr->fpu_sse.fip.sel;
>> -        typeof(ptr->fpu_sse.fdp.sel) fds = ptr->fpu_sse.fdp.sel;
>> -
>> -        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;
>> -            }
>> -        }
>> -
>>          XSAVE("0x48,");
>>  
>>          if ( !(mask & ptr->xsave_hdr.xstate_bv & XSTATE_FP) ||
>> @@ -293,15 +273,14 @@ void xsave(struct vcpu *v, uint64_t mask)
>>               (!(ptr->fpu_sse.fsw & 0x0080) &&
>>                boot_cpu_data.x86_vendor == X86_VENDOR_AMD) )
>>          {
>> -            if ( (cpu_has_xsaveopt || cpu_has_xsaves) && word_size > 0 )
>> -            {
>> -                ptr->fpu_sse.fip.sel = fcs;
>> -                ptr->fpu_sse.fdp.sel = fds;
>> -            }
>>              return;
> 
> I don't see how you can validly delete all of the above code without
> any replacement. Can you explain the rationale behind this?

I think it is unnecessary.

If XSAVEOPT/XSAVES doesn't save the FP state, it hasn't changed since
last time and we don't need to clear and rewrite the FCS/FDS fields
since the old values are still valid.

David

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

[Jan Beulich]

>>> On 19.02.16 at 15:16, <david.vrabel@citrix.com> wrote:
> On 19/02/16 14:08, Jan Beulich wrote:
>>>>> On 18.02.16 at 19:52, <david.vrabel@citrix.com> wrote:
>>> @@ -261,28 +261,8 @@ 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 != 4 )
>>>      {
>>> -        typeof(ptr->fpu_sse.fip.sel) fcs = ptr->fpu_sse.fip.sel;
>>> -        typeof(ptr->fpu_sse.fdp.sel) fds = ptr->fpu_sse.fdp.sel;
>>> -
>>> -        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;
>>> -            }
>>> -        }
>>> -
>>>          XSAVE("0x48,");
>>>  
>>>          if ( !(mask & ptr->xsave_hdr.xstate_bv & XSTATE_FP) ||
>>> @@ -293,15 +273,14 @@ void xsave(struct vcpu *v, uint64_t mask)
>>>               (!(ptr->fpu_sse.fsw & 0x0080) &&
>>>                boot_cpu_data.x86_vendor == X86_VENDOR_AMD) )
>>>          {
>>> -            if ( (cpu_has_xsaveopt || cpu_has_xsaves) && word_size > 0 )
>>> -            {
>>> -                ptr->fpu_sse.fip.sel = fcs;
>>> -                ptr->fpu_sse.fdp.sel = fds;
>>> -            }
>>>              return;
>> 
>> I don't see how you can validly delete all of the above code without
>> any replacement. Can you explain the rationale behind this?
> 
> I think it is unnecessary.
> 
> If XSAVEOPT/XSAVES doesn't save the FP state, it hasn't changed since
> last time and we don't need to clear and rewrite the FCS/FDS fields
> since the old values are still valid.

Well, this logic fixed a particular issue, and I'd like to be sure the
deletion won't re-introduce that issue. In particular your patch
doesn't touch the place where the zero values are actually being
looked at:

+        /*
+         * 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 &&
              !((ptr->fpu_sse.fip.addr | ptr->fpu_sse.fdp.addr) >> 32) )
         {
             struct ix87_env fpu_env;

So how is this check going to fulfill its purpose now?

Jan

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

[David Vrabel]

On 19/02/16 14:36, Jan Beulich wrote:
>>>> On 19.02.16 at 15:16, <david.vrabel@citrix.com> wrote:
>> On 19/02/16 14:08, Jan Beulich wrote:
>>>>>> On 18.02.16 at 19:52, <david.vrabel@citrix.com> wrote:
>>>> @@ -261,28 +261,8 @@ 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 != 4 )
>>>>      {
>>>> -        typeof(ptr->fpu_sse.fip.sel) fcs = ptr->fpu_sse.fip.sel;
>>>> -        typeof(ptr->fpu_sse.fdp.sel) fds = ptr->fpu_sse.fdp.sel;
>>>> -
>>>> -        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;
>>>> -            }
>>>> -        }
>>>> -
>>>>          XSAVE("0x48,");
>>>>  
>>>>          if ( !(mask & ptr->xsave_hdr.xstate_bv & XSTATE_FP) ||
>>>> @@ -293,15 +273,14 @@ void xsave(struct vcpu *v, uint64_t mask)
>>>>               (!(ptr->fpu_sse.fsw & 0x0080) &&
>>>>                boot_cpu_data.x86_vendor == X86_VENDOR_AMD) )
>>>>          {
>>>> -            if ( (cpu_has_xsaveopt || cpu_has_xsaves) && word_size > 0 )
>>>> -            {
>>>> -                ptr->fpu_sse.fip.sel = fcs;
>>>> -                ptr->fpu_sse.fdp.sel = fds;
>>>> -            }
>>>>              return;
>>>
>>> I don't see how you can validly delete all of the above code without
>>> any replacement. Can you explain the rationale behind this?
>>
>> I think it is unnecessary.
>>
>> If XSAVEOPT/XSAVES doesn't save the FP state, it hasn't changed since
>> last time and we don't need to clear and rewrite the FCS/FDS fields
>> since the old values are still valid.
> 
> Well, this logic fixed a particular issue, and I'd like to be sure the
> deletion won't re-introduce that issue. In particular your patch
> doesn't touch the place where the zero values are actually being
> looked at:

This was added because FCS/FDS were being cleared.  Without initially
clearing these fields, they do not need to be restored if the hardware
did not write them.

> +        /*
> +         * 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 &&
>               !((ptr->fpu_sse.fip.addr | ptr->fpu_sse.fdp.addr) >> 32) )
>          {
>              struct ix87_env fpu_env;
> 
> So how is this check going to fulfill its purpose now?

This is checking the values just written by the XSAVE* instruction.
Note that the FCS/FDS in the state overlap with the FIP/FDP[47:32].

David

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

[Jan Beulich]

>>> On 19.02.16 at 15:49, <david.vrabel@citrix.com> wrote:
> On 19/02/16 14:36, Jan Beulich wrote:
>>>>> On 19.02.16 at 15:16, <david.vrabel@citrix.com> wrote:
>>> On 19/02/16 14:08, Jan Beulich wrote:
>>>>>>> On 18.02.16 at 19:52, <david.vrabel@citrix.com> wrote:
>>>>> @@ -261,28 +261,8 @@ 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 != 4 )
>>>>>      {
>>>>> -        typeof(ptr->fpu_sse.fip.sel) fcs = ptr->fpu_sse.fip.sel;
>>>>> -        typeof(ptr->fpu_sse.fdp.sel) fds = ptr->fpu_sse.fdp.sel;
>>>>> -
>>>>> -        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;
>>>>> -            }
>>>>> -        }
>>>>> -
>>>>>          XSAVE("0x48,");
>>>>>  
>>>>>          if ( !(mask & ptr->xsave_hdr.xstate_bv & XSTATE_FP) ||
>>>>> @@ -293,15 +273,14 @@ void xsave(struct vcpu *v, uint64_t mask)
>>>>>               (!(ptr->fpu_sse.fsw & 0x0080) &&
>>>>>                boot_cpu_data.x86_vendor == X86_VENDOR_AMD) )
>>>>>          {
>>>>> -            if ( (cpu_has_xsaveopt || cpu_has_xsaves) && word_size > 0 )
>>>>> -            {
>>>>> -                ptr->fpu_sse.fip.sel = fcs;
>>>>> -                ptr->fpu_sse.fdp.sel = fds;
>>>>> -            }
>>>>>              return;
>>>>
>>>> I don't see how you can validly delete all of the above code without
>>>> any replacement. Can you explain the rationale behind this?
>>>
>>> I think it is unnecessary.
>>>
>>> If XSAVEOPT/XSAVES doesn't save the FP state, it hasn't changed since
>>> last time and we don't need to clear and rewrite the FCS/FDS fields
>>> since the old values are still valid.
>> 
>> Well, this logic fixed a particular issue, and I'd like to be sure the
>> deletion won't re-introduce that issue. In particular your patch
>> doesn't touch the place where the zero values are actually being
>> looked at:
> 
> This was added because FCS/FDS were being cleared.  Without initially
> clearing these fields, they do not need to be restored if the hardware
> did not write them.

My initial comment referred to all of the still quoted code, while
you now seem to think it was only about the second of those
hunks.

>> +        /*
>> +         * 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 &&
>>               !((ptr->fpu_sse.fip.addr | ptr->fpu_sse.fdp.addr) >> 32) )
>>          {
>>              struct ix87_env fpu_env;
>> 
>> So how is this check going to fulfill its purpose now?
> 
> This is checking the values just written by the XSAVE* instruction.
> Note that the FCS/FDS in the state overlap with the FIP/FDP[47:32].

Iirc the issue was with a 64-bit XSAVE having got the selector
values stuck in via FNSTENV (forcing word size to 4), and a
subsequent XSAVEOPT not further touching the fields
(because no change to the FPU state had occurred), leading
to (without the code you're deleting) the word size for the
restore wrongly getting set to 8, and the selector values then
lost. I cannot see how this situation would be handled with
this patch in place.

Jan

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

[David Vrabel]

On 19/02/16 15:14, Jan Beulich wrote:
>>>> On 19.02.16 at 15:49, <david.vrabel@citrix.com> wrote:
>> On 19/02/16 14:36, Jan Beulich wrote:
>>>>>> On 19.02.16 at 15:16, <david.vrabel@citrix.com> wrote:
>>>> On 19/02/16 14:08, Jan Beulich wrote:
>>>>>>>> On 18.02.16 at 19:52, <david.vrabel@citrix.com> wrote:
>>>>>> @@ -261,28 +261,8 @@ 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 != 4 )
>>>>>>      {
>>>>>> -        typeof(ptr->fpu_sse.fip.sel) fcs = ptr->fpu_sse.fip.sel;
>>>>>> -        typeof(ptr->fpu_sse.fdp.sel) fds = ptr->fpu_sse.fdp.sel;
>>>>>> -
>>>>>> -        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;
>>>>>> -            }
>>>>>> -        }
>>>>>> -
>>>>>>          XSAVE("0x48,");
>>>>>>  
>>>>>>          if ( !(mask & ptr->xsave_hdr.xstate_bv & XSTATE_FP) ||
>>>>>> @@ -293,15 +273,14 @@ void xsave(struct vcpu *v, uint64_t mask)
>>>>>>               (!(ptr->fpu_sse.fsw & 0x0080) &&
>>>>>>                boot_cpu_data.x86_vendor == X86_VENDOR_AMD) )
>>>>>>          {
>>>>>> -            if ( (cpu_has_xsaveopt || cpu_has_xsaves) && word_size > 0 )
>>>>>> -            {
>>>>>> -                ptr->fpu_sse.fip.sel = fcs;
>>>>>> -                ptr->fpu_sse.fdp.sel = fds;
>>>>>> -            }
>>>>>>              return;
>>>>>
>>>>> I don't see how you can validly delete all of the above code without
>>>>> any replacement. Can you explain the rationale behind this?
>>>>
>>>> I think it is unnecessary.
>>>>
>>>> If XSAVEOPT/XSAVES doesn't save the FP state, it hasn't changed since
>>>> last time and we don't need to clear and rewrite the FCS/FDS fields
>>>> since the old values are still valid.
>>>
>>> Well, this logic fixed a particular issue, and I'd like to be sure the
>>> deletion won't re-introduce that issue. In particular your patch
>>> doesn't touch the place where the zero values are actually being
>>> looked at:
>>
>> This was added because FCS/FDS were being cleared.  Without initially
>> clearing these fields, they do not need to be restored if the hardware
>> did not write them.
> 
> My initial comment referred to all of the still quoted code, while
> you now seem to think it was only about the second of those
> hunks.
> 
>>> +        /*
>>> +         * 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 &&
>>>               !((ptr->fpu_sse.fip.addr | ptr->fpu_sse.fdp.addr) >> 32) )
>>>          {
>>>              struct ix87_env fpu_env;
>>>
>>> So how is this check going to fulfill its purpose now?
>>
>> This is checking the values just written by the XSAVE* instruction.
>> Note that the FCS/FDS in the state overlap with the FIP/FDP[47:32].
> 
> Iirc the issue was with a 64-bit XSAVE having got the selector
> values stuck in via FNSTENV (forcing word size to 4), and a
> subsequent XSAVEOPT not further touching the fields
> (because no change to the FPU state had occurred), leading
> to (without the code you're deleting) the word size for the
> restore wrongly getting set to 8, and the selector values then
> lost. I cannot see how this situation would be handled with
> this patch in place.

Er, yes. You're right. Sorry.

David

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

[David Vrabel]

On 19/02/16 15:43, David Vrabel wrote:
> On 19/02/16 15:14, Jan Beulich wrote:
>>
>> Iirc the issue was with a 64-bit XSAVE having got the selector
>> values stuck in via FNSTENV (forcing word size to 4), and a
>> subsequent XSAVEOPT not further touching the fields
>> (because no change to the FPU state had occurred), leading
>> to (without the code you're deleting) the word size for the
>> restore wrongly getting set to 8, and the selector values then
>> lost. I cannot see how this situation would be handled with
>> this patch in place.
> 
> Er, yes. You're right. Sorry.

Would this function be less confusing with something like this?

The resulting code is a lot smaller too (by 163 bytes) and has fewer branches.

--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -263,41 +263,24 @@ 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 bad_fip;
 
-        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, make the FIP
+         * field non-canonical by flipping the top bit.
+         */
+        bad_fip = ptr->fpu_sse.fip.addr ^= 1 << 63;
 
         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 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 ^= 1 << 63;
             return;
         }

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

[Jan Beulich]

>>> On 19.02.16 at 17:38, <david.vrabel@citrix.com> wrote:
> On 19/02/16 15:43, David Vrabel wrote:
>> On 19/02/16 15:14, Jan Beulich wrote:
>>>
>>> Iirc the issue was with a 64-bit XSAVE having got the selector
>>> values stuck in via FNSTENV (forcing word size to 4), and a
>>> subsequent XSAVEOPT not further touching the fields
>>> (because no change to the FPU state had occurred), leading
>>> to (without the code you're deleting) the word size for the
>>> restore wrongly getting set to 8, and the selector values then
>>> lost. I cannot see how this situation would be handled with
>>> this patch in place.
>> 
>> Er, yes. You're right. Sorry.
> 
> Would this function be less confusing with something like this?
> 
> The resulting code is a lot smaller too (by 163 bytes) and has fewer 
> branches.

Well, if it works everywhere - why not.

Jan

> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -263,41 +263,24 @@ 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 bad_fip;
>  
> -        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, make the FIP
> +         * field non-canonical by flipping the top bit.
> +         */
> +        bad_fip = ptr->fpu_sse.fip.addr ^= 1 << 63;
>  
>          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 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 ^= 1 << 63;
>              return;
>          }