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

[Janis Schoetterl-Glausch]

 * On Wed, 2022-12-07 at 11:00 +0100, Pierre Morel wrote:
> 
> 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
> > > > > > 
[...]

> > > > > > > + 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?

The properties ref belongs to the property, if the property were removed,
it would be unref'ed. There is no extra ref for the pointer in 
S390CcwMachineState.
I'm coming from a clean code perspective, I don't think we'd run into this 
problem in practice.
> 
> Regards,
> Pierre
> 
> > 
> > > as long it is not unrealized it belongs to the sysbus doesn't it?
> > > 
> > > Regards,
> > > Pierre
> > > 
> > 
>