[PATCH] target/i386: apply mod to immediate count of an RCL/RCR operation

Paolo Bonzini posted 1 patch 2 days, 1 hour ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260528151245.845539-1-pbonzini@redhat.com
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>
There is a newer version of this series
target/i386/tcg/emit.c.inc | 85 +++++++++++++++++++++++---------------
roms/opensbi               |  2 +-
2 files changed, 53 insertions(+), 34 deletions(-)
[PATCH] target/i386: apply mod to immediate count of an RCL/RCR operation
Posted by Paolo Bonzini 2 days, 1 hour ago
RCR and RCL instructions with a count of 9 are the same as if the
count was 0, but they generated incorrect code because the can_be_zero
flag is false.  This causes 0 to underflow into -1 at
tcg_gen_subi_tl(count, count, 1).

Fix by absorbind the call to gen_shift_count() into gen_rotc_mod(),
so that the new function handles both mask and mod.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3452
---
 target/i386/tcg/emit.c.inc | 85 +++++++++++++++++++++++---------------
 roms/opensbi               |  2 +-
 2 files changed, 53 insertions(+), 34 deletions(-)

diff --git a/target/i386/tcg/emit.c.inc b/target/i386/tcg/emit.c.inc
index ce636b6c56c..41dcd82ab70 100644
--- a/target/i386/tcg/emit.c.inc
+++ b/target/i386/tcg/emit.c.inc
@@ -3244,12 +3244,10 @@ static void gen_PUSHF(DisasContext *s, X86DecodedInsn *decode)
     assume_cc_op(s, CC_OP_EFLAGS);
 }
 
