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

[Aggeler Fabian]

On 02 Jun 2014, at 18:02, Peter Maydell <address@hidden> wrote:

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

Thanks for catching this. I will correct it in v2.

> 
>> +        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;
>    }

Looks cleaner indeed, I will change it.

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

You’re right of course. I missed that. I will remove it completely.

Thanks,
Fabian

> 
> thanks
> -- PMM