Re: [Xen-devel] [PATCH 4/4] x86emul: correct FPU code/data pointers and opcode handling

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/03/17 11:05, Jan Beulich wrote:
> @@ -1027,13 +1076,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 )

Why are the read_segment hooks optional here?

In the case that we are hitting FPU instructions for emulation, we can
reasonably require read/write_segment hooks.

In particular, regs->%sreg are only valid for PV guests.

> +            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);
>  }
>  
>  static inline bool fpu_check_write(void)
> @@ -4231,8 +4268,10 @@ x86_emulate(
>                  dst.bytes = 4;
>                  break;
>              case 4: /* fldenv - TODO */
> +                state->fpu_ctrl = true;

Arguably, state->fpu_ctrl is a decode property rather than an emulation
property.  It is the kind of information which we might plausibly want
an x86_insn_*() accessor for.

~Andrew

>                  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;
>


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel