diff mbox series

[v5,02/17] target/arm: Split out rebuild_hflags_a64

Message ID 20190820210720.18976-3-richard.henderson@linaro.org
State Superseded
Headers show
Series target/arm: Reduce overhead of cpu_get_tb_cpu_state | expand

Commit Message

Richard Henderson Aug. 20, 2019, 9:07 p.m. UTC 
Create a function to compute the values of the TBFLAG_A64 bits
that will be cached.  For now, the env->hflags variable is not
used, and the results are fed back to cpu_get_tb_cpu_state.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
 target/arm/helper.c | 131 +++++++++++++++++++++++---------------------
 1 file changed, 69 insertions(+), 62 deletions(-)

-- 
2.17.1

Comments

Alex Bennée Sept. 5, 2019, 3:28 p.m. UTC | #1 
Richard Henderson <richard.henderson@linaro.org> writes:

> Create a function to compute the values of the TBFLAG_A64 bits

> that will be cached.  For now, the env->hflags variable is not

> used, and the results are fed back to cpu_get_tb_cpu_state.

>

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> ---

>  target/arm/helper.c | 131 +++++++++++++++++++++++---------------------

>  1 file changed, 69 insertions(+), 62 deletions(-)

>

> diff --git a/target/arm/helper.c b/target/arm/helper.c

> index f2c6419369..02cb43cf58 100644

> --- a/target/arm/helper.c

> +++ b/target/arm/helper.c

> @@ -11032,6 +11032,71 @@ static uint32_t rebuild_hflags_common(CPUARMState *env, int fp_el,

>      return flags;

>  }

>

> +static uint32_t rebuild_hflags_a64(CPUARMState *env, int el, int fp_el,

> +                                   ARMMMUIdx mmu_idx)

