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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/09/16 07:43, Jan Beulich wrote:
>>>> 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?

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

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

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

Thats ok - I presume we will be gaining some in due course.

~Andrew

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