[PATCH 1/8] Support APX GPR32 with rex2 prefix

[Cui, Lili]

> Subject: Re: [PATCH 1/8] Support APX GPR32 with rex2 prefix
> 
> On 02.11.2023 12:29, Cui, Lili wrote:
> > @@ -269,9 +275,17 @@ struct dis_private {
> >        ins->rex_used |= REX_OPCODE;			\
> >    }
> >
> > +#define USED_REX2(value)				\
> > +  {							\
> > +    if ((ins->rex2 & value))				\
> > +      ins->rex2_used |= value;				\
> > +  }
> > +
> >
> >  #define EVEX_b_used 1
> 
> Nit: Please avoid (re)introducing double blank lines. Instead ...
> 

Done.

> >  #define EVEX_len_used 2
> > +/* M0 in rex2 prefix represents map0 or map1.  */ #define REX2_M 0x8
> 
> ... a blank line ahead of this insertion would be helpful.
> 

Done.

> > @@ -1872,23 +1888,23 @@ static const struct dis386 dis386[] = {
> > +  { "scasB",		{ AL, Yb }, PREFIX_REX2_ILLEGAL },
> > +  { "scasS",		{ eAX, Yv }, PREFIX_REX2_ILLEGAL },
> >    /* b0 */
> >    { "movB",		{ RMAL, Ib }, 0 },
> >    { "movB",		{ RMCL, Ib }, 0 },
> 
> Like in the i386-gen.c adjustments for row E look to be missing here, too.
> 

Added them, and also added bad testcase for row E.

> > @@ -2091,12 +2107,12 @@ static const struct dis386 dis386_twobyte[] = {
> >    { PREFIX_TABLE (PREFIX_0F2E) },
> >    { PREFIX_TABLE (PREFIX_0F2F) },
> >    /* 30 */
> > -  { "wrmsr",		{ XX }, 0 },
> > -  { "rdtsc",		{ XX }, 0 },
> > -  { "rdmsr",		{ XX }, 0 },
> > -  { "rdpmc",		{ XX }, 0 },
> > -  { "sysenter",		{ SEP }, 0 },
> > -  { "sysexit%LQ",	{ SEP }, 0 },
> > +  { "wrmsr",		{ XX }, PREFIX_REX2_ILLEGAL },
> > +  { "rdtsc",		{ XX }, PREFIX_REX2_ILLEGAL },
> > +  { "rdmsr",		{ XX }, PREFIX_REX2_ILLEGAL },
> > +  { "rdpmc",		{ XX }, PREFIX_REX2_ILLEGAL },
> > +  { "sysenter",		{ SEP }, PREFIX_REX2_ILLEGAL },
> > +  { "sysexit%LQ",	{ SEP }, PREFIX_REX2_ILLEGAL },
> >    { Bad_Opcode },
> >    { "getsec",		{ XX }, 0 },
> >    /* 38 */
> 
> Down from here row 8 also wants adjustment afaict.
> 

Done.

> > @@ -8289,6 +8313,7 @@ ckprefix (instr_info *ins)  {
> >    int i, length;
> >    uint8_t newrex;
> > +  unsigned char rex2_payload;
> 
> Please can this be restricted to the inner scope where it's used?
> 

Done.

> > @@ -9292,13 +9338,17 @@ print_insn (bfd_vma pc, disassemble_info
> *info, int intel_syntax)
> >        goto out;
> >      }
> >
> > -  if (*ins.codep == 0x0f)
> > +  /* M0 in rex2 prefix represents map0 or map1.  */  if (*ins.codep
> > + == 0x0f || (ins.rex2 & REX2_M))
> 
> I'm struggling with the M0 in the comment. DYM just M, or maybe REX2.M?
> 

Changed to REX2.M.

> Also is this, ...
> 
> >      {
> >        unsigned char threebyte;
> >
> > -      ins.codep++;
> > -      if (!fetch_code (info, ins.codep + 1))
> > -	goto fetch_error_out;
> > +      if (!ins.rex2)
> > +	{
> > +	  ins.codep++;
> > +	  if (!fetch_code (info, ins.codep + 1))
> > +	    goto fetch_error_out;
> > +	}
> >        threebyte = *ins.codep;
> >        dp = &dis386_twobyte[threebyte];
> >        ins.need_modrm = twobyte_has_modrm[threebyte];
> 
> ... all the way to here, really correct for d5 00 0f?
> 

I think the 0f here must indicate that it is the first byte of the legacy map1 instruction, meaning legacy map0 does not have 0f opcode. If this instruction has a rex2 prefix, rex2.w must be 1 and should be d5 80. If a bad binary does appear, our original code also has the same issue.

static const struct dis386 dis386[] = {
...
/ * 0f  */
{ Bad_Opcode },       /* 0x0f extended opcode escape */

> > @@ -9454,6 +9504,14 @@ print_insn (bfd_vma pc, disassemble_info *info,
> int intel_syntax)
> >        goto out;
> >      }
> >
> > +  if ((dp->prefix_requirement & PREFIX_REX2_ILLEGAL)
> > +      && ins.last_rex2_prefix >= 0)
> > +    {
> > +      i386_dis_printf (info, dis_style_text, "(bad)");
> > +      ret = ins.end_codep - priv.the_buffer;
> > +      goto out;
> > +    }
> > +
> >    switch (dp->prefix_requirement)
> >      {
> >      case PREFIX_DATA:
> > @@ -9468,6 +9526,7 @@ print_insn (bfd_vma pc, disassemble_info *info,
> int intel_syntax)
> >        ins.used_prefixes |= PREFIX_DATA;
> >        /* Fall through.  */
> >      case PREFIX_OPCODE:
> > +    case PREFIX_OPCODE | PREFIX_REX2_ILLEGAL:
> 
> May more robust to mask off PREFIX_REX2_ILLEGAL in the control expression
> of the switch()? Or else why don't you move the if() immediately ahead of the
> switch() into here, as a new case block?
> 

Changed it to

+  switch (dp->prefix_requirement & ~PREFIX_REX2_ILLEGAL)
     {
     case PREFIX_DATA:
       /* If only the data prefix is marked as mandatory, its absence renders
@@ -9600,7 +9599,7 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
       ins.used_prefixes |= PREFIX_DATA;
       /* Fall through.  */
     case PREFIX_OPCODE:
-    case PREFIX_OPCODE | PREFIX_REX2_ILLEGAL:


> > @@ -9513,6 +9572,13 @@ print_insn (bfd_vma pc, disassemble_info *info,
> int intel_syntax)
> >        && !ins.need_vex && ins.last_rex_prefix >= 0)
> >      ins.all_prefixes[ins.last_rex_prefix] = 0;
> >
> > +  /* Check if the REX2 prefix is used.  */
> > +  if (ins.last_rex2_prefix >= 0
> > +      && ((((ins.rex2 & 0x7) ^ (ins.rex2_used & 0x7)) == 0
> > +	   && (ins.rex2 & 0x7))
> 
> DYM ((ins.rex2 & 7) & ~(ins.rex2_used & 7)) != 0
>

Here's an example of a negative scenario, when ins.rex2 == 1 and ins.rex2_used == 1, we want to clear last_rex2_prefix, because it has egpr and we don't want to add {rex2} to it. 

> > +	  || dp == &bad_opcode))
> 
> What is this last part of the condition about? Other prefix zapping code
> doesn't have such.

Deleted it , there is no impact on current testcase. Don’t know what the author’s intention was at that time. Deleted it.

> 
> > +    ins.all_prefixes[ins.last_rex2_prefix] = 0;
> > +
> >    /* Check if the SEG prefix is used.  */
> >    if ((ins.prefixes & (PREFIX_CS | PREFIX_SS | PREFIX_DS | PREFIX_ES
> >  		       | PREFIX_FS | PREFIX_GS)) != 0 @@ -9541,7 +9607,10
> @@
> > print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
> >  	if (name == NULL)
> >  	  abort ();
> >  	prefix_length += strlen (name) + 1;
> > -	i386_dis_printf (info, dis_style_mnemonic, "%s ", name);
> > +	if (ins.all_prefixes[i] == REX2_OPCODE)
> > +	  i386_dis_printf (info, dis_style_mnemonic, "{%s} ", name);
> 
> Do braces really count as part of the mnemonic?
> 

Yes, rex2 prefix prefers to use mnemonic {rex2}, unlike rex prefix use rex, rex.B....

> > +	else
> > +	  i386_dis_printf (info, dis_style_mnemonic, "%s ", name);
> >        }
> 
> Aren't you at risk of wrongly printing a REX prefix here if the high 4 bits of the
> REX2 payload were all zero, but some of the low 4 bits turned out unused?
> 
  For d500 I think we should print rex2 for it. 

> > @@ -11086,8 +11155,11 @@ print_register (instr_info *ins, unsigned int
> reg, unsigned int rexmask,
> >      ins->illegal_masking = true;
> >
> >    USED_REX (rexmask);
> > +  USED_REX2 (rexmask);
> 
> Do both really need tracking separately? Whatever consumes REX.B will also
> consume REX2.B4, an so on.
> 
I was confused here, I think we only need to print {rex2} for the upper 4 bits == *000, which means egpr is not used and we need to use {rex2} to distinguish it from legacy encoding.  maybe we don’t need ((ins.rex2 & 0x7) ^ (ins.rex2_used & 0x7)) == 0, and nor USED_REX2 (rexmask). I intend to delete them.

+  /* Check if the REX2 prefix is used.  */
+  if (ins.last_rex2_prefix >= 0
+      && ((((ins.rex2 & 0x7) ^ (ins.rex2_used & 0x7)) == 0
+	   && (ins.rex2 & 0x7))

Thanks,
Lili