[PATCH 1/2] x86: insn decoder: create artificial 3rd byte for 2-byte VEX

[PATCH 1/2] x86: insn decoder: create artificial 3rd byte for 2-byte VEX

[Denys Vlasenko]

Before this patch, users need to do this to fetch vex.vvvv:

        if (insn->vex_prefix.nbytes == 2) {
                vex_vvvv = ((insn->vex_prefix.bytes[1] >> 3) & 0xf) ^ 0xf;
        }
        if (insn->vex_prefix.nbytes == 3) {
                vex_vvvv = ((insn->vex_prefix.bytes[2] >> 3) & 0xf) ^ 0xf;
        }

Make it so that insn->vex_prefix.bytes[2] always contains vex.wvvvvLpp bits.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Frank Ch. Eigler <fche@redhat.com>
Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Cc: Jim Keniston <jkenisto@linux.vnet.ibm.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/lib/insn.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
index 54fcffe..829ca4c 100644
--- a/arch/x86/lib/insn.c
+++ b/arch/x86/lib/insn.c
@@ -163,6 +163,12 @@ found:
 				/* VEX.W overrides opnd_size */
 				insn->opnd_bytes = 8;
 		} else {
+			/*
+			 * For VEX2, fake VEX3-like byte#2.
+			 * Makes it easier to decode vex.W, vex.vvvv,
+			 * vex.L and vex.pp. Masking with 0x7f sets vex.W == 0.
+			 */
+			insn->vex_prefix.bytes[2] = b2 & 0x7f;
 			insn->vex_prefix.nbytes = 2;
 			insn->next_byte += 2;
 		}
-- 
1.8.1.4

[PATCH 2/2] x86: extend insn decoder to understand xop and evex prefixes

[Denys Vlasenko]

Since xop and evex prefixes are extensions of vex mechanism,
they have similar bit layouts, and they can never be combined
(an instruction can have only one of them),
(ab)use insn->vex_prefix to store data of xop and evex too.

Users will need to conditionalize on insn->vex_prefix.bytes[0]
instead of insn->vex_prefix.nbytes if they want to determine
which of vex(-like) prefixes are there.

Instead of adding more inattr bits for these prefixes, drop
VEX inattr bits and use VEX opcode values directly to detect them.
There is no point in having additional level of indirection here.
(And we are close to running out of inattr bits for prefixes).

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Frank Ch. Eigler <fche@redhat.com>
Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Cc: Jim Keniston <jkenisto@linux.vnet.ibm.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/inat.h          | 14 -----------
 arch/x86/include/asm/insn.h          |  6 +++++
 arch/x86/lib/insn.c                  | 47 +++++++++++++++++++++++++-----------
 arch/x86/lib/x86-opcode-map.txt      |  4 +--
 arch/x86/tools/gen-insn-attr-x86.awk |  2 --
 5 files changed, 41 insertions(+), 32 deletions(-)

diff --git a/arch/x86/include/asm/inat.h b/arch/x86/include/asm/inat.h
index 74a2e31..5aa6c05 100644
--- a/arch/x86/include/asm/inat.h
+++ b/arch/x86/include/asm/inat.h
@@ -45,9 +45,6 @@
 #define INAT_PFX_ADDRSZ	11	/* 0x67 */
 /* x86-64 REX prefix */
 #define INAT_PFX_REX	12	/* 0x4X */
-/* AVX VEX prefixes */
-#define INAT_PFX_VEX2	13	/* 2-bytes VEX prefix */
-#define INAT_PFX_VEX3	14	/* 3-bytes VEX prefix */
 
 #define INAT_LSTPFX_MAX	3
 #define INAT_LGCPFX_MAX	11
@@ -138,17 +135,6 @@ static inline int inat_last_prefix_id(insn_attr_t attr)
 		return attr & INAT_PFX_MASK;
 }
 