-static MemOp gen_shift_count(DisasContext *s, X86DecodedInsn *decode,
-                             bool *can_be_zero, TCGv *count, int unit)
+static void gen_shift_count_1(DisasContext *s, X86DecodedInsn *decode,
+                              bool *can_be_zero, TCGv *count, int unit,
+                              int mask)
 {
-    MemOp ot = decode->op[0].ot;
-    int mask = (ot <= MO_32 ? 0x1f : 0x3f);
-
     *can_be_zero = false;
     switch (unit) {
     case X86_OP_INT:
@@ -3259,12 +3257,17 @@ static MemOp gen_shift_count(DisasContext *s, X86DecodedInsn *decode,
         break;
 
     case X86_OP_IMM:
-        if ((decode->immediate & mask) == 0) {
+        /*
+         * The caller applied the mask (this is unobvious; unfortunately the
+         * mask must be applied *before* the modulo operation for RCL/RCR,
+         * and the modulo operation must be before this check for zero).
+         */
+        if (decode->immediate == 0) {
             *count = NULL;
             break;
         }
         *count = tcg_temp_new();
-        tcg_gen_movi_tl(*count, decode->immediate & mask);
+        tcg_gen_movi_tl(*count, decode->immediate);
         break;
 
     case X86_OP_SKIP:
@@ -3275,7 +3278,19 @@ static MemOp gen_shift_count(DisasContext *s, X86DecodedInsn *decode,
     default:
         g_assert_not_reached();
     }
+}
 
+static MemOp gen_shift_count(DisasContext *s, X86DecodedInsn *decode,
+                             bool *can_be_zero, TCGv *count, int unit)
+{
+    MemOp ot = decode->op[0].ot;
+    int mask = (ot <= MO_32 ? 0x1f : 0x3f);
+
+    if (unit == X86_OP_IMM) {
+        decode->immediate &= mask;
+    }
+
+    gen_shift_count_1(s, decode, can_be_zero, count, unit, mask);
     return ot;
 }
 
@@ -3394,32 +3409,38 @@ static void gen_rot_overflow(X86DecodedInsn *decode, TCGv result, TCGv old,
     }
 }
 
-/*
- * RCx operations are invariant modulo 8*operand_size+1.  For 8 and 16-bit operands,
- * this is less than 0x1f (the mask applied by gen_shift_count) so reduce further.
- */
-static void gen_rotc_mod(MemOp ot, TCGv count)
+static MemOp gen_rotc_count(DisasContext *s, X86DecodedInsn *decode,
+                            bool *can_be_zero, TCGv *count, int unit)
 {
     TCGv temp;
+    MemOp ot = decode->op[0].ot;
+    int mod = (8 << ot) + 1;
+    int mask = (ot <= MO_32 ? 0x1f : 0x3f);
 
-    switch (ot) {
-    case MO_8:
-        temp = tcg_temp_new();
-        tcg_gen_subi_tl(temp, count, 18);
-        tcg_gen_movcond_tl(TCG_COND_GE, count, temp, tcg_constant_tl(0), temp, count);
-        tcg_gen_subi_tl(temp, count, 9);
-        tcg_gen_movcond_tl(TCG_COND_GE, count, temp, tcg_constant_tl(0), temp, count);
-        break;
-
-    case MO_16:
-        temp = tcg_temp_new();
-        tcg_gen_subi_tl(temp, count, 17);
-        tcg_gen_movcond_tl(TCG_COND_GE, count, temp, tcg_constant_tl(0), temp, count);
-        break;
-
-    default:
-        break;
+    /*
+     * RCx operations are invariant modulo 8*operand_size+1.  For 8 and 16-bit
+     * operands, this is less than 0x1f (the mask applied by gen_shift_count)
+     * so reduce further.  Failure to do so results in incorrect shifts
+     * in gen_RCL/gen_RCR.
+     */
+    if (unit == X86_OP_IMM) {
+        decode->immediate &= mask;
+        decode->immediate %= mod;
     }
+
+    gen_shift_count_1(s, decode, can_be_zero, count, unit, mask);
+
+    if (unit == X86_OP_INT && mod < mask) {
+        temp = tcg_temp_new();
+        if (mod * 2 < mask) {
+            tcg_gen_subi_tl(temp, count, mod * 2);
+            tcg_gen_movcond_tl(TCG_COND_GE, count, temp, tcg_constant_tl(0), temp, count);
+        }
+        tcg_gen_subi_tl(temp, count, mod);
+        tcg_gen_movcond_tl(TCG_COND_GE, count, temp, tcg_constant_tl(0), temp, count);
+    }
+
+    return ot;
 }
 
 /*
@@ -3440,7 +3461,7 @@ static void gen_RCL(DisasContext *s, X86DecodedInsn *decode)
     bool have_1bit_cin, can_be_zero;
     TCGv count;
     TCGLabel *zero_label = NULL;
-    MemOp ot = gen_shift_count(s, decode, &can_be_zero, &count, decode->op[2].unit);
+    MemOp ot = gen_rotc_count(s, decode, &can_be_zero, &count, decode->op[2].unit);
     TCGv low, high, low_count;
 
     if (!count) {
@@ -3451,7 +3472,6 @@ static void gen_RCL(DisasContext *s, X86DecodedInsn *decode)
     high = tcg_temp_new();
     low_count = tcg_temp_new();
 
-    gen_rotc_mod(ot, count);
     have_1bit_cin = gen_eflags_adcox(s, decode, true, can_be_zero);
     if (can_be_zero) {
         zero_label = gen_new_label();
@@ -3492,7 +3512,7 @@ static void gen_RCR(DisasContext *s, X86DecodedInsn *decode)
     bool have_1bit_cin, can_be_zero;
     TCGv count;
     TCGLabel *zero_label = NULL;
-    MemOp ot = gen_shift_count(s, decode, &can_be_zero, &count, decode->op[2].unit);
+    MemOp ot = gen_rotc_count(s, decode, &can_be_zero, &count, decode->op[2].unit);
     TCGv low, high, high_count;
 
     if (!count) {
@@ -3503,7 +3523,6 @@ static void gen_RCR(DisasContext *s, X86DecodedInsn *decode)
     high = tcg_temp_new();
     high_count = tcg_temp_new();
 
-    gen_rotc_mod(ot, count);
     have_1bit_cin = gen_eflags_adcox(s, decode, true, can_be_zero);
     if (can_be_zero) {
         zero_label = gen_new_label();
diff --git a/roms/opensbi b/roms/opensbi
index 74434f25587..a32a9106911 160000
--- a/roms/opensbi
+++ b/roms/opensbi
@@ -1 +1 @@
-Subproject commit 74434f255873d74e56cc50aa762d1caf24c099f8
+Subproject commit a32a91069119e7a5aa31e6bc51d5e00860be3d80
-- 
2.54.0
Re: [PATCH] target/i386: apply mod to immediate count of an RCL/RCR operation
Posted by Philippe Mathieu-Daudé 2 days, 1 hour ago
On 28/5/26 17:12, Paolo Bonzini wrote:
> RCR and RCL instructions with a count of 9 are the same as if the
> count was 0, but they generated incorrect code because the can_be_zero
> flag is false.  This causes 0 to underflow into -1 at
> tcg_gen_subi_tl(count, count, 1).
> 
> Fix by absorbind the call to gen_shift_count() into gen_rotc_mod(),
> so that the new function handles both mask and mod.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3452

S-o-b?

> ---
>   target/i386/tcg/emit.c.inc | 85 +++++++++++++++++++++++---------------
>   roms/opensbi               |  2 +-

(Unrelated)

>   2 files changed, 53 insertions(+), 34 deletions(-)