Re: [PATCH v9 02/10] s390x/cpu topology: core_id sets s390x CPU topology

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

On 9/6/22 07:58, Nico Boehr wrote:
> Quoting Pierre Morel (2022-09-02 09:55:23)
> [...]
>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>> new file mode 100644
>> index 0000000000..a6ca006ec5
>> --- /dev/null
>> +++ b/hw/s390x/cpu-topology.c
> [...]
>> +void s390_topology_new_cpu(int core_id)
>> +{
> [...]
>> +    socket_id = core_id / topo->cores;
> 
> The comment below is essential for understanding all of this. Move it before this line.
> 

OK

>> +
>> +    bit = core_id;
>> +    origin = bit / 64;
>> +    bit %= 64;
>> +    bit = 63 - bit;
>> +
>> +    /*
>> +     * At the core level, each CPU is represented by a bit in a 64bit
>> +     * unsigned long. Set on plug and clear on unplug of a CPU.
> 
> cleared                                  ^
> 
> [...]
>> +     * In that case the origin field, representing the offset of the first CPU
>> +     * in the CPU container allows to represent up to the maximal number of
>> +     * CPU inside several CPU containers inside the socket container.
> 
> How about:
> "In that case the origin variable represents the offset of the first CPU in the
> CPU container. More than 64 CPUs per socket are represented in several CPU
> containers inside the socket container."

Yes, better, thanks I take it


> 
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index b5ca154e2f..15cefd104b 100644
> [...]
>> @@ -247,6 +248,12 @@ static void ccw_init(MachineState *machine)
>>       /* init memory + setup max page size. Required for the CPU model */
>>       s390_memory_init(machine->ram);
>>   
>> +    /* Adding the topology must be done before CPU intialization*/
> 
> space                                                              ^
> 

Yes thanks

regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen