Re: [Qemu-devel] [PATCH] target-arm: Avoid calling arm_el_is_aa64() function for unimplemented EL

[Sergey Sorokin]

That is ok.

06.10.2015, 00:55, "Peter Maydell" <address@hidden>:
> On 2 October 2015 at 14:21, Sergey Sorokin <address@hidden> wrote:
>>  It is incorrect to call arm_el_is_aa64() function for unimplemented EL.
>>  This patch fixes several attempts to do so.
>>
>>  Signed-off-by: Sergey Sorokin <address@hidden>
>>  ---
>>   target-arm/cpu.h | 8 +++++---
>>   target-arm/helper.c | 15 +++++++++++++--
>>   2 files changed, 18 insertions(+), 5 deletions(-)
>>
>>  diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>>  index cc1578c..df456a5 100644
>>  --- a/target-arm/cpu.h
>>  +++ b/target-arm/cpu.h
>>  @@ -1015,11 +1015,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);
>>  @@ -1586,7 +1586,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. */
>>  + if (arm_feature(env, ARM_FEATURE_AARCH64) ||
>>  + ((scr || hcr) && (!secure))) {
>>               unmasked = 1;
>>           }
>>       }
>
> I know we discussed this one before, but having looked more carefully
> at it, I think the reason it's weird is that the original code is
> only correct if we're not implementing EL2.
>
> For instance (table D1-14 in the v8 ARMARM rev A.g)
> if we're in an all-AArch64 environment executing in Secure EL0
> then the interrupt mask is supposed to have an effect if the interrupt
> is targeting EL2. But the current code will always set 'unmasked' to 1 if
> EL3 is 64 bit.
>
> So I think what the code ought to read is:
>
>     if ((target_el > cur_el) && (target_el != 1)) {
>         /* Exceptions targeting a higher EL may not be maskable */
>         if (arm_feature(env, ARM_FEATURE_AARCH64)) {
>             /* 64-bit masking rules are simple: exceptions to EL3
>              * can't be masked, and exceptions to EL2 can only be
>              * masked from Secure state.
>              */
>             if (target_el == 3 || !secure) {
>                 unmasked = 1;
>             }
>         } else {
>             /* The old 32-bit-only environment has a more complicated
>              * masking setup.
>              */
>             if ((scr || hcr) && !secure) {
>                 unmasked = 1;
>             }
>         }
>     }
>
> Except that then for the AArch64 case we've just calculated
> scr and hcr and then not needed them. I think most of the
> code calculating them ought to move into the else clause here.
>
> I'll write a patch for this and post it tomorrow.
>
> In the meantime, we can just make the comment say
>
>    /* ARM_FEATURE_AARCH64 enabled means the highest EL is AArch64.
>     * This code currently assumes that EL2 is not implemented
>     * (and so that highest EL will be 3 and the target_el also 3).
>     */
>
>>  diff --git a/target-arm/helper.c b/target-arm/helper.c
>>  index 8367997..1f11dbd 100644
>>  --- a/target-arm/helper.c
>>  +++ b/target-arm/helper.c
>>  @@ -5220,11 +5220,22 @@ uint32_t arm_phys_excp_target_el(CPUState *cs, 
>> uint32_t excp_idx,
>>                                    uint32_t cur_el, bool secure)
>>   {
>>       CPUARMState *env = cs->env_ptr;
>>  - int rw = ((env->cp15.scr_el3 & SCR_RW) == SCR_RW);
>>  + int rw;
>>       int scr;
>>       int hcr;
>>       int target_el;
>>  - int is64 = arm_el_is_aa64(env, 3);
>>  + /* Is the higher EL AArch64? */
>
> "highest". I pointed this one out before...
>
>>  + int is64 = arm_feature(env, ARM_FEATURE_AARCH64);
>>  +
>>  + /* If the highest EL is in AArch64 state, and EL3 is not implemented,
>>  + * we must behave as if EL3 is implemented and is in AArch64 state.
>>  + * Therefore we need appropriate RW bit.
>>  + */
>
> I think we could put this comment into the else branch, and
> rephrase it a bit:
>
> + if (arm_feature(env, ARM_FEATURE_EL3)) {
> + rw = ((env->cp15.scr_el3 & SCR_RW) == SCR_RW);
> + } else {
> + /* Either EL2 is the highest EL (and so the EL2 register width
> + * is given by is64); or there is no EL2 or EL3, in which case
> + * the value of 'rw' does not affect the table lookup anyway.
> + */
> + rw = is64;
> + }
>
>>  + if (arm_feature(env, ARM_FEATURE_EL3)) {
>>  + rw = ((env->cp15.scr_el3 & SCR_RW) == SCR_RW);
>>  + } else {
>>  + rw = is64;
>>  + }
>
> What I can do with this patch is:
>  * make the minor comment tweaks above
>  * apply the result to target-arm.next
>
> Is that OK?
>
>>       switch (excp_idx) {
>>       case EXCP_IRQ:
>>  --
>>  1.9.3
>
> thanks
> -- PMM