On 02 Jun 2014, at 18:02, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 30 May 2014 16:15, Fabian Aggeler <aggel...@ethz.ch> 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 <aggel...@ethz.ch> >> --- >> 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