|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v8 08/12] x86emul: support FLDENV and FRSTOR
On Tue, May 05, 2020 at 10:16:20AM +0200, Jan Beulich wrote:
> While the Intel SDM claims that FRSTOR itself may raise #MF upon
> completion, this was confirmed by Intel to be a doc error which will be
> corrected in due course; behavior is like FLDENV, and like old hard copy
> manuals describe it. Otherwise we'd have to emulate the insn by filling
> st(N) in suitable order, followed by FLDENV.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v7: New.
>
> --- a/tools/tests/x86_emulator/test_x86_emulator.c
> +++ b/tools/tests/x86_emulator/test_x86_emulator.c
> @@ -2442,6 +2442,27 @@ int main(int argc, char **argv)
> else
> printf("skipped\n");
>
> + printf("%-40s", "Testing fldenv 8(%edx)...");
Likely a stupid question, but why the added 8? edx will contain the
memory address used to save the sate by fnstenv, so I would expect
fldenv to just load from there?
> + if ( stack_exec && cpu_has_fpu )
> + {
> + asm volatile ( "fnstenv %0\n\t"
> + "fninit"
> + : "=m" (res[2]) :: "memory" );
Why do you need the memory clobber here? I assume it's because res is
of type unsigned int and hence doesn't have the right size that
fnstenv will actually write to?
> + zap_fpsel(&res[2], true);
> + instr[0] = 0xd9; instr[1] = 0x62; instr[2] = 0x08;
> + regs.eip = (unsigned long)&instr[0];
> + regs.edx = (unsigned long)res;
> + rc = x86_emulate(&ctxt, &emulops);
> + asm volatile ( "fnstenv %0" : "=m" (res[9]) :: "memory" );
> + if ( (rc != X86EMUL_OKAY) ||
> + memcmp(res + 2, res + 9, 28) ||
> + (regs.eip != (unsigned long)&instr[3]) )
> + goto fail;
> + printf("okay\n");
> + }
> + else
> + printf("skipped\n");
> +
> printf("%-40s", "Testing 16-bit fnsave (%ecx)...");
> if ( stack_exec && cpu_has_fpu )
> {
> @@ -2468,6 +2489,31 @@ int main(int argc, char **argv)
> goto fail;
> printf("okay\n");
> }
> + else
> + printf("skipped\n");
> +
> + printf("%-40s", "Testing frstor (%edx)...");
> + if ( stack_exec && cpu_has_fpu )
> + {
> + const uint16_t seven = 7;
> +
> + asm volatile ( "fninit\n\t"
> + "fld1\n\t"
> + "fidivs %1\n\t"
> + "fnsave %0\n\t"
> + : "=&m" (res[0]) : "m" (seven) : "memory" );
> + zap_fpsel(&res[0], true);
> + instr[0] = 0xdd; instr[1] = 0x22;
> + regs.eip = (unsigned long)&instr[0];
> + regs.edx = (unsigned long)res;
> + rc = x86_emulate(&ctxt, &emulops);
> + asm volatile ( "fnsave %0" : "=m" (res[27]) :: "memory" );
> + if ( (rc != X86EMUL_OKAY) ||
> + memcmp(res, res + 27, 108) ||
> + (regs.eip != (unsigned long)&instr[2]) )
> + goto fail;
> + printf("okay\n");
> + }
> else
> printf("skipped\n");
>
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -857,6 +857,7 @@ struct x86_emulate_state {
> blk_NONE,
> blk_enqcmd,
> #ifndef X86EMUL_NO_FPU
> + blk_fld, /* FLDENV, FRSTOR */
> blk_fst, /* FNSTENV, FNSAVE */
> #endif
> blk_movdir,
> @@ -4948,21 +4949,14 @@ x86_emulate(
> dst.bytes = 4;
> emulate_fpu_insn_memdst(b, modrm_reg & 7, dst.val);
> break;
> - case 4: /* fldenv - TODO */
> - state->fpu_ctrl = true;
> - goto unimplemented_insn;
> - case 5: /* fldcw m2byte */
> - state->fpu_ctrl = true;
> - fpu_memsrc16:
> - if ( (rc = ops->read(ea.mem.seg, ea.mem.off, &src.val,
> - 2, ctxt)) != X86EMUL_OKAY )
> - goto done;
> - emulate_fpu_insn_memsrc(b, modrm_reg & 7, src.val);
> - break;
> + case 4: /* fldenv */
> + /* Raise #MF now if there are pending unmasked exceptions. */
> + emulate_fpu_insn_stub(0xd9, 0xd0 /* fnop */);
Maybe it would make sense to have a wrapper for fnop?
> + /* fall through */
> case 6: /* fnstenv */
> fail_if(!ops->blk);
> - state->blk = blk_fst;
> - /* REX is meaningless for this insn by this point. */
> + state->blk = modrm_reg & 2 ? blk_fst : blk_fld;
> + /* REX is meaningless for these insns by this point. */
> rex_prefix = in_protmode(ctxt, ops);
> if ( (rc = ops->blk(ea.mem.seg, ea.mem.off, NULL,
> op_bytes > 2 ? sizeof(struct x87_env32)
> @@ -4972,6 +4966,14 @@ x86_emulate(
> goto done;
> state->fpu_ctrl = true;
> break;
> + case 5: /* fldcw m2byte */
> + state->fpu_ctrl = true;
> + fpu_memsrc16:
> + if ( (rc = ops->read(ea.mem.seg, ea.mem.off, &src.val,
> + 2, ctxt)) != X86EMUL_OKAY )
> + goto done;
> + emulate_fpu_insn_memsrc(b, modrm_reg & 7, src.val);
> + break;
> case 7: /* fnstcw m2byte */
> state->fpu_ctrl = true;
> fpu_memdst16:
> @@ -5124,13 +5126,14 @@ x86_emulate(
> dst.bytes = 8;
> emulate_fpu_insn_memdst(b, modrm_reg & 7, dst.val);
> break;
> - case 4: /* frstor - TODO */
> - state->fpu_ctrl = true;
> - goto unimplemented_insn;
> + case 4: /* frstor */
> + /* Raise #MF now if there are pending unmasked exceptions. */
> + emulate_fpu_insn_stub(0xd9, 0xd0 /* fnop */);
> + /* fall through */
> case 6: /* fnsave */
> fail_if(!ops->blk);
> - state->blk = blk_fst;
> - /* REX is meaningless for this insn by this point. */
> + state->blk = modrm_reg & 2 ? blk_fst : blk_fld;
> + /* REX is meaningless for these insns by this point. */
> rex_prefix = in_protmode(ctxt, ops);
> if ( (rc = ops->blk(ea.mem.seg, ea.mem.off, NULL,
> op_bytes > 2 ? sizeof(struct x87_env32)
> + 80
> @@ -11648,6 +11651,89 @@ int x86_emul_blk(
>
> #ifndef X86EMUL_NO_FPU
>
> + case blk_fld:
> + ASSERT(!data);
> +
> + /* state->rex_prefix carries CR0.PE && !EFLAGS.VM setting */
> + switch ( bytes )
> + {
> + case sizeof(fpstate.env):
> + case sizeof(fpstate):
> + memcpy(&fpstate.env, ptr, sizeof(fpstate.env));
> + if ( !state->rex_prefix )
> + {
> + unsigned int fip = fpstate.env.mode.real.fip_lo +
> + (fpstate.env.mode.real.fip_hi << 16);
> + unsigned int fdp = fpstate.env.mode.real.fdp_lo +
> + (fpstate.env.mode.real.fdp_hi << 16);
> + unsigned int fop = fpstate.env.mode.real.fop;
> +
> + fpstate.env.mode.prot.fip = fip & 0xf;
> + fpstate.env.mode.prot.fcs = fip >> 4;
> + fpstate.env.mode.prot.fop = fop;
> + fpstate.env.mode.prot.fdp = fdp & 0xf;
> + fpstate.env.mode.prot.fds = fdp >> 4;
I've found the layouts in the SDM vol. 1, but I haven't been able to
found the translation mechanism from real to protected. Could you
maybe add a reference here?
> + }
> +
> + if ( bytes == sizeof(fpstate.env) )
> + ptr = NULL;
> + else
> + ptr += sizeof(fpstate.env);
> + break;
> +
> + case sizeof(struct x87_env16):
> + case sizeof(struct x87_env16) + sizeof(fpstate.freg):
> + {
> + const struct x87_env16 *env = ptr;
> +
> + fpstate.env.fcw = env->fcw;
> + fpstate.env.fsw = env->fsw;
> + fpstate.env.ftw = env->ftw;
> +
> + if ( state->rex_prefix )
> + {
> + fpstate.env.mode.prot.fip = env->mode.prot.fip;
> + fpstate.env.mode.prot.fcs = env->mode.prot.fcs;
> + fpstate.env.mode.prot.fdp = env->mode.prot.fdp;
> + fpstate.env.mode.prot.fds = env->mode.prot.fds;
> + fpstate.env.mode.prot.fop = 0; /* unknown */
> + }
> + else
> + {
> + unsigned int fip = env->mode.real.fip_lo +
> + (env->mode.real.fip_hi << 16);
> + unsigned int fdp = env->mode.real.fdp_lo +
> + (env->mode.real.fdp_hi << 16);
> + unsigned int fop = env->mode.real.fop;
> +
> + fpstate.env.mode.prot.fip = fip & 0xf;
> + fpstate.env.mode.prot.fcs = fip >> 4;
> + fpstate.env.mode.prot.fop = fop;
> + fpstate.env.mode.prot.fdp = fdp & 0xf;
> + fpstate.env.mode.prot.fds = fdp >> 4;
This looks mostly the same as the translation done above, so maybe
could be abstracted anyway in a macro to avoid the code repetition?
(ie: fpstate_real_to_prot(src, dst) or some such).
Thanks, Roger.
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |