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

[Jan Beulich <jbeulich@suse.com>]

On 19.09.2023 17:25, Cui, Lili wrote:
> --- a/opcodes/i386-dis.c
> +++ b/opcodes/i386-dis.c
> @@ -144,6 +144,12 @@ struct instr_info
>    /* Bits of REX we've already used.  */
>    uint8_t rex_used;
>  
> +  /* REX2 prefix for the current instruction use gpr32(r16-r31). */
> +  unsigned char rex2;
> +  /* Bits of REX2 we've already used.  */
> +  unsigned char rex2_used;

Since these two fields don't hold the REX2 bits directly, the comment(s)
want(s) saying what they contain.

> +  unsigned char rex2_payload;

I don't see this field used in more than one function. In which case it can
be a local variable there (in the innermost scope covering all uses).

> @@ -367,6 +381,7 @@ fetch_error (const instr_info *ins)
>  #define PREFIX_IGNORED_DATA	(PREFIX_DATA << PREFIX_IGNORED_SHIFT)
>  #define PREFIX_IGNORED_ADDR	(PREFIX_ADDR << PREFIX_IGNORED_SHIFT)
>  #define PREFIX_IGNORED_LOCK	(PREFIX_LOCK << PREFIX_IGNORED_SHIFT)
> +#define PREFIX_IGNORED_REX2	(PREFIX_REX2 << PREFIX_IGNORED_SHIFT)

I don't think "ignored" is what you mean, considering ...

> @@ -2794,9 +2817,9 @@ static const struct dis386 reg_table[][8] = {
>      { Bad_Opcode },
>      { "cmpxchg8b", { { CMPXCHG8B_Fixup, q_mode } }, 0 },
>      { Bad_Opcode },
> -    { "xrstors", { FXSAVE }, 0 },
> -    { "xsavec", { FXSAVE }, 0 },
> -    { "xsaves", { FXSAVE }, 0 },
> +    { "xrstors", { FXSAVE }, PREFIX_IGNORED_REX2 },
> +    { "xsavec", { FXSAVE }, PREFIX_IGNORED_REX2 },
> +    { "xsaves", { FXSAVE }, PREFIX_IGNORED_REX2 },
>      { MOD_TABLE (MOD_0FC7_REG_6) },
>      { MOD_TABLE (MOD_0FC7_REG_7) },
>    },
> @@ -3364,7 +3387,7 @@ static const struct dis386 prefix_table[][4] = {
>  
>    /* PREFIX_0FAE_REG_4_MOD_0 */
>    {
> -    { "xsave",	{ FXSAVE }, 0 },
> +    { "xsave",	{ FXSAVE }, PREFIX_IGNORED_REX2 },
>      { "ptwrite{%LQ|}", { Edq }, 0 },
>    },
>  
> @@ -3382,7 +3405,7 @@ static const struct dis386 prefix_table[][4] = {
>  
>    /* PREFIX_0FAE_REG_6_MOD_0 */
>    {
> -    { "xsaveopt",	{ FXSAVE }, PREFIX_OPCODE },
> +    { "xsaveopt",	{ FXSAVE }, PREFIX_OPCODE | PREFIX_IGNORED_REX2 },
>      { "clrssbsy",	{ Mq }, PREFIX_OPCODE },
>      { "clwb",	{ Mb }, PREFIX_OPCODE },
>    },
> @@ -8125,7 +8148,7 @@ static const struct dis386 mod_table[][2] = {
>    },
>    {
>      /* MOD_0FAE_REG_5 */
> -    { "xrstor",		{ FXSAVE }, PREFIX_OPCODE },
> +    { "xrstor",		{ FXSAVE }, PREFIX_OPCODE | PREFIX_IGNORED_REX2 },
>      { PREFIX_TABLE (PREFIX_0FAE_REG_5_MOD_3) },
>    },

... these uses here.

> @@ -8751,6 +8796,8 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins)
>        break;
>  
>      case USE_VEX_C4_TABLE:
> +      if (ins->last_rex2_prefix >= 0)
> +	return &bad_opcode;
>        /* VEX prefix.  */
>        if (!fetch_code (ins->info, ins->codep + 3))
>  	return &err_opcode;
> @@ -8812,6 +8859,8 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins)
>        break;
>  
>      case USE_VEX_C5_TABLE:
> +      if (ins->last_rex2_prefix >= 0)
> +	return &bad_opcode;
>        /* VEX prefix.  */
>        if (!fetch_code (ins->info, ins->codep + 2))
>  	return &err_opcode;
> @@ -8853,6 +8902,8 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins)
>        break;
>  
>      case USE_EVEX_TABLE:
> +      if (ins->last_rex2_prefix >= 0)
> +	return &bad_opcode;
>        ins->two_source_ops = false;
>        /* EVEX prefix.  */
>        ins->vex.evex = true;

There aren't similar REX checks here, yet both should be handled as similarly
as possible.

> @@ -9292,13 +9344,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 & 0x8))

Please can literals like the 0x8 here gain meaningful names (REX2_M in this
case)?

> @@ -9468,6 +9532,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_IGNORED_REX2:

You may rather want to mask off the high part in the switch() expression
itself.

> @@ -9510,9 +9575,17 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
>  
>    /* Check if the REX prefix is used.  */
>    if ((ins.rex ^ ins.rex_used) == 0
> -      && !ins.need_vex && ins.last_rex_prefix >= 0)
> +      && !ins.need_vex && ins.last_rex_prefix >= 0
> +      && ins.last_rex2_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))
> +	  || dp == &bad_opcode))
> +    ins.all_prefixes[ins.last_rex2_prefix] = 0;

I'm again puzzled by the dissimilarity with the REX handling. Furthermore,
with the way you split the REX2 payload byte, I don't think the combination
of REX and REX2 handling above will result in correct output in all cases.
(There shouldn't be mention of an unused REX bit when REX2 was in use on an
insn.)

> @@ -11307,6 +11386,7 @@ static bool
>  OP_E_memory (instr_info *ins, int bytemode, int sizeflag)
>  {
>    int add = (ins->rex & REX_B) ? 8 : 0;
> +  add += (ins->rex2 & REX_B) ? 16 : 0;
>    int riprel = 0;
>    int shift;

While generally mixing declaration and statements is okay nowadays, putting
a statement in the middle of a block's initial declarations would imo still
better be avoided.

Jan