Support Intel USER_MSR

  This patch aims to support Intel USER_MSR.

gas/ChangeLog:

	* NEWS: Support Intel USER_MSR.
	* config/tc-i386.c: Add user_msr and add VEXMAP7 in
	opc_spc and build_vex_prefix.
	(build_modrm_byte): Swap Operands for USER_MSR.
	* doc/c-i386.texi: Document .user_msr.
	* testsuite/gas/i386/i386.exp: Run USER_MSR tests.
	* testsuite/gas/i386/x86-64.exp: Ditto.
	* testsuite/gas/i386/user_msr-inval.l: New test.
	* testsuite/gas/i386/user_msr-inval.s: Ditto.
	* testsuite/gas/i386/x86-64-user_msr-intel.d: Ditto.
	* testsuite/gas/i386/x86-64-user_msr.d: Ditto.
	* testsuite/gas/i386/x86-64-user_msr.s: Ditto.

opcodes/ChangeLog:

	* i386-dis.c (get32_operand0): Add a new function
	to get operand0 that is IMM32.
	(Gq): New.
	(Id0): Ditto.
	(get_valid_dis386): Add VEX_MAP7 in VEX prefix.
	(OP_I): Handle d_0_mode.
	(d_0_mode): New.
	(REG_VEX_MAP7_F8_L_0_W_0_M_1): New.
	(MOD_VEX_MAP7_F8_L_0_W_0): Ditto.
	(PREFIX_VEX_MAP7_F8_L_0_W_0_M_1_R_0_X86_64): Ditto.
	(X86_64_VEX_MAP7_F8_L_0_W_0_M_1_R_0): Ditto.
	(VEX_MAP7): Ditto.
	(VEX_LEN_MAP7_F8): Ditto.
	(VEX_W_MAP7_F8_L_0): Ditto.
	(MOD_0F38F8): Ditto.
	(PREFIX_0F38F8_M_0): Ditto.
	(PREFIX_0F38F8_M_1_X86_64): Ditto.
	(X86_64_0F38F8_M_1): Ditto.
	(PREFIX_0F38F8): Remove.
	(prefix_table): Add PREFIX_0F38F8_M_1_X86_64.
	Remove PREFIX_0F38F8.
	(reg_table): Add REG_VEX_MAP7_F8_L_0_W_0_M_1,
	PREFIX_VEX_MAP7_F8_L_0_W_0_M_1_R_0_X86_64.
	(x86_64_table): Add X86_64_0F38F8_PREFIX_3_M_1,
	X86_64_VEX_MAP7_F8_L_0_W_0_M_1_R_0 and X86_64_0F38F8_M_1.
	(vex_table): Add VEX_MAP7.
	(vex_len_table): Add VEX_LEN_MAP7_F8,
	VEX_W_MAP7_F8_L_0.
	(mod_table): New entry for USER_MSR and
	add MOD_VEX_MAP7_F8_L_0_W_0 and MOD_0F38F8.
	* i386-gen.c (cpu_flag_init): Add CPU_USER_MSR_FLAGS and
	CPU_ANY_USER_MSR_FLAGS.
	* i386-init.h: Regenerated.
	* i386-mnem.h: Ditto.
	* i386-opc.h (SPACE_VEXMAP7): New.
	(SWAP_SOURCE_DEST): Ditto.
	(CPU_USER_MSR_FLAGS): Ditoo.
	(CPU_ANY_USER_MSR_FLAGS): Ditto.
	(i386_cpu_flags): Add cpuuser_msr.
	* i386-opc.tbl: Add USER_MSR instructions.
	* i386-tbl.h: Regenerated.
