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

[Peter Maydell]

On 9 September 2015 at 16:10, Sergey Sorokin <address@hidden> wrote:
> 08.09.2015, 16:44, "Peter Maydell" <address@hidden>:
>>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.

Assertions are nice when they're easy, but we don't have to
necessarily make the code more complex purely in order to try
to provide a place to assert. In practice I would expect that
pretty much all 64-bit CPUs we emulate will implement both
EL2 and EL3 anyway.

>>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.

If you can show that the cacheing is worth having (by showing
an actual speedup on some workload), then that's fine. But
it does complicate the code, so I'd prefer not to unless we
have some evidence for the benefit side of things. Gut feeling
about "I think this will be faster" is often inaccurate.

>>>  /* 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.

Yeah, I see why you've done it. But it's only "more intelligible"
if you can match the code up with what the spec requires...
As I say, I think it's correct, it's just not very obvious.

thanks
-- PMM