On 12/01/2017 03:44 PM, Peter Maydell wrote: > For the TT instruction we're going to need to do an MPU lookup that > also tells us which MPU region the access hit. This requires us > to do the MPU lookup without first doing the SAU security access > check, so pull the MPU lookup parts of get_phys_addr_pmsav8() > out into their own function. > > The TT instruction also needs to know the MPU region number which > the lookup hit, so provide this information to the caller of the > MPU lookup code, even though get_phys_addr_pmsav8() doesn't > need to know it. > > Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
Elegant refactor! Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org> > --- > target/arm/helper.c | 130 > +++++++++++++++++++++++++++++++--------------------- > 1 file changed, 79 insertions(+), 51 deletions(-) > > diff --git a/target/arm/helper.c b/target/arm/helper.c > index 14ab1f4..be8d797 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -9351,67 +9351,28 @@ static void v8m_security_lookup(CPUARMState *env, > uint32_t address, > } > } > > -static bool get_phys_addr_pmsav8(CPUARMState *env, uint32_t address, > - MMUAccessType access_type, ARMMMUIdx > mmu_idx, > - hwaddr *phys_ptr, MemTxAttrs *txattrs, > - int *prot, uint32_t *fsr) > +static bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address, > + MMUAccessType access_type, ARMMMUIdx mmu_idx, > + hwaddr *phys_ptr, MemTxAttrs *txattrs, > + int *prot, uint32_t *fsr, uint32_t *mregion) > { > + /* Perform a PMSAv8 MPU lookup (without also doing the SAU check > + * that a full phys-to-virt translation does). > + * mregion is (if not NULL) set to the region number which matched, > + * or -1 if no region number is returned (MPU off, address did not > + * hit a region, address hit in multiple regions). > + */ > ARMCPU *cpu = arm_env_get_cpu(env); > bool is_user = regime_is_user(env, mmu_idx); > uint32_t secure = regime_is_secure(env, mmu_idx); > int n; > int matchregion = -1; > bool hit = false; > - V8M_SAttributes sattrs = {}; > > *phys_ptr = address; > *prot = 0; > - > - if (arm_feature(env, ARM_FEATURE_M_SECURITY)) { > - v8m_security_lookup(env, address, access_type, mmu_idx, &sattrs); > - if (access_type == MMU_INST_FETCH) { > - /* Instruction fetches always use the MMU bank and the > - * transaction attribute determined by the fetch address, > - * regardless of CPU state. This is painful for QEMU > - * to handle, because it would mean we need to encode > - * into the mmu_idx not just the (user, negpri) information > - * for the current security state but also that for the > - * other security state, which would balloon the number > - * of mmu_idx values needed alarmingly. > - * Fortunately we can avoid this because it's not actually > - * possible to arbitrarily execute code from memory with > - * the wrong security attribute: it will always generate > - * an exception of some kind or another, apart from the > - * special case of an NS CPU executing an SG instruction > - * in S&NSC memory. So we always just fail the translation > - * here and sort things out in the exception handler > - * (including possibly emulating an SG instruction). > - */ > - if (sattrs.ns != !secure) { > - *fsr = sattrs.nsc ? M_FAKE_FSR_NSC_EXEC : M_FAKE_FSR_SFAULT; > - return true; > - } > - } else { > - /* For data accesses we always use the MMU bank indicated > - * by the current CPU state, but the security attributes > - * might downgrade a secure access to nonsecure. > - */ > - if (sattrs.ns) { > - txattrs->secure = false; > - } else if (!secure) { > - /* NS access to S memory must fault. > - * Architecturally we should first check whether the > - * MPU information for this address indicates that we > - * are doing an unaligned access to Device memory, which > - * should generate a UsageFault instead. QEMU does not > - * currently check for that kind of unaligned access though. > - * If we added it we would need to do so as a special case > - * for M_FAKE_FSR_SFAULT in arm_v7m_cpu_do_interrupt(). > - */ > - *fsr = M_FAKE_FSR_SFAULT; > - return true; > - } > - } > + if (mregion) { > + *mregion = -1; > } > > /* Unlike the ARM ARM pseudocode, we don't need to check whether this > @@ -9500,12 +9461,79 @@ static bool get_phys_addr_pmsav8(CPUARMState *env, > uint32_t address, > /* We don't need to look the attribute up in the MAIR0/MAIR1 > * registers because that only tells us about cacheability. > */ > + if (mregion) { > + *mregion = matchregion; > + } > } > > *fsr = 0x00d; /* Permission fault */ > return !(*prot & (1 << access_type)); > } > > + > +static bool get_phys_addr_pmsav8(CPUARMState *env, uint32_t address, > + MMUAccessType access_type, ARMMMUIdx > mmu_idx, > + hwaddr *phys_ptr, MemTxAttrs *txattrs, > + int *prot, uint32_t *fsr) > +{ > + uint32_t secure = regime_is_secure(env, mmu_idx); > + V8M_SAttributes sattrs = {}; > + > + if (arm_feature(env, ARM_FEATURE_M_SECURITY)) { > + v8m_security_lookup(env, address, access_type, mmu_idx, &sattrs); > + if (access_type == MMU_INST_FETCH) { > + /* Instruction fetches always use the MMU bank and the > + * transaction attribute determined by the fetch address, > + * regardless of CPU state. This is painful for QEMU > + * to handle, because it would mean we need to encode > + * into the mmu_idx not just the (user, negpri) information > + * for the current security state but also that for the > + * other security state, which would balloon the number > + * of mmu_idx values needed alarmingly. > + * Fortunately we can avoid this because it's not actually > + * possible to arbitrarily execute code from memory with > + * the wrong security attribute: it will always generate > + * an exception of some kind or another, apart from the > + * special case of an NS CPU executing an SG instruction > + * in S&NSC memory. So we always just fail the translation > + * here and sort things out in the exception handler > + * (including possibly emulating an SG instruction). > + */ > + if (sattrs.ns != !secure) { > + *fsr = sattrs.nsc ? M_FAKE_FSR_NSC_EXEC : M_FAKE_FSR_SFAULT; > + *phys_ptr = address; > + *prot = 0; > + return true; > + } > + } else { > + /* For data accesses we always use the MMU bank indicated > + * by the current CPU state, but the security attributes > + * might downgrade a secure access to nonsecure. > + */ > + if (sattrs.ns) { > + txattrs->secure = false; > + } else if (!secure) { > + /* NS access to S memory must fault. > + * Architecturally we should first check whether the > + * MPU information for this address indicates that we > + * are doing an unaligned access to Device memory, which > + * should generate a UsageFault instead. QEMU does not > + * currently check for that kind of unaligned access though. > + * If we added it we would need to do so as a special case > + * for M_FAKE_FSR_SFAULT in arm_v7m_cpu_do_interrupt(). > + */ > + *fsr = M_FAKE_FSR_SFAULT; > + *phys_ptr = address; > + *prot = 0; > + return true; > + } > + } > + } > + > + return pmsav8_mpu_lookup(env, address, access_type, mmu_idx, phys_ptr, > + txattrs, prot, fsr, NULL); > +} > + > static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address, > MMUAccessType access_type, ARMMMUIdx > mmu_idx, > hwaddr *phys_ptr, int *prot, uint32_t *fsr) >