[PATCH][v4] Support Intel USER_MSR
Hu, Lin1
lin1.hu@intel.com
Thu Oct 26 09:08:26 GMT 2023
> -----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
More information about the Binutils
mailing list