Re: [PATCH v10 1/9] s390x/cpu topology: core_id sets s390x CPU topology

[Pierre Morel <pmorel@linux.ibm.com>]

On 10/25/22 21:58, Janis Schoetterl-Glausch wrote:
> On Wed, 2022-10-12 at 18:20 +0200, Pierre Morel wrote:
>> In the S390x CPU topology the core_id specifies the CPU address
>> and the position of the core withing the topology.
>>
>> Let's build the topology based on the core_id.
>> s390x/cpu topology: core_id sets s390x CPU topology
>>
>> In the S390x CPU topology the core_id specifies the CPU address
>> and the position of the cpu withing the topology.
>>
>> Let's build the topology based on the core_id.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   include/hw/s390x/cpu-topology.h |  45 +++++++++++
>>   hw/s390x/cpu-topology.c         | 132 ++++++++++++++++++++++++++++++++
>>   hw/s390x/s390-virtio-ccw.c      |  21 +++++
>>   hw/s390x/meson.build            |   1 +
>>   4 files changed, 199 insertions(+)
>>   create mode 100644 include/hw/s390x/cpu-topology.h
>>   create mode 100644 hw/s390x/cpu-topology.c
>>
> [...]
> 
>> +/**
>> + * s390_topology_realize:
>> + * @dev: the device state
>> + * @errp: the error pointer (not used)
>> + *
>> + * During realize the machine CPU topology is initialized with the
>> + * QEMU -smp parameters.
>> + * The maximum count of CPU TLE in the all Topology can not be greater
>> + * than the maximum CPUs.
>> + */
>> +static void s390_topology_realize(DeviceState *dev, Error **errp)
>> +{
>> +    MachineState *ms = MACHINE(qdev_get_machine());
>> +    S390Topology *topo = S390_CPU_TOPOLOGY(dev);
>> +
>> +    topo->cpus = ms->smp.cores * ms->smp.threads;
> 
> Currently threads are not supported, effectively increasing the number of cpus,
> so this is currently correct. Once the machine version limits the threads to 1,
> it is also correct. However, once we support multiple threads, this becomes incorrect.
> I wonder if it's ok from a backward compatibility point of view to modify the smp values
> by doing cores *= threads, threads = 1 for old machines.

Right, this will become incorrect with thread support.
What about having a dedicated function:

	topo->cpus = s390_get_cpus(ms);

This function will use the S390CcwMachineClass->max_thread introduced 
later to report the correct number of CPUs.


> Then you can just use the cores value and it is always correct.
> In any case, if you keep it as is, I'd like to see a comment here saying that this
> is correct only so long as we don't support threads.
>> +
>> +    topo->socket = g_new0(S390TopoContainer, ms->smp.sockets);
>> +    topo->tle = g_new0(S390TopoTLE, ms->smp.max_cpus);
>> +
>> +    topo->ms = ms;
>> +}
>> +
> [...]

-- 
Pierre Morel
IBM Lab Boeblingen