Re: [Qemu-devel] [PATCH v2 06/10] target/arm: Use arm_hcr_el2_eff more places

[Peter Maydell]

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