Re: [Qemu-devel] [PATCH v2 2/2] target-arm: Fix arm_el_is_aa64() function to support EL2 and EL3

[Sergey Sorokin]

08.09.2015, 16:44, "Peter Maydell" <address@hidden>:
>On 2 September 2015 at 16:39, Sergey Sorokin <address@hidden> wrote:
>> Function arm_el_is_aa64() was fixed to support EL2 and EL3.
>> It is needed for a future support of EL2 and/or EL3,
>> and 32 bit EL1/EL2 support for ARMv8 cpu.
>> ARM_FEATURE_AARCH64 flag means that the highest exception level is
>> in AArch64 state. The state of lower exception levels is controlled
>> by the HCR_EL2.RW, SCR_EL3.RW and SCR_EL3.NS bits.
>> If EL2 or EL3 is not permitted by the appropriate ARM_FEATURE flag,
>> then the function arm_el_is_aa64() aborts on an attempt to get
>> the bitness of this EL. So we need to be sure that EL passed to
>> the function is enabled. Therefore some conditional statements was
>> slightly changed to check it.
>>
>> Signed-off-by: Sergey Sorokin <address@hidden>
>> ---
>> Changes since previous version:
>> * Some typos was fixed.
>> * Extended comments was added.
>> * The initial patch was divided in two parts.
>> * The erroneous changes in arm_excp_unmasked() function was fixed.
>>
>>  hw/arm/boot.c        |  4 ++++
>>  target-arm/cpu.c     | 59 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  target-arm/cpu.h     | 51 ++++++++++++++++++++++++++++++++-------------
>>  target-arm/helper.c  | 23 +++++++++++++++++++-
>>  target-arm/machine.c |  5 +++++
>>  5 files changed, 126 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index 5b969cd..e5d00af 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -486,6 +486,10 @@ static void do_cpu_reset(void *opaque)
>>                  if (!info->secure_boot) {
>>                      /* Linux expects non-secure state */
>>                      env->cp15.scr_el3 |= SCR_NS;
>> +                    /* EL bitness can depend on SCR.NS bit, so we need
>> +                     * recalc it.
>> +                     */
>> +                    calc_el_bitness(env);
>>                  }
>>              }
>>
>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> index cc6c6f3..af4beab 100644
>> --- a/target-arm/cpu.c
>> +++ b/target-arm/cpu.c
>> @@ -100,6 +100,64 @@ static void cp_reg_check_reset(gpointer key, gpointer 
>> value,  gpointer opaque)
>>      assert(oldvalue == newvalue);
>>  }
>>
>> +/* This function must be called if SCR_EL3.RW, SCR_EL3.NS
>> + * or HCR_EL2.RW has been changed.
>> + */
>> +void calc_el_bitness(CPUARMState *env)
>> +{
>> +    /* -1 is invalid value */
>> +    env->el_is_aa64[0] = -1;
>> +
>> +    /* If the highest EL is not AArch64, then all ELs are AArch32. */
>> +    if (!arm_feature(env, ARM_FEATURE_AARCH64)) {
>> +        if (arm_feature(env, ARM_FEATURE_EL3)) {
>> +            env->el_is_aa64[3] = 0;
>> +        } else {
>> +            env->el_is_aa64[3] = -1;
>> +        }
>> +
>> +        if (arm_feature(env, ARM_FEATURE_EL2)) {
>> +            env->el_is_aa64[2] = 0;
>> +        } else {
>> +            env->el_is_aa64[2] = -1;
>> +        }
>> +
>> +        env->el_is_aa64[1] = 0;
>> +        return;
>> +    }
>> +
>> +    if (arm_feature(env, ARM_FEATURE_EL3)) {
>> +        env->el_is_aa64[3] = 1;
>> +        int8_t scr_rw = env->cp15.scr_el3 & SCR_RW ? 1 : 0;
>> +
>> +        if (arm_feature(env, ARM_FEATURE_EL2) && (env->cp15.scr_el3 & 
>> SCR_NS)) {
>> +            if (scr_rw) {
>> +                env->el_is_aa64[2] = 1;
>> +                env->el_is_aa64[1] = env->cp15.hcr_el2 & HCR_RW ? 1 : 0;
>> +            } else {
>> +                env->el_is_aa64[2] = 0;
>> +                env->el_is_aa64[1] = 0;
>> +            }
>> +        } else {
>> +            /* If there is no EL2 or if secure. */
>> +            env->el_is_aa64[2] = -1;
>> +            env->el_is_aa64[1] = scr_rw;
>> +        }
>> +    } else {
>> +        env->el_is_aa64[3] = -1;
>> +
>> +        if (arm_feature(env, ARM_FEATURE_EL2)) {
>> +            /* The highest EL is EL2, and it is AArch64. */
>> +            env->el_is_aa64[2] = 1;
>> +            env->el_is_aa64[1] = env->cp15.hcr_el2 & HCR_RW ? 1 : 0;
>> +        } else {
>> +            /* The highest EL is EL1, and it is AArch64. */
>> +            env->el_is_aa64[2] = -1;
>> +            env->el_is_aa64[1] = 1;
>> +        }
>> +    }
>> +}
>
>This is definitely rather overlarge for what it's doing. I think
>it's sufficient to write:
>
>
>void calc_el_bitness(CPUARMState *env)
>{
>    bool aa64 = arm_feature(env, ARM_FEATURE_AARCH64);
>
>    if (arm_feature(env, ARM_FEATURE_EL3)) {
>        env->el_is_aa64[3] = aa64;
>        aa64 = aa64 && (env->cp15.scr_el3 & SCR_RW);
>    } else {
>        env->el_is_aa64[3] = -1;
>    }
>
>    if (arm_feature(env, ARM_FEATURE_EL2) && !arm_is_secure_below_el3(env)) {
>        env->el_is_aa64[2] = aa64;
>        aa64 = aa64 && (env->cp15.hcr_el2 & HCR_RW);
>    } else {
>        env->el_is_aa64[2] = -1;
>    }
>
>    env->el_is_aa64[1] = aa64;
>}

I agree, this version looks better.

>
>And in fact this is pretty simple, so we could really just have
>
>static inline bool arm_el_is_aa64(CPUARMState *env, int el)
>{
>    /* This isn't valid for EL0 (if we're in EL0, is_a64() is what you want,
>     * and if we're not in EL0 then the state of EL0 isn't well defined.)
>     */
>    assert(el >= 1 && el <= 3);
>    bool aa64 = arm_feature(env, ARM_FEATURE_AARCH64);
>
>    if (el == 3) {
>        return aa64;
>    }
>
>    if (arm_feature(env, ARM_FEATURE_EL3)) {
>        aa64 = aa64 && (env->cp15.scr_el3 & SCR_RW);
>    }
>
>    if (el == 2) {
>        return aa64;
>    }
>
>    if (arm_feature(env, ARM_FEATURE_EL2) && !arm_is_secure_below_el3(env)) {
>        aa64 = aa64 && (env->cp15.hcr_el2 & HCR_RW);
>    }
>
>    return aa64;
>}

I supposed that the function should abort on attempt
to call it with unimplemented EL. Just to avoid bugs.
This your version does not abort in such case.
With the check it will not be so short.

>
>and not bother with the caching at all. I think I'd prefer that
>unless you have performance figures showing the caching makes
>a noticeable difference.

arm_el_is_aa64() is used (maybe indirectly) in address translations,
in do_interrupt() functions, some sysreg access,
cpu_get_tb_cpu_state () and arm_current_el() functions, etc.
And obviously it will be used more widely in future.
I think it's enough to show the caching makes sense.
Nevertheless, if you disagree with me, I could remove the caching
from the patch.

>>  /* Function for determing whether guest cp register reads and writes should
>> @@ -1012,11 +1031,11 @@ static inline bool access_secure_reg(CPUARMState 
>> *env)
>>   */
>>  #define A32_BANKED_CURRENT_REG_GET(_env, _regname)        \
>>      A32_BANKED_REG_GET((_env), _regname,                \
>> -                       ((!arm_el_is_aa64((_env), 3) && 
>> arm_is_secure(_env))))
>> +                       (arm_is_secure(_env) && !arm_el_is_aa64((_env), 3)))
>>
>>  #define A32_BANKED_CURRENT_REG_SET(_env, _regname, _val)                    
>>    \
>>      A32_BANKED_REG_SET((_env), _regname,                                    
>> \
>> -                       ((!arm_el_is_aa64((_env), 3) && 
>> arm_is_secure(_env))),  \
>> +                       (arm_is_secure(_env) && !arm_el_is_aa64((_env), 3)), 
>> \
>>                         (_val))
>>
>>  void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf);
>
>This hunk, and the two I've quoted below, are trying to avoid calling
>arm_el_is_aa64() for an unimplemented EL. I think it makes sense
>to do that, but they should be a different patch from the one
>that changes the implementation of arm_el_is_aa64().
>
>> @@ -1583,7 +1602,9 @@ static inline bool arm_excp_unmasked(CPUState *cs, 
>> unsigned int excp_idx,
>>       * interrupt.
>>       */
>>      if ((target_el > cur_el) && (target_el != 1)) {
>> -        if (arm_el_is_aa64(env, 3) || ((scr || hcr) && (!secure))) {
>> +        /* ARM_FEATURE_AARCH64 enabled means the higher EL is AArch64. */
>
>"highest"
>
>> +        if (arm_feature(env, ARM_FEATURE_AARCH64) ||
>> +            ((scr || hcr) && (!secure))) {
>>              unmasked = 1;
>>          }
>>      }
>
>I think this change is correct, though it took me a while to
>convince myself of it. (Roughly, I think that in the case where
>we have AArch64 EL2 but not EL3 then the (hcr && !secure) half
>of the condition is going to be true anyway, so it doesn't matter
>what the test on the left side of the || is.)
>

The change makes the check more intelligible in case of unimplemented EL3.
Moreover, arm_el_is_aa64() will abort with el argument == 3
if there is no EL3 in current configuration.

>> @@ -5088,7 +5104,8 @@ uint32_t arm_phys_excp_target_el(CPUState *cs, 
>> uint32_t excp_idx,
>>      int scr;
>>      int hcr;
>>      int target_el;
>> -    int is64 = arm_el_is_aa64(env, 3);
>> +    /* Is the higher EL AArch64? */
>
>"highest"
>
>> +    int is64 = arm_feature(env, ARM_FEATURE_AARCH64);
>>
>>      switch (excp_idx) {
>>      case EXCP_IRQ:
>
>I think this change is insufficient. If there is no EL3 but
>there is an EL2 and EL2 is AArch64, then we should route
>exceptions in the same way as if EL3 existed and was AArch64.
>For this to be the case we need to also get rw correct (ie
>reflecting the width of EL2). So something like:
>
>    if (arm_feature(env, ARM_FEATURE_EL3)) {
>        rw = ((env->cp15.scr_el3 & SCR_RW) == SCR_RW);
>    } else {
>        rw = is64;
>    }
>

Yes, I agree.

>> diff --git a/target-arm/machine.c b/target-arm/machine.c
>> index 32adfe7..e5bfb59 100644
>> --- a/target-arm/machine.c
>> +++ b/target-arm/machine.c
>> @@ -265,6 +265,11 @@ static int cpu_post_load(void *opaque, int version_id)
>>          }
>>      }
>>
>> +    /* It is not nesessary, because this call was made on
>> +     * HCR and SCR writes above, but just to make sure.
>> +     */
>> +    calc_el_bitness(&cpu->env);
>
>We should generally be confident about our code. Either
>this call *is* necessary, in which case the comment should
>say why, or it is *not* necessary, in which case it shouldn't
>be here (but you can have a comment saying why it isn't).
>In this case I think you are correct that it's not needed.
>(Some of the only-happens-on-value-change behaviour of register
>writes needs care in register loading, because you want
>the behaviour to happen anyway even if the old stale value
>in the CPU state happened to be the same as the new value
>from incoming migration data. But in this case we're OK
>because if the incoming migration data happens to be the
>same as the old data for all of the SCR and HCR bits then
>the old cached el_is_aa64[] values will all still be correct,
>even though we will never actually call calc_el_bitness().)
>

I have made this change acording to your last comment
on the first version of the patch. I will be glad to remove it :)