Re: [Qemu-devel] [PATCH 5/6] ARM: enable ARM_FEATURE_MPU for Cortex-M3/M4

[Peter Crosthwaite]

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
>>>
>>>
>
>
>