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

[Richard Henderson]

On 12/6/18 7:06 AM, Peter Maydell wrote:
> 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...

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~