[PATCH] Support Intel USER_MSR
Hu, Lin1
lin1.hu@intel.com
Wed Oct 18 07:51:41 GMT 2023
Thanks for your review. I responded individually under each comment.
Attached is the modified version.
BRs,
Lin
-----Original Message-----
From: Jan Beulich <jbeulich@suse.com>
Sent: Monday, October 16, 2023 8:11 PM
To: Hu, Lin1 <lin1.hu@intel.com>
Cc: Lu, Hongjiu <hongjiu.lu@intel.com>; binutils@sourceware.org
Subject: Re: [PATCH] Support Intel USER_MSR
On 10.10.2023 09:24, Hu, Lin1 wrote:
> @@ -3863,6 +3864,8 @@ build_vex_prefix (const insn_template *t)
> case SPACE_0F:
> case SPACE_0F38:
> case SPACE_0F3A:
> + case SPACE_EVEXMAP5:
> + case SPACE_VEXMAP7:
> i.vex.bytes[0] = 0xc4;
> break;
> I can see the need for the latter line you add, but why the former?
> (If it is needed for some reason, this is a strong hint at the description being overly brief.)
I think it's a wrong that is made by reordering patches. I have removed it.
> @@ -8752,6 +8755,18 @@ build_modrm_byte (void)
> source = v;
> v = tmp;
> }
> + if (i.tm.opcode_modifier.operandconstraint == SWAP_SOURCE_DEST)
> + {
> + if (dest == (unsigned int) ~0)
> + source = source ^ 1;
> + else
> + {
> + unsigned int tmp = source;
> +
> + source = dest;
> + dest = tmp;
> + }
> + }
> Why is this needed? There's only a single register operand in both affected insn forms (see comment below on the 2-register form).
Furthermore I think it would be easier if you "canonicalized" the early immediate to be the 1st operand, such that for all other purposes immediates remain first.
> As a cosmetic nit: Please have a blank line ahead of the if() block (if it needs to stay).
Indeed, I've only kept the part that deals with a single register. Do you mean to complain to the person who designed the insn. Unfortunately, that's impossible.
> --- /dev/null
> +++ b/gas/testsuite/gas/i386/user_msr-inval.s
> @@ -0,0 +1,7 @@
> +# Check Illegal 32bit USER_MSR instructions
> +
> + .allow_index_reg
> + .text
> +_start:
> + urdmsr %r12, %r14 #USER_MSR
> + uwrmsr %r12, %r14 #USER_MSR
> As per comments on earlier series: What use are the comments here?
> (Applicable also again below.)
I have removed it.
> --- /dev/null
> +++ b/gas/testsuite/gas/i386/x86-64-user_msr.s
> @@ -0,0 +1,15 @@
> +# Check 64bit USER_MSR instructions
> +
> + .allow_index_reg
> + .text
> +_start:
> + urdmsr %r14, %r12 #USER_MSR
> + urdmsr $51515151, %r12 #USER_MSR
> + uwrmsr %r12, %r14 #USER_MSR
> + uwrmsr %r12, $51515151 #USER_MSR
> +
> +.intel_syntax noprefix
> Nit: Please indent directives.
Have removed these comments.
> + urdmsr r12, r14 #USER_MSR
> + urdmsr r12, 51515151 #USER_MSR
> + uwrmsr r14, r12 #USER_MSR
> + uwrmsr 51515151, r12 #USER_MSR
> I think varying registers slightly more (such that each two-register form has one low-8 and one high-8 operand, totaling to two forms each to prove that the REX.[RB] bits are also correctly dealt with) would be better.
I have added some other tests here.
> Btw, what's the interaction here with APX? The legacy forms are going to use REX2, but the VEX forms would need EVEX variants then.
Not at the moment. If it gets added to the documentation later, I'll add its.
> @@ -618,6 +620,8 @@ enum
> w_mode,
> /* double word operand */
> d_mode,
> + /* double word operand 0 */
> + d_0_mode,
> Why is this needed? IOW why does d_mode not do? Or alternatively why isn't this a name indicating that it's an unsigned 32-bit value (as opposed to other 32-bit immediates in 64-bit mode)?
I want to output imm32 first, but modrm byte is behind imm32, so I need to skip modrm for the moment. If I use { "uwrmsr", { Id, Eq }, 0} Id will read modrm byte. The mode is for a imm32 not an unsigned 32-bit value, so its name doesn't indicate that it's an unsigned integer.
> @@ -845,6 +849,7 @@ enum
> REG_VEX_0FAE,
> REG_VEX_0F3849_X86_64_L_0_W_0_M_1_P_0,
> REG_VEX_0F38F3_L_0,
> + REG_VEX_MAP7_F8_L_0_W_0_M_1,
>
> REG_XOP_09_01_L_0,
> REG_XOP_09_02_L_0,
> @@ -893,8 +898,10 @@ enum
> MOD_0FC7_REG_6,
> MOD_0FC7_REG_7,
> MOD_0F38DC_PREFIX_1,
> + MOD_0F38F8,
>
> MOD_VEX_0F3849_X86_64_L_0_W_0,
> + MOD_VEX_MAP7_F8_L_0_W_0,
> };
> As before - no new mod_table[] entries please which don't have both branches populated.
Have removed it.
+ {
+ /* MOD_VEX_MAP7_F8_L_0_W_0 */
+ { Bad_Opcode },
+ { REG_TABLE (REG_VEX_MAP7_F8_L_0_W_0_M_1) },
+ },
> @@ -6791,6 +6839,297 @@ static const struct dis386 vex_table[][256] = {
> { Bad_Opcode },
> { Bad_Opcode },
> },
> + /* VEX_MAP7 */
> + {
> + /* 00 */
> + { Bad_Opcode },
> I wonder whether adding a full new table (rather than some special case
> code) is really a god use of space. Of course if you know that more of it will be populated in the not too distant future ...
I don't know, I'm just treating the new opcode_space MAP7 like other opcode_space.
> @@ -11248,6 +11609,20 @@ get32s (instr_info *ins, bfd_vma *res)
> return true;
> }
>
> +/* The function is used to get imm32, when imm32 is operand 0, and
> +ins only has 2 operands. */ static bool
> +get32_operand0 (instr_info *ins, bfd_vma *res) {
> +
> + if (!fetch_code (ins->info, ins->codep + 5))
> + return false;
> + *res = *(ins->codep++ + 1) & (bfd_vma) 0xff;
> + *res |= (*(ins->codep++ + 1) & (bfd_vma) 0xff) << 8;
> + *res |= (*(ins->codep++ + 1) & (bfd_vma) 0xff) << 16;
> + *res |= (*(ins->codep++ + 1) & (bfd_vma) 0xff) << 24;
> + return true;
> +}
> Instead of this (which assumes ModRM.mod == 3) I think you want to arrange for dealing with ModRM first. We already have OP_Skip_MODRM() for such needs, which you could use in a first "hidden" operand.
I want to output imm32 first, but modrm byte is behind imm32, so I need to skip modrm for the moment. but I can't use OP_Skip_MODRM to deal with my problem, If I use it, I should add ins->codep-- at the end of get32_operand0.
> @@ -3346,3 +3349,12 @@ erets, 0xf20f01ca, FRED|x64, NoSuf, {} eretu,
> 0xf30f01ca, FRED|x64, NoSuf, {}
>
> // FRED instructions end.
> +
> +// USER_MSR instructions.
> +
> +urdmsr, 0xf20f38f8, USER_MSR|x64,
> +Modrm|IgnoreSize|SwapSourceDest|NoSuf, { Reg64, Reg64 }
> Iirc RegMem is the attribute to use here, not any new one.
Indeed.
> +urdmsr, 0xf2f8/0, USER_MSR|x64,
> +Modrm|Vex128|VexMap7|VexW0|IgnoreSize|NoSuf, { Imm32S, Reg64 }
> This and ...
> +uwrmsr, 0xf30f38f8, USER_MSR|x64, Modrm|IgnoreSize|NoSuf, { Reg64,
> +Reg64 } uwrmsr, 0xf3f8/0, USER_MSR|x64,
> +Modrm|Vex128|VexMap7|VexW0|IgnoreSize|SwapSourceDest|NoSuf, { Reg64,
> +Imm32S }
> ... this needs to use Imm32, not Imm32S. I understand this is going to cause complications elsewhere, but we can't afford getting this wrong.
> Also in all forms I think you don't mean IgnoreSize, but NoRex64.
Have modified them.
Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Support-Intel-USER_MSR.patch
Type: application/octet-stream
Size: 240971 bytes
Desc: 0001-Support-Intel-USER_MSR.patch
URL: <https://sourceware.org/pipermail/binutils/attachments/20231018/17ba3145/attachment-0001.obj>
More information about the Binutils
mailing list