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

["Alex Züpke" <alexander.zuepke@hs-rm.de>]

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