Re: [PATCH 8/8] Support APX JMPABS

[Jan Beulich <jbeulich@suse.com>]

On 14.11.2023 04:26, Hu, Lin1 wrote:
>  > On 02.11.2023 12:29, Cui, Lili wrote:
>>> --- a/gas/config/tc-i386.c
>>> +++ b/gas/config/tc-i386.c
>>> @@ -7790,7 +7790,8 @@ match_template (char mnem_suffix)
>>>    if (!quiet_warnings)
>>>      {
>>>        if (!intel_syntax
>>> -	  && (i.jumpabsolute != (t->opcode_modifier.jump ==
>> JUMP_ABSOLUTE)))
>>> +	  && (i.jumpabsolute != (t->opcode_modifier.jump == JUMP_ABSOLUTE))
>>> +	  && t->mnem_off != MN_jmpabs)
>>>  	as_warn (_("indirect %s without `*'"), insn_name (t));
>>
>> Coming back to this, which I did comment on before already. The insn taking an
>> immediate operand doesn't really justify this, as it leaves open the underlying
>> question of why you use JumpAbsolute in the insn template in the first place. I've
>> gone through all the uses of JUMP_ABSOLUTE, and I didn't find any where the
>> respective handling would be applicable here. In fact it's unclear whether the
>> insn needs marking as any JUMP_* at all: It's not really different from, say, "mov
>> $imm, %rcx".
>>
> 
> According to the doc JMPABS is a 64-bit only ISA extension, and acts as a near-direct branch with an absolute target. I made this markup simply because I was mimicking other jmp's.

"absolute target" can have different meaning. There's nothing wrong with the
linker establishing the ultimate value; it may well not be an assembly-time
constant. In terms of ELF relocations it simply wouldn't be R_X86_64_PC64
(resembling R_X86_64_PC32 used for other direct branches), but R_X86_64_64.

> If we don't need the attribute, I'm glad I can remove it.

Please do, unless you can explain why you add it.

>> There's a further question regarding its operand representation,
>> though: Can you explain why it's Imm64, not Disp64? The latter would, to me,
>> seem more natural to use here. Not only from a assembler internals perspective,
>> but also from the users' one: The $ in the operand carries absolutely no meaning
>> (see also the related testcase comment below) in AT&T syntax, and there's no
>> noticeable difference in Intel syntax afaict.
> 
> In my opinion, If compiler want  to jump "anywhere" and the displacement can not fit in a 32-bit immediate , compiler will fallback to indirect branches.

Unless it knows that it may use this ISA extension.

> My current knowledge is that jmpabs came about as a solution to the problem about indirect braches. It's not the same as the jmp. Currently the parameters of jmpabs are absolute addresses optimized by PLT or JIT. I think using imm64 avoids confusion with the disp64. That's why the designer designed it this way.
> 
> One colleague in our group have written an introductory document can be referred. (https://kanrobert.github.io/rfc/All-about-APX-JMPABS/)

Well, "Compiler usages" there ignores other than small-code-model programs.
Furthermore "none currently" doesn't mean the assembler shouldn't support
all possible uses. If I was going from what's said there, there wouldn't be
a need to support JMPABS in gas at all.

>>> @@ -8939,6 +8940,9 @@ process_operands (void)
>>>  	}
>>>      }
>>>
>>> +  if (i.tm.mnem_off == MN_jmpabs)
>>> +    i.rex2_encoding = true;
>>
>> Please see my earlier remarks wrt "rex2" vs "{rex2}". What you do here is effect
>> the latter. Yet as indicated, the pseudo-prefix isn't really an indication of "must
>> have REX2 prefix", but only a weak request to do so if possible. I think you want
>> to set i.rex2 here instead, requiring a way to express that an empty REX2 prefix
>> is wanted.
>>
> 
> But in terms of encoding, i.rex2 should be 0. Can I do special handling in build_rex2_prefix? 

build_rex2_prefix() wants to remain generic. What I was trying to hint at though
is that it ought to be possible to set bits in i.rex2 (to make it non-zero) which
then aren't encoded into the REX2 payload byte (leveraging that only the low
three bits are actually contributing to the final encoding). The important point
is that both i.rex2 and i.rex2_encoding retain the specific meaning they are
intended to have.

