Re: [PATCH][v3] Support Intel USER_MSR

[Jan Beulich <jbeulich@suse.com>]

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.

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

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

> @@ -624,6 +629,8 @@ enum
>    d_swap_mode,
>    /* quad word operand */
>    q_mode,
> +  /* 64-byte MM operand */
> +  q_mm_mode,

Byte or rather bit?

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

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

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

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

Jan