Re: [Xen-devel] [PATCH 08/17] x86emul: generate and make use of canonical opcode representation
>>> On 27.09.16 at 16:03,wrote: > On 15/09/16 07:43, Jan Beulich wrote: > On 14.09.16 at 19:30, wrote: +#define X86EMUL_OPC_PFX_MASK 0x0300 +# define X86EMUL_OPC_66(ext, byte) (X86EMUL_OPC(ext, byte) | 0x0100) +# define X86EMUL_OPC_F3(ext, byte) (X86EMUL_OPC(ext, byte) | 0x0200) +# define X86EMUL_OPC_F2(ext, byte) (X86EMUL_OPC(ext, byte) | 0x0300) >>> The PFX mask is moderately obvious from here, but a sentence describing >>> what is legitimate to add in the future wouldn't go amiss. >> I don't understand the "what is legitimate to add in the future" >> part: Nothing should be added to this set. > > It occurs to me that using only 2 bits rather than 8 bits for the prefix > information would help the compiler make a smaller switch statements. I don't think this would help - the compiler struggles with the high 16 bits, and that wouldn't change. I'm surprised they're not smart enough to split this into a few compares and a couple of independent branch tables. +#define X86EMUL_OPC_KIND_MASK0x3000 +#define X86EMUL_OPC_VEX_ 0x1000 >>> OTOH, I am rather more confused about what is eligible for inclusion >>> into "kind". Also, what does a kind of 0 indicate? >> VEX, XOP, and EVEX are the valid non-zero kinds. Zero (I would >> say obviously) means neither of those three. > > It is not clear how "kind" is a suitable collective term for VEX/XOP/EVEX. > > Or in other words, X86EMUL_OPC_KIND_MASK doesn't provide any hint that > the operation is referring to a legacy or vex encoding of the instruction. > > Would s/kind/encoding/ be ok? Sure, changed. > At that point, X86EMUL_OPC_LEGACY_ with a > value of 0 might be useful. (e.g. perhaps (opcode & > X86EMUL_OPC_ENCODING_MASK) == X86EMUL_OPC_LEGACY_?) Added for completeness (but it'll be unused for now). +# define X86EMUL_OPC_VEX(ext, byte) \ +(X86EMUL_OPC(ext, byte) | X86EMUL_OPC_VEX_) +# define X86EMUL_OPC_VEX_66(ext, byte) \ +(X86EMUL_OPC_66(ext, byte) | X86EMUL_OPC_VEX_) +# define X86EMUL_OPC_VEX_F3(ext, byte) \ +(X86EMUL_OPC_F3(ext, byte) | X86EMUL_OPC_VEX_) +# define X86EMUL_OPC_VEX_F2(ext, byte) \ +(X86EMUL_OPC_F2(ext, byte) | X86EMUL_OPC_VEX_) +#define X86EMUL_OPC_EVEX_0x2000 +# define X86EMUL_OPC_EVEX(ext, byte) \ +(X86EMUL_OPC(ext, byte) | X86EMUL_OPC_EVEX_) +# define X86EMUL_OPC_EVEX_66(ext, byte) \ +(X86EMUL_OPC_66(ext, byte) | X86EMUL_OPC_EVEX_) +# define X86EMUL_OPC_EVEX_F3(ext, byte) \ +(X86EMUL_OPC_F3(ext, byte) | X86EMUL_OPC_EVEX_) +# define X86EMUL_OPC_EVEX_F2(ext, byte) \ +(X86EMUL_OPC_F2(ext, byte) | X86EMUL_OPC_EVEX_) >>> Why do we go to the effort of spelling out the individual VEX/EVEX >>> possibilities, but not the XOP ones? >> Because I need some of them right away, but we currently don't >> emulate any XOP insns. If you feel strongly about it, I surely can >> add XOP ones. > > Thats ok - I presume we will be gaining some in due course. Actually I was wrong with the earlier reply - the lack of XOP counterparts is because they wouldn't get encoded this way. Instead they'd use X86EMUL_OPC(0x8fXX, 0xYY). Whether a "shorthand" to make this X86EMUL_OPC_XOP(0xXX, 0xYY) or X86EMUL_OPC_XOP_XX(0xYY) would be worthwhile I'm not sure at this point, so I'd rather leave it out until we actually get to see what's most suitable. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 08/17] x86emul: generate and make use of canonical opcode representation
On 15/09/16 07:43, Jan Beulich wrote: On 14.09.16 at 19:30,wrote: >>> @@ -435,6 +438,51 @@ struct x86_emulate_ctxt >>> void *data; >>> }; >>> >>> +/* >>> + * This encodes the opcode extension in a "natural" way: >> I am not sure what you mean by natural way here. All you seem to mean >> is that you are encoding instructions with the following method > Hence the quotes. Do you have a suggestion for a better word? It doesn't need qualifying at all. It is fine to state simply that this is the representation chosen to be used. The commit message is the better place to make an argument as to why this is a sensible representation, but as this comment is simply a description of the encoding format, the "natural" feels out of place. > >>> +#define X86EMUL_OPC_PFX_MASK 0x0300 >>> +# define X86EMUL_OPC_66(ext, byte) (X86EMUL_OPC(ext, byte) | 0x0100) >>> +# define X86EMUL_OPC_F3(ext, byte) (X86EMUL_OPC(ext, byte) | 0x0200) >>> +# define X86EMUL_OPC_F2(ext, byte) (X86EMUL_OPC(ext, byte) | 0x0300) >> The PFX mask is moderately obvious from here, but a sentence describing >> what is legitimate to add in the future wouldn't go amiss. > I don't understand the "what is legitimate to add in the future" > part: Nothing should be added to this set. It occurs to me that using only 2 bits rather than 8 bits for the prefix information would help the compiler make a smaller switch statements. > >>> + >>> +#define X86EMUL_OPC_KIND_MASK0x3000 >>> +#define X86EMUL_OPC_VEX_ 0x1000 >> OTOH, I am rather more confused about what is eligible for inclusion >> into "kind". Also, what does a kind of 0 indicate? > VEX, XOP, and EVEX are the valid non-zero kinds. Zero (I would > say obviously) means neither of those three. It is not clear how "kind" is a suitable collective term for VEX/XOP/EVEX. Or in other words, X86EMUL_OPC_KIND_MASK doesn't provide any hint that the operation is referring to a legacy or vex encoding of the instruction. Would s/kind/encoding/ be ok? At that point, X86EMUL_OPC_LEGACY_ with a value of 0 might be useful. (e.g. perhaps (opcode & X86EMUL_OPC_ENCODING_MASK) == X86EMUL_OPC_LEGACY_?) > >>> +# define X86EMUL_OPC_VEX(ext, byte) \ >>> +(X86EMUL_OPC(ext, byte) | X86EMUL_OPC_VEX_) >>> +# define X86EMUL_OPC_VEX_66(ext, byte) \ >>> +(X86EMUL_OPC_66(ext, byte) | X86EMUL_OPC_VEX_) >>> +# define X86EMUL_OPC_VEX_F3(ext, byte) \ >>> +(X86EMUL_OPC_F3(ext, byte) | X86EMUL_OPC_VEX_) >>> +# define X86EMUL_OPC_VEX_F2(ext, byte) \ >>> +(X86EMUL_OPC_F2(ext, byte) | X86EMUL_OPC_VEX_) >>> +#define X86EMUL_OPC_EVEX_0x2000 >>> +# define X86EMUL_OPC_EVEX(ext, byte) \ >>> +(X86EMUL_OPC(ext, byte) | X86EMUL_OPC_EVEX_) >>> +# define X86EMUL_OPC_EVEX_66(ext, byte) \ >>> +(X86EMUL_OPC_66(ext, byte) | X86EMUL_OPC_EVEX_) >>> +# define X86EMUL_OPC_EVEX_F3(ext, byte) \ >>> +(X86EMUL_OPC_F3(ext, byte) | X86EMUL_OPC_EVEX_) >>> +# define X86EMUL_OPC_EVEX_F2(ext, byte) \ >>> +(X86EMUL_OPC_F2(ext, byte) | X86EMUL_OPC_EVEX_) >> Why do we go to the effort of spelling out the individual VEX/EVEX >> possibilities, but not the XOP ones? > Because I need some of them right away, but we currently don't > emulate any XOP insns. If you feel strongly about it, I surely can > add XOP ones. Thats ok - I presume we will be gaining some in due course. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 08/17] x86emul: generate and make use of canonical opcode representation
>>> On 14.09.16 at 19:30,wrote: >> @@ -435,6 +438,51 @@ struct x86_emulate_ctxt >> void *data; >> }; >> >> +/* >> + * This encodes the opcode extension in a "natural" way: > > I am not sure what you mean by natural way here. All you seem to mean > is that you are encoding instructions with the following method Hence the quotes. Do you have a suggestion for a better word? >> + *0x0f for 0f-prefixed opcodes (or their VEX/EVEX equivalents) >> + * 0x0f38 for 0f38-prefixed opcodes (or their VEX/EVEX equivalents) >> + * 0x0f3a for 0f3a-prefixed opcodes (or their VEX/EVEX equivalents) >> + * 0x8f08 for 8f/8-prefixed XOP opcodes >> + * 0x8f09 for 8f/9-prefixed XOP opcodes >> + * 0x8f0a for 8f/a-prefixed XOP opcodes >> + * Hence no separate #define-s get added. > > Please also describe what the fields mean. Looking below, I guess > that the bottom byte is the opcode itself, and some bits of the 2nd byte > are legacy prefixes? Yes. Comment extended. >> + */ >> +#define X86EMUL_OPC_EXT_MASK 0x >> +#define X86EMUL_OPC(ext, byte) ((byte) | \ >> + MASK_INSR((ext), > X86EMUL_OPC_EXT_MASK)) > > I would highly suggest using ((byte) & 0xff). In the case that a change > is slightly out of range, this should cause a compiler error (duplicate > case statement) rather than a very subtle bug. Well, okay. >> +/* >> + * This includes the 0x66, 0xF3, and 0xF2 prefixes when used to alter >> + * functionality instead of just insn attributes, as well as VEX/EVEX: >> + */ >> +#define X86EMUL_OPC_MASK (0x00ff | X86EMUL_OPC_PFX_MASK | \ >> + X86EMUL_OPC_KIND_MASK) > > The definition should presumably live after introducing the PFX_MASK and > KIND_MASK ? I would prefer it to live closer to X86EMUL_OPC_EXT_MASK. >> +#define X86EMUL_OPC_PFX_MASK 0x0300 >> +# define X86EMUL_OPC_66(ext, byte) (X86EMUL_OPC(ext, byte) | 0x0100) >> +# define X86EMUL_OPC_F3(ext, byte) (X86EMUL_OPC(ext, byte) | 0x0200) >> +# define X86EMUL_OPC_F2(ext, byte) (X86EMUL_OPC(ext, byte) | 0x0300) > > The PFX mask is moderately obvious from here, but a sentence describing > what is legitimate to add in the future wouldn't go amiss. I don't understand the "what is legitimate to add in the future" part: Nothing should be added to this set. >> + >> +#define X86EMUL_OPC_KIND_MASK0x3000 >> +#define X86EMUL_OPC_VEX_ 0x1000 > > OTOH, I am rather more confused about what is eligible for inclusion > into "kind". Also, what does a kind of 0 indicate? VEX, XOP, and EVEX are the valid non-zero kinds. Zero (I would say obviously) means neither of those three. >> +# define X86EMUL_OPC_VEX(ext, byte) \ >> +(X86EMUL_OPC(ext, byte) | X86EMUL_OPC_VEX_) >> +# define X86EMUL_OPC_VEX_66(ext, byte) \ >> +(X86EMUL_OPC_66(ext, byte) | X86EMUL_OPC_VEX_) >> +# define X86EMUL_OPC_VEX_F3(ext, byte) \ >> +(X86EMUL_OPC_F3(ext, byte) | X86EMUL_OPC_VEX_) >> +# define X86EMUL_OPC_VEX_F2(ext, byte) \ >> +(X86EMUL_OPC_F2(ext, byte) | X86EMUL_OPC_VEX_) >> +#define X86EMUL_OPC_EVEX_0x2000 >> +# define X86EMUL_OPC_EVEX(ext, byte) \ >> +(X86EMUL_OPC(ext, byte) | X86EMUL_OPC_EVEX_) >> +# define X86EMUL_OPC_EVEX_66(ext, byte) \ >> +(X86EMUL_OPC_66(ext, byte) | X86EMUL_OPC_EVEX_) >> +# define X86EMUL_OPC_EVEX_F3(ext, byte) \ >> +(X86EMUL_OPC_F3(ext, byte) | X86EMUL_OPC_EVEX_) >> +# define X86EMUL_OPC_EVEX_F2(ext, byte) \ >> +(X86EMUL_OPC_F2(ext, byte) | X86EMUL_OPC_EVEX_) > > Why do we go to the effort of spelling out the individual VEX/EVEX > possibilities, but not the XOP ones? Because I need some of them right away, but we currently don't emulate any XOP insns. If you feel strongly about it, I surely can add XOP ones. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 08/17] x86emul: generate and make use of canonical opcode representation
On 08/09/16 14:14, Jan Beulich wrote: "of a canonical opcode representation". You appear to be inventing your own here, but it isn't the only canonical form you could represent x86 opcodes with. > --- a/xen/arch/x86/x86_emulate/x86_emulate.h > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > @@ -415,12 +415,15 @@ struct x86_emulate_ctxt > /* Stack pointer width in bits (16, 32 or 64). */ > unsigned int sp_size; > > -/* Set this if writes may have side effects. */ > -uint8_t force_writeback; > +/* Canonical opcode (see below). */ > +unsigned int opcode; > > /* Software event injection support. */ > enum x86_swint_emulation swint_emulate; > > +/* Set this if writes may have side effects. */ > +uint8_t force_writeback; > + > /* Retirement state, set by the emulator (valid only on X86EMUL_OKAY). */ > union { > struct { > @@ -435,6 +438,51 @@ struct x86_emulate_ctxt > void *data; > }; > > +/* > + * This encodes the opcode extension in a "natural" way: I am not sure what you mean by natural way here. All you seem to mean is that you are encoding instructions with the following method > + *0x0f for 0f-prefixed opcodes (or their VEX/EVEX equivalents) > + * 0x0f38 for 0f38-prefixed opcodes (or their VEX/EVEX equivalents) > + * 0x0f3a for 0f3a-prefixed opcodes (or their VEX/EVEX equivalents) > + * 0x8f08 for 8f/8-prefixed XOP opcodes > + * 0x8f09 for 8f/9-prefixed XOP opcodes > + * 0x8f0a for 8f/a-prefixed XOP opcodes > + * Hence no separate #define-s get added. Please also describe what the fields mean. Looking below, I guess that the bottom byte is the opcode itself, and some bits of the 2nd byte are legacy prefixes? > + */ > +#define X86EMUL_OPC_EXT_MASK 0x > +#define X86EMUL_OPC(ext, byte) ((byte) | \ > + MASK_INSR((ext), X86EMUL_OPC_EXT_MASK)) I would highly suggest using ((byte) & 0xff). In the case that a change is slightly out of range, this should cause a compiler error (duplicate case statement) rather than a very subtle bug. > +/* > + * This includes the 0x66, 0xF3, and 0xF2 prefixes when used to alter > + * functionality instead of just insn attributes, as well as VEX/EVEX: > + */ > +#define X86EMUL_OPC_MASK (0x00ff | X86EMUL_OPC_PFX_MASK | \ > + X86EMUL_OPC_KIND_MASK) The definition should presumably live after introducing the PFX_MASK and KIND_MASK ? > + > +#define X86EMUL_OPC_PFX_MASK 0x0300 > +# define X86EMUL_OPC_66(ext, byte) (X86EMUL_OPC(ext, byte) | 0x0100) > +# define X86EMUL_OPC_F3(ext, byte) (X86EMUL_OPC(ext, byte) | 0x0200) > +# define X86EMUL_OPC_F2(ext, byte) (X86EMUL_OPC(ext, byte) | 0x0300) The PFX mask is moderately obvious from here, but a sentence describing what is legitimate to add in the future wouldn't go amiss. > + > +#define X86EMUL_OPC_KIND_MASK0x3000 > +#define X86EMUL_OPC_VEX_ 0x1000 OTOH, I am rather more confused about what is eligible for inclusion into "kind". Also, what does a kind of 0 indicate? > +# define X86EMUL_OPC_VEX(ext, byte) \ > +(X86EMUL_OPC(ext, byte) | X86EMUL_OPC_VEX_) > +# define X86EMUL_OPC_VEX_66(ext, byte) \ > +(X86EMUL_OPC_66(ext, byte) | X86EMUL_OPC_VEX_) > +# define X86EMUL_OPC_VEX_F3(ext, byte) \ > +(X86EMUL_OPC_F3(ext, byte) | X86EMUL_OPC_VEX_) > +# define X86EMUL_OPC_VEX_F2(ext, byte) \ > +(X86EMUL_OPC_F2(ext, byte) | X86EMUL_OPC_VEX_) > +#define X86EMUL_OPC_EVEX_0x2000 > +# define X86EMUL_OPC_EVEX(ext, byte) \ > +(X86EMUL_OPC(ext, byte) | X86EMUL_OPC_EVEX_) > +# define X86EMUL_OPC_EVEX_66(ext, byte) \ > +(X86EMUL_OPC_66(ext, byte) | X86EMUL_OPC_EVEX_) > +# define X86EMUL_OPC_EVEX_F3(ext, byte) \ > +(X86EMUL_OPC_F3(ext, byte) | X86EMUL_OPC_EVEX_) > +# define X86EMUL_OPC_EVEX_F2(ext, byte) \ > +(X86EMUL_OPC_F2(ext, byte) | X86EMUL_OPC_EVEX_) Why do we go to the effort of spelling out the individual VEX/EVEX possibilities, but not the XOP ones? ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 08/17] x86emul: generate and make use of canonical opcode representation
This representation is then being made available to interested callers, to facilitate replacing their custom decoding. This entails combining the three main switch statements into one. Signed-off-by: Jan Beulich--- a/tools/tests/x86_emulator/x86_emulate.c +++ b/tools/tests/x86_emulator/x86_emulate.c @@ -14,6 +14,9 @@ typedef bool bool_t; #define ASSERT assert #define ASSERT_UNREACHABLE() assert(!__LINE__) +#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) +#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) + #define cpu_has_amd_erratum(nr) 0 #define mark_regs_dirty(r) ((void)(r)) --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -1611,7 +1611,6 @@ struct x86_emulate_state { ext_8f09, ext_8f0a, } ext; -uint8_t opcode; uint8_t modrm, modrm_mod, modrm_reg, modrm_rm; uint8_t rex_prefix; bool lock_prefix; @@ -1657,7 +1656,7 @@ x86_decode_base( { int rc = X86EMUL_OKAY; -switch ( state->opcode ) +switch ( ctxt->opcode ) { case 0x9a: /* call (far, absolute) */ case 0xea: /* jmp (far, absolute) */ @@ -1696,11 +1695,9 @@ x86_decode_twobyte( { int rc = X86EMUL_OKAY; -switch ( state->opcode ) +switch ( ctxt->opcode & X86EMUL_OPC_MASK ) { case 0x78: -if ( vex.opcx ) -break; switch ( vex.pfx ) { case vex_66: /* extrq $imm8, $imm8, xmm */ @@ -1709,7 +1706,23 @@ x86_decode_twobyte( imm2 = insn_fetch_type(uint8_t); break; } -break; +/* fall through */ +case 0x10 ... 0x18: +case 0x28 ... 0x2f: +case 0x50 ... 0x77: +case 0x79 ... 0x7f: +case 0xae: +case 0xc2: +case 0xc4 ... 0xc7: +case 0xd0 ... 0xfe: +ctxt->opcode |= MASK_INSR(vex.pfx, X86EMUL_OPC_PFX_MASK); +break; +/* Intentionally not handling here despite being modified by F3: +case 0xb8: jmpe / popcnt +case 0xbc: bsf / tzcnt +case 0xbd: bsr / lzcnt + * They're being dealt with in the execution phase (if at all). + */ } done: @@ -1717,13 +1730,35 @@ x86_decode_twobyte( } static int +x86_decode_0f38( +struct x86_emulate_state *state, +struct x86_emulate_ctxt *ctxt, +const struct x86_emulate_ops *ops) +{ +switch ( ctxt->opcode & X86EMUL_OPC_MASK ) +{ +case 0x00 ... 0xef: +case 0xf2 ... 0xff: +ctxt->opcode |= MASK_INSR(vex.pfx, X86EMUL_OPC_PFX_MASK); +break; + +case 0xf0: case 0xf1: /* movbe / crc32 */ +if ( rep_prefix() ) +ctxt->opcode |= MASK_INSR(vex.pfx, X86EMUL_OPC_PFX_MASK); +break; +} + +return X86EMUL_OKAY; +} + +static int x86_decode( struct x86_emulate_state *state, struct x86_emulate_ctxt *ctxt, const struct x86_emulate_ops *ops) { uint8_t b, d, sib, sib_index, sib_base; -unsigned int def_op_bytes, def_ad_bytes; +unsigned int def_op_bytes, def_ad_bytes, opcode; int rc = X86EMUL_OKAY; memset(state, 0, sizeof(*state)); @@ -1804,29 +1839,31 @@ x86_decode( /* Opcode byte(s). */ d = opcode_table[b]; -if ( d == 0 ) +if ( d == 0 && b == 0x0f) { -/* Two-byte opcode? */ -if ( b == 0x0f ) +/* Two-byte opcode. */ +b = insn_fetch_type(uint8_t); +d = twobyte_table[b]; +switch ( b ) { +default: +opcode = b | MASK_INSR(0x0f, X86EMUL_OPC_EXT_MASK); +ext = ext_0f; +break; +case 0x38: b = insn_fetch_type(uint8_t); -d = twobyte_table[b]; -switch ( b ) -{ -default: -ext = ext_0f; -break; -case 0x38: -b = insn_fetch_type(uint8_t); -ext = ext_0f38; -break; -case 0x3a: -b = insn_fetch_type(uint8_t); -ext = ext_0f3a; -break; -} +opcode = b | MASK_INSR(0x0f38, X86EMUL_OPC_EXT_MASK); +ext = ext_0f38; +break; +case 0x3a: +b = insn_fetch_type(uint8_t); +opcode = b | MASK_INSR(0x0f3a, X86EMUL_OPC_EXT_MASK); +ext = ext_0f3a; +break; } } +else +opcode = b; /* ModRM and SIB bytes. */ if ( d & ModRM ) @@ -1855,6 +1892,7 @@ x86_decode( vex.raw[0] = modrm; if ( b == 0xc5 ) { +opcode = X86EMUL_OPC_VEX_; vex.raw[1] = modrm; vex.opcx = vex_0f; vex.x = 1; @@ -1876,31 +1914,44 @@ x86_decode( op_bytes = 8; } } -if ( b == 0x62 ) +switch ( b ) { +