All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 1/2] target/i386: Trivial code motion
@ 2021-05-07  8:00 Ziqiao Kong
  2021-05-07  8:00 ` [PATCH v4 2/2] target/i386: Correct implementation for FCS, FIP, FDS and FDP Ziqiao Kong
  2021-05-17 20:16 ` [PATCH v4 1/2] target/i386: Trivial code motion Eduardo Habkost
  0 siblings, 2 replies; 10+ messages in thread
From: Ziqiao Kong @ 2021-05-07  8:00 UTC (permalink / raw)
  To: qemu-devel; +Cc: pbonzini, richard.henderson, ehabkost, Ziqiao Kong

Move the float translation case to a new block by a new pair of braces.

Fix some coding style problem for the old code.

Signed-off-by: Ziqiao Kong <ziqiaokong@gmail.com>
---
 VERSION                     |   2 +-
 target/i386/tcg/translate.c | 953 ++++++++++++++++++------------------
 2 files changed, 481 insertions(+), 474 deletions(-)

diff --git a/VERSION b/VERSION
index e479d55a5e..09b254e90c 100644
--- a/VERSION
+++ b/VERSION
@@ -1 +1 @@
-5.2.95
+6.0.0
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 880bc45561..52e94fe106 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -5838,503 +5838,510 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
         /************************/
         /* floats */
     case 0xd8 ... 0xdf:
-        if (s->flags & (HF_EM_MASK | HF_TS_MASK)) {
-            /* if CR0.EM or CR0.TS are set, generate an FPU exception */
-            /* XXX: what to do if illegal op ? */
-            gen_exception(s, EXCP07_PREX, pc_start - s->cs_base);
-            break;
-        }
-        modrm = x86_ldub_code(env, s);
-        mod = (modrm >> 6) & 3;
-        rm = modrm & 7;
-        op = ((b & 7) << 3) | ((modrm >> 3) & 7);
-        if (mod != 3) {
-            /* memory op */
-            gen_lea_modrm(env, s, modrm);
-            switch(op) {
-            case 0x00 ... 0x07: /* fxxxs */
-            case 0x10 ... 0x17: /* fixxxl */
-            case 0x20 ... 0x27: /* fxxxl */
-            case 0x30 ... 0x37: /* fixxx */
-                {
-                    int op1;
-                    op1 = op & 7;
-
-                    switch(op >> 4) {
-                    case 0:
-                        tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
-                                            s->mem_index, MO_LEUL);
-                        gen_helper_flds_FT0(cpu_env, s->tmp2_i32);
-                        break;
-                    case 1:
-                        tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
-                                            s->mem_index, MO_LEUL);
-                        gen_helper_fildl_FT0(cpu_env, s->tmp2_i32);
-                        break;
-                    case 2:
-                        tcg_gen_qemu_ld_i64(s->tmp1_i64, s->A0,
-                                            s->mem_index, MO_LEQ);
-                        gen_helper_fldl_FT0(cpu_env, s->tmp1_i64);
-                        break;
-                    case 3:
-                    default:
-                        tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
-                                            s->mem_index, MO_LESW);
-                        gen_helper_fildl_FT0(cpu_env, s->tmp2_i32);
-                        break;
-                    }
-
-                    gen_helper_fp_arith_ST0_FT0(op1);
-                    if (op1 == 3) {
-                        /* fcomp needs pop */
-                        gen_helper_fpop(cpu_env);
-                    }
-                }
+        {
+            if (s->flags & (HF_EM_MASK | HF_TS_MASK)) {
+                /* if CR0.EM or CR0.TS are set, generate an FPU exception */
+                /* XXX: what to do if illegal op ? */
+                gen_exception(s, EXCP07_PREX, pc_start - s->cs_base);
                 break;
-            case 0x08: /* flds */
-            case 0x0a: /* fsts */
-            case 0x0b: /* fstps */
-            case 0x18 ... 0x1b: /* fildl, fisttpl, fistl, fistpl */
-            case 0x28 ... 0x2b: /* fldl, fisttpll, fstl, fstpl */
-            case 0x38 ... 0x3b: /* filds, fisttps, fists, fistps */
-                switch(op & 7) {
-                case 0:
-                    switch(op >> 4) {
-                    case 0:
-                        tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
-                                            s->mem_index, MO_LEUL);
-                        gen_helper_flds_ST0(cpu_env, s->tmp2_i32);
-                        break;
-                    case 1:
-                        tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
-                                            s->mem_index, MO_LEUL);
-                        gen_helper_fildl_ST0(cpu_env, s->tmp2_i32);
-                        break;
-                    case 2:
-                        tcg_gen_qemu_ld_i64(s->tmp1_i64, s->A0,
-                                            s->mem_index, MO_LEQ);
-                        gen_helper_fldl_ST0(cpu_env, s->tmp1_i64);
-                        break;
-                    case 3:
-                    default:
-                        tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
-                                            s->mem_index, MO_LESW);
-                        gen_helper_fildl_ST0(cpu_env, s->tmp2_i32);
-                        break;
-                    }
-                    break;
-                case 1:
-                    /* XXX: the corresponding CPUID bit must be tested ! */
-                    switch(op >> 4) {
-                    case 1:
-                        gen_helper_fisttl_ST0(s->tmp2_i32, cpu_env);
-                        tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
-                                            s->mem_index, MO_LEUL);
-                        break;
-                    case 2:
-                        gen_helper_fisttll_ST0(s->tmp1_i64, cpu_env);
-                        tcg_gen_qemu_st_i64(s->tmp1_i64, s->A0,
-                                            s->mem_index, MO_LEQ);
-                        break;
-                    case 3:
-                    default:
-                        gen_helper_fistt_ST0(s->tmp2_i32, cpu_env);
-                        tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
-                                            s->mem_index, MO_LEUW);
-                        break;
-                    }
-                    gen_helper_fpop(cpu_env);
-                    break;
-                default:
-                    switch(op >> 4) {
-                    case 0:
-                        gen_helper_fsts_ST0(s->tmp2_i32, cpu_env);
-                        tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
-                                            s->mem_index, MO_LEUL);
-                        break;
-                    case 1:
-                        gen_helper_fistl_ST0(s->tmp2_i32, cpu_env);
-                        tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
-                                            s->mem_index, MO_LEUL);
-                        break;
-                    case 2:
-                        gen_helper_fstl_ST0(s->tmp1_i64, cpu_env);
-                        tcg_gen_qemu_st_i64(s->tmp1_i64, s->A0,
-                                            s->mem_index, MO_LEQ);
-                        break;
-                    case 3:
-                    default:
-                        gen_helper_fist_ST0(s->tmp2_i32, cpu_env);
-                        tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
-                                            s->mem_index, MO_LEUW);
-                        break;
-                    }
-                    if ((op & 7) == 3)
-                        gen_helper_fpop(cpu_env);
-                    break;
-                }
-                break;
-            case 0x0c: /* fldenv mem */
-                gen_helper_fldenv(cpu_env, s->A0, tcg_const_i32(dflag - 1));
-                break;
-            case 0x0d: /* fldcw mem */
-                tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
-                                    s->mem_index, MO_LEUW);
-                gen_helper_fldcw(cpu_env, s->tmp2_i32);
-                break;
-            case 0x0e: /* fnstenv mem */
-                gen_helper_fstenv(cpu_env, s->A0, tcg_const_i32(dflag - 1));
-                break;
-            case 0x0f: /* fnstcw mem */
-                gen_helper_fnstcw(s->tmp2_i32, cpu_env);
-                tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
-                                    s->mem_index, MO_LEUW);
-                break;
-            case 0x1d: /* fldt mem */
-                gen_helper_fldt_ST0(cpu_env, s->A0);
-                break;
-            case 0x1f: /* fstpt mem */
-                gen_helper_fstt_ST0(cpu_env, s->A0);
-                gen_helper_fpop(cpu_env);
-                break;
-            case 0x2c: /* frstor mem */
-                gen_helper_frstor(cpu_env, s->A0, tcg_const_i32(dflag - 1));
-                break;
-            case 0x2e: /* fnsave mem */
-                gen_helper_fsave(cpu_env, s->A0, tcg_const_i32(dflag - 1));
-                break;
-            case 0x2f: /* fnstsw mem */
-                gen_helper_fnstsw(s->tmp2_i32, cpu_env);
-                tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
-                                    s->mem_index, MO_LEUW);
-                break;
-            case 0x3c: /* fbld */
-                gen_helper_fbld_ST0(cpu_env, s->A0);
-                break;
-            case 0x3e: /* fbstp */
-                gen_helper_fbst_ST0(cpu_env, s->A0);
-                gen_helper_fpop(cpu_env);
-                break;
-            case 0x3d: /* fildll */
-                tcg_gen_qemu_ld_i64(s->tmp1_i64, s->A0, s->mem_index, MO_LEQ);
-                gen_helper_fildll_ST0(cpu_env, s->tmp1_i64);
-                break;
-            case 0x3f: /* fistpll */
-                gen_helper_fistll_ST0(s->tmp1_i64, cpu_env);
-                tcg_gen_qemu_st_i64(s->tmp1_i64, s->A0, s->mem_index, MO_LEQ);
-                gen_helper_fpop(cpu_env);
-                break;
-            default:
-                goto unknown_op;
             }
-        } else {
-            /* register float ops */
-            opreg = rm;
+            modrm = x86_ldub_code(env, s);
+            mod = (modrm >> 6) & 3;
+            rm = modrm & 7;
+            op = ((b & 7) << 3) | ((modrm >> 3) & 7);
+            if (mod != 3) {
+                /* memory op */
+                gen_lea_modrm(env, s, modrm);
+                switch (op) {
+                case 0x00 ... 0x07: /* fxxxs */
+                case 0x10 ... 0x17: /* fixxxl */
+                case 0x20 ... 0x27: /* fxxxl */
+                case 0x30 ... 0x37: /* fixxx */
+                    {
+                        int op1;
+                        op1 = op & 7;
 
-            switch(op) {
-            case 0x08: /* fld sti */
-                gen_helper_fpush(cpu_env);
-                gen_helper_fmov_ST0_STN(cpu_env,
-                                        tcg_const_i32((opreg + 1) & 7));
-                break;
-            case 0x09: /* fxchg sti */
-            case 0x29: /* fxchg4 sti, undocumented op */
-            case 0x39: /* fxchg7 sti, undocumented op */
-                gen_helper_fxchg_ST0_STN(cpu_env, tcg_const_i32(opreg));
-                break;
-            case 0x0a: /* grp d9/2 */
-                switch(rm) {
-                case 0: /* fnop */
-                    /* check exceptions (FreeBSD FPU probe) */
-                    gen_helper_fwait(cpu_env);
-                    break;
-                default:
-                    goto unknown_op;
-                }
-                break;
-            case 0x0c: /* grp d9/4 */
-                switch(rm) {
-                case 0: /* fchs */
-                    gen_helper_fchs_ST0(cpu_env);
-                    break;
-                case 1: /* fabs */
-                    gen_helper_fabs_ST0(cpu_env);
-                    break;
-                case 4: /* ftst */
-                    gen_helper_fldz_FT0(cpu_env);
-                    gen_helper_fcom_ST0_FT0(cpu_env);
-                    break;
-                case 5: /* fxam */
-                    gen_helper_fxam_ST0(cpu_env);
-                    break;
-                default:
-                    goto unknown_op;
-                }
-                break;
-            case 0x0d: /* grp d9/5 */
-                {
-                    switch(rm) {
-                    case 0:
-                        gen_helper_fpush(cpu_env);
-                        gen_helper_fld1_ST0(cpu_env);
-                        break;
-                    case 1:
-                        gen_helper_fpush(cpu_env);
-                        gen_helper_fldl2t_ST0(cpu_env);
-                        break;
-                    case 2:
-                        gen_helper_fpush(cpu_env);
-                        gen_helper_fldl2e_ST0(cpu_env);
-                        break;
-                    case 3:
-                        gen_helper_fpush(cpu_env);
-                        gen_helper_fldpi_ST0(cpu_env);
-                        break;
-                    case 4:
-                        gen_helper_fpush(cpu_env);
-                        gen_helper_fldlg2_ST0(cpu_env);
-                        break;
-                    case 5:
-                        gen_helper_fpush(cpu_env);
-                        gen_helper_fldln2_ST0(cpu_env);
-                        break;
-                    case 6:
-                        gen_helper_fpush(cpu_env);
-                        gen_helper_fldz_ST0(cpu_env);
-                        break;
-                    default:
-                        goto unknown_op;
-                    }
-                }
-                break;
-            case 0x0e: /* grp d9/6 */
-                switch(rm) {
-                case 0: /* f2xm1 */
-                    gen_helper_f2xm1(cpu_env);
-                    break;
-                case 1: /* fyl2x */
-                    gen_helper_fyl2x(cpu_env);
-                    break;
-                case 2: /* fptan */
-                    gen_helper_fptan(cpu_env);
-                    break;
-                case 3: /* fpatan */
-                    gen_helper_fpatan(cpu_env);
-                    break;
-                case 4: /* fxtract */
-                    gen_helper_fxtract(cpu_env);
-                    break;
-                case 5: /* fprem1 */
-                    gen_helper_fprem1(cpu_env);
-                    break;
-                case 6: /* fdecstp */
-                    gen_helper_fdecstp(cpu_env);
-                    break;
-                default:
-                case 7: /* fincstp */
-                    gen_helper_fincstp(cpu_env);
-                    break;
-                }
-                break;
-            case 0x0f: /* grp d9/7 */
-                switch(rm) {
-                case 0: /* fprem */
-                    gen_helper_fprem(cpu_env);
-                    break;
-                case 1: /* fyl2xp1 */
-                    gen_helper_fyl2xp1(cpu_env);
-                    break;
-                case 2: /* fsqrt */
-                    gen_helper_fsqrt(cpu_env);
-                    break;
-                case 3: /* fsincos */
-                    gen_helper_fsincos(cpu_env);
-                    break;
-                case 5: /* fscale */
-                    gen_helper_fscale(cpu_env);
-                    break;
-                case 4: /* frndint */
-                    gen_helper_frndint(cpu_env);
-                    break;
-                case 6: /* fsin */
-                    gen_helper_fsin(cpu_env);
-                    break;
-                default:
-                case 7: /* fcos */
-                    gen_helper_fcos(cpu_env);
-                    break;
-                }
-                break;
-            case 0x00: case 0x01: case 0x04 ... 0x07: /* fxxx st, sti */
-            case 0x20: case 0x21: case 0x24 ... 0x27: /* fxxx sti, st */
-            case 0x30: case 0x31: case 0x34 ... 0x37: /* fxxxp sti, st */
-                {
-                    int op1;
+                        switch (op >> 4) {
+                        case 0:
+                            tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
+                                                s->mem_index, MO_LEUL);
+                            gen_helper_flds_FT0(cpu_env, s->tmp2_i32);
+                            break;
+                        case 1:
+                            tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
+                                                s->mem_index, MO_LEUL);
+                            gen_helper_fildl_FT0(cpu_env, s->tmp2_i32);
+                            break;
+                        case 2:
+                            tcg_gen_qemu_ld_i64(s->tmp1_i64, s->A0,
+                                                s->mem_index, MO_LEQ);
+                            gen_helper_fldl_FT0(cpu_env, s->tmp1_i64);
+                            break;
+                        case 3:
+                        default:
+                            tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
+                                                s->mem_index, MO_LESW);
+                            gen_helper_fildl_FT0(cpu_env, s->tmp2_i32);
+                            break;
+                        }
 
-                    op1 = op & 7;
-                    if (op >= 0x20) {
-                        gen_helper_fp_arith_STN_ST0(op1, opreg);
-                        if (op >= 0x30)
-                            gen_helper_fpop(cpu_env);
-                    } else {
-                        gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
                         gen_helper_fp_arith_ST0_FT0(op1);
+                        if (op1 == 3) {
+                            /* fcomp needs pop */
+                            gen_helper_fpop(cpu_env);
+                        }
                     }
-                }
-                break;
-            case 0x02: /* fcom */
-            case 0x22: /* fcom2, undocumented op */
-                gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
-                gen_helper_fcom_ST0_FT0(cpu_env);
-                break;
-            case 0x03: /* fcomp */
-            case 0x23: /* fcomp3, undocumented op */
-            case 0x32: /* fcomp5, undocumented op */
-                gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
-                gen_helper_fcom_ST0_FT0(cpu_env);
-                gen_helper_fpop(cpu_env);
-                break;
-            case 0x15: /* da/5 */
-                switch(rm) {
-                case 1: /* fucompp */
-                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(1));
-                    gen_helper_fucom_ST0_FT0(cpu_env);
-                    gen_helper_fpop(cpu_env);
-                    gen_helper_fpop(cpu_env);
                     break;
-                default:
-                    goto unknown_op;
-                }
-                break;
-            case 0x1c:
-                switch(rm) {
-                case 0: /* feni (287 only, just do nop here) */
+                case 0x08: /* flds */
+                case 0x0a: /* fsts */
+                case 0x0b: /* fstps */
+                case 0x18 ... 0x1b: /* fildl, fisttpl, fistl, fistpl */
+                case 0x28 ... 0x2b: /* fldl, fisttpll, fstl, fstpl */
+                case 0x38 ... 0x3b: /* filds, fisttps, fists, fistps */
+                    switch (op & 7) {
+                    case 0:
+                        switch (op >> 4) {
+                        case 0:
+                            tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
+                                                s->mem_index, MO_LEUL);
+                            gen_helper_flds_ST0(cpu_env, s->tmp2_i32);
+                            break;
+                        case 1:
+                            tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
+                                                s->mem_index, MO_LEUL);
+                            gen_helper_fildl_ST0(cpu_env, s->tmp2_i32);
+                            break;
+                        case 2:
+                            tcg_gen_qemu_ld_i64(s->tmp1_i64, s->A0,
+                                                s->mem_index, MO_LEQ);
+                            gen_helper_fldl_ST0(cpu_env, s->tmp1_i64);
+                            break;
+                        case 3:
+                        default:
+                            tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
+                                                s->mem_index, MO_LESW);
+                            gen_helper_fildl_ST0(cpu_env, s->tmp2_i32);
+                            break;
+                        }
+                        break;
+                    case 1:
+                        /* XXX: the corresponding CPUID bit must be tested ! */
+                        switch (op >> 4) {
+                        case 1:
+                            gen_helper_fisttl_ST0(s->tmp2_i32, cpu_env);
+                            tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
+                                                s->mem_index, MO_LEUL);
+                            break;
+                        case 2:
+                            gen_helper_fisttll_ST0(s->tmp1_i64, cpu_env);
+                            tcg_gen_qemu_st_i64(s->tmp1_i64, s->A0,
+                                                s->mem_index, MO_LEQ);
+                            break;
+                        case 3:
+                        default:
+                            gen_helper_fistt_ST0(s->tmp2_i32, cpu_env);
+                            tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
+                                                s->mem_index, MO_LEUW);
+                            break;
+                        }
+                        gen_helper_fpop(cpu_env);
+                        break;
+                    default:
+                        switch (op >> 4) {
+                        case 0:
+                            gen_helper_fsts_ST0(s->tmp2_i32, cpu_env);
+                            tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
+                                                s->mem_index, MO_LEUL);
+                            break;
+                        case 1:
+                            gen_helper_fistl_ST0(s->tmp2_i32, cpu_env);
+                            tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
+                                                s->mem_index, MO_LEUL);
+                            break;
+                        case 2:
+                            gen_helper_fstl_ST0(s->tmp1_i64, cpu_env);
+                            tcg_gen_qemu_st_i64(s->tmp1_i64, s->A0,
+                                                s->mem_index, MO_LEQ);
+                            break;
+                        case 3:
+                        default:
+                            gen_helper_fist_ST0(s->tmp2_i32, cpu_env);
+                            tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
+                                                s->mem_index, MO_LEUW);
+                            break;
+                        }
+                        if ((op & 7) == 3) {
+                            gen_helper_fpop(cpu_env);
+                        }
+                        break;
+                    }
                     break;
-                case 1: /* fdisi (287 only, just do nop here) */
+                case 0x0c: /* fldenv mem */
+                    gen_helper_fldenv(cpu_env, s->A0, tcg_const_i32(dflag - 1));
                     break;
-                case 2: /* fclex */
-                    gen_helper_fclex(cpu_env);
+                case 0x0d: /* fldcw mem */
+                    tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
+                                        s->mem_index, MO_LEUW);
+                    gen_helper_fldcw(cpu_env, s->tmp2_i32);
                     break;
-                case 3: /* fninit */
-                    gen_helper_fninit(cpu_env);
+                case 0x0e: /* fnstenv mem */
+                    gen_helper_fstenv(cpu_env, s->A0, tcg_const_i32(dflag - 1));
                     break;
-                case 4: /* fsetpm (287 only, just do nop here) */
+                case 0x0f: /* fnstcw mem */
+                    gen_helper_fnstcw(s->tmp2_i32, cpu_env);
+                    tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
+                                        s->mem_index, MO_LEUW);
                     break;
-                default:
-                    goto unknown_op;
-                }
-                break;
-            case 0x1d: /* fucomi */
-                if (!(s->cpuid_features & CPUID_CMOV)) {
-                    goto illegal_op;
-                }
-                gen_update_cc_op(s);
-                gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
-                gen_helper_fucomi_ST0_FT0(cpu_env);
-                set_cc_op(s, CC_OP_EFLAGS);
-                break;
-            case 0x1e: /* fcomi */
-                if (!(s->cpuid_features & CPUID_CMOV)) {
-                    goto illegal_op;
-                }
-                gen_update_cc_op(s);
-                gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
-                gen_helper_fcomi_ST0_FT0(cpu_env);
-                set_cc_op(s, CC_OP_EFLAGS);
-                break;
-            case 0x28: /* ffree sti */
-                gen_helper_ffree_STN(cpu_env, tcg_const_i32(opreg));
-                break;
-            case 0x2a: /* fst sti */
-                gen_helper_fmov_STN_ST0(cpu_env, tcg_const_i32(opreg));
-                break;
-            case 0x2b: /* fstp sti */
-            case 0x0b: /* fstp1 sti, undocumented op */
-            case 0x3a: /* fstp8 sti, undocumented op */
-            case 0x3b: /* fstp9 sti, undocumented op */
-                gen_helper_fmov_STN_ST0(cpu_env, tcg_const_i32(opreg));
-                gen_helper_fpop(cpu_env);
-                break;
-            case 0x2c: /* fucom st(i) */
-                gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
-                gen_helper_fucom_ST0_FT0(cpu_env);
-                break;
-            case 0x2d: /* fucomp st(i) */
-                gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
-                gen_helper_fucom_ST0_FT0(cpu_env);
-                gen_helper_fpop(cpu_env);
-                break;
-            case 0x33: /* de/3 */
-                switch(rm) {
-                case 1: /* fcompp */
-                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(1));
-                    gen_helper_fcom_ST0_FT0(cpu_env);
-                    gen_helper_fpop(cpu_env);
+                case 0x1d: /* fldt mem */
+                    gen_helper_fldt_ST0(cpu_env, s->A0);
+                    break;
+                case 0x1f: /* fstpt mem */
+                    gen_helper_fstt_ST0(cpu_env, s->A0);
                     gen_helper_fpop(cpu_env);
                     break;
