qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] target-arm: Avoid calling arm_el_is_aa64() func


From: Peter Maydell
Subject: Re: [Qemu-devel] [PATCH] target-arm: Avoid calling arm_el_is_aa64() function for unimplemented EL
Date: Mon, 5 Oct 2015 22:55:31 +0100

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]