RE: [PATCH V2 3/8] Support APX GPR32 with extend evex prefix

["Cui, Lili" <lili.cui@intel.com>]

> > --- a/opcodes/i386-dis-evex-mod.h
> > +++ b/opcodes/i386-dis-evex-mod.h
> > @@ -1 +1,43 @@
> >  /* Nothing at present.  */
> 
> This comment needs to go away when new stuff is added here. However, I'm
> sure I requested before that no new entries be put here which have only one
> of their two slots populated. The reg-only and mem-only aspects can be
> expressed via proper choice of operand specifiers, at least in almost all cases.
> Note how you already use ...
> 
> > +  /* MOD_EVEX_MAP4_DA_PREFIX_1 */
> > +  {
> > +    { Bad_Opcode },
> > +    { "encodekey128", { Gd, Ed }, 0 },  },
> > +  /* MOD_EVEX_MAP4_DB_PREFIX_1 */
> > +  {
> > +    { Bad_Opcode },
> > +    { "encodekey256", { Gd, Ed }, 0 },  },
> > +  /* MOD_EVEX_MAP4_DC_PREFIX_1 */
> > +  {
> > +    { "aesenc128kl",    { XM, M }, 0 },
> > +  },
> > +  /* MOD_EVEX_MAP4_DD_PREFIX_1 */
> > +  {
> > +    { "aesdec128kl",    { XM, M }, 0 },
> > +  },
> > +  /* MOD_EVEX_MAP4_DE_PREFIX_1 */
> > +  {
> > +    { "aesenc256kl",    { XM, M }, 0 },
> > +  },
> > +  /* MOD_EVEX_MAP4_DF_PREFIX_1 */
> > +  {
> > +    { "aesdec256kl",    { XM, M }, 0 },
> > +  },
> > +  /* MOD_EVEX_MAP4_F8_PREFIX_1 */
> > +  {
> > +    { "enqcmds",	{ Gva, M },  0 },
> > +  },
> > +  /* MOD_EVEX_MAP4_F8_PREFIX_2 */
> > +  {
> > +    { "movdir64b",	{ Gva, M }, 0 },
> > +  },
> > +  /* MOD_EVEX_MAP4_F8_PREFIX_3 */
> > +  {
> > +    { "enqcmd",		{ Gva, M }, 0 },
> > +  },
> 
> ... M in many entries anyway. These can move one level up without needing
> further adjustment.

Done.

> +  /* PREFIX_EVEX_MAP4_F8 */
> +  {
> +    { Bad_Opcode },
> +    { MOD_TABLE (MOD_EVEX_MAP4_F8_PREFIX_1) },
> +    { MOD_TABLE (MOD_EVEX_MAP4_F8_PREFIX_2) },
> +    { MOD_TABLE (MOD_EVEX_MAP4_F8_PREFIX_3) },  },
> +  /* PREFIX_EVEX_MAP4_FC */
> +  {
> +    { "aadd",	{ Mdq, Gdq }, 0 },
> +    { "axor",	{ Mdq, Gdq }, 0 },
> +    { "aand",	{ Mdq, Gdq }, 0 },
> +    { "aor",	{ Mdq, Gdq }, 0 },
> +  },

>This looks to match the PREFIX_0F38FC entry.

Done.

> > --- a/opcodes/i386-dis-evex-reg.h
> > +++ b/opcodes/i386-dis-evex-reg.h
> > @@ -49,3 +49,17 @@
> >      { "vscatterpf0qp%XW",  { MVexVSIBQWpX }, PREFIX_DATA },
> >      { "vscatterpf1qp%XW",  { MVexVSIBQWpX }, PREFIX_DATA },
> >    },
> > +  /* REG_EVEX_0F38F3_L_0 */
> > +  {
> > +    { Bad_Opcode },
> > +    { "blsrS",		{ VexGdq, Edq }, 0 },
> > +    { "blsmskS",	{ VexGdq, Edq }, 0 },
> > +    { "blsiS",		{ VexGdq, Edq }, 0 },
> > +  },
> 
> Compared to the REG_VEX_0F38F3_L_0 entry this lacks PREFIX_OPCODE, but
> then the question is why you do not re-use that entry in the first place.
> 

Added PREFIX_TABLE for them.

> > +  /* REG_EVEX_MAP4_D8_PREFIX_1 */
> > +  {
> > +    { "aesencwide128kl",	{ M }, 0 },
> > +    { "aesdecwide128kl",	{ M }, 0 },
> > +    { "aesencwide256kl",	{ M }, 0 },
> > +    { "aesdecwide256kl",	{ M }, 0 },
> > +  },
> 
> How is this different from the REG_0F38D8_PREFIX_1 entry?
> 

They are same, deleted REG_EVEX_MAP4_D8_PREFIX_1 and share REG_0F38D8_PREFIX_1.

> > --- /dev/null
> > +++ b/opcodes/i386-dis-evex-x86-64.h
> > @@ -0,0 +1,140 @@
> > +  /* X86_64_EVEX_0F3849 */
> > +  {
> > +    { Bad_Opcode },
> > +    { VEX_LEN_TABLE (VEX_LEN_0F3849_X86_64) },  },
> > +  /* X86_64_EVEX_0F384B */
> > +  {
> > +    { Bad_Opcode },
> > +    { VEX_LEN_TABLE (VEX_LEN_0F384B_X86_64) },  },
> > +  /* X86_64_EVEX_0F38E0 */
> > +  {
> > +    { Bad_Opcode },
> > +    { "cmpoxadd", { Mdq, Gdq, VexGdq }, PREFIX_DATA },  },
> 
> This and its sibling entries look to again fully match X86_64_VEX_0F38E<n>.

It cannot be moved to up level, it needs to go through i386-dis-evex-x86-64.h

> > --- a/opcodes/i386-dis-evex.h
> > +++ b/opcodes/i386-dis-evex.h
> >[...]
> > @@ -1113,19 +1113,19 @@ static const struct dis386 evex_table[][256] = {
> >      { Bad_Opcode },
> >      { Bad_Opcode },
> >      { Bad_Opcode },
> > -    { Bad_Opcode },
> > +    { "sha1rnds4", { XM, EXxmm, Ib }, 0 },
> 
> PREFIX_OPCODE? EVEX.W?
> 

Added PREFIX_TABLE , I think this instruction doesn't need to go through W_TABLE.

EVEX.LLZ.NP.MAP4. D4 /r ib
SHA1RNDS4 {NF=0} {ND=0} xmm1, xmm2/m128, imm8

> >      { Bad_Opcode },
> >      { Bad_Opcode },
> >      { Bad_Opcode },
> >      /* D8 */
> > -    { Bad_Opcode },
> > -    { Bad_Opcode },
> > -    { Bad_Opcode },
> > -    { Bad_Opcode },
> > -    { Bad_Opcode },
> > -    { Bad_Opcode },
> > -    { Bad_Opcode },
> > -    { Bad_Opcode },
> > +    { PREFIX_TABLE (PREFIX_EVEX_MAP4_D8) },
> > +    { "sha1msg1", { XM, EXxmm }, 0 },
> 
> Again.
> 

Done.

> > --- a/opcodes/i386-dis.c
> > +++ b/opcodes/i386-dis.c
> >[...]
> > @@ -4522,12 +4594,13 @@ static const struct dis386 x86_64_table[][2] = {
> >      { "cmpnlexadd", { Mdq, Gdq, VexGdq }, PREFIX_DATA },
> >    },
> >
> > +#include "i386-dis-evex-x86-64.h"
> > +
> >    /* X86_64_VEX_MAP7_F8_L_0_W_0_R_0 */
> >    {
> >      { Bad_Opcode },
> >      { PREFIX_TABLE (PREFIX_VEX_MAP7_F8_L_0_W_0_R_0_X86_64) },
> >    },
> > -
> >  };
> 
> Please can EVEX entries come after VEX ones?
> 

Done.

> > @@ -8979,9 +9055,13 @@ get_valid_dis386 (const struct dis386 *dp,
> instr_info *ins)
> >        if (!fetch_code (ins->info, ins->codep + 4))
> >  	return &err_opcode;
> >        /* The first byte after 0x62.  */
> > +      if (*ins->codep & 0x8)
> > +	ins->rex2 |= REX_B;
> > +      if (!(*ins->codep & 0x10))
> > +	ins->rex2 |= REX_R;
> 
> In patch 1 you check whether REX2 bits are properly consumed. While in
> principle I agree with re-using ->rex2 here, don't you need to take precautions
> to not print a random {rex2} (or whichever way it was
> expressed) when one of these bits ends up not contributing to any operand?
> 

We have discussed this issue in other email.

> > @@ -8994,12 +9074,19 @@ get_valid_dis386 (const struct dis386 *dp,
> instr_info *ins)
> >  	case 0x3:
> >  	  vex_table_index = EVEX_0F3A;
> >  	  break;
> > +	case 0x4:
> > +	  vex_table_index = EVEX_MAP4;
> > +	  ins->evex_type = evex_from_legacy;
> > +	  break;
> >  	case 0x5:
> >  	  vex_table_index = EVEX_MAP5;
> >  	  break;
> >  	case 0x6:
> >  	  vex_table_index = EVEX_MAP6;
> >  	  break;
> > +	case 0x7:
> > +	  vex_table_index = EVEX_MAP7;
> > +	  break;
> >  	}
> 
> How is map7 coming into play here?
> 

Removed.

> > @@ -9042,12 +9128,31 @@ get_valid_dis386 (const struct dis386 *dp,
> > instr_info *ins)
> >
> >        if (ins->address_mode != mode_64bit)
> >  	{
> > +	  if (ins->evex_type != evex_default
> > +	      || (ins->rex2 & (REX_B | REX_X)))
> > +	    return &bad_opcode;
> >  	  /* In 16/32-bit mode silently ignore following bits.  */
> >  	  ins->rex &= ~REX_B;
> > -	  ins->vex.r = true;
> > +	  ins->rex2 &= ~REX_R;
> >  	}
> >
> >        ins->need_vex = 4;
> > +
> > +      /* EVEX from legacy instructions require that EVEX.L'L, EVEX.z and the
> > +	 lower 2 bits of EVEX.aaa must be 0.
> > +	 EVEX from evex instrucions require that EVEX.L'L and the lower 2 bits
> of
> > +	 EVEX.aaa must be 0.  */
> 
> EVEX from VEX you mean? Also, what about EVEX.z for those? Really the sole
> difference between the two forms is EVEX.nd. The doc mentions EVEX.l for
> the from-VEX case, but afaics there's no insn actually having that bit set
> (which also matches what you say above).
> 

EVEX.z  is "ins->vex.zeroing = *ins->codep & 0x80;" The old code was wrong , changed it to:

     case USE_X86_64_EVEX_FROM_VEX_TABLE:
       ins->evex_type = evex_from_vex;
+      /* EVEX from evex instrucions require that EVEX.z, EVEX.L’L, EVEX.b and
+        the lower 2 bits of EVEX.aaa must be 0.  */
+      if ((ins->vex.mask_register_specifier & 0x3) != 0
+         || ins->vex.ll != 0
+         || ins->vex.zeroing != 0
+         || ins->vex.b)
+       return &bad_opcode;

     case USE_EVEX_TABLE:
+      /* EVEX from legacy instructions require that EVEX.z, EVEX.L’L and the
+        lower 2 bits of EVEX.aaa must be 0.  */
+      if (ins->evex_type == evex_from_legacy
+         && ((ins->vex.mask_register_specifier & 0x3) != 0
+             || ins->vex.ll != 0
+             || ins->vex.zeroing != 0))
+       return &bad_opcode;


> > +      if (ins->evex_type == evex_from_legacy || ins->evex_type ==
> evex_from_vex)
> > +	{
> > +	  if ((*ins->codep & 0x3) != 0
> > +	      || (*ins->codep >> 6 & 0x3) != 0
> > +	      || (ins->evex_type == evex_from_legacy
> > +		  && (*ins->codep >> 5 & 0x1) != 0)
> 
> Please can the shifts be parenthesized agaist the &-s? Albeit - this is perhaps
> easier to read as just &-s anyway.
> 

 Code changed.

> > +	      || (ins->evex_type == evex_from_vex
> > +		  && !ins->vex.b))
> 
> This doesn't look to match any part of the comment.
> 

Code changed and added a test case for it.

> > @@ -9640,6 +9752,19 @@ print_insn (bfd_vma pc, disassemble_info *info,
> int intel_syntax)
> >        if (ins.last_repnz_prefix >= 0)
> >  	ins.all_prefixes[ins.last_repnz_prefix] = 0xf2;
> >        break;
> > +
> > +    case PREFIX_NP_OR_DATA:
> > +      if (ins.vex.prefix & ~DATA_PREFIX_OPCODE)
> > +	{
> > +	  i386_dis_printf (info, dis_style_text, "(bad)");
> > +	  ret = ins.end_codep - priv.the_buffer;
> > +	  goto out;
> > +	}
> > +      break;
> > +
> > +    default:
> > +      break;
> > +
> >      }
> 
> Why suddenly a default case? And why an extra blank line below it?
> 

Removed.

> > @@ -11242,7 +11367,7 @@ print_register (instr_info *ins, unsigned int reg,
> unsigned int rexmask,
           case b_mode:
> >      case b_swap_mode:
> >        if (reg & 4)
> >  	USED_REX (0);
> > -      if (ins->rex)
> > +      if (ins->rex || ins->rex2)
> >  	names = att_names8rex;
> >        else
> >  	names = att_names8;
> 
> Isn't this already needed in the REX2 patch?
> 

CRC32  --> Eb ---> b_mode

> > @@ -11460,7 +11585,7 @@ OP_E_memory (instr_info *ins, int bytemode,
> > int sizeflag)
> >
> >    add += (ins->rex2 & REX_B) ? 16 : 0;
> >
> > -  if (ins->vex.evex)
> > +  if (ins->vex.evex && ins->evex_type == evex_default)
> >      {
> >
> >        /* Zeroing-masking is invalid for memory destinations. Set the
> > flag @@ -11608,6 +11733,13 @@ OP_E_memory (instr_info *ins, int
> bytemode, int sizeflag)
> >  		abort ();
> >  	      if (ins->vex.evex)
> >  		{
> > +		  /* S/G EVEX insns require REX_X not to be set.  */
> > +		  if (ins->rex2 & REX_X)
> 
> REX_X in a comment can only mean REX.X. You mean EVEX.X4 though, I think
> (which internally is latched into the REX_X bit of ->rex2), and that also needs
> to be set, not clear.
> 
Done.

Thanks,
Lili.