public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
From: "Cui, Lili" <lili.cui@intel.com>
To: "Beulich, Jan" <JBeulich@suse.com>
Cc: "Lu, Hongjiu" <hongjiu.lu@intel.com>,
	"ccoutant@gmail.com" <ccoutant@gmail.com>,
	"binutils@sourceware.org" <binutils@sourceware.org>
Subject: RE: [PATCH 1/8] Support APX GPR32 with rex2 prefix
Date: Thu, 9 Nov 2023 08:02:32 +0000	[thread overview]
Message-ID: <SJ0PR11MB5600117039F0E40CCFD38D5A9EAFA@SJ0PR11MB5600.namprd11.prod.outlook.com> (raw)
In-Reply-To: <5807de3c-c509-69df-cdaf-e334ab26bc5f@suse.com>

> Subject: Re: [PATCH 1/8] Support APX GPR32 with rex2 prefix
> 
> On 02.11.2023 12:29, Cui, Lili wrote:
> > @@ -269,9 +275,17 @@ struct dis_private {
> >        ins->rex_used |= REX_OPCODE;			\
> >    }
> >
> > +#define USED_REX2(value)				\
> > +  {							\
> > +    if ((ins->rex2 & value))				\
> > +      ins->rex2_used |= value;				\
> > +  }
> > +
> >
> >  #define EVEX_b_used 1
> 
> Nit: Please avoid (re)introducing double blank lines. Instead ...
> 

Done.

> >  #define EVEX_len_used 2
> > +/* M0 in rex2 prefix represents map0 or map1.  */ #define REX2_M 0x8
> 
> ... a blank line ahead of this insertion would be helpful.
> 

Done.

