Re: [PATCH 02/17] x86emul: fetch all insn bytes during the decode phase

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 08/09/16 14:07, Jan Beulich wrote:
> This way we can offer to callers the service of just sizing
> instructions, and we also can better guarantee not to raise the wrong
> fault due to not having read all relevant bytes.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -129,8 +129,8 @@ static const opcode_desc_t opcode_table[
>      ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
>      ImplicitOps|Mov, ImplicitOps|Mov, ImplicitOps, ImplicitOps,
>      /* 0xA0 - 0xA7 */
> -    ByteOp|DstEax|SrcImplicit|Mov, DstEax|SrcImplicit|Mov,
> -    ByteOp|ImplicitOps|Mov, ImplicitOps|Mov,
> +    ByteOp|DstEax|SrcMem|Mov, DstEax|SrcMem|Mov,
> +    ByteOp|DstMem|SrcEax|Mov, DstMem|SrcEax|Mov,
>      ByteOp|ImplicitOps|Mov, ImplicitOps|Mov,
>      ByteOp|ImplicitOps, ImplicitOps,
>      /* 0xA8 - 0xAF */
> @@ -1602,6 +1602,45 @@ struct x86_emulate_state {
>  #define _regs (state->regs)
>  
>  static int
> +x86_decode_base(

What do you mean by decode_base here?

> +    struct x86_emulate_state *state,
> +    struct x86_emulate_ctxt *ctxt,
> +    const struct x86_emulate_ops *ops)
> +{
> +    int rc = X86EMUL_OKAY;
> +
> +    switch ( state->opcode )
> +    {
> +    case 0x9a: /* call (far, absolute) */
> +    case 0xea: /* jmp (far, absolute) */
> +        generate_exception_if(mode_64bit(), EXC_UD, -1);
> +
> +        imm1 = insn_fetch_bytes(op_bytes);
> +        imm2 = insn_fetch_type(uint16_t);
> +        break;
> +
> +    case 0xa0: case 0xa1: /* mov mem.offs,{%al,%ax,%eax,%rax} */
> +    case 0xa2: case 0xa3: /* mov {%al,%ax,%eax,%rax},mem.offs */
> +        /* Source EA is not encoded via ModRM. */
> +        ea.mem.off = insn_fetch_bytes(ad_bytes);
> +        break;
> +
> +    case 0xb8 ... 0xbf: /* mov imm{16,32,64},r{16,32,64} */
> +        if ( op_bytes == 8 ) /* Fetch more bytes to obtain imm64. */
> +            imm1 = ((uint32_t)imm1 |
> +                    ((uint64_t)insn_fetch_type(uint32_t) << 32));
> +        break;
> +
> +    case 0xc8: /* enter imm16,imm8 */
> +        imm2 = insn_fetch_type(uint8_t);
> +        break;
> +    }
> +
> + done:
> +    return rc;
> +}
> +
> +static int
>  x86_decode(
>      struct x86_emulate_state *state,
>      struct x86_emulate_ctxt *ctxt,
> @@ -1994,10 +2033,29 @@ x86_decode(
>      state->opcode = b;
>      state->desc = d;
>  
> +    switch ( ext )
> +    {
> +    case ext_none:
> +        rc = x86_decode_base(state, ctxt, ops);
> +        break;
> +
> +    case ext_0f:
> +    case ext_0f38:
> +        break;
> +
> +    default:
> +        ASSERT_UNREACHABLE();
> +        return X86EMUL_UNHANDLEABLE;
> +    }
> +
>   done:
>      return rc;
>  }
>  
> +/* No insn fetching past this point. */
> +#undef insn_fetch_bytes
> +#undef insn_fetch_type
> +
>  int
>  x86_emulate(
>      struct x86_emulate_ctxt *ctxt,
> @@ -2560,6 +2618,8 @@ x86_emulate(
>      case 0xc6 ... 0xc7: /* mov (sole member of Grp11) */
>          generate_exception_if((modrm_reg & 7) != 0, EXC_UD, -1);
>      case 0x88 ... 0x8b: /* mov */
> +    case 0xa0 ... 0xa1: /* mov mem.offs,{%al,%ax,%eax,%rax} */
> +    case 0xa2 ... 0xa3: /* mov {%al,%ax,%eax,%rax},mem.offs */
>          dst.val = src.val;
>          break;
>  
> @@ -2644,18 +2704,13 @@ x86_emulate(
>  
>      case 0x9a: /* call (far, absolute) */ {
>          struct segment_register reg;
> -        uint16_t sel;
> -        uint32_t eip;
>  
> -        generate_exception_if(mode_64bit(), EXC_UD, -1);
> +        ASSERT(!mode_64bit());

Are we going to strictly require that noone ever hand-crafts a
x86_emulate_state and hands it to x86_emulate()?

I would suggest leaving the generate_exception_if(mode_64bit(), EXC_UD,
-1); after the ASSERT() so even if we do end up in a wonky state, we
don't try to jump the guest to 0.

Similarly for jmp.

~Andrew

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