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

[Jan Beulich]

>>> On 23.09.16 at 16:48,  wrote:
> On 14/09/16 10:55, Jan Beulich wrote:
> On 13.09.16 at 20:44,  wrote:
>>> 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.
>> That would look really strange to a reader, I think, and hence I'd
>> rather not do this if I can get the patch accepted without.
> 
> It is conceptually no different to
> 
> default:
> ASSERT_UNREACHABLE();
> return;

Except that
- we don't always follow ASSERT_UNREACHABLE() with return (or
  whatever else),
- here we don't risk doing ourselves or the guest anything bad: We'll
  just produce undefined behavior, which is to be expected if we are
  in a "wonky state".

If leaving the exception generation in is the only way to get this
ack-ed, I'll do so, but I'd prefer if I wasn't required to.

Jan


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

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

[Andrew Cooper]

On 14/09/16 10:55, Jan Beulich wrote:

On 13.09.16 at 20:44,  wrote:

On 08/09/16 14:07, Jan Beulich wrote:

@@ -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?

The base instruction set (no 0f or alike prefixes). Suggestions for
a better name welcome.


x86_decode_onebyte() to match the table of opcodes it is further decoding.




@@ -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()?

Absolutely - that's why its definition does not live in a header.


Ok.




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.

That would look really strange to a reader, I think, and hence I'd
rather not do this if I can get the patch accepted without.


It is conceptually no different to

default:
ASSERT_UNREACHABLE();
return;

~Andrew

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

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

[Jan Beulich]

>>> On 13.09.16 at 20:44,  wrote:
> On 08/09/16 14:07, Jan Beulich wrote:
>> @@ -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?

The base instruction set (no 0f or alike prefixes). Suggestions for
a better name welcome.

>> @@ -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()?

Absolutely - that's why its definition does not live in a header.

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

That would look really strange to a reader, I think, and hence I'd
rather not do this if I can get the patch accepted without.

Jan


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

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

[Andrew Cooper]

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

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

[Jan Beulich]

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 

--- 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(
+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());
 fail_if(ops->read_segment == NULL);
 
-eip = insn_fetch_bytes(op_bytes);
-sel = insn_fetch_type(uint16_t);
-
 if ( (rc = ops->read_segment(x86_seg_cs, , ctxt)) ||
- (rc = load_seg(x86_seg_cs, sel, 0, , ctxt, ops)) ||
- (validate_far_branch(, eip),
+ (rc = load_seg(x86_seg_cs, imm2, 0, , ctxt, ops)) ||
+ (validate_far_branch(, imm1),
   rc = ops->write(x86_seg_ss, sp_pre_dec(op_bytes),
   , op_bytes, ctxt)) ||
  (rc = ops->write(x86_seg_ss, sp_pre_dec(op_bytes),
@@ -2663,7 +2718,7 @@ x86_emulate(
  (rc = ops->write_segment(x86_seg_cs, , ctxt)) )
 goto done;
 
-_regs.eip = eip;
+_regs.eip = imm1;
 break;
 }
 
@@ -2706,23 +2761,6 @@ x86_emulate(
 ((uint8_t *)&_regs.eax)[1] = (_regs.eflags & 0xd7) | 0x02;
 break;
 
-case 0xa0 ... 0xa1: /* mov mem.offs,{%al,%ax,%eax,%rax} */
-/* Source EA is not encoded via ModRM. */
-dst.bytes = (d & ByteOp) ? 1 : op_bytes;
-if ( (rc = read_ulong(ea.mem.seg, insn_fetch_bytes(ad_bytes),
-  , dst.bytes, ctxt, ops)) != 0 )
-goto done;
-break;
-
-case 0xa2 ... 0xa3: /* mov {%al,%ax,%eax,%rax},mem.offs */
-/* Destination EA is not encoded via ModRM. */
-dst.type  = OP_MEM;
-dst.mem.seg = ea.mem.seg;
-dst.mem.off = insn_fetch_bytes(ad_bytes);
-dst.bytes = (d & ByteOp) ? 1 : op_bytes;
-dst.val   = (unsigned long)_regs.eax;
-break;
-
 case 0xa4 ... 0xa5: /* movs */ {
 unsigned long nr_reps = get_rep_prefix();
 dst.bytes = (d &