---
 gas/NEWS                                      |    2 +
 gas/config/tc-i386.c                          |   15 +
 gas/doc/c-i386.texi                           |    3 +-
 gas/testsuite/gas/i386/i386.exp               |    1 +
 gas/testsuite/gas/i386/user_msr-inval.l       |    3 +
 gas/testsuite/gas/i386/user_msr-inval.s       |    7 +
 .../gas/i386/x86-64-user_msr-intel.d          |   18 +
 gas/testsuite/gas/i386/x86-64-user_msr.d      |   18 +
 gas/testsuite/gas/i386/x86-64-user_msr.s      |   15 +
 gas/testsuite/gas/i386/x86-64.exp             |    2 +
 opcodes/i386-dis.c                            |  387 ++++-
 opcodes/i386-gen.c                            |    2 +
 opcodes/i386-init.h                           |  816 +++++------
 opcodes/i386-mnem.h                           | 1278 +++++++++--------
 opcodes/i386-opc.h                            |   10 +
 opcodes/i386-opc.tbl                          |   12 +
 opcodes/i386-tbl.h                            |  272 ++--
 17 files changed, 1698 insertions(+), 1163 deletions(-)
 create mode 100644 gas/testsuite/gas/i386/user_msr-inval.l
 create mode 100644 gas/testsuite/gas/i386/user_msr-inval.s
 create mode 100644 gas/testsuite/gas/i386/x86-64-user_msr-intel.d
 create mode 100644 gas/testsuite/gas/i386/x86-64-user_msr.d
 create mode 100644 gas/testsuite/gas/i386/x86-64-user_msr.s
  

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
  

Thanks for your review. I responded individually under each comment.
Attached is the modified version.

BRs,
Lin
-----Original Message-----
From: Jan Beulich <jbeulich@suse.com> 
Sent: Monday, October 16, 2023 8:11 PM
To: Hu, Lin1 <lin1.hu@intel.com>
Cc: Lu, Hongjiu <hongjiu.lu@intel.com>; binutils@sourceware.org
Subject: Re: [PATCH] Support Intel USER_MSR

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.)

I think it's a wrong that is made by reordering patches. I have removed it.

> @@ -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).

Indeed, I've only kept the part that deals with a single register. Do you mean to complain to the person who designed the insn. Unfortunately, that's impossible.

> --- /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.)

I have removed it.

> --- /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.
Have removed these comments.

> +	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.

I have added some other tests here.

> 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.

Not at the moment. If it gets added to the documentation later, I'll add its.

> @@ -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)?

I want to output imm32 first, but modrm byte is behind imm32, so I need to skip modrm for the moment. If I use { "uwrmsr", { Id, Eq }, 0} Id will read modrm byte. The mode is for a imm32 not an unsigned 32-bit value, so its name doesn't indicate that it's an unsigned integer.

> @@ -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.

Have removed it.

+  {
+    /* MOD_VEX_MAP7_F8_L_0_W_0 */
+    { Bad_Opcode },
+    { REG_TABLE (REG_VEX_MAP7_F8_L_0_W_0_M_1) },
+  },

> @@ -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 ...

I don't know, I'm just treating the new opcode_space MAP7 like other opcode_space. 

> @@ -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.

I want to output imm32 first, but modrm byte is behind imm32, so I need to skip modrm for the moment. but I can't use OP_Skip_MODRM to deal with my problem, If I use it, I should add ins->codep-- at the end of get32_operand0.

> @@ -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.

Indeed.

> +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.

Have modified them.

Jan
  

On 18.10.2023 09:51, Hu, Lin1 wrote:
> Thanks for your review. I responded individually under each comment.
> Attached is the modified version.

Please can you send proper new versions, to allow easy commenting?

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com> 
> Sent: Monday, October 16, 2023 8:11 PM
> 
> On 10.10.2023 09:24, Hu, Lin1 wrote:
>> @@ -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).
> 
> Indeed, I've only kept the part that deals with a single register. Do you mean to complain to the person who designed the insn. Unfortunately, that's impossible.

I'm having trouble connecting your reply to what I wrote. No, I do not
mean to complain; I can certainly see why the immediate is wanted as
first (Intel) / last (AT&T) operand.

I'm also not happy about the new change to build_modrm_byte(). When
asking to "canonicalize" operands, I meant to gave this generalized,
with SWAP_SOURCE_DEST dropped completely. (This will then also save
me from complaining about a missing blank in SwapSourceDest's
#define.)

>> --- /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.
> Have removed these comments.

Neither does your reply fit here, nor did you what I've asked for
here (in addition to the earlier, wider request of dropping
meaningless comments).

>> +	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.
> 
> I have added some other tests here.

Thanks.

>> @@ -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)?
> 
> I want to output imm32 first, but modrm byte is behind imm32, so I need to skip modrm for the moment. If I use { "uwrmsr", { Id, Eq }, 0} Id will read modrm byte. The mode is for a imm32 not an unsigned 32-bit value, so its name doesn't indicate that it's an unsigned integer.

But the immediate is an unsigned 32-bit one. Signed ones would be sign-
extended, which isn't the case here (or else the range 0x80000000 ...
0xffffffff wouldn't form valid MSR numbers).

>> @@ -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.
> 
> Have removed it.

But you're using Eq there, i.e. permitting memory operands as well.
What (I think) you want is Rq. For consistency this may then also
want using in the new PREFIX_0F38F8_M_1_X86_64 entry. But of course
you need to be careful about the collision with Nq.

>> @@ -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 ...
> 
> I don't know, I'm just treating the new opcode_space MAP7 like other opcode_space. 

I'm afraid that "I don't know" is not an answer here. You can basically
take two positions: Mine (waste of space) or you justify why the extra
space used (and the extra runtime relocations added) aren't of concern.

>> @@ -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.
> 
> I want to output imm32 first, but modrm byte is behind imm32, so I need to skip modrm for the moment. but I can't use OP_Skip_MODRM to deal with my problem, If I use it, I should add ins->codep-- at the end of get32_operand0.

get32_operand0() should go away imo; at the very least I don't agree
with special casing it being operand 0 (which, as you realize, is true
only in the textual representation, and only in Intel syntax, but in
particular not in the encoding). It may be necessary to special case
it being an unsigned immediate, but get32() already fits that purpose.

>> @@ -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.
> 
> Indeed.
> 
>> +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.

I see you've corrected this, but it doesn't look to me as if the
match_template() change is actually going to do the job. You can't
change i.types[] without looking at the actual values. I think this
needs dealing with in optimize_imm() and/or finalize_imm()
(irrespective of it's name more likely the former, but knock-on
adjustments to the latter may then turn out to be needed).

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

Hmm, I may have mislead you: NoRex64 is meaningless on VEX encodings.
There only the IgnoreSize needed dropping. I'm sorry.

Jan
  

Thanks for your advice. I've tentatively made some changes based on your comments, 
but it's not completely finalized yet, I'd like to consult with you for your opinion, and 
I'll send out a new patch after it's basically confirmed.

BRs,
Lin

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, October 19, 2023 4:36 PM
> To: Hu, Lin1 <lin1.hu@intel.com>
> Cc: Lu, Hongjiu <hongjiu.lu@intel.com>; binutils@sourceware.org
> Subject: Re: [PATCH] Support Intel USER_MSR
> 
> On 18.10.2023 09:51, Hu, Lin1 wrote:
> > Thanks for your review. I responded individually under each comment.
> > Attached is the modified version.
> 
> Please can you send proper new versions, to allow easy commenting?
> 
> > -----Original Message-----
> > From: Jan Beulich <jbeulich@suse.com>
> > Sent: Monday, October 16, 2023 8:11 PM
> >
> > On 10.10.2023 09:24, Hu, Lin1 wrote:
> >> @@ -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).
> >
> > Indeed, I've only kept the part that deals with a single register. Do you mean to
> complain to the person who designed the insn. Unfortunately, that's impossible.
> 
> I'm having trouble connecting your reply to what I wrote. No, I do not mean to
> complain; I can certainly see why the immediate is wanted as first (Intel) / last
> (AT&T) operand.
> 
> I'm also not happy about the new change to build_modrm_byte(). When asking
> to "canonicalize" operands, I meant to gave this generalized, with
> SWAP_SOURCE_DEST dropped completely. (This will then also save me from
> complaining about a missing blank in SwapSourceDest's
> #define.)
> 

OK, I implemented a basic logic to handle the current situation, and included remarks.
Like:

   /* If the last operand is an immediate number (ATT), we need to modify
       the source operand accordingly. If any instructions use other immediate
       (imm8, imm16, etc.) as the last operand, we must update the constraint.  */
    if (i.types[source].bitfield.imm32 == 1)
      source--;

What's your opinion on this version?

> >> --- /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.
> > Have removed these comments.
> 
> Neither does your reply fit here, nor did you what I've asked for here (in addition
> to the earlier, wider request of dropping meaningless comments).
> 

Perhaps there's an issue with my interface, but it seems to me that the
instruction now begins with a \t instead of two spaces for the updated patch.

