Re: [PATCH] Support Intel USER_MSR

[Jan Beulich <jbeulich@suse.com>]

On 10.10.2023 09:24, Hu, Lin1 wrote:
> @@ -3863,6 +3864,8 @@ build_vex_prefix (const insn_template *t)
>  	case SPACE_0F:
>  	case SPACE_0F38:
>  	case SPACE_0F3A:
> +	case SPACE_EVEXMAP5:
> +	case SPACE_VEXMAP7:
>  	  i.vex.bytes[0] = 0xc4;
>  	  break;

I can see the need for the latter line you add, but why the former?
(If it is needed for some reason, this is a strong hint at the description
being overly brief.)

> @@ -8752,6 +8755,18 @@ build_modrm_byte (void)
>        source = v;
>        v = tmp;
>      }
> +  if (i.tm.opcode_modifier.operandconstraint == SWAP_SOURCE_DEST)
> +    {
> +      if (dest == (unsigned int) ~0)
> +	source = source ^ 1;
> +      else
> +	{
> +	  unsigned int tmp = source;
> +
> +	  source = dest;
> +	  dest = tmp;
> +	}
> +    }

Why is this needed? There's only a single register operand in both
affected insn forms (see comment below on the 2-register form).
Furthermore I think it would be easier if you "canonicalized" the
early immediate to be the 1st operand, such that for all other
purposes immediates remain first.

As a cosmetic nit: Please have a blank line ahead of the if() block
(if it needs to stay).

> --- /dev/null
> +++ b/gas/testsuite/gas/i386/user_msr-inval.s
> @@ -0,0 +1,7 @@
> +# Check Illegal 32bit USER_MSR instructions
> +
> +	.allow_index_reg
> +	.text
> +_start:
> +	urdmsr	%r12, %r14	 #USER_MSR
> +	uwrmsr	%r12, %r14	 #USER_MSR

As per comments on earlier series: What use are the comments here?
(Applicable also again below.)

> --- /dev/null
> +++ b/gas/testsuite/gas/i386/x86-64-user_msr.s
> @@ -0,0 +1,15 @@
> +# Check 64bit USER_MSR instructions
> +
> +	.allow_index_reg
> +	.text
> +_start:
> +	urdmsr	%r14, %r12	 #USER_MSR
> +	urdmsr	$51515151, %r12	 #USER_MSR
> +	uwrmsr	%r12, %r14	 #USER_MSR
> +	uwrmsr	%r12, $51515151	 #USER_MSR
> +
> +.intel_syntax noprefix

Nit: Please indent directives.

> +	urdmsr	r12, r14	 #USER_MSR
> +	urdmsr	r12, 51515151	 #USER_MSR
> +	uwrmsr	r14, r12	 #USER_MSR
> +	uwrmsr	51515151, r12	 #USER_MSR

I think varying registers slightly more (such that each two-register
form has one low-8 and one high-8 operand, totaling to two forms each
to prove that the REX.[RB] bits are also correctly dealt with) would
be better.

Btw, what's the interaction here with APX? The legacy forms are going
to use REX2, but the VEX forms would need EVEX variants then.

> @@ -618,6 +620,8 @@ enum
>    w_mode,
>    /* double word operand  */
>    d_mode,
> +  /* double word operand 0 */
> +  d_0_mode,

Why is this needed? IOW why does d_mode not do? Or alternatively why
isn't this a name indicating that it's an unsigned 32-bit value (as
opposed to other 32-bit immediates in 64-bit mode)?

> @@ -845,6 +849,7 @@ enum
>    REG_VEX_0FAE,
>    REG_VEX_0F3849_X86_64_L_0_W_0_M_1_P_0,
>    REG_VEX_0F38F3_L_0,
> +  REG_VEX_MAP7_F8_L_0_W_0_M_1,
>  
>    REG_XOP_09_01_L_0,
>    REG_XOP_09_02_L_0,
> @@ -893,8 +898,10 @@ enum
>    MOD_0FC7_REG_6,
>    MOD_0FC7_REG_7,
>    MOD_0F38DC_PREFIX_1,
> +  MOD_0F38F8,
>  
>    MOD_VEX_0F3849_X86_64_L_0_W_0,
> +  MOD_VEX_MAP7_F8_L_0_W_0,
>  };

As before - no new mod_table[] entries please which don't have both
branches populated.

> @@ -6791,6 +6839,297 @@ static const struct dis386 vex_table[][256] = {
>      { Bad_Opcode },
>      { Bad_Opcode },
>    },
> +  /* VEX_MAP7 */
> +  {
> +    /* 00 */
> +    { Bad_Opcode },

I wonder whether adding a full new table (rather than some special case
code) is really a god use of space. Of course if you know that more of it
will be populated in the not too distant future ...

> @@ -11248,6 +11609,20 @@ get32s (instr_info *ins, bfd_vma *res)
>    return true;
>  }
>  
> +/* The function is used to get imm32, when imm32 is operand 0, and ins only has 2 operands. */
> +static bool
> +get32_operand0 (instr_info *ins, bfd_vma *res)
> +{
> +
> +  if (!fetch_code (ins->info, ins->codep + 5))
> +    return false;
> +  *res = *(ins->codep++ + 1) & (bfd_vma) 0xff;
> +  *res |= (*(ins->codep++ + 1) & (bfd_vma) 0xff) << 8;
> +  *res |= (*(ins->codep++ + 1) & (bfd_vma) 0xff) << 16;
> +  *res |= (*(ins->codep++ + 1) & (bfd_vma) 0xff) << 24;
> +  return true;
> +}

Instead of this (which assumes ModRM.mod == 3) I think you want to
arrange for dealing with ModRM first. We already have OP_Skip_MODRM()
for such needs, which you could use in a first "hidden" operand.

> @@ -3346,3 +3349,12 @@ erets, 0xf20f01ca, FRED|x64, NoSuf, {}
>  eretu, 0xf30f01ca, FRED|x64, NoSuf, {}
>  
>  // FRED instructions end.
> +
> +// USER_MSR instructions.
> +
> +urdmsr, 0xf20f38f8, USER_MSR|x64, Modrm|IgnoreSize|SwapSourceDest|NoSuf, { Reg64, Reg64 }

Iirc RegMem is the attribute to use here, not any new one.

> +urdmsr, 0xf2f8/0, USER_MSR|x64, Modrm|Vex128|VexMap7|VexW0|IgnoreSize|NoSuf, { Imm32S, Reg64 }

This and ...

> +uwrmsr, 0xf30f38f8, USER_MSR|x64, Modrm|IgnoreSize|NoSuf, { Reg64, Reg64 }
> +uwrmsr, 0xf3f8/0, USER_MSR|x64, Modrm|Vex128|VexMap7|VexW0|IgnoreSize|SwapSourceDest|NoSuf, { Reg64, Imm32S }

... this needs to use Imm32, not Imm32S. I understand this is going to
cause complications elsewhere, but we can't afford getting this wrong.

Also in all forms I think you don't mean IgnoreSize, but NoRex64.

Jan