[Qemu-devel] [PATCH v2 0/3] target/arm: Reduce overhead of cpu_get_tb_cpu_state

[Qemu-devel] [PATCH v2 0/3] target/arm: Reduce overhead of cpu_get_tb_cpu_state

[Richard Henderson]

Changes since v1:
  * Apparently I had started a last-minute API change, and failed to
    covert all of the users, and also failed to re-test afterward.
  * Retain assertions for --enable-debug-tcg.


r~


Richard Henderson (3):
  target/arm: Split out recompute_hflags et al
  target/arm: Rebuild hflags at el changes and MSR writes
  target/arm: Rely on hflags correct in cpu_get_tb_cpu_state

 target/arm/cpu.h           |  22 ++-
 target/arm/helper.h        |   3 +
 target/arm/internals.h     |   4 +
 linux-user/syscall.c       |   1 +
 target/arm/cpu.c           |   1 +
 target/arm/helper-a64.c    |   3 +
 target/arm/helper.c        | 279 ++++++++++++++++++++++---------------
 target/arm/machine.c       |   1 +
 target/arm/op_helper.c     |   1 +
 target/arm/translate-a64.c |   6 +-
 target/arm/translate.c     |  14 +-
 11 files changed, 216 insertions(+), 119 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v2 1/3] target/arm: Split out recompute_hflags et al

[Richard Henderson]

We will use these to minimize the computation for every call to
cpu_get_tb_cpu_state.  For now, the env->hflags variable is not used.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.h       |  22 +++-
 target/arm/helper.h    |   3 +
 target/arm/internals.h |   3 +
 target/arm/helper.c    | 267 ++++++++++++++++++++++++-----------------
 4 files changed, 179 insertions(+), 116 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 84ae6849c2..848f0926eb 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -240,6 +240,9 @@ typedef struct CPUARMState {
     uint32_t pstate;
     uint32_t aarch64; /* 1 if CPU is in aarch64 state; inverse of PSTATE.nRW */
 
+    /* Cached TBFLAGS state.  See below for which bits are included.  */
+    uint32_t hflags;
+
     /* Frequently accessed CPSR bits are stored separately for efficiency.
        This contains all the other bits.  Use cpsr_{read,write} to access
        the whole CPSR.  */
@@ -3065,25 +3068,28 @@ static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
 
 #include "exec/cpu-all.h"
 
-/* Bit usage in the TB flags field: bit 31 indicates whether we are
+/*
+ * Bit usage in the TB flags field: bit 31 indicates whether we are
  * in 32 or 64 bit mode. The meaning of the other bits depends on that.
  * We put flags which are shared between 32 and 64 bit mode at the top
  * of the word, and flags which apply to only one mode at the bottom.
+ *
+ * Unless otherwise noted, these bits are cached in env->hflags.
  */
 FIELD(TBFLAG_ANY, AARCH64_STATE, 31, 1)
 FIELD(TBFLAG_ANY, MMUIDX, 28, 3)
 FIELD(TBFLAG_ANY, SS_ACTIVE, 27, 1)
-FIELD(TBFLAG_ANY, PSTATE_SS, 26, 1)
+FIELD(TBFLAG_ANY, PSTATE_SS, 26, 1)     /* Not cached. */
 /* Target EL if we take a floating-point-disabled exception */
 FIELD(TBFLAG_ANY, FPEXC_EL, 24, 2)
 FIELD(TBFLAG_ANY, BE_DATA, 23, 1)
 
 /* Bit usage when in AArch32 state: */
-FIELD(TBFLAG_A32, THUMB, 0, 1)
+FIELD(TBFLAG_A32, THUMB, 0, 1)          /* Not cached. */
 FIELD(TBFLAG_A32, VECLEN, 1, 3)
 FIELD(TBFLAG_A32, VECSTRIDE, 4, 2)
 FIELD(TBFLAG_A32, VFPEN, 7, 1)
-FIELD(TBFLAG_A32, CONDEXEC, 8, 8)
+FIELD(TBFLAG_A32, CONDEXEC, 8, 8)       /* Not cached. */
 FIELD(TBFLAG_A32, SCTLR_B, 16, 1)
 /* We store the bottom two bits of the CPAR as TB flags and handle
  * checks on the other bits at runtime
@@ -3105,7 +3111,7 @@ FIELD(TBFLAG_A64, SVEEXC_EL, 2, 2)
 FIELD(TBFLAG_A64, ZCR_LEN, 4, 4)
 FIELD(TBFLAG_A64, PAUTH_ACTIVE, 8, 1)
 FIELD(TBFLAG_A64, BT, 9, 1)
-FIELD(TBFLAG_A64, BTYPE, 10, 2)
+FIELD(TBFLAG_A64, BTYPE, 10, 2)         /* Not cached. */
 FIELD(TBFLAG_A64, TBID, 12, 2)
 
 static inline bool bswap_code(bool sctlr_b)
@@ -3190,6 +3196,12 @@ void arm_register_pre_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
 void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook, void
         *opaque);
 