> >> +	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.
> >
> > I have added some other tests here.
> 
> Thanks.
> 
> >> @@ -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)?
> >
> > I want to output imm32 first, but modrm byte is behind imm32, so I need to
> skip modrm for the moment. If I use { "uwrmsr", { Id, Eq }, 0} Id will read modrm
> byte. The mode is for a imm32 not an unsigned 32-bit value, so its name doesn't
> indicate that it's an unsigned integer.
> 
> But the immediate is an unsigned 32-bit one. Signed ones would be sign-
> extended, which isn't the case here (or else the range 0x80000000 ...
> 0xffffffff wouldn't form valid MSR numbers).

OK, you are right.

> 
> >> @@ -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.
> >
> > Have removed it.
> 
> But you're using Eq there, i.e. permitting memory operands as well.
> What (I think) you want is Rq. For consistency this may then also want using in
> the new PREFIX_0F38F8_M_1_X86_64 entry. But of course you need to be
> careful about the collision with Nq.

I change the Nq's mode from q_mode to q_mm_mode and create Rq ( OP_R, q_mode ) for used.

> 
> >> @@ -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 ...
> >
> > I don't know, I'm just treating the new opcode_space MAP7 like other
> opcode_space.
> 
> I'm afraid that "I don't know" is not an answer here. You can basically take two
> positions: Mine (waste of space) or you justify why the extra space used (and the
> extra runtime relocations added) aren't of concern.
> 

OK, I will skip MAP7 table. If vex_table_index = VEX_MAP7 and index == f8
dp will be VEX_LEN_MAP7_F8. So I don't need to add a full new table untill other MAP7
instructions raise.

> >> @@ -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.
> >
> > I want to output imm32 first, but modrm byte is behind imm32, so I need to
> skip modrm for the moment. but I can't use OP_Skip_MODRM to deal with my
> problem, If I use it, I should add ins->codep-- at the end of get32_operand0.
> 
> get32_operand0() should go away imo; at the very least I don't agree with
> special casing it being operand 0 (which, as you realize, is true only in the textual
> representation, and only in Intel syntax, but in particular not in the encoding). It
> may be necessary to special case it being an unsigned immediate, but get32()
> already fits that purpose.

I've thought of it so far is I can use a Fixup function like

static bool
uwrmsr_Fixup (instr_info *ins, int bytemode, int sizeflag)
{
    if (bytemode == d_mode)
      {
        if (OP_Skip_MODRM (ins, 0, sizeflag))
          {
            if (OP_I (ins, bytemode, sizeflag))
              {
                ins->codep--;
              }
              return true;
          }
      }
    return false;
}

Then the uwrmsr's unit will be { "uwrmsr",       {	{ uwrmsr_Fixup, d_mode }, Rq }, 0 }.
What‘s your opinion?

> 
> >> @@ -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.
> >
> > Indeed.
> >
> >> +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.
> 
> I see you've corrected this, but it doesn't look to me as if the
> match_template() change is actually going to do the job. You can't change
> i.types[] without looking at the actual values. I think this needs dealing with in
> optimize_imm() and/or finalize_imm() (irrespective of it's name more likely the
> former, but knock-on adjustments to the latter may then turn out to be needed).
> 

OK.

> >> Also in all forms I think you don't mean IgnoreSize, but NoRex64.
> >
> > Have modified them.
> 
> Hmm, I may have mislead you: NoRex64 is meaningless on VEX encodings.
> There only the IgnoreSize needed dropping. I'm sorry.
> 
> Jan
  

