Re: Enable support to Intel Key locker instructions.

[Jan Beulich <jbeulich@suse.com>]

On 21.09.2020 05:25, Cui, Lili wrote:
> --- a/opcodes/i386-dis.c
> +++ b/opcodes/i386-dis.c
> @@ -691,6 +691,7 @@ enum
>    REG_0F18,
>    REG_0F1C_P_0_MOD_0,
>    REG_0F1E_P_1_MOD_3,
> +  REG_0F38D8_PREFIX_1,
>    REG_0F71,
>    REG_0F72,
>    REG_0F73,

This addition wants to go further down. While not immediately
visible here, ...

> @@ -797,12 +798,18 @@ enum
>    MOD_VEX_0F385E_X86_64_P_1_W_0,
>    MOD_VEX_0F385E_X86_64_P_2_W_0,
>    MOD_VEX_0F385E_X86_64_P_3_W_0,
> +  MOD_0F38DC_PREFIX_1,
> +  MOD_0F38DD_PREFIX_1,
> +  MOD_0F38DE_PREFIX_1,
> +  MOD_0F38DF_PREFIX_1,
>    MOD_0F38F5,
>    MOD_0F38F6_PREFIX_0,
>    MOD_0F38F8_PREFIX_1,
>    MOD_0F38F8_PREFIX_2,
>    MOD_0F38F8_PREFIX_3,
>    MOD_0F38F9,
> +  MOD_0F38FA_PREFIX_1,
> +  MOD_0F38FB_PREFIX_1,
>    MOD_62_32BIT,
>    MOD_C4_32BIT,
>    MOD_C5_32BIT,

... in this table you'll notice that MOD_0F38* all go together,
and _later_ there's a MOD_VEX_0F38* group. I notice that recent
additions (of yours?) also already violate this sorting model -
please may I ask for this to corrected as well? The more
outliers we have there, the more difficult will it be to maintain
this code.

> @@ -2890,6 +2904,13 @@ static const struct dis386 reg_table[][8] = {
>      { "nopQ",		{ Ev }, 0 },
>      { RM_TABLE (RM_0F1E_P_1_MOD_3_REG_7) },
>    },
> +  /* REG_0F38D8_PREFIX_1 */
> +  {
> +    { "aesencwide128kl",	{ M }, 0 },
> +    { "aesdecwide128kl",	{ M }, 0 },
> +    { "aesencwide256kl",	{ M }, 0 },
> +    { "aesdecwide256kl",	{ M }, 0 },
> +  },

As you properly omit PREFIX_OPCODE here, ...

> @@ -3543,6 +3564,40 @@ static const struct dis386 prefix_table[][4] = {
>      { "maskmovdqu", { XM, XS }, PREFIX_OPCODE },
>    },
>  
> +  /* PREFIX_0F38D8 */
> +  {
> +    { Bad_Opcode },
> +    { REG_TABLE (REG_0F38D8_PREFIX_1) },
> +  },
> +
> +  /* PREFIX_0F38DC */
> +  {
> +    { Bad_Opcode },
> +    { MOD_TABLE (MOD_0F38DC_PREFIX_1) },
> +    { "aesenc", { XM, EXx }, PREFIX_OPCODE },

... I wonder why you add it here (and several more times below).
It's generally pointless to have this attribute when in
prefix_table[] or already having gone through it. There are many
pointless uses left, yes, but they're slowly being eliminated.

> @@ -8236,6 +8319,16 @@ static const struct dis386 mod_table[][2] = {
>      /* MOD_0F38F9 */
>      { "movdiri",	{ Edq, Gdq }, PREFIX_OPCODE },
>    },
> +  {
> +    /* MOD_0F38FA_PREFIX_1 */
> +    { Bad_Opcode },
> +    { "encodekey128", { Gd, Ed }, PREFIX_OPCODE },
> +  },
> +  {
> +    /* MOD_0F38FB_PREFIX_1 */
> +    { Bad_Opcode },
> +    { "encodekey256", { Gd, Ed }, PREFIX_OPCODE },
> +  },

The use of Gd and Ed will, afaict, lead to REX.W decoding as 64-bit
register operands, which according to doc and gas implementation
looks wrong.

> --- a/opcodes/i386-opc.h
> +++ b/opcodes/i386-opc.h
> @@ -259,6 +259,10 @@ enum
>    CpuSEV_ES,
>    /* TSXLDTRK instruction required */
>    CpuTSXLDTRK,
> +  /* KL instruction support required */
> +  CpuKL,
> +  /* WIDEKL instruction support required */
> +  CpuWIDEKL,

May I suggest CpuWideKL? I see no need for the excess capitalization.

> --- a/opcodes/i386-opc.tbl
> +++ b/opcodes/i386-opc.tbl
> @@ -4117,3 +4117,19 @@ tilerelease, 0, 0x49c0, None, 2, CpuAMX_TILE|Cpu64, Vex128|VexOpcode=1|VexW0|No_
>  tilezero, 1, 0xf249, None, 1, CpuAMX_TILE|Cpu64, Modrm|Vex128|VexOpcode=1|VexW0|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, { RegTMM }
>  
>  // AMX instructions end.
> +
> +// KEYLOCKER instructions.
> +
> +loadiwkey, 2, 0xf30f38dc, None, 3, CpuKL, Load|Modrm|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf|NoRex64, { RegXMM, RegXMM }
> +encodekey128, 2, 0xf30f38fa, None, 3, CpuKL, Modrm|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf|NoRex64, { Reg32, Reg32 }
> +encodekey256, 2, 0xf30f38fb, None, 3, CpuKL, Modrm|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf|NoRex64, { Reg32, Reg32 }

These two and ...

> +aesenc128kl, 2, 0xf30f38dc, None, 3, CpuKL, Modrm|IgnoreSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, { Unspecified|BaseIndex, RegXMM }
> +aesdec128kl, 2, 0xf30f38dd, None, 3, CpuKL, Modrm|IgnoreSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, { Unspecified|BaseIndex, RegXMM }
> +aesenc256kl, 2, 0xf30f38de, None, 3, CpuKL, Modrm|IgnoreSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, { Unspecified|BaseIndex, RegXMM }
> +aesdec256kl, 2, 0xf30f38df, None, 3, CpuKL, Modrm|IgnoreSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, { Unspecified|BaseIndex, RegXMM }
> +aesencwide128kl, 1, 0xf30f38d8, 0x0, 3, CpuWIDEKL, Modrm|IgnoreSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, { Unspecified|BaseIndex }
> +aesdecwide128kl, 1, 0xf30f38d8, 0x1, 3, CpuWIDEKL, Modrm|IgnoreSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, { Unspecified|BaseIndex }
> +aesencwide256kl, 1, 0xf30f38d8, 0x2, 3, CpuWIDEKL, Modrm|IgnoreSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, { Unspecified|BaseIndex }
> +aesdecwide256kl, 1, 0xf30f38d8, 0x3, 3, CpuWIDEKL, Modrm|IgnoreSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, { Unspecified|BaseIndex }

... these four need special treatment in output_insn()'s
setting of GNU_PROPERTY_X86_FEATURE_2_XMM, due to the lack of
explicit RegXMM operands.

I'm also not seeing why you add IgnoreSize in a number of
cases. Could you remind me what would break without it? We're
trying to eliminate stray uses, after all, so new ones would
better not appear.

Jan