* [PATCHv4 1/3] x86/fpu: Add a per-domain field to set the width of FIP/FDP
2016-02-25 15:10 [PATCHv4 0/3] x86: workaround inability to fully restore FPU state David Vrabel
@ 2016-02-25 15:10 ` David Vrabel
2016-02-26 11:43 ` Jan Beulich
2016-02-25 15:10 ` [PATCHv4 2/3] x86/hvm: add HVM_PARAM_X87_FIP_WIDTH David Vrabel
2016-02-25 15:10 ` [PATCHv4 3/3] x86/fpu: improve check for XSAVE* not writing FIP/FDP fields David Vrabel
2 siblings, 1 reply; 5+ messages in thread
From: David Vrabel @ 2016-02-25 15:10 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, David Vrabel, Jan Beulich
The x86 architecture allows either: a) the 64-bit FIP/FDP registers to
be restored (clearing FCS and FDS); or b) the 32-bit FIP/FDP and
FCS/FDS registers to be restored (clearing the upper 32-bits).
Add a per-domain field to indicate which of these options a guest
needs. The options are: 8, 4 or 0. Where 0 indicates that the
hypervisor should automatically guess the FIP width by checking the
value of FIP/FDP when saving the state (this is the existing
behaviour).
The FIP width is initially automatic but is set explicitly in the
following cases:
- 32-bit PV guest: 4
- Newer CPUs that do not save FCS/FDS: 8
The x87_fip_width field is placed into an existing 1 byte hole in
struct arch_domain.
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
v4:
- Skip FIP check if FP state isn't being saved.
v3:
- Skip checking for FIP updates if 8 is always requested.
- Only update word size if FP state was requested.
- Improve comments a bit.
v2:
- Retain logic to detect if XSAVE* writes FIP/FDP.
- Keep 64-bit PV guests in auto-mode.
---
xen/arch/x86/domain.c | 10 ++++++++++
xen/arch/x86/i387.c | 19 ++++++++++++-------
xen/arch/x86/xstate.c | 32 ++++++++++++++++++++------------
xen/include/asm-x86/domain.h | 15 +++++++++++++++
4 files changed, 57 insertions(+), 19 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 9d43f7b..a6d721b 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -343,6 +343,8 @@ int switch_native(struct domain *d)
hvm_set_mode(v, 8);
}
+ d->arch.x87_fip_width = cpu_has_fpu_sel ? 0 : 8;
+
return 0;
}
@@ -377,6 +379,8 @@ int switch_compat(struct domain *d)
domain_set_alloc_bitsize(d);
+ d->arch.x87_fip_width = 4;
+
return 0;
undo_and_fail:
@@ -653,6 +657,12 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
/* PV/PVH guests get an emulated PIT too for video BIOSes to use. */
pit_init(d, cpu_khz);
+ /*
+ * If the FPU does not save FCS/FDS then we can always
+ * save/restore the 64-bit FIP/FDP and ignore the selectors.
+ */
+ d->arch.x87_fip_width = cpu_has_fpu_sel ? 0 : 8;
+
return 0;
fail:
diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c
index 67016c9..c29d0fa 100644
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -144,9 +144,9 @@ static inline void fpu_xsave(struct vcpu *v)
static inline void fpu_fxsave(struct vcpu *v)
{
typeof(v->arch.xsave_area->fpu_sse) *fpu_ctxt = v->arch.fpu_ctxt;
- int word_size = cpu_has_fpu_sel ? 8 : 0;
+ unsigned int fip_width = v->domain->arch.x87_fip_width;
- if ( !is_pv_32bit_vcpu(v) )
+ if ( fip_width != 4 )
{
/*
* The only way to force fxsaveq on a wide range of gas versions.
@@ -164,7 +164,11 @@ static inline void fpu_fxsave(struct vcpu *v)
boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
return;
- if ( word_size > 0 &&
+ /*
+ * If the FIP/FDP[63:32] are both zero, it is safe to use the
+ * 32-bit restore to also restore the selectors.
+ */
+ if ( !fip_width &&
!((fpu_ctxt->fip.addr | fpu_ctxt->fdp.addr) >> 32) )
{
struct ix87_env fpu_env;
@@ -172,17 +176,18 @@ static inline void fpu_fxsave(struct vcpu *v)
asm volatile ( "fnstenv %0" : "=m" (fpu_env) );
fpu_ctxt->fip.sel = fpu_env.fcs;
fpu_ctxt->fdp.sel = fpu_env.fds;
- word_size = 4;
+ fip_width = 4;
}
+ else
+ fip_width = 8;
}
else
{
asm volatile ( "fxsave %0" : "=m" (*fpu_ctxt) );
- word_size = 4;
+ fip_width = 4;
}
- if ( word_size >= 0 )
- fpu_ctxt->x[FPU_WORD_SIZE_OFFSET] = word_size;
+ fpu_ctxt->x[FPU_WORD_SIZE_OFFSET] = fip_width;
}
/*******************************/
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 4f2fb8e..65a4f64 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -249,7 +249,7 @@ void xsave(struct vcpu *v, uint64_t mask)
struct xsave_struct *ptr = v->arch.xsave_area;
uint32_t hmask = mask >> 32;
uint32_t lmask = mask;
- int word_size = mask & XSTATE_FP ? (cpu_has_fpu_sel ? 8 : 0) : -1;
+ unsigned int fip_width = v->domain->arch.x87_fip_width;
#define XSAVE(pfx) \
alternative_io_3(".byte " pfx "0x0f,0xae,0x27\n", /* xsave */ \
".byte " pfx "0x0f,0xae,0x37\n", /* xsaveopt */ \
@@ -261,7 +261,15 @@ void xsave(struct vcpu *v, uint64_t mask)
"=m" (*ptr), \
"a" (lmask), "d" (hmask), "D" (ptr))
- if ( word_size <= 0 || !is_pv_32bit_vcpu(v) )
+ if ( fip_width == 8 || !(mask & XSTATE_FP) )
+ {
+ XSAVE("0x48,");
+ }
+ else if ( fip_width == 4 )
+ {
+ XSAVE("");
+ }
+ else
{
typeof(ptr->fpu_sse.fip.sel) fcs = ptr->fpu_sse.fip.sel;
typeof(ptr->fpu_sse.fdp.sel) fds = ptr->fpu_sse.fdp.sel;
@@ -301,25 +309,25 @@ void xsave(struct vcpu *v, uint64_t mask)
return;
}
- if ( word_size > 0 &&
- !((ptr->fpu_sse.fip.addr | ptr->fpu_sse.fdp.addr) >> 32) )
+ /*
+ * If the FIP/FDP[63:32] are both zero, it is safe to use the
+ * 32-bit restore to also restore the selectors.
+ */
+ if ( !((ptr->fpu_sse.fip.addr | ptr->fpu_sse.fdp.addr) >> 32) )
{
struct ix87_env fpu_env;
asm volatile ( "fnstenv %0" : "=m" (fpu_env) );
ptr->fpu_sse.fip.sel = fpu_env.fcs;
ptr->fpu_sse.fdp.sel = fpu_env.fds;
- word_size = 4;
+ fip_width = 4;
}
- }
- else
- {
- XSAVE("");
- word_size = 4;
+ else
+ fip_width = 8;
}
#undef XSAVE
- if ( word_size >= 0 )
- ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = word_size;
+ if ( mask & XSTATE_FP )
+ ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = fip_width;
}
void xrstor(struct vcpu *v, uint64_t mask)
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 4fad638..7135709 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -339,6 +339,21 @@ struct arch_domain
u8 x86_vendor; /* CPU vendor */
u8 x86_model; /* CPU model */
+ /*
+ * The width of the FIP/FDP register in the FPU that needs to be
+ * saved/restored during a context switch. This is needed because
+ * the FPU can either: a) restore the 64-bit FIP/FDP and clear FCS
+ * and FDS; or b) restore the 32-bit FIP/FDP (clearing the upper
+ * 32-bits of FIP/FDP) and restore FCS/FDS.
+ *
+ * Which one is needed depends on the guest.
+ *
+ * This can be either: 8, 4 or 0. 0 means auto-detect the size
+ * based on the width of FIP/FDP values that are written by the
+ * guest.
+ */
+ uint8_t x87_fip_width;
+
cpuid_input_t *cpuids;
struct PITState vpit;
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCHv4 1/3] x86/fpu: Add a per-domain field to set the width of FIP/FDP
2016-02-25 15:10 ` [PATCHv4 1/3] x86/fpu: Add a per-domain field to set the width of FIP/FDP David Vrabel
@ 2016-02-26 11:43 ` Jan Beulich
0 siblings, 0 replies; 5+ messages in thread
From: Jan Beulich @ 2016-02-26 11:43 UTC (permalink / raw)
To: David Vrabel; +Cc: Andrew Cooper, xen-devel
>>> On 25.02.16 at 16:10, <david.vrabel@citrix.com> wrote:
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -249,7 +249,7 @@ void xsave(struct vcpu *v, uint64_t mask)
> struct xsave_struct *ptr = v->arch.xsave_area;
> uint32_t hmask = mask >> 32;
> uint32_t lmask = mask;
> - int word_size = mask & XSTATE_FP ? (cpu_has_fpu_sel ? 8 : 0) : -1;
> + unsigned int fip_width = v->domain->arch.x87_fip_width;
> #define XSAVE(pfx) \
> alternative_io_3(".byte " pfx "0x0f,0xae,0x27\n", /* xsave */ \
> ".byte " pfx "0x0f,0xae,0x37\n", /* xsaveopt */ \
> @@ -261,7 +261,15 @@ void xsave(struct vcpu *v, uint64_t mask)
> "=m" (*ptr), \
> "a" (lmask), "d" (hmask), "D" (ptr))
>
> - if ( word_size <= 0 || !is_pv_32bit_vcpu(v) )
> + if ( fip_width == 8 || !(mask & XSTATE_FP) )
> + {
> + XSAVE("0x48,");
> + }
> + else if ( fip_width == 4 )
> + {
> + XSAVE("");
> + }
> + else
> {
> typeof(ptr->fpu_sse.fip.sel) fcs = ptr->fpu_sse.fip.sel;
> typeof(ptr->fpu_sse.fdp.sel) fds = ptr->fpu_sse.fdp.sel;
> @@ -301,25 +309,25 @@ void xsave(struct vcpu *v, uint64_t mask)
> return;
> }
>
> - if ( word_size > 0 &&
> - !((ptr->fpu_sse.fip.addr | ptr->fpu_sse.fdp.addr) >> 32) )
> + /*
> + * If the FIP/FDP[63:32] are both zero, it is safe to use the
> + * 32-bit restore to also restore the selectors.
> + */
> + if ( !((ptr->fpu_sse.fip.addr | ptr->fpu_sse.fdp.addr) >> 32) )
> {
> struct ix87_env fpu_env;
>
> asm volatile ( "fnstenv %0" : "=m" (fpu_env) );
> ptr->fpu_sse.fip.sel = fpu_env.fcs;
> ptr->fpu_sse.fdp.sel = fpu_env.fds;
> - word_size = 4;
> + fip_width = 4;
> }
> - }
> - else
> - {
> - XSAVE("");
> - word_size = 4;
> + else
> + fip_width = 8;
> }
> #undef XSAVE
> - if ( word_size >= 0 )
> - ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = word_size;
> + if ( mask & XSTATE_FP )
> + ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = fip_width;
> }
This needed further fixing up, as two references to word_size
had been left in. Which means that patch 3 now will need to be
re-based. And please double check the adjustments.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCHv4 2/3] x86/hvm: add HVM_PARAM_X87_FIP_WIDTH
2016-02-25 15:10 [PATCHv4 0/3] x86: workaround inability to fully restore FPU state David Vrabel
2016-02-25 15:10 ` [PATCHv4 1/3] x86/fpu: Add a per-domain field to set the width of FIP/FDP David Vrabel
@ 2016-02-25 15:10 ` David Vrabel
2016-02-25 15:10 ` [PATCHv4 3/3] x86/fpu: improve check for XSAVE* not writing FIP/FDP fields David Vrabel
2 siblings, 0 replies; 5+ messages in thread
From: David Vrabel @ 2016-02-25 15:10 UTC (permalink / raw)
To: xen-devel
Cc: Wei Liu, Stefano Stabellini, Andrew Cooper, Ian Jackson,
David Vrabel, Jan Beulich
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCHv4 3/3] x86/fpu: improve check for XSAVE* not writing FIP/FDP fields
2016-02-25 15:10 [PATCHv4 0/3] x86: workaround inability to fully restore FPU state David Vrabel
2016-02-25 15:10 ` [PATCHv4 1/3] x86/fpu: Add a per-domain field to set the width of FIP/FDP David Vrabel
2016-02-25 15:10 ` [PATCHv4 2/3] x86/hvm: add HVM_PARAM_X87_FIP_WIDTH David Vrabel
@ 2016-02-25 15:10 ` David Vrabel
2 siblings, 0 replies; 5+ messages in thread
From: David Vrabel @ 2016-02-25 15:10 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, David Vrabel, Jan Beulich
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 65a4f64..1350748 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -271,41 +271,28 @@ void xsave(struct vcpu *v, uint64_t mask)
}
else
{
- typeof(ptr->fpu_sse.fip.sel) fcs = ptr->fpu_sse.fip.sel;
- typeof(ptr->fpu_sse.fdp.sel) fds = ptr->fpu_sse.fdp.sel;
+ 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
^ permalink raw reply related [flat|nested] 5+ messages in thread