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

Hi Jan,

Thank you reviewing my patch. I put MOD_VEX_0F38* together. 
Sorry, I didn't notice that there is already a MOD_VEX_0F38* group in the table, 
and introduced this outlier in AMX patch.

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

Done.

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

I didn't find the code we use REX.W to determine the size of register with Gd and Ed.
 Could you help elaborate on it? It seems that Gd and Ed are correct.

OP_G (int bytemode, int sizeflag)
{
    ...
    case d_mode:
    case db_mode:
    case dw_mode:
      oappend (names32[modrm.reg + add]);
      break;

$ cat y.s
.text
        .byte 0xf3, 0x0f, 0x38, 0xfb, 0xd0
$ gcc -m64 -c y.s
$ binary/bin/objdump -dw y.o
Disassembly of section .text:

0000000000000000 <.text>:
   0:   f3 0f 38 fb d0          encodekey256 %eax,%edx


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

Yes, changed it.

> 
> > +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.
> 
Added it.
> 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

IgnoreSize  is redundant here, I deleted them, thanks.

Lili.