RE: [PATCH] Support Intel USER_MSR

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

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