On 24.10.2023 10:38, Hu, Lin1 wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Thursday, October 19, 2023 4:36 PM
>>
>> On 18.10.2023 09:51, Hu, Lin1 wrote:
>>> -----Original Message-----
>>> From: Jan Beulich <jbeulich@suse.com>
>>> Sent: Monday, October 16, 2023 8:11 PM
>>>
>>> On 10.10.2023 09:24, Hu, Lin1 wrote:
>>>> @@ -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).
>>>
>>> Indeed, I've only kept the part that deals with a single register. Do you mean to
>> complain to the person who designed the insn. Unfortunately, that's impossible.
>>
>> I'm having trouble connecting your reply to what I wrote. No, I do not mean to
>> complain; I can certainly see why the immediate is wanted as first (Intel) / last
>> (AT&T) operand.
>>
>> I'm also not happy about the new change to build_modrm_byte(). When asking
>> to "canonicalize" operands, I meant to gave this generalized, with
>> SWAP_SOURCE_DEST dropped completely. (This will then also save me from
>> complaining about a missing blank in SwapSourceDest's
>> #define.)
>>
> 
> OK, I implemented a basic logic to handle the current situation, and included remarks.
> Like:
> 
>    /* If the last operand is an immediate number (ATT), we need to modify
>        the source operand accordingly. If any instructions use other immediate
>        (imm8, imm16, etc.) as the last operand, we must update the constraint.  */
>     if (i.types[source].bitfield.imm32 == 1)
>       source--;
> 
> What's your opinion on this version?

As before - build_modrm_byte() is the wrong place to make the intended
(generalized) adjustment. You want to move the unusually placed imm
operand to its usual place as soon as you're done with matching input
against the respective template.

>>>> --- /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.
>>> Have removed these comments.
>>
>> Neither does your reply fit here, nor did you what I've asked for here (in addition
>> to the earlier, wider request of dropping meaningless comments).
>>
> 
> Perhaps there's an issue with my interface, but it seems to me that the
> instruction now begins with a \t instead of two spaces for the updated patch.

Not that you say "instruction" when I said "directive". My comment
wasn't about any instruction, but about the ".intel_syntax noprefix"
line.

>>>> @@ -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 ...
>>>
>>> I don't know, I'm just treating the new opcode_space MAP7 like other
>> opcode_space.
>>
>> I'm afraid that "I don't know" is not an answer here. You can basically take two
>> positions: Mine (waste of space) or you justify why the extra space used (and the
>> extra runtime relocations added) aren't of concern.
>>
> 
> OK, I will skip MAP7 table. If vex_table_index = VEX_MAP7 and index == f8
> dp will be VEX_LEN_MAP7_F8. So I don't need to add a full new table untill other MAP7
> instructions raise.

H.J., before we go this route, what's your view here?

>>>> @@ -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.
>>>
>>> I want to output imm32 first, but modrm byte is behind imm32, so I need to
>> skip modrm for the moment. but I can't use OP_Skip_MODRM to deal with my
>> problem, If I use it, I should add ins->codep-- at the end of get32_operand0.
>>
>> get32_operand0() should go away imo; at the very least I don't agree with
>> special casing it being operand 0 (which, as you realize, is true only in the textual
>> representation, and only in Intel syntax, but in particular not in the encoding). It
>> may be necessary to special case it being an unsigned immediate, but get32()
>> already fits that purpose.
> 
> I've thought of it so far is I can use a Fixup function like
> 
> static bool
> uwrmsr_Fixup (instr_info *ins, int bytemode, int sizeflag)
> {
>     if (bytemode == d_mode)
>       {
>         if (OP_Skip_MODRM (ins, 0, sizeflag))
>           {
>             if (OP_I (ins, bytemode, sizeflag))
>               {
>                 ins->codep--;
>               }
>               return true;
>           }
>       }
>     return false;
> }
> 
> Then the uwrmsr's unit will be { "uwrmsr",       {	{ uwrmsr_Fixup, d_mode }, Rq }, 0 }.
> What‘s your opinion?

