Re: [Qemu-devel] [PATCH] target-arm: implement PD0/PD1 bits for TTBCR

[Peter Maydell]

On 30 May 2014 16:15, Fabian Aggeler <address@hidden> wrote:
> Corrected handling of writes to TTBCR for ARMv8 (previously UNK/SBZP
> bits are not RES0) and ARMv7 (new bits PD0/PD1 for CPUs with Security
> Extensions). Extracting T0SZ/T1SZ now uses 3 bits in Aarch32 and 6 bits
> in Aarch64 as bits [5:3] are now RES0 when writing to Aarch32 TTBCR,
> and not guaranteed to be zero anymore.
>
> Bits PD0/PD1 are now respected in get_phys_addr_lpae() and
> get_phys_addr_v6/v5().
>
> Signed-off-by: Fabian Aggeler <address@hidden>
> ---
> Parts of this patch were previously part of the TZ patchset but
> were rewritten to include ARMv8 RES0 and PD0/PD1 handling.
>
>  target-arm/cpu.h    | 16 ++++++++++++
>  target-arm/helper.c | 70 
> +++++++++++++++++++++++++++++++++++++++++------------
>  2 files changed, 71 insertions(+), 15 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 17a1ddd..fc5771e 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -441,6 +441,22 @@ int arm_cpu_handle_mmu_fault(CPUState *cpu, vaddr 
> address, int rw,
>  /* Execution state bits.  MRS read as zero, MSR writes ignored.  */
>  #define CPSR_EXEC (CPSR_T | CPSR_IT | CPSR_J)
>
> +#define TTBCR_N      (7U << 0) /* TTBCR.EAE==0 */
> +#define TTBCR_T0SZ   (7U << 0) /* TTBCR.EAE==1 */
> +#define TTBCR_PD0    (1U << 4)
> +#define TTBCR_PD1    (1U << 5)
> +#define TTBCR_EPD0   (1U << 7)
> +#define TTBCR_IRGN0  (3U << 8)
> +#define TTBCR_ORGN0  (3U << 10)
> +#define TTBCR_SH0    (3U << 12)
> +#define TTBCR_T1SZ   (3U << 16)
> +#define TTBCR_A1     (1U << 22)
> +#define TTBCR_EPD1   (1U << 23)
> +#define TTBCR_IRGN1  (3U << 24)
> +#define TTBCR_ORGN1  (3U << 26)
> +#define TTBCR_SH1    (1U << 28)
> +#define TTBCR_EAE    (1U << 31)
> +
>  /* Bit definitions for ARMv8 SPSR (PSTATE) format.
>   * Only these are valid when in AArch64 mode; in
>   * AArch32 mode SPSRs are basically CPSR-format.
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 4e52145..10b965e 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -312,7 +312,7 @@ static inline bool extended_addresses_enabled(CPUARMState 
> *env)
>  {
>      return arm_el_is_aa64(env, 1)
>          || ((arm_feature(env, ARM_FEATURE_LPAE)
> -             && (env->cp15.c2_control & (1U << 31))));
> +             && (env->cp15.c2_control & TTBCR_EAE)));
>  }
>
>  static void dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t 
> value)
> @@ -1410,11 +1410,22 @@ static void vmsa_ttbcr_raw_write(CPUARMState *env, 
> const ARMCPRegInfo *ri,
>  {
>      int maskshift = extract32(value, 0, 3);
>
> -    if (arm_feature(env, ARM_FEATURE_LPAE) && (value & (1 << 31))) {
> -        value &= ~((7 << 19) | (3 << 14) | (0xf << 3));
> -    } else {
> -        value &= 7;
> +    if (!arm_feature(env, ARM_FEATURE_V8)){

Missing space before '{' (checkpatch finds this).

> +        if (arm_feature(env, ARM_FEATURE_LPAE) && (value & TTBCR_EAE)) {
> +            /* Pre ARMv8 bits [21:19], [15:14] and [6:3] are UNK/SBZP when
> +             * using Long-desciptor translation table format */
> +            value &= ~((7 << 19) | (3 << 14) | (0xf << 3));
> +        } else if (arm_feature(env, ARM_FEATURE_EL3)) {
> +            /* In an implementation that includes the Security Extensions
> +             * TTBCR has additional fields PD0 [4] and PD1 [5] for
> +             * Short-descriptor translation table format.
> +             */
> +            value &= TTBCR_PD1 | TTBCR_PD0 | TTBCR_N;
> +        } else {
> +            value &= TTBCR_N;
> +        }
>      }
> +
>      /* Note that we always calculate c2_mask and c2_base_mask, but
>       * they are only used for short-descriptor tables (ie if EAE is 0);
>       * for long-descriptor tables the TTBCR fields are used differently
> @@ -3670,15 +3681,18 @@ static inline int check_ap(CPUARMState *env, int ap, 
> int domain_prot,
>    }
>  }
>
> -static uint32_t get_level1_table_address(CPUARMState *env, uint32_t address)
> +static uint32_t get_level1_table_address(CPUARMState *env, uint32_t address,
> +                                         int *ttbr_id)
>  {
>      uint32_t table;
>
> -    if (address & env->cp15.c2_mask)
> +    if (address & env->cp15.c2_mask) {
>          table = env->cp15.ttbr1_el1 & 0xffffc000;
> -    else
> +        *ttbr_id = 1;
> +    } else {
>          table = env->cp15.ttbr0_el1 & env->cp15.c2_base_mask;
> -
> +        *ttbr_id = 0;
> +    }
>      table |= (address >> 18) & 0x3ffc;
>      return table;
>  }
> @@ -3691,6 +3705,7 @@ static int get_phys_addr_v5(CPUARMState *env, uint32_t 
> address, int access_type,
>      int code;
>      uint32_t table;
>      uint32_t desc;
> +    int ttbr_id;
>      int type;
>      int ap;
>      int domain;
> @@ -3699,7 +3714,14 @@ static int get_phys_addr_v5(CPUARMState *env, uint32_t 
> address, int access_type,
>
>      /* Pagetable walk.  */
>      /* Lookup l1 descriptor.  */
> -    table = get_level1_table_address(env, address);
> +    table = get_level1_table_address(env, address, &ttbr_id);
> +    if ((ttbr_id == 0 && (env->cp15.c2_control & TTBCR_PD0)) ||
> +        (ttbr_id == 1 && (env->cp15.c2_control & TTBCR_PD1))) {
> +        /* Section translation fault if page walk is disabled by one of the
> +         * PD0 or PD1 bits  */
> +        code = 5;
> +        goto do_fault;
> +    }
>      desc = ldl_phys(cs->as, table);
>      type = (desc & 3);
>      domain = (desc >> 5) & 0x0f;
> @@ -3789,6 +3811,7 @@ static int get_phys_addr_v6(CPUARMState *env, uint32_t 
> address, int access_type,
>      uint32_t desc;
>      uint32_t xn;
>      uint32_t pxn = 0;
> +    int ttbr_id;
>      int type;
>      int ap;
>      int domain = 0;
> @@ -3797,7 +3820,14 @@ static int get_phys_addr_v6(CPUARMState *env, uint32_t 
> address, int access_type,
>
>      /* Pagetable walk.  */
>      /* Lookup l1 descriptor.  */
> -    table = get_level1_table_address(env, address);
> +    table = get_level1_table_address(env, address, &ttbr_id);
> +    if ((ttbr_id == 0 && (env->cp15.c2_control & TTBCR_PD0)) ||
> +        (ttbr_id == 1 && (env->cp15.c2_control & TTBCR_PD1))) {
> +        /* Section translation fault if page walk is disabled by one of the
> +         * PD0 or PD1 bits  */
> +        code = 5;
> +        goto do_fault;
> +    }
>      desc = ldl_phys(cs->as, table);
>      type = (desc & 3);
>      if (type == 0 || (type == 3 && !arm_feature(env, ARM_FEATURE_PXN))) {

This is functionally right, but there's some repetition between
the two callers of get_level1_table_address() here. I think if
we make that function be:
bool get_level1_table_address(CPUARMState *env, uint32_t *table,
                              uint32_t address)

(return true and fill in *table if page walk allowed, else
 return false) then we can move the TTBCR bit checks into
get_level1_table_address(), and then the callers can just be
    if (!get_level1_table_address(env, &table, address)) {
        code = 5;
        goto do_fault;
    }

> @@ -3937,15 +3967,23 @@ static int get_phys_addr_lpae(CPUARMState *env, 
> target_ulong address,
>       * This is a Non-secure PL0/1 stage 1 translation, so controlled by
>       * TTBCR/TTBR0/TTBR1 in accordance with ARM ARM DDI0406C table B-32:
>       */
> -    uint32_t t0sz = extract32(env->cp15.c2_control, 0, 6);
> +    uint32_t t0sz = 0;
>      if (arm_el_is_aa64(env, 1)) {
> +        t0sz = extract32(env->cp15.c2_control, 0, 6);
>          t0sz = MIN(t0sz, 39);
>          t0sz = MAX(t0sz, 16);
> +    } else {
> +        /* TTBCR.T0SZ in Aarch32 only has 3 bits [2:0] and the others RES0 */
> +        extract32(env->cp15.c2_control, 0, 3);
>      }
> -    uint32_t t1sz = extract32(env->cp15.c2_control, 16, 6);
> +    uint32_t t1sz = 0;
>      if (arm_el_is_aa64(env, 1)) {
> +        t1sz = extract32(env->cp15.c2_control, 16, 6);
>          t1sz = MIN(t1sz, 39);
>          t1sz = MAX(t1sz, 16);
> +    } else {
> +        /* TTBCR.T1SZ in Aarch32 only has 3 bits [18:16] and the others RES0 
> */
> +        t1sz = extract32(env->cp15.c2_control, 16, 3);
>      }
>      if (t0sz && !extract64(address, va_size - t0sz, t0sz - tbi)) {
>          /* there is a ttbr0 region and we are in it (high bits all zero) */
> @@ -3974,7 +4012,8 @@ static int get_phys_addr_lpae(CPUARMState *env, 
> target_ulong address,
>       */
>      if (ttbr_select == 0) {
>          ttbr = env->cp15.ttbr0_el1;
> -        epd = extract32(env->cp15.c2_control, 7, 1);
> +        epd = extract32(env->cp15.c2_control, 7, 1) | /* EPD0 */
> +              extract32(env->cp15.c2_control, 4, 1);  /* PD0 */
>          tsz = t0sz;
>
>          tg = extract32(env->cp15.c2_control, 14, 2);
> @@ -3986,7 +4025,8 @@ static int get_phys_addr_lpae(CPUARMState *env, 
> target_ulong address,
>          }
>      } else {
>          ttbr = env->cp15.ttbr1_el1;
> -        epd = extract32(env->cp15.c2_control, 23, 1);
> +        epd = extract32(env->cp15.c2_control, 23, 1) | /* EPD1 */
> +              extract32(env->cp15.c2_control, 5, 1);   /* PD1 */
>          tsz = t1sz;
>
>          tg = extract32(env->cp15.c2_control, 30, 2);

I think all these changes to get_phys_addr_lpae() are
either unnecessary or actively wrong. We only enter this
function if TTBCR.EAE is 1 (or if we're in AArch64 and
interpreting c2_control as TCR_EL1), so it's never the
case that bits 4 and 5 are PD0/PD1. In particular for
AArch64 T0SZ is 5 bits and so bits 4 and 5 may be set
but this doesn't mean we should be failing the translation.
So since get_phys_addr_lpae() already honours the EPD0/EPD1
bits we don't need to change anything in it.

thanks
-- PMM