|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] [PATCH v2 3/4] x86/xstate: fix fault behavior on XRSTORS
XRSTORS unconditionally faults when xcomp_bv has bit 63 clear. Instead
of just fixing this issue, overhaul the fault recovery code, which -
one of the many mistakes made when xstate support got introduced - was
blindly mirroring that accompanying FXRSTOR, neglecting the fact that
XRSTOR{,S} aren't all-or-nothing instructions. The new code, first of
all, does all the recovery actions in C, simplifying the inline
assembly used. And it does its work in a multi-stage fashion: Upon
first seeing a fault, state fixups get applied strictly based on what
architecturally may cause #GP. When seeing another fault despite the
fixups done, state gets fully reset. A third fault would then lead to
crashing the domain (instead of hanging the hypervisor in an infinite
loop of recurring faults).
Reported-by: Harmandeep Kaur <write.harmandeep@xxxxxxxxx>
Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
---
v2: Fix default MXCSR mask value. Avoid infinite fault recovery loop.
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -29,6 +29,8 @@ unsigned int *__read_mostly xstate_sizes
static unsigned int __read_mostly xstate_features;
static unsigned int __read_mostly xstate_comp_offsets[sizeof(xfeature_mask)*8];
+static uint32_t __read_mostly mxcsr_mask = 0x0000ffbf;
+
/* Cached xcr0 for fast read */
static DEFINE_PER_CPU(uint64_t, xcr0);
@@ -342,6 +344,7 @@ void xrstor(struct vcpu *v, uint64_t mas
uint32_t hmask = mask >> 32;
uint32_t lmask = mask;
struct xsave_struct *ptr = v->arch.xsave_area;
+ unsigned int faults, prev_faults;
/*
* AMD CPUs don't save/restore FDP/FIP/FOP unless an exception
@@ -361,35 +364,84 @@ void xrstor(struct vcpu *v, uint64_t mas
/*
* XRSTOR 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.
+ * tools or through VCPUOP_initialise, by silently adjusting state.
*/
- switch ( __builtin_expect(ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET], 8) )
+ for ( prev_faults = faults = 0; ; prev_faults = faults )
{
+ switch ( __builtin_expect(ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET], 8) )
+ {
#define XRSTOR(pfx) \
alternative_io("1: .byte " pfx "0x0f,0xae,0x2f\n" \
+ "3:\n" \
" .section .fixup,\"ax\"\n" \
- "2: mov %[size],%%ecx\n" \
- " xor %[lmask_out],%[lmask_out]\n" \
- " rep stosb\n" \
- " lea %[mem],%[ptr]\n" \
- " mov %[lmask_in],%[lmask_out]\n" \
- " jmp 1b\n" \
+ "2: inc%z[faults] %[faults]\n" \
+ " jmp 3b\n" \
" .previous\n" \
_ASM_EXTABLE(1b, 2b), \
".byte " pfx "0x0f,0xc7,0x1f\n", \
X86_FEATURE_XSAVES, \
- ASM_OUTPUT2([ptr] "+&D" (ptr), [lmask_out] "+&a"
(lmask)), \
- [mem] "m" (*ptr), [lmask_in] "g" (lmask), \
- [hmask] "d" (hmask), [size] "m" (xsave_cntxt_size) \
- : "ecx")
-
- default:
- XRSTOR("0x48,");
- break;
- case 4: case 2:
- XRSTOR("");
- break;
+ ASM_OUTPUT2([mem] "+m" (*ptr), [faults] "+g" (faults)),
\
+ [lmask] "a" (lmask), [hmask] "d" (hmask), \
+ [ptr] "D" (ptr))
+
+ default:
+ XRSTOR("0x48,");
+ break;
+ case 4: case 2:
+ XRSTOR("");
+ break;
#undef XRSTOR
+ }
+ if ( likely(faults == prev_faults) )
+ break;
+#ifndef NDEBUG
+ gprintk(XENLOG_WARNING, "fault#%u: mxcsr=%08x\n",
+ faults, ptr->fpu_sse.mxcsr);
+ gprintk(XENLOG_WARNING, "xs=%016lx xc=%016lx\n",
+ ptr->xsave_hdr.xstate_bv, ptr->xsave_hdr.xcomp_bv);
+ gprintk(XENLOG_WARNING, "r0=%016lx r1=%016lx\n",
+ ptr->xsave_hdr.reserved[0], ptr->xsave_hdr.reserved[1]);
+ gprintk(XENLOG_WARNING, "r2=%016lx r3=%016lx\n",
+ ptr->xsave_hdr.reserved[2], ptr->xsave_hdr.reserved[3]);
+ gprintk(XENLOG_WARNING, "r4=%016lx r5=%016lx\n",
+ ptr->xsave_hdr.reserved[4], ptr->xsave_hdr.reserved[5]);
+#endif
+ switch ( faults )
+ {
+ case 1: /* Stage 1: Reset state to be loaded. */
+ ptr->xsave_hdr.xstate_bv &= ~mask;
+ /*
+ * Also try to eliminate fault reasons, even if this shouldn't be
+ * needed here (other code should ensure the sanity of the data).
+ */
+ if ( ((mask & XSTATE_SSE) ||
+ ((mask & XSTATE_YMM) &&
+ !(ptr->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED))) )
+ ptr->fpu_sse.mxcsr &= mxcsr_mask;
+ if ( cpu_has_xsaves || cpu_has_xsavec )
+ {
+ ptr->xsave_hdr.xcomp_bv &= this_cpu(xcr0) | this_cpu(xss);
+ ptr->xsave_hdr.xstate_bv &= ptr->xsave_hdr.xcomp_bv;
+ ptr->xsave_hdr.xcomp_bv |= XSTATE_COMPACTION_ENABLED;
+ }
+ else
+ {
+ ptr->xsave_hdr.xstate_bv &= this_cpu(xcr0);
+ ptr->xsave_hdr.xcomp_bv = 0;
+ }
+ memset(ptr->xsave_hdr.reserved, 0,
sizeof(ptr->xsave_hdr.reserved));
+ continue;
+
+ 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
+ ? XSTATE_COMPACTION_ENABLED : 0;
+ continue;
+ }
+
+ domain_crash(current->domain);
+ return;
}
}
@@ -496,6 +548,8 @@ void xstate_init(struct cpuinfo_x86 *c)
if ( bsp )
{
+ static typeof(current->arch.xsave_area->fpu_sse) __initdata ctxt;
+
xfeature_mask = feature_mask;
/*
* xsave_cntxt_size is the max size required by enabled features.
@@ -504,6 +558,10 @@ void xstate_init(struct cpuinfo_x86 *c)
xsave_cntxt_size = _xstate_ctxt_size(feature_mask);
printk("%s: using cntxt_size: %#x and states: %#"PRIx64"\n",
__func__, xsave_cntxt_size, xfeature_mask);
+
+ asm ( "fxsave %0" : "=m" (ctxt) );
+ if ( ctxt.mxcsr_mask )
+ mxcsr_mask = ctxt.mxcsr_mask;
}
else
{
Attachment:
x86-xrstors-fault.patch _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |