Re: [Xen-devel] [PATCH 08/17] x86emul: generate and make use of canonical opcode representation

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.09.16 at 19:30, <andrew.cooper3@xxxxxxxxxx> 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?

>> + *    0x0fxxxx for 0f-prefixed opcodes (or their VEX/EVEX equivalents)
>> + *  0x0f38xxxx for 0f38-prefixed opcodes (or their VEX/EVEX equivalents)
>> + *  0x0f3axxxx for 0f3a-prefixed opcodes (or their VEX/EVEX equivalents)
>> + *  0x8f08xxxx for 8f/8-prefixed XOP opcodes
>> + *  0x8f09xxxx for 8f/9-prefixed XOP opcodes
>> + *  0x8f0axxxx for 8f/a-prefixed XOP opcodes
>> + * Hence no separate #define-s get added.
> 
> Please also describe what the xxxx 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         0xffff0000
>> +#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             (0x000000ff | 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         0x00000300
>> +# define X86EMUL_OPC_66(ext, byte)   (X86EMUL_OPC(ext, byte) | 0x00000100)
>> +# define X86EMUL_OPC_F3(ext, byte)   (X86EMUL_OPC(ext, byte) | 0x00000200)
>> +# define X86EMUL_OPC_F2(ext, byte)   (X86EMUL_OPC(ext, byte) | 0x00000300)
> 
> 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_MASK        0x00003000
>> +#define X86EMUL_OPC_VEX_             0x00001000
> 
> 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_            0x00002000
>> +# 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel