Re: [PATCH v12 1/7] s390x/cpu topology: Creating CPU topology device

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

On 12/6/22 10:31, Janis Schoetterl-Glausch wrote:
> On Tue, 2022-11-29 at 18:42 +0100, Pierre Morel wrote:
>> We will need a Topology device to transfer the topology
>> during migration and to implement machine reset.
>>
>> The device creation is fenced by s390_has_topology().
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   include/hw/s390x/cpu-topology.h    | 44 +++++++++++++++
>>   include/hw/s390x/s390-virtio-ccw.h |  1 +
>>   hw/s390x/cpu-topology.c            | 87 ++++++++++++++++++++++++++++++
>>   hw/s390x/s390-virtio-ccw.c         | 25 +++++++++
>>   hw/s390x/meson.build               |  1 +
>>   5 files changed, 158 insertions(+)
>>   create mode 100644 include/hw/s390x/cpu-topology.h
>>   create mode 100644 hw/s390x/cpu-topology.c
>>
> [...]
> 
>> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
>> index 9bba21a916..47ce0aa6fa 100644
>> --- a/include/hw/s390x/s390-virtio-ccw.h
>> +++ b/include/hw/s390x/s390-virtio-ccw.h
>> @@ -28,6 +28,7 @@ struct S390CcwMachineState {
>>       bool dea_key_wrap;
>>       bool pv;
>>       uint8_t loadparm[8];
>> +    DeviceState *topology;
> 
> Why is this a DeviceState, not S390Topology?
> It *has* to be a S390Topology, right? Since you cast it to one in patch 2.

Yes, currently it is the S390Topology.
The idea of Cedric was to have something more generic for future use.

> 
>>   };
>>   
>>   struct S390CcwMachineClass {
>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>> new file mode 100644
>> index 0000000000..bbf97cd66a
>> --- /dev/null
>> +++ b/hw/s390x/cpu-topology.c
>>
> [...]
>>   
>> +static DeviceState *s390_init_topology(MachineState *machine, Error **errp)
>> +{
>> +    DeviceState *dev;
>> +
>> +    dev = qdev_new(TYPE_S390_CPU_TOPOLOGY);
>> +
>> +    object_property_add_child(&machine->parent_obj,
>> +                              TYPE_S390_CPU_TOPOLOGY, OBJECT(dev));
> 
> Why set this property, and why on the machine parent?

For what I understood setting the num_cores and num_sockets as 
properties of the CPU Topology object allows to have them better 
integrated in the QEMU object framework.

The topology is added to the S390CcwmachineState, it is the parent of 
the machine.


> 
>> +    object_property_set_int(OBJECT(dev), "num-cores",
>> +                            machine->smp.cores * machine->smp.threads, errp);
>> +    object_property_set_int(OBJECT(dev), "num-sockets",
>> +                            machine->smp.sockets, errp);
>> +
>> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), errp);
> 
> I must admit that I haven't fully grokked qemu's memory management yet.
> Is the topology devices now owned by the sysbus?

Yes it is so we see it on the qtree with its properties.


> If so, is it fine to have a pointer to it S390CcwMachineState?

Why not?
It seems logical to me that the sysbus belong to the virtual machine.
But sometime the way of QEMU are not very transparent for me :)
so I can be wrong.

Regards,
Pierre

>> +
>> +    return dev;
>> +}
>> +
> [...]

-- 
Pierre Morel
IBM Lab Boeblingen