-                default:
-                    goto unknown_op;
-                }
-                break;
-            case 0x38: /* ffreep sti, undocumented op */
-                gen_helper_ffree_STN(cpu_env, tcg_const_i32(opreg));
-                gen_helper_fpop(cpu_env);
-                break;
-            case 0x3c: /* df/4 */
-                switch(rm) {
-                case 0:
+                case 0x2c: /* frstor mem */
+                    gen_helper_frstor(cpu_env, s->A0, tcg_const_i32(dflag - 1));
+                    break;
+                case 0x2e: /* fnsave mem */
+                    gen_helper_fsave(cpu_env, s->A0, tcg_const_i32(dflag - 1));
+                    break;
+                case 0x2f: /* fnstsw mem */
                     gen_helper_fnstsw(s->tmp2_i32, cpu_env);
-                    tcg_gen_extu_i32_tl(s->T0, s->tmp2_i32);
-                    gen_op_mov_reg_v(s, MO_16, R_EAX, s->T0);
+                    tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
+                                        s->mem_index, MO_LEUW);
+                    break;
+                case 0x3c: /* fbld */
+                    gen_helper_fbld_ST0(cpu_env, s->A0);
+                    break;
+                case 0x3e: /* fbstp */
+                    gen_helper_fbst_ST0(cpu_env, s->A0);
+                    gen_helper_fpop(cpu_env);
+                    break;
+                case 0x3d: /* fildll */
+                    tcg_gen_qemu_ld_i64(s->tmp1_i64, s->A0,
+                                        s->mem_index, MO_LEQ);
+                    gen_helper_fildll_ST0(cpu_env, s->tmp1_i64);
+                    break;
+                case 0x3f: /* fistpll */
+                    gen_helper_fistll_ST0(s->tmp1_i64, cpu_env);
+                    tcg_gen_qemu_st_i64(s->tmp1_i64, s->A0,
+                                        s->mem_index, MO_LEQ);
+                    gen_helper_fpop(cpu_env);
                     break;
                 default:
                     goto unknown_op;
                 }
-                break;
-            case 0x3d: /* fucomip */
-                if (!(s->cpuid_features & CPUID_CMOV)) {
-                    goto illegal_op;
-                }
-                gen_update_cc_op(s);
-                gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
-                gen_helper_fucomi_ST0_FT0(cpu_env);
-                gen_helper_fpop(cpu_env);
-                set_cc_op(s, CC_OP_EFLAGS);
-                break;
-            case 0x3e: /* fcomip */
-                if (!(s->cpuid_features & CPUID_CMOV)) {
-                    goto illegal_op;
-                }
-                gen_update_cc_op(s);
-                gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
-                gen_helper_fcomi_ST0_FT0(cpu_env);
-                gen_helper_fpop(cpu_env);
-                set_cc_op(s, CC_OP_EFLAGS);
-                break;
-            case 0x10 ... 0x13: /* fcmovxx */
-            case 0x18 ... 0x1b:
-                {
-                    int op1;
-                    TCGLabel *l1;
-                    static const uint8_t fcmov_cc[8] = {
-                        (JCC_B << 1),
-                        (JCC_Z << 1),
-                        (JCC_BE << 1),
-                        (JCC_P << 1),
-                    };
+            } else {
+                /* register float ops */
+                opreg = rm;
+
+                switch (op) {
+                case 0x08: /* fld sti */
+                    gen_helper_fpush(cpu_env);
+                    gen_helper_fmov_ST0_STN(cpu_env,
+                                            tcg_const_i32((opreg + 1) & 7));
+                    break;
+                case 0x09: /* fxchg sti */
+                case 0x29: /* fxchg4 sti, undocumented op */
+                case 0x39: /* fxchg7 sti, undocumented op */
+                    gen_helper_fxchg_ST0_STN(cpu_env, tcg_const_i32(opreg));
+                    break;
+                case 0x0a: /* grp d9/2 */
+                    switch (rm) {
+                    case 0: /* fnop */
+                        /* check exceptions (FreeBSD FPU probe) */
+                        gen_helper_fwait(cpu_env);
+                        break;
+                    default:
+                        goto unknown_op;
+                    }
+                    break;
+                case 0x0c: /* grp d9/4 */
+                    switch (rm) {
+                    case 0: /* fchs */
+                        gen_helper_fchs_ST0(cpu_env);
+                        break;
+                    case 1: /* fabs */
+                        gen_helper_fabs_ST0(cpu_env);
+                        break;
+                    case 4: /* ftst */
+                        gen_helper_fldz_FT0(cpu_env);
+                        gen_helper_fcom_ST0_FT0(cpu_env);
+                        break;
+                    case 5: /* fxam */
+                        gen_helper_fxam_ST0(cpu_env);
+                        break;
+                    default:
+                        goto unknown_op;
+                    }
+                    break;
+                case 0x0d: /* grp d9/5 */
+                    {
+                        switch (rm) {
+                        case 0:
+                            gen_helper_fpush(cpu_env);
+                            gen_helper_fld1_ST0(cpu_env);
+                            break;
+                        case 1:
+                            gen_helper_fpush(cpu_env);
+                            gen_helper_fldl2t_ST0(cpu_env);
+                            break;
+                        case 2:
+                            gen_helper_fpush(cpu_env);
+                            gen_helper_fldl2e_ST0(cpu_env);
+                            break;
+                        case 3:
+                            gen_helper_fpush(cpu_env);
+                            gen_helper_fldpi_ST0(cpu_env);
+                            break;
+                        case 4:
+                            gen_helper_fpush(cpu_env);
+                            gen_helper_fldlg2_ST0(cpu_env);
+                            break;
+                        case 5:
+                            gen_helper_fpush(cpu_env);
+                            gen_helper_fldln2_ST0(cpu_env);
+                            break;
+                        case 6:
+                            gen_helper_fpush(cpu_env);
+                            gen_helper_fldz_ST0(cpu_env);
+                            break;
+                        default:
+                            goto unknown_op;
+                        }
+                    }
+                    break;
+                case 0x0e: /* grp d9/6 */
+                    switch (rm) {
+                    case 0: /* f2xm1 */
+                        gen_helper_f2xm1(cpu_env);
+                        break;
+                    case 1: /* fyl2x */
+                        gen_helper_fyl2x(cpu_env);
+                        break;
+                    case 2: /* fptan */
+                        gen_helper_fptan(cpu_env);
+                        break;
+                    case 3: /* fpatan */
+                        gen_helper_fpatan(cpu_env);
+                        break;
+                    case 4: /* fxtract */
+                        gen_helper_fxtract(cpu_env);
+                        break;
+                    case 5: /* fprem1 */
+                        gen_helper_fprem1(cpu_env);
+                        break;
+                    case 6: /* fdecstp */
+                        gen_helper_fdecstp(cpu_env);
+                        break;
+                    default:
+                    case 7: /* fincstp */
+                        gen_helper_fincstp(cpu_env);
+                        break;
+                    }
+                    break;
+                case 0x0f: /* grp d9/7 */
+                    switch (rm) {
+                    case 0: /* fprem */
+                        gen_helper_fprem(cpu_env);
+                        break;
+                    case 1: /* fyl2xp1 */
+                        gen_helper_fyl2xp1(cpu_env);
+                        break;
+                    case 2: /* fsqrt */
+                        gen_helper_fsqrt(cpu_env);
+                        break;
+                    case 3: /* fsincos */
+                        gen_helper_fsincos(cpu_env);
+                        break;
+                    case 5: /* fscale */
+                        gen_helper_fscale(cpu_env);
+                        break;
+                    case 4: /* frndint */
+                        gen_helper_frndint(cpu_env);
+                        break;
+                    case 6: /* fsin */
+                        gen_helper_fsin(cpu_env);
+                        break;
+                    default:
+                    case 7: /* fcos */
+                        gen_helper_fcos(cpu_env);
+                        break;
+                    }
+                    break;
+                case 0x00: case 0x01: case 0x04 ... 0x07: /* fxxx st, sti */
+                case 0x20: case 0x21: case 0x24 ... 0x27: /* fxxx sti, st */
+                case 0x30: case 0x31: case 0x34 ... 0x37: /* fxxxp sti, st */
+                    {
+                        int op1;
 
+                        op1 = op & 7;
+                        if (op >= 0x20) {
+                            gen_helper_fp_arith_STN_ST0(op1, opreg);
+                            if (op >= 0x30) {
+                                gen_helper_fpop(cpu_env);
+                            }
+                        } else {
+                            gen_helper_fmov_FT0_STN(cpu_env,
+                                                    tcg_const_i32(opreg));
+                            gen_helper_fp_arith_ST0_FT0(op1);
+                        }
+                    }
+                    break;
+                case 0x02: /* fcom */
+                case 0x22: /* fcom2, undocumented op */
+                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
+                    gen_helper_fcom_ST0_FT0(cpu_env);
+                    break;
+                case 0x03: /* fcomp */
+                case 0x23: /* fcomp3, undocumented op */
+                case 0x32: /* fcomp5, undocumented op */
+                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
+                    gen_helper_fcom_ST0_FT0(cpu_env);
+                    gen_helper_fpop(cpu_env);
+                    break;
+                case 0x15: /* da/5 */
+                    switch (rm) {
+                    case 1: /* fucompp */
+                        gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(1));
+                        gen_helper_fucom_ST0_FT0(cpu_env);
+                        gen_helper_fpop(cpu_env);
+                        gen_helper_fpop(cpu_env);
+                        break;
+                    default:
+                        goto unknown_op;
+                    }
+                    break;
+                case 0x1c:
+                    switch (rm) {
+                    case 0: /* feni (287 only, just do nop here) */
+                        break;
+                    case 1: /* fdisi (287 only, just do nop here) */
+                        break;
+                    case 2: /* fclex */
+                        gen_helper_fclex(cpu_env);
+                        break;
+                    case 3: /* fninit */
+                        gen_helper_fninit(cpu_env);
+                        break;
+                    case 4: /* fsetpm (287 only, just do nop here) */
+                        break;
+                    default:
+                        goto unknown_op;
+                    }
+                    break;
+                case 0x1d: /* fucomi */
                     if (!(s->cpuid_features & CPUID_CMOV)) {
                         goto illegal_op;
                     }
-                    op1 = fcmov_cc[op & 3] | (((op >> 3) & 1) ^ 1);
-                    l1 = gen_new_label();
-                    gen_jcc1_noeob(s, op1, l1);
-                    gen_helper_fmov_ST0_STN(cpu_env, tcg_const_i32(opreg));
-                    gen_set_label(l1);
+                    gen_update_cc_op(s);
+                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
+                    gen_helper_fucomi_ST0_FT0(cpu_env);
+                    set_cc_op(s, CC_OP_EFLAGS);
+                    break;
+                case 0x1e: /* fcomi */
+                    if (!(s->cpuid_features & CPUID_CMOV)) {
+                        goto illegal_op;
+                    }
+                    gen_update_cc_op(s);
+                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
+                    gen_helper_fcomi_ST0_FT0(cpu_env);
+                    set_cc_op(s, CC_OP_EFLAGS);
+                    break;
+                case 0x28: /* ffree sti */
+                    gen_helper_ffree_STN(cpu_env, tcg_const_i32(opreg));
+                    break;
+                case 0x2a: /* fst sti */
+                    gen_helper_fmov_STN_ST0(cpu_env, tcg_const_i32(opreg));
+                    break;
+                case 0x2b: /* fstp sti */
+                case 0x0b: /* fstp1 sti, undocumented op */
+                case 0x3a: /* fstp8 sti, undocumented op */
+                case 0x3b: /* fstp9 sti, undocumented op */
+                    gen_helper_fmov_STN_ST0(cpu_env, tcg_const_i32(opreg));
+                    gen_helper_fpop(cpu_env);
+                    break;
+                case 0x2c: /* fucom st(i) */
+                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
+                    gen_helper_fucom_ST0_FT0(cpu_env);
+                    break;
+                case 0x2d: /* fucomp st(i) */
+                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
+                    gen_helper_fucom_ST0_FT0(cpu_env);
+                    gen_helper_fpop(cpu_env);
+                    break;
+                case 0x33: /* de/3 */
+                    switch (rm) {
+                    case 1: /* fcompp */
+                        gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(1));
+                        gen_helper_fcom_ST0_FT0(cpu_env);
+                        gen_helper_fpop(cpu_env);
+                        gen_helper_fpop(cpu_env);
+                        break;
+                    default:
+                        goto unknown_op;
+                    }
+                    break;
+                case 0x38: /* ffreep sti, undocumented op */
+                    gen_helper_ffree_STN(cpu_env, tcg_const_i32(opreg));
+                    gen_helper_fpop(cpu_env);
+                    break;
+                case 0x3c: /* df/4 */
+                    switch (rm) {
+                    case 0:
+                        gen_helper_fnstsw(s->tmp2_i32, cpu_env);
+                        tcg_gen_extu_i32_tl(s->T0, s->tmp2_i32);
+                        gen_op_mov_reg_v(s, MO_16, R_EAX, s->T0);
+                        break;
+                    default:
+                        goto unknown_op;
+                    }
+                    break;
+                case 0x3d: /* fucomip */
+                    if (!(s->cpuid_features & CPUID_CMOV)) {
+                        goto illegal_op;
+                    }
+                    gen_update_cc_op(s);
+                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
+                    gen_helper_fucomi_ST0_FT0(cpu_env);
+                    gen_helper_fpop(cpu_env);
+                    set_cc_op(s, CC_OP_EFLAGS);
+                    break;
+                case 0x3e: /* fcomip */
+                    if (!(s->cpuid_features & CPUID_CMOV)) {
+                        goto illegal_op;
+                    }
+                    gen_update_cc_op(s);
+                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
+                    gen_helper_fcomi_ST0_FT0(cpu_env);
+                    gen_helper_fpop(cpu_env);
+                    set_cc_op(s, CC_OP_EFLAGS);
+                    break;
+                case 0x10 ... 0x13: /* fcmovxx */
+                case 0x18 ... 0x1b:
+                    {
+                        int op1;
+                        TCGLabel *l1;
+                        static const uint8_t fcmov_cc[8] = {
+                            (JCC_B << 1),
+                            (JCC_Z << 1),
+                            (JCC_BE << 1),
+                            (JCC_P << 1),
+                        };
+
+                        if (!(s->cpuid_features & CPUID_CMOV)) {
+                            goto illegal_op;
+                        }
+                        op1 = fcmov_cc[op & 3] | (((op >> 3) & 1) ^ 1);
+                        l1 = gen_new_label();
+                        gen_jcc1_noeob(s, op1, l1);
+                        gen_helper_fmov_ST0_STN(cpu_env, tcg_const_i32(opreg));
+                        gen_set_label(l1);
+                    }
+                    break;
+                default:
+                    goto unknown_op;
                 }
-                break;
-            default:
-                goto unknown_op;
             }
         }
         break;
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v4 2/2] target/i386: Correct implementation for FCS, FIP, FDS and FDP
  2021-05-07  8:00 [PATCH v4 1/2] target/i386: Trivial code motion Ziqiao Kong
@ 2021-05-07  8:00 ` Ziqiao Kong
  2021-05-13 16:44   ` Ziqiao Kong
  2021-05-17 20:29   ` Eduardo Habkost
  2021-05-17 20:16 ` [PATCH v4 1/2] target/i386: Trivial code motion Eduardo Habkost
  1 sibling, 2 replies; 10+ messages in thread
From: Ziqiao Kong @ 2021-05-07  8:00 UTC (permalink / raw)
  To: qemu-devel; +Cc: pbonzini, richard.henderson, ehabkost, Ziqiao Kong

Changes since v3:
 - Split the long patches to series to make review easier.
 - Fix the coding style problems in v3.

Changes since v2:
 - Change the sequence of fpcs, fpds, fpip and fpdp in CPUX86State.
 - Use stl instead of stw in do_fstenv.
 - Move variables to floats instruction case block.
 - Move last accessed memory operand to a temp variable to avoid another load.
 - Move segment selectors instead of segment base to fpcs and fpds.
 - Fix some code stype problems for the original code in floats case block.

Signed-off-by: Ziqiao Kong <ziqiaokong@gmail.com>
---
 target/i386/cpu.h            |  4 +++
 target/i386/tcg/fpu_helper.c | 48 ++++++++++++++++++++++--------------
 target/i386/tcg/translate.c  | 45 ++++++++++++++++++++++++++++++++-
 3 files changed, 77 insertions(+), 20 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 570f916878..241945320b 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -705,6 +705,8 @@ typedef uint64_t FeatureWordArray[FEATURE_WORDS];
 #define CPUID_7_0_EBX_INVPCID           (1U << 10)
 /* Restricted Transactional Memory */
 #define CPUID_7_0_EBX_RTM               (1U << 11)
+/* Deprecates FPU CS and FPU DS values */
+#define CPUID_7_0_EBX_FCS_FDS           (1U << 13)
 /* Memory Protection Extension */
 #define CPUID_7_0_EBX_MPX               (1U << 14)
 /* AVX-512 Foundation */
@@ -1440,6 +1442,8 @@ typedef struct CPUX86State {
     FPReg fpregs[8];
     /* KVM-only so far */
     uint16_t fpop;
+    uint16_t fpcs;
+    uint16_t fpds;
     uint64_t fpip;
     uint64_t fpdp;
 
diff --git a/target/i386/tcg/fpu_helper.c b/target/i386/tcg/fpu_helper.c
index 60ed93520a..f1a8717ed8 100644
--- a/target/i386/tcg/fpu_helper.c
+++ b/target/i386/tcg/fpu_helper.c
@@ -766,6 +766,10 @@ void helper_fninit(CPUX86State *env)
 {
     env->fpus = 0;
     env->fpstt = 0;
+    env->fpcs = 0;
+    env->fpip = 0;
+    env->fpds = 0;
+    env->fpdp = 0;
     cpu_set_fpuc(env, 0x37f);
     env->fptags[0] = 1;
     env->fptags[1] = 1;
@@ -2368,6 +2372,7 @@ static void do_fstenv(CPUX86State *env, target_ulong ptr, int data32,
 {
     int fpus, fptag, exp, i;
     uint64_t mant;
+    uint16_t fpcs, fpds;
     CPU_LDoubleU tmp;
 
     fpus = (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11;
@@ -2390,24 +2395,39 @@ static void do_fstenv(CPUX86State *env, target_ulong ptr, int data32,
             }
         }
     }
+
+    /*
+     * If CR0.PE = 1, each instruction saves FCS and FDS into memory. If
+     * CPUID.(EAX=07H,ECX=0H):EBX[bit 13] = 1, the processor deprecates
+     * FCS and FDS; it saves each as 0000H.
+     */
+    if (!(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_FCS_FDS)
+        && (env->cr[0] & CR0_PE_MASK)) {
+        fpcs = env->fpcs;
+        fpds = env->fpds;
+    } else {
+        fpcs = 0;
+        fpds = 0;
+    }
+
     if (data32) {
         /* 32 bit */
         cpu_stl_data_ra(env, ptr, env->fpuc, retaddr);
         cpu_stl_data_ra(env, ptr + 4, fpus, retaddr);
         cpu_stl_data_ra(env, ptr + 8, fptag, retaddr);
-        cpu_stl_data_ra(env, ptr + 12, 0, retaddr); /* fpip */
-        cpu_stl_data_ra(env, ptr + 16, 0, retaddr); /* fpcs */
-        cpu_stl_data_ra(env, ptr + 20, 0, retaddr); /* fpoo */
-        cpu_stl_data_ra(env, ptr + 24, 0, retaddr); /* fpos */
+        cpu_stl_data_ra(env, ptr + 12, env->fpip, retaddr); /* fpip */
+        cpu_stl_data_ra(env, ptr + 16, fpcs, retaddr); /* fpcs */
+        cpu_stl_data_ra(env, ptr + 20, env->fpdp, retaddr); /* fpdp */
+        cpu_stl_data_ra(env, ptr + 24, fpds, retaddr); /* fpds */
     } else {
         /* 16 bit */
         cpu_stw_data_ra(env, ptr, env->fpuc, retaddr);
         cpu_stw_data_ra(env, ptr + 2, fpus, retaddr);
         cpu_stw_data_ra(env, ptr + 4, fptag, retaddr);
-        cpu_stw_data_ra(env, ptr + 6, 0, retaddr);
-        cpu_stw_data_ra(env, ptr + 8, 0, retaddr);
-        cpu_stw_data_ra(env, ptr + 10, 0, retaddr);
-        cpu_stw_data_ra(env, ptr + 12, 0, retaddr);
+        cpu_stw_data_ra(env, ptr + 6, env->fpip, retaddr);
+        cpu_stw_data_ra(env, ptr + 8, fpcs, retaddr);
+        cpu_stw_data_ra(env, ptr + 10, env->fpdp, retaddr);
+        cpu_stw_data_ra(env, ptr + 12, fpds, retaddr);
     }
 }
 
@@ -2473,17 +2493,7 @@ void helper_fsave(CPUX86State *env, target_ulong ptr, int data32)
     }
 
     /* fninit */
-    env->fpus = 0;
-    env->fpstt = 0;
-    cpu_set_fpuc(env, 0x37f);
-    env->fptags[0] = 1;
-    env->fptags[1] = 1;
-    env->fptags[2] = 1;
-    env->fptags[3] = 1;
-    env->fptags[4] = 1;
-    env->fptags[5] = 1;
-    env->fptags[6] = 1;
-    env->fptags[7] = 1;
+    helper_fninit(env);
 }
 
 void helper_frstor(CPUX86State *env, target_ulong ptr, int data32)
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 52e94fe106..59647ea5b7 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -5839,6 +5839,11 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
         /* floats */
     case 0xd8 ... 0xdf:
         {
+            TCGv last_addr = tcg_temp_new();
+            int last_seg;
+            bool update_fdp = false;
+            bool update_fip = true;
+
             if (s->flags & (HF_EM_MASK | HF_TS_MASK)) {
                 /* if CR0.EM or CR0.TS are set, generate an FPU exception */
                 /* XXX: what to do if illegal op ? */
@@ -5851,7 +5856,14 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
             op = ((b & 7) << 3) | ((modrm >> 3) & 7);
             if (mod != 3) {
                 /* memory op */
-                gen_lea_modrm(env, s, modrm);
+                AddressParts a = gen_lea_modrm_0(env, s, modrm);
+                TCGv ea = gen_lea_modrm_1(s, a);
+
+                update_fdp = true;
+                last_seg = a.def_seg;
+                tcg_gen_mov_tl(last_addr, ea);
+                gen_lea_v_seg(s, s->aflag, ea, a.def_seg, s->override);
+
                 switch (op) {
                 case 0x00 ... 0x07: /* fxxxs */
                 case 0x10 ... 0x17: /* fixxxl */
@@ -5978,19 +5990,23 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
                     break;
                 case 0x0c: /* fldenv mem */
                     gen_helper_fldenv(cpu_env, s->A0, tcg_const_i32(dflag - 1));
+                    update_fip = update_fdp = false;
                     break;
                 case 0x0d: /* fldcw mem */
                     tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
                                         s->mem_index, MO_LEUW);
                     gen_helper_fldcw(cpu_env, s->tmp2_i32);
+                    update_fip = update_fdp = false;
                     break;
                 case 0x0e: /* fnstenv mem */
                     gen_helper_fstenv(cpu_env, s->A0, tcg_const_i32(dflag - 1));
+                    update_fip = update_fdp = false;
                     break;
                 case 0x0f: /* fnstcw mem */
                     gen_helper_fnstcw(s->tmp2_i32, cpu_env);
                     tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
                                         s->mem_index, MO_LEUW);
+                    update_fip = update_fdp = false;
                     break;
                 case 0x1d: /* fldt mem */
                     gen_helper_fldt_ST0(cpu_env, s->A0);
@@ -6001,14 +6017,17 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
                     break;
                 case 0x2c: /* frstor mem */
                     gen_helper_frstor(cpu_env, s->A0, tcg_const_i32(dflag - 1));
+                    update_fip = update_fdp = false;
                     break;
                 case 0x2e: /* fnsave mem */
                     gen_helper_fsave(cpu_env, s->A0, tcg_const_i32(dflag - 1));
+                    update_fip = update_fdp = false;
                     break;
                 case 0x2f: /* fnstsw mem */
                     gen_helper_fnstsw(s->tmp2_i32, cpu_env);
                     tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
                                         s->mem_index, MO_LEUW);
+                    update_fip = update_fdp = false;
                     break;
                 case 0x3c: /* fbld */
                     gen_helper_fbld_ST0(cpu_env, s->A0);
@@ -6051,6 +6070,7 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
                     case 0: /* fnop */
                         /* check exceptions (FreeBSD FPU probe) */
                         gen_helper_fwait(cpu_env);
+                        update_fip = update_fdp = false;
                         break;
                     default:
                         goto unknown_op;
@@ -6220,9 +6240,11 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
                         break;
                     case 2: /* fclex */
                         gen_helper_fclex(cpu_env);
+                        update_fip = update_fdp = false;
                         break;
                     case 3: /* fninit */
                         gen_helper_fninit(cpu_env);
+                        update_fip = update_fdp = false;
                         break;
                     case 4: /* fsetpm (287 only, just do nop here) */
                         break;
@@ -6343,6 +6365,27 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
                     goto unknown_op;
                 }
             }
