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
next prev parent 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).