[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 06/10] target/arm: Use arm_hcr_el2_eff more p
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH v2 06/10] target/arm: Use arm_hcr_el2_eff more places |
Date: |
Thu, 6 Dec 2018 13:06:04 +0000 |
On Mon, 3 Dec 2018 at 20:38, Richard Henderson
<address@hidden> 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 <address@hidden>
> ---
> @@ -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 <address@hidden>
thanks
-- PMM