| Message ID | 7bcd69ddf339487403b6cd4bca81a66266a13ef2.1433180153.git.peter.crosthwaite@xilinx.com |
|---|---|
| State | New |
| Headers | show |
On 1 June 2015 at 19:04, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > Unified MPU only. Uses ARM architecture major revision to switch > between PMSAv5 and v7 when ARM_FEATURE_MPU is set. PMSA v6 remains > unsupported and is asserted against. > > The return code from get_phys_addr has to patched to handle the case > of a 0 FSR with error. Use -1 to indicate this condition. > > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > --- > target-arm/cpu.h | 1 + > target-arm/helper.c | 153 ++++++++++++++++++++++++++++++++++++++++++++++++---- > 2 files changed, 144 insertions(+), 10 deletions(-) > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index 9cb2e49..73e2619 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -559,6 +559,7 @@ void pmccntr_sync(CPUARMState *env); > #define SCTLR_DT (1U << 16) /* up to ??, RAO in v6 and v7 */ > #define SCTLR_nTWI (1U << 16) /* v8 onward */ > #define SCTLR_HA (1U << 17) > +#define SCTLR_BR (1U << 17) /* PMSA only */ > #define SCTLR_IT (1U << 18) /* up to ??, RAO in v6 and v7 */ > #define SCTLR_nTWE (1U << 18) /* v8 onward */ > #define SCTLR_WXN (1U << 19) > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 63859a4..09210d3 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -3323,13 +3323,13 @@ void register_cp_regs_for_features(ARMCPU *cpu) > define_one_arm_cp_reg(cpu, &rvbar); > } > if (arm_feature(env, ARM_FEATURE_MPU)) { > - /* These are the MPU registers prior to PMSAv6. Any new > - * PMSA core later than the ARM946 will require that we > - * implement the PMSAv6 or PMSAv7 registers, which are > - * completely different. > - */ > - assert(!arm_feature(env, ARM_FEATURE_V6)); > - define_arm_cp_regs(cpu, pmsav5_cp_reginfo); > + if (arm_feature(env, ARM_FEATURE_V6)) { > + assert(arm_feature(env, ARM_FEATURE_V7)); Could use a comment /* PMSAv6 is not implemented */ > + define_arm_cp_regs(cpu, vmsa_pmsa_cp_reginfo); It's a bit unfortunate that we've managed to end up with a cpreg array for "present in both VMSA and PMSA" but the code path doesn't then allow for calling the define_ function just once. Oh well. > + define_arm_cp_regs(cpu, pmsav7_cp_reginfo); > + } else { > + define_arm_cp_regs(cpu, pmsav5_cp_reginfo); > + } > } else { > define_arm_cp_regs(cpu, vmsa_pmsa_cp_reginfo); > define_arm_cp_regs(cpu, vmsa_cp_reginfo); > @@ -5720,6 +5720,130 @@ do_fault: > return (1 << 9) | (fault_type << 2) | level; > } > > +static inline void get_phys_addr_pmsav7_default(CPUARMState *env, > + ARMMMUIdx mmu_idx, > + int32_t address, int *prot) > +{ > + *prot = PAGE_READ | PAGE_WRITE; > + switch (address) { > + case 0xF0000000 ... 0xFFFFFFFF: > + if (regime_sctlr(env, mmu_idx) & SCTLR_V) { /* hivecs execing is ok */ > + *prot |= PAGE_EXEC; > + } > + break; > + case 0x00000000 ... 0x7FFFFFFF: > + *prot |= PAGE_EXEC; > + break; > + } > + > +} > + > +static int get_phys_addr_pmsav7(CPUARMState *env, uint32_t address, > + int access_type, ARMMMUIdx mmu_idx, > + hwaddr *phys_ptr, int *prot) > +{ > + int n; > + bool is_user = regime_is_user(env, mmu_idx); > + > + *phys_ptr = address; > + *prot = 0; > + > + if (regime_translation_disabled(env, mmu_idx)) { /* MPU disabled */ > + get_phys_addr_pmsav7_default(env, mmu_idx, address, prot); When will this be reached? get_phys_addr() has already caught the 'MPU disabled' case. > + } else { /* MPU enabled */ > + for (n = 15; n >= 0; n--) { /* region search */ > + uint32_t base = env->cp15.c6_drbar[n]; > + uint32_t rsize = extract32(env->cp15.c6_drsr[n], 1, 5) + 1; > + int snd; > + > + if (!(env->cp15.c6_drsr[n] & 0x1)) { > + continue; > + } > + if (rsize < 2) { > + qemu_log_mask(LOG_GUEST_ERROR, "DRSR.Rsize field can not be 0"); This case is UNPREDICTABLE so I would continue here (ie treat the region as disabled) rather than doing something potentially odd with an out of range value later. > + } Our effective minimum region size is 1K because that's the target pagesize setting. Should we enforce that (minimum region size is impdef so it would be architecturally ok) or will that break otherwise ok guests? Base address not aligned to the region size is also UNPREDICTABLE, incidentally. > + > + if (address < base || address > base - 1 + (1ull << rsize)) { > + continue; > + } > + > + if (rsize < 8) { /* no subregions for regions < 256 bytes */ > + break; > + } > + > + rsize -= 3; /* sub region size (power of 2) */ > + snd = (address - base) >> rsize & 0x7; > + if (env->cp15.c6_drsr[n] & 1 << (snd + 8)) { I think I would find these easier to read with more parens so I didn't have to look up & vs << precedence. > + continue; > + } > + break; > + } > + > + if (n == -1) { /* no hits */ > + if (is_user || !(regime_sctlr(env, mmu_idx) & SCTLR_BR)) { > + /* background fault */ > + return -1; > + } else { > + get_phys_addr_pmsav7_default(env, mmu_idx, address, prot); > + } > + } else { /* a MPU hit! */ > + uint32_t ap = extract32(env->cp15.c6_dracr[n], 8, 3); > + > + if (is_user) { /* User mode AP bit decoding */ > + switch (ap) { > + case 0: > + case 1: > + case 5: > + break; /* no access */ > + case 3: > + *prot |= PAGE_WRITE; > + /* fall through */ > + case 2: > + case 6: > + *prot |= PAGE_READ | PAGE_EXEC; > + break; > + default: > + qemu_log_mask(LOG_GUEST_ERROR, "Bad value for AP bits in " > + "DRACR"); Putting the line break after the ',' would let you avoid splitting the string. > + } > + } else { /* Priv. mode AP bits decoding */ > + switch (ap) { > + case 0: > + break; /* no access */ > + case 1: > + case 2: > + case 3: > + *prot |= PAGE_WRITE; > + /* fall through */ > + case 5: > + case 6: > + *prot |= PAGE_READ | PAGE_EXEC; > + break; > + default: > + qemu_log_mask(LOG_GUEST_ERROR, "Bad value for AP bits in " > + "DRACR"); > + } > + } > + > + /* execute never */ > + *prot &= env->cp15.c6_dracr[n] & 1 << 12 ? ~PAGE_EXEC : ~0; Break this down into something more readable, please. > + } > + } > + > + switch (access_type) { > + case 0: > + return *prot & PAGE_READ ? 0 : 0x00D; > + case 1: > + return *prot & PAGE_WRITE ? 0 : 0x00D; > + case 2: > + return *prot & PAGE_EXEC ? 0 : 0x00D; This is if (!(*prot & (1 << access_type))) { return 0xD; /* Permission fault */ } else { return 0; } isn't it? > + default: > + abort(); /* should be unreachable */ > + return 0; The usual way to write this is g_assert_not_reached(), and the return isn't needed. > + } > + > +} > + > static int get_phys_addr_pmsav5(CPUARMState *env, uint32_t address, > int access_type, ARMMMUIdx mmu_idx, > hwaddr *phys_ptr, int *prot) > @@ -5795,13 +5919,14 @@ static int get_phys_addr_pmsav5(CPUARMState *env, uint32_t address, > * MPU state on MPU based systems. > * > * Returns 0 if the translation was successful. Otherwise, phys_ptr, attrs, > - * prot and page_size may not be filled in, and the return value provides > + * prot and page_size may or may-not be filled in, and the return value provides No hyphen. > * information on why the translation aborted, in the format of a > * DFSR/IFSR fault register, with the following caveats: > * * we honour the short vs long DFSR format differences. > * * the WnR bit is never set (the caller must do this). > * * for MPU based systems we don't bother to return a full FSR format > * value. Is this bullet point still true? If it is, is that a bug we now need to fix? > + * * -1 return value indicates a 0 FSR. > * > * @env: CPUARMState > * @address: virtual address to get physical address for > @@ -5857,8 +5982,14 @@ static inline int get_phys_addr(CPUARMState *env, target_ulong address, > > if (arm_feature(env, ARM_FEATURE_MPU)) { > *page_size = TARGET_PAGE_SIZE; > - return get_phys_addr_pmsav5(env, address, access_type, mmu_idx, > - phys_ptr, prot); > + if (arm_feature(env, ARM_FEATURE_V6)) { > + assert(arm_feature(env, ARM_FEATURE_V7)); > + return get_phys_addr_pmsav7(env, address, access_type, mmu_idx, > + phys_ptr, prot); v7M will take this code path now (which is the right thing, it is closer to v7R than to the 946); did you cross check against the M profile spec to see if any of this is R-profile specific? (We don't actually implement the M profile MPU right now, but it would be nice to avoid leaving beartraps for whoever does.) > + } else { > + return get_phys_addr_pmsav5(env, address, access_type, mmu_idx, > + phys_ptr, prot); > + } > } > > if (regime_using_lpae_format(env, mmu_idx)) { > @@ -5897,6 +6028,8 @@ int arm_tlb_fill(CPUState *cs, vaddr address, > tlb_set_page_with_attrs(cs, address, phys_addr, attrs, > prot, mmu_idx, page_size); > return 0; > + } else if (ret == -1) { > + ret = 0; > } > > return ret; This isn't going to work, because tlb_fill() which calls this function making the same "0 means OK, non-0 means FSR value" assumption. I think the best thing to do here is to switch get_phys_addr() and friends to a bool return type for success/failure and pass in a uint32_t* for FSR value. thanks -- PMM
On Tue, Jun 2, 2015 at 4:59 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 1 June 2015 at 19:04, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: >> Unified MPU only. Uses ARM architecture major revision to switch >> between PMSAv5 and v7 when ARM_FEATURE_MPU is set. PMSA v6 remains >> unsupported and is asserted against. >> >> The return code from get_phys_addr has to patched to handle the case >> of a 0 FSR with error. Use -1 to indicate this condition. >> >> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> --- >> target-arm/cpu.h | 1 + >> target-arm/helper.c | 153 ++++++++++++++++++++++++++++++++++++++++++++++++---- >> 2 files changed, 144 insertions(+), 10 deletions(-) >> >> diff --git a/target-arm/cpu.h b/target-arm/cpu.h >> index 9cb2e49..73e2619 100644 >> --- a/target-arm/cpu.h >> +++ b/target-arm/cpu.h >> @@ -559,6 +559,7 @@ void pmccntr_sync(CPUARMState *env); >> #define SCTLR_DT (1U << 16) /* up to ??, RAO in v6 and v7 */ >> #define SCTLR_nTWI (1U << 16) /* v8 onward */ >> #define SCTLR_HA (1U << 17) >> +#define SCTLR_BR (1U << 17) /* PMSA only */ >> #define SCTLR_IT (1U << 18) /* up to ??, RAO in v6 and v7 */ >> #define SCTLR_nTWE (1U << 18) /* v8 onward */ >> #define SCTLR_WXN (1U << 19) >> diff --git a/target-arm/helper.c b/target-arm/helper.c >> index 63859a4..09210d3 100644 >> --- a/target-arm/helper.c >> +++ b/target-arm/helper.c >> @@ -3323,13 +3323,13 @@ void register_cp_regs_for_features(ARMCPU *cpu) >> define_one_arm_cp_reg(cpu, &rvbar); >> } >> if (arm_feature(env, ARM_FEATURE_MPU)) { >> - /* These are the MPU registers prior to PMSAv6. Any new >> - * PMSA core later than the ARM946 will require that we >> - * implement the PMSAv6 or PMSAv7 registers, which are >> - * completely different. >> - */ >> - assert(!arm_feature(env, ARM_FEATURE_V6)); >> - define_arm_cp_regs(cpu, pmsav5_cp_reginfo); >> + if (arm_feature(env, ARM_FEATURE_V6)) { >> + assert(arm_feature(env, ARM_FEATURE_V7)); > > Could use a comment /* PMSAv6 is not implemented */ > Added. >> + define_arm_cp_regs(cpu, vmsa_pmsa_cp_reginfo); > > It's a bit unfortunate that we've managed to end up with > a cpreg array for "present in both VMSA and PMSA" but the > code path doesn't then allow for calling the define_ function > just once. Oh well. > Yeh, so I thought this preferrable to having to dup the arm_feature() iffery. >> + define_arm_cp_regs(cpu, pmsav7_cp_reginfo); >> + } else { >> + define_arm_cp_regs(cpu, pmsav5_cp_reginfo); >> + } >> } else { >> define_arm_cp_regs(cpu, vmsa_pmsa_cp_reginfo); >> define_arm_cp_regs(cpu, vmsa_cp_reginfo); >> @@ -5720,6 +5720,130 @@ do_fault: >> return (1 << 9) | (fault_type << 2) | level; >> } >> >> +static inline void get_phys_addr_pmsav7_default(CPUARMState *env, >> + ARMMMUIdx mmu_idx, >> + int32_t address, int *prot) >> +{ >> + *prot = PAGE_READ | PAGE_WRITE; >> + switch (address) { >> + case 0xF0000000 ... 0xFFFFFFFF: >> + if (regime_sctlr(env, mmu_idx) & SCTLR_V) { /* hivecs execing is ok */ >> + *prot |= PAGE_EXEC; >> + } >> + break; >> + case 0x00000000 ... 0x7FFFFFFF: >> + *prot |= PAGE_EXEC; >> + break; >> + } >> + >> +} >> + >> +static int get_phys_addr_pmsav7(CPUARMState *env, uint32_t address, >> + int access_type, ARMMMUIdx mmu_idx, >> + hwaddr *phys_ptr, int *prot) >> +{ >> + int n; >> + bool is_user = regime_is_user(env, mmu_idx); >> + >> + *phys_ptr = address; >> + *prot = 0; >> + >> + if (regime_translation_disabled(env, mmu_idx)) { /* MPU disabled */ >> + get_phys_addr_pmsav7_default(env, mmu_idx, address, prot); > > When will this be reached? get_phys_addr() has already caught the > 'MPU disabled' case. > Nice catch. It is actually a bug in get_phys_addr. Unlike the existing get_phys_addr_foo implementations, get_phys_addr_pmsav7 needs to handle the disabled case, as the behaviour should still default to the backgrounding rather than full system access. That check needs to be disabled in our case. >> + } else { /* MPU enabled */ >> + for (n = 15; n >= 0; n--) { /* region search */ >> + uint32_t base = env->cp15.c6_drbar[n]; >> + uint32_t rsize = extract32(env->cp15.c6_drsr[n], 1, 5) + 1; >> + int snd; >> + >> + if (!(env->cp15.c6_drsr[n] & 0x1)) { >> + continue; >> + } >> + if (rsize < 2) { >> + qemu_log_mask(LOG_GUEST_ERROR, "DRSR.Rsize field can not be 0"); > > This case is UNPREDICTABLE so I would continue here (ie treat the > region as disabled) rather than doing something potentially odd > with an out of range value later. > Fixed. >> + } > > Our effective minimum region size is 1K because that's the > target pagesize setting. Should we enforce that (minimum region > size is impdef so it would be architecturally ok) or will that > break otherwise ok guests? > So this gets complex due to subregions. The real permissions granularity in on the subregion level. To be safe we would have to go lowest common denominator with 8K regions so that subregions are only 1k. I have instead added a check to allow use for a 1k region with consistent subregion settings but disallow inconsistent SRs. The solution is generalised for all region sizes and subregion bit consistency combinations (e.g. 2k region with 4 consistent SR bits is ok too). I'm disallowing accesses with an UNIMP (ignoring region and continuing) in the inconsistent case. > Base address not aligned to the region size is also UNPREDICTABLE, > incidentally. > Check added. region is ignored (continue) in this case. >> + >> + if (address < base || address > base - 1 + (1ull << rsize)) { >> + continue; >> + } >> + >> + if (rsize < 8) { /* no subregions for regions < 256 bytes */ >> + break; >> + } >> + >> + rsize -= 3; /* sub region size (power of 2) */ >> + snd = (address - base) >> rsize & 0x7; >> + if (env->cp15.c6_drsr[n] & 1 << (snd + 8)) { > > I think I would find these easier to read with more parens > so I didn't have to look up & vs << precedence. > changed to extract32. >> + continue; >> + } >> + break; >> + } >> + >> + if (n == -1) { /* no hits */ >> + if (is_user || !(regime_sctlr(env, mmu_idx) & SCTLR_BR)) { >> + /* background fault */ >> + return -1; >> + } else { >> + get_phys_addr_pmsav7_default(env, mmu_idx, address, prot); >> + } >> + } else { /* a MPU hit! */ >> + uint32_t ap = extract32(env->cp15.c6_dracr[n], 8, 3); >> + >> + if (is_user) { /* User mode AP bit decoding */ >> + switch (ap) { >> + case 0: >> + case 1: >> + case 5: >> + break; /* no access */ >> + case 3: >> + *prot |= PAGE_WRITE; >> + /* fall through */ >> + case 2: >> + case 6: >> + *prot |= PAGE_READ | PAGE_EXEC; >> + break; >> + default: >> + qemu_log_mask(LOG_GUEST_ERROR, "Bad value for AP bits in " >> + "DRACR"); > > Putting the line break after the ',' would let you avoid > splitting the string. > Fixed. This message should probably also include said bad value of ap so I have added that. >> + } >> + } else { /* Priv. mode AP bits decoding */ >> + switch (ap) { >> + case 0: >> + break; /* no access */ >> + case 1: >> + case 2: >> + case 3: >> + *prot |= PAGE_WRITE; >> + /* fall through */ >> + case 5: >> + case 6: >> + *prot |= PAGE_READ | PAGE_EXEC; >> + break; >> + default: >> + qemu_log_mask(LOG_GUEST_ERROR, "Bad value for AP bits in " >> + "DRACR"); Same. >> + } >> + } >> + >> + /* execute never */ >> + *prot &= env->cp15.c6_dracr[n] & 1 << 12 ? ~PAGE_EXEC : ~0; > > Break this down into something more readable, please. > With the FSR return refactoring this is now an if anyway. >> + } >> + } >> + >> + switch (access_type) { >> + case 0: >> + return *prot & PAGE_READ ? 0 : 0x00D; >> + case 1: >> + return *prot & PAGE_WRITE ? 0 : 0x00D; >> + case 2: >> + return *prot & PAGE_EXEC ? 0 : 0x00D; > > This is > if (!(*prot & (1 << access_type))) { > return 0xD; /* Permission fault */ > } else { > return 0; > } > > isn't it? > Yes but that assumes that access_type encoding is correlated to PAGE_FOO masks so I didn't want this to break if one or the other was re-encoded. >> + default: >> + abort(); /* should be unreachable */ >> + return 0; > > The usual way to write this is g_assert_not_reached(), and the return > isn't needed. > Fixed. >> + } >> + >> +} >> + >> static int get_phys_addr_pmsav5(CPUARMState *env, uint32_t address, >> int access_type, ARMMMUIdx mmu_idx, >> hwaddr *phys_ptr, int *prot) >> @@ -5795,13 +5919,14 @@ static int get_phys_addr_pmsav5(CPUARMState *env, uint32_t address, >> * MPU state on MPU based systems. >> * >> * Returns 0 if the translation was successful. Otherwise, phys_ptr, attrs, >> - * prot and page_size may not be filled in, and the return value provides >> + * prot and page_size may or may-not be filled in, and the return value provides > > No hyphen. > Fixed. >> * information on why the translation aborted, in the format of a >> * DFSR/IFSR fault register, with the following caveats: >> * * we honour the short vs long DFSR format differences. >> * * the WnR bit is never set (the caller must do this). >> * * for MPU based systems we don't bother to return a full FSR format >> * value. > > Is this bullet point still true? Not quite. It should read "PMSAv5 based systems". Fixed > If it is, is that a bug > we now need to fix? > Don't think so. Should be ok. >> + * * -1 return value indicates a 0 FSR. >> * >> * @env: CPUARMState >> * @address: virtual address to get physical address for >> @@ -5857,8 +5982,14 @@ static inline int get_phys_addr(CPUARMState *env, target_ulong address, >> >> if (arm_feature(env, ARM_FEATURE_MPU)) { >> *page_size = TARGET_PAGE_SIZE; >> - return get_phys_addr_pmsav5(env, address, access_type, mmu_idx, >> - phys_ptr, prot); >> + if (arm_feature(env, ARM_FEATURE_V6)) { >> + assert(arm_feature(env, ARM_FEATURE_V7)); >> + return get_phys_addr_pmsav7(env, address, access_type, mmu_idx, >> + phys_ptr, prot); > > v7M will take this code path now (which is the right thing, it is > closer to v7R than to the 946); did you cross check against > the M profile spec to see if any of this is R-profile specific? > Nope. Just add !ARM_FEATURE_M to avoid the beartrap? > (We don't actually implement the M profile MPU right now, but > it would be nice to avoid leaving beartraps for whoever does.) > >> + } else { >> + return get_phys_addr_pmsav5(env, address, access_type, mmu_idx, >> + phys_ptr, prot); >> + } >> } >> >> if (regime_using_lpae_format(env, mmu_idx)) { >> @@ -5897,6 +6028,8 @@ int arm_tlb_fill(CPUState *cs, vaddr address, >> tlb_set_page_with_attrs(cs, address, phys_addr, attrs, >> prot, mmu_idx, page_size); >> return 0; >> + } else if (ret == -1) { >> + ret = 0; >> } >> >> return ret; > > This isn't going to work, because tlb_fill() which calls this > function making the same "0 means OK, non-0 means FSR value" > assumption. > > I think the best thing to do here is to switch > get_phys_addr() and friends to a bool return type for > success/failure and pass in a uint32_t* for FSR value. > And done (new patch that'll go up front of v2). Regards, Peter > thanks > -- PMM >
On 10 June 2015 at 23:17, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Tue, Jun 2, 2015 at 4:59 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 1 June 2015 at 19:04, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: >>> + switch (access_type) { >>> + case 0: >>> + return *prot & PAGE_READ ? 0 : 0x00D; >>> + case 1: >>> + return *prot & PAGE_WRITE ? 0 : 0x00D; >>> + case 2: >>> + return *prot & PAGE_EXEC ? 0 : 0x00D; >> >> This is >> if (!(*prot & (1 << access_type))) { >> return 0xD; /* Permission fault */ >> } else { >> return 0; >> } >> >> isn't it? >> > > Yes but that assumes that access_type encoding is correlated to > PAGE_FOO masks so I didn't want this to break if one or the other was > re-encoded. We already do this in the lpae code path; I think it's safe. >>> + * * -1 return value indicates a 0 FSR. >>> * >>> * @env: CPUARMState >>> * @address: virtual address to get physical address for >>> @@ -5857,8 +5982,14 @@ static inline int get_phys_addr(CPUARMState *env, target_ulong address, >>> >>> if (arm_feature(env, ARM_FEATURE_MPU)) { >>> *page_size = TARGET_PAGE_SIZE; >>> - return get_phys_addr_pmsav5(env, address, access_type, mmu_idx, >>> - phys_ptr, prot); >>> + if (arm_feature(env, ARM_FEATURE_V6)) { >>> + assert(arm_feature(env, ARM_FEATURE_V7)); >>> + return get_phys_addr_pmsav7(env, address, access_type, mmu_idx, >>> + phys_ptr, prot); >> >> v7M will take this code path now (which is the right thing, it is >> closer to v7R than to the 946); did you cross check against >> the M profile spec to see if any of this is R-profile specific? >> > > Nope. Just add !ARM_FEATURE_M to avoid the beartrap? Currently we don't set the MPU feature bit for M3; anybody adding MPU support will need to check the code anyway, so having the function not be called at all for M profile is probably more confusing than helpful. I just wondered if you'd looked at whether the two are really identical or not... thanks -- PMM
On Wed, Jun 10, 2015 at 3:21 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 10 June 2015 at 23:17, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> On Tue, Jun 2, 2015 at 4:59 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> On 1 June 2015 at 19:04, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: >>>> + switch (access_type) { >>>> + case 0: >>>> + return *prot & PAGE_READ ? 0 : 0x00D; >>>> + case 1: >>>> + return *prot & PAGE_WRITE ? 0 : 0x00D; >>>> + case 2: >>>> + return *prot & PAGE_EXEC ? 0 : 0x00D; >>> >>> This is >>> if (!(*prot & (1 << access_type))) { >>> return 0xD; /* Permission fault */ >>> } else { >>> return 0; >>> } >>> >>> isn't it? >>> >> >> Yes but that assumes that access_type encoding is correlated to >> PAGE_FOO masks so I didn't want this to break if one or the other was >> re-encoded. > > We already do this in the lpae code path; I think it's safe. > Ok, doing it the quick way. Regards, Peter
diff --git a/target-arm/cpu.h b/target-arm/cpu.h index 9cb2e49..73e2619 100644 --- a/target-arm/cpu.h +++ b/target-arm/cpu.h @@ -559,6 +559,7 @@ void pmccntr_sync(CPUARMState *env); #define SCTLR_DT (1U << 16) /* up to ??, RAO in v6 and v7 */ #define SCTLR_nTWI (1U << 16) /* v8 onward */ #define SCTLR_HA (1U << 17) +#define SCTLR_BR (1U << 17) /* PMSA only */ #define SCTLR_IT (1U << 18) /* up to ??, RAO in v6 and v7 */ #define SCTLR_nTWE (1U << 18) /* v8 onward */ #define SCTLR_WXN (1U << 19) diff --git a/target-arm/helper.c b/target-arm/helper.c index 63859a4..09210d3 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -3323,13 +3323,13 @@ void register_cp_regs_for_features(ARMCPU *cpu) define_one_arm_cp_reg(cpu, &rvbar); } if (arm_feature(env, ARM_FEATURE_MPU)) { - /* These are the MPU registers prior to PMSAv6. Any new - * PMSA core later than the ARM946 will require that we - * implement the PMSAv6 or PMSAv7 registers, which are - * completely different. - */ - assert(!arm_feature(env, ARM_FEATURE_V6)); - define_arm_cp_regs(cpu, pmsav5_cp_reginfo); + if (arm_feature(env, ARM_FEATURE_V6)) { + assert(arm_feature(env, ARM_FEATURE_V7)); + define_arm_cp_regs(cpu, vmsa_pmsa_cp_reginfo); + define_arm_cp_regs(cpu, pmsav7_cp_reginfo); + } else { + define_arm_cp_regs(cpu, pmsav5_cp_reginfo); + } } else { define_arm_cp_regs(cpu, vmsa_pmsa_cp_reginfo); define_arm_cp_regs(cpu, vmsa_cp_reginfo); @@ -5720,6 +5720,130 @@ do_fault: return (1 << 9) | (fault_type << 2) | level; } +static inline void get_phys_addr_pmsav7_default(CPUARMState *env, + ARMMMUIdx mmu_idx, + int32_t address, int *prot) +{ + *prot = PAGE_READ | PAGE_WRITE; + switch (address) { + case 0xF0000000 ... 0xFFFFFFFF: + if (regime_sctlr(env, mmu_idx) & SCTLR_V) { /* hivecs execing is ok */ + *prot |= PAGE_EXEC; + } + break; + case 0x00000000 ... 0x7FFFFFFF: + *prot |= PAGE_EXEC; + break; + } + +} + +static int get_phys_addr_pmsav7(CPUARMState *env, uint32_t address, + int access_type, ARMMMUIdx mmu_idx, + hwaddr *phys_ptr, int *prot) +{ + int n; + bool is_user = regime_is_user(env, mmu_idx); + + *phys_ptr = address; + *prot = 0; + + if (regime_translation_disabled(env, mmu_idx)) { /* MPU disabled */ + get_phys_addr_pmsav7_default(env, mmu_idx, address, prot); + } else { /* MPU enabled */ + for (n = 15; n >= 0; n--) { /* region search */ + uint32_t base = env->cp15.c6_drbar[n]; + uint32_t rsize = extract32(env->cp15.c6_drsr[n], 1, 5) + 1; + int snd; + + if (!(env->cp15.c6_drsr[n] & 0x1)) { + continue; + } + if (rsize < 2) { + qemu_log_mask(LOG_GUEST_ERROR, "DRSR.Rsize field can not be 0"); + } + + if (address < base || address > base - 1 + (1ull << rsize)) { + continue; + } + + if (rsize < 8) { /* no subregions for regions < 256 bytes */ + break; + } + + rsize -= 3; /* sub region size (power of 2) */ + snd = (address - base) >> rsize & 0x7; + if (env->cp15.c6_drsr[n] & 1 << (snd + 8)) { + continue; + } + break; + } + + if (n == -1) { /* no hits */ + if (is_user || !(regime_sctlr(env, mmu_idx) & SCTLR_BR)) { + /* background fault */ + return -1; + } else { + get_phys_addr_pmsav7_default(env, mmu_idx, address, prot); + } + } else { /* a MPU hit! */ + uint32_t ap = extract32(env->cp15.c6_dracr[n], 8, 3); + + if (is_user) { /* User mode AP bit decoding */ + switch (ap) { + case 0: + case 1: + case 5: + break; /* no access */ + case 3: + *prot |= PAGE_WRITE; + /* fall through */ + case 2: + case 6: + *prot |= PAGE_READ | PAGE_EXEC; + break; + default: + qemu_log_mask(LOG_GUEST_ERROR, "Bad value for AP bits in " + "DRACR"); + } + } else { /* Priv. mode AP bits decoding */ + switch (ap) { + case 0: + break; /* no access */ + case 1: + case 2: + case 3: + *prot |= PAGE_WRITE; + /* fall through */ + case 5: + case 6: + *prot |= PAGE_READ | PAGE_EXEC; + break; + default: + qemu_log_mask(LOG_GUEST_ERROR, "Bad value for AP bits in " + "DRACR"); + } + } + + /* execute never */ + *prot &= env->cp15.c6_dracr[n] & 1 << 12 ? ~PAGE_EXEC : ~0; + } + } + + switch (access_type) { + case 0: + return *prot & PAGE_READ ? 0 : 0x00D; + case 1: + return *prot & PAGE_WRITE ? 0 : 0x00D; + case 2: + return *prot & PAGE_EXEC ? 0 : 0x00D; + default: + abort(); /* should be unreachable */ + return 0; + } + +} + static int get_phys_addr_pmsav5(CPUARMState *env, uint32_t address, int access_type, ARMMMUIdx mmu_idx, hwaddr *phys_ptr, int *prot) @@ -5795,13 +5919,14 @@ static int get_phys_addr_pmsav5(CPUARMState *env, uint32_t address, * MPU state on MPU based systems. * * Returns 0 if the translation was successful. Otherwise, phys_ptr, attrs, - * prot and page_size may not be filled in, and the return value provides + * prot and page_size may or may-not be filled in, and the return value provides * information on why the translation aborted, in the format of a * DFSR/IFSR fault register, with the following caveats: * * we honour the short vs long DFSR format differences. * * the WnR bit is never set (the caller must do this). * * for MPU based systems we don't bother to return a full FSR format * value. + * * -1 return value indicates a 0 FSR. * * @env: CPUARMState * @address: virtual address to get physical address for @@ -5857,8 +5982,14 @@ static inline int get_phys_addr(CPUARMState *env, target_ulong address, if (arm_feature(env, ARM_FEATURE_MPU)) { *page_size = TARGET_PAGE_SIZE; - return get_phys_addr_pmsav5(env, address, access_type, mmu_idx, - phys_ptr, prot); + if (arm_feature(env, ARM_FEATURE_V6)) { + assert(arm_feature(env, ARM_FEATURE_V7)); + return get_phys_addr_pmsav7(env, address, access_type, mmu_idx, + phys_ptr, prot); + } else { + return get_phys_addr_pmsav5(env, address, access_type, mmu_idx, + phys_ptr, prot); + } } if (regime_using_lpae_format(env, mmu_idx)) { @@ -5897,6 +6028,8 @@ int arm_tlb_fill(CPUState *cs, vaddr address, tlb_set_page_with_attrs(cs, address, phys_addr, attrs, prot, mmu_idx, page_size); return 0; + } else if (ret == -1) { + ret = 0; } return ret;
Unified MPU only. Uses ARM architecture major revision to switch between PMSAv5 and v7 when ARM_FEATURE_MPU is set. PMSA v6 remains unsupported and is asserted against. The return code from get_phys_addr has to patched to handle the case of a 0 FSR with error. Use -1 to indicate this condition. Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> --- target-arm/cpu.h | 1 + target-arm/helper.c | 153 ++++++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 144 insertions(+), 10 deletions(-)