+
+            if (update_fip) {
+                tcg_gen_ld32u_tl(s->T0, cpu_env,
+                                offsetof(CPUX86State, segs[R_CS].selector));
+                tcg_gen_st16_tl(s->T0, cpu_env, offsetof(CPUX86State, fpcs));
+
+                tcg_gen_movi_tl(s->T0, pc_start - s->cs_base);
+                tcg_gen_st_tl(s->T0, cpu_env, offsetof(CPUX86State, fpip));
+            }
+
+            if (update_fdp) {
+                if (s->override >= 0) {
+                    last_seg = s->override;
+                }
+                tcg_gen_ld32u_tl(s->T0, cpu_env,
+                                 offsetof(CPUX86State,
+                                 segs[last_seg].selector));
+                tcg_gen_st16_tl(s->T0, cpu_env, offsetof(CPUX86State, fpds));
+
+                tcg_gen_st_tl(last_addr, cpu_env, offsetof(CPUX86State, fpdp));
+            }
         }
         break;
         /************************/
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 2/2] target/i386: Correct implementation for FCS, FIP,  FDS and FDP
  2021-05-07  8:00 ` [PATCH v4 2/2] target/i386: Correct implementation for FCS, FIP, FDS and FDP Ziqiao Kong
@ 2021-05-13 16:44   ` Ziqiao Kong
  2021-05-17 20:29   ` Eduardo Habkost
  1 sibling, 0 replies; 10+ messages in thread
From: Ziqiao Kong @ 2021-05-13 16:44 UTC (permalink / raw)
  To: qemu-devel; +Cc: pbonzini, richard.henderson, ehabkost, Ziqiao Kong

Ping.

On 5/7/21, Ziqiao Kong <ziqiaokong@gmail.com> wrote:
> Changes since v3:
>  - Split the long patches to series to make review easier.
>  - Fix the coding style problems in v3.
>
> Changes since v2:
>  - Change the sequence of fpcs, fpds, fpip and fpdp in CPUX86State.
>  - Use stl instead of stw in do_fstenv.
>  - Move variables to floats instruction case block.
>  - Move last accessed memory operand to a temp variable to avoid another
> load.
>  - Move segment selectors instead of segment base to fpcs and fpds.
>  - Fix some code stype problems for the original code in floats case block.
>
> Signed-off-by: Ziqiao Kong <ziqiaokong@gmail.com>
> ---
>  target/i386/cpu.h            |  4 +++
>  target/i386/tcg/fpu_helper.c | 48 ++++++++++++++++++++++--------------
>  target/i386/tcg/translate.c  | 45 ++++++++++++++++++++++++++++++++-
>  3 files changed, 77 insertions(+), 20 deletions(-)
>
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 570f916878..241945320b 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -705,6 +705,8 @@ typedef uint64_t FeatureWordArray[FEATURE_WORDS];
>  #define CPUID_7_0_EBX_INVPCID           (1U << 10)
>  /* Restricted Transactional Memory */
>  #define CPUID_7_0_EBX_RTM               (1U << 11)
> +/* Deprecates FPU CS and FPU DS values */
> +#define CPUID_7_0_EBX_FCS_FDS           (1U << 13)
>  /* Memory Protection Extension */
>  #define CPUID_7_0_EBX_MPX               (1U << 14)
>  /* AVX-512 Foundation */
> @@ -1440,6 +1442,8 @@ typedef struct CPUX86State {
>      FPReg fpregs[8];
>      /* KVM-only so far */
>      uint16_t fpop;
> +    uint16_t fpcs;
> +    uint16_t fpds;
>      uint64_t fpip;
>      uint64_t fpdp;
>
> diff --git a/target/i386/tcg/fpu_helper.c b/target/i386/tcg/fpu_helper.c
> index 60ed93520a..f1a8717ed8 100644
> --- a/target/i386/tcg/fpu_helper.c
> +++ b/target/i386/tcg/fpu_helper.c
> @@ -766,6 +766,10 @@ void helper_fninit(CPUX86State *env)
>  {
>      env->fpus = 0;
>      env->fpstt = 0;
> +    env->fpcs = 0;
> +    env->fpip = 0;
> +    env->fpds = 0;
> +    env->fpdp = 0;
>      cpu_set_fpuc(env, 0x37f);
>      env->fptags[0] = 1;
>      env->fptags[1] = 1;
> @@ -2368,6 +2372,7 @@ static void do_fstenv(CPUX86State *env, target_ulong
> ptr, int data32,
>  {
>      int fpus, fptag, exp, i;
>      uint64_t mant;
> +    uint16_t fpcs, fpds;
>      CPU_LDoubleU tmp;
>
>      fpus = (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11;
> @@ -2390,24 +2395,39 @@ static void do_fstenv(CPUX86State *env, target_ulong
> ptr, int data32,
>              }
>          }
>      }
> +
> +    /*
> +     * If CR0.PE = 1, each instruction saves FCS and FDS into memory. If
> +     * CPUID.(EAX=07H,ECX=0H):EBX[bit 13] = 1, the processor deprecates
> +     * FCS and FDS; it saves each as 0000H.
> +     */
> +    if (!(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_FCS_FDS)
> +        && (env->cr[0] & CR0_PE_MASK)) {
> +        fpcs = env->fpcs;
> +        fpds = env->fpds;
> +    } else {
> +        fpcs = 0;
> +        fpds = 0;
> +    }
> +
>      if (data32) {
>          /* 32 bit */
>          cpu_stl_data_ra(env, ptr, env->fpuc, retaddr);
>          cpu_stl_data_ra(env, ptr + 4, fpus, retaddr);
>          cpu_stl_data_ra(env, ptr + 8, fptag, retaddr);
> -        cpu_stl_data_ra(env, ptr + 12, 0, retaddr); /* fpip */
> -        cpu_stl_data_ra(env, ptr + 16, 0, retaddr); /* fpcs */
> -        cpu_stl_data_ra(env, ptr + 20, 0, retaddr); /* fpoo */
> -        cpu_stl_data_ra(env, ptr + 24, 0, retaddr); /* fpos */
> +        cpu_stl_data_ra(env, ptr + 12, env->fpip, retaddr); /* fpip */
> +        cpu_stl_data_ra(env, ptr + 16, fpcs, retaddr); /* fpcs */
> +        cpu_stl_data_ra(env, ptr + 20, env->fpdp, retaddr); /* fpdp */
> +        cpu_stl_data_ra(env, ptr + 24, fpds, retaddr); /* fpds */
>      } else {
>          /* 16 bit */
>          cpu_stw_data_ra(env, ptr, env->fpuc, retaddr);
>          cpu_stw_data_ra(env, ptr + 2, fpus, retaddr);
>          cpu_stw_data_ra(env, ptr + 4, fptag, retaddr);
> -        cpu_stw_data_ra(env, ptr + 6, 0, retaddr);
> -        cpu_stw_data_ra(env, ptr + 8, 0, retaddr);
> -        cpu_stw_data_ra(env, ptr + 10, 0, retaddr);
> -        cpu_stw_data_ra(env, ptr + 12, 0, retaddr);
> +        cpu_stw_data_ra(env, ptr + 6, env->fpip, retaddr);
> +        cpu_stw_data_ra(env, ptr + 8, fpcs, retaddr);
> +        cpu_stw_data_ra(env, ptr + 10, env->fpdp, retaddr);
> +        cpu_stw_data_ra(env, ptr + 12, fpds, retaddr);
>      }
>  }
>
> @@ -2473,17 +2493,7 @@ void helper_fsave(CPUX86State *env, target_ulong ptr,
> int data32)
>      }
>
>      /* fninit */
> -    env->fpus = 0;
> -    env->fpstt = 0;
> -    cpu_set_fpuc(env, 0x37f);
> -    env->fptags[0] = 1;
> -    env->fptags[1] = 1;
> -    env->fptags[2] = 1;
> -    env->fptags[3] = 1;
> -    env->fptags[4] = 1;
> -    env->fptags[5] = 1;
> -    env->fptags[6] = 1;
> -    env->fptags[7] = 1;
> +    helper_fninit(env);
>  }
>
>  void helper_frstor(CPUX86State *env, target_ulong ptr, int data32)
> diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
> index 52e94fe106..59647ea5b7 100644
> --- a/target/i386/tcg/translate.c
> +++ b/target/i386/tcg/translate.c
> @@ -5839,6 +5839,11 @@ static target_ulong disas_insn(DisasContext *s,
> CPUState *cpu)
>          /* floats */
>      case 0xd8 ... 0xdf:
>          {
> +            TCGv last_addr = tcg_temp_new();
> +            int last_seg;
> +            bool update_fdp = false;
> +            bool update_fip = true;
> +
>              if (s->flags & (HF_EM_MASK | HF_TS_MASK)) {
>                  /* if CR0.EM or CR0.TS are set, generate an FPU exception
> */
>                  /* XXX: what to do if illegal op ? */
> @@ -5851,7 +5856,14 @@ static target_ulong disas_insn(DisasContext *s,
> CPUState *cpu)
>              op = ((b & 7) << 3) | ((modrm >> 3) & 7);
>              if (mod != 3) {
>                  /* memory op */
> -                gen_lea_modrm(env, s, modrm);
> +                AddressParts a = gen_lea_modrm_0(env, s, modrm);
> +                TCGv ea = gen_lea_modrm_1(s, a);
> +
> +                update_fdp = true;
> +                last_seg = a.def_seg;
> +                tcg_gen_mov_tl(last_addr, ea);
> +                gen_lea_v_seg(s, s->aflag, ea, a.def_seg, s->override);
> +
>                  switch (op) {
>                  case 0x00 ... 0x07: /* fxxxs */
>                  case 0x10 ... 0x17: /* fixxxl */
> @@ -5978,19 +5990,23 @@ static target_ulong disas_insn(DisasContext *s,
> CPUState *cpu)
>                      break;
>                  case 0x0c: /* fldenv mem */
>                      gen_helper_fldenv(cpu_env, s->A0, tcg_const_i32(dflag -
> 1));
> +                    update_fip = update_fdp = false;
>                      break;
>                  case 0x0d: /* fldcw mem */
>                      tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
>                                          s->mem_index, MO_LEUW);
>                      gen_helper_fldcw(cpu_env, s->tmp2_i32);
> +                    update_fip = update_fdp = false;
>                      break;
>                  case 0x0e: /* fnstenv mem */
>                      gen_helper_fstenv(cpu_env, s->A0, tcg_const_i32(dflag -
> 1));
> +                    update_fip = update_fdp = false;
>                      break;
>                  case 0x0f: /* fnstcw mem */
>                      gen_helper_fnstcw(s->tmp2_i32, cpu_env);
>                      tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
>                                          s->mem_index, MO_LEUW);
> +                    update_fip = update_fdp = false;
>                      break;
>                  case 0x1d: /* fldt mem */
>                      gen_helper_fldt_ST0(cpu_env, s->A0);
> @@ -6001,14 +6017,17 @@ static target_ulong disas_insn(DisasContext *s,
> CPUState *cpu)
>                      break;
>                  case 0x2c: /* frstor mem */
>                      gen_helper_frstor(cpu_env, s->A0, tcg_const_i32(dflag -
> 1));
> +                    update_fip = update_fdp = false;
>                      break;
>                  case 0x2e: /* fnsave mem */
>                      gen_helper_fsave(cpu_env, s->A0, tcg_const_i32(dflag -
> 1));
> +                    update_fip = update_fdp = false;
>                      break;
>                  case 0x2f: /* fnstsw mem */
>                      gen_helper_fnstsw(s->tmp2_i32, cpu_env);
>                      tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
>                                          s->mem_index, MO_LEUW);
> +                    update_fip = update_fdp = false;
>                      break;
>                  case 0x3c: /* fbld */
>                      gen_helper_fbld_ST0(cpu_env, s->A0);
> @@ -6051,6 +6070,7 @@ static target_ulong disas_insn(DisasContext *s,
> CPUState *cpu)
>                      case 0: /* fnop */
>                          /* check exceptions (FreeBSD FPU probe) */
>                          gen_helper_fwait(cpu_env);
> +                        update_fip = update_fdp = false;
>                          break;
>                      default:
>                          goto unknown_op;
> @@ -6220,9 +6240,11 @@ static target_ulong disas_insn(DisasContext *s,
> CPUState *cpu)
>                          break;
>                      case 2: /* fclex */
>                          gen_helper_fclex(cpu_env);
> +                        update_fip = update_fdp = false;
>                          break;
>                      case 3: /* fninit */
>                          gen_helper_fninit(cpu_env);
> +                        update_fip = update_fdp = false;
>                          break;
>                      case 4: /* fsetpm (287 only, just do nop here) */
>                          break;
> @@ -6343,6 +6365,27 @@ static target_ulong disas_insn(DisasContext *s,
> CPUState *cpu)
>                      goto unknown_op;
>                  }
>              }
> +
> +            if (update_fip) {
> +                tcg_gen_ld32u_tl(s->T0, cpu_env,
> +                                offsetof(CPUX86State,
> segs[R_CS].selector));
> +                tcg_gen_st16_tl(s->T0, cpu_env, offsetof(CPUX86State,
> fpcs));
> +
> +                tcg_gen_movi_tl(s->T0, pc_start - s->cs_base);
> +                tcg_gen_st_tl(s->T0, cpu_env, offsetof(CPUX86State,
> fpip));
> +            }
> +
> +            if (update_fdp) {
> +                if (s->override >= 0) {
> +                    last_seg = s->override;
> +                }
> +                tcg_gen_ld32u_tl(s->T0, cpu_env,
> +                                 offsetof(CPUX86State,
> +                                 segs[last_seg].selector));
> +                tcg_gen_st16_tl(s->T0, cpu_env, offsetof(CPUX86State,
> fpds));
> +
> +                tcg_gen_st_tl(last_addr, cpu_env, offsetof(CPUX86State,
> fpdp));
> +            }
>          }
>          break;
>          /************************/
> --
> 2.25.1
>
>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 1/2] target/i386: Trivial code motion
  2021-05-07  8:00 [PATCH v4 1/2] target/i386: Trivial code motion Ziqiao Kong
  2021-05-07  8:00 ` [PATCH v4 2/2] target/i386: Correct implementation for FCS, FIP, FDS and FDP Ziqiao Kong
@ 2021-05-17 20:16 ` Eduardo Habkost
  2021-05-18  2:53   ` Ziqiao Kong
  1 sibling, 1 reply; 10+ messages in thread
From: Eduardo Habkost @ 2021-05-17 20:16 UTC (permalink / raw)
  To: Ziqiao Kong; +Cc: pbonzini, richard.henderson, qemu-devel

On Fri, May 07, 2021 at 04:00:56PM +0800, Ziqiao Kong wrote:
> Move the float translation case to a new block by a new pair of braces.

If you are just adding braces around the code, do you really need
to reindent all the code?  I don't see any mention of `switch`
statements on style.rst, but I see 235 existing cases where the
brackets are aligned below the `c` in `case`.

In either case, I'm looking for a description of "why", not
"what", but I couldn't find it.  Why are the braces necessary or
useful here?

> 
> Fix some coding style problem for the old code.
> 
> Signed-off-by: Ziqiao Kong <ziqiaokong@gmail.com>
> ---
>  VERSION                     |   2 +-
>  target/i386/tcg/translate.c | 953 ++++++++++++++++++------------------
>  2 files changed, 481 insertions(+), 474 deletions(-)
> 
> diff --git a/VERSION b/VERSION
> index e479d55a5e..09b254e90c 100644
> --- a/VERSION
> +++ b/VERSION
> @@ -1 +1 @@
> -5.2.95
> +6.0.0
> diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
> index 880bc45561..52e94fe106 100644
> --- a/target/i386/tcg/translate.c
> +++ b/target/i386/tcg/translate.c
> @@ -5838,503 +5838,510 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
>          /************************/
>          /* floats */
>      case 0xd8 ... 0xdf:
> -        if (s->flags & (HF_EM_MASK | HF_TS_MASK)) {
> -            /* if CR0.EM or CR0.TS are set, generate an FPU exception */
> -            /* XXX: what to do if illegal op ? */
> -            gen_exception(s, EXCP07_PREX, pc_start - s->cs_base);
> -            break;
> -        }
> -        modrm = x86_ldub_code(env, s);
> -        mod = (modrm >> 6) & 3;
> -        rm = modrm & 7;
> -        op = ((b & 7) << 3) | ((modrm >> 3) & 7);
> -        if (mod != 3) {
> -            /* memory op */
> -            gen_lea_modrm(env, s, modrm);
> -            switch(op) {
> -            case 0x00 ... 0x07: /* fxxxs */
> -            case 0x10 ... 0x17: /* fixxxl */
> -            case 0x20 ... 0x27: /* fxxxl */
> -            case 0x30 ... 0x37: /* fixxx */
> -                {
> -                    int op1;
> -                    op1 = op & 7;
> -
> -                    switch(op >> 4) {
> -                    case 0:
> -                        tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> -                                            s->mem_index, MO_LEUL);
> -                        gen_helper_flds_FT0(cpu_env, s->tmp2_i32);
> -                        break;
> -                    case 1:
> -                        tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> -                                            s->mem_index, MO_LEUL);
> -                        gen_helper_fildl_FT0(cpu_env, s->tmp2_i32);
> -                        break;
> -                    case 2:
> -                        tcg_gen_qemu_ld_i64(s->tmp1_i64, s->A0,
> -                                            s->mem_index, MO_LEQ);
> -                        gen_helper_fldl_FT0(cpu_env, s->tmp1_i64);
> -                        break;
> -                    case 3:
> -                    default:
> -                        tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> -                                            s->mem_index, MO_LESW);
> -                        gen_helper_fildl_FT0(cpu_env, s->tmp2_i32);
> -                        break;
> -                    }
> -
> -                    gen_helper_fp_arith_ST0_FT0(op1);
> -                    if (op1 == 3) {
> -                        /* fcomp needs pop */
> -                        gen_helper_fpop(cpu_env);
> -                    }
> -                }
> +        {
> +            if (s->flags & (HF_EM_MASK | HF_TS_MASK)) {
> +                /* if CR0.EM or CR0.TS are set, generate an FPU exception */
> +                /* XXX: what to do if illegal op ? */
> +                gen_exception(s, EXCP07_PREX, pc_start - s->cs_base);
>                  break;
> -            case 0x08: /* flds */
> -            case 0x0a: /* fsts */
> -            case 0x0b: /* fstps */
> -            case 0x18 ... 0x1b: /* fildl, fisttpl, fistl, fistpl */
> -            case 0x28 ... 0x2b: /* fldl, fisttpll, fstl, fstpl */
> -            case 0x38 ... 0x3b: /* filds, fisttps, fists, fistps */
> -                switch(op & 7) {
> -                case 0:
> -                    switch(op >> 4) {
> -                    case 0:
> -                        tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> -                                            s->mem_index, MO_LEUL);
> -                        gen_helper_flds_ST0(cpu_env, s->tmp2_i32);
> -                        break;
> -                    case 1:
> -                        tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> -                                            s->mem_index, MO_LEUL);
> -                        gen_helper_fildl_ST0(cpu_env, s->tmp2_i32);
> -                        break;
> -                    case 2:
> -                        tcg_gen_qemu_ld_i64(s->tmp1_i64, s->A0,
> -                                            s->mem_index, MO_LEQ);
> -                        gen_helper_fldl_ST0(cpu_env, s->tmp1_i64);
> -                        break;
> -                    case 3:
> -                    default:
> -                        tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> -                                            s->mem_index, MO_LESW);
> -                        gen_helper_fildl_ST0(cpu_env, s->tmp2_i32);
> -                        break;
> -                    }
> -                    break;
> -                case 1:
> -                    /* XXX: the corresponding CPUID bit must be tested ! */
> -                    switch(op >> 4) {
> -                    case 1:
> -                        gen_helper_fisttl_ST0(s->tmp2_i32, cpu_env);
> -                        tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> -                                            s->mem_index, MO_LEUL);
> -                        break;
> -                    case 2:
> -                        gen_helper_fisttll_ST0(s->tmp1_i64, cpu_env);
> -                        tcg_gen_qemu_st_i64(s->tmp1_i64, s->A0,
> -                                            s->mem_index, MO_LEQ);
> -                        break;
> -                    case 3:
> -                    default:
> -                        gen_helper_fistt_ST0(s->tmp2_i32, cpu_env);
> -                        tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> -                                            s->mem_index, MO_LEUW);
> -                        break;
> -                    }
> -                    gen_helper_fpop(cpu_env);
> -                    break;
> -                default:
> -                    switch(op >> 4) {
> -                    case 0:
> -                        gen_helper_fsts_ST0(s->tmp2_i32, cpu_env);
> -                        tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> -                                            s->mem_index, MO_LEUL);
> -                        break;
> -                    case 1:
> -                        gen_helper_fistl_ST0(s->tmp2_i32, cpu_env);
> -                        tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> -                                            s->mem_index, MO_LEUL);
> -                        break;
> -                    case 2:
> -                        gen_helper_fstl_ST0(s->tmp1_i64, cpu_env);
> -                        tcg_gen_qemu_st_i64(s->tmp1_i64, s->A0,
> -                                            s->mem_index, MO_LEQ);
> -                        break;
> -                    case 3:
> -                    default:
> -                        gen_helper_fist_ST0(s->tmp2_i32, cpu_env);
> -                        tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> -                                            s->mem_index, MO_LEUW);
> -                        break;
> -                    }
> -                    if ((op & 7) == 3)
> -                        gen_helper_fpop(cpu_env);
> -                    break;
> -                }
> -                break;
> -            case 0x0c: /* fldenv mem */
> -                gen_helper_fldenv(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> -                break;
> -            case 0x0d: /* fldcw mem */
> -                tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> -                                    s->mem_index, MO_LEUW);
> -                gen_helper_fldcw(cpu_env, s->tmp2_i32);
> -                break;
> -            case 0x0e: /* fnstenv mem */
> -                gen_helper_fstenv(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> -                break;
> -            case 0x0f: /* fnstcw mem */
> -                gen_helper_fnstcw(s->tmp2_i32, cpu_env);
> -                tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> -                                    s->mem_index, MO_LEUW);
> -                break;
> -            case 0x1d: /* fldt mem */
> -                gen_helper_fldt_ST0(cpu_env, s->A0);
> -                break;
> -            case 0x1f: /* fstpt mem */
> -                gen_helper_fstt_ST0(cpu_env, s->A0);
> -                gen_helper_fpop(cpu_env);
> -                break;
> -            case 0x2c: /* frstor mem */
> -                gen_helper_frstor(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> -                break;
> -            case 0x2e: /* fnsave mem */
> -                gen_helper_fsave(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> -                break;
> -            case 0x2f: /* fnstsw mem */
> -                gen_helper_fnstsw(s->tmp2_i32, cpu_env);
> -                tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> -                                    s->mem_index, MO_LEUW);
> -                break;
> -            case 0x3c: /* fbld */
> -                gen_helper_fbld_ST0(cpu_env, s->A0);
> -                break;
> -            case 0x3e: /* fbstp */
> -                gen_helper_fbst_ST0(cpu_env, s->A0);
> -                gen_helper_fpop(cpu_env);
> -                break;
> -            case 0x3d: /* fildll */
> -                tcg_gen_qemu_ld_i64(s->tmp1_i64, s->A0, s->mem_index, MO_LEQ);
> -                gen_helper_fildll_ST0(cpu_env, s->tmp1_i64);
> -                break;
> -            case 0x3f: /* fistpll */
> -                gen_helper_fistll_ST0(s->tmp1_i64, cpu_env);
> -                tcg_gen_qemu_st_i64(s->tmp1_i64, s->A0, s->mem_index, MO_LEQ);
> -                gen_helper_fpop(cpu_env);
> -                break;
> -            default:
> -                goto unknown_op;
>              }
> -        } else {
> -            /* register float ops */
> -            opreg = rm;
> +            modrm = x86_ldub_code(env, s);
> +            mod = (modrm >> 6) & 3;
> +            rm = modrm & 7;
> +            op = ((b & 7) << 3) | ((modrm >> 3) & 7);
> +            if (mod != 3) {
> +                /* memory op */
> +                gen_lea_modrm(env, s, modrm);
> +                switch (op) {
> +                case 0x00 ... 0x07: /* fxxxs */
> +                case 0x10 ... 0x17: /* fixxxl */
> +                case 0x20 ... 0x27: /* fxxxl */
> +                case 0x30 ... 0x37: /* fixxx */
> +                    {
> +                        int op1;
> +                        op1 = op & 7;
>  
> -            switch(op) {
> -            case 0x08: /* fld sti */
> -                gen_helper_fpush(cpu_env);
> -                gen_helper_fmov_ST0_STN(cpu_env,
> -                                        tcg_const_i32((opreg + 1) & 7));
> -                break;
> -            case 0x09: /* fxchg sti */
> -            case 0x29: /* fxchg4 sti, undocumented op */
> -            case 0x39: /* fxchg7 sti, undocumented op */
> -                gen_helper_fxchg_ST0_STN(cpu_env, tcg_const_i32(opreg));
> -                break;
> -            case 0x0a: /* grp d9/2 */
> -                switch(rm) {
> -                case 0: /* fnop */
> -                    /* check exceptions (FreeBSD FPU probe) */
> -                    gen_helper_fwait(cpu_env);
> -                    break;
> -                default:
> -                    goto unknown_op;
> -                }
> -                break;
> -            case 0x0c: /* grp d9/4 */
> -                switch(rm) {
> -                case 0: /* fchs */
> -                    gen_helper_fchs_ST0(cpu_env);
> -                    break;
> -                case 1: /* fabs */
> -                    gen_helper_fabs_ST0(cpu_env);
> -                    break;
> -                case 4: /* ftst */
> -                    gen_helper_fldz_FT0(cpu_env);
> -                    gen_helper_fcom_ST0_FT0(cpu_env);
> -                    break;
> -                case 5: /* fxam */
> -                    gen_helper_fxam_ST0(cpu_env);
> -                    break;
> -                default:
> -                    goto unknown_op;
> -                }
> -                break;
> -            case 0x0d: /* grp d9/5 */
> -                {
> -                    switch(rm) {
> -                    case 0:
> -                        gen_helper_fpush(cpu_env);
> -                        gen_helper_fld1_ST0(cpu_env);
> -                        break;
> -                    case 1:
> -                        gen_helper_fpush(cpu_env);
> -                        gen_helper_fldl2t_ST0(cpu_env);
> -                        break;
> -                    case 2:
> -                        gen_helper_fpush(cpu_env);
> -                        gen_helper_fldl2e_ST0(cpu_env);
> -                        break;
> -                    case 3:
> -                        gen_helper_fpush(cpu_env);
> -                        gen_helper_fldpi_ST0(cpu_env);
> -                        break;
> -                    case 4:
> -                        gen_helper_fpush(cpu_env);
> -                        gen_helper_fldlg2_ST0(cpu_env);
> -                        break;
> -                    case 5:
> -                        gen_helper_fpush(cpu_env);
> -                        gen_helper_fldln2_ST0(cpu_env);
> -                        break;
> -                    case 6:
> -                        gen_helper_fpush(cpu_env);
> -                        gen_helper_fldz_ST0(cpu_env);
> -                        break;
> -                    default:
> -                        goto unknown_op;
> -                    }
> -                }
> -                break;
> -            case 0x0e: /* grp d9/6 */
> -                switch(rm) {
> -                case 0: /* f2xm1 */
> -                    gen_helper_f2xm1(cpu_env);
> -                    break;
> -                case 1: /* fyl2x */
> -                    gen_helper_fyl2x(cpu_env);
> -                    break;
> -                case 2: /* fptan */
> -                    gen_helper_fptan(cpu_env);
> -                    break;
> -                case 3: /* fpatan */
> -                    gen_helper_fpatan(cpu_env);
> -                    break;
> -                case 4: /* fxtract */
> -                    gen_helper_fxtract(cpu_env);
> -                    break;
> -                case 5: /* fprem1 */
> -                    gen_helper_fprem1(cpu_env);
> -                    break;
> -                case 6: /* fdecstp */
> -                    gen_helper_fdecstp(cpu_env);
> -                    break;
> -                default:
> -                case 7: /* fincstp */
> -                    gen_helper_fincstp(cpu_env);
> -                    break;
> -                }
> -                break;
> -            case 0x0f: /* grp d9/7 */
> -                switch(rm) {
> -                case 0: /* fprem */
> -                    gen_helper_fprem(cpu_env);
> -                    break;
> -                case 1: /* fyl2xp1 */
> -                    gen_helper_fyl2xp1(cpu_env);
> -                    break;
> -                case 2: /* fsqrt */
> -                    gen_helper_fsqrt(cpu_env);
> -                    break;
> -                case 3: /* fsincos */
> -                    gen_helper_fsincos(cpu_env);
> -                    break;
> -                case 5: /* fscale */
> -                    gen_helper_fscale(cpu_env);
> -                    break;
> -                case 4: /* frndint */
> -                    gen_helper_frndint(cpu_env);
> -                    break;
> -                case 6: /* fsin */
> -                    gen_helper_fsin(cpu_env);
> -                    break;
> -                default:
> -                case 7: /* fcos */
> -                    gen_helper_fcos(cpu_env);
> -                    break;
> -                }
> -                break;
> -            case 0x00: case 0x01: case 0x04 ... 0x07: /* fxxx st, sti */
> -            case 0x20: case 0x21: case 0x24 ... 0x27: /* fxxx sti, st */
> -            case 0x30: case 0x31: case 0x34 ... 0x37: /* fxxxp sti, st */
> -                {
> -                    int op1;
> +                        switch (op >> 4) {
> +                        case 0:
> +                            tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> +                                                s->mem_index, MO_LEUL);
> +                            gen_helper_flds_FT0(cpu_env, s->tmp2_i32);
> +                            break;
> +                        case 1:
> +                            tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> +                                                s->mem_index, MO_LEUL);
> +                            gen_helper_fildl_FT0(cpu_env, s->tmp2_i32);
> +                            break;
> +                        case 2:
> +                            tcg_gen_qemu_ld_i64(s->tmp1_i64, s->A0,
> +                                                s->mem_index, MO_LEQ);
> +                            gen_helper_fldl_FT0(cpu_env, s->tmp1_i64);
> +                            break;
> +                        case 3:
> +                        default:
> +                            tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> +                                                s->mem_index, MO_LESW);
> +                            gen_helper_fildl_FT0(cpu_env, s->tmp2_i32);
> +                            break;
> +                        }
>  
> -                    op1 = op & 7;
> -                    if (op >= 0x20) {
> -                        gen_helper_fp_arith_STN_ST0(op1, opreg);
> -                        if (op >= 0x30)
> -                            gen_helper_fpop(cpu_env);
> -                    } else {
> -                        gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
>                          gen_helper_fp_arith_ST0_FT0(op1);
> +                        if (op1 == 3) {
> +                            /* fcomp needs pop */
> +                            gen_helper_fpop(cpu_env);
> +                        }
>                      }
> -                }
> -                break;
> -            case 0x02: /* fcom */
> -            case 0x22: /* fcom2, undocumented op */
> -                gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> -                gen_helper_fcom_ST0_FT0(cpu_env);
> -                break;
> -            case 0x03: /* fcomp */
> -            case 0x23: /* fcomp3, undocumented op */
> -            case 0x32: /* fcomp5, undocumented op */
> -                gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> -                gen_helper_fcom_ST0_FT0(cpu_env);
> -                gen_helper_fpop(cpu_env);
> -                break;
> -            case 0x15: /* da/5 */
> -                switch(rm) {
> -                case 1: /* fucompp */
> -                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(1));
> -                    gen_helper_fucom_ST0_FT0(cpu_env);
> -                    gen_helper_fpop(cpu_env);
> -                    gen_helper_fpop(cpu_env);
>                      break;
> -                default:
> -                    goto unknown_op;
> -                }
> -                break;
> -            case 0x1c:
> -                switch(rm) {
> -                case 0: /* feni (287 only, just do nop here) */
> +                case 0x08: /* flds */
> +                case 0x0a: /* fsts */
> +                case 0x0b: /* fstps */
> +                case 0x18 ... 0x1b: /* fildl, fisttpl, fistl, fistpl */
> +                case 0x28 ... 0x2b: /* fldl, fisttpll, fstl, fstpl */
> +                case 0x38 ... 0x3b: /* filds, fisttps, fists, fistps */
> +                    switch (op & 7) {
> +                    case 0:
> +                        switch (op >> 4) {
> +                        case 0:
> +                            tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> +                                                s->mem_index, MO_LEUL);
> +                            gen_helper_flds_ST0(cpu_env, s->tmp2_i32);
> +                            break;
> +                        case 1:
> +                            tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> +                                                s->mem_index, MO_LEUL);
> +                            gen_helper_fildl_ST0(cpu_env, s->tmp2_i32);
> +                            break;
> +                        case 2:
> +                            tcg_gen_qemu_ld_i64(s->tmp1_i64, s->A0,
> +                                                s->mem_index, MO_LEQ);
> +                            gen_helper_fldl_ST0(cpu_env, s->tmp1_i64);
> +                            break;
> +                        case 3:
> +                        default:
> +                            tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> +                                                s->mem_index, MO_LESW);
> +                            gen_helper_fildl_ST0(cpu_env, s->tmp2_i32);
> +                            break;
> +                        }
> +                        break;
> +                    case 1:
> +                        /* XXX: the corresponding CPUID bit must be tested ! */
> +                        switch (op >> 4) {
> +                        case 1:
> +                            gen_helper_fisttl_ST0(s->tmp2_i32, cpu_env);
> +                            tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> +                                                s->mem_index, MO_LEUL);
> +                            break;
> +                        case 2:
> +                            gen_helper_fisttll_ST0(s->tmp1_i64, cpu_env);
> +                            tcg_gen_qemu_st_i64(s->tmp1_i64, s->A0,
> +                                                s->mem_index, MO_LEQ);
> +                            break;
> +                        case 3:
> +                        default:
> +                            gen_helper_fistt_ST0(s->tmp2_i32, cpu_env);
> +                            tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> +                                                s->mem_index, MO_LEUW);
> +                            break;
> +                        }
> +                        gen_helper_fpop(cpu_env);
> +                        break;
> +                    default:
> +                        switch (op >> 4) {
> +                        case 0:
> +                            gen_helper_fsts_ST0(s->tmp2_i32, cpu_env);
> +                            tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> +                                                s->mem_index, MO_LEUL);
> +                            break;
> +                        case 1:
> +                            gen_helper_fistl_ST0(s->tmp2_i32, cpu_env);
> +                            tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> +                                                s->mem_index, MO_LEUL);
> +                            break;
> +                        case 2:
> +                            gen_helper_fstl_ST0(s->tmp1_i64, cpu_env);
> +                            tcg_gen_qemu_st_i64(s->tmp1_i64, s->A0,
> +                                                s->mem_index, MO_LEQ);
> +                            break;
> +                        case 3:
> +                        default:
> +                            gen_helper_fist_ST0(s->tmp2_i32, cpu_env);
> +                            tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> +                                                s->mem_index, MO_LEUW);
> +                            break;
> +                        }
> +                        if ((op & 7) == 3) {
> +                            gen_helper_fpop(cpu_env);
> +                        }
> +                        break;
> +                    }
>                      break;
> -                case 1: /* fdisi (287 only, just do nop here) */
> +                case 0x0c: /* fldenv mem */
> +                    gen_helper_fldenv(cpu_env, s->A0, tcg_const_i32(dflag - 1));
>                      break;
> -                case 2: /* fclex */
> -                    gen_helper_fclex(cpu_env);
> +                case 0x0d: /* fldcw mem */
> +                    tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> +                                        s->mem_index, MO_LEUW);
> +                    gen_helper_fldcw(cpu_env, s->tmp2_i32);
>                      break;
> -                case 3: /* fninit */
> -                    gen_helper_fninit(cpu_env);
> +                case 0x0e: /* fnstenv mem */
> +                    gen_helper_fstenv(cpu_env, s->A0, tcg_const_i32(dflag - 1));
>                      break;
> -                case 4: /* fsetpm (287 only, just do nop here) */
> +                case 0x0f: /* fnstcw mem */
> +                    gen_helper_fnstcw(s->tmp2_i32, cpu_env);
> +                    tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> +                                        s->mem_index, MO_LEUW);
>                      break;
> -                default:
> -                    goto unknown_op;
> -                }
> -                break;
> -            case 0x1d: /* fucomi */
> -                if (!(s->cpuid_features & CPUID_CMOV)) {
> -                    goto illegal_op;
> -                }
> -                gen_update_cc_op(s);
> -                gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> -                gen_helper_fucomi_ST0_FT0(cpu_env);
> -                set_cc_op(s, CC_OP_EFLAGS);
> -                break;
> -            case 0x1e: /* fcomi */
> -                if (!(s->cpuid_features & CPUID_CMOV)) {
> -                    goto illegal_op;
> -                }
> -                gen_update_cc_op(s);
> -                gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> -                gen_helper_fcomi_ST0_FT0(cpu_env);
> -                set_cc_op(s, CC_OP_EFLAGS);
> -                break;
> -            case 0x28: /* ffree sti */
> -                gen_helper_ffree_STN(cpu_env, tcg_const_i32(opreg));
> -                break;
> -            case 0x2a: /* fst sti */
> -                gen_helper_fmov_STN_ST0(cpu_env, tcg_const_i32(opreg));
> -                break;
> -            case 0x2b: /* fstp sti */
> -            case 0x0b: /* fstp1 sti, undocumented op */
> -            case 0x3a: /* fstp8 sti, undocumented op */
> -            case 0x3b: /* fstp9 sti, undocumented op */
> -                gen_helper_fmov_STN_ST0(cpu_env, tcg_const_i32(opreg));
> -                gen_helper_fpop(cpu_env);
> -                break;
> -            case 0x2c: /* fucom st(i) */
> -                gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> -                gen_helper_fucom_ST0_FT0(cpu_env);
> -                break;
> -            case 0x2d: /* fucomp st(i) */
> -                gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> -                gen_helper_fucom_ST0_FT0(cpu_env);
> -                gen_helper_fpop(cpu_env);
> -                break;
> -            case 0x33: /* de/3 */
> -                switch(rm) {
> -                case 1: /* fcompp */
> -                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(1));
> -                    gen_helper_fcom_ST0_FT0(cpu_env);
> -                    gen_helper_fpop(cpu_env);
> +                case 0x1d: /* fldt mem */
> +                    gen_helper_fldt_ST0(cpu_env, s->A0);
> +                    break;
> +                case 0x1f: /* fstpt mem */
> +                    gen_helper_fstt_ST0(cpu_env, s->A0);
>                      gen_helper_fpop(cpu_env);
>                      break;
> -                default:
> -                    goto unknown_op;
> -                }
> -                break;
> -            case 0x38: /* ffreep sti, undocumented op */
> -                gen_helper_ffree_STN(cpu_env, tcg_const_i32(opreg));
> -                gen_helper_fpop(cpu_env);
> -                break;
> -            case 0x3c: /* df/4 */
> -                switch(rm) {
> -                case 0:
> +                case 0x2c: /* frstor mem */
> +                    gen_helper_frstor(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> +                    break;
> +                case 0x2e: /* fnsave mem */
> +                    gen_helper_fsave(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> +                    break;
> +                case 0x2f: /* fnstsw mem */
>                      gen_helper_fnstsw(s->tmp2_i32, cpu_env);
> -                    tcg_gen_extu_i32_tl(s->T0, s->tmp2_i32);
> -                    gen_op_mov_reg_v(s, MO_16, R_EAX, s->T0);
> +                    tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> +                                        s->mem_index, MO_LEUW);
> +                    break;
> +                case 0x3c: /* fbld */
> +                    gen_helper_fbld_ST0(cpu_env, s->A0);
> +                    break;
> +                case 0x3e: /* fbstp */
> +                    gen_helper_fbst_ST0(cpu_env, s->A0);
> +                    gen_helper_fpop(cpu_env);
> +                    break;
> +                case 0x3d: /* fildll */
> +                    tcg_gen_qemu_ld_i64(s->tmp1_i64, s->A0,
> +                                        s->mem_index, MO_LEQ);
> +                    gen_helper_fildll_ST0(cpu_env, s->tmp1_i64);
> +                    break;
> +                case 0x3f: /* fistpll */
> +                    gen_helper_fistll_ST0(s->tmp1_i64, cpu_env);
> +                    tcg_gen_qemu_st_i64(s->tmp1_i64, s->A0,
> +                                        s->mem_index, MO_LEQ);
> +                    gen_helper_fpop(cpu_env);
>                      break;
>                  default:
>                      goto unknown_op;
>                  }
> -                break;
> -            case 0x3d: /* fucomip */
> -                if (!(s->cpuid_features & CPUID_CMOV)) {
> -                    goto illegal_op;
> -                }
> -                gen_update_cc_op(s);
> -                gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> -                gen_helper_fucomi_ST0_FT0(cpu_env);
> -                gen_helper_fpop(cpu_env);
> -                set_cc_op(s, CC_OP_EFLAGS);
> -                break;
> -            case 0x3e: /* fcomip */
> -                if (!(s->cpuid_features & CPUID_CMOV)) {
> -                    goto illegal_op;
> -                }
> -                gen_update_cc_op(s);
> -                gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> -                gen_helper_fcomi_ST0_FT0(cpu_env);
> -                gen_helper_fpop(cpu_env);
> -                set_cc_op(s, CC_OP_EFLAGS);
> -                break;
> -            case 0x10 ... 0x13: /* fcmovxx */
> -            case 0x18 ... 0x1b:
> -                {
> -                    int op1;
> -                    TCGLabel *l1;
> -                    static const uint8_t fcmov_cc[8] = {
> -                        (JCC_B << 1),
> -                        (JCC_Z << 1),
> -                        (JCC_BE << 1),
> -                        (JCC_P << 1),
> -                    };
> +            } else {
> +                /* register float ops */
> +                opreg = rm;
> +
> +                switch (op) {
> +                case 0x08: /* fld sti */
> +                    gen_helper_fpush(cpu_env);
> +                    gen_helper_fmov_ST0_STN(cpu_env,
> +                                            tcg_const_i32((opreg + 1) & 7));
> +                    break;
> +                case 0x09: /* fxchg sti */
> +                case 0x29: /* fxchg4 sti, undocumented op */
> +                case 0x39: /* fxchg7 sti, undocumented op */
> +                    gen_helper_fxchg_ST0_STN(cpu_env, tcg_const_i32(opreg));
> +                    break;
> +                case 0x0a: /* grp d9/2 */
> +                    switch (rm) {
> +                    case 0: /* fnop */
> +                        /* check exceptions (FreeBSD FPU probe) */
> +                        gen_helper_fwait(cpu_env);
> +                        break;
> +                    default:
> +                        goto unknown_op;
> +                    }
> +                    break;
> +                case 0x0c: /* grp d9/4 */
> +                    switch (rm) {
> +                    case 0: /* fchs */
> +                        gen_helper_fchs_ST0(cpu_env);
> +                        break;
> +                    case 1: /* fabs */
> +                        gen_helper_fabs_ST0(cpu_env);
> +                        break;
> +                    case 4: /* ftst */
> +                        gen_helper_fldz_FT0(cpu_env);
> +                        gen_helper_fcom_ST0_FT0(cpu_env);
> +                        break;
> +                    case 5: /* fxam */
> +                        gen_helper_fxam_ST0(cpu_env);
> +                        break;
> +                    default:
> +                        goto unknown_op;
> +                    }
> +                    break;
> +                case 0x0d: /* grp d9/5 */
> +                    {
> +                        switch (rm) {
> +                        case 0:
> +                            gen_helper_fpush(cpu_env);
> +                            gen_helper_fld1_ST0(cpu_env);
> +                            break;
> +                        case 1:
> +                            gen_helper_fpush(cpu_env);
> +                            gen_helper_fldl2t_ST0(cpu_env);
> +                            break;
> +                        case 2:
> +                            gen_helper_fpush(cpu_env);
> +                            gen_helper_fldl2e_ST0(cpu_env);
> +                            break;
> +                        case 3:
> +                            gen_helper_fpush(cpu_env);
> +                            gen_helper_fldpi_ST0(cpu_env);
> +                            break;
> +                        case 4:
> +                            gen_helper_fpush(cpu_env);
> +                            gen_helper_fldlg2_ST0(cpu_env);
> +                            break;
> +                        case 5:
> +                            gen_helper_fpush(cpu_env);
> +                            gen_helper_fldln2_ST0(cpu_env);
> +                            break;
> +                        case 6:
> +                            gen_helper_fpush(cpu_env);
> +                            gen_helper_fldz_ST0(cpu_env);
> +                            break;
> +                        default:
> +                            goto unknown_op;
> +                        }
> +                    }
> +                    break;
> +                case 0x0e: /* grp d9/6 */
> +                    switch (rm) {
> +                    case 0: /* f2xm1 */
> +                        gen_helper_f2xm1(cpu_env);
> +                        break;
> +                    case 1: /* fyl2x */
> +                        gen_helper_fyl2x(cpu_env);
> +                        break;
> +                    case 2: /* fptan */
> +                        gen_helper_fptan(cpu_env);
> +                        break;
> +                    case 3: /* fpatan */
> +                        gen_helper_fpatan(cpu_env);
> +                        break;
> +                    case 4: /* fxtract */
> +                        gen_helper_fxtract(cpu_env);
> +                        break;
> +                    case 5: /* fprem1 */
> +                        gen_helper_fprem1(cpu_env);
> +                        break;
> +                    case 6: /* fdecstp */
> +                        gen_helper_fdecstp(cpu_env);
> +                        break;
> +                    default:
> +                    case 7: /* fincstp */
> +                        gen_helper_fincstp(cpu_env);
> +                        break;
> +                    }
> +                    break;
> +                case 0x0f: /* grp d9/7 */
> +                    switch (rm) {
> +                    case 0: /* fprem */
> +                        gen_helper_fprem(cpu_env);
> +                        break;
> +                    case 1: /* fyl2xp1 */
> +                        gen_helper_fyl2xp1(cpu_env);
> +                        break;
> +                    case 2: /* fsqrt */
> +                        gen_helper_fsqrt(cpu_env);
> +                        break;
> +                    case 3: /* fsincos */
> +                        gen_helper_fsincos(cpu_env);
> +                        break;
> +                    case 5: /* fscale */
> +                        gen_helper_fscale(cpu_env);
> +                        break;
> +                    case 4: /* frndint */
> +                        gen_helper_frndint(cpu_env);
> +                        break;
> +                    case 6: /* fsin */
> +                        gen_helper_fsin(cpu_env);
> +                        break;
> +                    default:
> +                    case 7: /* fcos */
> +                        gen_helper_fcos(cpu_env);
> +                        break;
> +                    }
> +                    break;
> +                case 0x00: case 0x01: case 0x04 ... 0x07: /* fxxx st, sti */
> +                case 0x20: case 0x21: case 0x24 ... 0x27: /* fxxx sti, st */
> +                case 0x30: case 0x31: case 0x34 ... 0x37: /* fxxxp sti, st */
> +                    {
> +                        int op1;
>  
> +                        op1 = op & 7;
> +                        if (op >= 0x20) {
> +                            gen_helper_fp_arith_STN_ST0(op1, opreg);
> +                            if (op >= 0x30) {
> +                                gen_helper_fpop(cpu_env);
> +                            }
> +                        } else {
> +                            gen_helper_fmov_FT0_STN(cpu_env,
> +                                                    tcg_const_i32(opreg));
> +                            gen_helper_fp_arith_ST0_FT0(op1);
> +                        }
> +                    }
> +                    break;
> +                case 0x02: /* fcom */
> +                case 0x22: /* fcom2, undocumented op */
> +                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> +                    gen_helper_fcom_ST0_FT0(cpu_env);
> +                    break;
> +                case 0x03: /* fcomp */
> +                case 0x23: /* fcomp3, undocumented op */
> +                case 0x32: /* fcomp5, undocumented op */
> +                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> +                    gen_helper_fcom_ST0_FT0(cpu_env);
> +                    gen_helper_fpop(cpu_env);
> +                    break;
> +                case 0x15: /* da/5 */
> +                    switch (rm) {
> +                    case 1: /* fucompp */
> +                        gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(1));
> +                        gen_helper_fucom_ST0_FT0(cpu_env);
> +                        gen_helper_fpop(cpu_env);
> +                        gen_helper_fpop(cpu_env);
> +                        break;
> +                    default:
> +                        goto unknown_op;
> +                    }
> +                    break;
> +                case 0x1c:
> +                    switch (rm) {
> +                    case 0: /* feni (287 only, just do nop here) */
> +                        break;
> +                    case 1: /* fdisi (287 only, just do nop here) */
> +                        break;
> +                    case 2: /* fclex */
> +                        gen_helper_fclex(cpu_env);
> +                        break;
> +                    case 3: /* fninit */
> +                        gen_helper_fninit(cpu_env);
> +                        break;
> +                    case 4: /* fsetpm (287 only, just do nop here) */
> +                        break;
> +                    default:
> +                        goto unknown_op;
> +                    }
> +                    break;
> +                case 0x1d: /* fucomi */
>                      if (!(s->cpuid_features & CPUID_CMOV)) {
>                          goto illegal_op;
>                      }
> -                    op1 = fcmov_cc[op & 3] | (((op >> 3) & 1) ^ 1);
> -                    l1 = gen_new_label();
> -                    gen_jcc1_noeob(s, op1, l1);
> -                    gen_helper_fmov_ST0_STN(cpu_env, tcg_const_i32(opreg));
> -                    gen_set_label(l1);
> +                    gen_update_cc_op(s);
> +                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> +                    gen_helper_fucomi_ST0_FT0(cpu_env);
> +                    set_cc_op(s, CC_OP_EFLAGS);
> +                    break;
> +                case 0x1e: /* fcomi */
> +                    if (!(s->cpuid_features & CPUID_CMOV)) {
> +                        goto illegal_op;
> +                    }
> +                    gen_update_cc_op(s);
> +                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> +                    gen_helper_fcomi_ST0_FT0(cpu_env);
> +                    set_cc_op(s, CC_OP_EFLAGS);
> +                    break;
> +                case 0x28: /* ffree sti */
> +                    gen_helper_ffree_STN(cpu_env, tcg_const_i32(opreg));
> +                    break;
> +                case 0x2a: /* fst sti */
> +                    gen_helper_fmov_STN_ST0(cpu_env, tcg_const_i32(opreg));
> +                    break;
> +                case 0x2b: /* fstp sti */
> +                case 0x0b: /* fstp1 sti, undocumented op */
> +                case 0x3a: /* fstp8 sti, undocumented op */
> +                case 0x3b: /* fstp9 sti, undocumented op */
> +                    gen_helper_fmov_STN_ST0(cpu_env, tcg_const_i32(opreg));
> +                    gen_helper_fpop(cpu_env);
> +                    break;
> +                case 0x2c: /* fucom st(i) */
> +                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> +                    gen_helper_fucom_ST0_FT0(cpu_env);
> +                    break;
> +                case 0x2d: /* fucomp st(i) */
> +                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> +                    gen_helper_fucom_ST0_FT0(cpu_env);
> +                    gen_helper_fpop(cpu_env);
> +                    break;
> +                case 0x33: /* de/3 */
> +                    switch (rm) {
> +                    case 1: /* fcompp */
> +                        gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(1));
> +                        gen_helper_fcom_ST0_FT0(cpu_env);
> +                        gen_helper_fpop(cpu_env);
> +                        gen_helper_fpop(cpu_env);
> +                        break;
> +                    default:
> +                        goto unknown_op;
> +                    }
> +                    break;
> +                case 0x38: /* ffreep sti, undocumented op */
> +                    gen_helper_ffree_STN(cpu_env, tcg_const_i32(opreg));
> +                    gen_helper_fpop(cpu_env);
> +                    break;
> +                case 0x3c: /* df/4 */
> +                    switch (rm) {
> +                    case 0:
> +                        gen_helper_fnstsw(s->tmp2_i32, cpu_env);
> +                        tcg_gen_extu_i32_tl(s->T0, s->tmp2_i32);
> +                        gen_op_mov_reg_v(s, MO_16, R_EAX, s->T0);
> +                        break;
> +                    default:
> +                        goto unknown_op;
> +                    }
> +                    break;
> +                case 0x3d: /* fucomip */
> +                    if (!(s->cpuid_features & CPUID_CMOV)) {
> +                        goto illegal_op;
> +                    }
> +                    gen_update_cc_op(s);
> +                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> +                    gen_helper_fucomi_ST0_FT0(cpu_env);
> +                    gen_helper_fpop(cpu_env);
> +                    set_cc_op(s, CC_OP_EFLAGS);
> +                    break;
> +                case 0x3e: /* fcomip */
> +                    if (!(s->cpuid_features & CPUID_CMOV)) {
> +                        goto illegal_op;
> +                    }
> +                    gen_update_cc_op(s);
> +                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> +                    gen_helper_fcomi_ST0_FT0(cpu_env);
> +                    gen_helper_fpop(cpu_env);
> +                    set_cc_op(s, CC_OP_EFLAGS);
> +                    break;
> +                case 0x10 ... 0x13: /* fcmovxx */
> +                case 0x18 ... 0x1b:
> +                    {
> +                        int op1;
> +                        TCGLabel *l1;
> +                        static const uint8_t fcmov_cc[8] = {
> +                            (JCC_B << 1),
> +                            (JCC_Z << 1),
> +                            (JCC_BE << 1),
> +                            (JCC_P << 1),
> +                        };
> +
> +                        if (!(s->cpuid_features & CPUID_CMOV)) {
> +                            goto illegal_op;
> +                        }
> +                        op1 = fcmov_cc[op & 3] | (((op >> 3) & 1) ^ 1);
> +                        l1 = gen_new_label();
> +                        gen_jcc1_noeob(s, op1, l1);
> +                        gen_helper_fmov_ST0_STN(cpu_env, tcg_const_i32(opreg));
> +                        gen_set_label(l1);
> +                    }
> +                    break;
> +                default:
> +                    goto unknown_op;
>                  }
> -                break;
> -            default:
> -                goto unknown_op;
>              }
>          }
>          break;
> -- 
> 2.25.1
> 

