Re: [Qemu-devel] [PATCH v1 1/1] target/arm: Fix the A53 L2CTLR typo

[Peter Maydell]

On 2 March 2018 at 04:34, Alistair Francis <address@hidden> wrote:
> On Thu, Mar 1, 2018 at 4:20 PM, Alistair Francis
> <address@hidden> wrote:
>> The cortex A53 TRM specifices that bits 24 and 25 of the L2CTLR register
>> specify the number of cores present and not the number of processors. We
>> have correctly been reporting the number of cores, so just fix the
>> comment to match the TRM.
>>
>> Signed-off-by: Alistair Francis <address@hidden>
>
> Ah! This isn't actually what I want, I want something more like this 
> (untested):
>
> commit ce9d9795ebff42e1f742e7dc3786e52524807c65
> Author: Alistair Francis <address@hidden>
> Date:   Thu Mar 1 20:19:23 2018 -0800
>
>     target/arm: Report the number of cores in the cluster
>
>     Previously we assumed that we only has a single cluster, which meant we
>     could get away with reporting smp_cpus to the guest. There are cases
>     where we have two clusters (Xilinx's ZynqMP is a good example) so
>     reporting the number of smp_cpus is incorrect. Instead count the cores
>     in the cluster.
>
>     Signed-off-by: Alistair Francis <address@hidden>
>
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 9743bdc..e7b1f3c 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -42,8 +42,24 @@ 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;
> +    CPUState *cpu;
> +    CPUState *cpu_prev = NULL;
> +    int num_cores = 0;
> +
> +    /* Figure out the number of cores in the cluster */
> +    for (cpu = first_cpu; cpu; cpu = CPU_NEXT(cpu)) {
> +        /* Only increase the core count if the CPU we are on is the same
> +         * class as the caller and the previous cpu.
> +         */
> +        if ((CPU_GET_CLASS(cpu) == CPU_GET_CLASS(cpu_prev)) &&
> +            (CPU_GET_CLASS(cpu) == CPU_GET_CLASS(CPU(env)))) {
> +            num_cores++;
> +        }
> +        cpu_prev = cpu;
> +    }
> +
> +    /* Number of cores is in [25:24]; otherwise we RAZ */
> +    return num_cores << 24;
>  }
>  #endif

This seems a bit ad-hoc (it will give the wrong answer if we have
two clusters both of the same kind of CPU, for instance).
Maybe it would be better to have a "cluster-size" property on
the CPU (with the default being number of cores in whole system) ?

thanks
-- PMM