On 12/6/18 7:06 AM, Peter Maydell wrote:
> On Mon, 3 Dec 2018 at 20:38, Richard Henderson
> <richard.hender...@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.hender...@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~
