Re: [Qemu-devel] [PATCH target-arm v2 07/13] target-arm/helper.c: define MPUIR register

[Peter Crosthwaite]

On Mon, Jun 15, 2015 at 6:44 AM, Peter Maydell <address@hidden> wrote:
> On 12 June 2015 at 20:10, Peter Crosthwaite
> <address@hidden> wrote:
>> Define the MPUIR register for MPU supporting systems V6 onwards.
>
> "(ARMv6 and onwards)".
>
>> Currently only support unified MPU.
>
> "we only"
>
>> The size of the unified MPU is supported via the number of "dregions".
>> So just a single config as added to specify this size. (When split MPU
>> is implemented iregions will accompany).
>
> Try:
>
>     The size of the unified MPU is defined via the number of "dregions".
>     So just a single config is added to specify this size. (When split MPU
>     is implemented we will add an extra iregions config).
>

Fixed all.

>> Signed-off-by: Peter Crosthwaite <address@hidden>
>> ---
>> changed since v1:
>> Add #regions configuration
>> conditionalise MPUIR existence
>>
>>  target-arm/cpu-qom.h |  2 ++
>>  target-arm/cpu.c     |  8 ++++++++
>>  target-arm/helper.c  | 10 ++++++++++
>>  3 files changed, 20 insertions(+)
>>
>> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
>> index 30832d9..05c33ac 100644
>> --- a/target-arm/cpu-qom.h
>> +++ b/target-arm/cpu-qom.h
>> @@ -105,6 +105,8 @@ typedef struct ARMCPU {
>>
>>      /* CPU has memory protection unit */
>>      bool has_mpu;
>> +    /* PMSAv7 MPU number of supported regions */
>> +    uint32_t pmsav7_dregion;
>>
>>      /* PSCI conduit used to invoke PSCI methods
>>       * 0 - disabled, 1 - smc, 2 - hvc
>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> index 82ac52d..c967763 100644
>> --- a/target-arm/cpu.c
>> +++ b/target-arm/cpu.c
>> @@ -445,6 +445,9 @@ static Property arm_cpu_has_el3_property =
>>  static Property arm_cpu_has_mpu_property =
>>              DEFINE_PROP_BOOL("has-mpu", ARMCPU, has_mpu, true);
>>
>> +static Property arm_cpu_pmsav7_dregion_property =
>> +            DEFINE_PROP_UINT32("pmsav7-dregion", ARMCPU, pmsav7_dregion, 
>> 16);
>> +
>>  static void arm_cpu_post_init(Object *obj)
>>  {
>>      ARMCPU *cpu = ARM_CPU(obj);
>> @@ -476,6 +479,11 @@ static void arm_cpu_post_init(Object *obj)
>>      if (arm_feature(&cpu->env, ARM_FEATURE_MPU)) {
>>          qdev_property_add_static(DEVICE(obj), &arm_cpu_has_mpu_property,
>>                                   &error_abort);
>> +        if (arm_feature(&cpu->env, ARM_FEATURE_V7)) {
>> +            qdev_property_add_static(DEVICE(obj),
>> +                                     &arm_cpu_pmsav7_dregion_property,
>> +                                     &error_abort);
>> +        }
>>      }
>
>
> Worth making bogus values (<0, >255, at least) a realize error,
> I think, especially since we start allocating memory based on
> the number of regions later on.
>

Added realize error. So it occurs to me that 0 itself is valid for
#dregions. I have patched the memory allocation to not happen in 0
case and added null guards to the pointer accessors. This means that a
PMSA with 0 regions can be configured which will still call
get_phys_addr_pmsav7 and get the default behaviour.

Regards,
Peter

> Otherwise OK.
>
> -- PMM
>