-- 
Eduardo



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 2/2] target/i386: Correct implementation for FCS, FIP,  FDS and FDP
  2021-05-07  8:00 ` [PATCH v4 2/2] target/i386: Correct implementation for FCS, FIP, FDS and FDP Ziqiao Kong
  2021-05-13 16:44   ` Ziqiao Kong
@ 2021-05-17 20:29   ` Eduardo Habkost
  2021-05-18  3:06     ` Ziqiao Kong
  1 sibling, 1 reply; 10+ messages in thread
From: Eduardo Habkost @ 2021-05-17 20:29 UTC (permalink / raw)
  To: Ziqiao Kong; +Cc: pbonzini, richard.henderson, qemu-devel

Hi,

Thanks for the patch, and apologies for not reviewing earlier
versions.

On Fri, May 07, 2021 at 04:00:58PM +0800, Ziqiao Kong wrote:
> Changes since v3:
>  - Split the long patches to series to make review easier.
>  - Fix the coding style problems in v3.
> 
> Changes since v2:
>  - Change the sequence of fpcs, fpds, fpip and fpdp in CPUX86State.
>  - Use stl instead of stw in do_fstenv.
>  - Move variables to floats instruction case block.
>  - Move last accessed memory operand to a temp variable to avoid another load.
>  - Move segment selectors instead of segment base to fpcs and fpds.
>  - Fix some code stype problems for the original code in floats case block.

On the next versions, please include the changelog after a "---"
line, so it won't be included in the final commit.

In addition to the changelog, the actual commit message (the text
above "---") needs to include an explanation for the change.  If
you are fixing a bug, please explain what's the bug you are
fixing.


