[PATCH][v3] Support Intel USER_MSR
Hu, Lin1
lin1.hu@intel.com
Thu Oct 26 06:14:36 GMT 2023
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, October 25, 2023 7:43 PM
> To: Hu, Lin1 <lin1.hu@intel.com>
> Cc: Lu, Hongjiu <hongjiu.lu@intel.com>; binutils@sourceware.org
> Subject: Re: [PATCH][v3] Support Intel USER_MSR
>
> On 25.10.2023 11:11, Hu, Lin1 wrote:
> > @@ -5235,6 +5237,17 @@ md_assemble (char *line)
> > if (i.imm_operands)
>
> This if() ...
>
> > 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));
>
> ... should now enclose all of these additions as well. Both for performance
> reasons (insns without immediates can skip the extra
> conditionals) and to avoid latent issues (i.op[].imms is not valid to de-reference
> without first checking [or knowing by implication] that the respective operand
> actually is an immediate; considering we're ahead of template matching, that'll
> need some further adjustment here anyway).
>
> But then the question is - do you actually need to go through
> optimize_imm() for these two insns? Or, worse, is it perhaps even wrong to do
> so? It looks at least latently risky to me.
>
I am sorry for I didn't notice that current_template was a global variable, that part of the change has been removed by me, and the solution is now
@@ -6371,8 +6371,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;
Or you want USER_MSR not to go through optimize_imm(), I can extract the code that the USER_MSR should perform out of optimize_imm().
> > @@ -7566,6 +7579,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
> > + && i.tm.operand_types[0].bitfield.class == Reg
>
> This part is needlessly strict. Altogether I'd suggest that you check that you have
> more than one operand, the last is an immediate (as you ...
>
> > + && operand_type_check (i.tm.operand_types[i.operands - 1],
> > + imm))
>
> ... do already), and the first is not.
>
> Generated code wise the checks would likely be cheaper when done against the
> local variable operand_types[].
>
OK, I have modified them.
>
> > --- /dev/null
> > +++ b/gas/testsuite/gas/i386/x86-64-user_msr.s
> > @@ -0,0 +1,31 @@
> > +# Check 64bit USER_MSR instructions
> > +
> > + .allow_index_reg
>
> This doesn't look to have any meaning here.
>
> > + .text
> > +_start:
> > + urdmsr %r14, %r12
> > + urdmsr %r14, %rax
> > + urdmsr %rdx, %r12
> > + urdmsr %rdx, %rax
> > + urdmsr $51515151, %r12
> > + urdmsr $51515151, %rax
> > + uwrmsr %r12, %r14
> > + uwrmsr %rax, %r14
> > + uwrmsr %r12, %rdx
> > + uwrmsr %rax, %rdx
> > + uwrmsr %r12, $51515151
> > + uwrmsr %rax, $51515151
>
> Considering the special handling of immediates, may I ask that you check further
> values. E.g. 0x7f, 0x7fff, and 0x80000000? It may further be worthwhile to have
> another testcase checking that out of range values (negative or too large) are
> properly rejected.
>
Ok, I have added some tests.
>
> > @@ -624,6 +629,8 @@ enum
> > d_swap_mode,
> > /* quad word operand */
> > q_mode,
> > + /* 64-byte MM operand */
> > + q_mm_mode,
>
> Byte or rather bit?
Oh, It's a mistake.
>
> > @@ -1240,6 +1252,7 @@ enum
> > X86_64_VEX_0F38ED,
> > X86_64_VEX_0F38EE,
> > X86_64_VEX_0F38EF,
> > + X86_64_VEX_MAP7_F8_L_0_W_0_R_0,
> > };
>
> As you can observe from e.g. the change you're making here, ...
>
> > @@ -1259,7 +1272,8 @@ enum
> > {
> > VEX_0F = 0,
> > VEX_0F38,
> > - VEX_0F3A
> > + VEX_0F3A,
> > + VEX_MAP7
> > };
>
> ... it is beneficial to have a trailing comma in enumeration which may further be
> extended.
>
OK.
>
> > @@ -8803,7 +8872,12 @@ 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 && vindex == 0xf8)
> > + {
> > + dp = &map7_f8_opcode;
> > + }
> > + else
> > + dp = &vex_table[vex_table_index][vindex];
>
> In the VEX_MAP7 case this is an out of bounds access now, isn't it?
>
Yes, you are right.
>
> > @@ -9130,6 +9204,7 @@ print_insn (bfd_vma pc, disassemble_info *info, int
> intel_syntax)
> > .last_rex_prefix = -1,
> > .last_seg_prefix = -1,
> > .fwait_prefix = -1,
> > + .has_skipped_modrm = 0,
> > };
>
> No need to add explicit initializers when the value is zero. Omitting the line also
> would save me from demanding that you use "false", not "0".
>
OK, I have removed the line.
>
> > @@ -10017,7 +10092,11 @@ dofloat (instr_info *ins, int sizeflag)
> > }
> > /* Skip mod/rm byte. */
> > MODRM_CHECK;
> > - ins->codep++;
> > + if (!ins->has_skipped_modrm)
> > + {
> > + ins->codep++;
> > + ins->has_skipped_modrm = true;
> > + }
> >
> > dp = &float_reg[floatop - 0xd8][ins->modrm.reg];
> > if (dp->name == NULL)
> > @@ -11299,7 +11378,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;
> > }
> >
> > @@ -11818,7 +11901,11 @@ OP_E (instr_info *ins, int bytemode, int
> > sizeflag) {
> > /* Skip mod/rm byte. */
> > MODRM_CHECK;
> > - ins->codep++;
> > + if (!ins->has_skipped_modrm)
> > + {
> > + ins->codep++;
> > + ins->has_skipped_modrm = true;
> > + }
> >
> > if (ins->modrm.mod == 3)
> > {
> > @@ -12522,7 +12609,11 @@ OP_EM (instr_info *ins, int bytemode, int
> > sizeflag)
> >
> > /* Skip mod/rm byte. */
> > MODRM_CHECK;
> > - ins->codep++;
> > + if (!ins->has_skipped_modrm)
> > + {
> > + ins->codep++;
> > + ins->has_skipped_modrm = true;
> > + }
> > ins->used_prefixes |= (ins->prefixes & PREFIX_DATA);
> > reg = ins->modrm.rm;
> > if (ins->prefixes & PREFIX_DATA)
> > @@ -12558,7 +12649,11 @@ OP_EMC (instr_info *ins, int bytemode, int
> > sizeflag)
> >
> > /* Skip mod/rm byte. */
> > MODRM_CHECK;
> > - ins->codep++;
> > + if (!ins->has_skipped_modrm)
> > + {
> > + ins->codep++;
> > + ins->has_skipped_modrm = true;
> > + }
> > ins->used_prefixes |= (ins->prefixes & PREFIX_DATA);
> > oappend_register (ins, att_names_mm[ins->modrm.rm]);
> > return true;
> > @@ -12580,7 +12675,11 @@ OP_EX (instr_info *ins, int bytemode, int
> > sizeflag)
> >
> > /* Skip mod/rm byte. */
> > MODRM_CHECK;
> > - ins->codep++;
> > + if (!ins->has_skipped_modrm)
> > + {
> > + ins->codep++;
> > + ins->has_skipped_modrm = true;
> > + }
> >
> > if (bytemode == dq_mode)
> > bytemode = ins->vex.w ? q_mode : d_mode;
>
> Do you really need all of these adjustments? The only place I can see it's needed
> is ...
>
> > @@ -12623,9 +12722,10 @@ OP_R (instr_info *ins, int bytemode, int sizeflag)
> > {
> > case d_mode:
> > case dq_mode:
> > + case q_mode:
> > case mask_mode:
> > return OP_E (ins, bytemode, sizeflag);
>
> ... OP_E() for now. Otherwise, if you really want to do it uniformly, I think you'd
> want to fold this into MODRM_CHECK, such that the same code doesn't need
> repeating a whopping 9 times.
>
Ok, I'm fine with both, in terms of saving time and lines of code, it's better to just add what's needed for now.
>
> Jan
BRs,
Lin
More information about the Binutils
mailing list