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@xxxxxxxxxxxxx>
> > > > > ---
> > > > > 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?
>
>
>
> > >
> > >
> > > >
> > > > > + 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)
> as long it is not unrealized it belongs to the sysbus doesn't it?
>
> Regards,
> Pierre
>
