| Message ID | 20190820210720.18976-3-richard.henderson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm: Reduce overhead of cpu_get_tb_cpu_state | expand |
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
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~
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 --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:
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(-)