+/**
+ * arm_rebuild_hflags:
+ * Rebuild the cached TBFLAGS for arbitrary changed processor state.
+ */
+void arm_rebuild_hflags(CPUARMState *env);
+
 /**
  * aa32_vfp_dreg:
  * Return a pointer to the Dn register within env in 32-bit mode.
diff --git a/target/arm/helper.h b/target/arm/helper.h
index 923e8e1525..bbc1a48089 100644
--- a/target/arm/helper.h
+++ b/target/arm/helper.h
@@ -89,6 +89,9 @@ DEF_HELPER_4(msr_banked, void, env, i32, i32, i32)
 DEF_HELPER_2(get_user_reg, i32, env, i32)
 DEF_HELPER_3(set_user_reg, void, env, i32, i32)
 
+DEF_HELPER_FLAGS_2(rebuild_hflags_a32, TCG_CALL_NO_RWG, void, env, i32)
+DEF_HELPER_FLAGS_2(rebuild_hflags_a64, TCG_CALL_NO_RWG, void, env, i32)
+
 DEF_HELPER_1(vfp_get_fpscr, i32, env)
 DEF_HELPER_2(vfp_set_fpscr, void, env, i32)
 
diff --git a/target/arm/internals.h b/target/arm/internals.h
index a4bd1becb7..8c1b813364 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -968,4 +968,7 @@ ARMVAParameters aa64_va_parameters_both(CPUARMState *env, uint64_t va,
 ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
                                    ARMMMUIdx mmu_idx, bool data);
 
+uint32_t rebuild_hflags_a32(CPUARMState *env, int el);
+uint32_t rebuild_hflags_a64(CPUARMState *env, int el);
+
 #endif
diff --git a/target/arm/helper.c b/target/arm/helper.c
index a018eb23fe..189e97a083 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -13886,122 +13886,15 @@ ARMMMUIdx arm_stage1_mmu_idx(CPUARMState *env)
 }
 #endif
 
-void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
-                          target_ulong *cs_base, uint32_t *pflags)
+static uint32_t common_hflags(CPUARMState *env, int el, ARMMMUIdx mmu_idx,
+                              int fp_el, uint32_t flags)
 {
-    ARMMMUIdx mmu_idx = arm_mmu_idx(env);
-    int current_el = arm_current_el(env);
-    int fp_el = fp_exception_el(env, current_el);
-    uint32_t flags = 0;
-
-    if (is_a64(env)) {
-        ARMCPU *cpu = arm_env_get_cpu(env);
-        uint64_t sctlr;
-
-        *pc = env->pc;
-        flags = FIELD_DP32(flags, TBFLAG_ANY, AARCH64_STATE, 1);
-
-        /* Get control bits for tagged addresses.  */
-        {
-            ARMMMUIdx stage1 = stage_1_mmu_idx(mmu_idx);
-            ARMVAParameters p0 = aa64_va_parameters_both(env, 0, stage1);
-            int tbii, tbid;
-
-            /* FIXME: ARMv8.1-VHE S2 translation regime.  */
-            if (regime_el(env, stage1) < 2) {
-                ARMVAParameters p1 = aa64_va_parameters_both(env, -1, stage1);
-                tbid = (p1.tbi << 1) | p0.tbi;
-                tbii = tbid & ~((p1.tbid << 1) | p0.tbid);
-            } else {
-                tbid = p0.tbi;
-                tbii = tbid & !p0.tbid;
-            }
-
-            flags = FIELD_DP32(flags, TBFLAG_A64, TBII, tbii);
-            flags = FIELD_DP32(flags, TBFLAG_A64, TBID, tbid);
-        }
-
-        if (cpu_isar_feature(aa64_sve, cpu)) {
-            int sve_el = sve_exception_el(env, current_el);
-            uint32_t zcr_len;
-
-            /* If SVE is disabled, but FP is enabled,
-             * then the effective len is 0.
-             */
-            if (sve_el != 0 && fp_el == 0) {
-                zcr_len = 0;
-            } else {
-                zcr_len = sve_zcr_len_for_el(env, current_el);
-            }
-            flags = FIELD_DP32(flags, TBFLAG_A64, SVEEXC_EL, sve_el);
-            flags = FIELD_DP32(flags, TBFLAG_A64, ZCR_LEN, zcr_len);
-        }
-
-        if (current_el == 0) {
-            /* FIXME: ARMv8.1-VHE S2 translation regime.  */
-            sctlr = env->cp15.sctlr_el[1];
-        } else {
-            sctlr = env->cp15.sctlr_el[current_el];
-        }
-        if (cpu_isar_feature(aa64_pauth, cpu)) {
-            /*
-             * In order to save space in flags, we record only whether
-             * pauth is "inactive", meaning all insns are implemented as
-             * a nop, or "active" when some action must be performed.
-             * The decision of which action to take is left to a helper.
-             */
-            if (sctlr & (SCTLR_EnIA | SCTLR_EnIB | SCTLR_EnDA | SCTLR_EnDB)) {
-                flags = FIELD_DP32(flags, TBFLAG_A64, PAUTH_ACTIVE, 1);
-            }
-        }
-
-        if (cpu_isar_feature(aa64_bti, cpu)) {
-            /* Note that SCTLR_EL[23].BT == SCTLR_BT1.  */
-            if (sctlr & (current_el == 0 ? SCTLR_BT0 : SCTLR_BT1)) {
-                flags = FIELD_DP32(flags, TBFLAG_A64, BT, 1);
-            }
-            flags = FIELD_DP32(flags, TBFLAG_A64, BTYPE, env->btype);
-        }
-    } else {
-        *pc = env->regs[15];
-        flags = FIELD_DP32(flags, TBFLAG_A32, THUMB, env->thumb);
-        flags = FIELD_DP32(flags, TBFLAG_A32, VECLEN, env->vfp.vec_len);
-        flags = FIELD_DP32(flags, TBFLAG_A32, VECSTRIDE, env->vfp.vec_stride);
-        flags = FIELD_DP32(flags, TBFLAG_A32, CONDEXEC, env->condexec_bits);
-        flags = FIELD_DP32(flags, TBFLAG_A32, SCTLR_B, arm_sctlr_b(env));
-        flags = FIELD_DP32(flags, TBFLAG_A32, NS, !access_secure_reg(env));
-        if (env->vfp.xregs[ARM_VFP_FPEXC] & (1 << 30)
-            || arm_el_is_aa64(env, 1)) {
-            flags = FIELD_DP32(flags, TBFLAG_A32, VFPEN, 1);
-        }
-        flags = FIELD_DP32(flags, TBFLAG_A32, XSCALE_CPAR, env->cp15.c15_cpar);
-    }
-
     flags = FIELD_DP32(flags, TBFLAG_ANY, MMUIDX, arm_to_core_mmu_idx(mmu_idx));
+    flags = FIELD_DP32(flags, TBFLAG_ANY, FPEXC_EL, fp_el);
 
-    /* The SS_ACTIVE and PSTATE_SS bits correspond to the state machine
-     * states defined in the ARM ARM for software singlestep:
-     *  SS_ACTIVE   PSTATE.SS   State
-     *     0            x       Inactive (the TB flag for SS is always 0)
-     *     1            0       Active-pending
-     *     1            1       Active-not-pending
-     */
-    if (arm_singlestep_active(env)) {
-        flags = FIELD_DP32(flags, TBFLAG_ANY, SS_ACTIVE, 1);
-        if (is_a64(env)) {
-            if (env->pstate & PSTATE_SS) {
-                flags = FIELD_DP32(flags, TBFLAG_ANY, PSTATE_SS, 1);
-            }
-        } else {
-            if (env->uncached_cpsr & PSTATE_SS) {
-                flags = FIELD_DP32(flags, TBFLAG_ANY, PSTATE_SS, 1);
-            }
-        }
-    }
     if (arm_cpu_data_is_big_endian(env)) {
         flags = FIELD_DP32(flags, TBFLAG_ANY, BE_DATA, 1);
     }
-    flags = FIELD_DP32(flags, TBFLAG_ANY, FPEXC_EL, fp_el);
 
     if (arm_v7m_is_handler_mode(env)) {
         flags = FIELD_DP32(flags, TBFLAG_A32, HANDLER, 1);
@@ -14017,8 +13910,160 @@ void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
         flags = FIELD_DP32(flags, TBFLAG_A32, STACKCHECK, 1);
     }
 
-    *pflags = flags;
+    if (arm_singlestep_active(env)) {
+        flags = FIELD_DP32(flags, TBFLAG_ANY, SS_ACTIVE, 1);
+    }
+
+    return flags;
+}
+
+uint32_t rebuild_hflags_a32(CPUARMState *env, int el)
+{
+    uint32_t flags = 0;
+    ARMMMUIdx mmu_idx;
+    int fp_el;
+
+    flags = FIELD_DP32(flags, TBFLAG_A32, VECLEN, env->vfp.vec_len);
+    flags = FIELD_DP32(flags, TBFLAG_A32, VECSTRIDE, env->vfp.vec_stride);
+    flags = FIELD_DP32(flags, TBFLAG_A32, SCTLR_B, arm_sctlr_b(env));
+    flags = FIELD_DP32(flags, TBFLAG_A32, NS, !access_secure_reg(env));
+    if (env->vfp.xregs[ARM_VFP_FPEXC] & (1 << 30)
+        || arm_el_is_aa64(env, 1)) {
+        flags = FIELD_DP32(flags, TBFLAG_A32, VFPEN, 1);
+    }
+    flags = FIELD_DP32(flags, TBFLAG_A32, XSCALE_CPAR, env->cp15.c15_cpar);
+
+    mmu_idx = arm_mmu_idx(env);
+    fp_el = fp_exception_el(env, el);
+    return common_hflags(env, el, mmu_idx, fp_el, flags);
+}
+
+uint32_t rebuild_hflags_a64(CPUARMState *env, int el)
+{
+    ARMCPU *cpu = arm_env_get_cpu(env);
+    ARMMMUIdx mmu_idx = arm_mmu_idx(env);
+    ARMMMUIdx stage1 = stage_1_mmu_idx(mmu_idx);
+    ARMVAParameters p0 = aa64_va_parameters_both(env, 0, stage1);
+    int fp_el = fp_exception_el(env, el);
+    uint32_t flags = 0;
+    uint64_t sctlr;
+    int tbii, tbid;
+
+    flags = FIELD_DP32(flags, TBFLAG_ANY, AARCH64_STATE, 1);
+
+    /* Get control bits for tagged addresses.  */
+    /* FIXME: ARMv8.1-VHE S2 translation regime.  */
+    if (regime_el(env, stage1) < 2) {
+        ARMVAParameters p1 = aa64_va_parameters_both(env, -1, stage1);
+        tbid = (p1.tbi << 1) | p0.tbi;
+        tbii = tbid & ~((p1.tbid << 1) | p0.tbid);
+    } else {
+        tbid = p0.tbi;
+        tbii = tbid & !p0.tbid;
+    }
+
+    flags = FIELD_DP32(flags, TBFLAG_A64, TBII, tbii);
+    flags = FIELD_DP32(flags, TBFLAG_A64, TBID, tbid);
+
+    if (cpu_isar_feature(aa64_sve, cpu)) {
+        int sve_el = sve_exception_el(env, el);
+        uint32_t zcr_len;
+
+        /* If SVE is disabled, but FP is enabled,
+         * then the effective len is 0.
+         */
+        if (sve_el != 0 && fp_el == 0) {
+            zcr_len = 0;
+        } else {
+            zcr_len = sve_zcr_len_for_el(env, el);
+        }
+        flags = FIELD_DP32(flags, TBFLAG_A64, SVEEXC_EL, sve_el);
+        flags = FIELD_DP32(flags, TBFLAG_A64, ZCR_LEN, zcr_len);
+    }
+
+    if (el == 0) {
+        /* FIXME: ARMv8.1-VHE S2 translation regime.  */
+        sctlr = env->cp15.sctlr_el[1];
+    } else {
+        sctlr = env->cp15.sctlr_el[el];
+    }
+    if (cpu_isar_feature(aa64_pauth, cpu)) {
+        /*
+         * In order to save space in flags, we record only whether
+         * pauth is "inactive", meaning all insns are implemented as
+         * a nop, or "active" when some action must be performed.
+         * The decision of which action to take is left to a helper.
+         */
+        if (sctlr & (SCTLR_EnIA | SCTLR_EnIB | SCTLR_EnDA | SCTLR_EnDB)) {
+            flags = FIELD_DP32(flags, TBFLAG_A64, PAUTH_ACTIVE, 1);
+        }
+    }
+
+    if (cpu_isar_feature(aa64_bti, cpu)) {
+        /* Note that SCTLR_EL[23].BT == SCTLR_BT1.  */
+        if (sctlr & (el == 0 ? SCTLR_BT0 : SCTLR_BT1)) {
+            flags = FIELD_DP32(flags, TBFLAG_A64, BT, 1);
+        }
+    }
+
+    return common_hflags(env, el, mmu_idx, fp_el, flags);
+}
+
+void arm_rebuild_hflags(CPUARMState *env)
+{
+    int el = arm_current_el(env);
+    env->hflags = (is_a64(env)
+                   ? rebuild_hflags_a64(env, el)
+                   : rebuild_hflags_a32(env, el));
+}
+
+void HELPER(rebuild_hflags_a32)(CPUARMState *env, uint32_t el)
+{
+    tcg_debug_assert(!is_a64(env));
+    env->hflags = rebuild_hflags_a32(env, el);
+}
+
+void HELPER(rebuild_hflags_a64)(CPUARMState *env, uint32_t el)
+{
+    tcg_debug_assert(is_a64(env));
+    env->hflags = rebuild_hflags_a64(env, el);
+}
+
+void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
+                          target_ulong *cs_base, uint32_t *pflags)
+{
+    int current_el = arm_current_el(env);
+    uint32_t flags;
+    uint32_t pstate_for_ss;
+
     *cs_base = 0;
+    if (is_a64(env)) {
+        *pc = env->pc;
+        flags = rebuild_hflags_a64(env, current_el);
+        flags = FIELD_DP32(flags, TBFLAG_A64, BTYPE, env->btype);
+        pstate_for_ss = env->pstate;
+    } else {
+        *pc = env->regs[15];
+        flags = rebuild_hflags_a32(env, current_el);
+        flags = FIELD_DP32(flags, TBFLAG_A32, THUMB, env->thumb);
+        flags = FIELD_DP32(flags, TBFLAG_A32, CONDEXEC, env->condexec_bits);
+        pstate_for_ss = env->uncached_cpsr;
+    }
+
+    /* The SS_ACTIVE and PSTATE_SS bits correspond to the state machine
+     * states defined in the ARM ARM for software singlestep:
+     *  SS_ACTIVE   PSTATE.SS   State
+     *     0            x       Inactive (the TB flag for SS is always 0)
+     *     1            0       Active-pending
+     *     1            1       Active-not-pending
+     * SS_ACTIVE is set in hflags; PSTATE_SS is computed every TB.
+     */
+    if (FIELD_EX32(flags, TBFLAG_ANY, SS_ACTIVE)
+        && (pstate_for_ss & PSTATE_SS)) {
+        flags = FIELD_DP32(flags, TBFLAG_ANY, PSTATE_SS, 1);
+    }
+
+    *pflags = flags;
 }
 
 #ifdef TARGET_AARCH64
-- 
2.17.1

[Qemu-devel] [PATCH v2 2/3] target/arm: Rebuild hflags at el changes and MSR writes

[Richard Henderson]

Now setting, but not relying upon, env->hflags.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
v2: Fixed partial conversion to assignment to env->hflags.
---
 target/arm/internals.h     |  1 +
 linux-user/syscall.c       |  1 +
 target/arm/cpu.c           |  1 +
 target/arm/helper-a64.c    |  3 +++
 target/arm/helper.c        |  2 ++
 target/arm/machine.c       |  1 +
 target/arm/op_helper.c     |  1 +
 target/arm/translate-a64.c |  6 +++++-
 target/arm/translate.c     | 14 ++++++++++++--
 9 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 8c1b813364..235f4fafec 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -970,5 +970,6 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
 
 uint32_t rebuild_hflags_a32(CPUARMState *env, int el);
 uint32_t rebuild_hflags_a64(CPUARMState *env, int el);
+void rebuild_hflags_any(CPUARMState *env);
 
 #endif
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 5bbb72f3d5..123f342bdc 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -9691,6 +9691,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
                     aarch64_sve_narrow_vq(env, vq);
                 }
                 env->vfp.zcr_el[1] = vq - 1;
+                arm_rebuild_hflags(env);
                 ret = vq * 16;
             }
             return ret;
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index edf6e0e1f1..e4da513eb3 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -390,6 +390,7 @@ static void arm_cpu_reset(CPUState *s)
 
     hw_breakpoint_update_all(cpu);
     hw_watchpoint_update_all(cpu);
+    arm_rebuild_hflags(env);
 }
 
 bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c
index 70850e564d..17200f1288 100644
--- a/target/arm/helper-a64.c
+++ b/target/arm/helper-a64.c
@@ -995,6 +995,7 @@ void HELPER(exception_return)(CPUARMState *env, uint64_t new_pc)
         } else {
             env->regs[15] = new_pc & ~0x3;
         }
+        env->hflags = rebuild_hflags_a32(env, new_el);
         qemu_log_mask(CPU_LOG_INT, "Exception return from AArch64 EL%d to "
                       "AArch32 EL%d PC 0x%" PRIx32 "\n",
                       cur_el, new_el, env->regs[15]);
@@ -1006,10 +1007,12 @@ void HELPER(exception_return)(CPUARMState *env, uint64_t new_pc)
         }
         aarch64_restore_sp(env, new_el);
         env->pc = new_pc;
+        env->hflags = rebuild_hflags_a64(env, new_el);
         qemu_log_mask(CPU_LOG_INT, "Exception return from AArch64 EL%d to "
                       "AArch64 EL%d PC 0x%" PRIx64 "\n",
                       cur_el, new_el, env->pc);
     }
