[PATCH v2 0/3] target/i386/tcg: fix REPScalar with memory operand

[PATCH v2 0/3] target/i386/tcg: fix REPScalar with memory operand

[Paolo Bonzini]

By the time validate_vex() runs, e->s2 is not used anymore and
decode->op[2].ot has already been filled in by decode_size().
Commit b7d4ec4832d ("target/i386/tcg: move VEX validation early",
2026-08-26) did the change from decode->op[2].ot to e->s2, but
did not move validate_vex() early enough.  Fix it, which also
allows to implement the VEX.LIG case that is needed for unary
AVX operations.

Paolo

Supersedes: <20260923124749.273446-1-pbonzini@redhat.com>

Paolo Bonzini (3):
  target/i386/tcg: extract validate_cr0_exceptions()
  target/i386/tcg: move validate_vex() before all operand decoding
  target/i386/tcg: clarify return value of decode_insn

 target/i386/tcg/decode-new.c.inc | 63 ++++++++++++++++++++------------
 1 file changed, 39 insertions(+), 24 deletions(-)

-- 
2.55.0

[PATCH 1/3] target/i386/tcg: extract validate_cr0_exceptions()

[Paolo Bonzini]

The checks on CR0.TS and CR0.EM must be done after all #UD checks,
so extract them to their own functions.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/tcg/decode-new.c.inc | 39 ++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/target/i386/tcg/decode-new.c.inc b/target/i386/tcg/decode-new.c.inc
index 459452b04e3..996e12d03d2 100644
--- a/target/i386/tcg/decode-new.c.inc
+++ b/target/i386/tcg/decode-new.c.inc
@@ -2742,23 +2742,39 @@ static bool validate_vex(DisasContext *s, X86DecodedInsn *decode)
         e->op2 != X86_TYPE_H && e->op2 != X86_TYPE_B) {
         goto illegal;
     }
-
-    if (s->flags & HF_TS_MASK) {
-        goto nm_exception;
-    }
-    if (s->flags & HF_EM_MASK) {
-        goto illegal;
-    }
     return true;
 
-nm_exception:
-    gen_NM_exception(s);
-    return false;
 illegal:
     gen_illegal_opcode(s);
     return false;
 }
 
