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

[Janis Schoetterl-Glausch]

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?
> 
> > 
> > >   };
> > >   
> > >   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?
> 
> 
> > 
> > > +    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.

> 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;
> > > +}
> > > +
> > [...]
>