Message ID | CAEgOgz6prhyY5D+o71SEbvXiHOHnBYrDdLM+NiqSTbtLMVp4vw@mail.gmail.com |
---|---|
State | New |
Headers | show |
Am 07.07.2015 um 22:50 schrieb Peter Crosthwaite: > On Tue, Jul 7, 2015 at 11:25 AM, Alex Zuepke <alexander.zuepke@hs-rm.de> wrote: >> >> Signed-off-by: Alex Zuepke <alexander.zuepke@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"? > >> +{ >> + *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 >> >>
On Wed, Jul 8, 2015 at 12:56 AM, Alex Züpke <alexander.zuepke@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.zuepke@hs-rm.de> wrote: >>> >>> Signed-off-by: Alex Zuepke <alexander.zuepke@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 >>> >>> > > >
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];