| Message ID | 20181203203839.757-7-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: LOR, HPD, AA32HPD | expand |
On Mon, 3 Dec 2018 at 20:38, Richard Henderson <richard.henderson@linaro.org> wrote: > > Since arm_hcr_el2_eff includes a check against > arm_is_secure_below_el3, we can often remove a > nearby check against secure state. > > In some cases, sort the call to arm_hcr_el2_eff > to the end of a short-circuit logical sequence. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > @@ -8797,6 +8795,8 @@ static inline uint32_t regime_sctlr(CPUARMState *env, ARMMMUIdx mmu_idx) > static inline bool regime_translation_disabled(CPUARMState *env, > ARMMMUIdx mmu_idx) > { > + uint64_t hcr_el2; > + > if (arm_feature(env, ARM_FEATURE_M)) { > switch (env->v7m.mpu_ctrl[regime_is_secure(env, mmu_idx)] & > (R_V7M_MPU_CTRL_ENABLE_MASK | R_V7M_MPU_CTRL_HFNMIENA_MASK)) { > @@ -8815,19 +8815,21 @@ static inline bool regime_translation_disabled(CPUARMState *env, > } > } > > + hcr_el2 = arm_hcr_el2_eff(env); Changing this in this function makes me nervous, because arm_hcr_el2_eff() looks at the current CPU state via arm_is_secure_below_el3() rather than at the security state of the translation regime we're asking about. I think it probably doesn't change behaviour, but I'm finding it hard to reason about... > + > if (mmu_idx == ARMMMUIdx_S2NS) { > /* HCR.DC means HCR.VM behaves as 1 */ > - return (env->cp15.hcr_el2 & (HCR_DC | HCR_VM)) == 0; > + return (hcr_el2 & (HCR_DC | HCR_VM)) == 0; > } > > - if (env->cp15.hcr_el2 & HCR_TGE) { > + if (hcr_el2 & HCR_TGE) { > /* TGE means that NS EL0/1 act as if SCTLR_EL1.M is zero */ > if (!regime_is_secure(env, mmu_idx) && regime_el(env, mmu_idx) == 1) { > return true; > } > } > > - if ((env->cp15.hcr_el2 & HCR_DC) && > + if ((hcr_el2 & HCR_DC) && > (mmu_idx == ARMMMUIdx_S1NSE0 || mmu_idx == ARMMMUIdx_S1NSE1)) { > /* HCR.DC means SCTLR_EL1.M behaves as 0 */ > return true; > @@ -995,7 +994,7 @@ void HELPER(pre_smc)(CPUARMState *env, uint32_t syndrome) > exception_target_el(env)); > } > > - if (!secure && cur_el == 1 && (env->cp15.hcr_el2 & HCR_TSC)) { > + if (!secure && cur_el == 1 && (arm_hcr_el2_eff(env) & HCR_TSC)) { Why can't we drop the !secure check here? > /* In NS EL1, HCR controlled routing to EL2 has priority over SMD. > * We also want an EL2 guest to be able to forbid its EL1 from > * making PSCI calls into QEMU's "firmware" via HCR.TSC. > @@ -1098,8 +1097,7 @@ void HELPER(exception_return)(CPUARMState *env) > goto illegal_return; > } > > - if (new_el == 1 && (env->cp15.hcr_el2 & HCR_TGE) > - && !arm_is_secure_below_el3(env)) { > + if (new_el == 1 && (arm_hcr_el2_eff(env) & HCR_TGE)) { > goto illegal_return; > } Otherwise Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On 12/6/18 7:06 AM, Peter Maydell wrote: > On Mon, 3 Dec 2018 at 20:38, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> Since arm_hcr_el2_eff includes a check against >> arm_is_secure_below_el3, we can often remove a >> nearby check against secure state. >> >> In some cases, sort the call to arm_hcr_el2_eff >> to the end of a short-circuit logical sequence. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- > > >> @@ -8797,6 +8795,8 @@ static inline uint32_t regime_sctlr(CPUARMState *env, ARMMMUIdx mmu_idx) >> static inline bool regime_translation_disabled(CPUARMState *env, >> ARMMMUIdx mmu_idx) >> { >> + uint64_t hcr_el2; >> + >> if (arm_feature(env, ARM_FEATURE_M)) { >> switch (env->v7m.mpu_ctrl[regime_is_secure(env, mmu_idx)] & >> (R_V7M_MPU_CTRL_ENABLE_MASK | R_V7M_MPU_CTRL_HFNMIENA_MASK)) { >> @@ -8815,19 +8815,21 @@ static inline bool regime_translation_disabled(CPUARMState *env, >> } >> } >> >> + hcr_el2 = arm_hcr_el2_eff(env); > > Changing this in this function makes me nervous, because > arm_hcr_el2_eff() looks at the current CPU state via > arm_is_secure_below_el3() rather than at the security state > of the translation regime we're asking about. I think > it probably doesn't change behaviour, but I'm finding > it hard to reason about... Oops, I missed that we weren't talking about "current" here. I can either just revert this function for now, or introduce a new arm_hcr_el2_eff_ns that does not take secure into effect, but does adjust all of the other flags within HCR relative to its own contents. >> - if (!secure && cur_el == 1 && (env->cp15.hcr_el2 & HCR_TSC)) { >> + if (!secure && cur_el == 1 && (arm_hcr_el2_eff(env) & HCR_TSC)) { > > Why can't we drop the !secure check here? Either I missed it, or it was premature optimization of the form "well, it's already computed into a local variable..." r~
On Thu, 6 Dec 2018 at 13:32, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 12/6/18 7:06 AM, Peter Maydell wrote: > > Changing this in this function makes me nervous, because > > arm_hcr_el2_eff() looks at the current CPU state via > > arm_is_secure_below_el3() rather than at the security state > > of the translation regime we're asking about. I think > > it probably doesn't change behaviour, but I'm finding > > it hard to reason about... > > Oops, I missed that we weren't talking about "current" here. > > I can either just revert this function for now, or introduce a new > arm_hcr_el2_eff_ns that does not take secure into effect, but does adjust all > of the other flags within HCR relative to its own contents. I think I'd just not change this function. thanks -- PMM
diff --git a/target/arm/helper.c b/target/arm/helper.c index 5874c5a73f..b248dfcd39 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -448,7 +448,7 @@ static CPAccessResult access_tdosa(CPUARMState *env, const ARMCPRegInfo *ri, int el = arm_current_el(env); bool mdcr_el2_tdosa = (env->cp15.mdcr_el2 & MDCR_TDOSA) || (env->cp15.mdcr_el2 & MDCR_TDE) || - (env->cp15.hcr_el2 & HCR_TGE); + (arm_hcr_el2_eff(env) & HCR_TGE); if (el < 2 && mdcr_el2_tdosa && !arm_is_secure_below_el3(env)) { return CP_ACCESS_TRAP_EL2; @@ -468,7 +468,7 @@ static CPAccessResult access_tdra(CPUARMState *env, const ARMCPRegInfo *ri, int el = arm_current_el(env); bool mdcr_el2_tdra = (env->cp15.mdcr_el2 & MDCR_TDRA) || (env->cp15.mdcr_el2 & MDCR_TDE) || - (env->cp15.hcr_el2 & HCR_TGE); + (arm_hcr_el2_eff(env) & HCR_TGE); if (el < 2 && mdcr_el2_tdra && !arm_is_secure_below_el3(env)) { return CP_ACCESS_TRAP_EL2; @@ -488,7 +488,7 @@ static CPAccessResult access_tda(CPUARMState *env, const ARMCPRegInfo *ri, int el = arm_current_el(env); bool mdcr_el2_tda = (env->cp15.mdcr_el2 & MDCR_TDA) || (env->cp15.mdcr_el2 & MDCR_TDE) || - (env->cp15.hcr_el2 & HCR_TGE); + (arm_hcr_el2_eff(env) & HCR_TGE); if (el < 2 && mdcr_el2_tda && !arm_is_secure_below_el3(env)) { return CP_ACCESS_TRAP_EL2; @@ -4548,8 +4548,7 @@ int sve_exception_el(CPUARMState *env, int el) if (disabled) { /* route_to_el2 */ return (arm_feature(env, ARM_FEATURE_EL2) - && !arm_is_secure(env) - && (env->cp15.hcr_el2 & HCR_TGE) ? 2 : 1); + && (arm_hcr_el2_eff(env) & HCR_TGE) ? 2 : 1); } /* Check CPACR.FPEN. */ @@ -6194,9 +6193,8 @@ static int bad_mode_switch(CPUARMState *env, int mode, CPSRWriteType write_type) * and CPS are treated as illegal mode changes. */ if (write_type == CPSRWriteByInstr && - (env->cp15.hcr_el2 & HCR_TGE) && (env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_MON && - !arm_is_secure_below_el3(env)) { + (arm_hcr_el2_eff(env) & HCR_TGE)) { return 1; } return 0; @@ -8797,6 +8795,8 @@ static inline uint32_t regime_sctlr(CPUARMState *env, ARMMMUIdx mmu_idx) static inline bool regime_translation_disabled(CPUARMState *env, ARMMMUIdx mmu_idx) { + uint64_t hcr_el2; + if (arm_feature(env, ARM_FEATURE_M)) { switch (env->v7m.mpu_ctrl[regime_is_secure(env, mmu_idx)] & (R_V7M_MPU_CTRL_ENABLE_MASK | R_V7M_MPU_CTRL_HFNMIENA_MASK)) { @@ -8815,19 +8815,21 @@ static inline bool regime_translation_disabled(CPUARMState *env, } } + hcr_el2 = arm_hcr_el2_eff(env); + if (mmu_idx == ARMMMUIdx_S2NS) { /* HCR.DC means HCR.VM behaves as 1 */ - return (env->cp15.hcr_el2 & (HCR_DC | HCR_VM)) == 0; + return (hcr_el2 & (HCR_DC | HCR_VM)) == 0; } - if (env->cp15.hcr_el2 & HCR_TGE) { + if (hcr_el2 & HCR_TGE) { /* TGE means that NS EL0/1 act as if SCTLR_EL1.M is zero */ if (!regime_is_secure(env, mmu_idx) && regime_el(env, mmu_idx) == 1) { return true; } } - if ((env->cp15.hcr_el2 & HCR_DC) && + if ((hcr_el2 & HCR_DC) && (mmu_idx == ARMMMUIdx_S1NSE0 || mmu_idx == ARMMMUIdx_S1NSE1)) { /* HCR.DC means SCTLR_EL1.M behaves as 0 */ return true; diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c index 0d6e89e474..780caf387b 100644 --- a/target/arm/op_helper.c +++ b/target/arm/op_helper.c @@ -33,8 +33,7 @@ void raise_exception(CPUARMState *env, uint32_t excp, { CPUState *cs = CPU(arm_env_get_cpu(env)); - if ((env->cp15.hcr_el2 & HCR_TGE) && - target_el == 1 && !arm_is_secure(env)) { + if (target_el == 1 && (arm_hcr_el2_eff(env) & HCR_TGE)) { /* * Redirect NS EL1 exceptions to NS EL2. These are reported with * their original syndrome register value, with the exception of @@ -428,9 +427,9 @@ static inline int check_wfx_trap(CPUARMState *env, bool is_wfe) * No need for ARM_FEATURE check as if HCR_EL2 doesn't exist the * bits will be zero indicating no trap. */ - if (cur_el < 2 && !arm_is_secure(env)) { - mask = (is_wfe) ? HCR_TWE : HCR_TWI; - if (env->cp15.hcr_el2 & mask) { + if (cur_el < 2) { + mask = is_wfe ? HCR_TWE : HCR_TWI; + if (arm_hcr_el2_eff(env) & mask) { return 2; } } @@ -995,7 +994,7 @@ void HELPER(pre_smc)(CPUARMState *env, uint32_t syndrome) exception_target_el(env)); } - if (!secure && cur_el == 1 && (env->cp15.hcr_el2 & HCR_TSC)) { + if (!secure && cur_el == 1 && (arm_hcr_el2_eff(env) & HCR_TSC)) { /* In NS EL1, HCR controlled routing to EL2 has priority over SMD. * We also want an EL2 guest to be able to forbid its EL1 from * making PSCI calls into QEMU's "firmware" via HCR.TSC. @@ -1098,8 +1097,7 @@ void HELPER(exception_return)(CPUARMState *env) goto illegal_return; } - if (new_el == 1 && (env->cp15.hcr_el2 & HCR_TGE) - && !arm_is_secure_below_el3(env)) { + if (new_el == 1 && (arm_hcr_el2_eff(env) & HCR_TGE)) { goto illegal_return; }
Since arm_hcr_el2_eff includes a check against arm_is_secure_below_el3, we can often remove a nearby check against secure state. In some cases, sort the call to arm_hcr_el2_eff to the end of a short-circuit logical sequence. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/helper.c | 22 ++++++++++++---------- target/arm/op_helper.c | 14 ++++++-------- 2 files changed, 18 insertions(+), 18 deletions(-) -- 2.17.2