Re: [PATCH v2 07/42] Enforce VEX encoding restrictions
On 4/25/22 00:01, Paul Brook wrote:
+/* If a VEX prefix is used then it must have V=b */
+#define CHECK_AVX_V0(s) do { \
+CHECK_AVX(s); \
+if ((s->prefix & PREFIX_VEX) && (s->vex_v != 0)) \
+goto illegal_op; \
+} while (0)
+
What do you think about
#define CHECK_AVX(s, flags) \
do {
if ((s->prefix & PREFIX_VEX) && !(env->hflags & HF_AVX_EN_MASK)) {
goto illegal_op;
}
if ((flags) & SSE_OPF_AVX2) {
CHECK_AVX2(s);
}
if ((flags) & SSE_OPF_AVX_128) {
CHECK_AVX_128(s);
}
if ((flags) & SSE_OPF_V0) {
CHECK_V0(s);
}
}
Macros such as CHECK_AVX_V0_128(s) would become CHECK_AVX(s, SSE_OPF_V0
| SSE_OPF_AVX_128); a bit longer but still bearable. And here you would
have:
case 0x210: /* movss xmm, ea */
if (mod != 3) {
+CHECK_AVX_V0_128(s);
gen_lea_modrm(env, s, modrm);
gen_op_ld_v(s, MO_32, s->T0, s->A0);
tcg_gen_st32_tl(s->T0, cpu_env,
@@ -3379,6 +3432,7 @@ static void gen_sse(CPUX86State *env, DisasContext *s,
int b,
tcg_gen_st32_tl(s->T0, cpu_env,
offsetof(CPUX86State,
xmm_regs[reg].ZMM_L(3)));
} else {
+CHECK_AVX_128(s);
CHECK_AVX(s, SSE_OPF_AVX_128);
if (mod != 3) {
CHECK_V0(s);
...
} else {
...
}
Another possibility is to add SSE_OPF_V0_MEM (i.e. V0 if mod != 3), and use
CHECK_AVX(s, SSE_OPF_AVX_128 | SSE_OPF_AVX_V0_MEM);
It's okay not to move _all_ flags checks in the macros, but for example
here:
+if (op6.ext_mask == CPUID_EXT_AVX
+&& (s->prefix & PREFIX_VEX) == 0) {
+goto illegal_op;
+}
+if (op6.flags & SSE_OPF_AVX2) {
+CHECK_AVX2(s);
+}
+
if (b1) {
+if (op6.flags & SSE_OPF_V0) {
+CHECK_AVX_V0(s);
+} else {
+CHECK_AVX(s);
+}
op1_offset = offsetof(CPUX86State,xmm_regs[reg]);
+
+if (op6.flags & SSE_OPF_MMX) {
+CHECK_AVX2_256(s);
+}
there is a lot of room for using a flags-extended CHECK_AVX macro.
Also, SSE_OPF_V0 seems overloaded, because it means depending on the
place in the code:
- always 2-operand
- 2-operand except if SCALAR && !CMP
- 2-operand except if SCALAR && !CMP && has REPZ/REPNZ prefixes
It is not clear to me if the former overlaps with the last (i.e. if
there are any SCALAR && !CMP operations that are always 2-operand). If
so, please use different constants for all three; if not, please use a
different constant for the last, e.g. SSE_OPF_V0 and SSE_OPF_VEC_V0, so
that the difference is visible in the flags-extended CHECK_AVX macro.
Also related to overloading, here and in patch 37 there is code like this:
+if (op7.flags & SSE_OPF_BLENDV && !(s->prefix & PREFIX_VEX)) {
+/* Only VEX encodings are valid for these blendv opcodes */
+goto illegal_op;
+}
If this is for all SSE_OPF_BLENDV operations, it can be handled in the
flags-enabled CHECK_AVX() macro above. If it is only for some, it
should be a new flag SSE_OPF_VEX_ONLY.
Finally (replying here just to keep things together), patch 29 has "We
abuse the SSE_OPF_SCALAR flag to select the memory operand width
appropriately". Please don't; use a separate function that takes in "b"
and returns a bool, with just a switch statement in it.
+CHECK_AVX(s);
+scalar_op = (s->prefix & PREFIX_VEX)
+&& (op7.flags & SSE_OPF_SCALAR)
+&& !(op7.flags & SSE_OPF_CMP);
+if (is_xmm && (op7.flags & SSE_OPF_MMX)) {
+CHECK_AVX2_256(s);
+}
I think the is_xmm check is always true here (inside case 0x03a: case
0x13a:, i.e. b is inside the 0x10..0x5f range)?
Paolo
Re: [PATCH v2 07/42] Enforce VEX encoding restrictions
On 4/24/22 15:01, Paul Brook wrote:
Add CHECK_AVX* macros, and use them to validate VEX encoded AVX instructions
All AVX instructions require both CPU and OS support, this is encapsulated
by HF_AVX_EN.
Some also require specific values in the VEX.L and VEX.V fields.
Some (mostly integer operations) also require AVX2
Signed-off-by: Paul Brook
---
target/i386/tcg/translate.c | 159 +---
1 file changed, 149 insertions(+), 10 deletions(-)
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 66ba690b7d..2f5cc24e0c 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -3185,10 +3185,54 @@ static const struct SSEOpHelper_table7
sse_op_table7[256] = {
goto illegal_op; \
} while (0)
+/*
+ * VEX encodings require AVX
+ * Allow legacy SSE encodings even if AVX not enabled
+ */
+#define CHECK_AVX(s) do { \
+if ((s->prefix & PREFIX_VEX) \
+&& !(env->hflags & HF_AVX_EN_MASK)) \
+goto illegal_op; \
+} while (0)
+
+/* If a VEX prefix is used then it must have V=b */
+#define CHECK_AVX_V0(s) do { \
+CHECK_AVX(s); \
+if ((s->prefix & PREFIX_VEX) && (s->vex_v != 0)) \
+goto illegal_op; \
+} while (0)
+
+/* If a VEX prefix is used then it must have L=0 */
+#define CHECK_AVX_128(s) do { \
+CHECK_AVX(s); \
+if ((s->prefix & PREFIX_VEX) && (s->vex_l != 0)) \
+goto illegal_op; \
+} while (0)
+
+/* If a VEX prefix is used then it must have V=b and L=0 */
+#define CHECK_AVX_V0_128(s) do { \
+CHECK_AVX(s); \
+if ((s->prefix & PREFIX_VEX) && (s->vex_v != 0 || s->vex_l != 0)) \
+goto illegal_op; \
+} while (0)
These predicates have some overlap, but awkwardly. It leaves you with cases
like
+if (op6.flags & SSE_OPF_V0) {
+CHECK_AVX_V0(s);
+} else {
+CHECK_AVX(s);
+}
this, where clearly the CHECK_AVX is common across the IF, and would be better
written as
CHECK_AVX(s);
if (flags & SSE_OPF_V0) {
CHECK_V0(s);
}
+CHECK_AVX(s);
+scalar_op = (s->prefix & PREFIX_VEX)
+&& (op7.flags & SSE_OPF_SCALAR)
+&& !(op7.flags & SSE_OPF_CMP);
+if (is_xmm && (op7.flags & SSE_OPF_MMX)) {
+CHECK_AVX2_256(s);
+}
+if (op7.flags & SSE_OPF_AVX2) {
+CHECK_AVX2(s);
+}
+if ((op7.flags & SSE_OPF_V0) && !scalar_op) {
+CHECK_AVX_V0(s);
+}
And these. Also, it would appear as if there's overlap between the AVX2 checks. Is this
clearer as
CHECK_AVX(s);
if (v0 && !scalar) {
CHECK_V0(s);
}
if ((flags & AVX2) || ((flags & MMX) && s->vex_l)) {
CHECK_AVX2(s);
}
and perhaps these could be broken out into helpers, so that
if (is_xmm) {
+scalar_op = (s->prefix & PREFIX_VEX)
+&& (sse_op.flags & SSE_OPF_SCALAR)
+&& !(sse_op.flags & SSE_OPF_CMP)
+&& (b1 == 2 || b1 == 3);
+/* VEX encoded scalar ops always have 3 operands! */
+if ((sse_op.flags & SSE_OPF_V0) && !scalar_op) {
+CHECK_AVX_V0(s);
+} else {
+CHECK_AVX(s);
+}
+if (sse_op.flags & SSE_OPF_MMX) {
+CHECK_AVX2_256(s);
+}
... you don't have to keep repeating stuff. This is where a better decoder
could really help.
r~
Re: [PATCH v2 07/42] Enforce VEX encoding restrictions
On 4/24/22 15:01, Paul Brook wrote:
+/*
+ * VEX encodings require AVX
+ * Allow legacy SSE encodings even if AVX not enabled
+ */
+#define CHECK_AVX(s) do { \
+if ((s->prefix & PREFIX_VEX) \
+&& !(env->hflags & HF_AVX_EN_MASK)) \
+goto illegal_op; \
+} while (0)
Likewise, fix coding style.
r~
[PATCH v2 07/42] Enforce VEX encoding restrictions
Add CHECK_AVX* macros, and use them to validate VEX encoded AVX instructions
All AVX instructions require both CPU and OS support, this is encapsulated
by HF_AVX_EN.
Some also require specific values in the VEX.L and VEX.V fields.
Some (mostly integer operations) also require AVX2
Signed-off-by: Paul Brook
---
target/i386/tcg/translate.c | 159 +---
1 file changed, 149 insertions(+), 10 deletions(-)
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 66ba690b7d..2f5cc24e0c 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -3185,10 +3185,54 @@ static const struct SSEOpHelper_table7
sse_op_table7[256] = {
goto illegal_op; \
} while (0)
+/*
+ * VEX encodings require AVX
+ * Allow legacy SSE encodings even if AVX not enabled
+ */
+#define CHECK_AVX(s) do { \
+if ((s->prefix & PREFIX_VEX) \
+&& !(env->hflags & HF_AVX_EN_MASK)) \
+goto illegal_op; \
+} while (0)
+
+/* If a VEX prefix is used then it must have V=b */
+#define CHECK_AVX_V0(s) do { \
+CHECK_AVX(s); \
+if ((s->prefix & PREFIX_VEX) && (s->vex_v != 0)) \
+goto illegal_op; \
+} while (0)
+
+/* If a VEX prefix is used then it must have L=0 */
+#define CHECK_AVX_128(s) do { \
+CHECK_AVX(s); \
+if ((s->prefix & PREFIX_VEX) && (s->vex_l != 0)) \
+goto illegal_op; \
+} while (0)
+
+/* If a VEX prefix is used then it must have V=b and L=0 */
+#define CHECK_AVX_V0_128(s) do { \
+CHECK_AVX(s); \
+if ((s->prefix & PREFIX_VEX) && (s->vex_v != 0 || s->vex_l != 0)) \
+goto illegal_op; \
+} while (0)
+
+/* 256-bit (ymm) variants require AVX2 */
+#define CHECK_AVX2_256(s) do { \
+if (s->vex_l && !(s->cpuid_7_0_ebx_features & CPUID_7_0_EBX_AVX2)) \
+goto illegal_op; \
+} while (0)
+
+/* Requires AVX2 and VEX encoding */
+#define CHECK_AVX2(s) do { \
+if ((s->prefix & PREFIX_VEX) == 0 \
+|| !(s->cpuid_7_0_ebx_features & CPUID_7_0_EBX_AVX2)) \
+goto illegal_op; \
+} while (0)
+
static void gen_sse(CPUX86State *env, DisasContext *s, int b,
target_ulong pc_start)
{
-int b1, op1_offset, op2_offset, is_xmm, val;
+int b1, op1_offset, op2_offset, is_xmm, val, scalar_op;
int modrm, mod, rm, reg;
struct SSEOpHelper_table1 sse_op;
struct SSEOpHelper_table6 op6;
@@ -3228,15 +3272,18 @@ static void gen_sse(CPUX86State *env, DisasContext *s,
int b,
gen_exception(s, EXCP07_PREX, pc_start - s->cs_base);
return;
}
-if (s->flags & HF_EM_MASK) {
-illegal_op:
-gen_illegal_opcode(s);
-return;
-}
-if (is_xmm
-&& !(s->flags & HF_OSFXSR_MASK)
-&& (b != 0x38 && b != 0x3a)) {
-goto unknown_op;
+/* VEX encoded instuctions ignore EM bit. See also CHECK_AVX */
+if (!(s->prefix & PREFIX_VEX)) {
+if (s->flags & HF_EM_MASK) {
+illegal_op:
+gen_illegal_opcode(s);
+return;
+}
+if (is_xmm
+&& !(s->flags & HF_OSFXSR_MASK)
+&& (b != 0x38 && b != 0x3a)) {
+goto unknown_op;
+}
}
if (b == 0x0e) {
if (!(s->cpuid_ext2_features & CPUID_EXT2_3DNOW)) {
@@ -3278,12 +3325,14 @@ static void gen_sse(CPUX86State *env, DisasContext *s,
int b,
case 0x1e7: /* movntdq */
case 0x02b: /* movntps */
case 0x12b: /* movntps */
+CHECK_AVX_V0(s);
if (mod == 3)
goto illegal_op;
gen_lea_modrm(env, s, modrm);
gen_sto_env_A0(s, offsetof(CPUX86State, xmm_regs[reg]));
break;
case 0x3f0: /* lddqu */
+CHECK_AVX_V0(s);
if (mod == 3)
goto illegal_op;
gen_lea_modrm(env, s, modrm);
@@ -3291,6 +3340,7 @@ static void gen_sse(CPUX86State *env, DisasContext *s,
int b,
break;
case 0x22b: /* movntss */
case 0x32b: /* movntsd */
+CHECK_AVX_V0_128(s);
if (mod == 3)
goto illegal_op;
gen_lea_modrm(env, s, modrm);
@@ -3321,6 +3371,7 @@ static void gen_sse(CPUX86State *env, DisasContext *s,
int b,
}
break;
case 0x16e: /* movd xmm, ea */
+CHECK_AVX_V0_128(s);
#ifdef TARGET_X86_64
if (s->dflag == MO_64) {
gen_ldst_modrm(env, s, modrm, MO_64, OR_TMP0, 0);
@@ -3356,6 +3407,7 @@ static void gen_sse(CPUX86State *env, DisasContext *s,
int b,
case 0x128: /* movapd */
case 0x16f: /* movdqa xmm, ea */
case 0x26f: /* movdqu xmm, ea */
+CHECK_AVX_V0(s);
if (mod != 3) {
gen_lea_modrm(env, s, modrm);
gen_ldo_env_A0(s, offsetof(CPUX86State, xmm_regs[reg]));
@@ -3367,6 +3419,7 @@ static void gen_sse(CPUX86State *env, DisasContext *s,
int b,
