On Wed, Jul 8, 2015 at 12:56 AM, Alex Züpke <alexander.zue...@hs-rm.de> wrote: > Am 07.07.2015 um 22:50 schrieb Peter Crosthwaite: >> On Tue, Jul 7, 2015 at 11:25 AM, Alex Zuepke <alexander.zue...@hs-rm.de> >> wrote: >>> >>> Signed-off-by: Alex Zuepke <alexander.zue...@hs-rm.de> >>> --- >>> hw/arm/armv7m.c | 17 ++++++++++++++++- >>> target-arm/cpu.c | 2 ++ >>> target-arm/helper.c | 30 ++++++++++++++++++++++++++++-- >>> 3 files changed, 46 insertions(+), 3 deletions(-) >>> >>> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c >>> index c6eab6d..db6bc3c 100644 >>> --- a/hw/arm/armv7m.c >>> +++ b/hw/arm/armv7m.c >>> @@ -179,15 +179,30 @@ qemu_irq *armv7m_init(MemoryRegion *system_memory, >>> int mem_size, int num_irq, >>> int i; >>> int big_endian; >>> MemoryRegion *hack = g_new(MemoryRegion, 1); >>> + ObjectClass *cpu_oc; >>> + Error *err = NULL; >>> >>> if (cpu_model == NULL) { >>> cpu_model = "cortex-m3"; >>> } >>> - cpu = cpu_arm_init(cpu_model); >>> + cpu_oc = cpu_class_by_name(TYPE_ARM_CPU, cpu_model); >>> + cpu = ARM_CPU(object_new(object_class_get_name(cpu_oc))); >> >> Is this change in scope of the patch? why did you need it? > > I found no other way than this to change the "pmsav7-dregion" property from > its default value. > Depending on this property, the existing constructors allocate memory for MPU > window handling internally. > >>> if (cpu == NULL) { >>> fprintf(stderr, "Unable to find CPU definition\n"); >>> exit(1); >>> } >>> + /* On Cortex-M3/M4, the MPU has 8 windows */ >>> + object_property_set_int(OBJECT(cpu), 8, "pmsav7-dregion", &err); >>> + if (err) { >>> + error_report_err(err); >>> + exit(1); >>> + } >>> + object_property_set_bool(OBJECT(cpu), true, "realized", &err); >>> + if (err) { >>> + error_report_err(err); >>> + exit(1); >>> + } >>> + >>> env = &cpu->env; >>> >>> armv7m_bitband_init(); >>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c >>> index 80669a6..d8cfbb1 100644 >>> --- a/target-arm/cpu.c >>> +++ b/target-arm/cpu.c >>> @@ -832,6 +832,7 @@ static void cortex_m3_initfn(Object *obj) >>> ARMCPU *cpu = ARM_CPU(obj); >>> set_feature(&cpu->env, ARM_FEATURE_V7); >>> set_feature(&cpu->env, ARM_FEATURE_M); >>> + set_feature(&cpu->env, ARM_FEATURE_MPU); >>> cpu->midr = 0x410fc231; >>> } >>> >>> @@ -841,6 +842,7 @@ static void cortex_m4_initfn(Object *obj) >>> >>> set_feature(&cpu->env, ARM_FEATURE_V7); >>> set_feature(&cpu->env, ARM_FEATURE_M); >>> + set_feature(&cpu->env, ARM_FEATURE_MPU); >>> set_feature(&cpu->env, ARM_FEATURE_THUMB_DSP); >>> cpu->midr = 0x410fc240; /* r0p0 */ >>> } >>> diff --git a/target-arm/helper.c b/target-arm/helper.c >>> index 555bc5f..637dbf6 100644 >>> --- a/target-arm/helper.c >>> +++ b/target-arm/helper.c >>> @@ -5854,6 +5854,26 @@ static inline void >>> get_phys_addr_pmsav7_default(CPUARMState *env, >>> >>> } >>> >>> +static inline void get_phys_addr_v7m_default(CPUARMState *env, >>> + ARMMMUIdx mmu_idx, >>> + int32_t address, int *prot) >> >> The fn name should include something about mpu or pmsa. v7m >> unqualified is a little general. Does the V7M doc use "PMSA" or is >> that an AR profile thing? > > The ARMv7-M docs describe the MPU as a PMSAv7 one, but the interface is > different to the specification > in the ARMv7-A/M manual. Since the existing code already used a "v7m" prefix > for v7-M, I tried to stick with that. > > The original get_phys_add _pmsav7_default() function describes the default > memory map for ARMv7-R CPUs, > so we should probably rename this function to get_phys_addr_v7r_default()? Is > it OK to introduce "v7r"? >
Yes, But we need to keep the "PMSA" in both R and M variants to make it clear it is about MPU. get_phys_add _pmsav7r_default() get_phys_add _pmsav7m_default() Regards, Peter > >> >>> +{ >>> + *prot = PAGE_READ | PAGE_WRITE; >>> + switch (address) { >>> + case 0xFFFFF000 ... 0xFFFFFFFF: >>> + /* the special exception return address memory region is EXEC only >>> */ >>> + *prot = PAGE_EXEC; >>> + break; >>> + >>> + case 0x00000000 ... 0x1FFFFFFF: >>> + case 0x20000000 ... 0x3FFFFFFF: >>> + case 0x60000000 ... 0x7FFFFFFF: >>> + case 0x80000000 ... 0x9FFFFFFF: >>> + *prot |= PAGE_EXEC; >>> + break; >>> + } >>> +} >>> + >>> static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address, >>> int access_type, ARMMMUIdx mmu_idx, >>> hwaddr *phys_ptr, int *prot, uint32_t >>> *fsr) >>> @@ -5866,7 +5886,10 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, >>> uint32_t address, >>> *prot = 0; >>> >>> if (regime_translation_disabled(env, mmu_idx)) { /* MPU disabled */ >>> - get_phys_addr_pmsav7_default(env, mmu_idx, address, prot); >>> + if (arm_feature(env, ARM_FEATURE_M)) >> >> Single line ifs still require { by coding style. scripts/checkpatch.pl >> will catch these. > > OK. > >> >>> + get_phys_addr_v7m_default(env, mmu_idx, address, prot); >>> + else >>> + get_phys_addr_pmsav7_default(env, mmu_idx, address, prot); >>> } else { /* MPU enabled */ >>> for (n = (int)cpu->pmsav7_dregion - 1; n >= 0; n--) { >>> /* region search */ >>> @@ -5944,7 +5967,10 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, >>> uint32_t address, >>> *fsr = 0; >>> return true; >>> } >>> - get_phys_addr_pmsav7_default(env, mmu_idx, address, prot); >>> + if (arm_feature(env, ARM_FEATURE_M)) >>> + get_phys_addr_v7m_default(env, mmu_idx, address, prot); >>> + else >>> + get_phys_addr_pmsav7_default(env, mmu_idx, address, prot); >> >> This if-else can be consolidated with something like: > > Will do, thanks a lot! > > Best regards > Alex > >> >> diff --git a/target-arm/helper.c b/target-arm/helper.c >> index 63f7e7b..bfe0afb 100644 >> --- a/target-arm/helper.c >> +++ b/target-arm/helper.c >> @@ -6573,10 +6573,9 @@ static int get_phys_addr_pmsav7(CPUARMState >> *env, uint32_t address, >> int n; >> *phys_ptr = address; >> *prot = 0; >> + bool do_default = true; >> >> - if (!(sctlr & 0x1)) { /* MPU disabled */ >> - get_phys_addr_pmsav7_default(env, address, prot); >> - } else { /* MPU enabled */ >> + if (sctlr & 0x1) { /* 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; >> @@ -6609,12 +6608,12 @@ static int get_phys_addr_pmsav7(CPUARMState >> *env, uint32_t address, >> if (is_user || !(sctlr & 1 << 17)) { /* BR */ >> /* background fault */ >> return -1; >> - } else { >> - get_phys_addr_pmsav7_default(env, address, prot); >> } >> } else { /* a MPU hit! */ >> uint32_t ap = extract32(env->cp15.c6_dracr[n], 8, 3); >> >> + do_default = false; >> + >> if (is_user) { /* User mode AP bit decoding */ >> switch (ap) { >> case 0: >> @@ -6656,6 +6655,14 @@ static int get_phys_addr_pmsav7(CPUARMState >> *env, uint32_t address, >> } >> } >> >> + if (do_default) { >> + if (arm_feature(env, ARM_FEATURE_M)) { >> + get_phys_addr_v7m_default(env, mmu_idx, address, prot); >> + } else { >> + get_phys_addr_pmsav7_default(env, mmu_idx, address, prot); >> + } >> + } >> + >> switch (access_type) { >> case 0: >> return *prot & PAGE_READ ? 0 : 0x405; >> >> Regards, >> Peter >> >> >>> } else { /* a MPU hit! */ >>> uint32_t ap = extract32(env->pmsav7.dracr[n], 8, 3); >>> >>> -- >>> 1.7.9.5 >>> >>> > > >