Re: [PATCH][v4] Support Intel USER_MSR

[Jan Beulich <jbeulich@suse.com>]

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.

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

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

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

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

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

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

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

Jan