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

[Alistair Francis]

On Fri, Mar 2, 2018 at 9:31 AM, Peter Maydell <address@hidden> wrote:
> On 2 March 2018 at 17:06, Alistair Francis <address@hidden> wrote:
>
> Subject should say "core count" rather than "cluster size" ?

Yes, that was left over. I'll fix.

>
>> 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".

Fixed these.

>
>> 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.

Ok will fix. I'll send a V2 out straight away, as today is my last day
and it'd be great if I could get this done.

Alistair

>
> thanks
> -- PMM