> > @@ -1872,23 +1888,23 @@ static const struct dis386 dis386[] = {
> > +  { "scasB",		{ AL, Yb }, PREFIX_REX2_ILLEGAL },
> > +  { "scasS",		{ eAX, Yv }, PREFIX_REX2_ILLEGAL },
> >    /* b0 */
> >    { "movB",		{ RMAL, Ib }, 0 },
> >    { "movB",		{ RMCL, Ib }, 0 },
> 
> Like in the i386-gen.c adjustments for row E look to be missing here, too.
> 

Added them, and also added bad testcase for row E.

> > @@ -2091,12 +2107,12 @@ static const struct dis386 dis386_twobyte[] = {
> >    { PREFIX_TABLE (PREFIX_0F2E) },
> >    { PREFIX_TABLE (PREFIX_0F2F) },
> >    /* 30 */
> > -  { "wrmsr",		{ XX }, 0 },
> > -  { "rdtsc",		{ XX }, 0 },
> > -  { "rdmsr",		{ XX }, 0 },
> > -  { "rdpmc",		{ XX }, 0 },
> > -  { "sysenter",		{ SEP }, 0 },
> > -  { "sysexit%LQ",	{ SEP }, 0 },
> > +  { "wrmsr",		{ XX }, PREFIX_REX2_ILLEGAL },
> > +  { "rdtsc",		{ XX }, PREFIX_REX2_ILLEGAL },
> > +  { "rdmsr",		{ XX }, PREFIX_REX2_ILLEGAL },
> > +  { "rdpmc",		{ XX }, PREFIX_REX2_ILLEGAL },
> > +  { "sysenter",		{ SEP }, PREFIX_REX2_ILLEGAL },
> > +  { "sysexit%LQ",	{ SEP }, PREFIX_REX2_ILLEGAL },
> >    { Bad_Opcode },
> >    { "getsec",		{ XX }, 0 },
> >    /* 38 */
> 
> Down from here row 8 also wants adjustment afaict.
> 

Done.

> > @@ -8289,6 +8313,7 @@ ckprefix (instr_info *ins)  {
> >    int i, length;
> >    uint8_t newrex;
> > +  unsigned char rex2_payload;
> 
> Please can this be restricted to the inner scope where it's used?
> 

Done.

> > @@ -9292,13 +9338,17 @@ print_insn (bfd_vma pc, disassemble_info
> *info, int intel_syntax)
> >        goto out;
> >      }
> >
> > -  if (*ins.codep == 0x0f)
> > +  /* M0 in rex2 prefix represents map0 or map1.  */  if (*ins.codep
> > + == 0x0f || (ins.rex2 & REX2_M))
> 
> I'm struggling with the M0 in the comment. DYM just M, or maybe REX2.M?
> 

Changed to REX2.M.

> Also is this, ...
> 
> >      {
> >        unsigned char threebyte;
> >
> > -      ins.codep++;
> > -      if (!fetch_code (info, ins.codep + 1))
> > -	goto fetch_error_out;
> > +      if (!ins.rex2)
> > +	{
> > +	  ins.codep++;
> > +	  if (!fetch_code (info, ins.codep + 1))
> > +	    goto fetch_error_out;
> > +	}
> >        threebyte = *ins.codep;
> >        dp = &dis386_twobyte[threebyte];
> >        ins.need_modrm = twobyte_has_modrm[threebyte];
> 
> ... all the way to here, really correct for d5 00 0f?
> 

I think the 0f here must indicate that it is the first byte of the legacy map1 instruction, meaning legacy map0 does not have 0f opcode. If this instruction has a rex2 prefix, rex2.w must be 1 and should be d5 80. If a bad binary does appear, our original code also has the same issue.

static const struct dis386 dis386[] = {
...
/ * 0f  */
{ Bad_Opcode },       /* 0x0f extended opcode escape */

> > @@ -9454,6 +9504,14 @@ print_insn (bfd_vma pc, disassemble_info *info,
> int intel_syntax)
> >        goto out;
> >      }
> >
> > +  if ((dp->prefix_requirement & PREFIX_REX2_ILLEGAL)
> > +      && ins.last_rex2_prefix >= 0)
> > +    {
> > +      i386_dis_printf (info, dis_style_text, "(bad)");
> > +      ret = ins.end_codep - priv.the_buffer;
> > +      goto out;
> > +    }
> > +
> >    switch (dp->prefix_requirement)
> >      {
> >      case PREFIX_DATA:
> > @@ -9468,6 +9526,7 @@ print_insn (bfd_vma pc, disassemble_info *info,
> int intel_syntax)
> >        ins.used_prefixes |= PREFIX_DATA;
> >        /* Fall through.  */
> >      case PREFIX_OPCODE:
> > +    case PREFIX_OPCODE | PREFIX_REX2_ILLEGAL:
> 
> May more robust to mask off PREFIX_REX2_ILLEGAL in the control expression
> of the switch()? Or else why don't you move the if() immediately ahead of the
> switch() into here, as a new case block?
> 

Changed it to

+  switch (dp->prefix_requirement & ~PREFIX_REX2_ILLEGAL)
     {
     case PREFIX_DATA:
       /* If only the data prefix is marked as mandatory, its absence renders
@@ -9600,7 +9599,7 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
       ins.used_prefixes |= PREFIX_DATA;
       /* Fall through.  */
     case PREFIX_OPCODE:
-    case PREFIX_OPCODE | PREFIX_REX2_ILLEGAL:


> > @@ -9513,6 +9572,13 @@ print_insn (bfd_vma pc, disassemble_info *info,
> int intel_syntax)
> >        && !ins.need_vex && ins.last_rex_prefix >= 0)
> >      ins.all_prefixes[ins.last_rex_prefix] = 0;
> >
> > +  /* Check if the REX2 prefix is used.  */
> > +  if (ins.last_rex2_prefix >= 0
> > +      && ((((ins.rex2 & 0x7) ^ (ins.rex2_used & 0x7)) == 0
> > +	   && (ins.rex2 & 0x7))
> 
> DYM ((ins.rex2 & 7) & ~(ins.rex2_used & 7)) != 0
>

Here's an example of a negative scenario, when ins.rex2 == 1 and ins.rex2_used == 1, we want to clear last_rex2_prefix, because it has egpr and we don't want to add {rex2} to it. 

> > +	  || dp == &bad_opcode))
> 
> What is this last part of the condition about? Other prefix zapping code
> doesn't have such.

Deleted it , there is no impact on current testcase. Don’t know what the author’s intention was at that time. Deleted it.

> 
> > +    ins.all_prefixes[ins.last_rex2_prefix] = 0;
> > +
> >    /* Check if the SEG prefix is used.  */
> >    if ((ins.prefixes & (PREFIX_CS | PREFIX_SS | PREFIX_DS | PREFIX_ES
> >  		       | PREFIX_FS | PREFIX_GS)) != 0 @@ -9541,7 +9607,10
> @@
> > print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
> >  	if (name == NULL)
> >  	  abort ();
> >  	prefix_length += strlen (name) + 1;
> > -	i386_dis_printf (info, dis_style_mnemonic, "%s ", name);
> > +	if (ins.all_prefixes[i] == REX2_OPCODE)
> > +	  i386_dis_printf (info, dis_style_mnemonic, "{%s} ", name);
> 
> Do braces really count as part of the mnemonic?
> 

Yes, rex2 prefix prefers to use mnemonic {rex2}, unlike rex prefix use rex, rex.B....

> > +	else
> > +	  i386_dis_printf (info, dis_style_mnemonic, "%s ", name);
> >        }
> 
> Aren't you at risk of wrongly printing a REX prefix here if the high 4 bits of the
> REX2 payload were all zero, but some of the low 4 bits turned out unused?
> 
  For d500 I think we should print rex2 for it. 

> > @@ -11086,8 +11155,11 @@ print_register (instr_info *ins, unsigned int
> reg, unsigned int rexmask,
> >      ins->illegal_masking = true;
> >
> >    USED_REX (rexmask);
> > +  USED_REX2 (rexmask);
> 
> Do both really need tracking separately? Whatever consumes REX.B will also
> consume REX2.B4, an so on.
> 
I was confused here, I think we only need to print {rex2} for the upper 4 bits == *000, which means egpr is not used and we need to use {rex2} to distinguish it from legacy encoding.  maybe we don’t need ((ins.rex2 & 0x7) ^ (ins.rex2_used & 0x7)) == 0, and nor USED_REX2 (rexmask). I intend to delete them.

+  /* Check if the REX2 prefix is used.  */
+  if (ins.last_rex2_prefix >= 0
+      && ((((ins.rex2 & 0x7) ^ (ins.rex2_used & 0x7)) == 0
+	   && (ins.rex2 & 0x7))

Thanks,
Lili

  reply	other threads:[~2023-11-09  8:02 UTC|newest]

Thread overview: 120+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-11-02 11:29 [PATCH v2 0/8] Support Intel APX EGPR Cui, Lili
2023-11-02 11:29 ` [PATCH 1/8] Support APX GPR32 with rex2 prefix Cui, Lili
2023-11-02 17:05   ` Jan Beulich
2023-11-03  6:20     ` Cui, Lili
2023-11-03 13:05     ` Jan Beulich
2023-11-03 14:19   ` Jan Beulich
2023-11-06 15:20     ` Cui, Lili
2023-11-06 16:08       ` Jan Beulich
2023-11-07  8:16         ` Cui, Lili
2023-11-07 10:43           ` Jan Beulich
2023-11-07 15:31             ` Cui, Lili
2023-11-07 15:43               ` Jan Beulich
2023-11-07 15:53                 ` Cui, Lili
2023-11-06 15:02   ` Jan Beulich
2023-11-07  8:06     ` Cui, Lili
2023-11-07 10:20       ` Jan Beulich
2023-11-07 14:32         ` Cui, Lili
2023-11-07 15:08           ` Jan Beulich
2023-11-06 15:39   ` Jan Beulich
2023-11-09  8:02     ` Cui, Lili [this message]
2023-11-09 10:52       ` Jan Beulich
2023-11-09 13:27         ` Cui, Lili
2023-11-09 15:22           ` Jan Beulich
2023-11-10  7:11             ` Cui, Lili
2023-11-10  9:14               ` Jan Beulich
2023-11-10  9:21                 ` Jan Beulich
2023-11-10 12:38                   ` Cui, Lili
2023-12-14 10:13                   ` Cui, Lili
2023-12-18 15:24                     ` Jan Beulich
2023-12-18 16:23                       ` H.J. Lu
2023-11-10  9:47                 ` Cui, Lili
2023-11-10  9:57                   ` Jan Beulich
2023-11-10 12:05                     ` Cui, Lili
2023-11-10 12:35                       ` Jan Beulich
2023-11-13  0:18                         ` Cui, Lili
2023-11-02 11:29 ` [PATCH 2/8] Created an empty EVEX_MAP4_ sub-table for EVEX instructions Cui, Lili
2023-11-02 11:29 ` [PATCH 3/8] Support APX GPR32 with extend evex prefix Cui, Lili
2023-11-02 11:29 ` [PATCH 4/8] Add tests for " Cui, Lili
2023-11-08  9:11   ` Jan Beulich
2023-11-15 14:56     ` Cui, Lili
2023-11-16  9:17       ` Jan Beulich
2023-11-16 15:34     ` Cui, Lili
2023-11-16 16:50       ` Jan Beulich
2023-11-17 12:42         ` Cui, Lili
2023-11-17 14:38           ` Jan Beulich
2023-11-22 13:40             ` Cui, Lili
2023-11-02 11:29 ` [PATCH 5/8] Support APX NDD Cui, Lili
2023-11-08 10:39   ` Jan Beulich
2023-11-20  1:19     ` Cui, Lili
2023-11-08 11:13   ` Jan Beulich
2023-11-20 12:36     ` Cui, Lili
2023-11-20 16:33       ` Jan Beulich
2023-11-22  7:46         ` Cui, Lili
2023-11-22  8:47           ` Jan Beulich
2023-11-22 10:45             ` Cui, Lili
2023-11-23 10:57               ` Jan Beulich
2023-11-23 12:14                 ` Cui, Lili
2023-11-24  6:56                 ` [PATCH v3 0/9] Support Intel APX EGPR Cui, Lili
2023-12-07  8:17                   ` Cui, Lili
2023-12-07  8:33                     ` Cui, Lili
2023-11-09  9:37   ` [PATCH 5/8] Support APX NDD Jan Beulich
2023-11-20  1:33     ` Cui, Lili
2023-11-20  8:19       ` Jan Beulich
2023-11-20 12:54         ` Cui, Lili
2023-11-20 16:43           ` Jan Beulich
2023-11-02 11:29 ` [PATCH 6/8] Support APX Push2/Pop2 Cui, Lili
2023-11-08 11:44   ` Jan Beulich
2023-11-08 12:52     ` Jan Beulich
2023-11-22  5:48     ` Cui, Lili
2023-11-22  8:53       ` Jan Beulich
2023-11-22 12:26         ` Cui, Lili
2023-11-09  9:57   ` Jan Beulich
2023-11-02 11:29 ` [PATCH 7/8] Support APX NDD optimized encoding Cui, Lili
2023-11-09 10:36   ` Jan Beulich
2023-11-10  5:43     ` Hu, Lin1
2023-11-10  9:54       ` Jan Beulich
2023-11-14  2:28         ` Hu, Lin1
2023-11-14 10:50           ` Jan Beulich
2023-11-15  2:52             ` Hu, Lin1
2023-11-15  8:57               ` Jan Beulich
2023-11-15  2:59             ` [PATCH][v3] " Hu, Lin1
2023-11-15  9:34               ` Jan Beulich
2023-11-17  7:24                 ` Hu, Lin1
2023-11-17  9:47                   ` Jan Beulich
2023-11-20  3:28                     ` Hu, Lin1
2023-11-20  8:34                       ` Jan Beulich
2023-11-14  2:58         ` [PATCH 1/2] Reorder APX insns in i386.tbl Hu, Lin1
2023-11-14 11:20           ` Jan Beulich
2023-11-15  1:49             ` Hu, Lin1
2023-11-15  8:52               ` Jan Beulich
2023-11-17  3:27                 ` Hu, Lin1
2023-11-02 11:29 ` [PATCH 8/8] Support APX JMPABS Cui, Lili
2023-11-09 12:59   ` Jan Beulich
2023-11-14  3:26     ` Hu, Lin1
2023-11-14 11:15       ` Jan Beulich
2023-11-24  5:40         ` Hu, Lin1
2023-11-24  7:21           ` Jan Beulich
2023-11-27  2:16             ` Hu, Lin1
2023-11-27  8:03               ` Jan Beulich
2023-11-27  8:46                 ` Hu, Lin1
2023-11-27  8:54                   ` Jan Beulich
2023-11-27  9:03                     ` Hu, Lin1
2023-11-27 10:32                       ` Jan Beulich
2023-12-04  7:33                         ` Hu, Lin1
2023-11-02 13:22 ` [PATCH v2 0/8] Support Intel APX EGPR Jan Beulich
2023-11-03 16:42   ` Cui, Lili
2023-11-06  7:30     ` Jan Beulich
2023-11-06 14:20       ` Cui, Lili
2023-11-06 14:44         ` Jan Beulich
2023-11-06 16:03           ` Cui, Lili
2023-11-06 16:10             ` Jan Beulich
2023-11-07  1:53               ` Cui, Lili
2023-11-07 10:11                 ` Jan Beulich
  -- strict thread matches above, loose matches on Subject: below --
2023-09-19 15:25 [PATCH 0/8] [RFC] " Cui, Lili
2023-09-19 15:25 ` [PATCH 1/8] Support APX GPR32 with rex2 prefix Cui, Lili
2023-09-21 15:27   ` Jan Beulich
2023-09-27 15:57     ` Cui, Lili
2023-09-21 15:51   ` Jan Beulich
2023-09-27 15:59     ` Cui, Lili
2023-09-28  8:02       ` Jan Beulich
2023-10-07  3:27         ` Cui, Lili

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=SJ0PR11MB5600117039F0E40CCFD38D5A9EAFA@SJ0PR11MB5600.namprd11.prod.outlook.com \
    --to=lili.cui@intel.com \
    --cc=JBeulich@suse.com \
    --cc=binutils@sourceware.org \
    --cc=ccoutant@gmail.com \
    --cc=hongjiu.lu@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).