[PATCH for-9.1 0/3] target/i386: Do not apply REX to MMX operands

[PATCH for-9.1 0/3] target/i386: Do not apply REX to MMX operands

[Richard Henderson]

Fixes #2495.

r~

Richard Henderson (3):
  target/i386: Do not apply REX to MMX operands
  target/i386: Use unit not type in decode_modrm
  target/i386: Assert MMX and XMM registers in range

 target/i386/tcg/decode-new.c.inc | 13 ++++++++-----
 target/i386/tcg/emit.c.inc       |  9 +++++++--
 2 files changed, 15 insertions(+), 7 deletions(-)

-- 
2.43.0

[PATCH 1/3] target/i386: Do not apply REX to MMX operands

[Richard Henderson]

Cc: qemu-stable@nongnu.org
Fixes: b3e22b2318a ("target/i386: add core of new i386 decoder")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2495
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/i386/tcg/decode-new.c.inc | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/target/i386/tcg/decode-new.c.inc b/target/i386/tcg/decode-new.c.inc
index b22210f45d..03138b3876 100644
--- a/target/i386/tcg/decode-new.c.inc
+++ b/target/i386/tcg/decode-new.c.inc
@@ -1979,7 +1979,10 @@ static bool decode_op(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode,
             op->unit = X86_OP_SSE;
         }
     get_reg:
-        op->n = ((get_modrm(s, env) >> 3) & 7) | REX_R(s);
+        op->n = ((get_modrm(s, env) >> 3) & 7);
+        if (op->unit != X86_OP_MMX) {
+            op->n |= REX_R(s);
+        }
         break;
 
     case X86_TYPE_E:  /* ALU modrm operand */
-- 
2.43.0

[PATCH 2/3] target/i386: Use unit not type in decode_modrm

[Richard Henderson]

Rather that enumerating the types that can produce
MMX operands, examine the unit.  No functional change.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/i386/tcg/decode-new.c.inc | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/i386/tcg/decode-new.c.inc b/target/i386/tcg/decode-new.c.inc
index 03138b3876..30be9237c3 100644
--- a/target/i386/tcg/decode-new.c.inc
+++ b/target/i386/tcg/decode-new.c.inc
@@ -1799,13 +1799,13 @@ static void decode_root(DisasContext *s, CPUX86State *env, X86OpEntry *entry, ui
 }
 
 
-static int decode_modrm(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode,
-                        X86DecodedOp *op, X86OpType type)
+static int decode_modrm(DisasContext *s, CPUX86State *env,
+                        X86DecodedInsn *decode, X86DecodedOp *op)
 {
     int modrm = get_modrm(s, env);
     if ((modrm >> 6) == 3) {
         op->n = (modrm & 7);
-        if (type != X86_TYPE_Q && type != X86_TYPE_N) {
+        if (op->unit != X86_OP_MMX) {
             op->n |= REX_B(s);
         }
     } else {
@@ -2040,7 +2040,7 @@ static bool decode_op(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode,
         /* fall through */
     case X86_TYPE_nop:  /* modrm operand decoded but not fetched */
     get_modrm:
-        decode_modrm(s, env, decode, op, type);
+        decode_modrm(s, env, decode, op);
         break;
 
     case X86_TYPE_O:  /* Absolute address encoded in the instruction */
-- 
2.43.0

Re: [PATCH 2/3] target/i386: Use unit not type in decode_modrm

[Philippe Mathieu-Daudé]

On 12/8/24 04:58, Richard Henderson wrote:
> Rather that enumerating the types that can produce
> MMX operands, examine the unit.  No functional change.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/i386/tcg/decode-new.c.inc | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 3/3] target/i386: Assert MMX and XMM registers in range

[Richard Henderson]

The mmx assert would fire without the fix for #2495.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/i386/tcg/emit.c.inc | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/target/i386/tcg/emit.c.inc b/target/i386/tcg/emit.c.inc
index 016dce8146..747ff71b33 100644
--- a/target/i386/tcg/emit.c.inc
+++ b/target/i386/tcg/emit.c.inc
@@ -33,8 +33,13 @@
 #define TCG_TARGET_extract_tl_valid     TCG_TARGET_extract_i32_valid
 #endif
 
+#define MMX_OFFSET(reg)
+  ({ assert((reg) >= 0 && (reg) <= 7);         \
+     offsetof(CPUX86State, fpregs[reg].mmx); })
 
-#define ZMM_OFFSET(reg) offsetof(CPUX86State, xmm_regs[reg])
+#define ZMM_OFFSET(reg)                        \
+  ({ assert((reg) >= 0 && (reg) <= 15);        \
+     offsetof(CPUX86State, xmm_regs[reg]); })
 
 typedef void (*SSEFunc_i_ep)(TCGv_i32 val, TCGv_ptr env, TCGv_ptr reg);
 typedef void (*SSEFunc_l_ep)(TCGv_i64 val, TCGv_ptr env, TCGv_ptr reg);
@@ -168,7 +173,7 @@ static int vector_elem_offset(X86DecodedOp *op, MemOp ot, int n)
 static void compute_mmx_offset(X86DecodedOp *op)
 {
     if (!op->has_ea) {
-        op->offset = offsetof(CPUX86State, fpregs[op->n].mmx) + mmx_offset(op->ot);
+        op->offset = MMX_OFFSET(op->n) + mmx_offset(op->ot);
     } else {
         op->offset = offsetof(CPUX86State, mmx_t0) + mmx_offset(op->ot);
     }
-- 
2.43.0

Re: [PATCH 3/3] target/i386: Assert MMX and XMM registers in range

[Philippe Mathieu-Daudé]

On 12/8/24 04:58, Richard Henderson wrote:
> The mmx assert would fire without the fix for #2495.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/i386/tcg/emit.c.inc | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH for-9.1 0/3] target/i386: Do not apply REX to MMX operands

[Paolo Bonzini]

Queued, thanks.

Paolo

Re: [PATCH for-9.1 0/3] target/i386: Do not apply REX to MMX operands

[Richard Henderson]

On 8/13/24 19:35, Paolo Bonzini wrote:
> Queued, thanks.
> 
> Paolo
> 

Beware there's a missing \ in patch 3.
Not sure how that happened between last test and posting...


r~