> +{

<snip>
> +

> +    if (cpu_isar_feature(aa64_bti, env_archcpu(env))) {

> +        /* 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 rebuild_hflags_common(env, fp_el, mmu_idx, flags);

> +}

> +

>  void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,

>                            target_ulong *cs_base, uint32_t *pflags)

>  {

> @@ -11041,67 +11106,9 @@ void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,

>      uint32_t flags = 0;

>

>      if (is_a64(env)) {

<snip>
> -

> -        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 = rebuild_hflags_a64(env, current_el, fp_el, mmu_idx);

> +        if (cpu_isar_feature(aa64_bti, env_archcpu(env))) {

>              flags = FIELD_DP32(flags, TBFLAG_A64, BTYPE, env->btype);


It seems off to only hoist part of the BTI flag check into the helper,
was it just missed or is there a reason? If so it could probably do with
an additional comment.

>          }

>      } else {

> @@ -11121,9 +11128,9 @@ void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,

>              flags = FIELD_DP32(flags, TBFLAG_A32,

>                                 XSCALE_CPAR, env->cp15.c15_cpar);

>          }

> -    }

>

> -    flags = rebuild_hflags_common(env, fp_el, mmu_idx, flags);

> +        flags = rebuild_hflags_common(env, fp_el, mmu_idx, flags);

> +    }

>

>      /* The SS_ACTIVE and PSTATE_SS bits correspond to the state machine

>       * states defined in the ARM ARM for software singlestep:



--
Alex Bennée
Richard Henderson Sept. 6, 2019, 3:26 a.m. UTC | #2 
On 9/5/19 11:28 AM, Alex Bennée wrote:
>> -

>> -        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 = rebuild_hflags_a64(env, current_el, fp_el, mmu_idx);

>> +        if (cpu_isar_feature(aa64_bti, env_archcpu(env))) {

>>              flags = FIELD_DP32(flags, TBFLAG_A64, BTYPE, env->btype);

> 

> It seems off to only hoist part of the BTI flag check into the helper,

> was it just missed or is there a reason? If so it could probably do with

> an additional comment.


The part of the bti stuff that is hoisted is solely based on system registers.
 The BTYPE field is in PSTATE and is a very different kind of animal -- in
particular, it is not set by MSR.

But also, comments in cpu.h say which fields are (not) cached in hflags, and
BTYPE is so documented.

Is your proposed comment really helpful here going forward, or do you just
think it's weird reviewing this patch, since not all BTI is treated the same
after the patch?


r~
Alex Bennée Sept. 6, 2019, 3:52 p.m. UTC | #3 
Richard Henderson <richard.henderson@linaro.org> writes:

> On 9/5/19 11:28 AM, Alex Bennée wrote:

>>> -

>>> -        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 = rebuild_hflags_a64(env, current_el, fp_el, mmu_idx);

>>> +        if (cpu_isar_feature(aa64_bti, env_archcpu(env))) {

>>>              flags = FIELD_DP32(flags, TBFLAG_A64, BTYPE, env->btype);

>>

>> It seems off to only hoist part of the BTI flag check into the helper,

>> was it just missed or is there a reason? If so it could probably do with

>> an additional comment.

>

> The part of the bti stuff that is hoisted is solely based on system registers.

>  The BTYPE field is in PSTATE and is a very different kind of animal -- in

> particular, it is not set by MSR.

>

> But also, comments in cpu.h say which fields are (not) cached in hflags, and

> BTYPE is so documented.

>

> Is your proposed comment really helpful here going forward, or do you just

> think it's weird reviewing this patch, since not all BTI is treated the same

> after the patch?


It was just weird seeing the isar_feature test twice. A mention in the
commit "not all bti related flags will be cached so we have to test the
feature twice" or something like that will suffice.

>

>

> r~



--
Alex Bennée
diff mbox series

Patch

diff --git a/target/arm/helper.c b/target/arm/helper.c
index f2c6419369..02cb43cf58 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -11032,6 +11032,71 @@  static uint32_t rebuild_hflags_common(CPUARMState *env, int fp_el,
     return flags;
 }
 
+static uint32_t rebuild_hflags_a64(CPUARMState *env, int el, int fp_el,
+                                   ARMMMUIdx mmu_idx)
+{
+    ARMMMUIdx stage1 = stage_1_mmu_idx(mmu_idx);
+    ARMVAParameters p0 = aa64_va_parameters_both(env, 0, stage1);
+    uint32_t flags = 0;
+    uint64_t sctlr;
+    int tbii, tbid;
+
+    flags = FIELD_DP32(flags, TBFLAG_ANY, AARCH64_STATE, 1);
+
+    /* 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, env_archcpu(env))) {
+        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);
+    }
+
+    sctlr = arm_sctlr(env, el);
+
+    if (cpu_isar_feature(aa64_pauth, env_archcpu(env))) {
+        /*
+         * 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, env_archcpu(env))) {
+        /* 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 rebuild_hflags_common(env, fp_el, mmu_idx, flags);
+}
+
 void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
                           target_ulong *cs_base, uint32_t *pflags)
 {
@@ -11041,67 +11106,9 @@  void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
     uint32_t flags = 0;
 
     if (is_a64(env)) {
-        ARMCPU *cpu = env_archcpu(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);
-        }
-
-        sctlr = arm_sctlr(env, 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 = rebuild_hflags_a64(env, current_el, fp_el, mmu_idx);
+        if (cpu_isar_feature(aa64_bti, env_archcpu(env))) {
             flags = FIELD_DP32(flags, TBFLAG_A64, BTYPE, env->btype);
         }
     } else {
@@ -11121,9 +11128,9 @@  void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
             flags = FIELD_DP32(flags, TBFLAG_A32,
                                XSCALE_CPAR, env->cp15.c15_cpar);
         }
-    }
 
-    flags = rebuild_hflags_common(env, fp_el, mmu_idx, flags);
+        flags = rebuild_hflags_common(env, fp_el, mmu_idx, flags);
+    }
 
     /* The SS_ACTIVE and PSTATE_SS bits correspond to the state machine
      * states defined in the ARM ARM for software singlestep: