RE: [PATCH 8/8] Support APX JMPABS

["Hu, Lin1" <lin1.hu@intel.com>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, November 14, 2023 7:15 PM
> To: Hu, Lin1 <lin1.hu@intel.com>
> Cc: Lu, Hongjiu <hongjiu.lu@intel.com>; ccoutant@gmail.com;
> binutils@sourceware.org; Cui, Lili <lili.cui@intel.com>
> Subject: Re: [PATCH 8/8] Support APX JMPABS
> 
> On 14.11.2023 04:26, Hu, Lin1 wrote:
> >  > On 02.11.2023 12:29, Cui, Lili wrote:
> >> 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.
>

We've decided to only support disassembler for now, and for future support for assembler, we're going to wait for HJ to come back to see if linker can support the way "jmpabs symbols" are used.

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

I have set i.rex2 = 16;
 
>
> >>> --- /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 think so. the original result with W = 1 was MOV RAX, moffs, but the insn can't support REX2. So If someone input .byte REX2.M = 0 && REX2.W = 1, it should be a Bad_Opcode. 

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

I think XCR0.APX is a state bit to control if support APX instruction set not if support eGPR-s.
 
>
> >>> --- 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.
>

Ok, Have modified.
 
BRs,
Lin