+/* Called after decode_ops, so that #UD for invalid encodings has priority over #NM.  */
+static bool validate_cr0_exceptions(DisasContext *s, X86DecodedInsn *decode)
+{
+    switch (decode->e.vex_class) {
+    case 0:
+    case 13:
+    case X86_EVEX_APX:
+    case X86_EVEX_APX_cmp:
+    case X86_EVEX_APX_pp2:
+    case X86_EVEX_APX_zu:
+        return true;
+    default:
+        break;
+    }
+
+    if (s->flags & HF_TS_MASK) {
+        gen_NM_exception(s);
+        return false;
+    }
+    if (s->flags & HF_EM_MASK) {
+        gen_illegal_opcode(s);
+        return false;
+    }
+    return true;
+}
+
 static void dump_unknown_opcode(CPUX86State *env, DisasContext *s)
 {
     if (qemu_loglevel_mask(LOG_UNIMP)) {
@@ -3037,6 +3053,9 @@ static void disas_insn(DisasContext *s, CPUState *cpu)
     if (!decode_ops(s, env, decode_func, &decode)) {
         goto illegal_op;
     }
+    if (!validate_cr0_exceptions(s, &decode)) {
+        return;
+    }
 
     switch (decode.e.special) {
     case X86_SPECIAL_None:
-- 
2.55.0

Re: [PATCH 1/3] target/i386/tcg: extract validate_cr0_exceptions()

[Zhao Liu]

On Wed, Sep 30, 2026 at 08:54:04AM +0200, Paolo Bonzini wrote:
> Date: Wed, 30 Sep 2026 08:54:04 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 1/3] target/i386/tcg: extract validate_cr0_exceptions()
> X-Mailer: git-send-email 2.55.0
> 
> The checks on CR0.TS and CR0.EM must be done after all #UD checks,
> so extract them to their own functions.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/i386/tcg/decode-new.c.inc | 39 ++++++++++++++++++++++++--------
>  1 file changed, 29 insertions(+), 10 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

[PATCH 2/3] target/i386/tcg: move validate_vex() before all operand decoding

[Paolo Bonzini]

Operand sizes are computed in decode_insn(), because s->rip_offset has to
be known before decode_modrm() computes a RIP-relative address.  However,
validate_vex() runs later and, for X86_VEX_REPScalar instructions, it must
be able to turn e->s2 into X86_SIZE_ss or X86_SIZE_sd in presence of a
memory operand.  But right now decode_op_size() has already run, and
therefore decode->op[2].ot stays MO_128; the instruction therefore
accesses 16 bytes of memory instead of 4 or 8.

The only user of s->rip_offset is decode_modrm(), which is called from
decode_ops(), so the size computation can move there, after validate_vex().
Thanks to the previous patch, the CR0.TS and CR0.EM checks come after
decode_ops(), so #UD for an invalid VEX.L still has priority over #NM.

This way, when validate_vex() adjusts the X86OpEntry, decode_op_size()
picks up the result.  This behaves as intended by commit b7d4ec4832d
("target/i386/tcg: move VEX validation early", 2026-08-26), and fixes
REPScalar instructions that have a memory operand.

This also applies to unary operations, which need to ignore VEX.L.
While this could also be fixed by using "dq" for the operand size, it
is untidy that simply setting "s->vex_l = 0" does not do the right thing.

Fixes: b7d4ec4832d ("target/i386/tcg: move VEX validation early", 2026-08-26)
Reported-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/tcg/decode-new.c.inc | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/target/i386/tcg/decode-new.c.inc b/target/i386/tcg/decode-new.c.inc
index 996e12d03d2..5212a24e470 100644
--- a/target/i386/tcg/decode-new.c.inc
+++ b/target/i386/tcg/decode-new.c.inc
@@ -2478,9 +2478,13 @@ static bool decode_insn(DisasContext *s, CPUX86State *env, X86DecodeFunc decode_
         e->decode(s, env, e, &decode->b);
     }
 
-    if (!validate_sse_prefix(s, e)) {
-        return false;
-    }
+    return validate_sse_prefix(s, e);
+}
+
+static bool decode_ops(DisasContext *s, CPUX86State *env, X86DecodeFunc decode_func,
+                       X86DecodedInsn *decode)
+{
+    X86OpEntry *e = &decode->e;
 
     /* Compute size of operands here in order to initialize s->rip_offset.  */
     if (e->op0 != X86_TYPE_None) {
@@ -2515,13 +2519,6 @@ static bool decode_insn(DisasContext *s, CPUX86State *env, X86DecodeFunc decode_
         assert(e->op3 == X86_TYPE_I && e->s3 == X86_SIZE_b);
         s->rip_offset += 1;
     }
-    return true;
-}
-
-static bool decode_ops(DisasContext *s, CPUX86State *env, X86DecodeFunc decode_func,
-                       X86DecodedInsn *decode)
-{
-    X86OpEntry *e = &decode->e;
 
     if (e->op0 != X86_TYPE_None &&
         !decode_op(s, env, decode, &decode->op[0], e->op0, decode->b)) {
-- 
2.55.0

Re: [PATCH 2/3] target/i386/tcg: move validate_vex() before all operand decoding

[Zhao Liu]

On Wed, Sep 30, 2026 at 08:54:05AM +0200, Paolo Bonzini wrote:
> Date: Wed, 30 Sep 2026 08:54:05 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 2/3] target/i386/tcg: move validate_vex() before all
>  operand decoding
> X-Mailer: git-send-email 2.55.0
> 
> Operand sizes are computed in decode_insn(), because s->rip_offset has to
> be known before decode_modrm() computes a RIP-relative address.  However,
> validate_vex() runs later and, for X86_VEX_REPScalar instructions, it must
> be able to turn e->s2 into X86_SIZE_ss or X86_SIZE_sd in presence of a
> memory operand.  But right now decode_op_size() has already run, and
> therefore decode->op[2].ot stays MO_128; the instruction therefore
> accesses 16 bytes of memory instead of 4 or 8.
> 
> The only user of s->rip_offset is decode_modrm(), which is called from
> decode_ops(), so the size computation can move there, after validate_vex().
> Thanks to the previous patch, the CR0.TS and CR0.EM checks come after
> decode_ops(), so #UD for an invalid VEX.L still has priority over #NM.
> 
> This way, when validate_vex() adjusts the X86OpEntry, decode_op_size()
> picks up the result.  This behaves as intended by commit b7d4ec4832d
> ("target/i386/tcg: move VEX validation early", 2026-08-26), and fixes
> REPScalar instructions that have a memory operand.
> 
> This also applies to unary operations, which need to ignore VEX.L.
> While this could also be fixed by using "dq" for the operand size, it
> is untidy that simply setting "s->vex_l = 0" does not do the right thing.
> 
> Fixes: b7d4ec4832d ("target/i386/tcg: move VEX validation early", 2026-08-26)
> Reported-by: Zhao Liu <zhao1.liu@intel.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/i386/tcg/decode-new.c.inc | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

[PATCH 3/3] target/i386/tcg: clarify return value of decode_insn

[Paolo Bonzini]

It is clearer if decode_insn() returns whether a generator exists;
if it does not, the entry is empty and all other checks such as
validate_sse_prefix() or has_cpuid_feature() are moot.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/tcg/decode-new.c.inc | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/target/i386/tcg/decode-new.c.inc b/target/i386/tcg/decode-new.c.inc
index 5212a24e470..4e1cf0a8297 100644
--- a/target/i386/tcg/decode-new.c.inc
+++ b/target/i386/tcg/decode-new.c.inc
@@ -2478,7 +2478,7 @@ static bool decode_insn(DisasContext *s, CPUX86State *env, X86DecodeFunc decode_
         e->decode(s, env, e, &decode->b);
     }
 
-    return validate_sse_prefix(s, e);
+    return !!decode.e.gen;
 }
 
 static bool decode_ops(DisasContext *s, CPUX86State *env, X86DecodeFunc decode_func,
@@ -2984,12 +2984,11 @@ static void disas_insn(DisasContext *s, CPUState *cpu)
     decode.cc_op = -1;
     decode.b = b;
     if (!decode_insn(s, env, decode_func, &decode)) {
-        goto illegal_op;
-    }
-    if (!decode.e.gen) {
         goto unknown_op;
     }
-
+    if (!validate_sse_prefix(s, &decode.e)) {
+        goto illegal_op;
+    }
     if (!has_cpuid_feature(s, decode.e.cpuid)) {
         goto illegal_op;
     }
-- 
2.55.0

Re: [PATCH 3/3] target/i386/tcg: clarify return value of decode_insn

[Zhao Liu]

On Wed, Sep 30, 2026 at 08:54:06AM +0200, Paolo Bonzini wrote:
> Date: Wed, 30 Sep 2026 08:54:06 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 3/3] target/i386/tcg: clarify return value of decode_insn
> X-Mailer: git-send-email 2.55.0
> 
> It is clearer if decode_insn() returns whether a generator exists;
> if it does not, the entry is empty and all other checks such as
> validate_sse_prefix() or has_cpuid_feature() are moot.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/i386/tcg/decode-new.c.inc | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 3/3] target/i386/tcg: clarify return value of decode_insn

[Andrey Polivoda]

> It is clearer if decode_insn() returns whether a generator exists;
> if it does not, the entry is empty and all other checks such as
> validate_sse_prefix() or has_cpuid_feature() are moot.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/i386/tcg/decode-new.c.inc | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/target/i386/tcg/decode-new.c.inc b/target/i386/tcg/decode-new.c.inc
> index 5212a24e470..4e1cf0a8297 100644
> --- a/target/i386/tcg/decode-new.c.inc
> +++ b/target/i386/tcg/decode-new.c.inc
> @@ -2478,7 +2478,7 @@ static bool decode_insn(DisasContext *s, CPUX86State *env, X86DecodeFunc decode_
>          e->decode(s, env, e, &decode->b);
>      }
>
> -    return validate_sse_prefix(s, e);
> +    return !!decode.e.gen;
>  }
>
Hi Paolo,

This patch currently fails to compile with the error below:
In file included from ../target/i386/tcg/translate.c:3334:
../target/i386/tcg/decode-new.c.inc:2481:20: error: member reference
type 'X86DecodedInsn *' (aka 'struct X86DecodedInsn *') is a pointer;
did you mean to use '->'?
 2481 |     return !!decode.e.gen;
      |              ~~~~~~^
      |                    ->

Should be:
    return !!decode->e.gen;

Thanks,
Andrey