* [PATCH 1/4] x86: preserve FPU selectors for 32-bit guest code
2013-06-04 7:58 [PATCH 0/4] XSA-52..54 follow-up Jan Beulich
@ 2013-06-04 8:05 ` Jan Beulich
2013-06-04 8:07 ` [PATCH 2/4] x86: fix XCR0 handling Jan Beulich
` (3 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Jan Beulich @ 2013-06-04 8:05 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Ben Guthro, Keir Fraser
[-- Attachment #1: Type: text/plain, Size: 14269 bytes --]
Doing {,F}X{SAVE,RSTOR} unconditionally with 64-bit operand size leads
to the selector values associated with the last instruction/data
pointers getting lost. This, besides being inconsistent and not
compatible with native hardware behavior especially for 32-bit guests,
leads to bug checks in 32-bit Windows when running with "Driver
verifier" (see e.g. http://support.microsoft.com/kb/244617).
In a first try I made the code figure out the current guest mode, but
that has the disadvantage of only taking care of the issue when the
guest executes in the mode for which the state currently is (i.e.
namely not in the case of a 64-bit guest running a 32-bit application,
but being in kernle [64-bit] mode).
The solution presented here is to conditionally execute an auxiliary
FNSTENV and use the selectors from there.
In either case the determined word size gets stored in the last byte
of the FPU/SSE save area, which is available for software use (and I
verified is being cleared to zero by all versions of Xen, i.e. will not
present a problem when migrating guests from older to newer hosts), and
evaluated for determining the operand size {,F}XRSTOR is to be issued
with.
Note that I did check whether using a second FXSAVE or a partial second
XSAVE would be faster than FNSTENV - neither on my Westmere (FXSAVE)
nor on my Romley (XSAVE) they are.
I was really tempted to use branches into the middle of instructions
(i.e. past the REX64 prefixes) here, as that would have allowed to
collapse the otherwise identical fault recovery blocks. I stayed away
from doing so just because I expect others to dislike such slightly
subtle/tricky code.
Reported-by: Ben Guthro <Benjamin.Guthro@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -55,28 +55,53 @@ static inline void fpu_fxrstor(struct vc
* possibility, which may occur if the block was passed to us by control
* tools or through VCPUOP_initialise, by silently clearing the block.
*/
- asm volatile (
- /* See above for why the operands/constraints are this way. */
- "1: " REX64_PREFIX "fxrstor (%2)\n"
- ".section .fixup,\"ax\" \n"
- "2: push %%"__OP"ax \n"
- " push %%"__OP"cx \n"
- " push %%"__OP"di \n"
- " lea %0,%%"__OP"di \n"
- " mov %1,%%ecx \n"
- " xor %%eax,%%eax \n"
- " rep ; stosl \n"
- " pop %%"__OP"di \n"
- " pop %%"__OP"cx \n"
- " pop %%"__OP"ax \n"
- " jmp 1b \n"
- ".previous \n"
- _ASM_EXTABLE(1b, 2b)
- :
- : "m" (*fpu_ctxt),
- "i" (sizeof(v->arch.xsave_area->fpu_sse)/4)
- ,"cdaSDb" (fpu_ctxt)
- );
+ switch ( __builtin_expect(fpu_ctxt[FPU_WORD_SIZE_OFFSET], 8) )
+ {
+ default:
+ asm volatile (
+ /* See below for why the operands/constraints are this way. */
+ "1: " REX64_PREFIX "fxrstor (%2)\n"
+ ".section .fixup,\"ax\" \n"
+ "2: push %%"__OP"ax \n"
+ " push %%"__OP"cx \n"
+ " push %%"__OP"di \n"
+ " mov %2,%%"__OP"di \n"
+ " mov %1,%%ecx \n"
+ " xor %%eax,%%eax \n"
+ " rep ; stosl \n"
+ " pop %%"__OP"di \n"
+ " pop %%"__OP"cx \n"
+ " pop %%"__OP"ax \n"
+ " jmp 1b \n"
+ ".previous \n"
+ _ASM_EXTABLE(1b, 2b)
+ :
+ : "m" (*fpu_ctxt),
+ "i" (sizeof(v->arch.xsave_area->fpu_sse) / 4),
+ "cdaSDb" (fpu_ctxt) );
+ break;
+ case 4: case 2:
+ asm volatile (
+ "1: fxrstor %0 \n"
+ ".section .fixup,\"ax\"\n"
+ "2: push %%"__OP"ax \n"
+ " push %%"__OP"cx \n"
+ " push %%"__OP"di \n"
+ " lea %0,%%"__OP"di \n"
+ " mov %1,%%ecx \n"
+ " xor %%eax,%%eax \n"
+ " rep ; stosl \n"
+ " pop %%"__OP"di \n"
+ " pop %%"__OP"cx \n"
+ " pop %%"__OP"ax \n"
+ " jmp 1b \n"
+ ".previous \n"
+ _ASM_EXTABLE(1b, 2b)
+ :
+ : "m" (*fpu_ctxt),
+ "i" (sizeof(v->arch.xsave_area->fpu_sse) / 4) );
+ break;
+ }
}
/* Restore x87 extended state */
@@ -104,19 +129,49 @@ static inline void fpu_xsave(struct vcpu
/* Save x87 FPU, MMX, SSE and SSE2 state */
static inline void fpu_fxsave(struct vcpu *v)
{
- char *fpu_ctxt = v->arch.fpu_ctxt;
+ typeof(v->arch.xsave_area->fpu_sse) *fpu_ctxt = v->arch.fpu_ctxt;
+ int word_size = cpu_has_fpu_sel ? 8 : 0;
- /*
- * The only way to force fxsaveq on a wide range of gas versions. On
- * older versions the rex64 prefix works only if we force an
- * addressing mode that doesn't require extended registers.
- */
- asm volatile (
- REX64_PREFIX "fxsave (%1)"
- : "=m" (*fpu_ctxt) : "cdaSDb" (fpu_ctxt) );
+ if ( !is_pv_32bit_vcpu(v) )
+ {
+ /*
+ * The only way to force fxsaveq on a wide range of gas versions.
+ * On older versions the rex64 prefix works only if we force an
+ * addressing mode that doesn't require extended registers.
+ */
+ asm volatile ( REX64_PREFIX "fxsave (%1)"
+ : "=m" (*fpu_ctxt) : "cdaSDb" (fpu_ctxt) );
+
+ /*
+ * AMD CPUs don't save/restore FDP/FIP/FOP unless an exception
+ * is pending.
+ */
+ if ( !(fpu_ctxt->fsw & 0x0080) &&
+ boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
+ word_size = -1;
+
+ if ( word_size > 0 &&
+ !((fpu_ctxt->fip.addr | fpu_ctxt->fdp.addr) >> 32) )
+ {
+ struct ix87_env fpu_env;
+
+ asm volatile ( "fnstenv %0" : "=m" (fpu_env) );
+ fpu_ctxt->fip.sel = fpu_env.fcs;
+ fpu_ctxt->fdp.sel = fpu_env.fds;
+ word_size = 4;
+ }
+ }
+ else
+ {
+ asm volatile ( "fxsave %0" : "=m" (*fpu_ctxt) );
+ word_size = 4;
+ }
+
+ if ( word_size >= 0 )
+ fpu_ctxt->x[FPU_WORD_SIZE_OFFSET] = word_size;
/* Clear exception flags if FSW.ES is set. */
- if ( unlikely(fpu_ctxt[2] & 0x80) )
+ if ( unlikely(fpu_ctxt->fsw & 0x0080) )
asm volatile ("fnclex");
/*
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -10,6 +10,7 @@
#include <asm/current.h>
#include <asm/processor.h>
#include <asm/hvm/support.h>
+#include <asm/i387.h>
#include <asm/xstate.h>
#include <asm/asm_defns.h>
@@ -56,26 +57,59 @@ 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;
- if ( cpu_has_xsaveopt )
- asm volatile (
- ".byte " REX_PREFIX "0x0f,0xae,0x37"
- :
- : "a" (lmask), "d" (hmask), "D"(ptr)
- : "memory" );
+ if ( word_size <= 0 || !is_pv_32bit_vcpu(v) )
+ {
+ if ( cpu_has_xsaveopt )
+ asm volatile ( ".byte 0x48,0x0f,0xae,0x37"
+ : "=m" (*ptr)
+ : "a" (lmask), "d" (hmask), "D" (ptr) );
+ else
+ asm volatile ( ".byte 0x48,0x0f,0xae,0x27"
+ : "=m" (*ptr)
+ : "a" (lmask), "d" (hmask), "D" (ptr) );
+
+ 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) )
+ return;
+
+ if ( word_size > 0 &&
+ !((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;
+ }
+ }
else
- asm volatile (
- ".byte " REX_PREFIX "0x0f,0xae,0x27"
- :
- : "a" (lmask), "d" (hmask), "D"(ptr)
- : "memory" );
+ {
+ if ( cpu_has_xsaveopt )
+ asm volatile ( ".byte 0x0f,0xae,0x37"
+ : "=m" (*ptr)
+ : "a" (lmask), "d" (hmask), "D" (ptr) );
+ else
+ asm volatile ( ".byte 0x0f,0xae,0x27"
+ : "=m" (*ptr)
+ : "a" (lmask), "d" (hmask), "D" (ptr) );
+ word_size = 4;
+ }
+ if ( word_size >= 0 )
+ ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = word_size;
}
void xrstor(struct vcpu *v, uint64_t mask)
{
uint32_t hmask = mask >> 32;
uint32_t lmask = mask;
-
struct xsave_struct *ptr = v->arch.xsave_area;
/*
@@ -98,20 +132,41 @@ void xrstor(struct vcpu *v, uint64_t mas
* possibility, which may occur if the block was passed to us by control
* tools or through VCPUOP_initialise, by silently clearing the block.
*/
- asm volatile ( "1: .byte " REX_PREFIX "0x0f,0xae,0x2f\n"
- ".section .fixup,\"ax\"\n"
- "2: mov %5,%%ecx \n"
- " xor %1,%1 \n"
- " rep stosb \n"
- " lea %2,%0 \n"
- " mov %3,%1 \n"
- " jmp 1b \n"
- ".previous \n"
- _ASM_EXTABLE(1b, 2b)
- : "+&D" (ptr), "+&a" (lmask)
- : "m" (*ptr), "g" (lmask), "d" (hmask),
- "m" (xsave_cntxt_size)
- : "ecx" );
+ switch ( __builtin_expect(ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET], 8) )
+ {
+ default:
+ asm volatile ( "1: .byte 0x48,0x0f,0xae,0x2f\n"
+ ".section .fixup,\"ax\" \n"
+ "2: mov %5,%%ecx \n"
+ " xor %1,%1 \n"
+ " rep stosb \n"
+ " lea %2,%0 \n"
+ " mov %3,%1 \n"
+ " jmp 1b \n"
+ ".previous \n"
+ _ASM_EXTABLE(1b, 2b)
+ : "+&D" (ptr), "+&a" (lmask)
+ : "m" (*ptr), "g" (lmask), "d" (hmask),
+ "m" (xsave_cntxt_size)
+ : "ecx" );
+ break;
+ case 4: case 2:
+ asm volatile ( "1: .byte 0x0f,0xae,0x2f\n"
+ ".section .fixup,\"ax\" \n"
+ "2: mov %5,%%ecx \n"
+ " xor %1,%1 \n"
+ " rep stosb \n"
+ " lea %2,%0 \n"
+ " mov %3,%1 \n"
+ " jmp 1b \n"
+ ".previous \n"
+ _ASM_EXTABLE(1b, 2b)
+ : "+&D" (ptr), "+&a" (lmask)
+ : "m" (*ptr), "g" (lmask), "d" (hmask),
+ "m" (xsave_cntxt_size)
+ : "ecx" );
+ break;
+ }
}
bool_t xsave_enabled(const struct vcpu *v)
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -147,6 +147,7 @@
#define X86_FEATURE_ERMS (7*32+ 9) /* Enhanced REP MOVSB/STOSB */
#define X86_FEATURE_INVPCID (7*32+10) /* Invalidate Process Context ID */
#define X86_FEATURE_RTM (7*32+11) /* Restricted Transactional Memory */
+#define X86_FEATURE_NO_FPU_SEL (7*32+13) /* FPU CS/DS stored as zero */
#define cpu_has(c, bit) test_bit(bit, (c)->x86_capability)
#define boot_cpu_has(bit) test_bit(bit, boot_cpu_data.x86_capability)
@@ -183,6 +184,7 @@
#define cpu_has_fsgsbase boot_cpu_has(X86_FEATURE_FSGSBASE)
#define cpu_has_smep boot_cpu_has(X86_FEATURE_SMEP)
+#define cpu_has_fpu_sel (!boot_cpu_has(X86_FEATURE_NO_FPU_SEL))
#define cpu_has_ffxsr ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD) \
&& boot_cpu_has(X86_FEATURE_FFXSR))
--- a/xen/include/asm-x86/i387.h
+++ b/xen/include/asm-x86/i387.h
@@ -14,6 +14,27 @@
#include <xen/types.h>
#include <xen/percpu.h>
+/* Byte offset of the stored word size within the FXSAVE area/portion. */
+#define FPU_WORD_SIZE_OFFSET 511
+
+struct ix87_state {
+ struct ix87_env {
+ uint16_t fcw, _res0;
+ uint16_t fsw, _res1;
+ uint16_t ftw, _res2;
+ uint32_t fip;
+ uint16_t fcs;
+ uint16_t fop;
+ uint32_t fdp;
+ uint16_t fds, _res6;
+ } env;
+ struct ix87_reg {
+ uint64_t mantissa;
+ uint16_t exponent:15;
+ uint16_t sign:1;
+ } __attribute__((__packed__)) r[8];
+};
+
void vcpu_restore_fpu_eager(struct vcpu *v);
void vcpu_restore_fpu_lazy(struct vcpu *v);
void vcpu_save_fpu(struct vcpu *v);
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -34,8 +34,6 @@
#define XSTATE_NONLAZY (XSTATE_LWP)
#define XSTATE_LAZY (XSTATE_ALL & ~XSTATE_NONLAZY)
-#define REX_PREFIX "0x48, "
-
/* extended state variables */
DECLARE_PER_CPU(uint64_t, xcr0);
[-- Attachment #2: x86-FPU-preserve-selectors.patch --]
[-- Type: text/plain, Size: 14318 bytes --]
x86: preserve FPU selectors for 32-bit guest code
Doing {,F}X{SAVE,RSTOR} unconditionally with 64-bit operand size leads
to the selector values associated with the last instruction/data
pointers getting lost. This, besides being inconsistent and not
compatible with native hardware behavior especially for 32-bit guests,
leads to bug checks in 32-bit Windows when running with "Driver
verifier" (see e.g. http://support.microsoft.com/kb/244617).
In a first try I made the code figure out the current guest mode, but
that has the disadvantage of only taking care of the issue when the
guest executes in the mode for which the state currently is (i.e.
namely not in the case of a 64-bit guest running a 32-bit application,
but being in kernle [64-bit] mode).
The solution presented here is to conditionally execute an auxiliary
FNSTENV and use the selectors from there.
In either case the determined word size gets stored in the last byte
of the FPU/SSE save area, which is available for software use (and I
verified is being cleared to zero by all versions of Xen, i.e. will not
present a problem when migrating guests from older to newer hosts), and
evaluated for determining the operand size {,F}XRSTOR is to be issued
with.
Note that I did check whether using a second FXSAVE or a partial second
XSAVE would be faster than FNSTENV - neither on my Westmere (FXSAVE)
nor on my Romley (XSAVE) they are.
I was really tempted to use branches into the middle of instructions
(i.e. past the REX64 prefixes) here, as that would have allowed to
collapse the otherwise identical fault recovery blocks. I stayed away
from doing so just because I expect others to dislike such slightly
subtle/tricky code.
Reported-by: Ben Guthro <Benjamin.Guthro@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -55,28 +55,53 @@ static inline void fpu_fxrstor(struct vc
* possibility, which may occur if the block was passed to us by control
* tools or through VCPUOP_initialise, by silently clearing the block.
*/
- asm volatile (
- /* See above for why the operands/constraints are this way. */
- "1: " REX64_PREFIX "fxrstor (%2)\n"
- ".section .fixup,\"ax\" \n"
- "2: push %%"__OP"ax \n"
- " push %%"__OP"cx \n"
- " push %%"__OP"di \n"
- " lea %0,%%"__OP"di \n"
- " mov %1,%%ecx \n"
- " xor %%eax,%%eax \n"
- " rep ; stosl \n"
- " pop %%"__OP"di \n"
- " pop %%"__OP"cx \n"
- " pop %%"__OP"ax \n"
- " jmp 1b \n"
- ".previous \n"
- _ASM_EXTABLE(1b, 2b)
- :
- : "m" (*fpu_ctxt),
- "i" (sizeof(v->arch.xsave_area->fpu_sse)/4)
- ,"cdaSDb" (fpu_ctxt)
- );
+ switch ( __builtin_expect(fpu_ctxt[FPU_WORD_SIZE_OFFSET], 8) )
+ {
+ default:
+ asm volatile (
+ /* See below for why the operands/constraints are this way. */
+ "1: " REX64_PREFIX "fxrstor (%2)\n"
+ ".section .fixup,\"ax\" \n"
+ "2: push %%"__OP"ax \n"
+ " push %%"__OP"cx \n"
+ " push %%"__OP"di \n"
+ " mov %2,%%"__OP"di \n"
+ " mov %1,%%ecx \n"
+ " xor %%eax,%%eax \n"
+ " rep ; stosl \n"
+ " pop %%"__OP"di \n"
+ " pop %%"__OP"cx \n"
+ " pop %%"__OP"ax \n"
+ " jmp 1b \n"
+ ".previous \n"
+ _ASM_EXTABLE(1b, 2b)
+ :
+ : "m" (*fpu_ctxt),
+ "i" (sizeof(v->arch.xsave_area->fpu_sse) / 4),
+ "cdaSDb" (fpu_ctxt) );
+ break;
+ case 4: case 2:
+ asm volatile (
+ "1: fxrstor %0 \n"
+ ".section .fixup,\"ax\"\n"
+ "2: push %%"__OP"ax \n"
+ " push %%"__OP"cx \n"
+ " push %%"__OP"di \n"
+ " lea %0,%%"__OP"di \n"
+ " mov %1,%%ecx \n"
+ " xor %%eax,%%eax \n"
+ " rep ; stosl \n"
+ " pop %%"__OP"di \n"
+ " pop %%"__OP"cx \n"
+ " pop %%"__OP"ax \n"
+ " jmp 1b \n"
+ ".previous \n"
+ _ASM_EXTABLE(1b, 2b)
+ :
+ : "m" (*fpu_ctxt),
+ "i" (sizeof(v->arch.xsave_area->fpu_sse) / 4) );
+ break;
+ }
}
/* Restore x87 extended state */
@@ -104,19 +129,49 @@ static inline void fpu_xsave(struct vcpu
/* Save x87 FPU, MMX, SSE and SSE2 state */
static inline void fpu_fxsave(struct vcpu *v)
{
- char *fpu_ctxt = v->arch.fpu_ctxt;
+ typeof(v->arch.xsave_area->fpu_sse) *fpu_ctxt = v->arch.fpu_ctxt;
+ int word_size = cpu_has_fpu_sel ? 8 : 0;
- /*
- * The only way to force fxsaveq on a wide range of gas versions. On
- * older versions the rex64 prefix works only if we force an
- * addressing mode that doesn't require extended registers.
- */
- asm volatile (
- REX64_PREFIX "fxsave (%1)"
- : "=m" (*fpu_ctxt) : "cdaSDb" (fpu_ctxt) );
+ if ( !is_pv_32bit_vcpu(v) )
+ {
+ /*
+ * The only way to force fxsaveq on a wide range of gas versions.
+ * On older versions the rex64 prefix works only if we force an
+ * addressing mode that doesn't require extended registers.
+ */
+ asm volatile ( REX64_PREFIX "fxsave (%1)"
+ : "=m" (*fpu_ctxt) : "cdaSDb" (fpu_ctxt) );
+
+ /*
+ * AMD CPUs don't save/restore FDP/FIP/FOP unless an exception
+ * is pending.
+ */
+ if ( !(fpu_ctxt->fsw & 0x0080) &&
+ boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
+ word_size = -1;
+
+ if ( word_size > 0 &&
+ !((fpu_ctxt->fip.addr | fpu_ctxt->fdp.addr) >> 32) )
+ {
+ struct ix87_env fpu_env;
+
+ asm volatile ( "fnstenv %0" : "=m" (fpu_env) );
+ fpu_ctxt->fip.sel = fpu_env.fcs;
+ fpu_ctxt->fdp.sel = fpu_env.fds;
+ word_size = 4;
+ }
+ }
+ else
+ {
+ asm volatile ( "fxsave %0" : "=m" (*fpu_ctxt) );
+ word_size = 4;
+ }
+
+ if ( word_size >= 0 )
+ fpu_ctxt->x[FPU_WORD_SIZE_OFFSET] = word_size;
/* Clear exception flags if FSW.ES is set. */
- if ( unlikely(fpu_ctxt[2] & 0x80) )
+ if ( unlikely(fpu_ctxt->fsw & 0x0080) )
asm volatile ("fnclex");
/*
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -10,6 +10,7 @@
#include <asm/current.h>
#include <asm/processor.h>
#include <asm/hvm/support.h>
+#include <asm/i387.h>
#include <asm/xstate.h>
#include <asm/asm_defns.h>
@@ -56,26 +57,59 @@ 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;
- if ( cpu_has_xsaveopt )
- asm volatile (
- ".byte " REX_PREFIX "0x0f,0xae,0x37"
- :
- : "a" (lmask), "d" (hmask), "D"(ptr)
- : "memory" );
+ if ( word_size <= 0 || !is_pv_32bit_vcpu(v) )
+ {
+ if ( cpu_has_xsaveopt )
+ asm volatile ( ".byte 0x48,0x0f,0xae,0x37"
+ : "=m" (*ptr)
+ : "a" (lmask), "d" (hmask), "D" (ptr) );
+ else
+ asm volatile ( ".byte 0x48,0x0f,0xae,0x27"
+ : "=m" (*ptr)
+ : "a" (lmask), "d" (hmask), "D" (ptr) );
+
+ 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) )
+ return;
+
+ if ( word_size > 0 &&
+ !((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;
+ }
+ }
else
- asm volatile (
- ".byte " REX_PREFIX "0x0f,0xae,0x27"
- :
- : "a" (lmask), "d" (hmask), "D"(ptr)
- : "memory" );
+ {
+ if ( cpu_has_xsaveopt )
+ asm volatile ( ".byte 0x0f,0xae,0x37"
+ : "=m" (*ptr)
+ : "a" (lmask), "d" (hmask), "D" (ptr) );
+ else
+ asm volatile ( ".byte 0x0f,0xae,0x27"
+ : "=m" (*ptr)
+ : "a" (lmask), "d" (hmask), "D" (ptr) );
+ word_size = 4;
+ }
+ if ( word_size >= 0 )
+ ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = word_size;
}
void xrstor(struct vcpu *v, uint64_t mask)
{
uint32_t hmask = mask >> 32;
uint32_t lmask = mask;
-
struct xsave_struct *ptr = v->arch.xsave_area;
/*
@@ -98,20 +132,41 @@ void xrstor(struct vcpu *v, uint64_t mas
* possibility, which may occur if the block was passed to us by control
* tools or through VCPUOP_initialise, by silently clearing the block.
*/
- asm volatile ( "1: .byte " REX_PREFIX "0x0f,0xae,0x2f\n"
- ".section .fixup,\"ax\"\n"
- "2: mov %5,%%ecx \n"
- " xor %1,%1 \n"
- " rep stosb \n"
- " lea %2,%0 \n"
- " mov %3,%1 \n"
- " jmp 1b \n"
- ".previous \n"
- _ASM_EXTABLE(1b, 2b)
- : "+&D" (ptr), "+&a" (lmask)
- : "m" (*ptr), "g" (lmask), "d" (hmask),
- "m" (xsave_cntxt_size)
- : "ecx" );
+ switch ( __builtin_expect(ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET], 8) )
+ {
+ default:
+ asm volatile ( "1: .byte 0x48,0x0f,0xae,0x2f\n"
+ ".section .fixup,\"ax\" \n"
+ "2: mov %5,%%ecx \n"
+ " xor %1,%1 \n"
+ " rep stosb \n"
+ " lea %2,%0 \n"
+ " mov %3,%1 \n"
+ " jmp 1b \n"
+ ".previous \n"
+ _ASM_EXTABLE(1b, 2b)
+ : "+&D" (ptr), "+&a" (lmask)
+ : "m" (*ptr), "g" (lmask), "d" (hmask),
+ "m" (xsave_cntxt_size)
+ : "ecx" );
+ break;
+ case 4: case 2:
+ asm volatile ( "1: .byte 0x0f,0xae,0x2f\n"
+ ".section .fixup,\"ax\" \n"
+ "2: mov %5,%%ecx \n"
+ " xor %1,%1 \n"
+ " rep stosb \n"
+ " lea %2,%0 \n"
+ " mov %3,%1 \n"
+ " jmp 1b \n"
+ ".previous \n"
+ _ASM_EXTABLE(1b, 2b)
+ : "+&D" (ptr), "+&a" (lmask)
+ : "m" (*ptr), "g" (lmask), "d" (hmask),
+ "m" (xsave_cntxt_size)
+ : "ecx" );
+ break;
+ }
}
bool_t xsave_enabled(const struct vcpu *v)
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -147,6 +147,7 @@
#define X86_FEATURE_ERMS (7*32+ 9) /* Enhanced REP MOVSB/STOSB */
#define X86_FEATURE_INVPCID (7*32+10) /* Invalidate Process Context ID */
#define X86_FEATURE_RTM (7*32+11) /* Restricted Transactional Memory */
+#define X86_FEATURE_NO_FPU_SEL (7*32+13) /* FPU CS/DS stored as zero */
#define cpu_has(c, bit) test_bit(bit, (c)->x86_capability)
#define boot_cpu_has(bit) test_bit(bit, boot_cpu_data.x86_capability)
@@ -183,6 +184,7 @@
#define cpu_has_fsgsbase boot_cpu_has(X86_FEATURE_FSGSBASE)
#define cpu_has_smep boot_cpu_has(X86_FEATURE_SMEP)
+#define cpu_has_fpu_sel (!boot_cpu_has(X86_FEATURE_NO_FPU_SEL))
#define cpu_has_ffxsr ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD) \
&& boot_cpu_has(X86_FEATURE_FFXSR))
--- a/xen/include/asm-x86/i387.h
+++ b/xen/include/asm-x86/i387.h
@@ -14,6 +14,27 @@
#include <xen/types.h>
#include <xen/percpu.h>
+/* Byte offset of the stored word size within the FXSAVE area/portion. */
+#define FPU_WORD_SIZE_OFFSET 511
+
+struct ix87_state {
+ struct ix87_env {
+ uint16_t fcw, _res0;
+ uint16_t fsw, _res1;
+ uint16_t ftw, _res2;
+ uint32_t fip;
+ uint16_t fcs;
+ uint16_t fop;
+ uint32_t fdp;
+ uint16_t fds, _res6;
+ } env;
+ struct ix87_reg {
+ uint64_t mantissa;
+ uint16_t exponent:15;
+ uint16_t sign:1;
+ } __attribute__((__packed__)) r[8];
+};
+
void vcpu_restore_fpu_eager(struct vcpu *v);
void vcpu_restore_fpu_lazy(struct vcpu *v);
void vcpu_save_fpu(struct vcpu *v);
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -34,8 +34,6 @@
#define XSTATE_NONLAZY (XSTATE_LWP)
#define XSTATE_LAZY (XSTATE_ALL & ~XSTATE_NONLAZY)
-#define REX_PREFIX "0x48, "
-
/* extended state variables */
DECLARE_PER_CPU(uint64_t, xcr0);
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 2/4] x86: fix XCR0 handling
2013-06-04 7:58 [PATCH 0/4] XSA-52..54 follow-up Jan Beulich
2013-06-04 8:05 ` [PATCH 1/4] x86: preserve FPU selectors for 32-bit guest code Jan Beulich
@ 2013-06-04 8:07 ` Jan Beulich
2013-06-04 8:09 ` [PATCH 3/4] x86/xsave: adjust state management Jan Beulich
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Jan Beulich @ 2013-06-04 8:07 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, suravee.suthikulpanit, George Dunlap, Eddie Dong,
Jun Nakajima, Boris Ostrovsky
[-- Attachment #1: Type: text/plain, Size: 9241 bytes --]
- both VMX and SVM ignored the ECX input to XSETBV
- both SVM and VMX used the full 64-bit RAX when calculating the input
mask to XSETBV
- faults on XSETBV did not get recovered from
Also consolidate the handling for PV and HVM into a single function,
and make the per-CPU variable "xcr0" static to xstate.c.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1347,8 +1347,9 @@ static void __context_switch(void)
if ( !is_idle_vcpu(n) )
{
memcpy(stack_regs, &n->arch.user_regs, CTXT_SWITCH_STACK_BYTES);
- if ( xsave_enabled(n) && n->arch.xcr0 != get_xcr0() )
- set_xcr0(n->arch.xcr0);
+ if ( xsave_enabled(n) && n->arch.xcr0 != get_xcr0() &&
+ !set_xcr0(n->arch.xcr0) )
+ BUG();
vcpu_restore_fpu_eager(n);
n->arch.ctxt_switch_to(n);
}
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1502,25 +1502,17 @@ out:
return rc;
}
-int hvm_handle_xsetbv(u64 new_bv)
+int hvm_handle_xsetbv(u32 index, u64 new_bv)
{
- struct vcpu *v = current;
struct segment_register sreg;
- hvm_get_segment_register(v, x86_seg_ss, &sreg);
+ hvm_get_segment_register(current, x86_seg_ss, &sreg);
if ( sreg.attr.fields.dpl != 0 )
goto err;
- if ( ((new_bv ^ xfeature_mask) & ~xfeature_mask) || !(new_bv & 1) )
- goto err;
-
- if ( (xfeature_mask & XSTATE_YMM & new_bv) && !(new_bv & XSTATE_SSE) )
+ if ( handle_xsetbv(index, new_bv) )
goto err;
- v->arch.xcr0 = new_bv;
- v->arch.xcr0_accum |= new_bv;
- set_xcr0(new_bv);
-
return 0;
err:
hvm_inject_hw_exception(TRAP_gp_fault, 0);
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2367,7 +2367,8 @@ void svm_vmexit_handler(struct cpu_user_
case VMEXIT_XSETBV:
if ( (inst_len = __get_instruction_length(current, INSTR_XSETBV))==0 )
break;
- if ( hvm_handle_xsetbv((((u64)regs->edx) << 32) | regs->eax) == 0 )
+ if ( hvm_handle_xsetbv(regs->ecx,
+ (regs->rdx << 32) | regs->_eax) == 0 )
__update_guest_eip(regs, inst_len);
break;
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2827,12 +2827,10 @@ void vmx_vmexit_handler(struct cpu_user_
break;
case EXIT_REASON_XSETBV:
- {
- u64 new_bv = (((u64)regs->edx) << 32) | regs->eax;
- if ( hvm_handle_xsetbv(new_bv) == 0 )
+ if ( hvm_handle_xsetbv(regs->ecx,
+ (regs->rdx << 32) | regs->_eax) == 0 )
update_guest_eip(); /* Safe: XSETBV */
break;
- }
case EXIT_REASON_APIC_WRITE:
if ( vmx_handle_apic_write() )
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -36,13 +36,17 @@ static void fpu_init(void)
/* Restore x87 extended state */
static inline void fpu_xrstor(struct vcpu *v, uint64_t mask)
{
+ bool_t ok;
+
/*
* XCR0 normally represents what guest OS set. In case of Xen itself,
* we set all supported feature mask before doing save/restore.
*/
- set_xcr0(v->arch.xcr0_accum);
+ ok = set_xcr0(v->arch.xcr0_accum);
+ ASSERT(ok);
xrstor(v, mask);
- set_xcr0(v->arch.xcr0);
+ ok = set_xcr0(v->arch.xcr0);
+ ASSERT(ok);
}
/* Restor x87 FPU, MMX, SSE and SSE2 state */
@@ -118,12 +122,16 @@ static inline void fpu_frstor(struct vcp
/* Save x87 extended state */
static inline void fpu_xsave(struct vcpu *v)
{
+ bool_t ok;
+
/* XCR0 normally represents what guest OS set. In case of Xen itself,
* we set all accumulated feature mask before doing save/restore.
*/
- set_xcr0(v->arch.xcr0_accum);
+ ok = set_xcr0(v->arch.xcr0_accum);
+ ASSERT(ok);
xsave(v, v->arch.nonlazy_xstate_used ? XSTATE_ALL : XSTATE_LAZY);
- set_xcr0(v->arch.xcr0);
+ ok = set_xcr0(v->arch.xcr0);
+ ASSERT(ok);
}
/* Save x87 FPU, MMX, SSE and SSE2 state */
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2198,25 +2198,9 @@ static int emulate_privileged_op(struct
if ( !guest_kernel_mode(v, regs) )
goto fail;
- switch ( (u32)regs->ecx )
- {
- case XCR_XFEATURE_ENABLED_MASK:
- /* bit 0 of XCR0 must be set and reserved bit must not be set */
- if ( !(new_xfeature & XSTATE_FP) || (new_xfeature & ~xfeature_mask) )
- goto fail;
-
- /* YMM state takes SSE state as prerequisite. */
- if ( (xfeature_mask & new_xfeature & XSTATE_YMM) &&
- !(new_xfeature & XSTATE_SSE) )
- goto fail;
-
- v->arch.xcr0 = new_xfeature;
- v->arch.xcr0_accum |= new_xfeature;
- set_xcr0(new_xfeature);
- break;
- default:
- goto fail;
- }
+ if ( handle_xsetbv(regs->ecx, new_xfeature) )
+ goto fail;
+
break;
}
default:
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -5,7 +5,7 @@
*
*/
-#include <xen/config.h>
+#include <xen/percpu.h>
#include <xen/sched.h>
#include <asm/current.h>
#include <asm/processor.h>
@@ -27,24 +27,34 @@ u32 xsave_cntxt_size;
u64 xfeature_mask;
/* Cached xcr0 for fast read */
-DEFINE_PER_CPU(uint64_t, xcr0);
+static DEFINE_PER_CPU(uint64_t, xcr0);
/* Because XCR0 is cached for each CPU, xsetbv() is not exposed. Users should
* use set_xcr0() instead.
*/
-static inline void xsetbv(u32 index, u64 xfeatures)
+static inline bool_t xsetbv(u32 index, u64 xfeatures)
{
u32 hi = xfeatures >> 32;
u32 lo = (u32)xfeatures;
- asm volatile (".byte 0x0f,0x01,0xd1" :: "c" (index),
- "a" (lo), "d" (hi));
+ asm volatile ( "1: .byte 0x0f,0x01,0xd1\n"
+ "3: \n"
+ ".section .fixup,\"ax\" \n"
+ "2: xor %0,%0 \n"
+ " jmp 3b \n"
+ ".previous \n"
+ _ASM_EXTABLE(1b, 2b)
+ : "+a" (lo)
+ : "c" (index), "d" (hi));
+ return lo != 0;
}
-void set_xcr0(u64 xfeatures)
+bool_t set_xcr0(u64 xfeatures)
{
+ if ( !xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures) )
+ return 0;
this_cpu(xcr0) = xfeatures;
- xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures);
+ return 1;
}
uint64_t get_xcr0(void)
@@ -236,7 +246,8 @@ void xstate_init(void)
* Set CR4_OSXSAVE and run "cpuid" to get xsave_cntxt_size.
*/
set_in_cr4(X86_CR4_OSXSAVE);
- set_xcr0((((u64)edx << 32) | eax) & XCNTXT_MASK);
+ if ( !set_xcr0((((u64)edx << 32) | eax) & XCNTXT_MASK) )
+ BUG();
cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
if ( cpu == 0 )
@@ -262,6 +273,28 @@ void xstate_init(void)
}
}
+int handle_xsetbv(u32 index, u64 new_bv)
+{
+ struct vcpu *curr = current;
+
+ if ( index != XCR_XFEATURE_ENABLED_MASK )
+ return -EOPNOTSUPP;
+
+ if ( (new_bv & ~xfeature_mask) || !(new_bv & XSTATE_FP) )
+ return -EINVAL;
+
+ if ( (new_bv & XSTATE_YMM) && !(new_bv & XSTATE_SSE) )
+ return -EINVAL;
+
+ if ( !set_xcr0(new_bv) )
+ return -EFAULT;
+
+ curr->arch.xcr0 = new_bv;
+ curr->arch.xcr0_accum |= new_bv;
+
+ return 0;
+}
+
/*
* Local variables:
* mode: C
--- a/xen/include/asm-x86/hvm/support.h
+++ b/xen/include/asm-x86/hvm/support.h
@@ -128,7 +128,7 @@ void hvm_triple_fault(void);
void hvm_rdtsc_intercept(struct cpu_user_regs *regs);
-int hvm_handle_xsetbv(u64 new_bv);
+int __must_check hvm_handle_xsetbv(u32 index, u64 new_bv);
/* These functions all return X86EMUL return codes. */
int hvm_set_efer(uint64_t value);
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -9,7 +9,6 @@
#define __ASM_XSTATE_H
#include <xen/types.h>
-#include <xen/percpu.h>
#define FCW_DEFAULT 0x037f
#define MXCSR_DEFAULT 0x1f80
@@ -34,9 +33,6 @@
#define XSTATE_NONLAZY (XSTATE_LWP)
#define XSTATE_LAZY (XSTATE_ALL & ~XSTATE_NONLAZY)
-/* extended state variables */
-DECLARE_PER_CPU(uint64_t, xcr0);
-
extern unsigned int xsave_cntxt_size;
extern u64 xfeature_mask;
@@ -75,11 +71,12 @@ struct xsave_struct
} __attribute__ ((packed, aligned (64)));
/* extended state operations */
-void set_xcr0(u64 xfeatures);
+bool_t __must_check set_xcr0(u64 xfeatures);
uint64_t get_xcr0(void);
void xsave(struct vcpu *v, uint64_t mask);
void xrstor(struct vcpu *v, uint64_t mask);
bool_t xsave_enabled(const struct vcpu *v);
+int __must_check handle_xsetbv(u32 index, u64 new_bv);
/* extended state init and cleanup functions */
void xstate_free_save_area(struct vcpu *v);
[-- Attachment #2: x86-xcr0-handling.patch --]
[-- Type: text/plain, Size: 9263 bytes --]
x86: fix XCR0 handling
- both VMX and SVM ignored the ECX input to XSETBV
- both SVM and VMX used the full 64-bit RAX when calculating the input
mask to XSETBV
- faults on XSETBV did not get recovered from
Also consolidate the handling for PV and HVM into a single function,
and make the per-CPU variable "xcr0" static to xstate.c.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1347,8 +1347,9 @@ static void __context_switch(void)
if ( !is_idle_vcpu(n) )
{
memcpy(stack_regs, &n->arch.user_regs, CTXT_SWITCH_STACK_BYTES);
- if ( xsave_enabled(n) && n->arch.xcr0 != get_xcr0() )
- set_xcr0(n->arch.xcr0);
+ if ( xsave_enabled(n) && n->arch.xcr0 != get_xcr0() &&
+ !set_xcr0(n->arch.xcr0) )
+ BUG();
vcpu_restore_fpu_eager(n);
n->arch.ctxt_switch_to(n);
}
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1502,25 +1502,17 @@ out:
return rc;
}
-int hvm_handle_xsetbv(u64 new_bv)
+int hvm_handle_xsetbv(u32 index, u64 new_bv)
{
- struct vcpu *v = current;
struct segment_register sreg;
- hvm_get_segment_register(v, x86_seg_ss, &sreg);
+ hvm_get_segment_register(current, x86_seg_ss, &sreg);
if ( sreg.attr.fields.dpl != 0 )
goto err;
- if ( ((new_bv ^ xfeature_mask) & ~xfeature_mask) || !(new_bv & 1) )
- goto err;
-
- if ( (xfeature_mask & XSTATE_YMM & new_bv) && !(new_bv & XSTATE_SSE) )
+ if ( handle_xsetbv(index, new_bv) )
goto err;
- v->arch.xcr0 = new_bv;
- v->arch.xcr0_accum |= new_bv;
- set_xcr0(new_bv);
-
return 0;
err:
hvm_inject_hw_exception(TRAP_gp_fault, 0);
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2367,7 +2367,8 @@ void svm_vmexit_handler(struct cpu_user_
case VMEXIT_XSETBV:
if ( (inst_len = __get_instruction_length(current, INSTR_XSETBV))==0 )
break;
- if ( hvm_handle_xsetbv((((u64)regs->edx) << 32) | regs->eax) == 0 )
+ if ( hvm_handle_xsetbv(regs->ecx,
+ (regs->rdx << 32) | regs->_eax) == 0 )
__update_guest_eip(regs, inst_len);
break;
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2827,12 +2827,10 @@ void vmx_vmexit_handler(struct cpu_user_
break;
case EXIT_REASON_XSETBV:
- {
- u64 new_bv = (((u64)regs->edx) << 32) | regs->eax;
- if ( hvm_handle_xsetbv(new_bv) == 0 )
+ if ( hvm_handle_xsetbv(regs->ecx,
+ (regs->rdx << 32) | regs->_eax) == 0 )
update_guest_eip(); /* Safe: XSETBV */
break;
- }
case EXIT_REASON_APIC_WRITE:
if ( vmx_handle_apic_write() )
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -36,13 +36,17 @@ static void fpu_init(void)
/* Restore x87 extended state */
static inline void fpu_xrstor(struct vcpu *v, uint64_t mask)
{
+ bool_t ok;
+
/*
* XCR0 normally represents what guest OS set. In case of Xen itself,
* we set all supported feature mask before doing save/restore.
*/
- set_xcr0(v->arch.xcr0_accum);
+ ok = set_xcr0(v->arch.xcr0_accum);
+ ASSERT(ok);
xrstor(v, mask);
- set_xcr0(v->arch.xcr0);
+ ok = set_xcr0(v->arch.xcr0);
+ ASSERT(ok);
}
/* Restor x87 FPU, MMX, SSE and SSE2 state */
@@ -118,12 +122,16 @@ static inline void fpu_frstor(struct vcp
/* Save x87 extended state */
static inline void fpu_xsave(struct vcpu *v)
{
+ bool_t ok;
+
/* XCR0 normally represents what guest OS set. In case of Xen itself,
* we set all accumulated feature mask before doing save/restore.
*/
- set_xcr0(v->arch.xcr0_accum);
+ ok = set_xcr0(v->arch.xcr0_accum);
+ ASSERT(ok);
xsave(v, v->arch.nonlazy_xstate_used ? XSTATE_ALL : XSTATE_LAZY);
- set_xcr0(v->arch.xcr0);
+ ok = set_xcr0(v->arch.xcr0);
+ ASSERT(ok);
}
/* Save x87 FPU, MMX, SSE and SSE2 state */
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2198,25 +2198,9 @@ static int emulate_privileged_op(struct
if ( !guest_kernel_mode(v, regs) )
goto fail;
- switch ( (u32)regs->ecx )
- {
- case XCR_XFEATURE_ENABLED_MASK:
- /* bit 0 of XCR0 must be set and reserved bit must not be set */
- if ( !(new_xfeature & XSTATE_FP) || (new_xfeature & ~xfeature_mask) )
- goto fail;
-
- /* YMM state takes SSE state as prerequisite. */
- if ( (xfeature_mask & new_xfeature & XSTATE_YMM) &&
- !(new_xfeature & XSTATE_SSE) )
- goto fail;
-
- v->arch.xcr0 = new_xfeature;
- v->arch.xcr0_accum |= new_xfeature;
- set_xcr0(new_xfeature);
- break;
- default:
- goto fail;
- }
+ if ( handle_xsetbv(regs->ecx, new_xfeature) )
+ goto fail;
+
break;
}
default:
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -5,7 +5,7 @@
*
*/
-#include <xen/config.h>
+#include <xen/percpu.h>
#include <xen/sched.h>
#include <asm/current.h>
#include <asm/processor.h>
@@ -27,24 +27,34 @@ u32 xsave_cntxt_size;
u64 xfeature_mask;
/* Cached xcr0 for fast read */
-DEFINE_PER_CPU(uint64_t, xcr0);
+static DEFINE_PER_CPU(uint64_t, xcr0);
/* Because XCR0 is cached for each CPU, xsetbv() is not exposed. Users should
* use set_xcr0() instead.
*/
-static inline void xsetbv(u32 index, u64 xfeatures)
+static inline bool_t xsetbv(u32 index, u64 xfeatures)
{
u32 hi = xfeatures >> 32;
u32 lo = (u32)xfeatures;
- asm volatile (".byte 0x0f,0x01,0xd1" :: "c" (index),
- "a" (lo), "d" (hi));
+ asm volatile ( "1: .byte 0x0f,0x01,0xd1\n"
+ "3: \n"
+ ".section .fixup,\"ax\" \n"
+ "2: xor %0,%0 \n"
+ " jmp 3b \n"
+ ".previous \n"
+ _ASM_EXTABLE(1b, 2b)
+ : "+a" (lo)
+ : "c" (index), "d" (hi));
+ return lo != 0;
}
-void set_xcr0(u64 xfeatures)
+bool_t set_xcr0(u64 xfeatures)
{
+ if ( !xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures) )
+ return 0;
this_cpu(xcr0) = xfeatures;
- xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures);
+ return 1;
}
uint64_t get_xcr0(void)
@@ -236,7 +246,8 @@ void xstate_init(void)
* Set CR4_OSXSAVE and run "cpuid" to get xsave_cntxt_size.
*/
set_in_cr4(X86_CR4_OSXSAVE);
- set_xcr0((((u64)edx << 32) | eax) & XCNTXT_MASK);
+ if ( !set_xcr0((((u64)edx << 32) | eax) & XCNTXT_MASK) )
+ BUG();
cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
if ( cpu == 0 )
@@ -262,6 +273,28 @@ void xstate_init(void)
}
}
+int handle_xsetbv(u32 index, u64 new_bv)
+{
+ struct vcpu *curr = current;
+
+ if ( index != XCR_XFEATURE_ENABLED_MASK )
+ return -EOPNOTSUPP;
+
+ if ( (new_bv & ~xfeature_mask) || !(new_bv & XSTATE_FP) )
+ return -EINVAL;
+
+ if ( (new_bv & XSTATE_YMM) && !(new_bv & XSTATE_SSE) )
+ return -EINVAL;
+
+ if ( !set_xcr0(new_bv) )
+ return -EFAULT;
+
+ curr->arch.xcr0 = new_bv;
+ curr->arch.xcr0_accum |= new_bv;
+
+ return 0;
+}
+
/*
* Local variables:
* mode: C
--- a/xen/include/asm-x86/hvm/support.h
+++ b/xen/include/asm-x86/hvm/support.h
@@ -128,7 +128,7 @@ void hvm_triple_fault(void);
void hvm_rdtsc_intercept(struct cpu_user_regs *regs);
-int hvm_handle_xsetbv(u64 new_bv);
+int __must_check hvm_handle_xsetbv(u32 index, u64 new_bv);
/* These functions all return X86EMUL return codes. */
int hvm_set_efer(uint64_t value);
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -9,7 +9,6 @@
#define __ASM_XSTATE_H
#include <xen/types.h>
-#include <xen/percpu.h>
#define FCW_DEFAULT 0x037f
#define MXCSR_DEFAULT 0x1f80
@@ -34,9 +33,6 @@
#define XSTATE_NONLAZY (XSTATE_LWP)
#define XSTATE_LAZY (XSTATE_ALL & ~XSTATE_NONLAZY)
-/* extended state variables */
-DECLARE_PER_CPU(uint64_t, xcr0);
-
extern unsigned int xsave_cntxt_size;
extern u64 xfeature_mask;
@@ -75,11 +71,12 @@ struct xsave_struct
} __attribute__ ((packed, aligned (64)));
/* extended state operations */
-void set_xcr0(u64 xfeatures);
+bool_t __must_check set_xcr0(u64 xfeatures);
uint64_t get_xcr0(void);
void xsave(struct vcpu *v, uint64_t mask);
void xrstor(struct vcpu *v, uint64_t mask);
bool_t xsave_enabled(const struct vcpu *v);
+int __must_check handle_xsetbv(u32 index, u64 new_bv);
/* extended state init and cleanup functions */
void xstate_free_save_area(struct vcpu *v);
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 3/4] x86/xsave: adjust state management
2013-06-04 7:58 [PATCH 0/4] XSA-52..54 follow-up Jan Beulich
2013-06-04 8:05 ` [PATCH 1/4] x86: preserve FPU selectors for 32-bit guest code Jan Beulich
2013-06-04 8:07 ` [PATCH 2/4] x86: fix XCR0 handling Jan Beulich
@ 2013-06-04 8:09 ` Jan Beulich
2013-06-04 8:10 ` [PATCH 4/4] x86/fxsave: bring in line with recent xsave adjustments Jan Beulich
2013-06-04 10:00 ` [PATCH 0/4] XSA-52..54 follow-up Keir Fraser
4 siblings, 0 replies; 13+ messages in thread
From: Jan Beulich @ 2013-06-04 8:09 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Keir Fraser
[-- Attachment #1: Type: text/plain, Size: 2883 bytes --]
The initial state for a vCPU is using default values, so there's no
need to force the XRSTOR to read the state from memory. This saves a
couple of thousand restores from memory just during boot of Linux on
my Sandy Bridge system (I didn't try to make further measurements).
The above requires that arch_set_info_guest() updates the state flags
in the save area when valid floating point state got passed in, but
that would really have been needed even before in case XSAVE{,OPT}
decided to clear one or both of the FP and SSE bits.
Furthermore, hvm_vcpu_reset_state() shouldn't just clear out the FPU/
SSE area, but needs to re-initialized MXCSR and FCW.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -712,7 +712,11 @@ int arch_set_info_guest(
v->arch.vgc_flags = flags;
if ( flags & VGCF_I387_VALID )
+ {
memcpy(v->arch.fpu_ctxt, &c.nat->fpu_ctxt, sizeof(c.nat->fpu_ctxt));
+ if ( v->arch.xsave_area )
+ v->arch.xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE;
+ }
if ( !compat )
{
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3454,6 +3454,7 @@ void hvm_vcpu_reset_state(struct vcpu *v
{
struct domain *d = v->domain;
struct segment_register reg;
+ typeof(v->arch.xsave_area->fpu_sse) *fpu_ctxt = v->arch.fpu_ctxt;
domain_lock(d);
@@ -3467,7 +3468,12 @@ void hvm_vcpu_reset_state(struct vcpu *v
v->arch.guest_table = pagetable_null();
}
- memset(v->arch.fpu_ctxt, 0, sizeof(v->arch.xsave_area->fpu_sse));
+ memset(fpu_ctxt, 0, sizeof(*fpu_ctxt));
+ fpu_ctxt->fcw = FCW_RESET;
+ fpu_ctxt->mxcsr = MXCSR_DEFAULT;
+ if ( v->arch.xsave_area )
+ v->arch.xsave_area->xsave_hdr.xstate_bv = XSTATE_FP;
+
v->arch.vgc_flags = VGCF_online;
memset(&v->arch.user_regs, 0, sizeof(v->arch.user_regs));
v->arch.user_regs.eflags = 2;
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -204,9 +204,13 @@ int xstate_alloc_save_area(struct vcpu *
if ( save_area == NULL )
return -ENOMEM;
+ /*
+ * Set the memory image to default values, but don't force the context
+ * to be loaded from memory (i.e. keep save_area->xsave_hdr.xstate_bv
+ * clear).
+ */
save_area->fpu_sse.fcw = FCW_DEFAULT;
save_area->fpu_sse.mxcsr = MXCSR_DEFAULT;
- save_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE;
v->arch.xsave_area = save_area;
v->arch.xcr0 = XSTATE_FP_SSE;
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -11,6 +11,7 @@
#include <xen/types.h>
#define FCW_DEFAULT 0x037f
+#define FCW_RESET 0x0040
#define MXCSR_DEFAULT 0x1f80
#define XSTATE_CPUID 0x0000000d
[-- Attachment #2: x86-xstate-default.patch --]
[-- Type: text/plain, Size: 2915 bytes --]
x86/xsave: adjust state management
The initial state for a vCPU is using default values, so there's no
need to force the XRSTOR to read the state from memory. This saves a
couple of thousand restores from memory just during boot of Linux on
my Sandy Bridge system (I didn't try to make further measurements).
The above requires that arch_set_info_guest() updates the state flags
in the save area when valid floating point state got passed in, but
that would really have been needed even before in case XSAVE{,OPT}
decided to clear one or both of the FP and SSE bits.
Furthermore, hvm_vcpu_reset_state() shouldn't just clear out the FPU/
SSE area, but needs to re-initialized MXCSR and FCW.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -712,7 +712,11 @@ int arch_set_info_guest(
v->arch.vgc_flags = flags;
if ( flags & VGCF_I387_VALID )
+ {
memcpy(v->arch.fpu_ctxt, &c.nat->fpu_ctxt, sizeof(c.nat->fpu_ctxt));
+ if ( v->arch.xsave_area )
+ v->arch.xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE;
+ }
if ( !compat )
{
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3454,6 +3454,7 @@ void hvm_vcpu_reset_state(struct vcpu *v
{
struct domain *d = v->domain;
struct segment_register reg;
+ typeof(v->arch.xsave_area->fpu_sse) *fpu_ctxt = v->arch.fpu_ctxt;
domain_lock(d);
@@ -3467,7 +3468,12 @@ void hvm_vcpu_reset_state(struct vcpu *v
v->arch.guest_table = pagetable_null();
}
- memset(v->arch.fpu_ctxt, 0, sizeof(v->arch.xsave_area->fpu_sse));
+ memset(fpu_ctxt, 0, sizeof(*fpu_ctxt));
+ fpu_ctxt->fcw = FCW_RESET;
+ fpu_ctxt->mxcsr = MXCSR_DEFAULT;
+ if ( v->arch.xsave_area )
+ v->arch.xsave_area->xsave_hdr.xstate_bv = XSTATE_FP;
+
v->arch.vgc_flags = VGCF_online;
memset(&v->arch.user_regs, 0, sizeof(v->arch.user_regs));
v->arch.user_regs.eflags = 2;
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -204,9 +204,13 @@ int xstate_alloc_save_area(struct vcpu *
if ( save_area == NULL )
return -ENOMEM;
+ /*
+ * Set the memory image to default values, but don't force the context
+ * to be loaded from memory (i.e. keep save_area->xsave_hdr.xstate_bv
+ * clear).
+ */
save_area->fpu_sse.fcw = FCW_DEFAULT;
save_area->fpu_sse.mxcsr = MXCSR_DEFAULT;
- save_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE;
v->arch.xsave_area = save_area;
v->arch.xcr0 = XSTATE_FP_SSE;
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -11,6 +11,7 @@
#include <xen/types.h>
#define FCW_DEFAULT 0x037f
+#define FCW_RESET 0x0040
#define MXCSR_DEFAULT 0x1f80
#define XSTATE_CPUID 0x0000000d
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 4/4] x86/fxsave: bring in line with recent xsave adjustments
2013-06-04 7:58 [PATCH 0/4] XSA-52..54 follow-up Jan Beulich
` (2 preceding siblings ...)
2013-06-04 8:09 ` [PATCH 3/4] x86/xsave: adjust state management Jan Beulich
@ 2013-06-04 8:10 ` Jan Beulich
2013-06-04 10:00 ` [PATCH 0/4] XSA-52..54 follow-up Keir Fraser
4 siblings, 0 replies; 13+ messages in thread
From: Jan Beulich @ 2013-06-04 8:10 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Keir Fraser, suravee.suthikulpanit
[-- Attachment #1: Type: text/plain, Size: 3898 bytes --]
Defer the FIP/FDP pointer reset needed on AMD CPUs to the restore path,
and switch from using EMMS to FFREE here too (to be resistant against
eventual future CPUs without MMX support). Also switch from using an
almost typeless pointer in fpu_fxrstor() to a properly typed one, thus
telling the compiler the truth about which memory gets accessed.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -52,14 +52,30 @@ static inline void fpu_xrstor(struct vcp
/* Restor x87 FPU, MMX, SSE and SSE2 state */
static inline void fpu_fxrstor(struct vcpu *v)
{
- const char *fpu_ctxt = v->arch.fpu_ctxt;
+ const typeof(v->arch.xsave_area->fpu_sse) *fpu_ctxt = v->arch.fpu_ctxt;
+
+ /*
+ * AMD CPUs don't save/restore FDP/FIP/FOP unless an exception
+ * is pending. Clear the x87 state here by setting it to fixed
+ * values. The hypervisor data segment can be sometimes 0 and
+ * sometimes new user value. Both should be ok. Use the FPU saved
+ * data block as a safe address because it should be in L1.
+ */
+ if ( !(fpu_ctxt->fsw & 0x0080) &&
+ boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
+ {
+ asm volatile ( "fnclex\n\t"
+ "ffree %%st(7)\n\t" /* clear stack tag */
+ "fildl %0" /* load to clear state */
+ : : "m" (*fpu_ctxt) );
+ }
/*
* FXRSTOR can fault if passed a corrupted data block. We handle this
* possibility, which may occur if the block was passed to us by control
* tools or through VCPUOP_initialise, by silently clearing the block.
*/
- switch ( __builtin_expect(fpu_ctxt[FPU_WORD_SIZE_OFFSET], 8) )
+ switch ( __builtin_expect(fpu_ctxt->x[FPU_WORD_SIZE_OFFSET], 8) )
{
default:
asm volatile (
@@ -80,8 +96,7 @@ static inline void fpu_fxrstor(struct vc
".previous \n"
_ASM_EXTABLE(1b, 2b)
:
- : "m" (*fpu_ctxt),
- "i" (sizeof(v->arch.xsave_area->fpu_sse) / 4),
+ : "m" (*fpu_ctxt), "i" (sizeof(*fpu_ctxt) / 4),
"cdaSDb" (fpu_ctxt) );
break;
case 4: case 2:
@@ -102,8 +117,7 @@ static inline void fpu_fxrstor(struct vc
".previous \n"
_ASM_EXTABLE(1b, 2b)
:
- : "m" (*fpu_ctxt),
- "i" (sizeof(v->arch.xsave_area->fpu_sse) / 4) );
+ : "m" (*fpu_ctxt), "i" (sizeof(*fpu_ctxt) / 4) );
break;
}
}
@@ -156,7 +170,7 @@ static inline void fpu_fxsave(struct vcp
*/
if ( !(fpu_ctxt->fsw & 0x0080) &&
boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
- word_size = -1;
+ return;
if ( word_size > 0 &&
!((fpu_ctxt->fip.addr | fpu_ctxt->fdp.addr) >> 32) )
@@ -177,25 +191,6 @@ static inline void fpu_fxsave(struct vcp
if ( word_size >= 0 )
fpu_ctxt->x[FPU_WORD_SIZE_OFFSET] = word_size;
-
- /* Clear exception flags if FSW.ES is set. */
- if ( unlikely(fpu_ctxt->fsw & 0x0080) )
- asm volatile ("fnclex");
-
- /*
- * AMD CPUs don't save/restore FDP/FIP/FOP unless an exception
- * is pending. Clear the x87 state here by setting it to fixed
- * values. The hypervisor data segment can be sometimes 0 and
- * sometimes new user value. Both should be ok. Use the FPU saved
- * data block as a safe address because it should be in L1.
- */
- if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
- {
- asm volatile (
- "emms\n\t" /* clear stack tags */
- "fildl %0" /* load to clear state */
- : : "m" (*fpu_ctxt) );
- }
}
/* Save x87 FPU state */
[-- Attachment #2: x86-fxrstor-reset-ptrs.patch --]
[-- Type: text/plain, Size: 3951 bytes --]
x86/fxsave: bring in line with recent xsave adjustments
Defer the FIP/FDP pointer reset needed on AMD CPUs to the restore path,
and switch from using EMMS to FFREE here too (to be resistant against
eventual future CPUs without MMX support). Also switch from using an
almost typeless pointer in fpu_fxrstor() to a properly typed one, thus
telling the compiler the truth about which memory gets accessed.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -52,14 +52,30 @@ static inline void fpu_xrstor(struct vcp
/* Restor x87 FPU, MMX, SSE and SSE2 state */
static inline void fpu_fxrstor(struct vcpu *v)
{
- const char *fpu_ctxt = v->arch.fpu_ctxt;
+ const typeof(v->arch.xsave_area->fpu_sse) *fpu_ctxt = v->arch.fpu_ctxt;
+
+ /*
+ * AMD CPUs don't save/restore FDP/FIP/FOP unless an exception
+ * is pending. Clear the x87 state here by setting it to fixed
+ * values. The hypervisor data segment can be sometimes 0 and
+ * sometimes new user value. Both should be ok. Use the FPU saved
+ * data block as a safe address because it should be in L1.
+ */
+ if ( !(fpu_ctxt->fsw & 0x0080) &&
+ boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
+ {
+ asm volatile ( "fnclex\n\t"
+ "ffree %%st(7)\n\t" /* clear stack tag */
+ "fildl %0" /* load to clear state */
+ : : "m" (*fpu_ctxt) );
+ }
/*
* FXRSTOR can fault if passed a corrupted data block. We handle this
* possibility, which may occur if the block was passed to us by control
* tools or through VCPUOP_initialise, by silently clearing the block.
*/
- switch ( __builtin_expect(fpu_ctxt[FPU_WORD_SIZE_OFFSET], 8) )
+ switch ( __builtin_expect(fpu_ctxt->x[FPU_WORD_SIZE_OFFSET], 8) )
{
default:
asm volatile (
@@ -80,8 +96,7 @@ static inline void fpu_fxrstor(struct vc
".previous \n"
_ASM_EXTABLE(1b, 2b)
:
- : "m" (*fpu_ctxt),
- "i" (sizeof(v->arch.xsave_area->fpu_sse) / 4),
+ : "m" (*fpu_ctxt), "i" (sizeof(*fpu_ctxt) / 4),
"cdaSDb" (fpu_ctxt) );
break;
case 4: case 2:
@@ -102,8 +117,7 @@ static inline void fpu_fxrstor(struct vc
".previous \n"
_ASM_EXTABLE(1b, 2b)
:
- : "m" (*fpu_ctxt),
- "i" (sizeof(v->arch.xsave_area->fpu_sse) / 4) );
+ : "m" (*fpu_ctxt), "i" (sizeof(*fpu_ctxt) / 4) );
break;
}
}
@@ -156,7 +170,7 @@ static inline void fpu_fxsave(struct vcp
*/
if ( !(fpu_ctxt->fsw & 0x0080) &&
boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
- word_size = -1;
+ return;
if ( word_size > 0 &&
!((fpu_ctxt->fip.addr | fpu_ctxt->fdp.addr) >> 32) )
@@ -177,25 +191,6 @@ static inline void fpu_fxsave(struct vcp
if ( word_size >= 0 )
fpu_ctxt->x[FPU_WORD_SIZE_OFFSET] = word_size;
-
- /* Clear exception flags if FSW.ES is set. */
- if ( unlikely(fpu_ctxt->fsw & 0x0080) )
- asm volatile ("fnclex");
-
- /*
- * AMD CPUs don't save/restore FDP/FIP/FOP unless an exception
- * is pending. Clear the x87 state here by setting it to fixed
- * values. The hypervisor data segment can be sometimes 0 and
- * sometimes new user value. Both should be ok. Use the FPU saved
- * data block as a safe address because it should be in L1.
- */
- if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
- {
- asm volatile (
- "emms\n\t" /* clear stack tags */
- "fildl %0" /* load to clear state */
- : : "m" (*fpu_ctxt) );
- }
}
/* Save x87 FPU state */
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 0/4] XSA-52..54 follow-up
2013-06-04 7:58 [PATCH 0/4] XSA-52..54 follow-up Jan Beulich
` (3 preceding siblings ...)
2013-06-04 8:10 ` [PATCH 4/4] x86/fxsave: bring in line with recent xsave adjustments Jan Beulich
@ 2013-06-04 10:00 ` Keir Fraser
2013-06-04 10:09 ` George Dunlap
2013-06-04 10:32 ` Jan Beulich
4 siblings, 2 replies; 13+ messages in thread
From: Keir Fraser @ 2013-06-04 10:00 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: George Dunlap
On 04/06/2013 08:58, "Jan Beulich" <JBeulich@suse.com> wrote:
> The first patch really isn't as much of a follow-up than what triggered
> the security issues to be noticed in the first place.
>
> 1: x86: preserve FPU selectors for 32-bit guest code
> 2: x86: fix XCR0 handling
> 3: x86/xsave: adjust state management
> 4: x86/fxsave: bring in line with recent xsave adjustments
>
> The first two I would see as candidates for 4.3 (as well as
> subsequent backporting, albeit I realize that especially the first
> one is non-trivial), while the third is code improvement only,
> and the fourth is really just cleanup, and hence I'd be fine with
> deferring them until after 4.3.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
I like the patches, 1 & 2 are good bug fixes.
Acked-by: Keir Fraser <keir@xen.org>
Patch #1 is quite scary though! I wonder really whether these long-lived
issues must be fixed right now, let alone backported?
-- Keir
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 0/4] XSA-52..54 follow-up
2013-06-04 10:00 ` [PATCH 0/4] XSA-52..54 follow-up Keir Fraser
@ 2013-06-04 10:09 ` George Dunlap
2013-06-04 10:45 ` Jan Beulich
2013-06-04 10:32 ` Jan Beulich
1 sibling, 1 reply; 13+ messages in thread
From: George Dunlap @ 2013-06-04 10:09 UTC (permalink / raw)
To: Keir Fraser; +Cc: Lars Kurth, Jan Beulich, xen-devel
On 06/04/2013 11:00 AM, Keir Fraser wrote:
> On 04/06/2013 08:58, "Jan Beulich" <JBeulich@suse.com> wrote:
>
>> The first patch really isn't as much of a follow-up than what triggered
>> the security issues to be noticed in the first place.
>>
>> 1: x86: preserve FPU selectors for 32-bit guest code
>> 2: x86: fix XCR0 handling
>> 3: x86/xsave: adjust state management
>> 4: x86/fxsave: bring in line with recent xsave adjustments
>>
>> The first two I would see as candidates for 4.3 (as well as
>> subsequent backporting, albeit I realize that especially the first
>> one is non-trivial), while the third is code improvement only,
>> and the fourth is really just cleanup, and hence I'd be fine with
>> deferring them until after 4.3.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> I like the patches, 1 & 2 are good bug fixes.
> Acked-by: Keir Fraser <keir@xen.org>
>
> Patch #1 is quite scary though! I wonder really whether these long-lived
> issues must be fixed right now, let alone backported?
Yeah, I was going to say, with all this tricky code going in, including
this one, and the XSA-55 (?) one that seems to have tons of tricky
changes, whether it might not be a good idea to make sure we have at
least 2 weeks of testing and another test day -- or, delay the test day
Wednesday until we can get all of these in.
Jan, looking at the comments, it seems like 3 and 4 are more about
performance than correctness? I think those should probably wait until
the 4.4 dev window opens up.
-George
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/4] XSA-52..54 follow-up
2013-06-04 10:09 ` George Dunlap
@ 2013-06-04 10:45 ` Jan Beulich
2013-06-04 10:47 ` George Dunlap
0 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2013-06-04 10:45 UTC (permalink / raw)
To: George Dunlap; +Cc: LarsKurth, Keir Fraser, xen-devel
>>> On 04.06.13 at 12:09, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> On 06/04/2013 11:00 AM, Keir Fraser wrote:
>> On 04/06/2013 08:58, "Jan Beulich" <JBeulich@suse.com> wrote:
>>
>>> The first patch really isn't as much of a follow-up than what triggered
>>> the security issues to be noticed in the first place.
>>>
>>> 1: x86: preserve FPU selectors for 32-bit guest code
>>> 2: x86: fix XCR0 handling
>>> 3: x86/xsave: adjust state management
>>> 4: x86/fxsave: bring in line with recent xsave adjustments
>>>
>>> The first two I would see as candidates for 4.3 (as well as
>>> subsequent backporting, albeit I realize that especially the first
>>> one is non-trivial), while the third is code improvement only,
>>> and the fourth is really just cleanup, and hence I'd be fine with
>>> deferring them until after 4.3.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> I like the patches, 1 & 2 are good bug fixes.
>> Acked-by: Keir Fraser <keir@xen.org>
>>
>> Patch #1 is quite scary though! I wonder really whether these long-lived
>> issues must be fixed right now, let alone backported?
>
> Yeah, I was going to say, with all this tricky code going in, including
> this one, and the XSA-55 (?) one that seems to have tons of tricky
> changes, whether it might not be a good idea to make sure we have at
> least 2 weeks of testing and another test day -- or, delay the test day
> Wednesday until we can get all of these in.
Agreed, but I don't know what implications delaying a Test Day
would have. We certainly don't want to release in a rush with all
these new fixes.
> Jan, looking at the comments, it seems like 3 and 4 are more about
> performance than correctness? I think those should probably wait until
> the 4.4 dev window opens up.
Yes, as I also said in the overview description above. The question
is really just about the first two to go in right away.
Jan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/4] XSA-52..54 follow-up
2013-06-04 10:45 ` Jan Beulich
@ 2013-06-04 10:47 ` George Dunlap
2013-06-04 13:58 ` Jan Beulich
0 siblings, 1 reply; 13+ messages in thread
From: George Dunlap @ 2013-06-04 10:47 UTC (permalink / raw)
To: Jan Beulich; +Cc: LarsKurth, Keir Fraser, xen-devel
On 06/04/2013 11:45 AM, Jan Beulich wrote:
>>>> On 04.06.13 at 12:09, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>> On 06/04/2013 11:00 AM, Keir Fraser wrote:
>>> On 04/06/2013 08:58, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>
>>>> The first patch really isn't as much of a follow-up than what triggered
>>>> the security issues to be noticed in the first place.
>>>>
>>>> 1: x86: preserve FPU selectors for 32-bit guest code
>>>> 2: x86: fix XCR0 handling
>>>> 3: x86/xsave: adjust state management
>>>> 4: x86/fxsave: bring in line with recent xsave adjustments
>>>>
>>>> The first two I would see as candidates for 4.3 (as well as
>>>> subsequent backporting, albeit I realize that especially the first
>>>> one is non-trivial), while the third is code improvement only,
>>>> and the fourth is really just cleanup, and hence I'd be fine with
>>>> deferring them until after 4.3.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> I like the patches, 1 & 2 are good bug fixes.
>>> Acked-by: Keir Fraser <keir@xen.org>
>>>
>>> Patch #1 is quite scary though! I wonder really whether these long-lived
>>> issues must be fixed right now, let alone backported?
>>
>> Yeah, I was going to say, with all this tricky code going in, including
>> this one, and the XSA-55 (?) one that seems to have tons of tricky
>> changes, whether it might not be a good idea to make sure we have at
>> least 2 weeks of testing and another test day -- or, delay the test day
>> Wednesday until we can get all of these in.
>
> Agreed, but I don't know what implications delaying a Test Day
> would have. We certainly don't want to release in a rush with all
> these new fixes.
>
>> Jan, looking at the comments, it seems like 3 and 4 are more about
>> performance than correctness? I think those should probably wait until
>> the 4.4 dev window opens up.
>
> Yes, as I also said in the overview description above. The question
> is really just about the first two to go in right away.
These seem pretty clearly like things we need to have fixed in the
release -- they're the kind of thing that is likely to have potentially
nasty, hard-to-track-down side effects.
-George
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/4] XSA-52..54 follow-up
2013-06-04 10:47 ` George Dunlap
@ 2013-06-04 13:58 ` Jan Beulich
2013-06-04 14:55 ` George Dunlap
0 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2013-06-04 13:58 UTC (permalink / raw)
To: George Dunlap; +Cc: LarsKurth, Keir Fraser, xen-devel
>>> On 04.06.13 at 12:47, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> On 06/04/2013 11:45 AM, Jan Beulich wrote:
>>>>> On 04.06.13 at 12:09, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>>> On 06/04/2013 11:00 AM, Keir Fraser wrote:
>>>> On 04/06/2013 08:58, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>>
>>>>> The first patch really isn't as much of a follow-up than what triggered
>>>>> the security issues to be noticed in the first place.
>>>>>
>>>>> 1: x86: preserve FPU selectors for 32-bit guest code
>>>>> 2: x86: fix XCR0 handling
>>>>> 3: x86/xsave: adjust state management
>>>>> 4: x86/fxsave: bring in line with recent xsave adjustments
>>>>>
>>>>> The first two I would see as candidates for 4.3 (as well as
>>>>> subsequent backporting, albeit I realize that especially the first
>>>>> one is non-trivial), while the third is code improvement only,
>>>>> and the fourth is really just cleanup, and hence I'd be fine with
>>>>> deferring them until after 4.3.
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> I like the patches, 1 & 2 are good bug fixes.
>>>> Acked-by: Keir Fraser <keir@xen.org>
>>>>
>>>> Patch #1 is quite scary though! I wonder really whether these long-lived
>>>> issues must be fixed right now, let alone backported?
>>>
>>> Yeah, I was going to say, with all this tricky code going in, including
>>> this one, and the XSA-55 (?) one that seems to have tons of tricky
>>> changes, whether it might not be a good idea to make sure we have at
>>> least 2 weeks of testing and another test day -- or, delay the test day
>>> Wednesday until we can get all of these in.
>>
>> Agreed, but I don't know what implications delaying a Test Day
>> would have. We certainly don't want to release in a rush with all
>> these new fixes.
>>
>>> Jan, looking at the comments, it seems like 3 and 4 are more about
>>> performance than correctness? I think those should probably wait until
>>> the 4.4 dev window opens up.
>>
>> Yes, as I also said in the overview description above. The question
>> is really just about the first two to go in right away.
>
> These seem pretty clearly like things we need to have fixed in the
> release -- they're the kind of thing that is likely to have potentially
> nasty, hard-to-track-down side effects.
So together with Ben's most recent reply - do I read the above as
an ack for 4.3?
Jan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/4] XSA-52..54 follow-up
2013-06-04 13:58 ` Jan Beulich
@ 2013-06-04 14:55 ` George Dunlap
0 siblings, 0 replies; 13+ messages in thread
From: George Dunlap @ 2013-06-04 14:55 UTC (permalink / raw)
To: Jan Beulich; +Cc: LarsKurth, Keir Fraser, xen-devel
On Tue, Jun 4, 2013 at 2:58 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 04.06.13 at 12:47, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>> On 06/04/2013 11:45 AM, Jan Beulich wrote:
>>>>>> On 04.06.13 at 12:09, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>>>> On 06/04/2013 11:00 AM, Keir Fraser wrote:
>>>>> On 04/06/2013 08:58, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>>>
>>>>>> The first patch really isn't as much of a follow-up than what triggered
>>>>>> the security issues to be noticed in the first place.
>>>>>>
>>>>>> 1: x86: preserve FPU selectors for 32-bit guest code
>>>>>> 2: x86: fix XCR0 handling
>>>>>> 3: x86/xsave: adjust state management
>>>>>> 4: x86/fxsave: bring in line with recent xsave adjustments
>>>>>>
>>>>>> The first two I would see as candidates for 4.3 (as well as
>>>>>> subsequent backporting, albeit I realize that especially the first
>>>>>> one is non-trivial), while the third is code improvement only,
>>>>>> and the fourth is really just cleanup, and hence I'd be fine with
>>>>>> deferring them until after 4.3.
>>>>>>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>
>>>>> I like the patches, 1 & 2 are good bug fixes.
>>>>> Acked-by: Keir Fraser <keir@xen.org>
>>>>>
>>>>> Patch #1 is quite scary though! I wonder really whether these long-lived
>>>>> issues must be fixed right now, let alone backported?
>>>>
>>>> Yeah, I was going to say, with all this tricky code going in, including
>>>> this one, and the XSA-55 (?) one that seems to have tons of tricky
>>>> changes, whether it might not be a good idea to make sure we have at
>>>> least 2 weeks of testing and another test day -- or, delay the test day
>>>> Wednesday until we can get all of these in.
>>>
>>> Agreed, but I don't know what implications delaying a Test Day
>>> would have. We certainly don't want to release in a rush with all
>>> these new fixes.
>>>
>>>> Jan, looking at the comments, it seems like 3 and 4 are more about
>>>> performance than correctness? I think those should probably wait until
>>>> the 4.4 dev window opens up.
>>>
>>> Yes, as I also said in the overview description above. The question
>>> is really just about the first two to go in right away.
>>
>> These seem pretty clearly like things we need to have fixed in the
>> release -- they're the kind of thing that is likely to have potentially
>> nasty, hard-to-track-down side effects.
>
> So together with Ben's most recent reply - do I read the above as
> an ack for 4.3?
Yes; the two bug fixes, re the release:
Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/4] XSA-52..54 follow-up
2013-06-04 10:00 ` [PATCH 0/4] XSA-52..54 follow-up Keir Fraser
2013-06-04 10:09 ` George Dunlap
@ 2013-06-04 10:32 ` Jan Beulich
2013-06-04 12:43 ` Ben Guthro
1 sibling, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2013-06-04 10:32 UTC (permalink / raw)
To: Keir Fraser; +Cc: George Dunlap, Ben Guthro, xen-devel
>>> On 04.06.13 at 12:00, Keir Fraser <keir.xen@gmail.com> wrote:
> On 04/06/2013 08:58, "Jan Beulich" <JBeulich@suse.com> wrote:
>
>> The first patch really isn't as much of a follow-up than what triggered
>> the security issues to be noticed in the first place.
>>
>> 1: x86: preserve FPU selectors for 32-bit guest code
>> 2: x86: fix XCR0 handling
>> 3: x86/xsave: adjust state management
>> 4: x86/fxsave: bring in line with recent xsave adjustments
>>
>> The first two I would see as candidates for 4.3 (as well as
>> subsequent backporting, albeit I realize that especially the first
>> one is non-trivial), while the third is code improvement only,
>> and the fourth is really just cleanup, and hence I'd be fine with
>> deferring them until after 4.3.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> I like the patches, 1 & 2 are good bug fixes.
> Acked-by: Keir Fraser <keir@xen.org>
>
> Patch #1 is quite scary though! I wonder really whether these long-lived
> issues must be fixed right now, let alone backported?
You can also view this as "finally we get these long standing
issues fixed" - I was aware of the selector corruption since back
when dealing with the original FXSAVE information leak, just that
no-one had an issue with this in practice.
Ben, who originally initiated this work, might want to give an
estimation on how important these fixes are for their practical
purposes. With that, I'll then defer to George for whether to
pull them in right now or only once branched.
Jan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/4] XSA-52..54 follow-up
2013-06-04 10:32 ` Jan Beulich
@ 2013-06-04 12:43 ` Ben Guthro
0 siblings, 0 replies; 13+ messages in thread
From: Ben Guthro @ 2013-06-04 12:43 UTC (permalink / raw)
To: Jan Beulich; +Cc: George Dunlap, Keir Fraser, xen-devel
On 06/04/2013 06:32 AM, Jan Beulich wrote:
>>>> On 04.06.13 at 12:00, Keir Fraser <keir.xen@gmail.com> wrote:
>> On 04/06/2013 08:58, "Jan Beulich" <JBeulich@suse.com> wrote:
>>
>>> The first patch really isn't as much of a follow-up than what triggered
>>> the security issues to be noticed in the first place.
>>>
>>> 1: x86: preserve FPU selectors for 32-bit guest code
>>> 2: x86: fix XCR0 handling
>>> 3: x86/xsave: adjust state management
>>> 4: x86/fxsave: bring in line with recent xsave adjustments
>>>
>>> The first two I would see as candidates for 4.3 (as well as
>>> subsequent backporting, albeit I realize that especially the first
>>> one is non-trivial), while the third is code improvement only,
>>> and the fourth is really just cleanup, and hence I'd be fine with
>>> deferring them until after 4.3.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> I like the patches, 1 & 2 are good bug fixes.
>> Acked-by: Keir Fraser <keir@xen.org>
>>
>> Patch #1 is quite scary though! I wonder really whether these long-lived
>> issues must be fixed right now, let alone backported?
>
> You can also view this as "finally we get these long standing
> issues fixed" - I was aware of the selector corruption since back
> when dealing with the original FXSAVE information leak, just that
> no-one had an issue with this in practice.
>
> Ben, who originally initiated this work, might want to give an
> estimation on how important these fixes are for their practical
> purposes. With that, I'll then defer to George for whether to
> pull them in right now or only once branched.
This was more of an exercise of QA exercising of code paths uncovering a
bug, rather than real-world usage implications.
Microsoft ships something called the "driver verifier"[1] in every OS
after XP, that can dynamically be turned on for any driver. When enabled
for some Citrix PV drivers, they would bug-check (occasionally) during
QA nightly test runs on 32bit OSes on this XSAVE/XRSTOR data mismatch.
To the best of my knowledge, outside of this somewhat contrived
scenario, we never saw this fail in any real-world usage. Therefore, I
think the assessment of deferring until after 4.3 is prudent, given the
release cycle stage.
With my downstream hat on, it would certainly be nice to see these
backported to 4.2 though, once in mainline, so we don't have to carry a
patch. However, we can always do so, if necessary.
Ben
[1] http://support.microsoft.com/kb/244617
^ permalink raw reply [flat|nested] 13+ messages in thread