[PATCH][v4] Support Intel USER_MSR

[Jan Beulich]

On 26.10.2023 11:08, Hu, Lin1 wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Thursday, October 26, 2023 4:31 PM
>>
>> On 26.10.2023 08:21, Hu, Lin1 wrote:
>>> @@ -2504,7 +2505,8 @@ smallest_imm_type (offsetT num)
>>>  	t.bitfield.imm8 = 1;
>>>        t.bitfield.imm8s = 1;
>>>        t.bitfield.imm16 = 1;
>>> -      t.bitfield.imm32 = 1;
>>> +      if (fits_in_unsigned_long (num))
>>> +	t.bitfield.imm32 = 1;
>>>        t.bitfield.imm32s = 1;
>>>      }
>>
>> I fear this isn't correct for 32-bit code, where all immediates are deemed fitting
>> in both 32-bit signed and unsigned. Otoh you surely ran the testsuite, and I
>> would have expected mistakes here to be covered by at least one testcase.
>>
> 
> OK, so we might need special handling in places for cases where the operand of a USER_MSR instruction is negative, do you have a suggestion for where this should be handled, after match_template()?
> 
> PS. This part of change is for raise error when user input urdmsr  $-1, %r14.

Hmm, I realize even when avoiding optimize_imm() you still need to have
smallest_imm_type() capable of dealing with the case. That's fine, the
logic shouldn't be put elsewhere (which would only make things yet less
manageable). But outside of 64-bit code I think you want to set .imm32
without consulting fits_in_unsigned_long().

>>> @@ -6358,8 +6374,11 @@ optimize_imm (void)
>>>  				 smallest_imm_type (i.op[op].imms-
>>> X_add_number));
>>>
>>>  	    /* We must avoid matching of Imm32 templates when 64bit
>>> -	       only immediate is available.  */
>>> -	    if (guess_suffix == QWORD_MNEM_SUFFIX)
>>> +	       only immediate is available. user_msr instructions can
>>> +	       match Imm32 templates when guess_suffix ==
>> QWORD_MNEM_SUFFIX.
>>> +	    */
>>> +	    if (guess_suffix == QWORD_MNEM_SUFFIX
>>> +		&& !is_cpu(current_templates->start, CpuUSER_MSR))
>>>  	      i.types[op].bitfield.imm32 = 0;
>>>  	    break;
>>
>> Taking together the changes you make to smallest_imm_type() and the change
>> you make here, I guess - to come back to an earlier comment of yours - it would
>> be less risky if these changes were omitted and the new insns instead bypassed
>> optimize_imm(), as suggested before as an alternative.
> 
> For solve the problem of Imm32, I just need theses change without smallest_imm_type().
> 
> I want to make sure I'm not misunderstanding. For solving the Imm32 problem, do you mean you prefer
> 
> if (i.imm_operands)
> {
>     if (is_cpu(current_templates->start, CpuUSER_MSR))
>     {
>         for (op == i.operands; --op >= 0;)
>         {
>             if (operand_type_check (i.types[op], imm))
>             {
>                 i.types[op] = operand_type_or (i.types[op], 
>                                                 		 smallest_imm_type (i.op[op].imms->X_add_number));
>             }
>         }
>     }
>     else
>         optimize_imm();
> }
> 
> This part of the code is currently just a prototype.

Along these lines (and with a suitable comment), yes. I'm not sure,
btw, whether you really need operand_type_or(); I seems to me that you
could assign the return value directly to i.types[op]. But I may be
overlooking some particular case ...

>>> --- /dev/null
>>> +++ b/gas/testsuite/gas/i386/x86-64-user_msr-inval.s
>>> @@ -0,0 +1,11 @@
>>> +# Check Illegal 64bit USER_MSR instructions
>>> +
>>> +	.allow_index_reg
>>
>> Yet another instance of this when it's not needed?
>>
> 
> Since it looked to me like they were denied for the same reason, I'll add them。

Why do you say "add" here, ...

>>> --- /dev/null
>>> +++ b/gas/testsuite/gas/i386/x86-64-user_msr.s
>>> @@ -0,0 +1,43 @@
>>> +# Check 64bit USER_MSR instructions
>>> +
>>> +	.allow_index_reg
>>
>> Iirc I did ask to remove this, for being meaningless here. Please uniformly
>> remove this from all the new tests introduced here.
>>
> 
> OK, I have removed them.

... when here you say you removed them?

Jan