|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] [V4] x86/xsaves: fix overwriting between non-lazy/lazy xsaves
The offset at which components xsaved by xsave[sc] are not fixed.
So when when a save with v->fpu_dirtied set is followed by one
with v->fpu_dirtied clear, non-lazy xsave[sc] may overwriting data
written by the lazy one.
The solution is when using_xsave_compact is enabled and taking xcr0_accum into
consideration, if guest has ever used XSTATE_LAZY & ~XSTATE_FP_SSE
(XSTATE_FP_SSE will be excluded beacause xsave will write XSTATE_FP_SSE
part in legacy region of xsave area which is fixed, saving XSTATE_FS_SSE
will not cause overwriting problem), vcpu_xsave_mask will return XSTATE_ALL.
Otherwise vcpu_xsave_mask will return XSTATE_NONLAZY.
This may cause overhead save on lazy states which will cause performance
impact. After doing some performance tests on xsavec and xsaveopt
(suggested by jan), the results show xsaveopt performs better than xsavec.
So hypervisor will not use xsavec anymore.
As xsaves will be used until suprevised state is instroduced in hypervisor.
So in this patch add a new synthetic CPU feature X86_FEATURE_XSAVE_COMPACT
which indicates whether hypervisor uses compact xsave area or not.
Code related with hypervisor xsave will depend on this feature.
Signed-off-by: Shuai Ruan <shuai.ruan@xxxxxxxxxxxxxxx>
Reported-by: Jan Beulich <jbeulich@xxxxxxxx>
---
v4: Address comments from Jan
1. Add synthetic CPU feature X86_FEATURE_XSAVE_COMPACT and use this feature
indicate whether hypervisor use compact xsave area or not.
2. Fix type/grammer errors of the comment in vcpu_xsave_mask.
v3: Address comments from Jan
1. Add xsavc clean up code and disable xsaves.
2. Add comment on why certain mask should be return in vcpu_xsave_mask.
v2: Address comments from Jan
add performance impact and next step to do in the description.
xen/arch/x86/domain.c | 2 +-
xen/arch/x86/domctl.c | 2 +-
xen/arch/x86/hvm/hvm.c | 4 ++--
xen/arch/x86/i387.c | 18 ++++++++++++++++--
xen/arch/x86/xstate.c | 22 ++++++++++------------
xen/include/asm-x86/cpufeature.h | 2 ++
6 files changed, 32 insertions(+), 18 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index a6d721b..83db3a5 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -948,7 +948,7 @@ int arch_set_info_guest(
fpu_sse->fcw = FCW_DEFAULT;
fpu_sse->mxcsr = MXCSR_DEFAULT;
}
- if ( cpu_has_xsaves )
+ if ( using_xsave_compact )
{
ASSERT(v->arch.xsave_area);
v->arch.xsave_area->xsave_hdr.xcomp_bv = XSTATE_COMPACTION_ENABLED |
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 55aecdc..1b56732 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -922,7 +922,7 @@ long arch_do_domctl(
ret = -EFAULT;
offset += sizeof(v->arch.xcr0_accum);
- if ( !ret && (cpu_has_xsaves || cpu_has_xsavec) )
+ if ( !ret && using_xsave_compact )
{
void *xsave_area;
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 5bc2812..215df9a 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2202,7 +2202,7 @@ static int hvm_load_cpu_ctxt(struct domain *d,
hvm_domain_context_t *h)
xsave_area->xsave_hdr.xstate_bv = 0;
xsave_area->fpu_sse.mxcsr = MXCSR_DEFAULT;
}
- if ( cpu_has_xsaves && xsave_area )
+ if ( using_xsave_compact && xsave_area )
xsave_area->xsave_hdr.xcomp_bv = XSTATE_COMPACTION_ENABLED |
xsave_area->xsave_hdr.xstate_bv;
@@ -5591,7 +5591,7 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs,
uint16_t ip)
if ( v->arch.xsave_area )
{
v->arch.xsave_area->xsave_hdr.xstate_bv = XSTATE_FP;
- v->arch.xsave_area->xsave_hdr.xcomp_bv = cpu_has_xsaves
+ v->arch.xsave_area->xsave_hdr.xcomp_bv = using_xsave_compact
? XSTATE_COMPACTION_ENABLED | XSTATE_FP : 0;
}
diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c
index c29d0fa..51a8614 100644
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -118,7 +118,21 @@ static inline uint64_t vcpu_xsave_mask(const struct vcpu
*v)
if ( v->fpu_dirtied )
return v->arch.nonlazy_xstate_used ? XSTATE_ALL : XSTATE_LAZY;
- return v->arch.nonlazy_xstate_used ? XSTATE_NONLAZY : 0;
+ /*
+ * The offsets of components which live in the extended region of
+ * compact xsave area are not fixed. Xsave area may be overwritten
+ * when v->fpu_dirtied set is followed by one with v->fpu_dirtied
+ * clear.
+ * In such case, if hypervisor uses compact xsave area and guest
+ * has ever used lazy states (checking xcr0_accum also excluding
+ * XSTATE_FP_SSE), vcpu_xsave_mask will return XSTATE_ALL. Otherwise
+ * return XSTATE_NONLAZY.
+ * XSTATE_FP_SSE may be excluded, because the offsets of XSTATE_FP_SSE
+ * (in the legacy region of xsave area) are fixed, so saving
+ * XSTATE_FS_SSE will not cause overwriting problem.
+ */
+ return using_xsave_compact && (v->arch.xcr0_accum & XSTATE_LAZY &
~XSTATE_FP_SSE) ?
+ XSTATE_ALL : XSTATE_NONLAZY;
}
/* Save x87 extended state */
@@ -277,7 +291,7 @@ int vcpu_init_fpu(struct vcpu *v)
if ( v->arch.xsave_area )
{
v->arch.fpu_ctxt = &v->arch.xsave_area->fpu_sse;
- if ( cpu_has_xsaves )
+ if ( using_xsave_compact )
v->arch.xsave_area->xsave_hdr.xcomp_bv = XSTATE_COMPACTION_ENABLED;
}
else
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 8316bd9..c08e171 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -165,7 +165,7 @@ void expand_xsave_states(struct vcpu *v, void *dest,
unsigned int size)
u64 xstate_bv = xsave->xsave_hdr.xstate_bv;
u64 valid;
- if ( !cpu_has_xsaves && !cpu_has_xsavec )
+ if ( !(xsave->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED) )
{
memcpy(dest, xsave, size);
return;
@@ -206,7 +206,7 @@ void compress_xsave_states(struct vcpu *v, const void *src,
unsigned int size)
u64 xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv;
u64 valid;
- if ( !cpu_has_xsaves && !cpu_has_xsavec )
+ if ( !(xsave->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED) )
{
memcpy(xsave, src, size);
return;
@@ -251,13 +251,11 @@ void xsave(struct vcpu *v, uint64_t mask)
uint32_t lmask = mask;
unsigned int fip_width = v->domain->arch.x87_fip_width;
#define XSAVE(pfx) \
- alternative_io_3(".byte " pfx "0x0f,0xae,0x27\n", /* xsave */ \
+ alternative_io_2(".byte " pfx "0x0f,0xae,0x27\n", /* xsave */ \
".byte " pfx "0x0f,0xae,0x37\n", /* xsaveopt */ \
X86_FEATURE_XSAVEOPT, \
- ".byte " pfx "0x0f,0xc7,0x27\n", /* xsavec */ \
- X86_FEATURE_XSAVEC, \
".byte " pfx "0x0f,0xc7,0x2f\n", /* xsaves */ \
- X86_FEATURE_XSAVES, \
+ X86_FEATURE_XSAVE_COMPACT, \
"=m" (*ptr), \
"a" (lmask), "d" (hmask), "D" (ptr))
@@ -274,7 +272,7 @@ void xsave(struct vcpu *v, uint64_t mask)
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 )
+ if ( cpu_has_xsaveopt || using_xsave_compact )
{
/*
* XSAVEOPT/XSAVES may not write the FPU portion even when the
@@ -300,7 +298,7 @@ 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 )
+ if ( cpu_has_xsaveopt || using_xsave_compact )
{
ptr->fpu_sse.fip.sel = fcs;
ptr->fpu_sse.fdp.sel = fds;
@@ -370,7 +368,7 @@ void xrstor(struct vcpu *v, uint64_t mask)
" .previous\n" \
_ASM_EXTABLE(1b, 2b), \
".byte " pfx "0x0f,0xc7,0x1f\n", \
- X86_FEATURE_XSAVES, \
+ X86_FEATURE_XSAVE_COMPACT, \
ASM_OUTPUT2([mem] "+m" (*ptr), [faults] "+g" (faults)),
\
[lmask] "a" (lmask), [hmask] "d" (hmask), \
[ptr] "D" (ptr))
@@ -409,7 +407,7 @@ void xrstor(struct vcpu *v, uint64_t mask)
((mask & XSTATE_YMM) &&
!(ptr->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED))) )
ptr->fpu_sse.mxcsr &= mxcsr_mask;
- if ( cpu_has_xsaves || cpu_has_xsavec )
+ if ( using_xsave_compact )
{
ptr->xsave_hdr.xcomp_bv &= this_cpu(xcr0) | this_cpu(xss);
ptr->xsave_hdr.xstate_bv &= ptr->xsave_hdr.xcomp_bv;
@@ -426,7 +424,7 @@ void xrstor(struct vcpu *v, uint64_t mask)
case 2: /* Stage 2: Reset all state. */
ptr->fpu_sse.mxcsr = MXCSR_DEFAULT;
ptr->xsave_hdr.xstate_bv = 0;
- ptr->xsave_hdr.xcomp_bv = cpu_has_xsaves
+ ptr->xsave_hdr.xcomp_bv = using_xsave_compact
? XSTATE_COMPACTION_ENABLED : 0;
continue;
}
@@ -575,7 +573,7 @@ void xstate_init(struct cpuinfo_x86 *c)
if ( setup_xstate_features(bsp) && bsp )
BUG();
- if ( bsp && (cpu_has_xsaves || cpu_has_xsavec) )
+ if ( bsp && using_xsave_compact )
setup_xstate_comp();
}
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index 07ba368..cf247f0 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -76,6 +76,7 @@
#define X86_FEATURE_CPUID_FAULTING (3*32+14) /* cpuid faulting */
#define X86_FEATURE_CLFLUSH_MONITOR (3*32+15) /* clflush reqd with monitor */
#define X86_FEATURE_APERFMPERF (3*32+16) /* APERFMPERF */
+#define X86_FEATURE_XSAVE_COMPACT (3*32 +17) /* use compact xsave area */
/* Intel-defined CPU features, CPUID level 0x00000001 (ecx), word 4 */
#define X86_FEATURE_XMM3 (4*32+ 0) /* Streaming SIMD Extensions-3 */
@@ -224,6 +225,7 @@
#define cpu_has_xsavec boot_cpu_has(X86_FEATURE_XSAVEC)
#define cpu_has_xgetbv1 boot_cpu_has(X86_FEATURE_XGETBV1)
#define cpu_has_xsaves boot_cpu_has(X86_FEATURE_XSAVES)
+#define using_xsave_compact boot_cpu_has(X86_FEATURE_XSAVE_COMPACT)
enum _cache_type {
CACHE_TYPE_NULL = 0,
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |