Re: [Qemu-devel] [PATCH v5 05/33] target-arm: make arm_current_pl() return PL3

[Greg Bellows]

On 6 October 2014 10:34, Peter Maydell <address@hidden> wrote:

On 30 September 2014 22:49, Greg Bellows <address@hidden> wrote:
> From: Fabian Aggeler <address@hidden>
>
> Make arm_current_pl() return PL3 for secure PL1 and monitor mode.
> Increase MMU modes since mmu_index is directly infered from arm_
> current_pl(). Changes assertion in arm_el_is_aa64() to allow EL3.
>
> Signed-off-by: Fabian Aggeler <address@hidden>
> Signed-off-by: Greg Bellows <address@hidden>
> ---
>  target-arm/cpu.h | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 101d139..c000716 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -100,7 +100,7 @@ typedef uint32_t ARMReadCPFunc(void *opaque, int cp_info,
>
>  struct arm_boot_info;
>
> -#define NB_MMU_MODES 2
> +#define NB_MMU_MODES 4
>
>  /* We currently assume float and double are IEEE single and double
>     precision respectively.
> @@ -753,7 +753,6 @@ static inline int arm_feature(CPUARMState *env, int feature)
>      return (env->features & (1ULL << feature)) != 0;
>  }
>
> -

Stray whitespace change.

Fixed in early commit where an additional blank line was inadvertently added.

 

>  /* Return true if exception level below EL3 is in secure state */
>  static inline bool arm_is_secure_below_el3(CPUARMState *env)
>  {
> @@ -794,11 +793,12 @@ static inline bool arm_is_secure(CPUARMState *env)
>  /* Return true if the specified exception level is running in AArch64 state. */
>  static inline bool arm_el_is_aa64(CPUARMState *env, int el)
>  {
> -    /* We don't currently support EL2 or EL3, and this isn't valid for EL0
> +    /* We don't currently support EL2, and 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);
> +    assert(el == 1 || el == 3);
> +
>      /* AArch64-capable CPUs always run with EL1 in AArch64 mode. This
>       * is a QEMU-imposed simplification which we may wish to change later.
>       * If we in future support EL2 and/or EL3, then the state of lower

> @@ -990,9 +990,12 @@ static inline int arm_current_el(CPUARMState *env)
>
>      if ((env->uncached_cpsr & 0x1f) == ARM_CPU_MODE_USR) {
>          return 0;
> +    } else if (arm_is_secure(env)) {
> +        /* Secure PL1 and monitor mode are mapped to PL3 */
> +        return 3;

This isn't correct. Secure privileged !Mon AArch32 modes are only
EL3 if EL3 is AArch32. If EL3 is AArch64 then the !Mon AArch32
modes are EL1.

Yes, this was on my list of updates based on our discussion and I missed it.  Fixed in v6 as prescribed.

 

>      }
> -    /* We don't currently implement the Virtualization or TrustZone
> -     * extensions, so PL2 and PL3 don't exist for us.
> +    /* We currently do not implement the Virtualization extensions, so PL2 does
> +     * not exist for us.
>       */
>      return 1;

Now that we've added the complications for handling secure mode,
we might as well also have the trivial code for Hyp too. So
that means the function looks something like:

    if (env->aarch64) {
        return extract32(env->pstate, 2, 2);
    }

    switch (env->uncached_cpsr & CPSR_M) {
    case ARM_CPU_MODE_USR:
        return 0;
    case ARM_CPU_MODE_HYP:
        return 2;
    case ARM_CPU_MODE_MON:
        return 3;
    default:
        if (arm_is_secure(env) && !arm_el_is_aa64(env, 3)) {
            /* If EL3 is 32-bit then all secure privileged modes run in EL3 */
            return 3;
        }
        return 1;
    }

thanks
-- PMM