Re: [PATCH] Support Intel USER_MSR

[Jan Beulich <jbeulich@suse.com>]

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