Re: [Qemu-devel] [PATCH 1/2] target-arm: make MPIDR a property

[Peter Maydell]

On 24 February 2014 22:14, Rob Herring <address@hidden> wrote:
> From: Rob Herring <address@hidden>
> MPIDR register is a machine configurable option and current kernels require
> the value to match with DT cpu reg properties. So add a property for MPIDR
> value and allow platforms to override.
>
> ARM_FEATURE_MPIDR is not used here because it is set too late.
>
> Signed-off-by: Rob Herring <address@hidden>
> ---
>  target-arm/cpu-qom.h |  1 +
>  target-arm/cpu.c     |  8 ++++++++
>  target-arm/helper.c  | 18 +++++++++++-------
>  3 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
> index afbd422..27a44a0 100644
> --- a/target-arm/cpu-qom.h
> +++ b/target-arm/cpu-qom.h
> @@ -113,6 +113,7 @@ typedef struct ARMCPU {
>       * prefix means a constant register.
>       */
>      uint32_t midr;
> +    uint32_t mpidr;
>      uint32_t reset_fpsid;
>      uint32_t mvfr0;
>      uint32_t mvfr1;
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 45ad7f0..5ac150e 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -249,6 +249,9 @@ static Property arm_cpu_reset_cbar_property =
>  static Property arm_cpu_reset_hivecs_property =
>              DEFINE_PROP_BOOL("reset-hivecs", ARMCPU, reset_hivecs, false);
>
> +static Property arm_cpu_mpidr_property =
> +            DEFINE_PROP_UINT32("mpidr", ARMCPU, mpidr, 0);
> +
>  static void arm_cpu_post_init(Object *obj)
>  {
>      ARMCPU *cpu = ARM_CPU(obj);
> @@ -262,6 +265,11 @@ static void arm_cpu_post_init(Object *obj)
>          qdev_property_add_static(DEVICE(obj), &arm_cpu_reset_hivecs_property,
>                                   &error_abort);
>      }
> +
> +    if (arm_feature(&cpu->env, ARM_FEATURE_V7MP)) {
> +        qdev_property_add_static(DEVICE(obj), &arm_cpu_mpidr_property,
> +                                 &error_abort);
> +    }

This fails to work for 11mpcore, which doesn't have FEATURE_V7MP
but does have FEATURE_MPIDR.

> index ca5b000..2590a05 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1442,6 +1442,11 @@ static int mpidr_read(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  {
>      CPUState *cs = CPU(arm_env_get_cpu(env));
>      uint32_t mpidr = cs->cpu_index;
> +
> +    if (ri->resetvalue) {
> +      *value = ri->resetvalue;
> +      return 0;
> +    }

This looks weird, because it's overriding the CPU index. (Also, you have
access to the CPU object here, you might as well just say "if (cpu->mpidr)"
rather than feeding it in through the ri->resetvalue when we don't actually
care about it at reset.)

Should the property actually be "cluster number" or something
similar? This is how the hardware does it, there are CLUSTERID
signals to set bits [11:8] of the MPIDR for A9/A15; A57 has
CLUSTERIDAFF1 and CLUSTERIDAFF2.

It might be easier to leave as a single uint32_t property, but I'm
pretty sure we should be ORing it in with the CPU index and
bit 31.

>      /* We don't support setting cluster ID ([8..11])
>       * so these bits always RAZ.
>       */
> @@ -1457,12 +1462,6 @@ static int mpidr_read(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>      return 0;
>  }
>
> -static const ARMCPRegInfo mpidr_cp_reginfo[] = {
> -    { .name = "MPIDR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 5,
> -      .access = PL1_R, .readfn = mpidr_read, .type = ARM_CP_NO_MIGRATE },
> -    REGINFO_SENTINEL
> -};
> -
>  static int par64_read(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t 
> *value)
>  {
>      *value = ((uint64_t)env->cp15.c7_par_hi << 32) | env->cp15.c7_par;
> @@ -1836,7 +1835,12 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>      }
>
>      if (arm_feature(env, ARM_FEATURE_MPIDR)) {
> -        define_arm_cp_regs(cpu, mpidr_cp_reginfo);
> +        ARMCPRegInfo mpidr_cp_reginfo = {
> +            .name = "MPIDR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 
> = 5,
> +            .access = PL1_R, .readfn = mpidr_read, .type = ARM_CP_NO_MIGRATE,
> +            .resetvalue = cpu->mpidr
> +        };
> +        define_one_arm_cp_reg(cpu, &mpidr_cp_reginfo);
>      }
>
>      if (arm_feature(env, ARM_FEATURE_AUXCR)) {

thanks
-- PMM