-static inline int inat_is_vex_prefix(insn_attr_t attr)
-{
-	attr &= INAT_PFX_MASK;
-	return attr == INAT_PFX_VEX2 || attr == INAT_PFX_VEX3;
-}
-
-static inline int inat_is_vex3_prefix(insn_attr_t attr)
-{
-	return (attr & INAT_PFX_MASK) == INAT_PFX_VEX3;
-}
-
 static inline int inat_is_escape(insn_attr_t attr)
 {
 	return attr & INAT_ESC_MASK;
diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
index 48eb30a..e098fec 100644
--- a/arch/x86/include/asm/insn.h
+++ b/arch/x86/include/asm/insn.h
@@ -83,6 +83,12 @@ struct insn {
 #define X86_REX_X(rex) ((rex) & 2)
 #define X86_REX_B(rex) ((rex) & 1)
 
+/* VEX and VEX-like multi-byte prefixes */
+#define X86_BYTE_VEX3  0xc4
+#define X86_BYTE_VEX2  0xc5
+#define X86_BYTE_XOP   0x8f
+#define X86_BYTE_EVEX  0x62
+
 /* VEX bit flags  */
 #define X86_VEX_W(vex)	((vex) & 0x80)	/* VEX3 Byte2 */
 #define X86_VEX_R(vex)	((vex) & 0x80)	/* VEX2/3 Byte1 */
diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
index 829ca4c..2351977 100644
--- a/arch/x86/lib/insn.c
+++ b/arch/x86/lib/insn.c
@@ -138,31 +138,34 @@ found:
 	}
 	insn->rex_prefix.got = 1;
 
-	/* Decode VEX prefix */
+	/* Decode VEX prefixes et al. Layouts are:
+	 * vex2:     c5    rvvvvLpp
+	 * vex3/xop: c4/8f rxbmmmmm wvvvvLpp
+	 * evex:     62    rxbR00mm wvvvv1pp zllBVaaa
+	 */
 	b = peek_next(insn_byte_t, insn);
-	attr = inat_get_opcode_attribute(b);
-	if (inat_is_vex_prefix(attr)) {
+	if (b == X86_BYTE_VEX2 || b == X86_BYTE_VEX3 ||
+	    b == X86_BYTE_EVEX ||
+	    b == X86_BYTE_XOP) {
 		insn_byte_t b2 = peek_nbyte_next(insn_byte_t, insn, 1);
-		if (!insn->x86_64) {
+		/*
+		 * XOP: If the modrm.reg bits are 000, it's POP reg/mem.
+		 */
+		if (b == X86_BYTE_XOP && X86_MODRM_REG(b2) == 0) {
+			goto vex_end;
+		}
+		else if (!insn->x86_64) {
 			/*
 			 * In 32-bits mode, if the [7:6] bits (mod bits of
 			 * ModRM) on the second byte are not 11b, it is
-			 * LDS or LES.
+			 * LDS, LES or BOUND.
 			 */
 			if (X86_MODRM_MOD(b2) != 3)
 				goto vex_end;
 		}
 		insn->vex_prefix.bytes[0] = b;
 		insn->vex_prefix.bytes[1] = b2;
-		if (inat_is_vex3_prefix(attr)) {
-			b2 = peek_nbyte_next(insn_byte_t, insn, 2);
-			insn->vex_prefix.bytes[2] = b2;
-			insn->vex_prefix.nbytes = 3;
-			insn->next_byte += 3;
-			if (insn->x86_64 && X86_VEX_W(b2))
-				/* VEX.W overrides opnd_size */
-				insn->opnd_bytes = 8;
-		} else {
+		if (b == X86_BYTE_VEX2) {
 			/*
 			 * For VEX2, fake VEX3-like byte#2.
 			 * Makes it easier to decode vex.W, vex.vvvv,
@@ -171,7 +174,23 @@ found:
 			insn->vex_prefix.bytes[2] = b2 & 0x7f;
 			insn->vex_prefix.nbytes = 2;
 			insn->next_byte += 2;
+			goto vex_end;
+		}
+		b2 = peek_nbyte_next(insn_byte_t, insn, 2);
+		insn->vex_prefix.bytes[2] = b2;
+		if (insn->x86_64 && X86_VEX_W(b2)) {
+			/* VEX.W overrides opnd_size */
+			insn->opnd_bytes = 8;
+		}
+		if (b == X86_BYTE_VEX3 || b == X86_BYTE_XOP) {
+			insn->vex_prefix.nbytes = 3;
+			insn->next_byte += 3;
+			goto vex_end;
 		}
+		b2 = peek_nbyte_next(insn_byte_t, insn, 3);
+		insn->vex_prefix.bytes[3] = b2;
+		insn->vex_prefix.nbytes = 4;
+		insn->next_byte += 4;
 	}
 vex_end:
 	insn->vex_prefix.got = 1;
diff --git a/arch/x86/lib/x86-opcode-map.txt b/arch/x86/lib/x86-opcode-map.txt
index 1a2be7c..9d8a964 100644
--- a/arch/x86/lib/x86-opcode-map.txt
+++ b/arch/x86/lib/x86-opcode-map.txt
@@ -137,7 +137,7 @@ AVXcode:
 # 0x60 - 0x6f
 60: PUSHA/PUSHAD (i64)
 61: POPA/POPAD (i64)
-62: BOUND Gv,Ma (i64)
+62: BOUND Gv,Ma (i64) | EVEX+3byte (Prefix)
 63: ARPL Ew,Gw (i64) | MOVSXD Gv,Ev (o64)
 64: SEG=FS (Prefix)
 65: SEG=GS (Prefix)
@@ -184,7 +184,7 @@ AVXcode:
 8c: MOV Ev,Sw
 8d: LEA Gv,M
 8e: MOV Sw,Ew
-8f: Grp1A (1A) | POP Ev (d64)
+8f: Grp1A (1A) | POP Ev (d64) | XOP+2byte (Prefix)
 # 0x90 - 0x9f
 90: NOP | PAUSE (F3) | XCHG r8,rAX
 91: XCHG rCX/r9,rAX
diff --git a/arch/x86/tools/gen-insn-attr-x86.awk b/arch/x86/tools/gen-insn-attr-x86.awk
index 093a892..b2db5ad 100644
--- a/arch/x86/tools/gen-insn-attr-x86.awk
+++ b/arch/x86/tools/gen-insn-attr-x86.awk
@@ -93,8 +93,6 @@ BEGIN {
 	prefix_num["SEG=GS"] = "INAT_PFX_GS"
 	prefix_num["SEG=SS"] = "INAT_PFX_SS"
 	prefix_num["Address-Size"] = "INAT_PFX_ADDRSZ"
-	prefix_num["VEX+1byte"] = "INAT_PFX_VEX2"
-	prefix_num["VEX+2byte"] = "INAT_PFX_VEX3"
 
 	clear_vars()
 }
-- 
1.8.1.4

Re: [PATCH 2/2] x86: extend insn decoder to understand xop and evex prefixes

[Masami Hiramatsu]

(2014/05/17 3:34), Denys Vlasenko wrote:
> Since xop and evex prefixes are extensions of vex mechanism,
> they have similar bit layouts, and they can never be combined
> (an instruction can have only one of them),
> (ab)use insn->vex_prefix to store data of xop and evex too.
> 
> Users will need to conditionalize on insn->vex_prefix.bytes[0]
> instead of insn->vex_prefix.nbytes if they want to determine
> which of vex(-like) prefixes are there.
> 
> Instead of adding more inattr bits for these prefixes, drop
> VEX inattr bits and use VEX opcode values directly to detect them.
> There is no point in having additional level of indirection here.
> (And we are close to running out of inattr bits for prefixes).

Thank you very much for trying this work :)
But sorry, Nak, I don't like to use the prefix byte directly.
I'd rather like to add additional inattr bits for them.
This is a drawback of what I've done by gen-insn-atter-x86.awk.
It is prefer that x86 instruction information should be hidden
in opcode map and awk decoder.

And also, we should expand x86-opcode-map.txt for new instructions.

> 
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Frank Ch. Eigler <fche@redhat.com>
> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
> Cc: Jim Keniston <jkenisto@linux.vnet.ibm.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Andi Kleen <andi@firstfloor.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> ---
>  arch/x86/include/asm/inat.h          | 14 -----------
>  arch/x86/include/asm/insn.h          |  6 +++++
>  arch/x86/lib/insn.c                  | 47 +++++++++++++++++++++++++-----------
>  arch/x86/lib/x86-opcode-map.txt      |  4 +--
>  arch/x86/tools/gen-insn-attr-x86.awk |  2 --
>  5 files changed, 41 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/x86/include/asm/inat.h b/arch/x86/include/asm/inat.h
> index 74a2e31..5aa6c05 100644
> --- a/arch/x86/include/asm/inat.h
> +++ b/arch/x86/include/asm/inat.h
> @@ -45,9 +45,6 @@
>  #define INAT_PFX_ADDRSZ	11	/* 0x67 */
>  /* x86-64 REX prefix */
>  #define INAT_PFX_REX	12	/* 0x4X */
> -/* AVX VEX prefixes */
> -#define INAT_PFX_VEX2	13	/* 2-bytes VEX prefix */
> -#define INAT_PFX_VEX3	14	/* 3-bytes VEX prefix */
>  
>  #define INAT_LSTPFX_MAX	3
>  #define INAT_LGCPFX_MAX	11
> @@ -138,17 +135,6 @@ static inline int inat_last_prefix_id(insn_attr_t attr)
>  		return attr & INAT_PFX_MASK;
>  }
>  
> -static inline int inat_is_vex_prefix(insn_attr_t attr)
> -{
> -	attr &= INAT_PFX_MASK;
> -	return attr == INAT_PFX_VEX2 || attr == INAT_PFX_VEX3;
> -}
> -
> -static inline int inat_is_vex3_prefix(insn_attr_t attr)
> -{
> -	return (attr & INAT_PFX_MASK) == INAT_PFX_VEX3;
> -}
> -
>  static inline int inat_is_escape(insn_attr_t attr)
>  {
>  	return attr & INAT_ESC_MASK;
> diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
> index 48eb30a..e098fec 100644
> --- a/arch/x86/include/asm/insn.h
> +++ b/arch/x86/include/asm/insn.h
> @@ -83,6 +83,12 @@ struct insn {
>  #define X86_REX_X(rex) ((rex) & 2)
>  #define X86_REX_B(rex) ((rex) & 1)
>  
> +/* VEX and VEX-like multi-byte prefixes */
> +#define X86_BYTE_VEX3  0xc4
> +#define X86_BYTE_VEX2  0xc5
> +#define X86_BYTE_XOP   0x8f
> +#define X86_BYTE_EVEX  0x62
> +
>  /* VEX bit flags  */
>  #define X86_VEX_W(vex)	((vex) & 0x80)	/* VEX3 Byte2 */
>  #define X86_VEX_R(vex)	((vex) & 0x80)	/* VEX2/3 Byte1 */
> diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
> index 829ca4c..2351977 100644
> --- a/arch/x86/lib/insn.c
> +++ b/arch/x86/lib/insn.c
> @@ -138,31 +138,34 @@ found:
>  	}
>  	insn->rex_prefix.got = 1;
>  
> -	/* Decode VEX prefix */
> +	/* Decode VEX prefixes et al. Layouts are:
> +	 * vex2:     c5    rvvvvLpp
> +	 * vex3/xop: c4/8f rxbmmmmm wvvvvLpp
> +	 * evex:     62    rxbR00mm wvvvv1pp zllBVaaa
> +	 */
>  	b = peek_next(insn_byte_t, insn);
> -	attr = inat_get_opcode_attribute(b);
> -	if (inat_is_vex_prefix(attr)) {
> +	if (b == X86_BYTE_VEX2 || b == X86_BYTE_VEX3 ||
> +	    b == X86_BYTE_EVEX ||
> +	    b == X86_BYTE_XOP) {
>  		insn_byte_t b2 = peek_nbyte_next(insn_byte_t, insn, 1);
> -		if (!insn->x86_64) {
> +		/*
> +		 * XOP: If the modrm.reg bits are 000, it's POP reg/mem.
> +		 */
> +		if (b == X86_BYTE_XOP && X86_MODRM_REG(b2) == 0) {
> +			goto vex_end;
> +		}
> +		else if (!insn->x86_64) {
>  			/*
>  			 * In 32-bits mode, if the [7:6] bits (mod bits of
>  			 * ModRM) on the second byte are not 11b, it is
> -			 * LDS or LES.
> +			 * LDS, LES or BOUND.
>  			 */
>  			if (X86_MODRM_MOD(b2) != 3)
>  				goto vex_end;
>  		}
>  		insn->vex_prefix.bytes[0] = b;
>  		insn->vex_prefix.bytes[1] = b2;
> -		if (inat_is_vex3_prefix(attr)) {
> -			b2 = peek_nbyte_next(insn_byte_t, insn, 2);
> -			insn->vex_prefix.bytes[2] = b2;
> -			insn->vex_prefix.nbytes = 3;
> -			insn->next_byte += 3;
> -			if (insn->x86_64 && X86_VEX_W(b2))
> -				/* VEX.W overrides opnd_size */
> -				insn->opnd_bytes = 8;
> -		} else {
> +		if (b == X86_BYTE_VEX2) {
>  			/*
>  			 * For VEX2, fake VEX3-like byte#2.
>  			 * Makes it easier to decode vex.W, vex.vvvv,
> @@ -171,7 +174,23 @@ found:
>  			insn->vex_prefix.bytes[2] = b2 & 0x7f;
>  			insn->vex_prefix.nbytes = 2;
>  			insn->next_byte += 2;
> +			goto vex_end;
> +		}
> +		b2 = peek_nbyte_next(insn_byte_t, insn, 2);
> +		insn->vex_prefix.bytes[2] = b2;
> +		if (insn->x86_64 && X86_VEX_W(b2)) {
> +			/* VEX.W overrides opnd_size */
> +			insn->opnd_bytes = 8;
> +		}
> +		if (b == X86_BYTE_VEX3 || b == X86_BYTE_XOP) {
> +			insn->vex_prefix.nbytes = 3;
> +			insn->next_byte += 3;
> +			goto vex_end;
>  		}
> +		b2 = peek_nbyte_next(insn_byte_t, insn, 3);
> +		insn->vex_prefix.bytes[3] = b2;
> +		insn->vex_prefix.nbytes = 4;
> +		insn->next_byte += 4;
>  	}
>  vex_end:
>  	insn->vex_prefix.got = 1;
> diff --git a/arch/x86/lib/x86-opcode-map.txt b/arch/x86/lib/x86-opcode-map.txt
> index 1a2be7c..9d8a964 100644
> --- a/arch/x86/lib/x86-opcode-map.txt
> +++ b/arch/x86/lib/x86-opcode-map.txt
> @@ -137,7 +137,7 @@ AVXcode:
>  # 0x60 - 0x6f
>  60: PUSHA/PUSHAD (i64)
>  61: POPA/POPAD (i64)
> -62: BOUND Gv,Ma (i64)
> +62: BOUND Gv,Ma (i64) | EVEX+3byte (Prefix)
>  63: ARPL Ew,Gw (i64) | MOVSXD Gv,Ev (o64)
>  64: SEG=FS (Prefix)
>  65: SEG=GS (Prefix)
> @@ -184,7 +184,7 @@ AVXcode:
>  8c: MOV Ev,Sw
>  8d: LEA Gv,M
>  8e: MOV Sw,Ew
> -8f: Grp1A (1A) | POP Ev (d64)
> +8f: Grp1A (1A) | POP Ev (d64) | XOP+2byte (Prefix)
>  # 0x90 - 0x9f
>  90: NOP | PAUSE (F3) | XCHG r8,rAX
>  91: XCHG rCX/r9,rAX
> diff --git a/arch/x86/tools/gen-insn-attr-x86.awk b/arch/x86/tools/gen-insn-attr-x86.awk
> index 093a892..b2db5ad 100644
> --- a/arch/x86/tools/gen-insn-attr-x86.awk
> +++ b/arch/x86/tools/gen-insn-attr-x86.awk
> @@ -93,8 +93,6 @@ BEGIN {
>  	prefix_num["SEG=GS"] = "INAT_PFX_GS"
>  	prefix_num["SEG=SS"] = "INAT_PFX_SS"
>  	prefix_num["Address-Size"] = "INAT_PFX_ADDRSZ"
> -	prefix_num["VEX+1byte"] = "INAT_PFX_VEX2"
> -	prefix_num["VEX+2byte"] = "INAT_PFX_VEX3"
>  
>  	clear_vars()
>  }
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH 2/2] x86: extend insn decoder to understand xop and evex prefixes

[Denys Vlasenko]

On 05/17/2014 06:00 PM, Masami Hiramatsu wrote:
> (2014/05/17 3:34), Denys Vlasenko wrote:
>> Since xop and evex prefixes are extensions of vex mechanism,
>> they have similar bit layouts, and they can never be combined
>> (an instruction can have only one of them),
>> (ab)use insn->vex_prefix to store data of xop and evex too.
>>
>> Users will need to conditionalize on insn->vex_prefix.bytes[0]
>> instead of insn->vex_prefix.nbytes if they want to determine
>> which of vex(-like) prefixes are there.
>>
>> Instead of adding more inattr bits for these prefixes, drop
>> VEX inattr bits and use VEX opcode values directly to detect them.
>> There is no point in having additional level of indirection here.
>> (And we are close to running out of inattr bits for prefixes).
> 
> Thank you very much for trying this work :)
> But sorry, Nak, I don't like to use the prefix byte directly.
> I'd rather like to add additional inattr bits for them.

Ok, I'm looking at inat.h and looks like I need to widen
INAT_PFX_BITS, otherwise I have no space for two more bits
for two more prefixes (XOP and EVEX):

/* AVX VEX prefixes */
#define INAT_PFX_VEX2   13      /* 2-bytes VEX prefix */
#define INAT_PFX_VEX3   14      /* 3-bytes VEX prefix */
...

/* Legacy prefix */
#define INAT_PFX_OFFS   0
#define INAT_PFX_BITS   4
#define INAT_PFX_MAX    ((1 << INAT_PFX_BITS) - 1)
#define INAT_PFX_MASK   (INAT_PFX_MAX << INAT_PFX_OFFS)

Re: [PATCH 2/2] x86: extend insn decoder to understand xop and evex prefixes

[Masami Hiramatsu]

(2014/05/20 0:04), Denys Vlasenko wrote:
> On 05/17/2014 06:00 PM, Masami Hiramatsu wrote:
>> (2014/05/17 3:34), Denys Vlasenko wrote:
>>> Since xop and evex prefixes are extensions of vex mechanism,
>>> they have similar bit layouts, and they can never be combined
>>> (an instruction can have only one of them),
>>> (ab)use insn->vex_prefix to store data of xop and evex too.
>>>
>>> Users will need to conditionalize on insn->vex_prefix.bytes[0]
>>> instead of insn->vex_prefix.nbytes if they want to determine
>>> which of vex(-like) prefixes are there.
>>>
>>> Instead of adding more inattr bits for these prefixes, drop
>>> VEX inattr bits and use VEX opcode values directly to detect them.
>>> There is no point in having additional level of indirection here.
>>> (And we are close to running out of inattr bits for prefixes).
>>
>> Thank you very much for trying this work :)
>> But sorry, Nak, I don't like to use the prefix byte directly.
>> I'd rather like to add additional inattr bits for them.
> 
> Ok, I'm looking at inat.h and looks like I need to widen
> INAT_PFX_BITS, otherwise I have no space for two more bits
> for two more prefixes (XOP and EVEX):

Yes, that's what I look forward! :)

> 
> /* AVX VEX prefixes */
> #define INAT_PFX_VEX2   13      /* 2-bytes VEX prefix */
> #define INAT_PFX_VEX3   14      /* 3-bytes VEX prefix */
> ...
> 
> /* Legacy prefix */
> #define INAT_PFX_OFFS   0
> #define INAT_PFX_BITS   4
> #define INAT_PFX_MAX    ((1 << INAT_PFX_BITS) - 1)
> #define INAT_PFX_MASK   (INAT_PFX_MAX << INAT_PFX_OFFS)
> 
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH 1/2] x86: insn decoder: create artificial 3rd byte for 2-byte VEX

[Masami Hiramatsu]

(2014/05/17 3:34), Denys Vlasenko wrote:
> Before this patch, users need to do this to fetch vex.vvvv:
> 
>         if (insn->vex_prefix.nbytes == 2) {
>                 vex_vvvv = ((insn->vex_prefix.bytes[1] >> 3) & 0xf) ^ 0xf;
>         }
>         if (insn->vex_prefix.nbytes == 3) {
>                 vex_vvvv = ((insn->vex_prefix.bytes[2] >> 3) & 0xf) ^ 0xf;
>         }
> 
> Make it so that insn->vex_prefix.bytes[2] always contains vex.wvvvvLpp bits.

I like this hack :)
If you don't mind, please add inline functions to get such vex bits from
struct insn too?

Thank you,

> 
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Frank Ch. Eigler <fche@redhat.com>
> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
> Cc: Jim Keniston <jkenisto@linux.vnet.ibm.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Andi Kleen <andi@firstfloor.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> ---
>  arch/x86/lib/insn.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
> index 54fcffe..829ca4c 100644
> --- a/arch/x86/lib/insn.c
> +++ b/arch/x86/lib/insn.c
> @@ -163,6 +163,12 @@ found:
>  				/* VEX.W overrides opnd_size */
>  				insn->opnd_bytes = 8;
>  		} else {
> +			/*
> +			 * For VEX2, fake VEX3-like byte#2.
> +			 * Makes it easier to decode vex.W, vex.vvvv,
> +			 * vex.L and vex.pp. Masking with 0x7f sets vex.W == 0.
> +			 */
> +			insn->vex_prefix.bytes[2] = b2 & 0x7f;
>  			insn->vex_prefix.nbytes = 2;
>  			insn->next_byte += 2;
>  		}
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH 1/2] x86: insn decoder: create artificial 3rd byte for 2-byte VEX

[Denys Vlasenko]

On 05/17/2014 05:59 PM, Masami Hiramatsu wrote:
> (2014/05/17 3:34), Denys Vlasenko wrote:
>> Before this patch, users need to do this to fetch vex.vvvv:
>>
>>         if (insn->vex_prefix.nbytes == 2) {
>>                 vex_vvvv = ((insn->vex_prefix.bytes[1] >> 3) & 0xf) ^ 0xf;
>>         }
>>         if (insn->vex_prefix.nbytes == 3) {
>>                 vex_vvvv = ((insn->vex_prefix.bytes[2] >> 3) & 0xf) ^ 0xf;
>>         }
>>
>> Make it so that insn->vex_prefix.bytes[2] always contains vex.wvvvvLpp bits.
> 
> I like this hack :)
> If you don't mind, please add inline functions to get such vex bits from
> struct insn too?

I prefer to add such functionality when the first user surfaces:
the needs of the user often dictate more suitable API.

In this case (VEX insns), the API isn't that straightforward.
For example, uprobe code narrowly escaped the need to detect maskmovq__
(0f f7) and maskmovdqu (66 0f f7 or equivalent VEX) instructions.

Let's assume we do need to detect them.
Merely fetching and looking at vex.mmmmm, vex.pp and opcode doesn't
really help that much in detecting these instructions:
with such simplistic help, user needs to check both VEX and legacy forms.

