Re: [PATCH v4 2/9] Support APX GPR32 with rex2 prefix

[Jan Beulich <jbeulich@suse.com>]

On 07.12.2023 09:53, Cui, Lili wrote:
> --- a/opcodes/i386-dis.c
> +++ b/opcodes/i386-dis.c
> @@ -144,6 +144,11 @@ struct instr_info
>    /* Bits of REX we've already used.  */
>    uint8_t rex_used;
>  
> +  /* REX2 prefix for the current instruction use gpr32(r16-r31). */

What is "use gpr32(r16-r31)" describing here? The prefix covers more
than just register use, after all, and even the four bits you actually
store here (it's not the full prefix, which may better be clarified)
use cover more than just register use.

> @@ -272,10 +278,18 @@ struct dis_private {
>        ins->rex_used |= REX_OPCODE;			\
>    }
>  
> +#define USED_REX2(value)				\
> +  {							\
> +    if ((ins->rex2 & value))				\

Nit: Excess parentheses. Yet as before - I don't understand why this is
needed in addition to USED_REX(). USED_REX() could simply perform both
functions, and then you wouldn't need to alter all use sites.

> +      ins->rex2_used |= value;				\
> +  }
>  
>  #define EVEX_b_used 1
>  #define EVEX_len_used 2
>  
> +/* M0 in rex2 prefix represents map0 or map1.  */
> +#define REX2_M 0x8

Btw, the other patch adding REX2_REQUIRED will want to add that next to
this #define.

> @@ -2406,22 +2422,30 @@ static const char intel_index16[][6] = {
>  
>  static const char att_names64[][8] = {
>    "%rax", "%rcx", "%rdx", "%rbx", "%rsp", "%rbp", "%rsi", "%rdi",
> -  "%r8", "%r9", "%r10", "%r11", "%r12", "%r13", "%r14", "%r15"
> +  "%r8", "%r9", "%r10", "%r11", "%r12", "%r13", "%r14", "%r15",
> +  "%r16", "%r17", "%r18", "%r19", "%r20", "%r21", "%r22", "%r23",
> +  "%r24", "%r25", "%r26", "%r27", "%r28", "%r29", "%r30", "%r31"
>  };
>  static const char att_names32[][8] = {
>    "%eax", "%ecx", "%edx", "%ebx", "%esp", "%ebp", "%esi", "%edi",
> -  "%r8d", "%r9d", "%r10d", "%r11d", "%r12d", "%r13d", "%r14d", "%r15d"
> +  "%r8d", "%r9d", "%r10d", "%r11d", "%r12d", "%r13d", "%r14d", "%r15d",
> +  "%r16d", "%r17d", "%r18d", "%r19d", "%r20d", "%r21d", "%r22d", "%r23d",
> +  "%r24d", "%r25d", "%r26d", "%r27d", "%r28d", "%r29d", "%r30d", "%r31d"
>  };
>  static const char att_names16[][8] = {
>    "%ax", "%cx", "%dx", "%bx", "%sp", "%bp", "%si", "%di",
> -  "%r8w", "%r9w", "%r10w", "%r11w", "%r12w", "%r13w", "%r14w", "%r15w"
> +  "%r8w", "%r9w", "%r10w", "%r11w", "%r12w", "%r13w", "%r14w", "%r15w",
> +  "%r16w", "%r17w", "%r18w", "%r19w", "%r20w", "%r21w", "%r22w", "%r23w",
> +  "%r24w", "%r25w", "%r26w", "%r27w", "%r28w", "%r29w", "%r30w", "%r31w"
>  };
>  static const char att_names8[][8] = {
>    "%al", "%cl", "%dl", "%bl", "%ah", "%ch", "%dh", "%bh",
>  };
>  static const char att_names8rex[][8] = {
>    "%al", "%cl", "%dl", "%bl", "%spl", "%bpl", "%sil", "%dil",
> -  "%r8b", "%r9b", "%r10b", "%r11b", "%r12b", "%r13b", "%r14b", "%r15b"
> +  "%r8b", "%r9b", "%r10b", "%r11b", "%r12b", "%r13b", "%r14b", "%r15b",
> +  "%r16b", "%r17b", "%r18b", "%r19b", "%r20b", "%r21b", "%r22b", "%r23b",
> +  "%r24b", "%r25b", "%r26b", "%r27b", "%r28b", "%r29b", "%r30b", "%r31b"
>  };

As you can see from the patch delta, it is good practice to have trailing
commas in such initializers.

> @@ -8387,6 +8411,24 @@ ckprefix (instr_info *ins)
>  	    return ckp_okay;
>  	  ins->last_rex_prefix = i;
>  	  break;
> +	/* REX2 must be the last prefix. */
> +	case 0xd5:

Why not use REX2_OPCODE here, just like ...

> +	  if (ins->address_mode == mode_64bit)
> +	    {
> +	      if (ins->last_rex_prefix >= 0)
> +		return ckp_bogus;
> +
> +	      ins->codep++;
> +	      if (!fetch_code (ins->info, ins->codep + 1))
> +		return ckp_fetch_error;
> +	      unsigned char rex2_payload = *ins->codep;
> +	      ins->rex2 = rex2_payload >> 4;
> +	      ins->rex = (rex2_payload & 0xf) | REX_OPCODE;
> +	      ins->codep++;
> +	      ins->last_rex2_prefix = i;
> +	      ins->all_prefixes[i] = REX2_OPCODE;

... you do here?

> @@ -11519,8 +11590,11 @@ OP_E_memory (instr_info *ins, int bytemode, int sizeflag)
>  	{
>  	  vindex = ins->sib.index;
>  	  USED_REX (REX_X);
> +	  USED_REX2 (REX_X);
>  	  if (ins->rex & REX_X)
>  	    vindex += 8;
> +	  if (ins->rex2 & REX_X)
> +	    vindex += 16;
>  	  switch (bytemode)
>  	    {
>  	    case vex_vsib_d_w_dq_mode:

Would you mind moving this adjustment down into the default case? The
variable's name doesn't really match its purpose, so we want to at
least try to avoid adding yet more confusion. The bit, after all, is
relevant to GPR operands only. (Note that I see no reason for this
to require the USED_REX2() to remain separate, for it to be moved as
well. That recording is fine to do centrally in USED_REX(); the change
I'm asking for is solely for code clarity purposes.)

> @@ -1072,10 +1076,45 @@ get_element_size (char **opnd, int lineno)
>    return elem_size;
>  }
>  
> +static bool
> +rex2_disallowed (const unsigned long long opcode, unsigned int length,
> +		 unsigned int space, const char *cpu_flags)
> +{
> +  /* Some opcodes encode a ModR/M-like byte directly in the opcode.  */
> +  unsigned long long
> +  base_opcode = opcode >> (8 * length - 8);

Nit: This easily fits on a single line. I also don't thing you need to
use unsigned long long here; afaict unsigned int will do quite fine.

Jan