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

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

[Alistair Francis]

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 <alistair.francis@xilinx.com>
---

 target/arm/cpu64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 9743bdc8c3..aac1746efe 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -42,7 +42,7 @@ 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 */
+    /* Number of cores is in [25:24]; otherwise we RAZ */
     return (smp_cpus - 1) << 24;
 }
 #endif
-- 
2.14.1

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

[Alistair Francis]

On Thu, Mar 1, 2018 at 4:20 PM, Alistair Francis
<alistair.francis@xilinx.com> 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 <alistair.francis@xilinx.com>

Ah! This isn't actually what I want, I want something more like this (untested):

commit ce9d9795ebff42e1f742e7dc3786e52524807c65
Author: Alistair Francis <alistair@alistair23.me>
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 <alistair@alistair23.me>

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

I'll send a patch tomorrow after testing.

Alistair

> ---
>
>  target/arm/cpu64.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 9743bdc8c3..aac1746efe 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -42,7 +42,7 @@ 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 */
> +    /* Number of cores is in [25:24]; otherwise we RAZ */
>      return (smp_cpus - 1) << 24;
>  }
>  #endif
> --
> 2.14.1
>

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 <alistair23@gmail.com> wrote:
> On Thu, Mar 1, 2018 at 4:20 PM, Alistair Francis
> <alistair.francis@xilinx.com> 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 <alistair.francis@xilinx.com>
>
> Ah! This isn't actually what I want, I want something more like this (untested):
>
> commit ce9d9795ebff42e1f742e7dc3786e52524807c65
> Author: Alistair Francis <alistair@alistair23.me>
> 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 <alistair@alistair23.me>
>
> 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

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

[Peter Maydell]

On 2 March 2018 at 10:29, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 March 2018 at 04:34, Alistair Francis <alistair23@gmail.com> wrote:
>>     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.

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

...though I think "cluster size" isn't quite the right name --
if you have a big.LITTLE cluster with 2xA57 and 2xA53, there are
4 cores in the cluster but I think reading the TRM that the cores
will report 2 in the L2CTLR.

thanks
-- PMM

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

[Alistair Francis]

On Fri, Mar 2, 2018 at 2:35 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 March 2018 at 10:29, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 2 March 2018 at 04:34, Alistair Francis <alistair23@gmail.com> wrote:
>>>     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.
>
>> 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) ?
>
> ...though I think "cluster size" isn't quite the right name --
> if you have a big.LITTLE cluster with 2xA57 and 2xA53, there are
> 4 cores in the cluster but I think reading the TRM that the cores
> will report 2 in the L2CTLR.

Hmmm... I would have thought cluster size is correct enough. The only
other option I can think of is "processor size" or "core size"?

Alistair

>
> thanks
> -- PMM