+
     /*
      * Note that cur_el can never be 0.  If new_el is 0, then
      * el0_a64 is return_to_aa64, else el0_a64 is ignored.
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 189e97a083..909535a3e3 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -9201,6 +9201,7 @@ static void take_aarch32_exception(CPUARMState *env, int new_mode,
         env->regs[14] = env->regs[15] + offset;
     }
     env->regs[15] = newpc;
+    env->hflags = rebuild_hflags_a32(env, arm_current_el(env));
 }
 
 static void arm_cpu_do_interrupt_aarch32_hyp(CPUState *cs)
@@ -9546,6 +9547,7 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
 
     pstate_write(env, PSTATE_DAIF | new_mode);
     env->aarch64 = 1;
+    env->hflags = rebuild_hflags_a64(env, new_el);
     aarch64_restore_sp(env, new_el);
 
     env->pc = addr;
diff --git a/target/arm/machine.c b/target/arm/machine.c
index 124192bfc2..e944d6b736 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -743,6 +743,7 @@ static int cpu_post_load(void *opaque, int version_id)
     if (!kvm_enabled()) {
         pmu_op_finish(&cpu->env);
     }
+    arm_rebuild_hflags(&cpu->env);
 
     return 0;
 }
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index c998eadfaa..f82eeae7e4 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -571,6 +571,7 @@ void HELPER(cpsr_write_eret)(CPUARMState *env, uint32_t val)
      */
     env->regs[15] &= (env->thumb ? ~1 : ~3);
 
+    env->hflags = rebuild_hflags_a32(env, arm_current_el(env));
     qemu_mutex_lock_iothread();
     arm_call_el_change_hook(arm_env_get_cpu(env));
     qemu_mutex_unlock_iothread();
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index af8e4fd4be..a786c7ef5f 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1841,11 +1841,15 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
         /* I/O operations must end the TB here (whether read or write) */
         gen_io_end();
         s->base.is_jmp = DISAS_UPDATE;
-    } else if (!isread && !(ri->type & ARM_CP_SUPPRESS_TB_END)) {
+    }
+    if (!isread && !(ri->type & ARM_CP_SUPPRESS_TB_END)) {
         /* We default to ending the TB on a coprocessor register write,
          * but allow this to be suppressed by the register definition
          * (usually only necessary to work around guest bugs).
          */
+        TCGv_i32 tcg_el = tcg_const_i32(s->current_el);
+        gen_helper_rebuild_hflags_a64(cpu_env, tcg_el);
+        tcg_temp_free_i32(tcg_el);
         s->base.is_jmp = DISAS_UPDATE;
     }
 }
diff --git a/target/arm/translate.c b/target/arm/translate.c
index dac737f6ca..1cdb575ccd 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -8563,6 +8563,8 @@ static int disas_coproc_insn(DisasContext *s, uint32_t insn)
     ri = get_arm_cp_reginfo(s->cp_regs,
             ENCODE_CP_REG(cpnum, is64, s->ns, crn, crm, opc1, opc2));
     if (ri) {
+        bool need_exit_tb;
+
         /* Check access permissions */
         if (!cp_access_ok(s->current_el, ri, isread)) {
             return 1;
@@ -8735,15 +8737,23 @@ static int disas_coproc_insn(DisasContext *s, uint32_t insn)
             }
         }
 
+        need_exit_tb = false;
         if ((tb_cflags(s->base.tb) & CF_USE_ICOUNT) && (ri->type & ARM_CP_IO)) {
             /* I/O operations must end the TB here (whether read or write) */
             gen_io_end();
-            gen_lookup_tb(s);
-        } else if (!isread && !(ri->type & ARM_CP_SUPPRESS_TB_END)) {
+            need_exit_tb = true;
+        }
+        if (!isread && !(ri->type & ARM_CP_SUPPRESS_TB_END)) {
             /* We default to ending the TB on a coprocessor register write,
              * but allow this to be suppressed by the register definition
              * (usually only necessary to work around guest bugs).
              */
+            TCGv_i32 tcg_el = tcg_const_i32(s->current_el);
+            gen_helper_rebuild_hflags_a32(cpu_env, tcg_el);
+            tcg_temp_free_i32(tcg_el);
+            need_exit_tb = true;
+        }
+        if (need_exit_tb) {
             gen_lookup_tb(s);
         }
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 2/3] target/arm: Rebuild hflags at el changes and MSR writes

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> Now setting, but not relying upon, env->hflags.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
> v2: Fixed partial conversion to assignment to env->hflags.
> ---
>  target/arm/internals.h     |  1 +
>  linux-user/syscall.c       |  1 +
>  target/arm/cpu.c           |  1 +
>  target/arm/helper-a64.c    |  3 +++
>  target/arm/helper.c        |  2 ++
>  target/arm/machine.c       |  1 +
>  target/arm/op_helper.c     |  1 +
>  target/arm/translate-a64.c |  6 +++++-
>  target/arm/translate.c     | 14 ++++++++++++--
>  9 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 8c1b813364..235f4fafec 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -970,5 +970,6 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
>
>  uint32_t rebuild_hflags_a32(CPUARMState *env, int el);
>  uint32_t rebuild_hflags_a64(CPUARMState *env, int el);
> +void rebuild_hflags_any(CPUARMState *env);
>
>  #endif
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 5bbb72f3d5..123f342bdc 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -9691,6 +9691,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>                      aarch64_sve_narrow_vq(env, vq);
>                  }
>                  env->vfp.zcr_el[1] = vq - 1;
> +                arm_rebuild_hflags(env);
>                  ret = vq * 16;
>              }
>              return ret;
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index edf6e0e1f1..e4da513eb3 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -390,6 +390,7 @@ static void arm_cpu_reset(CPUState *s)
>
>      hw_breakpoint_update_all(cpu);
>      hw_watchpoint_update_all(cpu);
> +    arm_rebuild_hflags(env);
>  }
>
>  bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
> diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c
> index 70850e564d..17200f1288 100644
> --- a/target/arm/helper-a64.c
> +++ b/target/arm/helper-a64.c
> @@ -995,6 +995,7 @@ void HELPER(exception_return)(CPUARMState *env, uint64_t new_pc)
>          } else {
>              env->regs[15] = new_pc & ~0x3;
>          }
> +        env->hflags = rebuild_hflags_a32(env, new_el);
>          qemu_log_mask(CPU_LOG_INT, "Exception return from AArch64 EL%d to "
>                        "AArch32 EL%d PC 0x%" PRIx32 "\n",
>                        cur_el, new_el, env->regs[15]);
> @@ -1006,10 +1007,12 @@ void HELPER(exception_return)(CPUARMState *env, uint64_t new_pc)
>          }
>          aarch64_restore_sp(env, new_el);
>          env->pc = new_pc;
> +        env->hflags = rebuild_hflags_a64(env, new_el);
>          qemu_log_mask(CPU_LOG_INT, "Exception return from AArch64 EL%d to "
>                        "AArch64 EL%d PC 0x%" PRIx64 "\n",
>                        cur_el, new_el, env->pc);
>      }
> +
>      /*
>       * Note that cur_el can never be 0.  If new_el is 0, then
>       * el0_a64 is return_to_aa64, else el0_a64 is ignored.
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 189e97a083..909535a3e3 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -9201,6 +9201,7 @@ static void take_aarch32_exception(CPUARMState *env, int new_mode,
>          env->regs[14] = env->regs[15] + offset;
>      }
>      env->regs[15] = newpc;
> +    env->hflags = rebuild_hflags_a32(env, arm_current_el(env));
>  }
>
>  static void arm_cpu_do_interrupt_aarch32_hyp(CPUState *cs)
> @@ -9546,6 +9547,7 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
>
>      pstate_write(env, PSTATE_DAIF | new_mode);
>      env->aarch64 = 1;
> +    env->hflags = rebuild_hflags_a64(env, new_el);
>      aarch64_restore_sp(env, new_el);
>
>      env->pc = addr;
> diff --git a/target/arm/machine.c b/target/arm/machine.c
> index 124192bfc2..e944d6b736 100644
> --- a/target/arm/machine.c
> +++ b/target/arm/machine.c
> @@ -743,6 +743,7 @@ static int cpu_post_load(void *opaque, int version_id)
>      if (!kvm_enabled()) {
>          pmu_op_finish(&cpu->env);
>      }
> +    arm_rebuild_hflags(&cpu->env);
>
>      return 0;
>  }
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index c998eadfaa..f82eeae7e4 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -571,6 +571,7 @@ void HELPER(cpsr_write_eret)(CPUARMState *env, uint32_t val)
>       */
>      env->regs[15] &= (env->thumb ? ~1 : ~3);
>
> +    env->hflags = rebuild_hflags_a32(env, arm_current_el(env));
>      qemu_mutex_lock_iothread();
>      arm_call_el_change_hook(arm_env_get_cpu(env));
>      qemu_mutex_unlock_iothread();
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index af8e4fd4be..a786c7ef5f 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -1841,11 +1841,15 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
>          /* I/O operations must end the TB here (whether read or write) */
>          gen_io_end();
>          s->base.is_jmp = DISAS_UPDATE;
> -    } else if (!isread && !(ri->type & ARM_CP_SUPPRESS_TB_END)) {
> +    }
> +    if (!isread && !(ri->type & ARM_CP_SUPPRESS_TB_END)) {
>          /* We default to ending the TB on a coprocessor register write,
>           * but allow this to be suppressed by the register definition
>           * (usually only necessary to work around guest bugs).
>           */
> +        TCGv_i32 tcg_el = tcg_const_i32(s->current_el);
> +        gen_helper_rebuild_hflags_a64(cpu_env, tcg_el);
> +        tcg_temp_free_i32(tcg_el);
>          s->base.is_jmp = DISAS_UPDATE;
>      }
>  }
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index dac737f6ca..1cdb575ccd 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -8563,6 +8563,8 @@ static int disas_coproc_insn(DisasContext *s, uint32_t insn)
>      ri = get_arm_cp_reginfo(s->cp_regs,
>              ENCODE_CP_REG(cpnum, is64, s->ns, crn, crm, opc1, opc2));
>      if (ri) {
> +        bool need_exit_tb;
> +
>          /* Check access permissions */
>          if (!cp_access_ok(s->current_el, ri, isread)) {
>              return 1;
> @@ -8735,15 +8737,23 @@ static int disas_coproc_insn(DisasContext *s, uint32_t insn)
>              }
>          }
>
> +        need_exit_tb = false;
>          if ((tb_cflags(s->base.tb) & CF_USE_ICOUNT) && (ri->type & ARM_CP_IO)) {
>              /* I/O operations must end the TB here (whether read or write) */
>              gen_io_end();
> -            gen_lookup_tb(s);
> -        } else if (!isread && !(ri->type & ARM_CP_SUPPRESS_TB_END)) {
> +            need_exit_tb = true;
> +        }
> +        if (!isread && !(ri->type & ARM_CP_SUPPRESS_TB_END)) {
>              /* We default to ending the TB on a coprocessor register write,
>               * but allow this to be suppressed by the register definition
>               * (usually only necessary to work around guest bugs).
>               */
> +            TCGv_i32 tcg_el = tcg_const_i32(s->current_el);
> +            gen_helper_rebuild_hflags_a32(cpu_env, tcg_el);
> +            tcg_temp_free_i32(tcg_el);
> +            need_exit_tb = true;
> +        }
> +        if (need_exit_tb) {
>              gen_lookup_tb(s);
>          }


