| Message ID | 5843dca9-1a1a-a32e-3cb0-95cd93533723@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86emul: further work | expand |
On 25/05/2020 15:26, Jan Beulich wrote: > Unlike similarly encoded insns these don't write their memory operands, "write to their". > and hence x86_is_mem_write() should return false for them. However, > rather than adding special logic there, rework how their emulation gets > done, by making decoding attributes properly describe the r/o nature of > their memory operands. Describe how? I see you've change the order of operands encoding, but then override it back? ~Andrew > Note how this also allows dropping custom LOCK prefix checks. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v10: New. > > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -84,7 +84,7 @@ static const opcode_desc_t opcode_table[ > ByteOp|DstReg|SrcMem|ModRM, DstReg|SrcMem|ModRM, > ByteOp|DstEax|SrcImm, DstEax|SrcImm, 0, ImplicitOps, > /* 0x38 - 0x3F */ > - ByteOp|DstMem|SrcReg|ModRM, DstMem|SrcReg|ModRM, > + ByteOp|DstReg|SrcMem|ModRM, DstReg|SrcMem|ModRM, > ByteOp|DstReg|SrcMem|ModRM, DstReg|SrcMem|ModRM, > ByteOp|DstEax|SrcImm, DstEax|SrcImm, 0, ImplicitOps, > /* 0x40 - 0x4F */ > @@ -2481,7 +2481,6 @@ x86_decode_onebyte( > case 0x60: /* pusha */ > case 0x61: /* popa */ > case 0x62: /* bound */ > - case 0x82: /* Grp1 (x86/32 only) */ > case 0xc4: /* les */ > case 0xc5: /* lds */ > case 0xce: /* into */ > @@ -2491,6 +2490,14 @@ x86_decode_onebyte( > state->not_64bit = true; > break; > > + case 0x82: /* Grp1 (x86/32 only) */ > + state->not_64bit = true; > + /* fall through */ > + case 0x80: case 0x81: case 0x83: /* Grp1 */ > + if ( (modrm_reg & 7) == 7 ) /* cmp */ > + state->desc = (state->desc & ByteOp) | DstNone | SrcMem; > + break; > + > case 0x90: /* nop / pause */ > if ( repe_prefix() ) > ctxt->opcode |= X86EMUL_OPC_F3(0, 0); > @@ -2521,6 +2528,11 @@ x86_decode_onebyte( > imm2 = insn_fetch_type(uint8_t); > break; > > + case 0xf6: case 0xf7: /* Grp3 */ > + if ( !(modrm_reg & 6) ) /* test */ > + state->desc = (state->desc & ByteOp) | DstNone | SrcMem; > + break; > + > case 0xff: /* Grp5 */ > switch ( modrm_reg & 7 ) > { > @@ -3928,13 +3940,11 @@ x86_emulate( > break; > > case 0x38: case 0x39: cmp: /* cmp reg,mem */ > - if ( ops->rmw && dst.type == OP_MEM && > - (rc = read_ulong(dst.mem.seg, dst.mem.off, &dst.val, > - dst.bytes, ctxt, ops)) != X86EMUL_OKAY ) > - goto done; > - /* fall through */ > + emulate_2op_SrcV("cmp", dst, src, _regs.eflags); > + dst.type = OP_NONE; > + break; > + > case 0x3a ... 0x3d: /* cmp */ > - generate_exception_if(lock_prefix, EXC_UD); > emulate_2op_SrcV("cmp", src, dst, _regs.eflags); > dst.type = OP_NONE; > break; > @@ -4239,7 +4249,9 @@ x86_emulate( > case 4: goto and; > case 5: goto sub; > case 6: goto xor; > - case 7: goto cmp; > + case 7: > + dst.val = imm1; > + goto cmp; > } > break; > > @@ -5233,11 +5245,8 @@ x86_emulate( > unsigned long u[2], v; > > case 0 ... 1: /* test */ > - generate_exception_if(lock_prefix, EXC_UD); > - if ( ops->rmw && dst.type == OP_MEM && > - (rc = read_ulong(dst.mem.seg, dst.mem.off, &dst.val, > - dst.bytes, ctxt, ops)) != X86EMUL_OKAY ) > - goto done; > + dst.val = imm1; > + dst.bytes = src.bytes; > goto test; > case 2: /* not */ > if ( ops->rmw && dst.type == OP_MEM ) >
On 29.05.2020 14:24, Andrew Cooper wrote: > On 25/05/2020 15:26, Jan Beulich wrote: >> Unlike similarly encoded insns these don't write their memory operands, > > "write to their". > >> and hence x86_is_mem_write() should return false for them. However, >> rather than adding special logic there, rework how their emulation gets >> done, by making decoding attributes properly describe the r/o nature of >> their memory operands. > > Describe how? I see you've change the order of operands encoding, but > then override it back? There's no overriding back, I don't think: I change the table entries for opcodes 0x38 and 0x39, with no other adjustments the the attributes later on. For the other opcodes I leave the table entries as they are, and override the attributes for the specific sub-cases (identified by ModRM.reg). For opcodes 0x38 and 0x39 the change of the table entries implies changing the order of operands as passed to emulate_2op_SrcV(), hence the splitting of the cases in the main switch(). I didn't think this was necessary to spell out in the commit message, but of course I can re-use most of the text above and add it into there, if you think that would help. Jan
On 29/05/2020 14:41, Jan Beulich wrote: > On 29.05.2020 14:24, Andrew Cooper wrote: >> On 25/05/2020 15:26, Jan Beulich wrote: >>> Unlike similarly encoded insns these don't write their memory operands, >> "write to their". >> >>> and hence x86_is_mem_write() should return false for them. However, >>> rather than adding special logic there, rework how their emulation gets >>> done, by making decoding attributes properly describe the r/o nature of >>> their memory operands. >> Describe how? I see you've change the order of operands encoding, but >> then override it back? > There's no overriding back, I don't think: I change the table entries > for opcodes 0x38 and 0x39, with no other adjustments the the attributes > later on. For the other opcodes I leave the table entries as they are, > and override the attributes for the specific sub-cases (identified by > ModRM.reg). > > For opcodes 0x38 and 0x39 the change of the table entries implies > changing the order of operands as passed to emulate_2op_SrcV(), hence > the splitting of the cases in the main switch(). > I didn't think this was necessary to spell out in the commit message, > but of course I can re-use most of the text above and add it into > there, if you think that would help. Yes please. With something suitable, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> ~Andrew
--- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -84,7 +84,7 @@ static const opcode_desc_t opcode_table[ ByteOp|DstReg|SrcMem|ModRM, DstReg|SrcMem|ModRM, ByteOp|DstEax|SrcImm, DstEax|SrcImm, 0, ImplicitOps, /* 0x38 - 0x3F */ - ByteOp|DstMem|SrcReg|ModRM, DstMem|SrcReg|ModRM, + ByteOp|DstReg|SrcMem|ModRM, DstReg|SrcMem|ModRM, ByteOp|DstReg|SrcMem|ModRM, DstReg|SrcMem|ModRM, ByteOp|DstEax|SrcImm, DstEax|SrcImm, 0, ImplicitOps, /* 0x40 - 0x4F */ @@ -2481,7 +2481,6 @@ x86_decode_onebyte( case 0x60: /* pusha */ case 0x61: /* popa */ case 0x62: /* bound */ - case 0x82: /* Grp1 (x86/32 only) */ case 0xc4: /* les */ case 0xc5: /* lds */ case 0xce: /* into */ @@ -2491,6 +2490,14 @@ x86_decode_onebyte( state->not_64bit = true; break; + case 0x82: /* Grp1 (x86/32 only) */ + state->not_64bit = true; + /* fall through */ + case 0x80: case 0x81: case 0x83: /* Grp1 */ + if ( (modrm_reg & 7) == 7 ) /* cmp */ + state->desc = (state->desc & ByteOp) | DstNone | SrcMem; + break; + case 0x90: /* nop / pause */ if ( repe_prefix() ) ctxt->opcode |= X86EMUL_OPC_F3(0, 0); @@ -2521,6 +2528,11 @@ x86_decode_onebyte( imm2 = insn_fetch_type(uint8_t); break; + case 0xf6: case 0xf7: /* Grp3 */ + if ( !(modrm_reg & 6) ) /* test */ + state->desc = (state->desc & ByteOp) | DstNone | SrcMem; + break; + case 0xff: /* Grp5 */ switch ( modrm_reg & 7 ) { @@ -3928,13 +3940,11 @@ x86_emulate( break; case 0x38: case 0x39: cmp: /* cmp reg,mem */ - if ( ops->rmw && dst.type == OP_MEM && - (rc = read_ulong(dst.mem.seg, dst.mem.off, &dst.val, - dst.bytes, ctxt, ops)) != X86EMUL_OKAY ) - goto done; - /* fall through */ + emulate_2op_SrcV("cmp", dst, src, _regs.eflags); + dst.type = OP_NONE; + break; + case 0x3a ... 0x3d: /* cmp */ - generate_exception_if(lock_prefix, EXC_UD); emulate_2op_SrcV("cmp", src, dst, _regs.eflags); dst.type = OP_NONE; break; @@ -4239,7 +4249,9 @@ x86_emulate( case 4: goto and; case 5: goto sub; case 6: goto xor; - case 7: goto cmp; + case 7: + dst.val = imm1; + goto cmp; } break; @@ -5233,11 +5245,8 @@ x86_emulate( unsigned long u[2], v; case 0 ... 1: /* test */ - generate_exception_if(lock_prefix, EXC_UD); - if ( ops->rmw && dst.type == OP_MEM && - (rc = read_ulong(dst.mem.seg, dst.mem.off, &dst.val, - dst.bytes, ctxt, ops)) != X86EMUL_OKAY ) - goto done; + dst.val = imm1; + dst.bytes = src.bytes; goto test; case 2: /* not */ if ( ops->rmw && dst.type == OP_MEM )
Unlike similarly encoded insns these don't write their memory operands, and hence x86_is_mem_write() should return false for them. However, rather than adding special logic there, rework how their emulation gets done, by making decoding attributes properly describe the r/o nature of their memory operands. Note how this also allows dropping custom LOCK prefix checks. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v10: New.