[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 5/6] ARM: enable ARM_FEATURE_MPU for Cortex-M3/M
From: |
Peter Crosthwaite |
Subject: |
Re: [Qemu-devel] [PATCH 5/6] ARM: enable ARM_FEATURE_MPU for Cortex-M3/M4 |
Date: |
Wed, 8 Jul 2015 14:42:48 -0700 |
On Wed, Jul 8, 2015 at 12:56 AM, Alex Züpke <address@hidden> wrote:
> Am 07.07.2015 um 22:50 schrieb Peter Crosthwaite:
>> On Tue, Jul 7, 2015 at 11:25 AM, Alex Zuepke <address@hidden> wrote:
>>>
>>> Signed-off-by: Alex Zuepke <address@hidden>
>>> ---
>>> 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
>>>
>>>
>
>
>