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

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

On 12/6/22 14:35, Janis Schoetterl-Glausch wrote:
> On Tue, 2022-12-06 at 11:32 +0100, Pierre Morel wrote:
>>
>> 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.
> 
> But it still needs to be a S390Topology otherwise you cannot cast it to one, can you?

May be I did not understand correctly what Cedric wants.
For my part I agree with you I do not see the point to have something 
different than a S390Topology pointer.

Also doing that is more secure as we do not need cast... which reveals a 
bug I have in setup_stsi().... !!!!

Let's do that and see what Cedric says.

>>
>>>
>>>>    };
>>>>    
>>>>    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.
> 
> That I understand.
>>
>> The topology is added to the S390CcwmachineState, it is the parent of
>> the machine.
> 
> But why? And is it added to the S390CcwMachineState, or its parent?

it is added to the S390CcwMachineState.
We receive the MachineState as the "machine" parameter here and it is 
added to the "machine->parent_obj" which is the S390CcwMachineState.



>>
>>
>>>
>>>> +    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?
> 
> If it's owned by the sysbus and the object is not explicitly referenced
> for the pointer, it might be deallocated and then you'd have a dangling pointer.

Why would it be deallocated ?
as long it is not unrealized it belongs to the sysbus doesn't it?

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen