* [PATCH v2 0/3] x86emul: FPU handling corrections
@ 2017-03-15 10:23 Jan Beulich
2017-03-15 10:27 ` [PATCH v2 1/3] x86emul: centralize put_fpu() invocations Jan Beulich
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Jan Beulich @ 2017-03-15 10:23 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper
1: centralize put_fpu() invocations
2: correct handling of FPU insns faulting on memory write
3: correct FPU code/data pointers and opcode handling
XTF: add FPU/SIMD register state test
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Patch 1 and the XTF one changed (see there), the other two
needed adjustment because of the change to #1.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/3] x86emul: centralize put_fpu() invocations
2017-03-15 10:23 [PATCH v2 0/3] x86emul: FPU handling corrections Jan Beulich
@ 2017-03-15 10:27 ` Jan Beulich
2017-03-20 17:10 ` Andrew Cooper
2017-03-15 10:28 ` [PATCH v2 2/3] x86emul: correct handling of FPU insns faulting on memory write Jan Beulich
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Jan Beulich @ 2017-03-15 10:27 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper
[-- Attachment #1: Type: text/plain, Size: 9266 bytes --]
..., splitting parts of it into check_*() macros. This is in
preparation of making ->put_fpu() do further adjustments to register
state. (Some of the check_xmm() invocations could be avoided, as in
some of the cases no insns handled there can actually raise #XM, but I
think we're better off keeping them to avoid later additions of further
insn patterns rendering the lack of the check a bug.)
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: check_fpu() -> check_fpu_exn(). check_xmm() -> check_xmm_exn().
Drop stray underscores from their parameter names. Avoid explicit
conditional on second put_fpu(). Assert type is not
X86EMUL_FPU_none in _get_fpu().
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -937,6 +937,7 @@ do {
struct fpu_insn_ctxt {
uint8_t insn_bytes;
+ uint8_t type;
int8_t exn_raised;
};
@@ -956,15 +957,16 @@ static int _get_fpu(
{
int rc;
- fic->exn_raised = -1;
-
fail_if(!ops->get_fpu);
+ ASSERT(type != X86EMUL_FPU_none);
rc = ops->get_fpu(fpu_handle_exception, fic, type, ctxt);
if ( rc == X86EMUL_OKAY )
{
unsigned long cr0;
+ fic->type = type;
+
fail_if(!ops->read_cr);
if ( type >= X86EMUL_FPU_xmm )
{
@@ -1006,22 +1008,32 @@ do {
rc = _get_fpu(_type, _fic, ctxt, ops); \
if ( rc ) goto done; \
} while (0)
-#define _put_fpu() \
+
+#define check_fpu_exn(fic) \
do { \
- if ( ops->put_fpu != NULL ) \
- (ops->put_fpu)(ctxt); \
+ generate_exception_if((fic)->exn_raised >= 0, \
+ (fic)->exn_raised); \
} while (0)
-#define put_fpu(_fic) \
+
+#define check_xmm_exn(fic) \
do { \
- _put_fpu(); \
- if ( (_fic)->exn_raised == EXC_XM && ops->read_cr && \
+ if ( (fic)->exn_raised == EXC_XM && ops->read_cr && \
ops->read_cr(4, &cr4, ctxt) == X86EMUL_OKAY && \
- !(cr4 & X86_CR4_OSXMMEXCPT) ) \
- (_fic)->exn_raised = EXC_UD; \
- generate_exception_if((_fic)->exn_raised >= 0, \
- (_fic)->exn_raised); \
+ !(cr4 & X86_CR4_OSXMMEXCPT) ) \
+ (fic)->exn_raised = EXC_UD; \
+ check_fpu_exn(fic); \
} while (0)
+static void put_fpu(
+ struct fpu_insn_ctxt *fic,
+ struct x86_emulate_ctxt *ctxt,
+ const struct x86_emulate_ops *ops)
+{
+ if ( fic->type != X86EMUL_FPU_none && ops->put_fpu )
+ ops->put_fpu(ctxt);
+ fic->type = X86EMUL_FPU_none;
+}
+
static inline bool fpu_check_write(void)
{
uint16_t fsw;
@@ -3015,7 +3027,7 @@ x86_emulate(
struct operand dst = { .reg = PTR_POISON };
unsigned long cr4;
enum x86_swint_type swint_type;
- struct fpu_insn_ctxt fic;
+ struct fpu_insn_ctxt fic = { .type = X86EMUL_FPU_none, .exn_raised = -1 };
struct x86_emulate_stub stub = {};
DECLARE_ALIGNED(mmval_t, mmval);
@@ -3708,7 +3720,7 @@ x86_emulate(
host_and_vcpu_must_have(fpu);
get_fpu(X86EMUL_FPU_wait, &fic);
asm volatile ( "fwait" ::: "memory" );
- put_fpu(&fic);
+ check_fpu_exn(&fic);
break;
case 0x9c: /* pushf */
@@ -4153,7 +4165,7 @@ x86_emulate(
break;
}
}
- put_fpu(&fic);
+ check_fpu_exn(&fic);
break;
case 0xd9: /* FPU 0xd9 */
@@ -4242,7 +4254,7 @@ x86_emulate(
if ( dst.type == OP_MEM && dst.bytes == 4 && !fpu_check_write() )
dst.type = OP_NONE;
}
- put_fpu(&fic);
+ check_fpu_exn(&fic);
break;
case 0xda: /* FPU 0xda */
@@ -4293,7 +4305,7 @@ x86_emulate(
break;
}
}
- put_fpu(&fic);
+ check_fpu_exn(&fic);
break;
case 0xdb: /* FPU 0xdb */
@@ -4365,7 +4377,7 @@ x86_emulate(
if ( dst.type == OP_MEM && !fpu_check_write() )
dst.type = OP_NONE;
}
- put_fpu(&fic);
+ check_fpu_exn(&fic);
break;
case 0xdc: /* FPU 0xdc */
@@ -4416,7 +4428,7 @@ x86_emulate(
break;
}
}
- put_fpu(&fic);
+ check_fpu_exn(&fic);
break;
case 0xdd: /* FPU 0xdd */
@@ -4475,7 +4487,7 @@ x86_emulate(
if ( dst.type == OP_MEM && dst.bytes == 8 && !fpu_check_write() )
dst.type = OP_NONE;
}
- put_fpu(&fic);
+ check_fpu_exn(&fic);
break;
case 0xde: /* FPU 0xde */
@@ -4523,7 +4535,7 @@ x86_emulate(
break;
}
}
- put_fpu(&fic);
+ check_fpu_exn(&fic);
break;
case 0xdf: /* FPU 0xdf */
@@ -4605,7 +4617,7 @@ x86_emulate(
if ( dst.type == OP_MEM && !fpu_check_write() )
dst.type = OP_NONE;
}
- put_fpu(&fic);
+ check_fpu_exn(&fic);
break;
case 0xe0 ... 0xe2: /* loop{,z,nz} */ {
@@ -5667,7 +5679,7 @@ x86_emulate(
: "c" (mmvalp), "m" (*mmvalp));
put_stub(stub);
- put_fpu(&fic);
+ check_xmm_exn(&fic);
state->simd_size = simd_none;
break;
@@ -5719,7 +5731,7 @@ x86_emulate(
[mask] "i" (EFLAGS_MASK));
put_stub(stub);
- put_fpu(&fic);
+ check_xmm_exn(&fic);
ASSERT(!state->simd_size);
break;
@@ -5903,7 +5915,7 @@ x86_emulate(
invoke_stub("", "", "=a" (dst.val) : [dummy] "i" (0));
put_stub(stub);
- put_fpu(&fic);
+ check_xmm_exn(&fic);
ASSERT(!state->simd_size);
dst.bytes = 4;
@@ -6111,7 +6123,7 @@ x86_emulate(
dst.val = src.val;
put_stub(stub);
- put_fpu(&fic);
+ check_xmm_exn(&fic);
ASSERT(!state->simd_size);
break;
@@ -6253,7 +6265,7 @@ x86_emulate(
invoke_stub("", "", [dummy_out] "=g" (dummy) : [dummy_in] "i" (0) );
put_stub(stub);
- put_fpu(&fic);
+ check_xmm_exn(&fic);
ASSERT(!state->simd_size);
break;
@@ -6323,8 +6335,6 @@ x86_emulate(
asm volatile ( ".byte 0xc5,0xc1,0xeb,0xff" );
}
- put_fpu(&fic);
-
ASSERT(!state->simd_size);
break;
}
@@ -7072,10 +7082,7 @@ x86_emulate(
put_stub(stub);
if ( !ea.val )
- {
- put_fpu(&fic);
goto complete_insn;
- }
opc = init_prefixes(stub);
opc[0] = b;
@@ -7223,7 +7230,7 @@ x86_emulate(
emulate_stub("+m" (*mmvalp), "a" (mmvalp));
put_stub(stub);
- put_fpu(&fic);
+ check_xmm_exn(&fic);
state->simd_size = simd_none;
dst.type = OP_NONE;
@@ -7527,7 +7534,7 @@ x86_emulate(
invoke_stub("", "", "=m" (dst.val) : "a" (&dst.val));
put_stub(stub);
- put_fpu(&fic);
+ check_xmm_exn(&fic);
ASSERT(!state->simd_size);
dst.bytes = dst.type == OP_REG || b == 0x17 ? 4 : 1 << (b & 3);
@@ -7843,7 +7850,7 @@ x86_emulate(
invoke_stub("", "", "+m" (*mmvalp) : "D" (mmvalp));
put_stub(stub);
- put_fpu(&fic);
+ check_xmm_exn(&fic);
}
switch ( dst.type )
@@ -7885,6 +7892,8 @@ x86_emulate(
}
complete_insn: /* Commit shadow register state. */
+ put_fpu(&fic, ctxt, ops);
+
/* Zero the upper 32 bits of %rip if not in 64-bit mode. */
if ( !mode_64bit() )
_regs.r(ip) = _regs.eip;
@@ -7907,7 +7916,7 @@ x86_emulate(
ctxt->regs->eflags &= ~X86_EFLAGS_RF;
done:
- _put_fpu();
+ put_fpu(&fic, ctxt, ops);
put_stub(stub);
return rc;
#undef state
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -161,7 +161,9 @@ enum x86_emulate_fpu_type {
X86EMUL_FPU_wait, /* WAIT/FWAIT instruction */
X86EMUL_FPU_mmx, /* MMX instruction set (%mm0-%mm7) */
X86EMUL_FPU_xmm, /* SSE instruction set (%xmm0-%xmm7/15) */
- X86EMUL_FPU_ymm /* AVX/XOP instruction set (%ymm0-%ymm7/15) */
+ X86EMUL_FPU_ymm, /* AVX/XOP instruction set (%ymm0-%ymm7/15) */
+ /* This sentinel will never be passed to ->get_fpu(). */
+ X86EMUL_FPU_none
};
struct cpuid_leaf
[-- Attachment #2: x86emul-move-put_fpu.patch --]
[-- Type: text/plain, Size: 9307 bytes --]
x86emul: centralize put_fpu() invocations
..., splitting parts of it into check_*() macros. This is in
preparation of making ->put_fpu() do further adjustments to register
state. (Some of the check_xmm() invocations could be avoided, as in
some of the cases no insns handled there can actually raise #XM, but I
think we're better off keeping them to avoid later additions of further
insn patterns rendering the lack of the check a bug.)
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: check_fpu() -> check_fpu_exn(). check_xmm() -> check_xmm_exn().
Drop stray underscores from their parameter names. Avoid explicit
conditional on second put_fpu(). Assert type is not
X86EMUL_FPU_none in _get_fpu().
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -937,6 +937,7 @@ do {
struct fpu_insn_ctxt {
uint8_t insn_bytes;
+ uint8_t type;
int8_t exn_raised;
};
@@ -956,15 +957,16 @@ static int _get_fpu(
{
int rc;
- fic->exn_raised = -1;
-
fail_if(!ops->get_fpu);
+ ASSERT(type != X86EMUL_FPU_none);
rc = ops->get_fpu(fpu_handle_exception, fic, type, ctxt);
if ( rc == X86EMUL_OKAY )
{
unsigned long cr0;
+ fic->type = type;
+
fail_if(!ops->read_cr);
if ( type >= X86EMUL_FPU_xmm )
{
@@ -1006,22 +1008,32 @@ do {
rc = _get_fpu(_type, _fic, ctxt, ops); \
if ( rc ) goto done; \
} while (0)
-#define _put_fpu() \
+
+#define check_fpu_exn(fic) \
do { \
- if ( ops->put_fpu != NULL ) \
- (ops->put_fpu)(ctxt); \
+ generate_exception_if((fic)->exn_raised >= 0, \
+ (fic)->exn_raised); \
} while (0)
-#define put_fpu(_fic) \
+
+#define check_xmm_exn(fic) \
do { \
- _put_fpu(); \
- if ( (_fic)->exn_raised == EXC_XM && ops->read_cr && \
+ if ( (fic)->exn_raised == EXC_XM && ops->read_cr && \
ops->read_cr(4, &cr4, ctxt) == X86EMUL_OKAY && \
- !(cr4 & X86_CR4_OSXMMEXCPT) ) \
- (_fic)->exn_raised = EXC_UD; \
- generate_exception_if((_fic)->exn_raised >= 0, \
- (_fic)->exn_raised); \
+ !(cr4 & X86_CR4_OSXMMEXCPT) ) \
+ (fic)->exn_raised = EXC_UD; \
+ check_fpu_exn(fic); \
} while (0)
+static void put_fpu(
+ struct fpu_insn_ctxt *fic,
+ struct x86_emulate_ctxt *ctxt,
+ const struct x86_emulate_ops *ops)
+{
+ if ( fic->type != X86EMUL_FPU_none && ops->put_fpu )
+ ops->put_fpu(ctxt);
+ fic->type = X86EMUL_FPU_none;
+}
+
static inline bool fpu_check_write(void)
{
uint16_t fsw;
@@ -3015,7 +3027,7 @@ x86_emulate(
struct operand dst = { .reg = PTR_POISON };
unsigned long cr4;
enum x86_swint_type swint_type;
- struct fpu_insn_ctxt fic;
+ struct fpu_insn_ctxt fic = { .type = X86EMUL_FPU_none, .exn_raised = -1 };
struct x86_emulate_stub stub = {};
DECLARE_ALIGNED(mmval_t, mmval);
@@ -3708,7 +3720,7 @@ x86_emulate(
host_and_vcpu_must_have(fpu);
get_fpu(X86EMUL_FPU_wait, &fic);
asm volatile ( "fwait" ::: "memory" );
- put_fpu(&fic);
+ check_fpu_exn(&fic);
break;
case 0x9c: /* pushf */
@@ -4153,7 +4165,7 @@ x86_emulate(
break;
}
}
- put_fpu(&fic);
+ check_fpu_exn(&fic);
break;
case 0xd9: /* FPU 0xd9 */
@@ -4242,7 +4254,7 @@ x86_emulate(
if ( dst.type == OP_MEM && dst.bytes == 4 && !fpu_check_write() )
dst.type = OP_NONE;
}
- put_fpu(&fic);
+ check_fpu_exn(&fic);
break;
case 0xda: /* FPU 0xda */
@@ -4293,7 +4305,7 @@ x86_emulate(
break;
}
}
- put_fpu(&fic);
+ check_fpu_exn(&fic);
break;
case 0xdb: /* FPU 0xdb */
@@ -4365,7 +4377,7 @@ x86_emulate(
if ( dst.type == OP_MEM && !fpu_check_write() )
dst.type = OP_NONE;
}
- put_fpu(&fic);
+ check_fpu_exn(&fic);
break;
case 0xdc: /* FPU 0xdc */
@@ -4416,7 +4428,7 @@ x86_emulate(
break;
}
}
- put_fpu(&fic);
+ check_fpu_exn(&fic);
break;
case 0xdd: /* FPU 0xdd */
@@ -4475,7 +4487,7 @@ x86_emulate(
if ( dst.type == OP_MEM && dst.bytes == 8 && !fpu_check_write() )
dst.type = OP_NONE;
}
- put_fpu(&fic);
+ check_fpu_exn(&fic);
break;
case 0xde: /* FPU 0xde */
@@ -4523,7 +4535,7 @@ x86_emulate(
break;
}
}
- put_fpu(&fic);
+ check_fpu_exn(&fic);
break;
case 0xdf: /* FPU 0xdf */
@@ -4605,7 +4617,7 @@ x86_emulate(
if ( dst.type == OP_MEM && !fpu_check_write() )
dst.type = OP_NONE;
}
- put_fpu(&fic);
+ check_fpu_exn(&fic);
break;
case 0xe0 ... 0xe2: /* loop{,z,nz} */ {
@@ -5667,7 +5679,7 @@ x86_emulate(
: "c" (mmvalp), "m" (*mmvalp));
put_stub(stub);
- put_fpu(&fic);
+ check_xmm_exn(&fic);
state->simd_size = simd_none;
break;
@@ -5719,7 +5731,7 @@ x86_emulate(
[mask] "i" (EFLAGS_MASK));
put_stub(stub);
- put_fpu(&fic);
+ check_xmm_exn(&fic);
ASSERT(!state->simd_size);
break;
@@ -5903,7 +5915,7 @@ x86_emulate(
invoke_stub("", "", "=a" (dst.val) : [dummy] "i" (0));
put_stub(stub);
- put_fpu(&fic);
+ check_xmm_exn(&fic);
ASSERT(!state->simd_size);
dst.bytes = 4;
@@ -6111,7 +6123,7 @@ x86_emulate(
dst.val = src.val;
put_stub(stub);
- put_fpu(&fic);
+ check_xmm_exn(&fic);
ASSERT(!state->simd_size);
break;
@@ -6253,7 +6265,7 @@ x86_emulate(
invoke_stub("", "", [dummy_out] "=g" (dummy) : [dummy_in] "i" (0) );
put_stub(stub);
- put_fpu(&fic);
+ check_xmm_exn(&fic);
ASSERT(!state->simd_size);
break;
@@ -6323,8 +6335,6 @@ x86_emulate(
asm volatile ( ".byte 0xc5,0xc1,0xeb,0xff" );
}
- put_fpu(&fic);
-
ASSERT(!state->simd_size);
break;
}
@@ -7072,10 +7082,7 @@ x86_emulate(
put_stub(stub);
if ( !ea.val )
- {
- put_fpu(&fic);
goto complete_insn;
- }
opc = init_prefixes(stub);
opc[0] = b;
@@ -7223,7 +7230,7 @@ x86_emulate(
emulate_stub("+m" (*mmvalp), "a" (mmvalp));
put_stub(stub);
- put_fpu(&fic);
+ check_xmm_exn(&fic);
state->simd_size = simd_none;
dst.type = OP_NONE;
@@ -7527,7 +7534,7 @@ x86_emulate(
invoke_stub("", "", "=m" (dst.val) : "a" (&dst.val));
put_stub(stub);
- put_fpu(&fic);
+ check_xmm_exn(&fic);
ASSERT(!state->simd_size);
dst.bytes = dst.type == OP_REG || b == 0x17 ? 4 : 1 << (b & 3);
@@ -7843,7 +7850,7 @@ x86_emulate(
invoke_stub("", "", "+m" (*mmvalp) : "D" (mmvalp));
put_stub(stub);
- put_fpu(&fic);
+ check_xmm_exn(&fic);
}
switch ( dst.type )
@@ -7885,6 +7892,8 @@ x86_emulate(
}
complete_insn: /* Commit shadow register state. */
+ put_fpu(&fic, ctxt, ops);
+
/* Zero the upper 32 bits of %rip if not in 64-bit mode. */
if ( !mode_64bit() )
_regs.r(ip) = _regs.eip;
@@ -7907,7 +7916,7 @@ x86_emulate(
ctxt->regs->eflags &= ~X86_EFLAGS_RF;
done:
- _put_fpu();
+ put_fpu(&fic, ctxt, ops);
put_stub(stub);
return rc;
#undef state
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -161,7 +161,9 @@ enum x86_emulate_fpu_type {
X86EMUL_FPU_wait, /* WAIT/FWAIT instruction */
X86EMUL_FPU_mmx, /* MMX instruction set (%mm0-%mm7) */
X86EMUL_FPU_xmm, /* SSE instruction set (%xmm0-%xmm7/15) */
- X86EMUL_FPU_ymm /* AVX/XOP instruction set (%ymm0-%ymm7/15) */
+ X86EMUL_FPU_ymm, /* AVX/XOP instruction set (%ymm0-%ymm7/15) */
+ /* This sentinel will never be passed to ->get_fpu(). */
+ X86EMUL_FPU_none
};
struct cpuid_leaf
[-- Attachment #3: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 2/3] x86emul: correct handling of FPU insns faulting on memory write
2017-03-15 10:23 [PATCH v2 0/3] x86emul: FPU handling corrections Jan Beulich
2017-03-15 10:27 ` [PATCH v2 1/3] x86emul: centralize put_fpu() invocations Jan Beulich
@ 2017-03-15 10:28 ` Jan Beulich
2017-03-15 12:06 ` Paul Durrant
2017-03-15 13:24 ` Boris Ostrovsky
2017-03-15 10:28 ` [PATCH v2 3/3] x86emul: correct FPU code/data pointers and opcode handling Jan Beulich
2017-03-15 10:29 ` [PATCH v2][XTF] add FPU/SIMD register state test Jan Beulich
3 siblings, 2 replies; 12+ messages in thread
From: Jan Beulich @ 2017-03-15 10:28 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Suravee Suthikulpanit, Andrew Cooper, Paul Durrant,
Jun Nakajima, Boris Ostrovsky
[-- Attachment #1: Type: text/plain, Size: 8838 bytes --]
When an FPU instruction with a memory destination fails during the
memory write, it should not affect FPU register state. Due to the way
we emulate FPU (and SIMD) instructions, we can only guarantee this by
- backing out changes to the FPU register state in such a case or
- doing a descriptor read and/or page walk up front, perhaps with the
stubs accessing the actual memory location then.
The latter would require a significant change in how the emulator does
its guest memory accessing, so for now the former variant is being
chosen.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
---
v2: Re-base.
---
Note that the state save overhead (unless state hadn't been loaded at
all before, which should only be possible if a guest is fiddling with
the instruction stream under emulation) is taken for every FPU insn
hitting the emulator. We could reduce this to just the ones writing to
memory, but that would involve quite a few further changes and
resulting code where even more code paths need to match up with one
another.
--- a/tools/fuzz/x86_instruction_emulator/x86-insn-emulator-fuzzer.c
+++ b/tools/fuzz/x86_instruction_emulator/x86-insn-emulator-fuzzer.c
@@ -433,6 +433,7 @@ static struct x86_emulate_ops fuzz_emulo
SET(wbinvd),
SET(invlpg),
.get_fpu = emul_test_get_fpu,
+ .put_fpu = emul_test_put_fpu,
.cpuid = emul_test_cpuid,
};
#undef SET
--- a/tools/tests/x86_emulator/test_x86_emulator.c
+++ b/tools/tests/x86_emulator/test_x86_emulator.c
@@ -293,6 +293,7 @@ static struct x86_emulate_ops emulops =
.read_cr = emul_test_read_cr,
.read_msr = read_msr,
.get_fpu = emul_test_get_fpu,
+ .put_fpu = emul_test_put_fpu,
};
int main(int argc, char **argv)
--- a/tools/tests/x86_emulator/x86_emulate.c
+++ b/tools/tests/x86_emulator/x86_emulate.c
@@ -138,4 +138,11 @@ int emul_test_get_fpu(
return X86EMUL_OKAY;
}
+void emul_test_put_fpu(
+ struct x86_emulate_ctxt *ctxt,
+ enum x86_emulate_fpu_type backout)
+{
+ /* TBD */
+}
+
#include "x86_emulate/x86_emulate.c"
--- a/tools/tests/x86_emulator/x86_emulate.h
+++ b/tools/tests/x86_emulator/x86_emulate.h
@@ -178,3 +178,7 @@ int emul_test_get_fpu(
void *exception_callback_arg,
enum x86_emulate_fpu_type type,
struct x86_emulate_ctxt *ctxt);
+
+void emul_test_put_fpu(
+ struct x86_emulate_ctxt *ctxt,
+ enum x86_emulate_fpu_type backout);
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -15,6 +15,7 @@
#include <xen/paging.h>
#include <xen/trace.h>
#include <asm/event.h>
+#include <asm/i387.h>
#include <asm/xstate.h>
#include <asm/hvm/emulate.h>
#include <asm/hvm/hvm.h>
@@ -1619,6 +1620,35 @@ static int hvmemul_get_fpu(
if ( !curr->fpu_dirtied )
hvm_funcs.fpu_dirty_intercept();
+ else if ( type == X86EMUL_FPU_fpu )
+ {
+ const typeof(curr->arch.xsave_area->fpu_sse) *fpu_ctxt =
+ curr->arch.fpu_ctxt;
+
+ /*
+ * Latch current register state so that we can back out changes
+ * if needed (namely when a memory write fails after register state
+ * has already been updated).
+ * NB: We don't really need the "enable" part of the called function
+ * (->fpu_dirtied set implies CR0.TS clear), but the additional
+ * overhead should be low enough to not warrant introduction of yet
+ * another slightly different function. However, we need to undo the
+ * ->fpu_dirtied clearing the function does as well as the possible
+ * masking of all exceptions by FNSTENV.)
+ */
+ save_fpu_enable();
+ curr->fpu_dirtied = true;
+ if ( (fpu_ctxt->fcw & 0x3f) != 0x3f )
+ {
+ uint16_t fcw;
+
+ asm ( "fnstcw %0" : "=m" (fcw) );
+ if ( (fcw & 0x3f) == 0x3f )
+ asm ( "fldcw %0" :: "m" (fpu_ctxt->fcw) );
+ else
+ ASSERT(fcw == fpu_ctxt->fcw);
+ }
+ }
curr->arch.hvm_vcpu.fpu_exception_callback = exception_callback;
curr->arch.hvm_vcpu.fpu_exception_callback_arg = exception_callback_arg;
@@ -1627,10 +1657,24 @@ static int hvmemul_get_fpu(
}
static void hvmemul_put_fpu(
- struct x86_emulate_ctxt *ctxt)
+ struct x86_emulate_ctxt *ctxt,
+ enum x86_emulate_fpu_type backout)
{
struct vcpu *curr = current;
+
curr->arch.hvm_vcpu.fpu_exception_callback = NULL;
+
+ if ( backout == X86EMUL_FPU_fpu )
+ {
+ /*
+ * To back out changes to the register file simply adjust state such
+ * that upon next FPU insn use by the guest we'll reload the state
+ * saved (or freshly loaded) by hvmemul_get_fpu().
+ */
+ curr->fpu_dirtied = false;
+ stts();
+ hvm_funcs.fpu_leave(curr);
+ }
}
static int hvmemul_invlpg(
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2268,6 +2268,7 @@ static struct hvm_function_table __initd
.update_guest_cr = svm_update_guest_cr,
.update_guest_efer = svm_update_guest_efer,
.update_guest_vendor = svm_update_guest_vendor,
+ .fpu_leave = svm_fpu_leave,
.set_guest_pat = svm_set_guest_pat,
.get_guest_pat = svm_get_guest_pat,
.set_tsc_offset = svm_set_tsc_offset,
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2128,6 +2128,7 @@ static struct hvm_function_table __initd
.update_guest_cr = vmx_update_guest_cr,
.update_guest_efer = vmx_update_guest_efer,
.update_guest_vendor = vmx_update_guest_vendor,
+ .fpu_leave = vmx_fpu_leave,
.set_guest_pat = vmx_set_guest_pat,
.get_guest_pat = vmx_get_guest_pat,
.set_tsc_offset = vmx_set_tsc_offset,
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -965,6 +965,7 @@ static int _get_fpu(
{
unsigned long cr0;
+ fail_if(type == X86EMUL_FPU_fpu && !ops->put_fpu);
fic->type = type;
fail_if(!ops->read_cr);
@@ -1026,11 +1027,14 @@ do {
static void put_fpu(
struct fpu_insn_ctxt *fic,
+ bool failed_late,
struct x86_emulate_ctxt *ctxt,
const struct x86_emulate_ops *ops)
{
- if ( fic->type != X86EMUL_FPU_none && ops->put_fpu )
- ops->put_fpu(ctxt);
+ if ( unlikely(failed_late) && fic->type == X86EMUL_FPU_fpu )
+ ops->put_fpu(ctxt, X86EMUL_FPU_fpu);
+ else if ( fic->type != X86EMUL_FPU_none && ops->put_fpu )
+ ops->put_fpu(ctxt, X86EMUL_FPU_none);
fic->type = X86EMUL_FPU_none;
}
@@ -3716,9 +3720,9 @@ x86_emulate(
break;
case 0x9b: /* wait/fwait */
- fic.insn_bytes = 1;
host_and_vcpu_must_have(fpu);
get_fpu(X86EMUL_FPU_wait, &fic);
+ fic.insn_bytes = 1;
asm volatile ( "fwait" ::: "memory" );
check_fpu_exn(&fic);
break;
@@ -7892,7 +7896,7 @@ x86_emulate(
}
complete_insn: /* Commit shadow register state. */
- put_fpu(&fic, ctxt, ops);
+ put_fpu(&fic, false, ctxt, ops);
/* Zero the upper 32 bits of %rip if not in 64-bit mode. */
if ( !mode_64bit() )
@@ -7916,7 +7920,7 @@ x86_emulate(
ctxt->regs->eflags &= ~X86_EFLAGS_RF;
done:
- put_fpu(&fic, ctxt, ops);
+ put_fpu(&fic, fic.insn_bytes > 0 && dst.type == OP_MEM, ctxt, ops);
put_stub(stub);
return rc;
#undef state
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -437,9 +437,12 @@ struct x86_emulate_ops
* put_fpu: Relinquish the FPU. Unhook from FPU/SIMD exception handlers.
* The handler, if installed, must be prepared to get called without
* the get_fpu one having got called before!
+ * @backout: Undo updates to the specified register file (can, besides
+ * X86EMUL_FPU_none, only be X86EMUL_FPU_fpu at present);
*/
void (*put_fpu)(
- struct x86_emulate_ctxt *ctxt);
+ struct x86_emulate_ctxt *ctxt,
+ enum x86_emulate_fpu_type backout);
/* invlpg: Invalidate paging structures which map addressed byte. */
int (*invlpg)(
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -140,6 +140,8 @@ struct hvm_function_table {
void (*update_guest_vendor)(struct vcpu *v);
+ void (*fpu_leave)(struct vcpu *v);
+
int (*get_guest_pat)(struct vcpu *v, u64 *);
int (*set_guest_pat)(struct vcpu *v, u64);
[-- Attachment #2: x86emul-FPU-backout.patch --]
[-- Type: text/plain, Size: 8901 bytes --]
x86emul: correct handling of FPU insns faulting on memory write
When an FPU instruction with a memory destination fails during the
memory write, it should not affect FPU register state. Due to the way
we emulate FPU (and SIMD) instructions, we can only guarantee this by
- backing out changes to the FPU register state in such a case or
- doing a descriptor read and/or page walk up front, perhaps with the
stubs accessing the actual memory location then.
The latter would require a significant change in how the emulator does
its guest memory accessing, so for now the former variant is being
chosen.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
---
v2: Re-base.
---
Note that the state save overhead (unless state hadn't been loaded at
all before, which should only be possible if a guest is fiddling with
the instruction stream under emulation) is taken for every FPU insn
hitting the emulator. We could reduce this to just the ones writing to
memory, but that would involve quite a few further changes and
resulting code where even more code paths need to match up with one
another.
--- a/tools/fuzz/x86_instruction_emulator/x86-insn-emulator-fuzzer.c
+++ b/tools/fuzz/x86_instruction_emulator/x86-insn-emulator-fuzzer.c
@@ -433,6 +433,7 @@ static struct x86_emulate_ops fuzz_emulo
SET(wbinvd),
SET(invlpg),
.get_fpu = emul_test_get_fpu,
+ .put_fpu = emul_test_put_fpu,
.cpuid = emul_test_cpuid,
};
#undef SET
--- a/tools/tests/x86_emulator/test_x86_emulator.c
+++ b/tools/tests/x86_emulator/test_x86_emulator.c
@@ -293,6 +293,7 @@ static struct x86_emulate_ops emulops =
.read_cr = emul_test_read_cr,
.read_msr = read_msr,
.get_fpu = emul_test_get_fpu,
+ .put_fpu = emul_test_put_fpu,
};
int main(int argc, char **argv)
--- a/tools/tests/x86_emulator/x86_emulate.c
+++ b/tools/tests/x86_emulator/x86_emulate.c
@@ -138,4 +138,11 @@ int emul_test_get_fpu(
return X86EMUL_OKAY;
}
+void emul_test_put_fpu(
+ struct x86_emulate_ctxt *ctxt,
+ enum x86_emulate_fpu_type backout)
+{
+ /* TBD */
+}
+
#include "x86_emulate/x86_emulate.c"
--- a/tools/tests/x86_emulator/x86_emulate.h
+++ b/tools/tests/x86_emulator/x86_emulate.h
@@ -178,3 +178,7 @@ int emul_test_get_fpu(
void *exception_callback_arg,
enum x86_emulate_fpu_type type,
struct x86_emulate_ctxt *ctxt);
+
+void emul_test_put_fpu(
+ struct x86_emulate_ctxt *ctxt,
+ enum x86_emulate_fpu_type backout);
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -15,6 +15,7 @@
#include <xen/paging.h>
#include <xen/trace.h>
#include <asm/event.h>
+#include <asm/i387.h>
#include <asm/xstate.h>
#include <asm/hvm/emulate.h>
#include <asm/hvm/hvm.h>
@@ -1619,6 +1620,35 @@ static int hvmemul_get_fpu(
if ( !curr->fpu_dirtied )
hvm_funcs.fpu_dirty_intercept();
+ else if ( type == X86EMUL_FPU_fpu )
+ {
+ const typeof(curr->arch.xsave_area->fpu_sse) *fpu_ctxt =
+ curr->arch.fpu_ctxt;
+
+ /*
+ * Latch current register state so that we can back out changes
+ * if needed (namely when a memory write fails after register state
+ * has already been updated).
+ * NB: We don't really need the "enable" part of the called function
+ * (->fpu_dirtied set implies CR0.TS clear), but the additional
+ * overhead should be low enough to not warrant introduction of yet
+ * another slightly different function. However, we need to undo the
+ * ->fpu_dirtied clearing the function does as well as the possible
+ * masking of all exceptions by FNSTENV.)
+ */
+ save_fpu_enable();
+ curr->fpu_dirtied = true;
+ if ( (fpu_ctxt->fcw & 0x3f) != 0x3f )
+ {
+ uint16_t fcw;
+
+ asm ( "fnstcw %0" : "=m" (fcw) );
+ if ( (fcw & 0x3f) == 0x3f )
+ asm ( "fldcw %0" :: "m" (fpu_ctxt->fcw) );
+ else
+ ASSERT(fcw == fpu_ctxt->fcw);
+ }
+ }
curr->arch.hvm_vcpu.fpu_exception_callback = exception_callback;
curr->arch.hvm_vcpu.fpu_exception_callback_arg = exception_callback_arg;
@@ -1627,10 +1657,24 @@ static int hvmemul_get_fpu(
}
static void hvmemul_put_fpu(
- struct x86_emulate_ctxt *ctxt)
+ struct x86_emulate_ctxt *ctxt,
+ enum x86_emulate_fpu_type backout)
{
struct vcpu *curr = current;
+
curr->arch.hvm_vcpu.fpu_exception_callback = NULL;
+
+ if ( backout == X86EMUL_FPU_fpu )
+ {
+ /*
+ * To back out changes to the register file simply adjust state such
+ * that upon next FPU insn use by the guest we'll reload the state
+ * saved (or freshly loaded) by hvmemul_get_fpu().
+ */
+ curr->fpu_dirtied = false;
+ stts();
+ hvm_funcs.fpu_leave(curr);
+ }
}
static int hvmemul_invlpg(
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2268,6 +2268,7 @@ static struct hvm_function_table __initd
.update_guest_cr = svm_update_guest_cr,
.update_guest_efer = svm_update_guest_efer,
.update_guest_vendor = svm_update_guest_vendor,
+ .fpu_leave = svm_fpu_leave,
.set_guest_pat = svm_set_guest_pat,
.get_guest_pat = svm_get_guest_pat,
.set_tsc_offset = svm_set_tsc_offset,
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2128,6 +2128,7 @@ static struct hvm_function_table __initd
.update_guest_cr = vmx_update_guest_cr,
.update_guest_efer = vmx_update_guest_efer,
.update_guest_vendor = vmx_update_guest_vendor,
+ .fpu_leave = vmx_fpu_leave,
.set_guest_pat = vmx_set_guest_pat,
.get_guest_pat = vmx_get_guest_pat,
.set_tsc_offset = vmx_set_tsc_offset,
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -965,6 +965,7 @@ static int _get_fpu(
{
unsigned long cr0;
+ fail_if(type == X86EMUL_FPU_fpu && !ops->put_fpu);
fic->type = type;
fail_if(!ops->read_cr);
@@ -1026,11 +1027,14 @@ do {
static void put_fpu(
struct fpu_insn_ctxt *fic,
+ bool failed_late,
struct x86_emulate_ctxt *ctxt,
const struct x86_emulate_ops *ops)
{
- if ( fic->type != X86EMUL_FPU_none && ops->put_fpu )
- ops->put_fpu(ctxt);
+ if ( unlikely(failed_late) && fic->type == X86EMUL_FPU_fpu )
+ ops->put_fpu(ctxt, X86EMUL_FPU_fpu);
+ else if ( fic->type != X86EMUL_FPU_none && ops->put_fpu )
+ ops->put_fpu(ctxt, X86EMUL_FPU_none);
fic->type = X86EMUL_FPU_none;
}
@@ -3716,9 +3720,9 @@ x86_emulate(
break;
case 0x9b: /* wait/fwait */
- fic.insn_bytes = 1;
host_and_vcpu_must_have(fpu);
get_fpu(X86EMUL_FPU_wait, &fic);
+ fic.insn_bytes = 1;
asm volatile ( "fwait" ::: "memory" );
check_fpu_exn(&fic);
break;
@@ -7892,7 +7896,7 @@ x86_emulate(
}
complete_insn: /* Commit shadow register state. */
- put_fpu(&fic, ctxt, ops);
+ put_fpu(&fic, false, ctxt, ops);
/* Zero the upper 32 bits of %rip if not in 64-bit mode. */
if ( !mode_64bit() )
@@ -7916,7 +7920,7 @@ x86_emulate(
ctxt->regs->eflags &= ~X86_EFLAGS_RF;
done:
- put_fpu(&fic, ctxt, ops);
+ put_fpu(&fic, fic.insn_bytes > 0 && dst.type == OP_MEM, ctxt, ops);
put_stub(stub);
return rc;
#undef state
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -437,9 +437,12 @@ struct x86_emulate_ops
* put_fpu: Relinquish the FPU. Unhook from FPU/SIMD exception handlers.
* The handler, if installed, must be prepared to get called without
* the get_fpu one having got called before!
+ * @backout: Undo updates to the specified register file (can, besides
+ * X86EMUL_FPU_none, only be X86EMUL_FPU_fpu at present);
*/
void (*put_fpu)(
- struct x86_emulate_ctxt *ctxt);
+ struct x86_emulate_ctxt *ctxt,
+ enum x86_emulate_fpu_type backout);
/* invlpg: Invalidate paging structures which map addressed byte. */
int (*invlpg)(
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -140,6 +140,8 @@ struct hvm_function_table {
void (*update_guest_vendor)(struct vcpu *v);
+ void (*fpu_leave)(struct vcpu *v);
+
int (*get_guest_pat)(struct vcpu *v, u64 *);
int (*set_guest_pat)(struct vcpu *v, u64);
[-- Attachment #3: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 3/3] x86emul: correct FPU code/data pointers and opcode handling
2017-03-15 10:23 [PATCH v2 0/3] x86emul: FPU handling corrections Jan Beulich
2017-03-15 10:27 ` [PATCH v2 1/3] x86emul: centralize put_fpu() invocations Jan Beulich
2017-03-15 10:28 ` [PATCH v2 2/3] x86emul: correct handling of FPU insns faulting on memory write Jan Beulich
@ 2017-03-15 10:28 ` Jan Beulich
2017-03-20 17:26 ` Andrew Cooper
2017-03-15 10:29 ` [PATCH v2][XTF] add FPU/SIMD register state test Jan Beulich
3 siblings, 1 reply; 12+ messages in thread
From: Jan Beulich @ 2017-03-15 10:28 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper
[-- Attachment #1: Type: text/plain, Size: 12332 bytes --]
Prevent leaking the hypervisor ones (stored by hardware during stub
execution), at once making sure the guest sees correct values there.
This piggybacks on the backout logic used to deal with write faults of
FPU insns.
Deliberately ignore the NO_FPU_SEL feature here: Honoring it would
merely mean extra code with no benefit (once we XRSTOR state, the
selector values will simply be lost anyway).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com> [hvm/emulate.c]
---
v2: Re-base.
--- a/tools/tests/x86_emulator/x86_emulate.c
+++ b/tools/tests/x86_emulator/x86_emulate.c
@@ -140,7 +140,8 @@ int emul_test_get_fpu(
void emul_test_put_fpu(
struct x86_emulate_ctxt *ctxt,
- enum x86_emulate_fpu_type backout)
+ enum x86_emulate_fpu_type backout,
+ const struct x86_emul_fpu_aux *aux)
{
/* TBD */
}
--- a/tools/tests/x86_emulator/x86_emulate.h
+++ b/tools/tests/x86_emulator/x86_emulate.h
@@ -181,4 +181,5 @@ int emul_test_get_fpu(
void emul_test_put_fpu(
struct x86_emulate_ctxt *ctxt,
- enum x86_emulate_fpu_type backout);
+ enum x86_emulate_fpu_type backout,
+ const struct x86_emul_fpu_aux *aux);
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1658,12 +1658,73 @@ static int hvmemul_get_fpu(
static void hvmemul_put_fpu(
struct x86_emulate_ctxt *ctxt,
- enum x86_emulate_fpu_type backout)
+ enum x86_emulate_fpu_type backout,
+ const struct x86_emul_fpu_aux *aux)
{
struct vcpu *curr = current;
curr->arch.hvm_vcpu.fpu_exception_callback = NULL;
+ if ( aux )
+ {
+ typeof(curr->arch.xsave_area->fpu_sse) *fpu_ctxt = curr->arch.fpu_ctxt;
+ bool dval = aux->dval;
+ int mode = hvm_guest_x86_mode(curr);
+
+ ASSERT(backout == X86EMUL_FPU_none);
+ /*
+ * Latch current register state so that we can replace FIP/FDP/FOP
+ * (which have values resulting from our own invocation of the FPU
+ * instruction during emulation).
+ * NB: See also the comment in hvmemul_get_fpu(); we don't need to
+ * set ->fpu_dirtied here as it is going to be cleared below, and
+ * we also don't need to reload FCW as we're forcing full state to
+ * be reloaded anyway.
+ */
+ save_fpu_enable();
+
+ if ( boot_cpu_has(X86_FEATURE_FDP_EXCP_ONLY) &&
+ !(fpu_ctxt->fsw & ~fpu_ctxt->fcw & 0x003f) )
+ dval = false;
+
+ switch ( mode )
+ {
+ case 8:
+ fpu_ctxt->fip.addr = aux->ip;
+ if ( dval )
+ fpu_ctxt->fdp.addr = aux->dp;
+ fpu_ctxt->x[FPU_WORD_SIZE_OFFSET] = 8;
+ break;
+
+ case 4: case 2:
+ fpu_ctxt->fip.offs = aux->ip;
+ fpu_ctxt->fip.sel = aux->cs;
+ if ( dval )
+ {
+ fpu_ctxt->fdp.offs = aux->dp;
+ fpu_ctxt->fdp.sel = aux->ds;
+ }
+ fpu_ctxt->x[FPU_WORD_SIZE_OFFSET] = mode;
+ break;
+
+ case 0: case 1:
+ fpu_ctxt->fip.addr = aux->ip | (aux->cs << 4);
+ if ( dval )
+ fpu_ctxt->fdp.addr = aux->dp | (aux->ds << 4);
+ fpu_ctxt->x[FPU_WORD_SIZE_OFFSET] = 2;
+ break;
+
+ default:
+ ASSERT_UNREACHABLE();
+ return;
+ }
+
+ fpu_ctxt->fop = aux->op;
+
+ /* Re-use backout code below. */
+ backout = X86EMUL_FPU_fpu;
+ }
+
if ( backout == X86EMUL_FPU_fpu )
{
/*
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -536,6 +536,55 @@ struct operand {
unsigned long off;
} mem;
};
+
+struct x86_emulate_state {
+ unsigned int op_bytes, ad_bytes;
+
+ enum {
+ ext_none = vex_none,
+ ext_0f = vex_0f,
+ ext_0f38 = vex_0f38,
+ ext_0f3a = vex_0f3a,
+ /*
+ * For XOP use values such that the respective instruction field
+ * can be used without adjustment.
+ */
+ ext_8f08 = 8,
+ ext_8f09,
+ ext_8f0a,
+ } ext;
+ uint8_t modrm, modrm_mod, modrm_reg, modrm_rm;
+ uint8_t rex_prefix;
+ bool lock_prefix;
+ bool not_64bit; /* Instruction not available in 64bit. */
+ bool fpu_ctrl; /* Instruction is an FPU control one. */
+ opcode_desc_t desc;
+ union vex vex;
+ union evex evex;
+ enum simd_opsize simd_size;
+
+ /*
+ * Data operand effective address (usually computed from ModRM).
+ * Default is a memory operand relative to segment DS.
+ */
+ struct operand ea;
+
+ /* Immediate operand values, if any. Use otherwise unused fields. */
+#define imm1 ea.val
+#define imm2 ea.orig_val
+
+ unsigned long ip;
+ struct cpu_user_regs *regs;
+
+#ifndef NDEBUG
+ /*
+ * Track caller of x86_decode_insn() to spot missing as well as
+ * premature calls to x86_emulate_free_state().
+ */
+ void *caller;
+#endif
+};
+
#ifdef __x86_64__
#define PTR_POISON ((void *)0x8086000000008086UL) /* non-canonical */
#else
@@ -1028,13 +1077,48 @@ do {
static void put_fpu(
struct fpu_insn_ctxt *fic,
bool failed_late,
+ const struct x86_emulate_state *state,
struct x86_emulate_ctxt *ctxt,
const struct x86_emulate_ops *ops)
{
if ( unlikely(failed_late) && fic->type == X86EMUL_FPU_fpu )
- ops->put_fpu(ctxt, X86EMUL_FPU_fpu);
+ ops->put_fpu(ctxt, X86EMUL_FPU_fpu, NULL);
+ else if ( unlikely(fic->type == X86EMUL_FPU_fpu) && !state->fpu_ctrl )
+ {
+ struct x86_emul_fpu_aux aux = {
+ .ip = ctxt->regs->r(ip),
+ .cs = ctxt->regs->cs,
+ .op = ((ctxt->opcode & 7) << 8) | state->modrm,
+ };
+ struct segment_register sreg;
+
+ if ( ops->read_segment &&
+ ops->read_segment(x86_seg_cs, &sreg, ctxt) == X86EMUL_OKAY )
+ aux.cs = sreg.sel;
+ if ( state->ea.type == OP_MEM )
+ {
+ aux.dp = state->ea.mem.off;
+ if ( ops->read_segment &&
+ ops->read_segment(state->ea.mem.seg, &sreg,
+ ctxt) == X86EMUL_OKAY )
+ aux.ds = sreg.sel;
+ else
+ switch ( state->ea.mem.seg )
+ {
+ case x86_seg_cs: aux.ds = ctxt->regs->cs; break;
+ case x86_seg_ds: aux.ds = ctxt->regs->ds; break;
+ case x86_seg_es: aux.ds = ctxt->regs->es; break;
+ case x86_seg_fs: aux.ds = ctxt->regs->fs; break;
+ case x86_seg_gs: aux.ds = ctxt->regs->gs; break;
+ case x86_seg_ss: aux.ds = ctxt->regs->ss; break;
+ default: ASSERT_UNREACHABLE(); break;
+ }
+ aux.dval = true;
+ }
+ ops->put_fpu(ctxt, X86EMUL_FPU_none, &aux);
+ }
else if ( fic->type != X86EMUL_FPU_none && ops->put_fpu )
- ops->put_fpu(ctxt, X86EMUL_FPU_none);
+ ops->put_fpu(ctxt, X86EMUL_FPU_none, NULL);
fic->type = X86EMUL_FPU_none;
}
@@ -2088,53 +2172,6 @@ int x86emul_unhandleable_rw(
return X86EMUL_UNHANDLEABLE;
}
-struct x86_emulate_state {
- unsigned int op_bytes, ad_bytes;
-
- enum {
- ext_none = vex_none,
- ext_0f = vex_0f,
- ext_0f38 = vex_0f38,
- ext_0f3a = vex_0f3a,
- /*
- * For XOP use values such that the respective instruction field
- * can be used without adjustment.
- */
- ext_8f08 = 8,
- ext_8f09,
- ext_8f0a,
- } ext;
- uint8_t modrm, modrm_mod, modrm_reg, modrm_rm;
- uint8_t rex_prefix;
- bool lock_prefix;
- bool not_64bit; /* Instruction not available in 64bit. */
- opcode_desc_t desc;
- union vex vex;
- union evex evex;
- enum simd_opsize simd_size;
-
- /*
- * Data operand effective address (usually computed from ModRM).
- * Default is a memory operand relative to segment DS.
- */
- struct operand ea;
-
- /* Immediate operand values, if any. Use otherwise unused fields. */
-#define imm1 ea.val
-#define imm2 ea.orig_val
-
- unsigned long ip;
- struct cpu_user_regs *regs;
-
-#ifndef NDEBUG
- /*
- * Track caller of x86_decode_insn() to spot missing as well as
- * premature calls to x86_emulate_free_state().
- */
- void *caller;
-#endif
-};
-
/* Helper definitions. */
#define op_bytes (state->op_bytes)
#define ad_bytes (state->ad_bytes)
@@ -4233,8 +4270,10 @@ x86_emulate(
dst.bytes = 4;
break;
case 4: /* fldenv - TODO */
+ state->fpu_ctrl = true;
goto cannot_emulate;
case 5: /* fldcw m2byte */
+ state->fpu_ctrl = true;
if ( (rc = ops->read(ea.mem.seg, ea.mem.off, &src.val,
2, ctxt)) != X86EMUL_OKAY )
goto done;
@@ -4242,8 +4281,10 @@ x86_emulate(
dst.type = OP_NONE;
break;
case 6: /* fnstenv - TODO */
+ state->fpu_ctrl = true;
goto cannot_emulate;
case 7: /* fnstcw m2byte */
+ state->fpu_ctrl = true;
emulate_fpu_insn_memdst("fnstcw", dst.val);
dst.bytes = 2;
break;
@@ -4332,6 +4373,7 @@ x86_emulate(
case 0xe3: /* fninit */
case 0xe4: /* fnsetpm - 287 only, ignored by 387 */
/* case 0xe5: frstpm - 287 only, #UD on 387 */
+ state->fpu_ctrl = true;
emulate_fpu_insn_stub(0xdb, modrm);
break;
default:
@@ -4475,8 +4517,10 @@ x86_emulate(
break;
case 4: /* frstor - TODO */
case 6: /* fnsave - TODO */
+ state->fpu_ctrl = true;
goto cannot_emulate;
case 7: /* fnstsw m2byte */
+ state->fpu_ctrl = true;
emulate_fpu_insn_memdst("fnstsw", dst.val);
dst.bytes = 2;
break;
@@ -4549,6 +4593,7 @@ x86_emulate(
{
case 0xe0:
/* fnstsw %ax */
+ state->fpu_ctrl = true;
dst.bytes = 2;
dst.type = OP_REG;
dst.reg = (void *)&_regs.ax;
@@ -7896,7 +7941,7 @@ x86_emulate(
}
complete_insn: /* Commit shadow register state. */
- put_fpu(&fic, false, ctxt, ops);
+ put_fpu(&fic, false, state, ctxt, ops);
/* Zero the upper 32 bits of %rip if not in 64-bit mode. */
if ( !mode_64bit() )
@@ -7920,7 +7965,7 @@ x86_emulate(
ctxt->regs->eflags &= ~X86_EFLAGS_RF;
done:
- put_fpu(&fic, fic.insn_bytes > 0 && dst.type == OP_MEM, ctxt, ops);
+ put_fpu(&fic, fic.insn_bytes > 0 && dst.type == OP_MEM, state, ctxt, ops);
put_stub(stub);
return rc;
#undef state
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -135,6 +135,13 @@ struct __attribute__((__packed__)) segme
uint64_t base;
};
+struct x86_emul_fpu_aux {
+ unsigned long ip, dp;
+ uint16_t cs, ds;
+ unsigned int op:11;
+ unsigned int dval:1;
+};
+
/*
* Return codes from state-accessor functions and from x86_emulate().
*/
@@ -439,10 +446,12 @@ struct x86_emulate_ops
* the get_fpu one having got called before!
* @backout: Undo updates to the specified register file (can, besides
* X86EMUL_FPU_none, only be X86EMUL_FPU_fpu at present);
+ * @aux: Packaged up FIP/FDP/FOP values to load into FPU.
*/
void (*put_fpu)(
struct x86_emulate_ctxt *ctxt,
- enum x86_emulate_fpu_type backout);
+ enum x86_emulate_fpu_type backout,
+ const struct x86_emul_fpu_aux *aux);
/* invlpg: Invalidate paging structures which map addressed byte. */
int (*invlpg)(
[-- Attachment #2: x86emul-FPU-FxP.patch --]
[-- Type: text/plain, Size: 12391 bytes --]
x86emul: correct FPU code/data pointers and opcode handling
Prevent leaking the hypervisor ones (stored by hardware during stub
execution), at once making sure the guest sees correct values there.
This piggybacks on the backout logic used to deal with write faults of
FPU insns.
Deliberately ignore the NO_FPU_SEL feature here: Honoring it would
merely mean extra code with no benefit (once we XRSTOR state, the
selector values will simply be lost anyway).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com> [hvm/emulate.c]
---
v2: Re-base.
--- a/tools/tests/x86_emulator/x86_emulate.c
+++ b/tools/tests/x86_emulator/x86_emulate.c
@@ -140,7 +140,8 @@ int emul_test_get_fpu(
void emul_test_put_fpu(
struct x86_emulate_ctxt *ctxt,
- enum x86_emulate_fpu_type backout)
+ enum x86_emulate_fpu_type backout,
+ const struct x86_emul_fpu_aux *aux)
{
/* TBD */
}
--- a/tools/tests/x86_emulator/x86_emulate.h
+++ b/tools/tests/x86_emulator/x86_emulate.h
@@ -181,4 +181,5 @@ int emul_test_get_fpu(
void emul_test_put_fpu(
struct x86_emulate_ctxt *ctxt,
- enum x86_emulate_fpu_type backout);
+ enum x86_emulate_fpu_type backout,
+ const struct x86_emul_fpu_aux *aux);
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1658,12 +1658,73 @@ static int hvmemul_get_fpu(
static void hvmemul_put_fpu(
struct x86_emulate_ctxt *ctxt,
- enum x86_emulate_fpu_type backout)
+ enum x86_emulate_fpu_type backout,
+ const struct x86_emul_fpu_aux *aux)
{
struct vcpu *curr = current;
curr->arch.hvm_vcpu.fpu_exception_callback = NULL;
+ if ( aux )
+ {
+ typeof(curr->arch.xsave_area->fpu_sse) *fpu_ctxt = curr->arch.fpu_ctxt;
+ bool dval = aux->dval;
+ int mode = hvm_guest_x86_mode(curr);
+
+ ASSERT(backout == X86EMUL_FPU_none);
+ /*
+ * Latch current register state so that we can replace FIP/FDP/FOP
+ * (which have values resulting from our own invocation of the FPU
+ * instruction during emulation).
+ * NB: See also the comment in hvmemul_get_fpu(); we don't need to
+ * set ->fpu_dirtied here as it is going to be cleared below, and
+ * we also don't need to reload FCW as we're forcing full state to
+ * be reloaded anyway.
+ */
+ save_fpu_enable();
+
+ if ( boot_cpu_has(X86_FEATURE_FDP_EXCP_ONLY) &&
+ !(fpu_ctxt->fsw & ~fpu_ctxt->fcw & 0x003f) )
+ dval = false;
+
+ switch ( mode )
+ {
+ case 8:
+ fpu_ctxt->fip.addr = aux->ip;
+ if ( dval )
+ fpu_ctxt->fdp.addr = aux->dp;
+ fpu_ctxt->x[FPU_WORD_SIZE_OFFSET] = 8;
+ break;
+
+ case 4: case 2:
+ fpu_ctxt->fip.offs = aux->ip;
+ fpu_ctxt->fip.sel = aux->cs;
+ if ( dval )
+ {
+ fpu_ctxt->fdp.offs = aux->dp;
+ fpu_ctxt->fdp.sel = aux->ds;
+ }
+ fpu_ctxt->x[FPU_WORD_SIZE_OFFSET] = mode;
+ break;
+
+ case 0: case 1:
+ fpu_ctxt->fip.addr = aux->ip | (aux->cs << 4);
+ if ( dval )
+ fpu_ctxt->fdp.addr = aux->dp | (aux->ds << 4);
+ fpu_ctxt->x[FPU_WORD_SIZE_OFFSET] = 2;
+ break;
+
+ default:
+ ASSERT_UNREACHABLE();
+ return;
+ }
+
+ fpu_ctxt->fop = aux->op;
+
+ /* Re-use backout code below. */
+ backout = X86EMUL_FPU_fpu;
+ }
+
if ( backout == X86EMUL_FPU_fpu )
{
/*
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -536,6 +536,55 @@ struct operand {
unsigned long off;
} mem;
};
+
+struct x86_emulate_state {
+ unsigned int op_bytes, ad_bytes;
+
+ enum {
+ ext_none = vex_none,
+ ext_0f = vex_0f,
+ ext_0f38 = vex_0f38,
+ ext_0f3a = vex_0f3a,
+ /*
+ * For XOP use values such that the respective instruction field
+ * can be used without adjustment.
+ */
+ ext_8f08 = 8,
+ ext_8f09,
+ ext_8f0a,
+ } ext;
+ uint8_t modrm, modrm_mod, modrm_reg, modrm_rm;
+ uint8_t rex_prefix;
+ bool lock_prefix;
+ bool not_64bit; /* Instruction not available in 64bit. */
+ bool fpu_ctrl; /* Instruction is an FPU control one. */
+ opcode_desc_t desc;
+ union vex vex;
+ union evex evex;
+ enum simd_opsize simd_size;
+
+ /*
+ * Data operand effective address (usually computed from ModRM).
+ * Default is a memory operand relative to segment DS.
+ */
+ struct operand ea;
+
+ /* Immediate operand values, if any. Use otherwise unused fields. */
+#define imm1 ea.val
+#define imm2 ea.orig_val
+
+ unsigned long ip;
+ struct cpu_user_regs *regs;
+
+#ifndef NDEBUG
+ /*
+ * Track caller of x86_decode_insn() to spot missing as well as
+ * premature calls to x86_emulate_free_state().
+ */
+ void *caller;
+#endif
+};
+
#ifdef __x86_64__
#define PTR_POISON ((void *)0x8086000000008086UL) /* non-canonical */
#else
@@ -1028,13 +1077,48 @@ do {
static void put_fpu(
struct fpu_insn_ctxt *fic,
bool failed_late,
+ const struct x86_emulate_state *state,
struct x86_emulate_ctxt *ctxt,
const struct x86_emulate_ops *ops)
{
if ( unlikely(failed_late) && fic->type == X86EMUL_FPU_fpu )
- ops->put_fpu(ctxt, X86EMUL_FPU_fpu);
+ ops->put_fpu(ctxt, X86EMUL_FPU_fpu, NULL);
+ else if ( unlikely(fic->type == X86EMUL_FPU_fpu) && !state->fpu_ctrl )
+ {
+ struct x86_emul_fpu_aux aux = {
+ .ip = ctxt->regs->r(ip),
+ .cs = ctxt->regs->cs,
+ .op = ((ctxt->opcode & 7) << 8) | state->modrm,
+ };
+ struct segment_register sreg;
+
+ if ( ops->read_segment &&
+ ops->read_segment(x86_seg_cs, &sreg, ctxt) == X86EMUL_OKAY )
+ aux.cs = sreg.sel;
+ if ( state->ea.type == OP_MEM )
+ {
+ aux.dp = state->ea.mem.off;
+ if ( ops->read_segment &&
+ ops->read_segment(state->ea.mem.seg, &sreg,
+ ctxt) == X86EMUL_OKAY )
+ aux.ds = sreg.sel;
+ else
+ switch ( state->ea.mem.seg )
+ {
+ case x86_seg_cs: aux.ds = ctxt->regs->cs; break;
+ case x86_seg_ds: aux.ds = ctxt->regs->ds; break;
+ case x86_seg_es: aux.ds = ctxt->regs->es; break;
+ case x86_seg_fs: aux.ds = ctxt->regs->fs; break;
+ case x86_seg_gs: aux.ds = ctxt->regs->gs; break;
+ case x86_seg_ss: aux.ds = ctxt->regs->ss; break;
+ default: ASSERT_UNREACHABLE(); break;
+ }
+ aux.dval = true;
+ }
+ ops->put_fpu(ctxt, X86EMUL_FPU_none, &aux);
+ }
else if ( fic->type != X86EMUL_FPU_none && ops->put_fpu )
- ops->put_fpu(ctxt, X86EMUL_FPU_none);
+ ops->put_fpu(ctxt, X86EMUL_FPU_none, NULL);
fic->type = X86EMUL_FPU_none;
}
@@ -2088,53 +2172,6 @@ int x86emul_unhandleable_rw(
return X86EMUL_UNHANDLEABLE;
}
-struct x86_emulate_state {
- unsigned int op_bytes, ad_bytes;
-
- enum {
- ext_none = vex_none,
- ext_0f = vex_0f,
- ext_0f38 = vex_0f38,
- ext_0f3a = vex_0f3a,
- /*
- * For XOP use values such that the respective instruction field
- * can be used without adjustment.
- */
- ext_8f08 = 8,
- ext_8f09,
- ext_8f0a,
- } ext;
- uint8_t modrm, modrm_mod, modrm_reg, modrm_rm;
- uint8_t rex_prefix;
- bool lock_prefix;
- bool not_64bit; /* Instruction not available in 64bit. */
- opcode_desc_t desc;
- union vex vex;
- union evex evex;
- enum simd_opsize simd_size;
-
- /*
- * Data operand effective address (usually computed from ModRM).
- * Default is a memory operand relative to segment DS.
- */
- struct operand ea;
-
- /* Immediate operand values, if any. Use otherwise unused fields. */
-#define imm1 ea.val
-#define imm2 ea.orig_val
-
- unsigned long ip;
- struct cpu_user_regs *regs;
-
-#ifndef NDEBUG
- /*
- * Track caller of x86_decode_insn() to spot missing as well as
- * premature calls to x86_emulate_free_state().
- */
- void *caller;
-#endif
-};
-
/* Helper definitions. */
#define op_bytes (state->op_bytes)
#define ad_bytes (state->ad_bytes)
@@ -4233,8 +4270,10 @@ x86_emulate(
dst.bytes = 4;
break;
case 4: /* fldenv - TODO */
+ state->fpu_ctrl = true;
goto cannot_emulate;
case 5: /* fldcw m2byte */
+ state->fpu_ctrl = true;
if ( (rc = ops->read(ea.mem.seg, ea.mem.off, &src.val,
2, ctxt)) != X86EMUL_OKAY )
goto done;
@@ -4242,8 +4281,10 @@ x86_emulate(
dst.type = OP_NONE;
break;
case 6: /* fnstenv - TODO */
+ state->fpu_ctrl = true;
goto cannot_emulate;
case 7: /* fnstcw m2byte */
+ state->fpu_ctrl = true;
emulate_fpu_insn_memdst("fnstcw", dst.val);
dst.bytes = 2;
break;
@@ -4332,6 +4373,7 @@ x86_emulate(
case 0xe3: /* fninit */
case 0xe4: /* fnsetpm - 287 only, ignored by 387 */
/* case 0xe5: frstpm - 287 only, #UD on 387 */
+ state->fpu_ctrl = true;
emulate_fpu_insn_stub(0xdb, modrm);
break;
default:
@@ -4475,8 +4517,10 @@ x86_emulate(
break;
case 4: /* frstor - TODO */
case 6: /* fnsave - TODO */
+ state->fpu_ctrl = true;
goto cannot_emulate;
case 7: /* fnstsw m2byte */
+ state->fpu_ctrl = true;
emulate_fpu_insn_memdst("fnstsw", dst.val);
dst.bytes = 2;
break;
@@ -4549,6 +4593,7 @@ x86_emulate(
{
case 0xe0:
/* fnstsw %ax */
+ state->fpu_ctrl = true;
dst.bytes = 2;
dst.type = OP_REG;
dst.reg = (void *)&_regs.ax;
@@ -7896,7 +7941,7 @@ x86_emulate(
}
complete_insn: /* Commit shadow register state. */
- put_fpu(&fic, false, ctxt, ops);
+ put_fpu(&fic, false, state, ctxt, ops);
/* Zero the upper 32 bits of %rip if not in 64-bit mode. */
if ( !mode_64bit() )
@@ -7920,7 +7965,7 @@ x86_emulate(
ctxt->regs->eflags &= ~X86_EFLAGS_RF;
done:
- put_fpu(&fic, fic.insn_bytes > 0 && dst.type == OP_MEM, ctxt, ops);
+ put_fpu(&fic, fic.insn_bytes > 0 && dst.type == OP_MEM, state, ctxt, ops);
put_stub(stub);
return rc;
#undef state
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -135,6 +135,13 @@ struct __attribute__((__packed__)) segme
uint64_t base;
};
+struct x86_emul_fpu_aux {
+ unsigned long ip, dp;
+ uint16_t cs, ds;
+ unsigned int op:11;
+ unsigned int dval:1;
+};
+
/*
* Return codes from state-accessor functions and from x86_emulate().
*/
@@ -439,10 +446,12 @@ struct x86_emulate_ops
* the get_fpu one having got called before!
* @backout: Undo updates to the specified register file (can, besides
* X86EMUL_FPU_none, only be X86EMUL_FPU_fpu at present);
+ * @aux: Packaged up FIP/FDP/FOP values to load into FPU.
*/
void (*put_fpu)(
struct x86_emulate_ctxt *ctxt,
- enum x86_emulate_fpu_type backout);
+ enum x86_emulate_fpu_type backout,
+ const struct x86_emul_fpu_aux *aux);
/* invlpg: Invalidate paging structures which map addressed byte. */
int (*invlpg)(
[-- Attachment #3: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2][XTF] add FPU/SIMD register state test
2017-03-15 10:23 [PATCH v2 0/3] x86emul: FPU handling corrections Jan Beulich
` (2 preceding siblings ...)
2017-03-15 10:28 ` [PATCH v2 3/3] x86emul: correct FPU code/data pointers and opcode handling Jan Beulich
@ 2017-03-15 10:29 ` Jan Beulich
3 siblings, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2017-03-15 10:29 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel
[-- Attachment #1: Type: text/plain, Size: 9444 bytes --]
Add tests to verify that
- FPU insns leave correct (guest) values in FIP/FDP/FOP/FCS/FDS (at the
example for FSTPS),
- FPU insns writing memory don't update FPU register state when the
write faults (at the example of FISTPS),
- VCVTPS2PH (once implemented in the emulator) doesn't update MXCSR
if its write faults (VCVTPS2PH is one of the very few SIMD insns
writing to memory _and_ updating register state; the scatter family
of insns also fall into this category, but we're quite far yet from
supporting AVX-512 insns).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Introduce and use x87.h. Tolerate VCVTPS2PH misbehavior on Intel
hardware. Tolerate AMD oddities in probe_fstp() and probe_fistp().
--- a/include/arch/x86/cpuid.h
+++ b/include/arch/x86/cpuid.h
@@ -77,6 +77,7 @@ static inline bool cpu_has(unsigned int
#define cpu_has_pcid cpu_has(X86_FEATURE_PCID)
#define cpu_has_xsave cpu_has(X86_FEATURE_XSAVE)
#define cpu_has_avx cpu_has(X86_FEATURE_AVX)
+#define cpu_has_f16c cpu_has(X86_FEATURE_F16C)
#define cpu_has_syscall cpu_has(X86_FEATURE_SYSCALL)
#define cpu_has_nx cpu_has(X86_FEATURE_NX)
--- /dev/null
+++ b/include/arch/x86/x87.h
@@ -0,0 +1,27 @@
+#ifndef XTF_X86_X87_H
+#define XTF_X86_X87_H
+
+#include <xtf/types.h>
+
+struct x87_env_pm32 {
+ uint16_t cw, :16;
+ uint16_t sw, :16;
+ uint16_t tw, :16;
+ uint32_t ip;
+ uint16_t cs;
+ uint16_t op:11, :5;
+ uint32_t dp;
+ uint16_t ds, :16;
+};
+
+#endif /* XTF_X86_X87_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--- a/include/xen/arch-x86/xen.h
+++ b/include/xen/arch-x86/xen.h
@@ -15,6 +15,16 @@
#include "cpuid.h"
+/*
+ * A number of GDT entries are reserved by Xen. These are not situated at the
+ * start of the GDT because some stupid OSes export hard-coded selector values
+ * in their ABI. These hard-coded values are always near the start of the GDT,
+ * so Xen places itself out of the way, at the far end of the GDT.
+ *
+ * NB The LDT is set using the MMUEXT_SET_LDT op of HYPERVISOR_mmuext_op
+ */
+#define FIRST_RESERVED_GDT_PAGE 14
+
#ifndef __ASSEMBLY__
typedef unsigned long xen_pfn_t;
--- /dev/null
+++ b/tests/fpu-state/Makefile
@@ -0,0 +1,9 @@
+include $(ROOT)/build/common.mk
+
+NAME := fpu-state
+CATEGORY := functional
+TEST-ENVS := hvm64 hvm32pse
+
+obj-perenv += main.o
+
+include $(ROOT)/build/gen.mk
--- /dev/null
+++ b/tests/fpu-state/main.c
@@ -0,0 +1,212 @@
+/**
+ * @file tests/fpu-state/main.c
+ * @ref test-fpu-state - Emulation of FPU state
+ *
+ * @page test-fpu-state FPU State Emulation
+ *
+ * FPU code/data pointers and opcode must not be the ones resulting
+ * from the stub execution in the hypervisor.
+ *
+ * FPU and SIMD instructions faulting during memory write must not
+ * update the respective register files.
+ *
+ * @see tests/fpu-state/main.c
+ */
+#include <xtf.h>
+
+#include <arch/x86/exinfo.h>
+#include <arch/x86/x87.h>
+
+const char test_title[] = "FPU State";
+
+void probe_fstp(bool force)
+{
+ const uint8_t *fstp_offs;
+ uint32_t flt;
+ struct x87_env_pm32 fenv;
+
+ fenv.cw = 0x35f; /* unmask PE */
+ asm volatile ( "fninit;"
+ "fldcw %[cw];"
+ "fldpi;"
+ "mov $1f, %[offs];"
+ "test %[fep], %[fep];"
+ "jz 1f;"
+ _ASM_XEN_FEP
+ "1: fstps %[data]; 2:"
+ : [offs] "=&g" (fstp_offs), [data] "=m" (flt)
+ : [cw] "m" (fenv.cw), [fep] "q" (force) );
+
+ asm ( "fnstenv %0" : "=m" (fenv) );
+ if ( fenv.ip != (unsigned long)fstp_offs )
+ xtf_failure("Fail: FIP wrong (%08x)\n", fenv.ip);
+ if ( fenv.cs && fenv.cs != __KERN_CS )
+ {
+#ifdef __x86_64__
+ /*
+ * Tolerate CS being in the hypervisor reserved selector range on
+ * AMD hardware, as their 64-bit {F,}XRSTOR do not appear to clear
+ * FCS/FDS.
+ */
+ if ( vendor_is_amd && !(fenv.cs & X86_SEL_LDT) &&
+ (fenv.cs >> PAGE_SHIFT) == FIRST_RESERVED_GDT_PAGE )
+ xtf_warning("Warning: FCS wrong (%04x)\n", fenv.cs);
+ else
+#endif
+ xtf_failure("Fail: FCS wrong (%04x)\n", fenv.cs);
+ }
+ if ( fenv.dp != (unsigned long)&flt )
+ xtf_failure("Fail: FDP wrong (%08x)\n", fenv.dp);
+ if ( fenv.ds && fenv.ds != __KERN_DS )
+ xtf_failure("Fail: FDS wrong (%04x)\n", fenv.ds);
+ /* Skip possible opcode prefixes before checking the opcode. */
+ while ( (fstp_offs[0] & ~7) != 0xd8 )
+ ++fstp_offs;
+ if ( fenv.op && fenv.op != (((fstp_offs[0] & 7) << 8) | fstp_offs[1]) )
+ xtf_failure("Fail: FOP wrong (%03x)\n", fenv.op);
+}
+
+void probe_fistp(bool force)
+{
+ unsigned long fldpi_offs;
+ exinfo_t fault = 0;
+ uint16_t fsw;
+ struct x87_env_pm32 fenv;
+ typeof(xtf_failure) *diagfn;
+ const char *prefix;
+
+ asm volatile ( "fninit;"
+ "0: fldpi;"
+ "mov $0b, %[offs];"
+ "test %[fep], %[fep];"
+ "jz 1f;"
+ _ASM_XEN_FEP
+ "1: fistps (%[ptr]); 2:"
+ _ASM_EXTABLE_HANDLER(1b, 2b, ex_record_fault_eax)
+ : [offs] "=&g" (fldpi_offs), "+a" (fault)
+ : [ptr] "r" (0), [fep] "q" (force) );
+
+ if ( !fault )
+ xtf_error("Error: FISTP to NULL did not fault\n");
+
+ asm ( "fnstsw %0" : "=am" (fsw) );
+ if ( fsw != 0x3800 )
+ xtf_failure("Fail: FSW changed unexpectedly (%04x)\n", fsw);
+
+ asm ( "fnstenv %0" : "=m" (fenv) );
+ /*
+ * The AMD-specific FPU pointer leak workaround in Xen (using FISTPL,
+ * which we check for below) causes all the remaining checks to fail.
+ */
+ if ( !vendor_is_amd || (fenv.op & 0x738) != 0x300 )
+ {
+ diagfn = xtf_failure;
+ prefix = "Fail";
+ }
+ else
+ {
+ diagfn = xtf_warning;
+ prefix = "Warning";
+ }
+ if ( fenv.ip != fldpi_offs )
+ diagfn("%s: FIP changed unexpectedly (%08x)\n", prefix, fenv.ip);
+ if ( fenv.cs && fenv.cs != __KERN_CS )
+ diagfn("%s: FCS changed unexpectedly (%04x)\n", prefix, fenv.cs);
+ if ( fenv.dp )
+ diagfn("%s: FDP changed unexpectedly (%08x)\n", prefix, fenv.dp);
+ if ( fenv.ds )
+ diagfn("%s: FDS changed unexpectedly (%04x)\n", prefix, fenv.ds);
+ if ( fenv.op && fenv.op != 0x1eb )
+ diagfn("%s: FOP changed unexpectedly (%03x)\n", prefix, fenv.op);
+}
+
+void probe_vcvtps2ph(bool force)
+{
+ exinfo_t fault = 0;
+ uint32_t mxcsr = 0x1f80;
+
+ asm volatile ( "vldmxcsr %[mxcsr];"
+ "vpcmpeqb %%xmm0,%%xmm0,%%xmm0;"
+ "vpcmpgtb %%xmm0,%%xmm0,%%xmm1;"
+ "vpunpcklbw %%xmm1,%%xmm0,%%xmm2;"
+ "test %[fep], %[fep];"
+ "jz 1f;"
+ _ASM_XEN_FEP
+ "1: vcvtps2ph $0,%%xmm2,(%[ptr]); 2:"
+ _ASM_EXTABLE_HANDLER(1b, 2b, ex_record_fault_eax)
+ : "+a" (fault)
+ : [ptr] "r" (0), [mxcsr] "m" (mxcsr), [fep] "q" (force) );
+
+ if ( !fault )
+ xtf_error("Error: VCVTPS2PH to NULL did not fault\n");
+ else if ( exinfo_vec(fault) == X86_EXC_UD )
+ {
+ if ( force )
+ xtf_skip("Emulator does not support VCVTPS2PH\n");
+ else
+ xtf_failure("Fail: VCVTPS2PH did #UD\n");
+ }
+
+ asm ( "vstmxcsr %0" : "=m" (mxcsr) );
+ if ( mxcsr != 0x1f80 )
+ {
+ /*
+ * Expect AMD hardware and emulation to behave correctly, but tolerate
+ * unexpected behavior on Intel hardware.
+ */
+ if ( force || vendor_is_amd )
+ xtf_failure("Fail: MXCSR changed unexpectedly (%08x)\n", mxcsr);
+ else
+ xtf_warning("Warning: MXCSR changed unexpectedly (%08x)\n", mxcsr);
+ }
+}
+
+void run_tests(bool force)
+{
+ if ( cpu_has_fpu )
+ {
+ printk("Testing%s FSTP\n", force ? " emulated" : "");
+ probe_fstp(force);
+
+ printk("Testing%s FISTP (to NULL)\n", force ? " emulated" : "");
+ probe_fistp(force);
+ }
+
+ if ( cpu_has_f16c )
+ {
+ unsigned long cr4 = read_cr4();
+ unsigned long xcr0;
+
+ write_cr4(cr4 | X86_CR4_OSXSAVE);
+ xcr0 = read_xcr0();
+ write_xcr0(xcr0 | XSTATE_SSE | XSTATE_YMM);
+
+ printk("Testing%s VCVTPS2PH (to NULL)\n", force ? " emulated" : "");
+ probe_vcvtps2ph(force);
+
+ write_xcr0(xcr0);
+ write_cr4(cr4);
+ }
+}
+
+void test_main(void)
+{
+ run_tests(false);
+
+ if ( !xtf_has_fep )
+ xtf_skip("FEP support not detected - some tests will be skipped\n");
+ else
+ run_tests(true);
+
+ xtf_success(NULL);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
[-- Attachment #2: xtf-FPU-state.patch --]
[-- Type: text/plain, Size: 9476 bytes --]
add FPU/SIMD register state test
Add tests to verify that
- FPU insns leave correct (guest) values in FIP/FDP/FOP/FCS/FDS (at the
example for FSTPS),
- FPU insns writing memory don't update FPU register state when the
write faults (at the example of FISTPS),
- VCVTPS2PH (once implemented in the emulator) doesn't update MXCSR
if its write faults (VCVTPS2PH is one of the very few SIMD insns
writing to memory _and_ updating register state; the scatter family
of insns also fall into this category, but we're quite far yet from
supporting AVX-512 insns).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Introduce and use x87.h. Tolerate VCVTPS2PH misbehavior on Intel
hardware. Tolerate AMD oddities in probe_fstp() and probe_fistp().
--- a/include/arch/x86/cpuid.h
+++ b/include/arch/x86/cpuid.h
@@ -77,6 +77,7 @@ static inline bool cpu_has(unsigned int
#define cpu_has_pcid cpu_has(X86_FEATURE_PCID)
#define cpu_has_xsave cpu_has(X86_FEATURE_XSAVE)
#define cpu_has_avx cpu_has(X86_FEATURE_AVX)
+#define cpu_has_f16c cpu_has(X86_FEATURE_F16C)
#define cpu_has_syscall cpu_has(X86_FEATURE_SYSCALL)
#define cpu_has_nx cpu_has(X86_FEATURE_NX)
--- /dev/null
+++ b/include/arch/x86/x87.h
@@ -0,0 +1,27 @@
+#ifndef XTF_X86_X87_H
+#define XTF_X86_X87_H
+
+#include <xtf/types.h>
+
+struct x87_env_pm32 {
+ uint16_t cw, :16;
+ uint16_t sw, :16;
+ uint16_t tw, :16;
+ uint32_t ip;
+ uint16_t cs;
+ uint16_t op:11, :5;
+ uint32_t dp;
+ uint16_t ds, :16;
+};
+
+#endif /* XTF_X86_X87_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--- a/include/xen/arch-x86/xen.h
+++ b/include/xen/arch-x86/xen.h
@@ -15,6 +15,16 @@
#include "cpuid.h"
+/*
+ * A number of GDT entries are reserved by Xen. These are not situated at the
+ * start of the GDT because some stupid OSes export hard-coded selector values
+ * in their ABI. These hard-coded values are always near the start of the GDT,
+ * so Xen places itself out of the way, at the far end of the GDT.
+ *
+ * NB The LDT is set using the MMUEXT_SET_LDT op of HYPERVISOR_mmuext_op
+ */
+#define FIRST_RESERVED_GDT_PAGE 14
+
#ifndef __ASSEMBLY__
typedef unsigned long xen_pfn_t;
--- /dev/null
+++ b/tests/fpu-state/Makefile
@@ -0,0 +1,9 @@
+include $(ROOT)/build/common.mk
+
+NAME := fpu-state
+CATEGORY := functional
+TEST-ENVS := hvm64 hvm32pse
+
+obj-perenv += main.o
+
+include $(ROOT)/build/gen.mk
--- /dev/null
+++ b/tests/fpu-state/main.c
@@ -0,0 +1,212 @@
+/**
+ * @file tests/fpu-state/main.c
+ * @ref test-fpu-state - Emulation of FPU state
+ *
+ * @page test-fpu-state FPU State Emulation
+ *
+ * FPU code/data pointers and opcode must not be the ones resulting
+ * from the stub execution in the hypervisor.
+ *
+ * FPU and SIMD instructions faulting during memory write must not
+ * update the respective register files.
+ *
+ * @see tests/fpu-state/main.c
+ */
+#include <xtf.h>
+
+#include <arch/x86/exinfo.h>
+#include <arch/x86/x87.h>
+
+const char test_title[] = "FPU State";
+
+void probe_fstp(bool force)
+{
+ const uint8_t *fstp_offs;
+ uint32_t flt;
+ struct x87_env_pm32 fenv;
+
+ fenv.cw = 0x35f; /* unmask PE */
+ asm volatile ( "fninit;"
+ "fldcw %[cw];"
+ "fldpi;"
+ "mov $1f, %[offs];"
+ "test %[fep], %[fep];"
+ "jz 1f;"
+ _ASM_XEN_FEP
+ "1: fstps %[data]; 2:"
+ : [offs] "=&g" (fstp_offs), [data] "=m" (flt)
+ : [cw] "m" (fenv.cw), [fep] "q" (force) );
+
+ asm ( "fnstenv %0" : "=m" (fenv) );
+ if ( fenv.ip != (unsigned long)fstp_offs )
+ xtf_failure("Fail: FIP wrong (%08x)\n", fenv.ip);
+ if ( fenv.cs && fenv.cs != __KERN_CS )
+ {
+#ifdef __x86_64__
+ /*
+ * Tolerate CS being in the hypervisor reserved selector range on
+ * AMD hardware, as their 64-bit {F,}XRSTOR do not appear to clear
+ * FCS/FDS.
+ */
+ if ( vendor_is_amd && !(fenv.cs & X86_SEL_LDT) &&
+ (fenv.cs >> PAGE_SHIFT) == FIRST_RESERVED_GDT_PAGE )
+ xtf_warning("Warning: FCS wrong (%04x)\n", fenv.cs);
+ else
+#endif
+ xtf_failure("Fail: FCS wrong (%04x)\n", fenv.cs);
+ }
+ if ( fenv.dp != (unsigned long)&flt )
+ xtf_failure("Fail: FDP wrong (%08x)\n", fenv.dp);
+ if ( fenv.ds && fenv.ds != __KERN_DS )
+ xtf_failure("Fail: FDS wrong (%04x)\n", fenv.ds);
+ /* Skip possible opcode prefixes before checking the opcode. */
+ while ( (fstp_offs[0] & ~7) != 0xd8 )
+ ++fstp_offs;
+ if ( fenv.op && fenv.op != (((fstp_offs[0] & 7) << 8) | fstp_offs[1]) )
+ xtf_failure("Fail: FOP wrong (%03x)\n", fenv.op);
+}
+
+void probe_fistp(bool force)
+{
+ unsigned long fldpi_offs;
+ exinfo_t fault = 0;
+ uint16_t fsw;
+ struct x87_env_pm32 fenv;
+ typeof(xtf_failure) *diagfn;
+ const char *prefix;
+
+ asm volatile ( "fninit;"
+ "0: fldpi;"
+ "mov $0b, %[offs];"
+ "test %[fep], %[fep];"
+ "jz 1f;"
+ _ASM_XEN_FEP
+ "1: fistps (%[ptr]); 2:"
+ _ASM_EXTABLE_HANDLER(1b, 2b, ex_record_fault_eax)
+ : [offs] "=&g" (fldpi_offs), "+a" (fault)
+ : [ptr] "r" (0), [fep] "q" (force) );
+
+ if ( !fault )
+ xtf_error("Error: FISTP to NULL did not fault\n");
+
+ asm ( "fnstsw %0" : "=am" (fsw) );
+ if ( fsw != 0x3800 )
+ xtf_failure("Fail: FSW changed unexpectedly (%04x)\n", fsw);
+
+ asm ( "fnstenv %0" : "=m" (fenv) );
+ /*
+ * The AMD-specific FPU pointer leak workaround in Xen (using FISTPL,
+ * which we check for below) causes all the remaining checks to fail.
+ */
+ if ( !vendor_is_amd || (fenv.op & 0x738) != 0x300 )
+ {
+ diagfn = xtf_failure;
+ prefix = "Fail";
+ }
+ else
+ {
+ diagfn = xtf_warning;
+ prefix = "Warning";
+ }
+ if ( fenv.ip != fldpi_offs )
+ diagfn("%s: FIP changed unexpectedly (%08x)\n", prefix, fenv.ip);
+ if ( fenv.cs && fenv.cs != __KERN_CS )
+ diagfn("%s: FCS changed unexpectedly (%04x)\n", prefix, fenv.cs);
+ if ( fenv.dp )
+ diagfn("%s: FDP changed unexpectedly (%08x)\n", prefix, fenv.dp);
+ if ( fenv.ds )
+ diagfn("%s: FDS changed unexpectedly (%04x)\n", prefix, fenv.ds);
+ if ( fenv.op && fenv.op != 0x1eb )
+ diagfn("%s: FOP changed unexpectedly (%03x)\n", prefix, fenv.op);
+}
+
+void probe_vcvtps2ph(bool force)
+{
+ exinfo_t fault = 0;
+ uint32_t mxcsr = 0x1f80;
+
+ asm volatile ( "vldmxcsr %[mxcsr];"
+ "vpcmpeqb %%xmm0,%%xmm0,%%xmm0;"
+ "vpcmpgtb %%xmm0,%%xmm0,%%xmm1;"
+ "vpunpcklbw %%xmm1,%%xmm0,%%xmm2;"
+ "test %[fep], %[fep];"
+ "jz 1f;"
+ _ASM_XEN_FEP
+ "1: vcvtps2ph $0,%%xmm2,(%[ptr]); 2:"
+ _ASM_EXTABLE_HANDLER(1b, 2b, ex_record_fault_eax)
+ : "+a" (fault)
+ : [ptr] "r" (0), [mxcsr] "m" (mxcsr), [fep] "q" (force) );
+
+ if ( !fault )
+ xtf_error("Error: VCVTPS2PH to NULL did not fault\n");
+ else if ( exinfo_vec(fault) == X86_EXC_UD )
+ {
+ if ( force )
+ xtf_skip("Emulator does not support VCVTPS2PH\n");
+ else
+ xtf_failure("Fail: VCVTPS2PH did #UD\n");
+ }
+
+ asm ( "vstmxcsr %0" : "=m" (mxcsr) );
+ if ( mxcsr != 0x1f80 )
+ {
+ /*
+ * Expect AMD hardware and emulation to behave correctly, but tolerate
+ * unexpected behavior on Intel hardware.
+ */
+ if ( force || vendor_is_amd )
+ xtf_failure("Fail: MXCSR changed unexpectedly (%08x)\n", mxcsr);
+ else
+ xtf_warning("Warning: MXCSR changed unexpectedly (%08x)\n", mxcsr);
+ }
+}
+
+void run_tests(bool force)
+{
+ if ( cpu_has_fpu )
+ {
+ printk("Testing%s FSTP\n", force ? " emulated" : "");
+ probe_fstp(force);
+
+ printk("Testing%s FISTP (to NULL)\n", force ? " emulated" : "");
+ probe_fistp(force);
+ }
+
+ if ( cpu_has_f16c )
+ {
+ unsigned long cr4 = read_cr4();
+ unsigned long xcr0;
+
+ write_cr4(cr4 | X86_CR4_OSXSAVE);
+ xcr0 = read_xcr0();
+ write_xcr0(xcr0 | XSTATE_SSE | XSTATE_YMM);
+
+ printk("Testing%s VCVTPS2PH (to NULL)\n", force ? " emulated" : "");
+ probe_vcvtps2ph(force);
+
+ write_xcr0(xcr0);
+ write_cr4(cr4);
+ }
+}
+
+void test_main(void)
+{
+ run_tests(false);
+
+ if ( !xtf_has_fep )
+ xtf_skip("FEP support not detected - some tests will be skipped\n");
+ else
+ run_tests(true);
+
+ xtf_success(NULL);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
[-- Attachment #3: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] x86emul: correct handling of FPU insns faulting on memory write
2017-03-15 10:28 ` [PATCH v2 2/3] x86emul: correct handling of FPU insns faulting on memory write Jan Beulich
@ 2017-03-15 12:06 ` Paul Durrant
2017-03-15 13:24 ` Boris Ostrovsky
1 sibling, 0 replies; 12+ messages in thread
From: Paul Durrant @ 2017-03-15 12:06 UTC (permalink / raw)
To: 'Jan Beulich', xen-devel
Cc: Boris Ostrovsky, Andrew Cooper, Kevin Tian, Jun Nakajima,
Suravee Suthikulpanit
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 15 March 2017 10:28
> To: xen-devel <xen-devel@lists.xenproject.org>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>; Andrew
> Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant
> <Paul.Durrant@citrix.com>; Jun Nakajima <jun.nakajima@intel.com>; Kevin
> Tian <kevin.tian@intel.com>; Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Subject: [PATCH v2 2/3] x86emul: correct handling of FPU insns faulting on
> memory write
>
> When an FPU instruction with a memory destination fails during the
> memory write, it should not affect FPU register state. Due to the way
> we emulate FPU (and SIMD) instructions, we can only guarantee this by
> - backing out changes to the FPU register state in such a case or
> - doing a descriptor read and/or page walk up front, perhaps with the
> stubs accessing the actual memory location then.
> The latter would require a significant change in how the emulator does
> its guest memory accessing, so for now the former variant is being
> chosen.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
emulate.c parts...
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> v2: Re-base.
> ---
> Note that the state save overhead (unless state hadn't been loaded at
> all before, which should only be possible if a guest is fiddling with
> the instruction stream under emulation) is taken for every FPU insn
> hitting the emulator. We could reduce this to just the ones writing to
> memory, but that would involve quite a few further changes and
> resulting code where even more code paths need to match up with one
> another.
>
> --- a/tools/fuzz/x86_instruction_emulator/x86-insn-emulator-fuzzer.c
> +++ b/tools/fuzz/x86_instruction_emulator/x86-insn-emulator-fuzzer.c
> @@ -433,6 +433,7 @@ static struct x86_emulate_ops fuzz_emulo
> SET(wbinvd),
> SET(invlpg),
> .get_fpu = emul_test_get_fpu,
> + .put_fpu = emul_test_put_fpu,
> .cpuid = emul_test_cpuid,
> };
> #undef SET
> --- a/tools/tests/x86_emulator/test_x86_emulator.c
> +++ b/tools/tests/x86_emulator/test_x86_emulator.c
> @@ -293,6 +293,7 @@ static struct x86_emulate_ops emulops =
> .read_cr = emul_test_read_cr,
> .read_msr = read_msr,
> .get_fpu = emul_test_get_fpu,
> + .put_fpu = emul_test_put_fpu,
> };
>
> int main(int argc, char **argv)
> --- a/tools/tests/x86_emulator/x86_emulate.c
> +++ b/tools/tests/x86_emulator/x86_emulate.c
> @@ -138,4 +138,11 @@ int emul_test_get_fpu(
> return X86EMUL_OKAY;
> }
>
> +void emul_test_put_fpu(
> + struct x86_emulate_ctxt *ctxt,
> + enum x86_emulate_fpu_type backout)
> +{
> + /* TBD */
> +}
> +
> #include "x86_emulate/x86_emulate.c"
> --- a/tools/tests/x86_emulator/x86_emulate.h
> +++ b/tools/tests/x86_emulator/x86_emulate.h
> @@ -178,3 +178,7 @@ int emul_test_get_fpu(
> void *exception_callback_arg,
> enum x86_emulate_fpu_type type,
> struct x86_emulate_ctxt *ctxt);
> +
> +void emul_test_put_fpu(
> + struct x86_emulate_ctxt *ctxt,
> + enum x86_emulate_fpu_type backout);
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -15,6 +15,7 @@
> #include <xen/paging.h>
> #include <xen/trace.h>
> #include <asm/event.h>
> +#include <asm/i387.h>
> #include <asm/xstate.h>
> #include <asm/hvm/emulate.h>
> #include <asm/hvm/hvm.h>
> @@ -1619,6 +1620,35 @@ static int hvmemul_get_fpu(
>
> if ( !curr->fpu_dirtied )
> hvm_funcs.fpu_dirty_intercept();
> + else if ( type == X86EMUL_FPU_fpu )
> + {
> + const typeof(curr->arch.xsave_area->fpu_sse) *fpu_ctxt =
> + curr->arch.fpu_ctxt;
> +
> + /*
> + * Latch current register state so that we can back out changes
> + * if needed (namely when a memory write fails after register state
> + * has already been updated).
> + * NB: We don't really need the "enable" part of the called function
> + * (->fpu_dirtied set implies CR0.TS clear), but the additional
> + * overhead should be low enough to not warrant introduction of yet
> + * another slightly different function. However, we need to undo the
> + * ->fpu_dirtied clearing the function does as well as the possible
> + * masking of all exceptions by FNSTENV.)
> + */
> + save_fpu_enable();
> + curr->fpu_dirtied = true;
> + if ( (fpu_ctxt->fcw & 0x3f) != 0x3f )
> + {
> + uint16_t fcw;
> +
> + asm ( "fnstcw %0" : "=m" (fcw) );
> + if ( (fcw & 0x3f) == 0x3f )
> + asm ( "fldcw %0" :: "m" (fpu_ctxt->fcw) );
> + else
> + ASSERT(fcw == fpu_ctxt->fcw);
> + }
> + }
>
> curr->arch.hvm_vcpu.fpu_exception_callback = exception_callback;
> curr->arch.hvm_vcpu.fpu_exception_callback_arg =
> exception_callback_arg;
> @@ -1627,10 +1657,24 @@ static int hvmemul_get_fpu(
> }
>
> static void hvmemul_put_fpu(
> - struct x86_emulate_ctxt *ctxt)
> + struct x86_emulate_ctxt *ctxt,
> + enum x86_emulate_fpu_type backout)
> {
> struct vcpu *curr = current;
> +
> curr->arch.hvm_vcpu.fpu_exception_callback = NULL;
> +
> + if ( backout == X86EMUL_FPU_fpu )
> + {
> + /*
> + * To back out changes to the register file simply adjust state such
> + * that upon next FPU insn use by the guest we'll reload the state
> + * saved (or freshly loaded) by hvmemul_get_fpu().
> + */
> + curr->fpu_dirtied = false;
> + stts();
> + hvm_funcs.fpu_leave(curr);
> + }
> }
>
> static int hvmemul_invlpg(
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2268,6 +2268,7 @@ static struct hvm_function_table __initd
> .update_guest_cr = svm_update_guest_cr,
> .update_guest_efer = svm_update_guest_efer,
> .update_guest_vendor = svm_update_guest_vendor,
> + .fpu_leave = svm_fpu_leave,
> .set_guest_pat = svm_set_guest_pat,
> .get_guest_pat = svm_get_guest_pat,
> .set_tsc_offset = svm_set_tsc_offset,
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2128,6 +2128,7 @@ static struct hvm_function_table __initd
> .update_guest_cr = vmx_update_guest_cr,
> .update_guest_efer = vmx_update_guest_efer,
> .update_guest_vendor = vmx_update_guest_vendor,
> + .fpu_leave = vmx_fpu_leave,
> .set_guest_pat = vmx_set_guest_pat,
> .get_guest_pat = vmx_get_guest_pat,
> .set_tsc_offset = vmx_set_tsc_offset,
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -965,6 +965,7 @@ static int _get_fpu(
> {
> unsigned long cr0;
>
> + fail_if(type == X86EMUL_FPU_fpu && !ops->put_fpu);
> fic->type = type;
>
> fail_if(!ops->read_cr);
> @@ -1026,11 +1027,14 @@ do {
>
> static void put_fpu(
> struct fpu_insn_ctxt *fic,
> + bool failed_late,
> struct x86_emulate_ctxt *ctxt,
> const struct x86_emulate_ops *ops)
> {
> - if ( fic->type != X86EMUL_FPU_none && ops->put_fpu )
> - ops->put_fpu(ctxt);
> + if ( unlikely(failed_late) && fic->type == X86EMUL_FPU_fpu )
> + ops->put_fpu(ctxt, X86EMUL_FPU_fpu);
> + else if ( fic->type != X86EMUL_FPU_none && ops->put_fpu )
> + ops->put_fpu(ctxt, X86EMUL_FPU_none);
> fic->type = X86EMUL_FPU_none;
> }
>
> @@ -3716,9 +3720,9 @@ x86_emulate(
> break;
>
> case 0x9b: /* wait/fwait */
> - fic.insn_bytes = 1;
> host_and_vcpu_must_have(fpu);
> get_fpu(X86EMUL_FPU_wait, &fic);
> + fic.insn_bytes = 1;
> asm volatile ( "fwait" ::: "memory" );
> check_fpu_exn(&fic);
> break;
> @@ -7892,7 +7896,7 @@ x86_emulate(
> }
>
> complete_insn: /* Commit shadow register state. */
> - put_fpu(&fic, ctxt, ops);
> + put_fpu(&fic, false, ctxt, ops);
>
> /* Zero the upper 32 bits of %rip if not in 64-bit mode. */
> if ( !mode_64bit() )
> @@ -7916,7 +7920,7 @@ x86_emulate(
> ctxt->regs->eflags &= ~X86_EFLAGS_RF;
>
> done:
> - put_fpu(&fic, ctxt, ops);
> + put_fpu(&fic, fic.insn_bytes > 0 && dst.type == OP_MEM, ctxt, ops);
> put_stub(stub);
> return rc;
> #undef state
> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -437,9 +437,12 @@ struct x86_emulate_ops
> * put_fpu: Relinquish the FPU. Unhook from FPU/SIMD exception
> handlers.
> * The handler, if installed, must be prepared to get called without
> * the get_fpu one having got called before!
> + * @backout: Undo updates to the specified register file (can, besides
> + * X86EMUL_FPU_none, only be X86EMUL_FPU_fpu at present);
> */
> void (*put_fpu)(
> - struct x86_emulate_ctxt *ctxt);
> + struct x86_emulate_ctxt *ctxt,
> + enum x86_emulate_fpu_type backout);
>
> /* invlpg: Invalidate paging structures which map addressed byte. */
> int (*invlpg)(
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -140,6 +140,8 @@ struct hvm_function_table {
>
> void (*update_guest_vendor)(struct vcpu *v);
>
> + void (*fpu_leave)(struct vcpu *v);
> +
> int (*get_guest_pat)(struct vcpu *v, u64 *);
> int (*set_guest_pat)(struct vcpu *v, u64);
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] x86emul: correct handling of FPU insns faulting on memory write
2017-03-15 10:28 ` [PATCH v2 2/3] x86emul: correct handling of FPU insns faulting on memory write Jan Beulich
2017-03-15 12:06 ` Paul Durrant
@ 2017-03-15 13:24 ` Boris Ostrovsky
2017-03-15 13:31 ` Jan Beulich
1 sibling, 1 reply; 12+ messages in thread
From: Boris Ostrovsky @ 2017-03-15 13:24 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Andrew Cooper, Kevin Tian, Paul Durrant, Jun Nakajima,
Suravee Suthikulpanit
On 03/15/2017 06:28 AM, Jan Beulich wrote:
> When an FPU instruction with a memory destination fails during the
> memory write, it should not affect FPU register state. Due to the way
> we emulate FPU (and SIMD) instructions, we can only guarantee this by
> - backing out changes to the FPU register state in such a case or
> - doing a descriptor read and/or page walk up front, perhaps with the
> stubs accessing the actual memory location then.
> The latter would require a significant change in how the emulator does
> its guest memory accessing, so for now the former variant is being
> chosen.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
with one question:
>
> @@ -3716,9 +3720,9 @@ x86_emulate(
> break;
>
> case 0x9b: /* wait/fwait */
> - fic.insn_bytes = 1;
> host_and_vcpu_must_have(fpu);
> get_fpu(X86EMUL_FPU_wait, &fic);
> + fic.insn_bytes = 1;
> asm volatile ( "fwait" ::: "memory" );
> check_fpu_exn(&fic);
> break;
>
Why is this needed?
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] x86emul: correct handling of FPU insns faulting on memory write
2017-03-15 13:24 ` Boris Ostrovsky
@ 2017-03-15 13:31 ` Jan Beulich
2017-03-15 13:48 ` Boris Ostrovsky
0 siblings, 1 reply; 12+ messages in thread
From: Jan Beulich @ 2017-03-15 13:31 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Kevin Tian, Suravee Suthikulpanit, Andrew Cooper, Paul Durrant,
Jun Nakajima, xen-devel
>>> On 15.03.17 at 14:24, <boris.ostrovsky@oracle.com> wrote:
> On 03/15/2017 06:28 AM, Jan Beulich wrote:
>> @@ -3716,9 +3720,9 @@ x86_emulate(
>> break;
>>
>> case 0x9b: /* wait/fwait */
>> - fic.insn_bytes = 1;
>> host_and_vcpu_must_have(fpu);
>> get_fpu(X86EMUL_FPU_wait, &fic);
>> + fic.insn_bytes = 1;
>> asm volatile ( "fwait" ::: "memory" );
>> check_fpu_exn(&fic);
>> break;
>
> Why is this needed?
This isn't strictly needed, but desirable, due to the conditional being
added in
@@ -7916,7 +7920,7 @@ x86_emulate(
ctxt->regs->eflags &= ~X86_EFLAGS_RF;
done:
- put_fpu(&fic, ctxt, ops);
+ put_fpu(&fic, fic.insn_bytes > 0 && dst.type == OP_MEM, ctxt, ops);
put_stub(stub);
return rc;
#undef state
(both host_and_vcpu_must_have() and get_fpu() may end up
branching to "done"). Everywhere else the field is already being
set after such basic checks.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] x86emul: correct handling of FPU insns faulting on memory write
2017-03-15 13:31 ` Jan Beulich
@ 2017-03-15 13:48 ` Boris Ostrovsky
2017-03-15 15:46 ` Jan Beulich
0 siblings, 1 reply; 12+ messages in thread
From: Boris Ostrovsky @ 2017-03-15 13:48 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Suravee Suthikulpanit, Andrew Cooper, Paul Durrant,
Jun Nakajima, xen-devel
On 03/15/2017 09:31 AM, Jan Beulich wrote:
>>>> On 15.03.17 at 14:24, <boris.ostrovsky@oracle.com> wrote:
>> On 03/15/2017 06:28 AM, Jan Beulich wrote:
>>> @@ -3716,9 +3720,9 @@ x86_emulate(
>>> break;
>>>
>>> case 0x9b: /* wait/fwait */
>>> - fic.insn_bytes = 1;
>>> host_and_vcpu_must_have(fpu);
>>> get_fpu(X86EMUL_FPU_wait, &fic);
>>> + fic.insn_bytes = 1;
>>> asm volatile ( "fwait" ::: "memory" );
>>> check_fpu_exn(&fic);
>>> break;
>> Why is this needed?
> This isn't strictly needed, but desirable, due to the conditional being
> added in
>
> @@ -7916,7 +7920,7 @@ x86_emulate(
> ctxt->regs->eflags &= ~X86_EFLAGS_RF;
>
> done:
> - put_fpu(&fic, ctxt, ops);
> + put_fpu(&fic, fic.insn_bytes > 0 && dst.type == OP_MEM, ctxt, ops);
> put_stub(stub);
> return rc;
> #undef state
>
> (both host_and_vcpu_must_have() and get_fpu() may end up
> branching to "done"). Everywhere else the field is already being
> set after such basic checks.
Ah, OK.
But fic is a local variable that is not initialized (is it?) so
insn_bytes may be non-zero anyway?
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] x86emul: correct handling of FPU insns faulting on memory write
2017-03-15 13:48 ` Boris Ostrovsky
@ 2017-03-15 15:46 ` Jan Beulich
0 siblings, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2017-03-15 15:46 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Kevin Tian, Suravee Suthikulpanit, Andrew Cooper, Paul Durrant,
Jun Nakajima, xen-devel
>>> On 15.03.17 at 14:48, <boris.ostrovsky@oracle.com> wrote:
> On 03/15/2017 09:31 AM, Jan Beulich wrote:
>>>>> On 15.03.17 at 14:24, <boris.ostrovsky@oracle.com> wrote:
>>> On 03/15/2017 06:28 AM, Jan Beulich wrote:
>>>> @@ -3716,9 +3720,9 @@ x86_emulate(
>>>> break;
>>>>
>>>> case 0x9b: /* wait/fwait */
>>>> - fic.insn_bytes = 1;
>>>> host_and_vcpu_must_have(fpu);
>>>> get_fpu(X86EMUL_FPU_wait, &fic);
>>>> + fic.insn_bytes = 1;
>>>> asm volatile ( "fwait" ::: "memory" );
>>>> check_fpu_exn(&fic);
>>>> break;
>>> Why is this needed?
>> This isn't strictly needed, but desirable, due to the conditional being
>> added in
>>
>> @@ -7916,7 +7920,7 @@ x86_emulate(
>> ctxt->regs->eflags &= ~X86_EFLAGS_RF;
>>
>> done:
>> - put_fpu(&fic, ctxt, ops);
>> + put_fpu(&fic, fic.insn_bytes > 0 && dst.type == OP_MEM, ctxt, ops);
>> put_stub(stub);
>> return rc;
>> #undef state
>>
>> (both host_and_vcpu_must_have() and get_fpu() may end up
>> branching to "done"). Everywhere else the field is already being
>> set after such basic checks.
>
> Ah, OK.
>
> But fic is a local variable that is not initialized (is it?) so
> insn_bytes may be non-zero anyway?
We have this at the top of x86_emulate():
struct fpu_insn_ctxt fic = { .type = X86EMUL_FPU_none, .exn_raised = -1 };
(introduced by patch 1).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] x86emul: centralize put_fpu() invocations
2017-03-15 10:27 ` [PATCH v2 1/3] x86emul: centralize put_fpu() invocations Jan Beulich
@ 2017-03-20 17:10 ` Andrew Cooper
0 siblings, 0 replies; 12+ messages in thread
From: Andrew Cooper @ 2017-03-20 17:10 UTC (permalink / raw)
To: Jan Beulich, xen-devel
On 15/03/17 10:27, Jan Beulich wrote:
> ..., splitting parts of it into check_*() macros. This is in
> preparation of making ->put_fpu() do further adjustments to register
> state. (Some of the check_xmm() invocations could be avoided, as in
> some of the cases no insns handled there can actually raise #XM, but I
> think we're better off keeping them to avoid later additions of further
> insn patterns rendering the lack of the check a bug.)
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] x86emul: correct FPU code/data pointers and opcode handling
2017-03-15 10:28 ` [PATCH v2 3/3] x86emul: correct FPU code/data pointers and opcode handling Jan Beulich
@ 2017-03-20 17:26 ` Andrew Cooper
0 siblings, 0 replies; 12+ messages in thread
From: Andrew Cooper @ 2017-03-20 17:26 UTC (permalink / raw)
To: Jan Beulich, xen-devel
On 15/03/17 10:28, Jan Beulich wrote:
> Prevent leaking the hypervisor ones (stored by hardware during stub
> execution), at once making sure the guest sees correct values there.
> This piggybacks on the backout logic used to deal with write faults of
> FPU insns.
>
> Deliberately ignore the NO_FPU_SEL feature here: Honoring it would
> merely mean extra code with no benefit (once we XRSTOR state, the
> selector values will simply be lost anyway).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-03-20 17:26 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-15 10:23 [PATCH v2 0/3] x86emul: FPU handling corrections Jan Beulich
2017-03-15 10:27 ` [PATCH v2 1/3] x86emul: centralize put_fpu() invocations Jan Beulich
2017-03-20 17:10 ` Andrew Cooper
2017-03-15 10:28 ` [PATCH v2 2/3] x86emul: correct handling of FPU insns faulting on memory write Jan Beulich
2017-03-15 12:06 ` Paul Durrant
2017-03-15 13:24 ` Boris Ostrovsky
2017-03-15 13:31 ` Jan Beulich
2017-03-15 13:48 ` Boris Ostrovsky
2017-03-15 15:46 ` Jan Beulich
2017-03-15 10:28 ` [PATCH v2 3/3] x86emul: correct FPU code/data pointers and opcode handling Jan Beulich
2017-03-20 17:26 ` Andrew Cooper
2017-03-15 10:29 ` [PATCH v2][XTF] add FPU/SIMD register state test Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.