[PATCH 2/2 v2] Support Intel MSR_IMM

[Jan Beulich]

On 11.11.2024 09:27, Hu, Lin1 wrote:
> --- a/gas/NEWS
> +++ b/gas/NEWS
> @@ -1,5 +1,7 @@
>  -*- text -*-
>  
> +* Add support for Intel MSR_IMM instructions.

Intel supported other architectures in the past, and it may support
others in the future. I think it is always a good idea to disambiguate
things here, like is done e.g. ...

>  * Add support for Intel AVX10.2 instructions.
>  
>  * Add support for the x86 Zhaoxin GMI instructions.

... here by naming x86.

> @@ -6990,18 +6993,19 @@ i386_assemble (char *line)
>       because of the swapping above) in the incoming set of operands.  */
>    if ((i.imm_operands == 2
>         && (t->mnem_off == MN_extrq || t->mnem_off == MN_insertq))
> -      || (t->mnem_off == MN_uwrmsr && i.imm_operands
> -	  && i.operands > i.imm_operands))
> +      || ((t->mnem_off == MN_uwrmsr || t->mnem_off == MN_wrmsrns)
> +	  && i.imm_operands && i.operands > i.imm_operands))
>        swap_2_operands (0, 1);
>  
>    if (i.imm_operands)
>      {
> -      /* For USER_MSR instructions, imm32 stands for the name of an model specific
> -	 register (MSR). That's an unsigned quantity, whereas all other insns with
> -	 32-bit immediate and 64-bit operand size use sign-extended
> -	 immediates (imm32s). Therefore these insns are special-cased, bypassing
> -	 the normal handling of immediates here.  */
> -      if (is_cpu(current_templates.start, CpuUSER_MSR))
> +      /* For USER_MSR and MSR_IMM instructions, imm32 stands for the name of an

While touching this comment, would you mind correcting "an" to "a", seeing
that ...

> +	 model specific register (MSR). That's an unsigned quantity, whereas all

... it's not the acronym that directly follows?

> --- a/gas/doc/c-i386.texi
> +++ b/gas/doc/c-i386.texi
> @@ -223,6 +223,7 @@ accept various extension mnemonics.  For example,
>  @code{avx10.2/512},
>  @code{avx10.2/256},
>  @code{avx10.2/128},
> +@code{msr_imm},
>  @code{amx_int8},
>  @code{amx_bf16},
>  @code{amx_fp16},
> @@ -1694,7 +1695,7 @@ supported on the CPU specified.  The choices for @var{cpu_type} are:
>  @item @samp{.cmpccxadd} @tab @samp{.wrmsrns} @tab @samp{.msrlist}
>  @item @samp{.avx_ne_convert} @tab @samp{.rao_int} @tab @samp{.fred} @tab @samp{.lkgs}
>  @item @samp{.avx_vnni_int16} @tab @samp{.sha512} @tab @samp{.sm3} @tab @samp{.sm4}
> -@item @samp{.pbndkb} @tab @samp{.user_msr} @tab @samp{.avx10.2}
> +@item @samp{.pbndkb} @tab @samp{.user_msr} @tab @samp{.avx10.2} @tab @samp{.msr_imm}
>  @item @samp{.wbnoinvd} @tab @samp{.pconfig} @tab @samp{.waitpkg} @tab @samp{.cldemote}
>  @item @samp{.shstk} @tab @samp{.gfni} @tab @samp{.vaes} @tab @samp{.vpclmulqdq}
>  @item @samp{.movdiri} @tab @samp{.movdir64b} @tab @samp{.enqcmd} @tab @samp{.tsxldtrk}

As indicated before - imo it would be nice if msr_imm was adjacent to
user_msr everywhere.

> @@ -8691,6 +8723,8 @@ static const struct dis386 bad_opcode = { "(bad)", { XX }, 0 };
>  /* Fetch error indicator.  */
>  static const struct dis386 err_opcode = { NULL, { XX }, 0 };
>  
> +static const struct dis386 map7_f6_opcode = { VEX_LEN_TABLE (VEX_LEN_MAP7_F6) };
> +
>  static const struct dis386 map7_f8_opcode = { VEX_LEN_TABLE (VEX_LEN_MAP7_F8) };

As asked for before, please omit the blank line between the two closely
related (for being hand-crafted rather than having full tables) entries.

> --- a/opcodes/i386-opc.tbl
> +++ b/opcodes/i386-opc.tbl
> @@ -965,6 +965,10 @@ cpuid, 0xfa2, i486, NoSuf, {}
>  // Pentium extensions.
>  wrmsr, 0xf30, i586, NoSuf, {}
>  rdtsc, 0xf31, i586, NoSuf, {}
> +// MSR_IMM instructions' imm32 stands for the name of an model specific
> +// register (MSR). The imm32 needs a special handler in tc-i386.c, hence put
> +// rdmsr of MSR_IMM firsty, since the logic is determined by is_cpu (...).
> +rdmsr, 0xf2f6/0, APX_F(MSR_IMM), Modrm|Vex128|xVexMap7|EVex128|VexW0|NoSuf, { Imm32, Reg64 }
>  rdmsr, 0xf32, i586, NoSuf, {}
>  cmpxchg8b, 0xfc7/1, i586, Modrm|No_bSuf|No_wSuf|No_lSuf|No_sSuf|NoRex64|HLEPrefixLock, { Qword|Unspecified|BaseIndex }
>  
> @@ -3374,6 +3378,12 @@ cmp<cc>xadd, 0x66e<cc:opc>, APX_F(CMPCCXADD), Modrm|Vex|EVex128|Space0F38|Src2VV
>  
>  // WRMSRNS instruction.
>  
> +// MSR_IMM instructions' imm32 stands for the name of an model specific
> +// register (MSR). The imm32 needs a special handler in tc-i386.c, hence put
> +// wrmsrns of MSR_IMM firsty, since the logic is determined by is_cpu (...).
> +// Immediates want to be first; md_assemble() takes care of swapping operands
> +// accordingly.
> +wrmsrns, 0xf3f6/0, APX_F(MSR_IMM), Modrm|Vex128|xVexMap7|EVex128|VexW0|NoSuf, { Imm32, Reg64 }
>  wrmsrns, 0x0f01c6, WRMSRNS, NoSuf, {}
>  
>  // WRMSRNS instruction end.

Following on to my v1 comment here: I continue to disagree with this placement.
I now further disagree with the same verbose comment living in two places. If
the new insns need to come first, place them in a MSR_IMM section living ahead
of the similarly named further insns, allowing to have the comment just once.
(Also nit: s/firsty/first/ and perhaps also "needs special handling".) However,
there's absolutely no requirement that in tc-i386.c:i386_assemble() you use
is_cpu() just because that's used for the USER_MSR insns. There it's simply
easier / cheaper than comparing against MN_urdmsr and MN_uwrmsr. Whereas here
it may be better to indeed check the mnemonics, allowing the new entries to
live (once again) adjacent to the USER_MSR ones. Once put there, the WRMSRNS
comment regarding the imm placement can also simply reference UWRMSR's,
instead of fully reproducing it.

Jan