Perhaps a better API would be to form a word-sized "expanded opcode"
for vector and VEX insns. Something along the lines of:

[x.mmmmm.pp.opcode__]

where x bit indicates XOP insn (as opposed to VEX), pp indicate
none/66/F2/F3 prefixes, opcode__ is 8-bit opcode.
then checking for maskmovFOO insns would be easier.

Re: Re: [PATCH 1/2] x86: insn decoder: create artificial 3rd byte for 2-byte VEX

[Masami Hiramatsu]

(2014/05/19 23:58), Denys Vlasenko wrote:
> On 05/17/2014 05:59 PM, Masami Hiramatsu wrote:
>> (2014/05/17 3:34), Denys Vlasenko wrote:
>>> Before this patch, users need to do this to fetch vex.vvvv:
>>>
>>>         if (insn->vex_prefix.nbytes == 2) {
>>>                 vex_vvvv = ((insn->vex_prefix.bytes[1] >> 3) & 0xf) ^ 0xf;
>>>         }
>>>         if (insn->vex_prefix.nbytes == 3) {
>>>                 vex_vvvv = ((insn->vex_prefix.bytes[2] >> 3) & 0xf) ^ 0xf;
>>>         }
>>>
>>> Make it so that insn->vex_prefix.bytes[2] always contains vex.wvvvvLpp bits.
>>
>> I like this hack :)
>> If you don't mind, please add inline functions to get such vex bits from
>> struct insn too?
> 
> I prefer to add such functionality when the first user surfaces:
> the needs of the user often dictate more suitable API.

OK, then this should be postponed until the actual user (uprobes)
appears.

> In this case (VEX insns), the API isn't that straightforward.
> For example, uprobe code narrowly escaped the need to detect maskmovq__
> (0f f7) and maskmovdqu (66 0f f7 or equivalent VEX) instructions.
> 
> Let's assume we do need to detect them.
> Merely fetching and looking at vex.mmmmm, vex.pp and opcode doesn't
> really help that much in detecting these instructions:
> with such simplistic help, user needs to check both VEX and legacy forms.
> 
> Perhaps a better API would be to form a word-sized "expanded opcode"
> for vector and VEX insns. Something along the lines of:
> 
> [x.mmmmm.pp.opcode__]
> 
> where x bit indicates XOP insn (as opposed to VEX), pp indicate
> none/66/F2/F3 prefixes, opcode__ is 8-bit opcode.
> then checking for maskmovFOO insns would be easier.

I see, but it also makes things complex just for special
instructions. I'd like to let decoder just decode it, not
elaborate.

Thank you,

-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com