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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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

> + *    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?

> + */
> +#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.

> +/*
> + * 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 ?

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

> +
> +#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?

> +# 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?

~Andrew

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