--
Alex Bennée

[Qemu-devel] [PATCH v2 3/3] target/arm: Rely on hflags correct in cpu_get_tb_cpu_state

[Richard Henderson]

This is the payoff.

>From perf record -g data of ubuntu 18 boot and shutdown:

BEFORE:

-   23.02%     2.82%  qemu-system-aar  [.] helper_lookup_tb_ptr
   - 20.22% helper_lookup_tb_ptr
      + 10.05% tb_htable_lookup
      - 9.13% cpu_get_tb_cpu_state
           3.20% aa64_va_parameters_both
           0.55% fp_exception_el

-   11.66%     4.74%  qemu-system-aar  [.] cpu_get_tb_cpu_state
   - 6.96% cpu_get_tb_cpu_state
        3.63% aa64_va_parameters_both
        0.60% fp_exception_el
        0.53% sve_exception_el

AFTER:

-   16.40%     3.40%  qemu-system-aar  [.] helper_lookup_tb_ptr
   - 13.03% helper_lookup_tb_ptr
      + 11.19% tb_htable_lookup
        0.55% cpu_get_tb_cpu_state

     0.98%     0.71%  qemu-system-aar  [.] cpu_get_tb_cpu_state

     0.87%     0.24%  qemu-system-aar  [.] rebuild_hflags_a64

Before, helper_lookup_tb_ptr is the second hottest function in the
application, consuming almost a quarter of the runtime.  Within the
entire execution, cpu_get_tb_cpu_state consumes about 12%.

After, helper_lookup_tb_ptr has dropped to the fourth hottest function,
with consumption dropping to a sixth of the runtime.  Within the
entire execution, cpu_get_tb_cpu_state has dropped below 1%, and the
supporting function to rebuild hflags also consumes about 1%.

