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

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

On 12/6/22 22:06, Janis Schoetterl-Glausch wrote:
> On Tue, 2022-12-06 at 15:35 +0100, Pierre Morel wrote:
>>
>> 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
>>>>>
> [...]
>>>>>>     
>>>>>> +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.
> 
> Oh, I was confused. &machine->parent_obj is just a cast of MachineState* to Object*.
> It's the very same object.
> And what is the reason to add the topology as child property?
> Just so it shows up in the qtree? Wouldn't it anyway under the sysbus?

Yes it would appear on the info qtree but not in the qom-tree


>>
>>
>>
>>>>
>>>>
>>>>>
>>>>>> +    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 ?
> 
> That's beside the point, if you transfer ownership, you have no control over when
> the deallocation happens.
> It's going to be fine in practice, but I don't think you should rely on it.
> I think you could just do sysbus_realize instead of ..._and_unref,
> but like I said, I haven't fully understood qemu memory management.
> (It would also leak in a sense, but since the machine exists forever that should be fine)

If I understand correctly:

- qdev_new adds a reference count to the new created object, dev.

- object_property_add_child adds a reference count to the child also 
here the new created device dev so the ref count of dev is 2 .

after the unref on dev, the ref count of dev get down to 1

then it seems OK. Did I miss something?

Regards,
Pierre

> 
>> as long it is not unrealized it belongs to the sysbus doesn't it?
>>
>> Regards,
>> Pierre
>>
> 

-- 
Pierre Morel
IBM Lab Boeblingen