> 
> Signed-off-by: Ziqiao Kong <ziqiaokong@gmail.com>
> ---
>  target/i386/cpu.h            |  4 +++
>  target/i386/tcg/fpu_helper.c | 48 ++++++++++++++++++++++--------------
>  target/i386/tcg/translate.c  | 45 ++++++++++++++++++++++++++++++++-
>  3 files changed, 77 insertions(+), 20 deletions(-)
> 
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 570f916878..241945320b 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -705,6 +705,8 @@ typedef uint64_t FeatureWordArray[FEATURE_WORDS];
>  #define CPUID_7_0_EBX_INVPCID           (1U << 10)
>  /* Restricted Transactional Memory */
>  #define CPUID_7_0_EBX_RTM               (1U << 11)
> +/* Deprecates FPU CS and FPU DS values */
> +#define CPUID_7_0_EBX_FCS_FDS           (1U << 13)
>  /* Memory Protection Extension */
>  #define CPUID_7_0_EBX_MPX               (1U << 14)
>  /* AVX-512 Foundation */
> @@ -1440,6 +1442,8 @@ typedef struct CPUX86State {
>      FPReg fpregs[8];
>      /* KVM-only so far */
>      uint16_t fpop;
> +    uint16_t fpcs;
> +    uint16_t fpds;
>      uint64_t fpip;
>      uint64_t fpdp;
>  
> diff --git a/target/i386/tcg/fpu_helper.c b/target/i386/tcg/fpu_helper.c
> index 60ed93520a..f1a8717ed8 100644
> --- a/target/i386/tcg/fpu_helper.c
> +++ b/target/i386/tcg/fpu_helper.c
> @@ -766,6 +766,10 @@ void helper_fninit(CPUX86State *env)
>  {
>      env->fpus = 0;
>      env->fpstt = 0;
> +    env->fpcs = 0;
> +    env->fpip = 0;
> +    env->fpds = 0;
> +    env->fpdp = 0;
>      cpu_set_fpuc(env, 0x37f);
>      env->fptags[0] = 1;
>      env->fptags[1] = 1;
> @@ -2368,6 +2372,7 @@ static void do_fstenv(CPUX86State *env, target_ulong ptr, int data32,
>  {
>      int fpus, fptag, exp, i;
>      uint64_t mant;
> +    uint16_t fpcs, fpds;
>      CPU_LDoubleU tmp;
>  
>      fpus = (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11;
> @@ -2390,24 +2395,39 @@ static void do_fstenv(CPUX86State *env, target_ulong ptr, int data32,
>              }
>          }
>      }
> +
> +    /*
> +     * If CR0.PE = 1, each instruction saves FCS and FDS into memory. If
> +     * CPUID.(EAX=07H,ECX=0H):EBX[bit 13] = 1, the processor deprecates
> +     * FCS and FDS; it saves each as 0000H.
> +     */
> +    if (!(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_FCS_FDS)
> +        && (env->cr[0] & CR0_PE_MASK)) {
> +        fpcs = env->fpcs;
> +        fpds = env->fpds;


If you want to start supporting this feature flag, I suggest
moving this to a separate patch.  The description of this patch
seems to imply it is just a bug fix, not the implementation of a
new feature flag.

When adding support for a new feature flag in TCG code, you need
to extend TCG_*_FEATURES in target/i386/cpu.c, otherwise the
feature flag will never be enabled by the CPU configuration code.


> +    } else {
> +        fpcs = 0;
> +        fpds = 0;
> +    }
> +
>      if (data32) {
>          /* 32 bit */
>          cpu_stl_data_ra(env, ptr, env->fpuc, retaddr);
>          cpu_stl_data_ra(env, ptr + 4, fpus, retaddr);
>          cpu_stl_data_ra(env, ptr + 8, fptag, retaddr);
> -        cpu_stl_data_ra(env, ptr + 12, 0, retaddr); /* fpip */
> -        cpu_stl_data_ra(env, ptr + 16, 0, retaddr); /* fpcs */
> -        cpu_stl_data_ra(env, ptr + 20, 0, retaddr); /* fpoo */
> -        cpu_stl_data_ra(env, ptr + 24, 0, retaddr); /* fpos */
> +        cpu_stl_data_ra(env, ptr + 12, env->fpip, retaddr); /* fpip */
> +        cpu_stl_data_ra(env, ptr + 16, fpcs, retaddr); /* fpcs */
> +        cpu_stl_data_ra(env, ptr + 20, env->fpdp, retaddr); /* fpdp */
> +        cpu_stl_data_ra(env, ptr + 24, fpds, retaddr); /* fpds */
>      } else {
>          /* 16 bit */
>          cpu_stw_data_ra(env, ptr, env->fpuc, retaddr);
>          cpu_stw_data_ra(env, ptr + 2, fpus, retaddr);
>          cpu_stw_data_ra(env, ptr + 4, fptag, retaddr);
> -        cpu_stw_data_ra(env, ptr + 6, 0, retaddr);
> -        cpu_stw_data_ra(env, ptr + 8, 0, retaddr);
> -        cpu_stw_data_ra(env, ptr + 10, 0, retaddr);
> -        cpu_stw_data_ra(env, ptr + 12, 0, retaddr);
> +        cpu_stw_data_ra(env, ptr + 6, env->fpip, retaddr);
> +        cpu_stw_data_ra(env, ptr + 8, fpcs, retaddr);
> +        cpu_stw_data_ra(env, ptr + 10, env->fpdp, retaddr);
> +        cpu_stw_data_ra(env, ptr + 12, fpds, retaddr);
>      }
>  }
>  
> @@ -2473,17 +2493,7 @@ void helper_fsave(CPUX86State *env, target_ulong ptr, int data32)
>      }
>  
>      /* fninit */
> -    env->fpus = 0;
> -    env->fpstt = 0;
> -    cpu_set_fpuc(env, 0x37f);
> -    env->fptags[0] = 1;
> -    env->fptags[1] = 1;
> -    env->fptags[2] = 1;
> -    env->fptags[3] = 1;
> -    env->fptags[4] = 1;
> -    env->fptags[5] = 1;
> -    env->fptags[6] = 1;
> -    env->fptags[7] = 1;
> +    helper_fninit(env);
>  }
>  
>  void helper_frstor(CPUX86State *env, target_ulong ptr, int data32)
> diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
> index 52e94fe106..59647ea5b7 100644
> --- a/target/i386/tcg/translate.c
> +++ b/target/i386/tcg/translate.c
> @@ -5839,6 +5839,11 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
>          /* floats */
>      case 0xd8 ... 0xdf:
>          {
> +            TCGv last_addr = tcg_temp_new();
> +            int last_seg;
> +            bool update_fdp = false;
> +            bool update_fip = true;
> +
>              if (s->flags & (HF_EM_MASK | HF_TS_MASK)) {
>                  /* if CR0.EM or CR0.TS are set, generate an FPU exception */
>                  /* XXX: what to do if illegal op ? */
> @@ -5851,7 +5856,14 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
>              op = ((b & 7) << 3) | ((modrm >> 3) & 7);
>              if (mod != 3) {
>                  /* memory op */
> -                gen_lea_modrm(env, s, modrm);
> +                AddressParts a = gen_lea_modrm_0(env, s, modrm);
> +                TCGv ea = gen_lea_modrm_1(s, a);
> +
> +                update_fdp = true;
> +                last_seg = a.def_seg;
> +                tcg_gen_mov_tl(last_addr, ea);
> +                gen_lea_v_seg(s, s->aflag, ea, a.def_seg, s->override);
> +
>                  switch (op) {
>                  case 0x00 ... 0x07: /* fxxxs */
>                  case 0x10 ... 0x17: /* fixxxl */
> @@ -5978,19 +5990,23 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
>                      break;
>                  case 0x0c: /* fldenv mem */
>                      gen_helper_fldenv(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> +                    update_fip = update_fdp = false;
>                      break;
>                  case 0x0d: /* fldcw mem */
>                      tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
>                                          s->mem_index, MO_LEUW);
>                      gen_helper_fldcw(cpu_env, s->tmp2_i32);
> +                    update_fip = update_fdp = false;
>                      break;
>                  case 0x0e: /* fnstenv mem */
>                      gen_helper_fstenv(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> +                    update_fip = update_fdp = false;
>                      break;
>                  case 0x0f: /* fnstcw mem */
>                      gen_helper_fnstcw(s->tmp2_i32, cpu_env);
>                      tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
>                                          s->mem_index, MO_LEUW);
> +                    update_fip = update_fdp = false;
>                      break;
>                  case 0x1d: /* fldt mem */
>                      gen_helper_fldt_ST0(cpu_env, s->A0);
> @@ -6001,14 +6017,17 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
>                      break;
>                  case 0x2c: /* frstor mem */
>                      gen_helper_frstor(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> +                    update_fip = update_fdp = false;
>                      break;
>                  case 0x2e: /* fnsave mem */
>                      gen_helper_fsave(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> +                    update_fip = update_fdp = false;
>                      break;
>                  case 0x2f: /* fnstsw mem */
>                      gen_helper_fnstsw(s->tmp2_i32, cpu_env);
>                      tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
>                                          s->mem_index, MO_LEUW);
> +                    update_fip = update_fdp = false;
>                      break;
>                  case 0x3c: /* fbld */
>                      gen_helper_fbld_ST0(cpu_env, s->A0);
> @@ -6051,6 +6070,7 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
>                      case 0: /* fnop */
>                          /* check exceptions (FreeBSD FPU probe) */
>                          gen_helper_fwait(cpu_env);
> +                        update_fip = update_fdp = false;
>                          break;
>                      default:
>                          goto unknown_op;
> @@ -6220,9 +6240,11 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
>                          break;
>                      case 2: /* fclex */
>                          gen_helper_fclex(cpu_env);
> +                        update_fip = update_fdp = false;
>                          break;
>                      case 3: /* fninit */
>                          gen_helper_fninit(cpu_env);
> +                        update_fip = update_fdp = false;
>                          break;
>                      case 4: /* fsetpm (287 only, just do nop here) */
>                          break;
> @@ -6343,6 +6365,27 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
>                      goto unknown_op;
>                  }
>              }
> +
> +            if (update_fip) {
> +                tcg_gen_ld32u_tl(s->T0, cpu_env,
> +                                offsetof(CPUX86State, segs[R_CS].selector));
> +                tcg_gen_st16_tl(s->T0, cpu_env, offsetof(CPUX86State, fpcs));
> +
> +                tcg_gen_movi_tl(s->T0, pc_start - s->cs_base);
> +                tcg_gen_st_tl(s->T0, cpu_env, offsetof(CPUX86State, fpip));
> +            }
> +
> +            if (update_fdp) {
> +                if (s->override >= 0) {
> +                    last_seg = s->override;
> +                }
> +                tcg_gen_ld32u_tl(s->T0, cpu_env,
> +                                 offsetof(CPUX86State,
> +                                 segs[last_seg].selector));
> +                tcg_gen_st16_tl(s->T0, cpu_env, offsetof(CPUX86State, fpds));
> +
> +                tcg_gen_st_tl(last_addr, cpu_env, offsetof(CPUX86State, fpdp));
> +            }
>          }
>          break;
>          /************************/
> -- 
> 2.25.1
> 

-- 
Eduardo



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 1/2] target/i386: Trivial code motion
  2021-05-17 20:16 ` [PATCH v4 1/2] target/i386: Trivial code motion Eduardo Habkost
@ 2021-05-18  2:53   ` Ziqiao Kong
  2021-05-27 21:10     ` Eduardo Habkost
  0 siblings, 1 reply; 10+ messages in thread
From: Ziqiao Kong @ 2021-05-18  2:53 UTC (permalink / raw)
  To: Eduardo Habkost; +Cc: Paolo Bonzini, Richard Henderson, QEMU Developers

On Tue, May 18, 2021 at 4:16 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> On Fri, May 07, 2021 at 04:00:56PM +0800, Ziqiao Kong wrote:
> > Move the float translation case to a new block by a new pair of braces.
>
> If you are just adding braces around the code, do you really need
> to reindent all the code?  I don't see any mention of `switch`
> statements on style.rst, but I see 235 existing cases where the
> brackets are aligned below the `c` in `case`.

The indention style is from the translate.c itself like here:

https://github.com/qemu/qemu/blob/master/target/i386/tcg/translate.c#L5634

There are also many similar cases in translate.c.

>
> In either case, I'm looking for a description of "why", not
> "what", but I couldn't find it.  Why are the braces necessary or
> useful here?

I have to declare some variables in this case scope, like last_addr, last_seg,
update_fdp and update_fip according to the previous review. Without the braces
here, they have to be declared at the beginning of the function like
the v2 patch.
Shall I state that in the commit message?

>
> >
> > Fix some coding style problem for the old code.
> >
> > Signed-off-by: Ziqiao Kong <ziqiaokong@gmail.com>
> > ---
> >  VERSION                     |   2 +-
> >  target/i386/tcg/translate.c | 953 ++++++++++++++++++------------------
> >  2 files changed, 481 insertions(+), 474 deletions(-)
> >
> > diff --git a/VERSION b/VERSION
> > index e479d55a5e..09b254e90c 100644
> > --- a/VERSION
> > +++ b/VERSION
> > @@ -1 +1 @@
> > -5.2.95
> > +6.0.0
> > diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
> > index 880bc45561..52e94fe106 100644
> > --- a/target/i386/tcg/translate.c
> > +++ b/target/i386/tcg/translate.c
> > @@ -5838,503 +5838,510 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
> >          /************************/
> >          /* floats */
> >      case 0xd8 ... 0xdf:
> > -        if (s->flags & (HF_EM_MASK | HF_TS_MASK)) {
> > -            /* if CR0.EM or CR0.TS are set, generate an FPU exception */
> > -            /* XXX: what to do if illegal op ? */
> > -            gen_exception(s, EXCP07_PREX, pc_start - s->cs_base);
> > -            break;
> > -        }
> > -        modrm = x86_ldub_code(env, s);
> > -        mod = (modrm >> 6) & 3;
> > -        rm = modrm & 7;
> > -        op = ((b & 7) << 3) | ((modrm >> 3) & 7);
> > -        if (mod != 3) {
> > -            /* memory op */
> > -            gen_lea_modrm(env, s, modrm);
> > -            switch(op) {
> > -            case 0x00 ... 0x07: /* fxxxs */
> > -            case 0x10 ... 0x17: /* fixxxl */
> > -            case 0x20 ... 0x27: /* fxxxl */
> > -            case 0x30 ... 0x37: /* fixxx */
> > -                {
> > -                    int op1;
> > -                    op1 = op & 7;
> > -
> > -                    switch(op >> 4) {
> > -                    case 0:
> > -                        tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> > -                                            s->mem_index, MO_LEUL);
> > -                        gen_helper_flds_FT0(cpu_env, s->tmp2_i32);
> > -                        break;
> > -                    case 1:
> > -                        tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> > -                                            s->mem_index, MO_LEUL);
> > -                        gen_helper_fildl_FT0(cpu_env, s->tmp2_i32);
> > -                        break;
> > -                    case 2:
> > -                        tcg_gen_qemu_ld_i64(s->tmp1_i64, s->A0,
> > -                                            s->mem_index, MO_LEQ);
> > -                        gen_helper_fldl_FT0(cpu_env, s->tmp1_i64);
> > -                        break;
> > -                    case 3:
> > -                    default:
> > -                        tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> > -                                            s->mem_index, MO_LESW);
> > -                        gen_helper_fildl_FT0(cpu_env, s->tmp2_i32);
> > -                        break;
> > -                    }
> > -
> > -                    gen_helper_fp_arith_ST0_FT0(op1);
> > -                    if (op1 == 3) {
> > -                        /* fcomp needs pop */
> > -                        gen_helper_fpop(cpu_env);
> > -                    }
> > -                }
> > +        {
> > +            if (s->flags & (HF_EM_MASK | HF_TS_MASK)) {
> > +                /* if CR0.EM or CR0.TS are set, generate an FPU exception */
> > +                /* XXX: what to do if illegal op ? */
> > +                gen_exception(s, EXCP07_PREX, pc_start - s->cs_base);
> >                  break;
> > -            case 0x08: /* flds */
> > -            case 0x0a: /* fsts */
> > -            case 0x0b: /* fstps */
> > -            case 0x18 ... 0x1b: /* fildl, fisttpl, fistl, fistpl */
> > -            case 0x28 ... 0x2b: /* fldl, fisttpll, fstl, fstpl */
> > -            case 0x38 ... 0x3b: /* filds, fisttps, fists, fistps */
> > -                switch(op & 7) {
> > -                case 0:
> > -                    switch(op >> 4) {
> > -                    case 0:
> > -                        tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> > -                                            s->mem_index, MO_LEUL);
> > -                        gen_helper_flds_ST0(cpu_env, s->tmp2_i32);
> > -                        break;
> > -                    case 1:
> > -                        tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> > -                                            s->mem_index, MO_LEUL);
> > -                        gen_helper_fildl_ST0(cpu_env, s->tmp2_i32);
> > -                        break;
> > -                    case 2:
> > -                        tcg_gen_qemu_ld_i64(s->tmp1_i64, s->A0,
> > -                                            s->mem_index, MO_LEQ);
> > -                        gen_helper_fldl_ST0(cpu_env, s->tmp1_i64);
> > -                        break;
> > -                    case 3:
> > -                    default:
> > -                        tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> > -                                            s->mem_index, MO_LESW);
> > -                        gen_helper_fildl_ST0(cpu_env, s->tmp2_i32);
> > -                        break;
> > -                    }
> > -                    break;
> > -                case 1:
> > -                    /* XXX: the corresponding CPUID bit must be tested ! */
> > -                    switch(op >> 4) {
> > -                    case 1:
> > -                        gen_helper_fisttl_ST0(s->tmp2_i32, cpu_env);
> > -                        tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> > -                                            s->mem_index, MO_LEUL);
> > -                        break;
> > -                    case 2:
> > -                        gen_helper_fisttll_ST0(s->tmp1_i64, cpu_env);
> > -                        tcg_gen_qemu_st_i64(s->tmp1_i64, s->A0,
> > -                                            s->mem_index, MO_LEQ);
> > -                        break;
> > -                    case 3:
> > -                    default:
> > -                        gen_helper_fistt_ST0(s->tmp2_i32, cpu_env);
> > -                        tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> > -                                            s->mem_index, MO_LEUW);
> > -                        break;
> > -                    }
> > -                    gen_helper_fpop(cpu_env);
> > -                    break;
> > -                default:
> > -                    switch(op >> 4) {
> > -                    case 0:
> > -                        gen_helper_fsts_ST0(s->tmp2_i32, cpu_env);
> > -                        tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> > -                                            s->mem_index, MO_LEUL);
> > -                        break;
> > -                    case 1:
> > -                        gen_helper_fistl_ST0(s->tmp2_i32, cpu_env);
> > -                        tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> > -                                            s->mem_index, MO_LEUL);
> > -                        break;
> > -                    case 2:
> > -                        gen_helper_fstl_ST0(s->tmp1_i64, cpu_env);
> > -                        tcg_gen_qemu_st_i64(s->tmp1_i64, s->A0,
> > -                                            s->mem_index, MO_LEQ);
> > -                        break;
> > -                    case 3:
> > -                    default:
> > -                        gen_helper_fist_ST0(s->tmp2_i32, cpu_env);
> > -                        tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> > -                                            s->mem_index, MO_LEUW);
> > -                        break;
> > -                    }
> > -                    if ((op & 7) == 3)
> > -                        gen_helper_fpop(cpu_env);
> > -                    break;
> > -                }
> > -                break;
> > -            case 0x0c: /* fldenv mem */
> > -                gen_helper_fldenv(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> > -                break;
> > -            case 0x0d: /* fldcw mem */
> > -                tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> > -                                    s->mem_index, MO_LEUW);
> > -                gen_helper_fldcw(cpu_env, s->tmp2_i32);
> > -                break;
> > -            case 0x0e: /* fnstenv mem */
> > -                gen_helper_fstenv(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> > -                break;
> > -            case 0x0f: /* fnstcw mem */
> > -                gen_helper_fnstcw(s->tmp2_i32, cpu_env);
> > -                tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> > -                                    s->mem_index, MO_LEUW);
> > -                break;
> > -            case 0x1d: /* fldt mem */
> > -                gen_helper_fldt_ST0(cpu_env, s->A0);
> > -                break;
> > -            case 0x1f: /* fstpt mem */
> > -                gen_helper_fstt_ST0(cpu_env, s->A0);
> > -                gen_helper_fpop(cpu_env);
> > -                break;
> > -            case 0x2c: /* frstor mem */
> > -                gen_helper_frstor(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> > -                break;
> > -            case 0x2e: /* fnsave mem */
> > -                gen_helper_fsave(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> > -                break;
> > -            case 0x2f: /* fnstsw mem */
> > -                gen_helper_fnstsw(s->tmp2_i32, cpu_env);
> > -                tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> > -                                    s->mem_index, MO_LEUW);
> > -                break;
> > -            case 0x3c: /* fbld */
> > -                gen_helper_fbld_ST0(cpu_env, s->A0);
> > -                break;
> > -            case 0x3e: /* fbstp */
> > -                gen_helper_fbst_ST0(cpu_env, s->A0);
> > -                gen_helper_fpop(cpu_env);
> > -                break;
> > -            case 0x3d: /* fildll */
> > -                tcg_gen_qemu_ld_i64(s->tmp1_i64, s->A0, s->mem_index, MO_LEQ);
> > -                gen_helper_fildll_ST0(cpu_env, s->tmp1_i64);
> > -                break;
> > -            case 0x3f: /* fistpll */
> > -                gen_helper_fistll_ST0(s->tmp1_i64, cpu_env);
> > -                tcg_gen_qemu_st_i64(s->tmp1_i64, s->A0, s->mem_index, MO_LEQ);
> > -                gen_helper_fpop(cpu_env);
> > -                break;
> > -            default:
> > -                goto unknown_op;
> >              }
> > -        } else {
> > -            /* register float ops */
> > -            opreg = rm;
> > +            modrm = x86_ldub_code(env, s);
> > +            mod = (modrm >> 6) & 3;
> > +            rm = modrm & 7;
> > +            op = ((b & 7) << 3) | ((modrm >> 3) & 7);
> > +            if (mod != 3) {
> > +                /* memory op */
> > +                gen_lea_modrm(env, s, modrm);
> > +                switch (op) {
> > +                case 0x00 ... 0x07: /* fxxxs */
> > +                case 0x10 ... 0x17: /* fixxxl */
> > +                case 0x20 ... 0x27: /* fxxxl */
> > +                case 0x30 ... 0x37: /* fixxx */
> > +                    {
> > +                        int op1;
> > +                        op1 = op & 7;
> >
> > -            switch(op) {
> > -            case 0x08: /* fld sti */
> > -                gen_helper_fpush(cpu_env);
> > -                gen_helper_fmov_ST0_STN(cpu_env,
> > -                                        tcg_const_i32((opreg + 1) & 7));
> > -                break;
> > -            case 0x09: /* fxchg sti */
> > -            case 0x29: /* fxchg4 sti, undocumented op */
> > -            case 0x39: /* fxchg7 sti, undocumented op */
> > -                gen_helper_fxchg_ST0_STN(cpu_env, tcg_const_i32(opreg));
> > -                break;
> > -            case 0x0a: /* grp d9/2 */
> > -                switch(rm) {
> > -                case 0: /* fnop */
> > -                    /* check exceptions (FreeBSD FPU probe) */
> > -                    gen_helper_fwait(cpu_env);
> > -                    break;
> > -                default:
> > -                    goto unknown_op;
> > -                }
> > -                break;
> > -            case 0x0c: /* grp d9/4 */
> > -                switch(rm) {
> > -                case 0: /* fchs */
> > -                    gen_helper_fchs_ST0(cpu_env);
> > -                    break;
> > -                case 1: /* fabs */
> > -                    gen_helper_fabs_ST0(cpu_env);
> > -                    break;
> > -                case 4: /* ftst */
> > -                    gen_helper_fldz_FT0(cpu_env);
> > -                    gen_helper_fcom_ST0_FT0(cpu_env);
> > -                    break;
> > -                case 5: /* fxam */
> > -                    gen_helper_fxam_ST0(cpu_env);
> > -                    break;
> > -                default:
> > -                    goto unknown_op;
> > -                }
> > -                break;
> > -            case 0x0d: /* grp d9/5 */
> > -                {
> > -                    switch(rm) {
> > -                    case 0:
> > -                        gen_helper_fpush(cpu_env);
> > -                        gen_helper_fld1_ST0(cpu_env);
> > -                        break;
> > -                    case 1:
> > -                        gen_helper_fpush(cpu_env);
> > -                        gen_helper_fldl2t_ST0(cpu_env);
> > -                        break;
> > -                    case 2:
> > -                        gen_helper_fpush(cpu_env);
> > -                        gen_helper_fldl2e_ST0(cpu_env);
> > -                        break;
> > -                    case 3:
> > -                        gen_helper_fpush(cpu_env);
> > -                        gen_helper_fldpi_ST0(cpu_env);
> > -                        break;
> > -                    case 4:
> > -                        gen_helper_fpush(cpu_env);
> > -                        gen_helper_fldlg2_ST0(cpu_env);
> > -                        break;
> > -                    case 5:
> > -                        gen_helper_fpush(cpu_env);
> > -                        gen_helper_fldln2_ST0(cpu_env);
> > -                        break;
> > -                    case 6:
> > -                        gen_helper_fpush(cpu_env);
> > -                        gen_helper_fldz_ST0(cpu_env);
> > -                        break;
> > -                    default:
> > -                        goto unknown_op;
> > -                    }
> > -                }
> > -                break;
> > -            case 0x0e: /* grp d9/6 */
> > -                switch(rm) {
> > -                case 0: /* f2xm1 */
> > -                    gen_helper_f2xm1(cpu_env);
> > -                    break;
> > -                case 1: /* fyl2x */
> > -                    gen_helper_fyl2x(cpu_env);
> > -                    break;
> > -                case 2: /* fptan */
> > -                    gen_helper_fptan(cpu_env);
> > -                    break;
> > -                case 3: /* fpatan */
> > -                    gen_helper_fpatan(cpu_env);
> > -                    break;
> > -                case 4: /* fxtract */
> > -                    gen_helper_fxtract(cpu_env);
> > -                    break;
> > -                case 5: /* fprem1 */
> > -                    gen_helper_fprem1(cpu_env);
> > -                    break;
> > -                case 6: /* fdecstp */
> > -                    gen_helper_fdecstp(cpu_env);
> > -                    break;
> > -                default:
> > -                case 7: /* fincstp */
> > -                    gen_helper_fincstp(cpu_env);
> > -                    break;
> > -                }
> > -                break;
> > -            case 0x0f: /* grp d9/7 */
> > -                switch(rm) {
> > -                case 0: /* fprem */
> > -                    gen_helper_fprem(cpu_env);
> > -                    break;
> > -                case 1: /* fyl2xp1 */
> > -                    gen_helper_fyl2xp1(cpu_env);
> > -                    break;
> > -                case 2: /* fsqrt */
> > -                    gen_helper_fsqrt(cpu_env);
> > -                    break;
> > -                case 3: /* fsincos */
> > -                    gen_helper_fsincos(cpu_env);
> > -                    break;
> > -                case 5: /* fscale */
> > -                    gen_helper_fscale(cpu_env);
> > -                    break;
> > -                case 4: /* frndint */
> > -                    gen_helper_frndint(cpu_env);
> > -                    break;
> > -                case 6: /* fsin */
> > -                    gen_helper_fsin(cpu_env);
> > -                    break;
> > -                default:
> > -                case 7: /* fcos */
> > -                    gen_helper_fcos(cpu_env);
> > -                    break;
> > -                }
> > -                break;
> > -            case 0x00: case 0x01: case 0x04 ... 0x07: /* fxxx st, sti */
> > -            case 0x20: case 0x21: case 0x24 ... 0x27: /* fxxx sti, st */
> > -            case 0x30: case 0x31: case 0x34 ... 0x37: /* fxxxp sti, st */
> > -                {
> > -                    int op1;
> > +                        switch (op >> 4) {
> > +                        case 0:
> > +                            tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> > +                                                s->mem_index, MO_LEUL);
> > +                            gen_helper_flds_FT0(cpu_env, s->tmp2_i32);
> > +                            break;
> > +                        case 1:
> > +                            tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> > +                                                s->mem_index, MO_LEUL);
> > +                            gen_helper_fildl_FT0(cpu_env, s->tmp2_i32);
> > +                            break;
> > +                        case 2:
> > +                            tcg_gen_qemu_ld_i64(s->tmp1_i64, s->A0,
> > +                                                s->mem_index, MO_LEQ);
> > +                            gen_helper_fldl_FT0(cpu_env, s->tmp1_i64);
> > +                            break;
> > +                        case 3:
> > +                        default:
> > +                            tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> > +                                                s->mem_index, MO_LESW);
> > +                            gen_helper_fildl_FT0(cpu_env, s->tmp2_i32);
> > +                            break;
> > +                        }
> >
> > -                    op1 = op & 7;
> > -                    if (op >= 0x20) {
> > -                        gen_helper_fp_arith_STN_ST0(op1, opreg);
> > -                        if (op >= 0x30)
> > -                            gen_helper_fpop(cpu_env);
> > -                    } else {
> > -                        gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> >                          gen_helper_fp_arith_ST0_FT0(op1);
> > +                        if (op1 == 3) {
> > +                            /* fcomp needs pop */
> > +                            gen_helper_fpop(cpu_env);
> > +                        }
> >                      }
> > -                }
> > -                break;
> > -            case 0x02: /* fcom */
> > -            case 0x22: /* fcom2, undocumented op */
> > -                gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> > -                gen_helper_fcom_ST0_FT0(cpu_env);
> > -                break;
> > -            case 0x03: /* fcomp */
> > -            case 0x23: /* fcomp3, undocumented op */
> > -            case 0x32: /* fcomp5, undocumented op */
> > -                gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> > -                gen_helper_fcom_ST0_FT0(cpu_env);
> > -                gen_helper_fpop(cpu_env);
> > -                break;
> > -            case 0x15: /* da/5 */
> > -                switch(rm) {
> > -                case 1: /* fucompp */
> > -                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(1));
> > -                    gen_helper_fucom_ST0_FT0(cpu_env);
> > -                    gen_helper_fpop(cpu_env);
> > -                    gen_helper_fpop(cpu_env);
> >                      break;
> > -                default:
> > -                    goto unknown_op;
> > -                }
> > -                break;
> > -            case 0x1c:
> > -                switch(rm) {
> > -                case 0: /* feni (287 only, just do nop here) */
> > +                case 0x08: /* flds */
> > +                case 0x0a: /* fsts */
> > +                case 0x0b: /* fstps */
> > +                case 0x18 ... 0x1b: /* fildl, fisttpl, fistl, fistpl */
> > +                case 0x28 ... 0x2b: /* fldl, fisttpll, fstl, fstpl */
> > +                case 0x38 ... 0x3b: /* filds, fisttps, fists, fistps */
> > +                    switch (op & 7) {
> > +                    case 0:
> > +                        switch (op >> 4) {
> > +                        case 0:
> > +                            tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> > +                                                s->mem_index, MO_LEUL);
> > +                            gen_helper_flds_ST0(cpu_env, s->tmp2_i32);
> > +                            break;
> > +                        case 1:
> > +                            tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> > +                                                s->mem_index, MO_LEUL);
> > +                            gen_helper_fildl_ST0(cpu_env, s->tmp2_i32);
> > +                            break;
> > +                        case 2:
> > +                            tcg_gen_qemu_ld_i64(s->tmp1_i64, s->A0,
> > +                                                s->mem_index, MO_LEQ);
> > +                            gen_helper_fldl_ST0(cpu_env, s->tmp1_i64);
> > +                            break;
> > +                        case 3:
> > +                        default:
> > +                            tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> > +                                                s->mem_index, MO_LESW);
> > +                            gen_helper_fildl_ST0(cpu_env, s->tmp2_i32);
> > +                            break;
> > +                        }
> > +                        break;
> > +                    case 1:
> > +                        /* XXX: the corresponding CPUID bit must be tested ! */
> > +                        switch (op >> 4) {
> > +                        case 1:
> > +                            gen_helper_fisttl_ST0(s->tmp2_i32, cpu_env);
> > +                            tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> > +                                                s->mem_index, MO_LEUL);
> > +                            break;
> > +                        case 2:
> > +                            gen_helper_fisttll_ST0(s->tmp1_i64, cpu_env);
> > +                            tcg_gen_qemu_st_i64(s->tmp1_i64, s->A0,
> > +                                                s->mem_index, MO_LEQ);
> > +                            break;
> > +                        case 3:
> > +                        default:
> > +                            gen_helper_fistt_ST0(s->tmp2_i32, cpu_env);
> > +                            tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> > +                                                s->mem_index, MO_LEUW);
> > +                            break;
> > +                        }
> > +                        gen_helper_fpop(cpu_env);
> > +                        break;
> > +                    default:
> > +                        switch (op >> 4) {
> > +                        case 0:
> > +                            gen_helper_fsts_ST0(s->tmp2_i32, cpu_env);
> > +                            tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> > +                                                s->mem_index, MO_LEUL);
> > +                            break;
> > +                        case 1:
> > +                            gen_helper_fistl_ST0(s->tmp2_i32, cpu_env);
> > +                            tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> > +                                                s->mem_index, MO_LEUL);
> > +                            break;
> > +                        case 2:
> > +                            gen_helper_fstl_ST0(s->tmp1_i64, cpu_env);
> > +                            tcg_gen_qemu_st_i64(s->tmp1_i64, s->A0,
> > +                                                s->mem_index, MO_LEQ);
> > +                            break;
> > +                        case 3:
> > +                        default:
> > +                            gen_helper_fist_ST0(s->tmp2_i32, cpu_env);
> > +                            tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> > +                                                s->mem_index, MO_LEUW);
> > +                            break;
> > +                        }
> > +                        if ((op & 7) == 3) {
> > +                            gen_helper_fpop(cpu_env);
> > +                        }
> > +                        break;
> > +                    }
> >                      break;
> > -                case 1: /* fdisi (287 only, just do nop here) */
> > +                case 0x0c: /* fldenv mem */
> > +                    gen_helper_fldenv(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> >                      break;
> > -                case 2: /* fclex */
> > -                    gen_helper_fclex(cpu_env);
> > +                case 0x0d: /* fldcw mem */
> > +                    tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> > +                                        s->mem_index, MO_LEUW);
> > +                    gen_helper_fldcw(cpu_env, s->tmp2_i32);
> >                      break;
> > -                case 3: /* fninit */
> > -                    gen_helper_fninit(cpu_env);
> > +                case 0x0e: /* fnstenv mem */
> > +                    gen_helper_fstenv(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> >                      break;
> > -                case 4: /* fsetpm (287 only, just do nop here) */
> > +                case 0x0f: /* fnstcw mem */
> > +                    gen_helper_fnstcw(s->tmp2_i32, cpu_env);
> > +                    tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> > +                                        s->mem_index, MO_LEUW);
> >                      break;
> > -                default:
> > -                    goto unknown_op;
> > -                }
> > -                break;
> > -            case 0x1d: /* fucomi */
> > -                if (!(s->cpuid_features & CPUID_CMOV)) {
> > -                    goto illegal_op;
> > -                }
> > -                gen_update_cc_op(s);
> > -                gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> > -                gen_helper_fucomi_ST0_FT0(cpu_env);
> > -                set_cc_op(s, CC_OP_EFLAGS);
> > -                break;
> > -            case 0x1e: /* fcomi */
> > -                if (!(s->cpuid_features & CPUID_CMOV)) {
> > -                    goto illegal_op;
> > -                }
> > -                gen_update_cc_op(s);
> > -                gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> > -                gen_helper_fcomi_ST0_FT0(cpu_env);
> > -                set_cc_op(s, CC_OP_EFLAGS);
> > -                break;
> > -            case 0x28: /* ffree sti */
> > -                gen_helper_ffree_STN(cpu_env, tcg_const_i32(opreg));
> > -                break;
> > -            case 0x2a: /* fst sti */
> > -                gen_helper_fmov_STN_ST0(cpu_env, tcg_const_i32(opreg));
> > -                break;
> > -            case 0x2b: /* fstp sti */
> > -            case 0x0b: /* fstp1 sti, undocumented op */
> > -            case 0x3a: /* fstp8 sti, undocumented op */
> > -            case 0x3b: /* fstp9 sti, undocumented op */
> > -                gen_helper_fmov_STN_ST0(cpu_env, tcg_const_i32(opreg));
> > -                gen_helper_fpop(cpu_env);
> > -                break;
> > -            case 0x2c: /* fucom st(i) */
> > -                gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> > -                gen_helper_fucom_ST0_FT0(cpu_env);
> > -                break;
> > -            case 0x2d: /* fucomp st(i) */
> > -                gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> > -                gen_helper_fucom_ST0_FT0(cpu_env);
> > -                gen_helper_fpop(cpu_env);
> > -                break;
> > -            case 0x33: /* de/3 */
> > -                switch(rm) {
> > -                case 1: /* fcompp */
> > -                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(1));
> > -                    gen_helper_fcom_ST0_FT0(cpu_env);
> > -                    gen_helper_fpop(cpu_env);
> > +                case 0x1d: /* fldt mem */
> > +                    gen_helper_fldt_ST0(cpu_env, s->A0);
> > +                    break;
> > +                case 0x1f: /* fstpt mem */
> > +                    gen_helper_fstt_ST0(cpu_env, s->A0);
> >                      gen_helper_fpop(cpu_env);
> >                      break;
> > -                default:
> > -                    goto unknown_op;
> > -                }
> > -                break;
> > -            case 0x38: /* ffreep sti, undocumented op */
> > -                gen_helper_ffree_STN(cpu_env, tcg_const_i32(opreg));
> > -                gen_helper_fpop(cpu_env);
> > -                break;
> > -            case 0x3c: /* df/4 */
> > -                switch(rm) {
> > -                case 0:
> > +                case 0x2c: /* frstor mem */
> > +                    gen_helper_frstor(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> > +                    break;
> > +                case 0x2e: /* fnsave mem */
> > +                    gen_helper_fsave(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> > +                    break;
> > +                case 0x2f: /* fnstsw mem */
> >                      gen_helper_fnstsw(s->tmp2_i32, cpu_env);
> > -                    tcg_gen_extu_i32_tl(s->T0, s->tmp2_i32);
> > -                    gen_op_mov_reg_v(s, MO_16, R_EAX, s->T0);
> > +                    tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> > +                                        s->mem_index, MO_LEUW);
> > +                    break;
> > +                case 0x3c: /* fbld */
> > +                    gen_helper_fbld_ST0(cpu_env, s->A0);
> > +                    break;
> > +                case 0x3e: /* fbstp */
> > +                    gen_helper_fbst_ST0(cpu_env, s->A0);
> > +                    gen_helper_fpop(cpu_env);
> > +                    break;
> > +                case 0x3d: /* fildll */
> > +                    tcg_gen_qemu_ld_i64(s->tmp1_i64, s->A0,
> > +                                        s->mem_index, MO_LEQ);
> > +                    gen_helper_fildll_ST0(cpu_env, s->tmp1_i64);
> > +                    break;
> > +                case 0x3f: /* fistpll */
> > +                    gen_helper_fistll_ST0(s->tmp1_i64, cpu_env);
> > +                    tcg_gen_qemu_st_i64(s->tmp1_i64, s->A0,
> > +                                        s->mem_index, MO_LEQ);
> > +                    gen_helper_fpop(cpu_env);
> >                      break;
> >                  default:
> >                      goto unknown_op;
> >                  }
> > -                break;
> > -            case 0x3d: /* fucomip */
> > -                if (!(s->cpuid_features & CPUID_CMOV)) {
> > -                    goto illegal_op;
> > -                }
> > -                gen_update_cc_op(s);
> > -                gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> > -                gen_helper_fucomi_ST0_FT0(cpu_env);
> > -                gen_helper_fpop(cpu_env);
> > -                set_cc_op(s, CC_OP_EFLAGS);
> > -                break;
> > -            case 0x3e: /* fcomip */
> > -                if (!(s->cpuid_features & CPUID_CMOV)) {
> > -                    goto illegal_op;
> > -                }
> > -                gen_update_cc_op(s);
> > -                gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> > -                gen_helper_fcomi_ST0_FT0(cpu_env);
> > -                gen_helper_fpop(cpu_env);
> > -                set_cc_op(s, CC_OP_EFLAGS);
> > -                break;
> > -            case 0x10 ... 0x13: /* fcmovxx */
> > -            case 0x18 ... 0x1b:
> > -                {
> > -                    int op1;
> > -                    TCGLabel *l1;
> > -                    static const uint8_t fcmov_cc[8] = {
> > -                        (JCC_B << 1),
> > -                        (JCC_Z << 1),
> > -                        (JCC_BE << 1),
> > -                        (JCC_P << 1),
> > -                    };
> > +            } else {
> > +                /* register float ops */
> > +                opreg = rm;
> > +
> > +                switch (op) {
> > +                case 0x08: /* fld sti */
> > +                    gen_helper_fpush(cpu_env);
> > +                    gen_helper_fmov_ST0_STN(cpu_env,
> > +                                            tcg_const_i32((opreg + 1) & 7));
> > +                    break;
> > +                case 0x09: /* fxchg sti */
> > +                case 0x29: /* fxchg4 sti, undocumented op */
> > +                case 0x39: /* fxchg7 sti, undocumented op */
> > +                    gen_helper_fxchg_ST0_STN(cpu_env, tcg_const_i32(opreg));
> > +                    break;
> > +                case 0x0a: /* grp d9/2 */
> > +                    switch (rm) {
> > +                    case 0: /* fnop */
> > +                        /* check exceptions (FreeBSD FPU probe) */
> > +                        gen_helper_fwait(cpu_env);
> > +                        break;
> > +                    default:
> > +                        goto unknown_op;
> > +                    }
> > +                    break;
> > +                case 0x0c: /* grp d9/4 */
> > +                    switch (rm) {
> > +                    case 0: /* fchs */
> > +                        gen_helper_fchs_ST0(cpu_env);
> > +                        break;
> > +                    case 1: /* fabs */
> > +                        gen_helper_fabs_ST0(cpu_env);
> > +                        break;
> > +                    case 4: /* ftst */
> > +                        gen_helper_fldz_FT0(cpu_env);
> > +                        gen_helper_fcom_ST0_FT0(cpu_env);
> > +                        break;
> > +                    case 5: /* fxam */
> > +                        gen_helper_fxam_ST0(cpu_env);
> > +                        break;
> > +                    default:
> > +                        goto unknown_op;
> > +                    }
> > +                    break;
> > +                case 0x0d: /* grp d9/5 */
> > +                    {
> > +                        switch (rm) {
> > +                        case 0:
> > +                            gen_helper_fpush(cpu_env);
> > +                            gen_helper_fld1_ST0(cpu_env);
> > +                            break;
> > +                        case 1:
> > +                            gen_helper_fpush(cpu_env);
> > +                            gen_helper_fldl2t_ST0(cpu_env);
> > +                            break;
> > +                        case 2:
> > +                            gen_helper_fpush(cpu_env);
> > +                            gen_helper_fldl2e_ST0(cpu_env);
> > +                            break;
> > +                        case 3:
> > +                            gen_helper_fpush(cpu_env);
> > +                            gen_helper_fldpi_ST0(cpu_env);
> > +                            break;
> > +                        case 4:
> > +                            gen_helper_fpush(cpu_env);
> > +                            gen_helper_fldlg2_ST0(cpu_env);
> > +                            break;
> > +                        case 5:
> > +                            gen_helper_fpush(cpu_env);
> > +                            gen_helper_fldln2_ST0(cpu_env);
> > +                            break;
> > +                        case 6:
> > +                            gen_helper_fpush(cpu_env);
> > +                            gen_helper_fldz_ST0(cpu_env);
> > +                            break;
> > +                        default:
> > +                            goto unknown_op;
> > +                        }
> > +                    }
> > +                    break;
> > +                case 0x0e: /* grp d9/6 */
> > +                    switch (rm) {
> > +                    case 0: /* f2xm1 */
> > +                        gen_helper_f2xm1(cpu_env);
> > +                        break;
> > +                    case 1: /* fyl2x */
> > +                        gen_helper_fyl2x(cpu_env);
> > +                        break;
> > +                    case 2: /* fptan */
> > +                        gen_helper_fptan(cpu_env);
> > +                        break;
> > +                    case 3: /* fpatan */
> > +                        gen_helper_fpatan(cpu_env);
> > +                        break;
> > +                    case 4: /* fxtract */
> > +                        gen_helper_fxtract(cpu_env);
> > +                        break;
> > +                    case 5: /* fprem1 */
> > +                        gen_helper_fprem1(cpu_env);
> > +                        break;
> > +                    case 6: /* fdecstp */
> > +                        gen_helper_fdecstp(cpu_env);
> > +                        break;
> > +                    default:
> > +                    case 7: /* fincstp */
> > +                        gen_helper_fincstp(cpu_env);
> > +                        break;
> > +                    }
> > +                    break;
> > +                case 0x0f: /* grp d9/7 */
> > +                    switch (rm) {
> > +                    case 0: /* fprem */
> > +                        gen_helper_fprem(cpu_env);
> > +                        break;
> > +                    case 1: /* fyl2xp1 */
> > +                        gen_helper_fyl2xp1(cpu_env);
> > +                        break;
> > +                    case 2: /* fsqrt */
> > +                        gen_helper_fsqrt(cpu_env);
> > +                        break;
> > +                    case 3: /* fsincos */
> > +                        gen_helper_fsincos(cpu_env);
> > +                        break;
> > +                    case 5: /* fscale */
> > +                        gen_helper_fscale(cpu_env);
> > +                        break;
> > +                    case 4: /* frndint */
> > +                        gen_helper_frndint(cpu_env);
> > +                        break;
> > +                    case 6: /* fsin */
> > +                        gen_helper_fsin(cpu_env);
> > +                        break;
> > +                    default:
> > +                    case 7: /* fcos */
> > +                        gen_helper_fcos(cpu_env);
> > +                        break;
> > +                    }
> > +                    break;
> > +                case 0x00: case 0x01: case 0x04 ... 0x07: /* fxxx st, sti */
> > +                case 0x20: case 0x21: case 0x24 ... 0x27: /* fxxx sti, st */
> > +                case 0x30: case 0x31: case 0x34 ... 0x37: /* fxxxp sti, st */
> > +                    {
> > +                        int op1;
> >
> > +                        op1 = op & 7;
> > +                        if (op >= 0x20) {
> > +                            gen_helper_fp_arith_STN_ST0(op1, opreg);
> > +                            if (op >= 0x30) {
> > +                                gen_helper_fpop(cpu_env);
> > +                            }
> > +                        } else {
> > +                            gen_helper_fmov_FT0_STN(cpu_env,
> > +                                                    tcg_const_i32(opreg));
> > +                            gen_helper_fp_arith_ST0_FT0(op1);
> > +                        }
> > +                    }
> > +                    break;
> > +                case 0x02: /* fcom */
> > +                case 0x22: /* fcom2, undocumented op */
> > +                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> > +                    gen_helper_fcom_ST0_FT0(cpu_env);
> > +                    break;
> > +                case 0x03: /* fcomp */
> > +                case 0x23: /* fcomp3, undocumented op */
> > +                case 0x32: /* fcomp5, undocumented op */
> > +                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> > +                    gen_helper_fcom_ST0_FT0(cpu_env);
> > +                    gen_helper_fpop(cpu_env);
> > +                    break;
> > +                case 0x15: /* da/5 */
> > +                    switch (rm) {
> > +                    case 1: /* fucompp */
> > +                        gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(1));
> > +                        gen_helper_fucom_ST0_FT0(cpu_env);
> > +                        gen_helper_fpop(cpu_env);
> > +                        gen_helper_fpop(cpu_env);
> > +                        break;
> > +                    default:
> > +                        goto unknown_op;
> > +                    }
> > +                    break;
> > +                case 0x1c:
> > +                    switch (rm) {
> > +                    case 0: /* feni (287 only, just do nop here) */
> > +                        break;
> > +                    case 1: /* fdisi (287 only, just do nop here) */
> > +                        break;
> > +                    case 2: /* fclex */
> > +                        gen_helper_fclex(cpu_env);
> > +                        break;
> > +                    case 3: /* fninit */
> > +                        gen_helper_fninit(cpu_env);
> > +                        break;
> > +                    case 4: /* fsetpm (287 only, just do nop here) */
> > +                        break;
> > +                    default:
> > +                        goto unknown_op;
> > +                    }
> > +                    break;
> > +                case 0x1d: /* fucomi */
> >                      if (!(s->cpuid_features & CPUID_CMOV)) {
> >                          goto illegal_op;
> >                      }
> > -                    op1 = fcmov_cc[op & 3] | (((op >> 3) & 1) ^ 1);
> > -                    l1 = gen_new_label();
> > -                    gen_jcc1_noeob(s, op1, l1);
> > -                    gen_helper_fmov_ST0_STN(cpu_env, tcg_const_i32(opreg));
> > -                    gen_set_label(l1);
> > +                    gen_update_cc_op(s);
> > +                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> > +                    gen_helper_fucomi_ST0_FT0(cpu_env);
> > +                    set_cc_op(s, CC_OP_EFLAGS);
> > +                    break;
> > +                case 0x1e: /* fcomi */
> > +                    if (!(s->cpuid_features & CPUID_CMOV)) {
> > +                        goto illegal_op;
> > +                    }
> > +                    gen_update_cc_op(s);
> > +                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> > +                    gen_helper_fcomi_ST0_FT0(cpu_env);
> > +                    set_cc_op(s, CC_OP_EFLAGS);
> > +                    break;
> > +                case 0x28: /* ffree sti */
> > +                    gen_helper_ffree_STN(cpu_env, tcg_const_i32(opreg));
> > +                    break;
> > +                case 0x2a: /* fst sti */
> > +                    gen_helper_fmov_STN_ST0(cpu_env, tcg_const_i32(opreg));
> > +                    break;
> > +                case 0x2b: /* fstp sti */
> > +                case 0x0b: /* fstp1 sti, undocumented op */
> > +                case 0x3a: /* fstp8 sti, undocumented op */
> > +                case 0x3b: /* fstp9 sti, undocumented op */
> > +                    gen_helper_fmov_STN_ST0(cpu_env, tcg_const_i32(opreg));
> > +                    gen_helper_fpop(cpu_env);
> > +                    break;
> > +                case 0x2c: /* fucom st(i) */
> > +                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> > +                    gen_helper_fucom_ST0_FT0(cpu_env);
> > +                    break;
> > +                case 0x2d: /* fucomp st(i) */
> > +                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> > +                    gen_helper_fucom_ST0_FT0(cpu_env);
> > +                    gen_helper_fpop(cpu_env);
> > +                    break;
> > +                case 0x33: /* de/3 */
> > +                    switch (rm) {
> > +                    case 1: /* fcompp */
> > +                        gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(1));
> > +                        gen_helper_fcom_ST0_FT0(cpu_env);
> > +                        gen_helper_fpop(cpu_env);
> > +                        gen_helper_fpop(cpu_env);
> > +                        break;
> > +                    default:
> > +                        goto unknown_op;
> > +                    }
> > +                    break;
> > +                case 0x38: /* ffreep sti, undocumented op */
> > +                    gen_helper_ffree_STN(cpu_env, tcg_const_i32(opreg));
> > +                    gen_helper_fpop(cpu_env);
> > +                    break;
> > +                case 0x3c: /* df/4 */
> > +                    switch (rm) {
> > +                    case 0:
> > +                        gen_helper_fnstsw(s->tmp2_i32, cpu_env);
> > +                        tcg_gen_extu_i32_tl(s->T0, s->tmp2_i32);
> > +                        gen_op_mov_reg_v(s, MO_16, R_EAX, s->T0);
> > +                        break;
> > +                    default:
> > +                        goto unknown_op;
> > +                    }
> > +                    break;
> > +                case 0x3d: /* fucomip */
> > +                    if (!(s->cpuid_features & CPUID_CMOV)) {
> > +                        goto illegal_op;
> > +                    }
> > +                    gen_update_cc_op(s);
> > +                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> > +                    gen_helper_fucomi_ST0_FT0(cpu_env);
> > +                    gen_helper_fpop(cpu_env);
> > +                    set_cc_op(s, CC_OP_EFLAGS);
> > +                    break;
> > +                case 0x3e: /* fcomip */
> > +                    if (!(s->cpuid_features & CPUID_CMOV)) {
> > +                        goto illegal_op;
> > +                    }
> > +                    gen_update_cc_op(s);
> > +                    gen_helper_fmov_FT0_STN(cpu_env, tcg_const_i32(opreg));
> > +                    gen_helper_fcomi_ST0_FT0(cpu_env);
> > +                    gen_helper_fpop(cpu_env);
> > +                    set_cc_op(s, CC_OP_EFLAGS);
> > +                    break;
> > +                case 0x10 ... 0x13: /* fcmovxx */
> > +                case 0x18 ... 0x1b:
> > +                    {
> > +                        int op1;
> > +                        TCGLabel *l1;
> > +                        static const uint8_t fcmov_cc[8] = {
> > +                            (JCC_B << 1),
> > +                            (JCC_Z << 1),
> > +                            (JCC_BE << 1),
> > +                            (JCC_P << 1),
> > +                        };
> > +
> > +                        if (!(s->cpuid_features & CPUID_CMOV)) {
> > +                            goto illegal_op;
> > +                        }
> > +                        op1 = fcmov_cc[op & 3] | (((op >> 3) & 1) ^ 1);
> > +                        l1 = gen_new_label();
> > +                        gen_jcc1_noeob(s, op1, l1);
> > +                        gen_helper_fmov_ST0_STN(cpu_env, tcg_const_i32(opreg));
> > +                        gen_set_label(l1);
> > +                    }
> > +                    break;
> > +                default:
> > +                    goto unknown_op;
> >                  }
> > -                break;
> > -            default:
> > -                goto unknown_op;
> >              }
> >          }
> >          break;
> > --
> > 2.25.1
> >
>
> --
> Eduardo
>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 2/2] target/i386: Correct implementation for FCS, FIP,  FDS and FDP
  2021-05-17 20:29   ` Eduardo Habkost
@ 2021-05-18  3:06     ` Ziqiao Kong
  2021-05-24  7:41       ` Ziqiao Kong
  2021-05-27 21:08       ` Eduardo Habkost
  0 siblings, 2 replies; 10+ messages in thread
From: Ziqiao Kong @ 2021-05-18  3:06 UTC (permalink / raw)
  To: Eduardo Habkost; +Cc: Paolo Bonzini, Richard Henderson, QEMU Developers

On Tue, May 18, 2021 at 4:29 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> Hi,
>
> Thanks for the patch, and apologies for not reviewing earlier
> versions.
>

Nevermind, the earlier version is also hard to review without a proper split.

> On Fri, May 07, 2021 at 04:00:58PM +0800, Ziqiao Kong wrote:
> > Changes since v3:
> >  - Split the long patches to series to make review easier.
> >  - Fix the coding style problems in v3.
> >
> > Changes since v2:
> >  - Change the sequence of fpcs, fpds, fpip and fpdp in CPUX86State.
> >  - Use stl instead of stw in do_fstenv.
> >  - Move variables to floats instruction case block.
> >  - Move last accessed memory operand to a temp variable to avoid another load.
> >  - Move segment selectors instead of segment base to fpcs and fpds.
> >  - Fix some code stype problems for the original code in floats case block.
>
> On the next versions, please include the changelog after a "---"
> line, so it won't be included in the final commit.
>
> In addition to the changelog, the actual commit message (the text
> above "---") needs to include an explanation for the change.  If
> you are fixing a bug, please explain what's the bug you are
> fixing.

The bug is tracked in this thread
https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg00307.html.

>
>
> >
> > Signed-off-by: Ziqiao Kong <ziqiaokong@gmail.com>
> > ---
> >  target/i386/cpu.h            |  4 +++
> >  target/i386/tcg/fpu_helper.c | 48 ++++++++++++++++++++++--------------
> >  target/i386/tcg/translate.c  | 45 ++++++++++++++++++++++++++++++++-
> >  3 files changed, 77 insertions(+), 20 deletions(-)
> >
> > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > index 570f916878..241945320b 100644
> > --- a/target/i386/cpu.h
> > +++ b/target/i386/cpu.h
> > @@ -705,6 +705,8 @@ typedef uint64_t FeatureWordArray[FEATURE_WORDS];
> >  #define CPUID_7_0_EBX_INVPCID           (1U << 10)
> >  /* Restricted Transactional Memory */
> >  #define CPUID_7_0_EBX_RTM               (1U << 11)
> > +/* Deprecates FPU CS and FPU DS values */
> > +#define CPUID_7_0_EBX_FCS_FDS           (1U << 13)
> >  /* Memory Protection Extension */
> >  #define CPUID_7_0_EBX_MPX               (1U << 14)
> >  /* AVX-512 Foundation */
> > @@ -1440,6 +1442,8 @@ typedef struct CPUX86State {
> >      FPReg fpregs[8];
> >      /* KVM-only so far */
> >      uint16_t fpop;
> > +    uint16_t fpcs;
> > +    uint16_t fpds;
> >      uint64_t fpip;
> >      uint64_t fpdp;
> >
> > diff --git a/target/i386/tcg/fpu_helper.c b/target/i386/tcg/fpu_helper.c
> > index 60ed93520a..f1a8717ed8 100644
> > --- a/target/i386/tcg/fpu_helper.c
> > +++ b/target/i386/tcg/fpu_helper.c
> > @@ -766,6 +766,10 @@ void helper_fninit(CPUX86State *env)
> >  {
> >      env->fpus = 0;
> >      env->fpstt = 0;
> > +    env->fpcs = 0;
> > +    env->fpip = 0;
> > +    env->fpds = 0;
> > +    env->fpdp = 0;
> >      cpu_set_fpuc(env, 0x37f);
> >      env->fptags[0] = 1;
> >      env->fptags[1] = 1;
> > @@ -2368,6 +2372,7 @@ static void do_fstenv(CPUX86State *env, target_ulong ptr, int data32,
> >  {
> >      int fpus, fptag, exp, i;
> >      uint64_t mant;
> > +    uint16_t fpcs, fpds;
> >      CPU_LDoubleU tmp;
> >
> >      fpus = (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11;
> > @@ -2390,24 +2395,39 @@ static void do_fstenv(CPUX86State *env, target_ulong ptr, int data32,
> >              }
> >          }
> >      }
> > +
> > +    /*
> > +     * If CR0.PE = 1, each instruction saves FCS and FDS into memory. If
> > +     * CPUID.(EAX=07H,ECX=0H):EBX[bit 13] = 1, the processor deprecates
> > +     * FCS and FDS; it saves each as 0000H.
> > +     */
> > +    if (!(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_FCS_FDS)
> > +        && (env->cr[0] & CR0_PE_MASK)) {
> > +        fpcs = env->fpcs;
> > +        fpds = env->fpds;
>
>
> If you want to start supporting this feature flag, I suggest
> moving this to a separate patch.  The description of this patch
> seems to imply it is just a bug fix, not the implementation of a
> new feature flag.
>
> When adding support for a new feature flag in TCG code, you need
> to extend TCG_*_FEATURES in target/i386/cpu.c, otherwise the
> feature flag will never be enabled by the CPU configuration code.

Yes, currently this feature flag is never enabled for all CPU types.
How about removing this
feature flag in this patch and leaving a TODO in the comment? Thus I
can issue another patch
to complete the feature later.

>
>
> > +    } else {
> > +        fpcs = 0;
> > +        fpds = 0;
> > +    }
> > +
> >      if (data32) {
> >          /* 32 bit */
> >          cpu_stl_data_ra(env, ptr, env->fpuc, retaddr);
> >          cpu_stl_data_ra(env, ptr + 4, fpus, retaddr);
> >          cpu_stl_data_ra(env, ptr + 8, fptag, retaddr);
> > -        cpu_stl_data_ra(env, ptr + 12, 0, retaddr); /* fpip */
> > -        cpu_stl_data_ra(env, ptr + 16, 0, retaddr); /* fpcs */
> > -        cpu_stl_data_ra(env, ptr + 20, 0, retaddr); /* fpoo */
> > -        cpu_stl_data_ra(env, ptr + 24, 0, retaddr); /* fpos */
> > +        cpu_stl_data_ra(env, ptr + 12, env->fpip, retaddr); /* fpip */
> > +        cpu_stl_data_ra(env, ptr + 16, fpcs, retaddr); /* fpcs */
> > +        cpu_stl_data_ra(env, ptr + 20, env->fpdp, retaddr); /* fpdp */
> > +        cpu_stl_data_ra(env, ptr + 24, fpds, retaddr); /* fpds */
> >      } else {
> >          /* 16 bit */
> >          cpu_stw_data_ra(env, ptr, env->fpuc, retaddr);
> >          cpu_stw_data_ra(env, ptr + 2, fpus, retaddr);
> >          cpu_stw_data_ra(env, ptr + 4, fptag, retaddr);
> > -        cpu_stw_data_ra(env, ptr + 6, 0, retaddr);
> > -        cpu_stw_data_ra(env, ptr + 8, 0, retaddr);
> > -        cpu_stw_data_ra(env, ptr + 10, 0, retaddr);
> > -        cpu_stw_data_ra(env, ptr + 12, 0, retaddr);
> > +        cpu_stw_data_ra(env, ptr + 6, env->fpip, retaddr);
> > +        cpu_stw_data_ra(env, ptr + 8, fpcs, retaddr);
> > +        cpu_stw_data_ra(env, ptr + 10, env->fpdp, retaddr);
> > +        cpu_stw_data_ra(env, ptr + 12, fpds, retaddr);
> >      }
> >  }
> >
> > @@ -2473,17 +2493,7 @@ void helper_fsave(CPUX86State *env, target_ulong ptr, int data32)
> >      }
> >
> >      /* fninit */
> > -    env->fpus = 0;
> > -    env->fpstt = 0;
> > -    cpu_set_fpuc(env, 0x37f);
> > -    env->fptags[0] = 1;
> > -    env->fptags[1] = 1;
> > -    env->fptags[2] = 1;
> > -    env->fptags[3] = 1;
> > -    env->fptags[4] = 1;
> > -    env->fptags[5] = 1;
> > -    env->fptags[6] = 1;
> > -    env->fptags[7] = 1;
> > +    helper_fninit(env);
> >  }
> >
> >  void helper_frstor(CPUX86State *env, target_ulong ptr, int data32)
> > diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
> > index 52e94fe106..59647ea5b7 100644
> > --- a/target/i386/tcg/translate.c
> > +++ b/target/i386/tcg/translate.c
> > @@ -5839,6 +5839,11 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
> >          /* floats */
> >      case 0xd8 ... 0xdf:
> >          {
> > +            TCGv last_addr = tcg_temp_new();
> > +            int last_seg;
> > +            bool update_fdp = false;
> > +            bool update_fip = true;
> > +

Note: The variables are declared here, in the case block. Without the
braces added in the previous commit,
the variables can only be moved outside.

> >              if (s->flags & (HF_EM_MASK | HF_TS_MASK)) {
> >                  /* if CR0.EM or CR0.TS are set, generate an FPU exception */
> >                  /* XXX: what to do if illegal op ? */
> > @@ -5851,7 +5856,14 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
> >              op = ((b & 7) << 3) | ((modrm >> 3) & 7);
> >              if (mod != 3) {
> >                  /* memory op */
> > -                gen_lea_modrm(env, s, modrm);
> > +                AddressParts a = gen_lea_modrm_0(env, s, modrm);
> > +                TCGv ea = gen_lea_modrm_1(s, a);
> > +
> > +                update_fdp = true;
> > +                last_seg = a.def_seg;
> > +                tcg_gen_mov_tl(last_addr, ea);
> > +                gen_lea_v_seg(s, s->aflag, ea, a.def_seg, s->override);
> > +
> >                  switch (op) {
> >                  case 0x00 ... 0x07: /* fxxxs */
> >                  case 0x10 ... 0x17: /* fixxxl */
> > @@ -5978,19 +5990,23 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
> >                      break;
> >                  case 0x0c: /* fldenv mem */
> >                      gen_helper_fldenv(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> > +                    update_fip = update_fdp = false;
> >                      break;
> >                  case 0x0d: /* fldcw mem */
> >                      tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> >                                          s->mem_index, MO_LEUW);
> >                      gen_helper_fldcw(cpu_env, s->tmp2_i32);
> > +                    update_fip = update_fdp = false;
> >                      break;
> >                  case 0x0e: /* fnstenv mem */
> >                      gen_helper_fstenv(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> > +                    update_fip = update_fdp = false;
> >                      break;
> >                  case 0x0f: /* fnstcw mem */
> >                      gen_helper_fnstcw(s->tmp2_i32, cpu_env);
> >                      tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> >                                          s->mem_index, MO_LEUW);
> > +                    update_fip = update_fdp = false;
> >                      break;
> >                  case 0x1d: /* fldt mem */
> >                      gen_helper_fldt_ST0(cpu_env, s->A0);
> > @@ -6001,14 +6017,17 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
> >                      break;
> >                  case 0x2c: /* frstor mem */
> >                      gen_helper_frstor(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> > +                    update_fip = update_fdp = false;
> >                      break;
> >                  case 0x2e: /* fnsave mem */
> >                      gen_helper_fsave(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> > +                    update_fip = update_fdp = false;
> >                      break;
> >                  case 0x2f: /* fnstsw mem */
> >                      gen_helper_fnstsw(s->tmp2_i32, cpu_env);
> >                      tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> >                                          s->mem_index, MO_LEUW);
> > +                    update_fip = update_fdp = false;
> >                      break;
> >                  case 0x3c: /* fbld */
> >                      gen_helper_fbld_ST0(cpu_env, s->A0);
> > @@ -6051,6 +6070,7 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
> >                      case 0: /* fnop */
> >                          /* check exceptions (FreeBSD FPU probe) */
> >                          gen_helper_fwait(cpu_env);
> > +                        update_fip = update_fdp = false;
> >                          break;
> >                      default:
> >                          goto unknown_op;
> > @@ -6220,9 +6240,11 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
> >                          break;
> >                      case 2: /* fclex */
> >                          gen_helper_fclex(cpu_env);
> > +                        update_fip = update_fdp = false;
> >                          break;
> >                      case 3: /* fninit */
> >                          gen_helper_fninit(cpu_env);
> > +                        update_fip = update_fdp = false;
> >                          break;
> >                      case 4: /* fsetpm (287 only, just do nop here) */
> >                          break;
> > @@ -6343,6 +6365,27 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
> >                      goto unknown_op;
> >                  }
> >              }
> > +
> > +            if (update_fip) {
> > +                tcg_gen_ld32u_tl(s->T0, cpu_env,
> > +                                offsetof(CPUX86State, segs[R_CS].selector));
> > +                tcg_gen_st16_tl(s->T0, cpu_env, offsetof(CPUX86State, fpcs));
> > +
> > +                tcg_gen_movi_tl(s->T0, pc_start - s->cs_base);
> > +                tcg_gen_st_tl(s->T0, cpu_env, offsetof(CPUX86State, fpip));
> > +            }
> > +
> > +            if (update_fdp) {
> > +                if (s->override >= 0) {
> > +                    last_seg = s->override;
> > +                }
> > +                tcg_gen_ld32u_tl(s->T0, cpu_env,
> > +                                 offsetof(CPUX86State,
> > +                                 segs[last_seg].selector));
> > +                tcg_gen_st16_tl(s->T0, cpu_env, offsetof(CPUX86State, fpds));
> > +
> > +                tcg_gen_st_tl(last_addr, cpu_env, offsetof(CPUX86State, fpdp));
> > +            }
> >          }
> >          break;
> >          /************************/
> > --
> > 2.25.1
> >
>
> --
> Eduardo
>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 2/2] target/i386: Correct implementation for FCS, FIP,  FDS and FDP
  2021-05-18  3:06     ` Ziqiao Kong
@ 2021-05-24  7:41       ` Ziqiao Kong
  2021-05-27 21:08       ` Eduardo Habkost
  1 sibling, 0 replies; 10+ messages in thread
From: Ziqiao Kong @ 2021-05-24  7:41 UTC (permalink / raw)
  To: Eduardo Habkost; +Cc: Paolo Bonzini, Richard Henderson, QEMU Developers

Ping?

On Tue, May 18, 2021 at 11:06 AM Ziqiao Kong <ziqiaokong@gmail.com> wrote:
>
> On Tue, May 18, 2021 at 4:29 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > Hi,
> >
> > Thanks for the patch, and apologies for not reviewing earlier
> > versions.
> >
>
> Nevermind, the earlier version is also hard to review without a proper split.
>
> > On Fri, May 07, 2021 at 04:00:58PM +0800, Ziqiao Kong wrote:
> > > Changes since v3:
> > >  - Split the long patches to series to make review easier.
> > >  - Fix the coding style problems in v3.
> > >
> > > Changes since v2:
> > >  - Change the sequence of fpcs, fpds, fpip and fpdp in CPUX86State.
> > >  - Use stl instead of stw in do_fstenv.
> > >  - Move variables to floats instruction case block.
> > >  - Move last accessed memory operand to a temp variable to avoid another load.
> > >  - Move segment selectors instead of segment base to fpcs and fpds.
> > >  - Fix some code stype problems for the original code in floats case block.
> >
> > On the next versions, please include the changelog after a "---"
> > line, so it won't be included in the final commit.
> >
> > In addition to the changelog, the actual commit message (the text
> > above "---") needs to include an explanation for the change.  If
> > you are fixing a bug, please explain what's the bug you are
> > fixing.
>
> The bug is tracked in this thread
> https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg00307.html.
>
> >
> >
> > >
> > > Signed-off-by: Ziqiao Kong <ziqiaokong@gmail.com>
> > > ---
> > >  target/i386/cpu.h            |  4 +++
> > >  target/i386/tcg/fpu_helper.c | 48 ++++++++++++++++++++++--------------
> > >  target/i386/tcg/translate.c  | 45 ++++++++++++++++++++++++++++++++-
> > >  3 files changed, 77 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > > index 570f916878..241945320b 100644
> > > --- a/target/i386/cpu.h
> > > +++ b/target/i386/cpu.h
> > > @@ -705,6 +705,8 @@ typedef uint64_t FeatureWordArray[FEATURE_WORDS];
> > >  #define CPUID_7_0_EBX_INVPCID           (1U << 10)
> > >  /* Restricted Transactional Memory */
> > >  #define CPUID_7_0_EBX_RTM               (1U << 11)
> > > +/* Deprecates FPU CS and FPU DS values */
> > > +#define CPUID_7_0_EBX_FCS_FDS           (1U << 13)
> > >  /* Memory Protection Extension */
> > >  #define CPUID_7_0_EBX_MPX               (1U << 14)
> > >  /* AVX-512 Foundation */
> > > @@ -1440,6 +1442,8 @@ typedef struct CPUX86State {
> > >      FPReg fpregs[8];
> > >      /* KVM-only so far */
> > >      uint16_t fpop;
> > > +    uint16_t fpcs;
> > > +    uint16_t fpds;
> > >      uint64_t fpip;
> > >      uint64_t fpdp;
> > >
> > > diff --git a/target/i386/tcg/fpu_helper.c b/target/i386/tcg/fpu_helper.c
> > > index 60ed93520a..f1a8717ed8 100644
> > > --- a/target/i386/tcg/fpu_helper.c
> > > +++ b/target/i386/tcg/fpu_helper.c
> > > @@ -766,6 +766,10 @@ void helper_fninit(CPUX86State *env)
> > >  {
> > >      env->fpus = 0;
> > >      env->fpstt = 0;
> > > +    env->fpcs = 0;
> > > +    env->fpip = 0;
> > > +    env->fpds = 0;
> > > +    env->fpdp = 0;
> > >      cpu_set_fpuc(env, 0x37f);
> > >      env->fptags[0] = 1;
> > >      env->fptags[1] = 1;
> > > @@ -2368,6 +2372,7 @@ static void do_fstenv(CPUX86State *env, target_ulong ptr, int data32,
> > >  {
> > >      int fpus, fptag, exp, i;
> > >      uint64_t mant;
> > > +    uint16_t fpcs, fpds;
> > >      CPU_LDoubleU tmp;
> > >
> > >      fpus = (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11;
> > > @@ -2390,24 +2395,39 @@ static void do_fstenv(CPUX86State *env, target_ulong ptr, int data32,
> > >              }
> > >          }
> > >      }
> > > +
> > > +    /*
> > > +     * If CR0.PE = 1, each instruction saves FCS and FDS into memory. If
> > > +     * CPUID.(EAX=07H,ECX=0H):EBX[bit 13] = 1, the processor deprecates
> > > +     * FCS and FDS; it saves each as 0000H.
> > > +     */
> > > +    if (!(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_FCS_FDS)
> > > +        && (env->cr[0] & CR0_PE_MASK)) {
> > > +        fpcs = env->fpcs;
> > > +        fpds = env->fpds;
> >
> >
> > If you want to start supporting this feature flag, I suggest
> > moving this to a separate patch.  The description of this patch
> > seems to imply it is just a bug fix, not the implementation of a
> > new feature flag.
> >
> > When adding support for a new feature flag in TCG code, you need
> > to extend TCG_*_FEATURES in target/i386/cpu.c, otherwise the
> > feature flag will never be enabled by the CPU configuration code.
>
> Yes, currently this feature flag is never enabled for all CPU types.
> How about removing this
> feature flag in this patch and leaving a TODO in the comment? Thus I
> can issue another patch
> to complete the feature later.
>
> >
> >
> > > +    } else {
> > > +        fpcs = 0;
> > > +        fpds = 0;
> > > +    }
> > > +
> > >      if (data32) {
> > >          /* 32 bit */
> > >          cpu_stl_data_ra(env, ptr, env->fpuc, retaddr);
> > >          cpu_stl_data_ra(env, ptr + 4, fpus, retaddr);
> > >          cpu_stl_data_ra(env, ptr + 8, fptag, retaddr);
> > > -        cpu_stl_data_ra(env, ptr + 12, 0, retaddr); /* fpip */
> > > -        cpu_stl_data_ra(env, ptr + 16, 0, retaddr); /* fpcs */
> > > -        cpu_stl_data_ra(env, ptr + 20, 0, retaddr); /* fpoo */
> > > -        cpu_stl_data_ra(env, ptr + 24, 0, retaddr); /* fpos */
> > > +        cpu_stl_data_ra(env, ptr + 12, env->fpip, retaddr); /* fpip */
> > > +        cpu_stl_data_ra(env, ptr + 16, fpcs, retaddr); /* fpcs */
> > > +        cpu_stl_data_ra(env, ptr + 20, env->fpdp, retaddr); /* fpdp */
> > > +        cpu_stl_data_ra(env, ptr + 24, fpds, retaddr); /* fpds */
> > >      } else {
> > >          /* 16 bit */
> > >          cpu_stw_data_ra(env, ptr, env->fpuc, retaddr);
> > >          cpu_stw_data_ra(env, ptr + 2, fpus, retaddr);
> > >          cpu_stw_data_ra(env, ptr + 4, fptag, retaddr);
> > > -        cpu_stw_data_ra(env, ptr + 6, 0, retaddr);
> > > -        cpu_stw_data_ra(env, ptr + 8, 0, retaddr);
> > > -        cpu_stw_data_ra(env, ptr + 10, 0, retaddr);
> > > -        cpu_stw_data_ra(env, ptr + 12, 0, retaddr);
> > > +        cpu_stw_data_ra(env, ptr + 6, env->fpip, retaddr);
> > > +        cpu_stw_data_ra(env, ptr + 8, fpcs, retaddr);
> > > +        cpu_stw_data_ra(env, ptr + 10, env->fpdp, retaddr);
> > > +        cpu_stw_data_ra(env, ptr + 12, fpds, retaddr);
> > >      }
> > >  }
> > >
> > > @@ -2473,17 +2493,7 @@ void helper_fsave(CPUX86State *env, target_ulong ptr, int data32)
> > >      }
> > >
> > >      /* fninit */
> > > -    env->fpus = 0;
> > > -    env->fpstt = 0;
> > > -    cpu_set_fpuc(env, 0x37f);
> > > -    env->fptags[0] = 1;
> > > -    env->fptags[1] = 1;
> > > -    env->fptags[2] = 1;
> > > -    env->fptags[3] = 1;
> > > -    env->fptags[4] = 1;
> > > -    env->fptags[5] = 1;
> > > -    env->fptags[6] = 1;
> > > -    env->fptags[7] = 1;
> > > +    helper_fninit(env);
> > >  }
> > >
> > >  void helper_frstor(CPUX86State *env, target_ulong ptr, int data32)
> > > diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
> > > index 52e94fe106..59647ea5b7 100644
> > > --- a/target/i386/tcg/translate.c
> > > +++ b/target/i386/tcg/translate.c
> > > @@ -5839,6 +5839,11 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
> > >          /* floats */
> > >      case 0xd8 ... 0xdf:
> > >          {
> > > +            TCGv last_addr = tcg_temp_new();
> > > +            int last_seg;
> > > +            bool update_fdp = false;
> > > +            bool update_fip = true;
> > > +
>
> Note: The variables are declared here, in the case block. Without the
> braces added in the previous commit,
> the variables can only be moved outside.
>
> > >              if (s->flags & (HF_EM_MASK | HF_TS_MASK)) {
> > >                  /* if CR0.EM or CR0.TS are set, generate an FPU exception */
> > >                  /* XXX: what to do if illegal op ? */
> > > @@ -5851,7 +5856,14 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
> > >              op = ((b & 7) << 3) | ((modrm >> 3) & 7);
> > >              if (mod != 3) {
> > >                  /* memory op */
> > > -                gen_lea_modrm(env, s, modrm);
> > > +                AddressParts a = gen_lea_modrm_0(env, s, modrm);
> > > +                TCGv ea = gen_lea_modrm_1(s, a);
> > > +
> > > +                update_fdp = true;
> > > +                last_seg = a.def_seg;
> > > +                tcg_gen_mov_tl(last_addr, ea);
> > > +                gen_lea_v_seg(s, s->aflag, ea, a.def_seg, s->override);
> > > +
> > >                  switch (op) {
> > >                  case 0x00 ... 0x07: /* fxxxs */
> > >                  case 0x10 ... 0x17: /* fixxxl */
> > > @@ -5978,19 +5990,23 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
> > >                      break;
> > >                  case 0x0c: /* fldenv mem */
> > >                      gen_helper_fldenv(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> > > +                    update_fip = update_fdp = false;
> > >                      break;
> > >                  case 0x0d: /* fldcw mem */
> > >                      tcg_gen_qemu_ld_i32(s->tmp2_i32, s->A0,
> > >                                          s->mem_index, MO_LEUW);
> > >                      gen_helper_fldcw(cpu_env, s->tmp2_i32);
> > > +                    update_fip = update_fdp = false;
> > >                      break;
> > >                  case 0x0e: /* fnstenv mem */
> > >                      gen_helper_fstenv(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> > > +                    update_fip = update_fdp = false;
> > >                      break;
> > >                  case 0x0f: /* fnstcw mem */
> > >                      gen_helper_fnstcw(s->tmp2_i32, cpu_env);
> > >                      tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> > >                                          s->mem_index, MO_LEUW);
> > > +                    update_fip = update_fdp = false;
> > >                      break;
> > >                  case 0x1d: /* fldt mem */
> > >                      gen_helper_fldt_ST0(cpu_env, s->A0);
> > > @@ -6001,14 +6017,17 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
> > >                      break;
> > >                  case 0x2c: /* frstor mem */
> > >                      gen_helper_frstor(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> > > +                    update_fip = update_fdp = false;
> > >                      break;
> > >                  case 0x2e: /* fnsave mem */
> > >                      gen_helper_fsave(cpu_env, s->A0, tcg_const_i32(dflag - 1));
> > > +                    update_fip = update_fdp = false;
> > >                      break;
> > >                  case 0x2f: /* fnstsw mem */
> > >                      gen_helper_fnstsw(s->tmp2_i32, cpu_env);
> > >                      tcg_gen_qemu_st_i32(s->tmp2_i32, s->A0,
> > >                                          s->mem_index, MO_LEUW);
> > > +                    update_fip = update_fdp = false;
> > >                      break;
> > >                  case 0x3c: /* fbld */
> > >                      gen_helper_fbld_ST0(cpu_env, s->A0);
> > > @@ -6051,6 +6070,7 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
> > >                      case 0: /* fnop */
> > >                          /* check exceptions (FreeBSD FPU probe) */
> > >                          gen_helper_fwait(cpu_env);
> > > +                        update_fip = update_fdp = false;
> > >                          break;
> > >                      default:
> > >                          goto unknown_op;
> > > @@ -6220,9 +6240,11 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
> > >                          break;
> > >                      case 2: /* fclex */
> > >                          gen_helper_fclex(cpu_env);
> > > +                        update_fip = update_fdp = false;
> > >                          break;
> > >                      case 3: /* fninit */
> > >                          gen_helper_fninit(cpu_env);
> > > +                        update_fip = update_fdp = false;
> > >                          break;
> > >                      case 4: /* fsetpm (287 only, just do nop here) */
> > >                          break;
> > > @@ -6343,6 +6365,27 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
> > >                      goto unknown_op;
> > >                  }
> > >              }
> > > +
> > > +            if (update_fip) {
> > > +                tcg_gen_ld32u_tl(s->T0, cpu_env,
> > > +                                offsetof(CPUX86State, segs[R_CS].selector));
> > > +                tcg_gen_st16_tl(s->T0, cpu_env, offsetof(CPUX86State, fpcs));
> > > +
> > > +                tcg_gen_movi_tl(s->T0, pc_start - s->cs_base);
> > > +                tcg_gen_st_tl(s->T0, cpu_env, offsetof(CPUX86State, fpip));
> > > +            }
> > > +
> > > +            if (update_fdp) {
> > > +                if (s->override >= 0) {
> > > +                    last_seg = s->override;
> > > +                }
> > > +                tcg_gen_ld32u_tl(s->T0, cpu_env,
> > > +                                 offsetof(CPUX86State,
> > > +                                 segs[last_seg].selector));
> > > +                tcg_gen_st16_tl(s->T0, cpu_env, offsetof(CPUX86State, fpds));
> > > +
> > > +                tcg_gen_st_tl(last_addr, cpu_env, offsetof(CPUX86State, fpdp));
> > > +            }
> > >          }
> > >          break;
> > >          /************************/
> > > --
> > > 2.25.1
> > >
> >
> > --
> > Eduardo
> >


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 2/2] target/i386: Correct implementation for FCS, FIP,  FDS and FDP
  2021-05-18  3:06     ` Ziqiao Kong
  2021-05-24  7:41       ` Ziqiao Kong
@ 2021-05-27 21:08       ` Eduardo Habkost
  1 sibling, 0 replies; 10+ messages in thread
From: Eduardo Habkost @ 2021-05-27 21:08 UTC (permalink / raw)
  To: Ziqiao Kong; +Cc: Paolo Bonzini, Richard Henderson, QEMU Developers

On Tue, May 18, 2021 at 11:06:56AM +0800, Ziqiao Kong wrote:
[...]
> > > +    /*
> > > +     * If CR0.PE = 1, each instruction saves FCS and FDS into memory. If
> > > +     * CPUID.(EAX=07H,ECX=0H):EBX[bit 13] = 1, the processor deprecates
> > > +     * FCS and FDS; it saves each as 0000H.
> > > +     */
> > > +    if (!(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_FCS_FDS)
> > > +        && (env->cr[0] & CR0_PE_MASK)) {
> > > +        fpcs = env->fpcs;
> > > +        fpds = env->fpds;
> >
> >
> > If you want to start supporting this feature flag, I suggest
> > moving this to a separate patch.  The description of this patch
> > seems to imply it is just a bug fix, not the implementation of a
> > new feature flag.
> >
> > When adding support for a new feature flag in TCG code, you need
> > to extend TCG_*_FEATURES in target/i386/cpu.c, otherwise the
> > feature flag will never be enabled by the CPU configuration code.
> 
> Yes, currently this feature flag is never enabled for all CPU types.
> How about removing this
> feature flag in this patch and leaving a TODO in the comment? Thus I
> can issue another patch
> to complete the feature later.

Sounds better to me.  Otherwise you'll be just adding dead code
(because the flag will is impossible to be enabled if it's not
present in TCG_*_FEATURES).

-- 
Eduardo



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 1/2] target/i386: Trivial code motion
  2021-05-18  2:53   ` Ziqiao Kong
@ 2021-05-27 21:10     ` Eduardo Habkost
  0 siblings, 0 replies; 10+ messages in thread
From: Eduardo Habkost @ 2021-05-27 21:10 UTC (permalink / raw)
  To: Ziqiao Kong; +Cc: Paolo Bonzini, Richard Henderson, QEMU Developers

On Tue, May 18, 2021 at 10:53:51AM +0800, Ziqiao Kong wrote:
> On Tue, May 18, 2021 at 4:16 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > On Fri, May 07, 2021 at 04:00:56PM +0800, Ziqiao Kong wrote:
> > > Move the float translation case to a new block by a new pair of braces.
> >
> > If you are just adding braces around the code, do you really need
> > to reindent all the code?  I don't see any mention of `switch`
> > statements on style.rst, but I see 235 existing cases where the
> > brackets are aligned below the `c` in `case`.
> 
> The indention style is from the translate.c itself like here:
> 
> https://github.com/qemu/qemu/blob/master/target/i386/tcg/translate.c#L5634
> 
> There are also many similar cases in translate.c.

Oh, right.  Makes sense to me (and sorry for not noticing this
before).

> 
> >
> > In either case, I'm looking for a description of "why", not
> > "what", but I couldn't find it.  Why are the braces necessary or
> > useful here?
> 
> I have to declare some variables in this case scope, like last_addr, last_seg,
> update_fdp and update_fip according to the previous review. Without the braces
> here, they have to be declared at the beginning of the function like
> the v2 patch.
> Shall I state that in the commit message?

Yes, please.  Ideally every commit message should state the
reason for the change.

-- 
Eduardo



^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-05-27 21:11 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-07  8:00 [PATCH v4 1/2] target/i386: Trivial code motion Ziqiao Kong
2021-05-07  8:00 ` [PATCH v4 2/2] target/i386: Correct implementation for FCS, FIP, FDS and FDP Ziqiao Kong
2021-05-13 16:44   ` Ziqiao Kong
2021-05-17 20:29   ` Eduardo Habkost
2021-05-18  3:06     ` Ziqiao Kong
2021-05-24  7:41       ` Ziqiao Kong
2021-05-27 21:08       ` Eduardo Habkost
2021-05-17 20:16 ` [PATCH v4 1/2] target/i386: Trivial code motion Eduardo Habkost
2021-05-18  2:53   ` Ziqiao Kong
2021-05-27 21:10     ` Eduardo Habkost

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.