Re: [Qemu-devel] [PATCH v1 1/2] target/arm: Add a cluster size property

[Peter Maydell]

On 2 March 2018 at 17:06, Alistair Francis <address@hidden> wrote:

Subject should say "core count" rather than "cluster size" ?

> The cortex A53 TRM specifices that bits 24 and 25 of the L2CTLR register

"specifies"

> specify the number of cores present and not the number of processors. To

"the number of cores in the processor, not the total number of cores
in the system".

> report this correctly on machines with multiple CPU clusters (ARM's
> big.LITTLE or Xilinx's ZynqMP) we need to allow the machine to overwrite
> this value. To do this let's add an optional property.
>
> Signed-off-by: Alistair Francis <address@hidden>
> ---
>
>  target/arm/cpu.h   |  5 +++++
>  target/arm/cpu.c   |  1 +
>  target/arm/cpu64.c | 11 +++++++++--
>  3 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 8dd6b788df..3fa8fdad21 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -745,6 +745,11 @@ struct ARMCPU {
>      /* Uniprocessor system with MP extensions */
>      bool mp_is_up;
>
> +    /* Specify the number of cores in this CPU cluster. Used for the L2CTLR
> +     * register.
> +     */
> +    int32_t core_count;
> +
>      /* The instance init functions for implementation-specific subclasses
>       * set these fields to specify the implementation-dependent values of
>       * various constant registers and reset values of non-constant
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 6b77aaa445..7a17ba1418 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1765,6 +1765,7 @@ static Property arm_cpu_properties[] = {
>      DEFINE_PROP_UINT64("mp-affinity", ARMCPU,
>                          mp_affinity, ARM64_AFFINITY_INVALID),
>      DEFINE_PROP_INT32("node-id", ARMCPU, node_id, CPU_UNSET_NUMA_NODE_ID),
> +    DEFINE_PROP_INT32("core-count", ARMCPU, core_count, -1),
>      DEFINE_PROP_END_OF_LIST()
>  };
>
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 4228713b19..5e5ae44aeb 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -42,8 +42,15 @@ static inline void unset_feature(CPUARMState *env, int 
> feature)
>  #ifndef CONFIG_USER_ONLY
>  static uint64_t a57_a53_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>  {
> -    /* Number of processors is in [25:24]; otherwise we RAZ */
> -    return (smp_cpus - 1) << 24;
> +    ARMCPU *cpu = arm_env_get_cpu(env);
> +
> +    /* Number of cores is in [25:24]; otherwise we RAZ */
> +    if (cpu->core_count == -1) {
> +        /* No core_count specified, default to smp_cpus. */
> +        return (smp_cpus - 1) << 24;
> +    } else {
> +        return (cpu->core_count - 1) << 24;
> +    }
>  }
>  #endif

I think having arm_cpu_realizefn do
    if (cpu->core_count == -1) {
        cpu->core_count = smp_cpus;
    }

would be neater than doing that check when the register is read.

thanks
-- PMM