>>> --- /dev/null
>>> +++ b/gas/testsuite/gas/i386/x86-64-apx-jmpabs-inval.s
>>> @@ -0,0 +1,17 @@
>>> +# Check bytecode of APX_F jmpabs instructions with illegal encode.
>>> +
>>> +	.text
>>> +# With 66 prefix
>>> +	.byte
>> 0x66,0x64,0xd5,0x00,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
>>> +	.byte 0x66,0xd5,0x00,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
>>> +# With 67 prefix
>>> +	.byte
>> 0x67,0x64,0xd5,0x00,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
>>> +	.byte 0x67,0xd5,0x00,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
>>> +# With F2 prefix
>>> +	.byte
>> 0xf2,0x64,0xd5,0x00,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
>>> +	.byte 0xf2,0xd5,0x00,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
>>> +# With F3 prefix
>>> +	.byte
>> 0xf3,0x64,0xd5,0x00,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
>>> +	.byte 0xf3,0xd5,0x00,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
>>> +# REX2.M0 = 0 REX2.W = 1
>>> +	.byte 0xd5,0x08,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
>>
>> As per earlier comments: This wants expressing via .insn, to yield input to gas
>> human-readable (even if, as it looks, two .insn are going to be required per
>> resulting construct). Further in the last comment, why is
>> REX2.M0 mentioned there, but not elsewhere? Also what purpose serve the
>> 0x64 bytes here? The encodings are invalid irrespective of them. Instead I'd kind
>> have expected LOCK to also be covered.
>>
> 
> Because this error line is only for the special case where M0 == 0, and base_opcode == 0xa1, W should be 0, other than 1. If M0 = 1, W = 1, base_opcode == 0xa1, I think it could decoding as mov rax, moffs or ( some future insn). Elsewhere it's just excluding invalid prefixes.

Yet REX2.M == 0 is as relevant there (until such time where some of those
prefixes used is assigned meaning).

> I don't see in the docs that it triggers #UD, am I missing something?

What's "it" here?

>> Also a spec question as we're talking of what is or is not valid (i.e.
>> causing #UD) here: Why would XCR0.APX=0 need to cause #UD? There's no use
>> of eGPR-s here.
>>
> 
> Sorry, what is XCR0.APX?

Bit 19 of the XCR0 register. It is mentioned in exactly this way in the
APX-LEGACY-JMPABS exception class description.