Hmm, not very nice, but I can't exclude it simply won't get any better.
My desire was for there to not be any new fixup function, and for
OP_Skip_MODRM to be used directly in the table entry. (In any event,
if you really need to keep this new function, please combine the three
if()-s into a single one, helping readability quite a bit.

Jan
  

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, October 24, 2023 4:56 PM
> To: Hu, Lin1 <lin1.hu@intel.com>; Lu, Hongjiu <hongjiu.lu@intel.com>
> Cc: binutils@sourceware.org
> Subject: Re: [PATCH] Support Intel USER_MSR
> 
> On 24.10.2023 10:38, Hu, Lin1 wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: Thursday, October 19, 2023 4:36 PM
> >>
> >> On 18.10.2023 09:51, Hu, Lin1 wrote:
> >>> -----Original Message-----
> >>> From: Jan Beulich <jbeulich@suse.com>
> >>> Sent: Monday, October 16, 2023 8:11 PM
> >>>
> >>> On 10.10.2023 09:24, Hu, Lin1 wrote:
> >>>> @@ -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).
> >>>
> >>> Indeed, I've only kept the part that deals with a single register.
> >>> Do you mean to
> >> complain to the person who designed the insn. Unfortunately, that's
> impossible.
> >>
> >> I'm having trouble connecting your reply to what I wrote. No, I do
> >> not mean to complain; I can certainly see why the immediate is wanted
> >> as first (Intel) / last
> >> (AT&T) operand.
> >>
> >> I'm also not happy about the new change to build_modrm_byte(). When
> >> asking to "canonicalize" operands, I meant to gave this generalized,
> >> with SWAP_SOURCE_DEST dropped completely. (This will then also save
> >> me from complaining about a missing blank in SwapSourceDest's
> >> #define.)
> >>
> >
> > OK, I implemented a basic logic to handle the current situation, and included
> remarks.
> > Like:
> >
> >    /* If the last operand is an immediate number (ATT), we need to modify
> >        the source operand accordingly. If any instructions use other immediate
> >        (imm8, imm16, etc.) as the last operand, we must update the constraint.
> */
> >     if (i.types[source].bitfield.imm32 == 1)
> >       source--;
> >
> > What's your opinion on this version?
> 
> As before - build_modrm_byte() is the wrong place to make the intended
> (generalized) adjustment. You want to move the unusually placed imm operand
> to its usual place as soon as you're done with matching input against the
> respective template.

Oh, I get you.
> 
> >>>> --- /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.
> >>> Have removed these comments.
> >>
> >> Neither does your reply fit here, nor did you what I've asked for
> >> here (in addition to the earlier, wider request of dropping meaningless
> comments).
> >>
> >
> > Perhaps there's an issue with my interface, but it seems to me that
> > the instruction now begins with a \t instead of two spaces for the updated
> patch.
> 
> Not that you say "instruction" when I said "directive". My comment wasn't
> about any instruction, but about the ".intel_syntax noprefix"
> line.
> 

Okay, thanks for the explanation.

> >>>> @@ -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 ...
> >>>
> >>> I don't know, I'm just treating the new opcode_space MAP7 like other
> >> opcode_space.
> >>
> >> I'm afraid that "I don't know" is not an answer here. You can
> >> basically take two
> >> positions: Mine (waste of space) or you justify why the extra space
> >> used (and the extra runtime relocations added) aren't of concern.
> >>
> >
> > OK, I will skip MAP7 table. If vex_table_index = VEX_MAP7 and index ==
> > f8 dp will be VEX_LEN_MAP7_F8. So I don't need to add a full new table
> > untill other MAP7 instructions raise.
> 
> H.J., before we go this route, what's your view here?
> 

Just mention: H.J. has a sabbatical until Nov 30 2023. I'm not sure if you got his e-mail.

> >>>> @@ -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.
> >>>
> >>> I want to output imm32 first, but modrm byte is behind imm32, so I
> >>> need to
> >> skip modrm for the moment. but I can't use OP_Skip_MODRM to deal with
> >> my problem, If I use it, I should add ins->codep-- at the end of
> get32_operand0.
> >>
> >> get32_operand0() should go away imo; at the very least I don't agree
> >> with special casing it being operand 0 (which, as you realize, is
> >> true only in the textual representation, and only in Intel syntax,
> >> but in particular not in the encoding). It may be necessary to
> >> special case it being an unsigned immediate, but get32() already fits that
> purpose.
> >
> > I've thought of it so far is I can use a Fixup function like
> >
> > static bool
> > uwrmsr_Fixup (instr_info *ins, int bytemode, int sizeflag) {
> >     if (bytemode == d_mode)
> >       {
> >         if (OP_Skip_MODRM (ins, 0, sizeflag))
> >           {
> >             if (OP_I (ins, bytemode, sizeflag))
> >               {
> >                 ins->codep--;
> >               }
> >               return true;
> >           }
> >       }
> >     return false;
> > }
> >
> > Then the uwrmsr's unit will be { "uwrmsr",       {	{ uwrmsr_Fixup, d_mode },
> Rq }, 0 }.
> > What‘s your opinion?
> 
> Hmm, not very nice, but I can't exclude it simply won't get any better.
> My desire was for there to not be any new fixup function, and for
> OP_Skip_MODRM to be used directly in the table entry. (In any event, if you
> really need to keep this new function, please combine the three if()-s into a
> single one, helping readability quite a bit.
> 

I have another idea, can I have a new function like

OP_back_codep(...)
{
	Ins->codep--;
	Return true;
}

So the uwrmsr's unit will be { "uwrmsr", { Skip_MODRM, Id, Back_Codep, Rq }, 0 }.

OK, thanks for your advices.

> Jan
  

On 24.10.2023 12:01, Hu, Lin1 wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Tuesday, October 24, 2023 4:56 PM
>>
>> On 24.10.2023 10:38, Hu, Lin1 wrote:
>>> I've thought of it so far is I can use a Fixup function like
>>>
>>> static bool
>>> uwrmsr_Fixup (instr_info *ins, int bytemode, int sizeflag) {
>>>     if (bytemode == d_mode)
>>>       {
>>>         if (OP_Skip_MODRM (ins, 0, sizeflag))
>>>           {
>>>             if (OP_I (ins, bytemode, sizeflag))
>>>               {
>>>                 ins->codep--;
>>>               }
>>>               return true;
>>>           }
>>>       }
>>>     return false;
>>> }
>>>
>>> Then the uwrmsr's unit will be { "uwrmsr",       {	{ uwrmsr_Fixup, d_mode },
>> Rq }, 0 }.
>>> What‘s your opinion?
>>
>> Hmm, not very nice, but I can't exclude it simply won't get any better.
>> My desire was for there to not be any new fixup function, and for
>> OP_Skip_MODRM to be used directly in the table entry. (In any event, if you
>> really need to keep this new function, please combine the three if()-s into a
>> single one, helping readability quite a bit.
>>
> 
> I have another idea, can I have a new function like
> 
> OP_back_codep(...)
> {
> 	Ins->codep--;
> 	Return true;
> }
> 
> So the uwrmsr's unit will be { "uwrmsr", { Skip_MODRM, Id, Back_Codep, Rq }, 0 }.

Well, the main thing I dislike is the decrementing of codep, no matter where
it's put. In case you don't think you can get away without, I guess I'll try
afterwards, aiming at an incremental change then.

Jan
  

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, October 24, 2023 8:02 PM
> To: Hu, Lin1 <lin1.hu@intel.com>
> Cc: binutils@sourceware.org; Lu, Hongjiu <hongjiu.lu@intel.com>
> Subject: Re: [PATCH] Support Intel USER_MSR
> 
> On 24.10.2023 12:01, Hu, Lin1 wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: Tuesday, October 24, 2023 4:56 PM
> >>
> >> On 24.10.2023 10:38, Hu, Lin1 wrote:
> >>> I've thought of it so far is I can use a Fixup function like
> >>>
> >>> static bool
> >>> uwrmsr_Fixup (instr_info *ins, int bytemode, int sizeflag) {
> >>>     if (bytemode == d_mode)
> >>>       {
> >>>         if (OP_Skip_MODRM (ins, 0, sizeflag))
> >>>           {
> >>>             if (OP_I (ins, bytemode, sizeflag))
> >>>               {
> >>>                 ins->codep--;
> >>>               }
> >>>               return true;
> >>>           }
> >>>       }
> >>>     return false;
> >>> }
> >>>
> >>> Then the uwrmsr's unit will be { "uwrmsr",       {	{ uwrmsr_Fixup,
> d_mode },
> >> Rq }, 0 }.
> >>> What‘s your opinion?
> >>
> >> Hmm, not very nice, but I can't exclude it simply won't get any better.
> >> My desire was for there to not be any new fixup function, and for
> >> OP_Skip_MODRM to be used directly in the table entry. (In any event,
> >> if you really need to keep this new function, please combine the
> >> three if()-s into a single one, helping readability quite a bit.
> >>
> >
> > I have another idea, can I have a new function like
> >
> > OP_back_codep(...)
> > {
> > 	Ins->codep--;
> > 	Return true;
> > }
> >
> > So the uwrmsr's unit will be { "uwrmsr", { Skip_MODRM, Id, Back_Codep, Rq },
> 0 }.
> 
> Well, the main thing I dislike is the decrementing of codep, no matter where it's
> put. In case you don't think you can get away without, I guess I'll try afterwards,
> aiming at an incremental change then.
> 

I have another one in mind at the moment. Can I have a bool variable in instr_info,

@@ -221,6 +221,9 @@ struct instr_info
   /* Record whether EVEX masking is used incorrectly.  */
   bool illegal_masking;

+  /* Record whether the modrm byte has been skipped.  */
+  bool has_skipped_modrm.
+
   unsigned char op_ad;

And the Skip mod/rm byte pattern will be 

@@ -11668,7 +11658,11 @@ OP_Skip_MODRM (instr_info *ins, int bytemode ATTRIBUTE_UNUSED,

   /* Skip mod/rm byte.  */
   MODRM_CHECK;
-  ins->codep++;
+  if (!ins->has_skipped_modrm)
+    {
+      ins->codep++;
+      ins->has_skipped_modrm = true;
+    }
   return true;
 }
.
This change will be applied to all other similar sections (include OP_E).

So the uwrmsr's unit will be { "uwrmsr", { Skip_MODRM, Id, Rq }, 0 }, because the codep won't increasing in Rq, I don't need the decrementing of codep.

> Jan
  

On 25.10.2023 04:01, Hu, Lin1 wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Tuesday, October 24, 2023 8:02 PM
>> To: Hu, Lin1 <lin1.hu@intel.com>
>> Cc: binutils@sourceware.org; Lu, Hongjiu <hongjiu.lu@intel.com>
>> Subject: Re: [PATCH] Support Intel USER_MSR
>>
>> On 24.10.2023 12:01, Hu, Lin1 wrote:
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: Tuesday, October 24, 2023 4:56 PM
>>>>
>>>> On 24.10.2023 10:38, Hu, Lin1 wrote:
>>>>> I've thought of it so far is I can use a Fixup function like
>>>>>
>>>>> static bool
>>>>> uwrmsr_Fixup (instr_info *ins, int bytemode, int sizeflag) {
>>>>>     if (bytemode == d_mode)
>>>>>       {
>>>>>         if (OP_Skip_MODRM (ins, 0, sizeflag))
>>>>>           {
>>>>>             if (OP_I (ins, bytemode, sizeflag))
>>>>>               {
>>>>>                 ins->codep--;
>>>>>               }
>>>>>               return true;
>>>>>           }
>>>>>       }
>>>>>     return false;
>>>>> }
>>>>>
>>>>> Then the uwrmsr's unit will be { "uwrmsr",       {	{ uwrmsr_Fixup,
>> d_mode },
>>>> Rq }, 0 }.
>>>>> What‘s your opinion?
>>>>
>>>> Hmm, not very nice, but I can't exclude it simply won't get any better.
>>>> My desire was for there to not be any new fixup function, and for
>>>> OP_Skip_MODRM to be used directly in the table entry. (In any event,
>>>> if you really need to keep this new function, please combine the
>>>> three if()-s into a single one, helping readability quite a bit.
>>>>
>>>
>>> I have another idea, can I have a new function like
>>>
>>> OP_back_codep(...)
>>> {
>>> 	Ins->codep--;
>>> 	Return true;
>>> }
>>>
>>> So the uwrmsr's unit will be { "uwrmsr", { Skip_MODRM, Id, Back_Codep, Rq },
>> 0 }.
>>
>> Well, the main thing I dislike is the decrementing of codep, no matter where it's
>> put. In case you don't think you can get away without, I guess I'll try afterwards,
>> aiming at an incremental change then.
>>
> 
> I have another one in mind at the moment. Can I have a bool variable in instr_info,
> 
> @@ -221,6 +221,9 @@ struct instr_info
>    /* Record whether EVEX masking is used incorrectly.  */
>    bool illegal_masking;
> 
> +  /* Record whether the modrm byte has been skipped.  */
> +  bool has_skipped_modrm.
> +
>    unsigned char op_ad;
> 
> And the Skip mod/rm byte pattern will be 
> 
> @@ -11668,7 +11658,11 @@ OP_Skip_MODRM (instr_info *ins, int bytemode ATTRIBUTE_UNUSED,
> 
>    /* Skip mod/rm byte.  */
>    MODRM_CHECK;
> -  ins->codep++;
> +  if (!ins->has_skipped_modrm)
> +    {
> +      ins->codep++;
> +      ins->has_skipped_modrm = true;
> +    }
>    return true;
>  }
> .
> This change will be applied to all other similar sections (include OP_E).

Yes, that's along the lines of what I had in mind.

Jan