Assertions are retained for --enable-debug-tcg.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
v2: Retain asserts for future debugging.
---
 target/arm/helper.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 909535a3e3..990a87876f 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -14034,19 +14034,29 @@ void HELPER(rebuild_hflags_a64)(CPUARMState *env, uint32_t el)
 void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
                           target_ulong *cs_base, uint32_t *pflags)
 {
-    int current_el = arm_current_el(env);
-    uint32_t flags;
+    uint32_t flags = env->hflags;
     uint32_t pstate_for_ss;
 
+#ifdef CONFIG_DEBUG_TCG
+    {
+        int el = arm_current_el(env);
+        uint32_t check_flags;
+        if (is_a64(env)) {
+            check_flags = rebuild_hflags_a64(env, el);
+        } else {
+            check_flags = rebuild_hflags_a32(env, el);
+        }
+        g_assert_cmphex(flags, ==, check_flags);
+    }
+#endif
+
     *cs_base = 0;
-    if (is_a64(env)) {
+    if (FIELD_EX32(flags, TBFLAG_ANY, AARCH64_STATE)) {
         *pc = env->pc;
-        flags = rebuild_hflags_a64(env, current_el);
         flags = FIELD_DP32(flags, TBFLAG_A64, BTYPE, env->btype);
         pstate_for_ss = env->pstate;
     } else {
         *pc = env->regs[15];
-        flags = rebuild_hflags_a32(env, current_el);
         flags = FIELD_DP32(flags, TBFLAG_A32, THUMB, env->thumb);
         flags = FIELD_DP32(flags, TBFLAG_A32, CONDEXEC, env->condexec_bits);
         pstate_for_ss = env->uncached_cpsr;
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 3/3] target/arm: Rely on hflags correct in cpu_get_tb_cpu_state

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> This is the payoff.
>
> From perf record -g data of ubuntu 18 boot and shutdown:
>
> BEFORE:
>
> -   23.02%     2.82%  qemu-system-aar  [.] helper_lookup_tb_ptr
>    - 20.22% helper_lookup_tb_ptr
>       + 10.05% tb_htable_lookup
>       - 9.13% cpu_get_tb_cpu_state
>            3.20% aa64_va_parameters_both
>            0.55% fp_exception_el
>
> -   11.66%     4.74%  qemu-system-aar  [.] cpu_get_tb_cpu_state
>    - 6.96% cpu_get_tb_cpu_state
>         3.63% aa64_va_parameters_both
>         0.60% fp_exception_el
>         0.53% sve_exception_el
>
> AFTER:
>
> -   16.40%     3.40%  qemu-system-aar  [.] helper_lookup_tb_ptr
>    - 13.03% helper_lookup_tb_ptr
>       + 11.19% tb_htable_lookup
>         0.55% cpu_get_tb_cpu_state
>
>      0.98%     0.71%  qemu-system-aar  [.] cpu_get_tb_cpu_state
>
>      0.87%     0.24%  qemu-system-aar  [.] rebuild_hflags_a64
>
> Before, helper_lookup_tb_ptr is the second hottest function in the
> application, consuming almost a quarter of the runtime.  Within the
> entire execution, cpu_get_tb_cpu_state consumes about 12%.
>
> After, helper_lookup_tb_ptr has dropped to the fourth hottest function,
> with consumption dropping to a sixth of the runtime.  Within the
> entire execution, cpu_get_tb_cpu_state has dropped below 1%, and the
> supporting function to rebuild hflags also consumes about 1%.
>
> Assertions are retained for --enable-debug-tcg.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Alex Bennée <alex.bennee@linaro.org>

> ---
> v2: Retain asserts for future debugging.
> ---
>  target/arm/helper.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 909535a3e3..990a87876f 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -14034,19 +14034,29 @@ void HELPER(rebuild_hflags_a64)(CPUARMState *env, uint32_t el)
>  void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>                            target_ulong *cs_base, uint32_t *pflags)
>  {
> -    int current_el = arm_current_el(env);
> -    uint32_t flags;
> +    uint32_t flags = env->hflags;
>      uint32_t pstate_for_ss;
>
> +#ifdef CONFIG_DEBUG_TCG
> +    {
> +        int el = arm_current_el(env);
> +        uint32_t check_flags;
> +        if (is_a64(env)) {
> +            check_flags = rebuild_hflags_a64(env, el);
> +        } else {
> +            check_flags = rebuild_hflags_a32(env, el);
> +        }
> +        g_assert_cmphex(flags, ==, check_flags);
> +    }
> +#endif
> +
>      *cs_base = 0;
> -    if (is_a64(env)) {
> +    if (FIELD_EX32(flags, TBFLAG_ANY, AARCH64_STATE)) {
>          *pc = env->pc;
> -        flags = rebuild_hflags_a64(env, current_el);
>          flags = FIELD_DP32(flags, TBFLAG_A64, BTYPE, env->btype);
>          pstate_for_ss = env->pstate;
>      } else {
>          *pc = env->regs[15];
> -        flags = rebuild_hflags_a32(env, current_el);
>          flags = FIELD_DP32(flags, TBFLAG_A32, THUMB, env->thumb);
>          flags = FIELD_DP32(flags, TBFLAG_A32, CONDEXEC, env->condexec_bits);
>          pstate_for_ss = env->uncached_cpsr;


--
Alex Bennée

Re: [Qemu-devel] [PATCH v2 0/3] target/arm: Reduce overhead of cpu_get_tb_cpu_state

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190219233421.388-1-richard.henderson@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20190219233421.388-1-richard.henderson@linaro.org
Subject: [Qemu-devel] [PATCH v2 0/3] target/arm: Reduce overhead of cpu_get_tb_cpu_state
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/20190219233421.388-1-richard.henderson@linaro.org -> patchew/20190219233421.388-1-richard.henderson@linaro.org
Switched to a new branch 'test'
fdca6b67d3 target/arm: Rely on hflags correct in cpu_get_tb_cpu_state
8615480629 target/arm: Rebuild hflags at el changes and MSR writes
0d2a83ccfd target/arm: Split out recompute_hflags et al

=== OUTPUT BEGIN ===
1/3 Checking commit 0d2a83ccfd5e (target/arm: Split out recompute_hflags et al)
WARNING: Block comments use a leading /* on a separate line
#277: FILE: target/arm/helper.c:13972:
+        /* If SVE is disabled, but FP is enabled,

WARNING: Block comments use a leading /* on a separate line
#358: FILE: target/arm/helper.c:14053:
+    /* The SS_ACTIVE and PSTATE_SS bits correspond to the state machine

total: 0 errors, 2 warnings, 363 lines checked

Patch 1/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/3 Checking commit 86154806299c (target/arm: Rebuild hflags at el changes and MSR writes)
3/3 Checking commit fdca6b67d324 (target/arm: Rely on hflags correct in cpu_get_tb_cpu_state)
ERROR: Use g_assert or g_assert_not_reached
#73: FILE: target/arm/helper.c:14049:
+        g_assert_cmphex(flags, ==, check_flags);

total: 1 errors, 0 warnings, 34 lines checked

Patch 3/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190219233421.388-1-richard.henderson@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH v2 0/3] target/arm: Reduce overhead of cpu_get_tb_cpu_state

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190219233421.388-1-richard.henderson@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20190219233421.388-1-richard.henderson@linaro.org
Subject: [Qemu-devel] [PATCH v2 0/3] target/arm: Reduce overhead of cpu_get_tb_cpu_state
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20190219233421.388-1-richard.henderson@linaro.org -> patchew/20190219233421.388-1-richard.henderson@linaro.org
Submodule 'capstone' (https://git.qemu.org/git/capstone.git) registered for path 'capstone'
Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc'
Submodule 'roms/QemuMacDrivers' (https://git.qemu.org/git/QemuMacDrivers.git) registered for path 'roms/QemuMacDrivers'
Submodule 'roms/SLOF' (https://git.qemu.org/git/SLOF.git) registered for path 'roms/SLOF'
Submodule 'roms/ipxe' (https://git.qemu.org/git/ipxe.git) registered for path 'roms/ipxe'
Submodule 'roms/openbios' (https://git.qemu.org/git/openbios.git) registered for path 'roms/openbios'
Submodule 'roms/openhackware' (https://git.qemu.org/git/openhackware.git) registered for path 'roms/openhackware'
Submodule 'roms/qemu-palcode' (https://git.qemu.org/git/qemu-palcode.git) registered for path 'roms/qemu-palcode'
Submodule 'roms/seabios' (https://git.qemu.org/git/seabios.git/) registered for path 'roms/seabios'
Submodule 'roms/seabios-hppa' (https://github.com/hdeller/seabios-hppa.git) registered for path 'roms/seabios-hppa'
Submodule 'roms/sgabios' (https://git.qemu.org/git/sgabios.git) registered for path 'roms/sgabios'
Submodule 'roms/skiboot' (https://git.qemu.org/git/skiboot.git) registered for path 'roms/skiboot'
Submodule 'roms/u-boot' (https://git.qemu.org/git/u-boot.git) registered for path 'roms/u-boot'
Submodule 'roms/u-boot-sam460ex' (https://git.qemu.org/git/u-boot-sam460ex.git) registered for path 'roms/u-boot-sam460ex'
Submodule 'tests/fp/berkeley-softfloat-3' (https://github.com/cota/berkeley-softfloat-3) registered for path 'tests/fp/berkeley-softfloat-3'
Submodule 'tests/fp/berkeley-testfloat-3' (https://github.com/cota/berkeley-testfloat-3) registered for path 'tests/fp/berkeley-testfloat-3'
Submodule 'ui/keycodemapdb' (https://git.qemu.org/git/keycodemapdb.git) registered for path 'ui/keycodemapdb'
Cloning into 'capstone'...
Submodule path 'capstone': checked out '22ead3e0bfdb87516656453336160e0a37b066bf'
Cloning into 'dtc'...
Submodule path 'dtc': checked out '88f18909db731a627456f26d779445f84e449536'
Cloning into 'roms/QemuMacDrivers'...
Submodule path 'roms/QemuMacDrivers': checked out '90c488d5f4a407342247b9ea869df1c2d9c8e266'
Cloning into 'roms/SLOF'...
Submodule path 'roms/SLOF': checked out 'a5b428e1c1eae703bdd62a3f527223c291ee3fdc'
Cloning into 'roms/ipxe'...
Submodule path 'roms/ipxe': checked out 'de4565cbe76ea9f7913a01f331be3ee901bb6e17'
Cloning into 'roms/openbios'...
Submodule path 'roms/openbios': checked out '3464681b2b5983df80086a40179d324102347da3'
Cloning into 'roms/openhackware'...
Submodule path 'roms/openhackware': checked out 'c559da7c8eec5e45ef1f67978827af6f0b9546f5'
Cloning into 'roms/qemu-palcode'...
Submodule path 'roms/qemu-palcode': checked out '51c237d7e20d05100eacadee2f61abc17e6bc097'
Cloning into 'roms/seabios'...
Submodule path 'roms/seabios': checked out 'a698c8995ffb2838296ec284fe3c4ad33dfca307'
Cloning into 'roms/seabios-hppa'...
Submodule path 'roms/seabios-hppa': checked out '1ef99a01572c2581c30e16e6fe69e9ea2ef92ce0'
Cloning into 'roms/sgabios'...
Submodule path 'roms/sgabios': checked out 'cbaee52287e5f32373181cff50a00b6c4ac9015a'
Cloning into 'roms/skiboot'...
Submodule path 'roms/skiboot': checked out 'e0ee24c27a172bcf482f6f2bc905e6211c134bcc'
Cloning into 'roms/u-boot'...
Submodule path 'roms/u-boot': checked out 'd85ca029f257b53a96da6c2fb421e78a003a9943'
Cloning into 'roms/u-boot-sam460ex'...
Submodule path 'roms/u-boot-sam460ex': checked out '60b3916f33e617a815973c5a6df77055b2e3a588'
Cloning into 'tests/fp/berkeley-softfloat-3'...
Submodule path 'tests/fp/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037'
Cloning into 'tests/fp/berkeley-testfloat-3'...
Submodule path 'tests/fp/berkeley-testfloat-3': checked out '5a59dcec19327396a011a17fd924aed4fec416b3'
Cloning into 'ui/keycodemapdb'...
Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce'
Switched to a new branch 'test'
fdca6b6 target/arm: Rely on hflags correct in cpu_get_tb_cpu_state
8615480 target/arm: Rebuild hflags at el changes and MSR writes
0d2a83c target/arm: Split out recompute_hflags et al

=== OUTPUT BEGIN ===
1/3 Checking commit 0d2a83ccfd5e (target/arm: Split out recompute_hflags et al)
WARNING: Block comments use a leading /* on a separate line
#277: FILE: target/arm/helper.c:13972:
+        /* If SVE is disabled, but FP is enabled,

WARNING: Block comments use a leading /* on a separate line
#358: FILE: target/arm/helper.c:14053:
+    /* The SS_ACTIVE and PSTATE_SS bits correspond to the state machine

total: 0 errors, 2 warnings, 363 lines checked

Patch 1/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/3 Checking commit 86154806299c (target/arm: Rebuild hflags at el changes and MSR writes)
3/3 Checking commit fdca6b67d324 (target/arm: Rely on hflags correct in cpu_get_tb_cpu_state)
ERROR: Use g_assert or g_assert_not_reached
#73: FILE: target/arm/helper.c:14049:
+        g_assert_cmphex(flags, ==, check_flags);

total: 1 errors, 0 warnings, 34 lines checked

Patch 3/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190219233421.388-1-richard.henderson@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH v2 0/3] target/arm: Reduce overhead of cpu_get_tb_cpu_state

[Emilio G. Cota]

On Tue, Feb 19, 2019 at 15:34:18 -0800, Richard Henderson wrote:
> Changes since v1:
>   * Apparently I had started a last-minute API change, and failed to
>     covert all of the users, and also failed to re-test afterward.
>   * Retain assertions for --enable-debug-tcg.

This brings my arm-softmmu bootup+shutdown test to an early death:

[...]
VFS: Mounted root (ext4 filesystem) readonly on device 254:1.
devtmpfs: mounted
Freeing unused kernel memory: 300K (80669000 - 806b4000)
BUG: unsupported FP instruction in kernel mode
Internal error: Oops - undefined instruction: 0 [#1] SMP ARM
Modules linked in:
CPU: 0 PID: 1 Comm: init Not tainted 4.5.0-ajb #10
Hardware name: Generic DT based system
task: eec58000 ti: eec52000 task.ti: eec52000
PC is at vfp_reload_hw+0xc/0x44
LR is at __und_usr_fault_32+0x0/0x8
pc : [<8000ab94>]    lr : [<800136c0>]    psr: 000c0013
sp : eec53fb0  ip : 7eb88918  fp : 00000000
r10: eec520f8  r9 : 8001371c  r8 : 00000b00
r7 : 00000001  r6 : eec5204c  r5 : 40000000  r4 : 00000000
r3 : 806e1058  r2 : 76fc1362  r1 : 40000000  r0 : ecac8b10
Flags: nzcv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
Control: 30c5387d  Table: ae4048c0  DAC: fffffffd
Process init (pid: 1, stack limit = 0xeec52210)
Stack: (0xeec53fb0 to 0xeec54000)
3fa0:                                     7eb888f0 00000000 003fb0d6 fffffd90
3fc0: 7eb888e0 76fd8050 00000000 7eb88ab0 00000000 0001223c 76fd8958 7eb88a90
3fe0: 7eb88918 7eb888c0 76fbb187 76fc1362 600c0030 ffffffff 00000000 00000000
[<8000ab94>] (vfp_reload_hw) from [<800136c0>] (__und_usr_fault_32+0x0/0x8)
Code: 0a000010 e58ab110 eee85a10 e783a10b (ecba0b20)
---[ end trace 26acd422f5b3785f ]---
Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b

Thanks,

		Emilio

Re: [Qemu-devel] [PATCH v2 0/3] target/arm: Reduce overhead of cpu_get_tb_cpu_state

[Alex Bennée]

Emilio G. Cota <cota@braap.org> writes:

> On Tue, Feb 19, 2019 at 15:34:18 -0800, Richard Henderson wrote:
>> Changes since v1:
>>   * Apparently I had started a last-minute API change, and failed to
>>     covert all of the users, and also failed to re-test afterward.
>>   * Retain assertions for --enable-debug-tcg.
>
> This brings my arm-softmmu bootup+shutdown test to an early death:

Can you retry with --enable-tcg-debug?

>
> [...]
> VFS: Mounted root (ext4 filesystem) readonly on device 254:1.
> devtmpfs: mounted
> Freeing unused kernel memory: 300K (80669000 - 806b4000)
> BUG: unsupported FP instruction in kernel mode
> Internal error: Oops - undefined instruction: 0 [#1] SMP ARM
> Modules linked in:
> CPU: 0 PID: 1 Comm: init Not tainted 4.5.0-ajb #10
> Hardware name: Generic DT based system
> task: eec58000 ti: eec52000 task.ti: eec52000
> PC is at vfp_reload_hw+0xc/0x44
> LR is at __und_usr_fault_32+0x0/0x8
> pc : [<8000ab94>]    lr : [<800136c0>]    psr: 000c0013
> sp : eec53fb0  ip : 7eb88918  fp : 00000000
> r10: eec520f8  r9 : 8001371c  r8 : 00000b00
> r7 : 00000001  r6 : eec5204c  r5 : 40000000  r4 : 00000000
> r3 : 806e1058  r2 : 76fc1362  r1 : 40000000  r0 : ecac8b10
> Flags: nzcv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
> Control: 30c5387d  Table: ae4048c0  DAC: fffffffd
> Process init (pid: 1, stack limit = 0xeec52210)
> Stack: (0xeec53fb0 to 0xeec54000)
> 3fa0:                                     7eb888f0 00000000 003fb0d6 fffffd90
> 3fc0: 7eb888e0 76fd8050 00000000 7eb88ab0 00000000 0001223c 76fd8958 7eb88a90
> 3fe0: 7eb88918 7eb888c0 76fbb187 76fc1362 600c0030 ffffffff 00000000 00000000
> [<8000ab94>] (vfp_reload_hw) from [<800136c0>] (__und_usr_fault_32+0x0/0x8)
> Code: 0a000010 e58ab110 eee85a10 e783a10b (ecba0b20)
> ---[ end trace 26acd422f5b3785f ]---
> Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
> ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
>
> Thanks,
>
> 		Emilio


--
Alex Bennée

Re: [Qemu-devel] [PATCH v2 0/3] target/arm: Reduce overhead of cpu_get_tb_cpu_state

[Emilio G. Cota]

On Thu, Feb 21, 2019 at 22:36:25 +0000, Alex Bennée wrote:
> Emilio G. Cota <cota@braap.org> writes:
> > This brings my arm-softmmu bootup+shutdown test to an early death:
> 
> Can you retry with --enable-tcg-debug?

Should have done so the first time. Here it is:

(gdb) r                                          
Starting program: /data/src/qemu/build/arm-softmmu/qemu-system-arm -machine type=virt -nographic -m 4096 -netdev user,id=unet,hostfwd=tcp::2222-:22 -d
evice virtio-net-device,netdev=unet -drive file=../img/arm/jessie-arm32-die-on-boot.qcow2,id=myblock,index=0,if=none -device virtio-blk-device,drive=m
yblock -kernel ../img/arm/aarch32-current-linux-kernel-only.img -append console=ttyAMA0\ root=/dev/vda1 -name arm,debug-threads=on -smp 1
[...]
VFS: Mounted root (ext4 filesystem) readonly on device 254:1.
devtmpfs: mounted
Freeing unused kernel memory: 300K (80669000 - 806b4000)
**
ERROR:/data/src/qemu/target/arm/helper.c:14049:cpu_get_tb_cpu_state: assertion failed (flags == check_flags): (0x10080000 == 0x10080080)

Thread 4 "CPU 0/TCG" received signal SIGABRT, Aborted.
[Switching to Thread 0x7fffee2aa700 (LWP 14033)]
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
51      ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007ffff4585801 in __GI_abort () at abort.c:79
#2  0x00007ffff570f2a5 in g_assertion_message () from /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
#3  0x00007ffff570f652 in g_assertion_message_cmpnum () from /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
#4  0x00005555559a339b in cpu_get_tb_cpu_state (env=0x5555565af060, pc=pc@entry=0x7fffee2a98fc, cs_base=cs_base@entry=0x7fffee2a98f8,
    pflags=pflags@entry=0x7fffee2a9900) at /data/src/qemu/target/arm/helper.c:14049
#5  0x000055555588fbdb in tb_lookup__cpu_state (cf_mask=524288, flags=0x7fffee2a9900, cs_base=0x7fffee2a98f8, pc=0x7fffee2a98fc, cpu=0x0)
    at /data/src/qemu/include/exec/tb-lookup.h:28
#6  tb_find (cf_mask=524288, tb_exit=0, last_tb=0x0, cpu=0x0) at /data/src/qemu/accel/tcg/cpu-exec.c:404
#7  cpu_exec (cpu=cpu@entry=0x5555565a6db0) at /data/src/qemu/accel/tcg/cpu-exec.c:728
#8  0x000055555584e49f in tcg_cpu_exec (cpu=0x5555565a6db0) at /data/src/qemu/cpus.c:1429
#9  0x0000555555850623 in qemu_tcg_cpu_thread_fn (arg=arg@entry=0x5555565a6db0) at /data/src/qemu/cpus.c:1733
#10 0x0000555555c83416 in qemu_thread_start (args=<optimized out>) at /data/src/qemu/util/qemu-thread-posix.c:502
#11 0x00007ffff493d6db in start_thread (arg=0x7fffee2aa700) at pthread_create.c:463
#12 0x00007ffff466688f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
(gdb)

Thanks,
		Emilio

Re: [Qemu-devel] [PATCH v2 0/3] target/arm: Reduce overhead of cpu_get_tb_cpu_state

[Richard Henderson]

On 2/21/19 2:59 PM, Emilio G. Cota wrote:
> Should have done so the first time. Here it is:
> 
> (gdb) r                                          
> Starting program: /data/src/qemu/build/arm-softmmu/qemu-system-arm -machine type=virt -nographic -m 4096 -netdev user,id=unet,hostfwd=tcp::2222-:22 -d
> evice virtio-net-device,netdev=unet -drive file=../img/arm/jessie-arm32-die-on-boot.qcow2,id=myblock,index=0,if=none -device virtio-blk-device,drive=m
> yblock -kernel ../img/arm/aarch32-current-linux-kernel-only.img -append console=ttyAMA0\ root=/dev/vda1 -name arm,debug-threads=on -smp 1
> [...]
> VFS: Mounted root (ext4 filesystem) readonly on device 254:1.
> devtmpfs: mounted
> Freeing unused kernel memory: 300K (80669000 - 806b4000)
> **
> ERROR:/data/src/qemu/target/arm/helper.c:14049:cpu_get_tb_cpu_state: assertion failed (flags == check_flags): (0x10080000 == 0x10080080)

Thanks, I've now reproduced this.


r~

Re: [Qemu-devel] [PATCH v2 0/3] target/arm: Reduce overhead of cpu_get_tb_cpu_state

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190219233421.388-1-richard.henderson@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20190219233421.388-1-richard.henderson@linaro.org
Subject: [Qemu-devel] [PATCH v2 0/3] target/arm: Reduce overhead of cpu_get_tb_cpu_state
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
f6e7ff3298 target/arm: Rely on hflags correct in cpu_get_tb_cpu_state
2ccfa3184b target/arm: Rebuild hflags at el changes and MSR writes
eed68828e4 target/arm: Split out recompute_hflags et al

=== OUTPUT BEGIN ===
1/3 Checking commit eed68828e48e (target/arm: Split out recompute_hflags et al)
WARNING: Block comments use a leading /* on a separate line
#277: FILE: target/arm/helper.c:12912:
+        /* If SVE is disabled, but FP is enabled,

WARNING: Block comments use a leading /* on a separate line
#358: FILE: target/arm/helper.c:12993:
+    /* The SS_ACTIVE and PSTATE_SS bits correspond to the state machine

total: 0 errors, 2 warnings, 363 lines checked

Patch 1/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/3 Checking commit 2ccfa3184bd5 (target/arm: Rebuild hflags at el changes and MSR writes)
3/3 Checking commit f6e7ff32983e (target/arm: Rely on hflags correct in cpu_get_tb_cpu_state)
ERROR: Use g_assert or g_assert_not_reached
#75: FILE: target/arm/helper.c:12989:
+        g_assert_cmphex(flags, ==, check_flags);

total: 1 errors, 0 warnings, 34 lines checked

Patch 3/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190219233421.388-1-richard.henderson@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com