>>> --- /dev/null
>>> +++ b/gas/testsuite/gas/i386/x86-64-apx-jmpabs.s
>>> @@ -0,0 +1,10 @@
>>> +# Check 64bit APX_F JMPABS instructions
>>> +
>>> +	.text
>>> + _start:
>>> +	jmpabs	      $0x0202020202020202
>>> +	jmpabs	      $0x2
>>> +
>>> +.intel_syntax noprefix
>>> +	jmpabs	      0x0202020202020202
>>> +	jmpabs	      0x2
>>
>> I expect this isn't going to be the normal use of the insn. Instead I would foresee
>> the typical users to be "jmpabs symbol" (and - as per above - intentionally
>> omitting the $ already). IOW the testcase also wants to cover the case requiring
>> a relocation, including a check that the correct relocation is emitted (covering
>> both ELF and COFF; I'm not going to insist on also covering Mach-O, as - for a
>> reason that escapes me - gas can't even be configured for x86_64-*-darwin*).
>>
> 
> Based on the previous discussion, we think that this usage is not currently supported. If users want to use symbol, I think they can use "jmp symbol".

See my respective comments above, the summary of which was "Then why add it to
gas in the first place?"

>>> --- a/opcodes/i386-dis.c
>>> +++ b/opcodes/i386-dis.c
>>> @@ -106,6 +106,7 @@ static bool MOVSXD_Fixup (instr_info *, int, int);
>>> static bool DistinctDest_Fixup (instr_info *, int, int);  static bool
>>> PREFETCHI_Fixup (instr_info *, int, int);  static bool
>>> PUSH2_POP2_Fixup (instr_info *, int, int);
>>> +static bool JMPABS_Fixup (instr_info *, int, int);
>>>
>>>  static void ATTRIBUTE_PRINTF_3 i386_dis_printf (const disassemble_info *,
>>>  						enum disassembler_style,
>>> @@ -258,6 +259,9 @@ struct instr_info
>>>    char scale_char;
>>>
>>>    enum x86_64_isa isa64;
>>> +
>>> +  /* Remember if the current op is jmpabs.  */  bool is_jmpabs;
>>>  };
>>
>> This field would probably best live next to op_is_jump (and then also be named
>> op_is_jmpabs, assuming a separate boolean is indeed needed).
>> I further expect that op_is_jump also wants setting for JMPABS.
>>
> 
> Can I change op_is_jump's type from bool to unsigned int?

If you need it to hold a 3rd value, perhaps. Albeit more to an enum then
than to unsigned int.

>>> @@ -2032,7 +2036,7 @@ static const struct dis386 dis386[] = {
>>>    { "lahf",		{ XX }, 0 },
>>>    /* a0 */
>>>    { "mov%LB",		{ AL, Ob }, PREFIX_REX2_ILLEGAL },
>>> -  { "mov%LS",		{ eAX, Ov }, PREFIX_REX2_ILLEGAL },
>>> +  { "mov%LS",		{ { JMPABS_Fixup, eAX_reg }, { JMPABS_Fixup,
>> v_mode } }, PREFIX_REX2_ILLEGAL },
>>>    { "mov%LB",		{ Ob, AL }, PREFIX_REX2_ILLEGAL },
>>>    { "mov%LS",		{ Ov, eAX }, PREFIX_REX2_ILLEGAL },
>>>    { "movs{b|}",		{ Ybr, Xb }, PREFIX_REX2_ILLEGAL },
>>> @@ -9648,7 +9652,7 @@ print_insn (bfd_vma pc, disassemble_info *info, int
>> intel_syntax)
>>>      }
>>>
>>>    if ((dp->prefix_requirement & PREFIX_REX2_ILLEGAL)
>>> -      && ins.last_rex2_prefix >= 0)
>>> +      && ins.last_rex2_prefix >= 0 && !ins.is_jmpabs)
>>>      {
>>>        i386_dis_printf (info, dis_style_text, "(bad)");
>>>        ret = ins.end_codep - priv.the_buffer; @@ -13857,3 +13861,38 @@
>>> PUSH2_POP2_Fixup (instr_info *ins, int bytemode, int sizeflag)
>>>
>>>    return OP_VEX (ins, bytemode, sizeflag);  }
>>> +
>>> +static bool
>>> +JMPABS_Fixup (instr_info *ins, int bytemode, int sizeflag) {
>>> +  if (ins->address_mode == mode_64bit
>>> +      && ins->last_rex2_prefix >= 0
>>> +      && (ins->rex2 & 0x80) == 0x0)
>>> +    {
>>> +      uint64_t op;
>>> +
>>> +      if (bytemode == eAX_reg)
>>> +	return true;
>>> +
>>> +      if (!get64 (ins, &op))
>>> +	return false;
>>> +
>>> +      if ((ins->prefixes & (PREFIX_OPCODE | PREFIX_ADDR)) != 0x0
>>> +	  || (ins->rex & REX_W) != 0x0)
>>> +	{
>>> +	  oappend (ins, "(bad)");
>>> +	  return true;
>>> +	}
>>> +
>>> +      ins->mnemonicendp = stpcpy (ins->obuf, "jmpabs");
>>> +      ins->all_prefixes[ins->last_rex2_prefix] = 0;
>>
>> This doesn't look right. REX2.{R,X,B}{3,4} set still want recording in the output. I
>> expect you may need to set a bit in rex2_used here, but how exactly that ought
>> to work depends on how comments on earlier patches are going to be
>> addressed. This may then also eliminate the need for ...
>>
>>> +      ins->is_jmpabs = true;
>>
>> ... this field, which likely will be covered by a more generic approach.
>>
>>
> 
> Then this part of the discussion, as well as the modifications, I will wait for the front patch to be finalized.

I suggested to Lili to go with the simplified approach for now, printing
just {rex2} but no details. Yet such minimal printing may still be needed
here then as well, consistent with what is (going to be) done in the
earlier patch. ("Consistent" as in: If nothing would be printed there,
printing nothing would be the goal here as well then.)

Jan