RE: [PATCH] Support Intel USER_MSR

["Hu, Lin1" <lin1.hu@intel.com>]

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