RE: [PATCH][v4] Support Intel USER_MSR

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

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, October 26, 2023 4:31 PM
> To: Hu, Lin1 <lin1.hu@intel.com>
> Cc: Lu, Hongjiu <hongjiu.lu@intel.com>; binutils@sourceware.org
> Subject: Re: [PATCH][v4] Support Intel USER_MSR
> 
> 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.

>
> > @@ -2517,12 +2519,14 @@ smallest_imm_type (offsetT num)
> >    else if (fits_in_signed_word (num) || fits_in_unsigned_word (num))
> >      {
> >        t.bitfield.imm16 = 1;
> > -      t.bitfield.imm32 = 1;
> > +      if (fits_in_unsigned_long (num))
> > +	t.bitfield.imm32 = 1;
> >        t.bitfield.imm32s = 1;
> >      }
> >    else if (fits_in_signed_long (num))
> >      {
> > -      t.bitfield.imm32 = 1;
> > +      if (fits_in_unsigned_long (num))
> > +	t.bitfield.imm32 = 1;
> >        t.bitfield.imm32s = 1;
> >      }
> 
> Same issue here then, if any.
> 
> > @@ -5235,6 +5240,17 @@ md_assemble (char *line)
> >    if (i.imm_operands)
> >      optimize_imm ();
> >
> > +  /* user_msr instructions can match Imm32 templates when
> > +     guess_suffix == QWORD_MNEM_SUFFIX.  */
> > +  if (t->mnem_off == MN_urdmsr)
> > +    i.types[0]
> > +      = operand_type_or (i.types[0],
> > +			 smallest_imm_type (i.op[0].imms->X_add_number));
> > +  if (t->mnem_off == MN_uwrmsr)
> > +    i.types[1]
> > +      = operand_type_or (i.types[1],
> > +			 smallest_imm_type (i.op[1].imms->X_add_number));
> 
> My respective comment on v3 was left entirely unaddressed?
>

It's a mistake, I forget to remove the part of the code.

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

>
> 
> > @@ -7566,6 +7585,18 @@ match_template (char mnem_suffix)
> >        break;
> >      }
> >
> > +  /* This pattern aims to put the unusually placed imm operand to a usual
> > +     place. The constraints are currently only adapted to uwrmsr, and may
> > +     need further tweaking when new similar instructions become
> > + available.  */  if (i.operands > 0
> 
> As said in reply to v3, can this please be "> 1"? There's no need to ...
> 
> > +      && !operand_type_check (operand_types[0], imm)
> > +      && operand_type_check (operand_types[i.operands - 1], imm))
> 
> ... rely on the combination of these two conditions to never be true when
> i.operands == 1.
> 
> Thinking about it, since operand_type_check() may - depending on what exact
> code the compiler generates - be comparibly expensive, how about
> 
>   if (i.imm_operands > 0 && i.imm_operands < i.operands
>       && operand_type_check (operand_types[i.operands - 1], imm))
> 
> instead?
> 

OK, thanks for your suggestion.

>
> > --- /dev/null
> > +++ b/gas/testsuite/gas/i386/x86-64-user_msr-inval.l
> > @@ -0,0 +1,7 @@
> > +.* Assembler messages:
> > +.*:6: Error: operand type mismatch for `urdmsr'.
> > +.*:7: Error: operand type mismatch for `urdmsr'.
> > +.*:8: Error: operand type mismatch for `urdmsr'.
> > +.*:9: Error: operand type mismatch for `urdmsr'.
> > +.*:10: Error: operand type mismatch for `uwrmsr'.
> > +.*:11: Error: operand type mismatch for `uwrmsr'.
> > diff --git a/gas/testsuite/gas/i386/x86-64-user_msr-inval.s
> > b/gas/testsuite/gas/i386/x86-64-user_msr-inval.s
> > new file mode 100644
> > index 00000000000..6aff469485b
> > --- /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。

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

> > @@ -8803,7 +8872,15 @@ get_valid_dis386 (const struct dis386 *dp,
> instr_info *ins)
> >        ins->need_vex = 3;
> >        ins->codep++;
> >        vindex = *ins->codep++;
> > -      dp = &vex_table[vex_table_index][vindex];
> > +      if (vex_table_index == VEX_MAP7)
> > +	{
> > +	  if (vindex == 0xf8)
> > +	    dp = &map7_f8_opcode;
> > +	  else
> > +	    dp = &bad_opcode;
> > +	}
> > +      else
> > +	dp = &vex_table[vex_table_index][vindex];
> 
> How about
> 
>       if (vex_table_index != VEX_MAP7)
> 	dp = &vex_table[vex_table_index][vindex];
>       else if (vindex == 0xf8)
> 	dp = &map7_f8_opcode;
>       else
> 	dp = &bad_opcode;
> 
> (i.e. common case first and overall less indentation)?
> 

Yes, it looks great.

>
> > @@ -11299,7 +11376,11 @@ OP_Skip_MODRM (instr_info *ins, int bytemode
> > ATTRIBUTE_UNUSED,
> >
> >    /* Skip mod/rm byte.  */
> >    MODRM_CHECK;
> > -  ins->codep++;
> > +  if (!ins->has_skipped_modrm)
> > +    {
> > +      ins->codep++;
> > +      ins->has_skipped_modrm = true;
> > +    }
> >    return true;
> >  }
> 
> I understand you need to set has_skipped_modrm here, but does this need to be
> conditional?
>

I just tend to keep them in line. I have remove the condition.

BRs,
Lin